steffengy / schannel-rs

Schannel API-bindings for rust (provides an interface for native SSL/TLS using windows APIs)
MIT License
49 stars 51 forks source link

Interface changes to support RFC 5077 TLS session tickets #66

Closed cyang1 closed 4 years ago

cyang1 commented 4 years ago

It looks like SChannel natively advertises support for the SessionTicket extension in its ClientHello, but scopes its stored tickets in some way to the CredHandle used to build the session.

As currently written, tls_stream::Builder consumes the given SchannelCred, so it's not possible to use this crate in a way that allows for RFC 5077. https://github.com/steffengy/schannel-rs/blob/3becee06f5117108ce342f230abed6239417e07b/src/tls_stream.rs#L119-L122

Would it be possible to update the interface to allow SchannelCred to be shared/refcounted? At a glance through the curl's usage of schannel, it looks like the Windows APIs that use CredHandle objects are most likely thread-safe.

Ref: https://github.com/curl/curl/blob/0e06c1637b30800d41636dcd02f5becf3f6664c1/lib/vtls/schannel.c#L794

steffengy commented 4 years ago

We can't change tls_stream::Builder::connect without it being a BC-breaking change.

Without looking into it too much, whats required for this usecase @cyang1? Is it sufficient to be able to get the SchannelCred of a TlsStream after the connection is shut down or do you really need to be able to use the cred for multiple connections? E.g. adding something like fn take_cred(self) -> SchannelCred { ... }

cyang1 commented 4 years ago

Good question about the usecase. I think something like fn take_cred(self) -> SchannelCred { ... } would make this possible, but wouldn't necessarily be ideal, since, IIUC, it should be possible to resume multiple connections simultaneously.

IMO consuming the TlsStream would also be harder to use. If we take rust-native-tls as an example, after the TlsConnector creates a TlsStream, it has no other interactions with it, so an implementation with this API would probably have to share some Arc<Mutex<_>> between the TlsConnector and TlsStream and have TlsStream::shutdown stash the credentials into that shared collection.

Agree that it would be a BC-breaking change to change tls_stream::Builder::connect though, so definitely open to discussions of alternatives.

steffengy commented 4 years ago

Some other options I think are reasonable:

cyang1 commented 4 years ago

Either of those sounds good to me. From my read of how things are structured right now, using a AsRef<SchannelCred> sounds more involved to implement, as it would require parameterizing TlsStream with the lifetime.

Would you like me to submit a PR?

steffengy commented 4 years ago

Would be great, if you could submit a PR for the Arc variant with maybe some tests of how the API would be used with session tickets (a testcase passing the same handle multiple times preferably in multiple threads to see if every case works as expected).