search

steffengy / schannel-rs

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

ALPN support #49

Closed Eroc33 closed 4 years ago

Eroc33 commented 6 years ago

First pass at supporting ALPN, this should work for both client and server, but some cleanup is still needed, especially given how poor the docs on MSDN are for info on ALPN in schannel.

Eroc33 commented 5 years ago

@steffengy This should be working for all versions of windows which support alpn now. I'm not sure what I should really be doing on versions of windows that don't support alpn though. Currently it will effectively ignore anything passed into request_application_protocols and always return None from get_negotiated_application_protocol.

Also it seems like the appveyor tests are failing because for some reason alpn isn't enabled on them? I'm not sure what windows server version they're using but from their docs it seems like it should be 2012 R2, but might actually be 2012 (I did some testing with appveyor setup on another repo, and if I add logging code to report the NT version when the test fails it shows 6.2 which is 2012). Not sure whats going on there? But I can confirm tests pass on a fresh 2012 R2 Vm on azure.

steffengy commented 5 years ago

Appveyor seems to be using the Visual Studio 2015 image, which Server 2012 R2. You might want to try running tests with Visual Studio 2017 (Server 2016).

So you say schannel doesn't fail/return an error when you pass ALPNs to it, but it doesn't support them? This "error state" then can be detected by calling get_negotiated_application_protocol and checking if that matches what was requested? I guess that's fine, since the application/library using it then can decide if it can recover that. Maybe we want an option that validates that automatically if not configured otherwise, so checks of that are opt-out insteadof opt-in manually.

Eroc33 commented 5 years ago

Actually I wrong in was assuming that the appveyor behaviour was what happens on older versions of windows. I just tested and it will return SEC_E_UNSUPPORTED_FUNCTION so get_negotiated_application_protocol will return that error.

As far as checking if negotiation succeeded automatically that should be doable. I'm not entirely sure where would be best to check this, but I known it needs to be done when the state is State::Streaming, so possibly when switching to that state in initialize? Although I'm not sure how useful that would be as most endpoints using ALPN probably want to know which of a given set of protocols were selected.

I still really don't know why the test won't pass on appveyor. I have no idea what is different there to any other machine I have tried it on.

steffengy commented 5 years ago

Yeah, good point so we can probably leave that to the caller for now. And it returns the error if it's not supported, so the caller has all information he needs.

Have you tried with configuring Visual Studio 2017 as an image?

Eroc33 commented 5 years ago

Yes. No matter which image is configured the test fails. It acts as if the handshake happens without ALPN being requested. I even tried adding a test which does ALPN over loopback to be sure the accepting socket is offering ALPN. (I'll probably add those tests to this PR even though they didn't help with appveyor as the existing test only tests ALPN on the client).

Eroc33 commented 5 years ago

Unfortunately I haven't really been able to figure out what's going on here since I can't reproduce it on any machine I have access to. As far as I can tell the test_loopback_alpn test shouldn't (silently) fail under "normal" installs of windows, so all I can really guess is that there's some weird configuration on the appveyor test runners.

steffengy commented 5 years ago

@Eroc33 Have you tried looking into it on appveyor iself with the newest image? I usually did that using e.g. https://www.appveyor.com/docs/how-to/rdp-to-build-worker/ , which will atleast help to track down what's going on.

Eroc33 commented 5 years ago

I wasn't aware that was possible, but I'll see if that helps me figure out what's going on when I have a bit more time to look into this.

kzys commented 5 years ago

Hello @Eroc33 and @steffengy,

I could repro the issue on Amazon EC2's Windows instance, but couldn't figure out the cause. Oddly adding "http/1.0" somehow fixes the issue.

https://github.com/kzys/schannel-rs/pull/1/commits/9f60934f21039006ecb7330814bb117be05d7025

cyang1 commented 4 years ago

I'd like to pick this back up if possible. I believe the reason that this diff is currently failing is because alpn_list isn't exactly correct. I've added a test and cleaned that up a bit locally, and everything appears to work on my branch. Should I make a new PR?

steffengy commented 4 years ago

@cyang1 I think it's unlikely this PR will be picked up, so feel free to open a new one.