-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TLS #104
base: main
Are you sure you want to change the base?
TLS #104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the proposal!
I'd expect handshake to be represented as a state machine based on resources, e.g. something roughly like this:
resource server {
/// Takes in ciphertext streams as parameters
/// In WASI 0.3.0, this could return a `result<future<client-hello>>`
accept(input: input-stream, output: output-stream) -> result<incoming-client-hello>;
}
resource incoming-client-hello {
/// Returns a pollable, which is ready once the client hello has been received and ready to be processed
subscribe: func() -> pollable;
requested-protocol-versions: func() -> result<list<protocol-version>>;
requested-server-name: func() -> result<option<string>>;
requested-alpn-ids: func() -> result<list<alpn-id>>;
requested-cipher-suites: func() -> result<list<cipher-suite>>;
// TODO:
// requested-extensions
// requested-compression-methods
early-data: func() -> result<option<input-stream>>;
accept: static func(this: incoming-client-hello, response: outgoing-server-hello) -> result<incoming-client-exchange>;
}
resource outgoing-server-hello {
constructor(version: protocol-version, cipher: cipher-suite, identity: borrow<public-identity>);
set-alpn: func(id: alpn-id);
}
resource incoming-client-exchange {
/// Returns a pollable, which is ready once the client key exchange and optonal identity has been received and ready to be processed
subscribe: func() -> pollable;
/// Client identity, if specified
identity: func() -> result<option<public-identity>>
/// Returns streams, on which plaintext can be written to and read from
finish: static func(this: incoming-client-exchange) -> result<tuple<input-stream, output-stream>>;
}
I feel like this approach would compose well with existing WASI interfaces, be extensible and provide type safety without impacting performance. What do you think?
We could then wrap these APIs in higher level libraries (e.g. rustls
in Rust)
/// - 0x0304: TLSv1.3 | ||
/// | ||
/// TODO: Want to use regular WIT `enum` type, but then adding a new protocol is backwards incompatible. | ||
type protocol-version = u16; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's worth not using enums at expense of user experience here. At least without WIT-defined constants for these values I'd expect users of this interface to make (potentially dangerous) mistakes and/or simply get confused.
Also, is addition to enums really backwards-incompatible? At least on the binary level it seems that backwards-incompatible change would only be bumping case count from 255 to 256, which seems quite unlikely.
Is there perhaps a way to utilize @since
annotations to solve this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not that familiar with the binary level. But ignoring that, how would such future additions be presented to guests built against an older version of the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it depends on the runtime, for example, let's say a runtime tries to pass TLS 1.4 case to a component export, where the component only supports TLS 1.3 and lower. I'd expect such a call to be disallowed in the runtime, so the function would simply not run.
Similarly, if the runtime would need to return an unsupported version of an enum back to the component via it's import, I'd guess the runtime would have a choice to either handling that case gracefully in some application-specific way (e.g. having an other(string)
case in the enum) or simply causing a trap in the component.
Perhaps, if we're talking about a "list of requested TLS versions", the runtime could simply omit the unsupported cases, so if a client requested TLS 1.3 or TLS 1.4, the runtime could only give the component a choice of TLS 1.3, since TLS 1.4 is not supported by the component's interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I'd expect such a call to be disallowed in the runtime, so the function would simply not run.
- I'd guess the runtime would have a choice to either handling that (...) causing a trap in the component.
I don't want the guest's security to be limited by the interface version they're targeting. I want the underlying TLS implementation/protocols/cipher-suites/etc to be able to evolve faster than & independently from the API that the the application uses to talk to the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I would agree that for extensibility here to use integers rather than enums. At the component model level enums of different shapes can't "match", even if variants aren't used at runtime. That means that if TLS 1.4 was eventually added then a runtime supporting that wouldn't be able even instantiate components that imported the previous version of the interface that didn't have TLS 1.4.
Supporting something like this is required at the component model level and isn't there yet today, so I think that the selection of integers here is a good alternative in the meantime. Pursuing a sort of extensible enum is I think worth exploring but is probably best done in the abstract context of the component model rather than the specific context of this proposal.
wit/tls.wit
Outdated
record io-streams { | ||
public-input: input-stream, | ||
public-output: output-stream, | ||
private-output: output-stream, | ||
private-input: input-stream, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I'm misunderstanding the use case here, but I'd expect to e.g. accept a TCP connection using
Line 194 in 74ea247
accept: func() -> result<tuple<tcp-socket, input-stream, output-stream>, error-code>; |
and then pass the acquired input-stream
and output-stream
into something like:
resource server {
/// Takes in ciphertext streams as parameters and returns streams, on which plaintext can be written to and read from
accept(input: input-stream, output: output-stream) -> result<tuple<input-stream, output-stream>>;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I had initially. It would definitely be a simpler interface. However, WASI guests are not able to create arbitrary input/output-streams themselves. Consuming the input+output stream would effectively mean that the TLS interface can only be used directly on top of TCP sockets. In theory this sounds fine for an initial version, but is likely a non-starter for the .NET folks who need raw access to the TLS data, because their primary TLS abstraction has been designed that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate to help me understand the incompatibility with .NET?
I'm not too familiar with that ecosystem, but looking at the docs, the SslStream
takes an abstract Stream
as a parameter https://learn.microsoft.com/en-us/dotnet/api/system.net.security.sslstream.-ctor?view=net-8.0
Such an abstract Stream
, it seems, would need to merge both input-stream
and output-stream
into a single structure. Assuming .NET standard library supports WASI, following the linked examples:
This would return a client structure, which would embed the input and output stream, received from wasi:sockets
TcpClient client = listener.AcceptTcpClient();
client.GetStream()
would return a structure, which embeds both input and output stream
SslStream sslStream = new SslStream(
client.GetStream(), false);
Also, not entirely sure why we'd need to create arbitrary streams here.
wit/tls.wit
Outdated
resource server { | ||
constructor(suspend-at: server-suspension-points); | ||
streams: func() -> result<io-streams>; | ||
|
||
configure-alpn-ids: func(value: list<alpn-id>) -> result; | ||
configure-identities: func(value: list<borrow<private-identity>>) -> result; | ||
|
||
server-name: func() -> option<string>; | ||
alpn-id: func() -> option<alpn-id>; | ||
protocol-version: func() -> option<protocol-version>; | ||
client-identity: func() -> option<public-identity>; | ||
server-identity: func() -> option<private-identity>; | ||
|
||
suspend: func() -> result<server-suspension, suspend-error>; | ||
resume: func() -> result; | ||
subscribe: func() -> pollable; | ||
} | ||
|
||
flags server-suspension-points { | ||
/// When the server received the initial message from the client. | ||
client-hello, | ||
|
||
/// When the server received the client's certificate. | ||
verify-client-identity, | ||
|
||
/// When the initial handshake was successful. | ||
accepted, | ||
} | ||
|
||
resource server-suspension { | ||
at: func() -> server-suspension-points; | ||
|
||
/// Only for client-hello: | ||
// TODO: requested-protocol-versions: func() -> result<list<protocol-version>>; | ||
// TODO: requested-server-name: func() -> result<option<string>>; | ||
// TODO: requested-alpn-ids: func() -> result<list<alpn-id>>; | ||
|
||
/// Only for verify-client-identity: | ||
// TODO: unverified-identity: func() -> result<public-identity>; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we flip the abstraction here and make each suspension point a resource in itself? (see suggestion in review comment)
@rvolosatovs Thanks for your feedback. I've been trying out different variations of your suggestions. Aside from some details, I think that overall they indeed yield a better solution. Remarks:
|
I was reading over this today and overall this looks quite good to me. While not necessarily the "perfect" interface we might have hoped for type-wise I think it's a good reflection of what we can do today with the component model and uses what we've got well. One other possible point worth comparing to (or just drawing inspiration from) is the I didn't happen to catch this before the latest refactoring, though, so I don't know how different the API would look like to support arbitrary I/O sources. |
The current version consumes a pair of streams and provides a pair of streams. The previous version didn't consume any stream, but instead provided two pairs of streams: /// The I/O streams that represent both sides of the transform.
///
/// The application side interacts with the cleartext "private" streams.
/// The network side interacts with the encrypted "public" streams.
///
/// A typical setup looks something like this:
///
/// ```text
/// : TCP Socket TLS Client/Server
/// +-----------------+ +---------------------------------------------+
/// | | splice | decryption | read
/// | `input-stream` | ========>> | `public-output` =========>> `private-input` | ======>> your
/// | | | | app
/// | | | | lives
/// | `output-stream` | <<======== | `public-input` <<========= `private-output` | <<====== here
/// | | splice | encryption | write
/// +-----------------+ +---------------------------------------------+
/// ```
///
/// The user of this interface is responsible for continually forwarding
/// data from the socket into the `public-output` stream and
/// data from the `public-input` into the socket.
///
/// # Caution
/// Because the guest acts as both the producer and the consumer for these
/// streams, do not use the `blocking_*` methods as that will deadlock yourself.
record io-streams {
public-input: input-stream,
public-output: output-stream,
private-output: output-stream,
private-input: input-stream,
} As noted in the |
Do you think it'd be too onerous to bring back such a construct? That feels like it would work quite well in terms of supporting for example in-memory tests and such which aren't required to be tied to I/O |
Yes, we definitely could. That being said, assuming a If we were to revert to returning 4 streams, we'd preferably also need to introduce some kind of automatic forwarding mechanism to get back to the same level of ergonomics as the current draft. I flipped my opinion to the current design, because in the end it seems like the least-worst solution. |
Those are good points yeah, and if |
Thinking about it some more, Edit: looks like we responded at the same time :) |
/// The combination of a private key with its public certificate(s). | ||
/// The private key data can not be exported. | ||
resource private-identity { | ||
/// TODO: find a way to "preopen" these private-identity resources, so that the sensitive private key data never has to flow through the guest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In lieu of support for value imports, do we need something more complex than something like
import: static func(key-id: string) -> result<private-identity>;
or potentially
import: static func(key-id: string, x509-chain-id: string) -> result<private-identity>;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like that could probably work, yes.
My idea so far was to add an identity-store
type, that can be used for both these "preopens" and also to access root certificates:
root-authorities: func() -> identity-store; // The TLS implementation's trust anchors.
user-identities: func() -> identity-store; // "preopens" per component instance
resource identity-store {
all: func() -> list<identity>;
// Optional utility methods:
find-by-hostname: func(hostname: string) -> list<identity>;
find-by-fingerprint: func(hash: string) -> option<identity>;
}
inspired by WebAssembly#104 Signed-off-by: Joel Dice <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the interface looks pretty good, thanks @badeend !
Not intended to be merged (anytime soon)