Skip to content
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

ALPN support #49

Closed
wants to merge 9 commits into from
Closed

ALPN support #49

wants to merge 9 commits into from

Conversation

Eroc33
Copy link

@Eroc33 Eroc33 commented May 11, 2018

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.

unsafe {
let mut client_protos: sspi::SecPkgContext_ApplicationProtocol = mem::zeroed();
let client_protos_ptr = &mut client_protos as *mut sspi::SecPkgContext_ApplicationProtocol;
assert_eq!(
Copy link
Author

@Eroc33 Eroc33 May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assert will need to be replaced with proper error handling, but I'm not sure what the possible errors are here, and how we should deal with them as MSDN doesn't really give much info on that.

@Eroc33 Eroc33 changed the title WIP: ALPN support ALPN support May 24, 2018
@Eroc33
Copy link
Author

Eroc33 commented Oct 30, 2018

@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
Copy link
Owner

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
Copy link
Author

Eroc33 commented Oct 30, 2018

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
Copy link
Owner

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
Copy link
Author

Eroc33 commented Oct 30, 2018

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
Copy link
Author

Eroc33 commented Dec 9, 2018

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
Copy link
Owner

@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
Copy link
Author

Eroc33 commented Dec 10, 2018

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
Copy link

kzys commented Mar 19, 2019

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.

kzys@9f60934

@cyang1
Copy link

cyang1 commented Mar 3, 2020

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
Copy link
Owner

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

@steffengy steffengy closed this Mar 5, 2020
@cyang1 cyang1 mentioned this pull request Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants