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

support server ALPN on macOS #79434

Merged
merged 2 commits into from
Dec 13, 2022
Merged

support server ALPN on macOS #79434

merged 2 commits into from
Dec 13, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Dec 9, 2022

As noted in #27727, there is no publicly documented API how to do this with the API we use. However, Apple publish relevant code and that is most accurate documentation :) This is somewhat tacky but similar to what we for OpenSSL on Linux.
With that everything works just fine (tested 11.6 on x85 & M1 on real HW and 13.0.1 in VM) and macOS gets on par with Linux & Windows e.g. it opens possibility for HTTP/2 in Kestrel + gRPC for Mac developers.

Here is the long story:

Fundamentally, coreTLS fail to read and process ALPN extension from ClientHello. That is OK and easy to fix.
With small changes to existing TlsFrameHelper we can get the data in SslStream. Conveniently, there is option to break the handshake when ClientHello arrives. It was mostly for SNI according to docs but does not matter. We can get back to managed code and run SelectApplicationProtocol to make selection.

Now is the tricky part. We can use same functions to set the selected protocol but it does not work because coreTLS is unaware of client's ALPN and therefore ignores the provided data. To fix that I added header stub from coreTLS so we can dereference so far opaque SSLContextRef. While this is generally problematic this code did not changed several years and it is deprecated by Apple so it feels like it is unlikely to change ever. I put in extra sanity check to verify that the ALPN data written by public API goes to place where we expect it. If the layout ever change we should be able to detect it and avoid writing to wrong location.

fixes #27727

@wfurt wfurt requested review from stephentoub and rzikm December 9, 2022 06:11
@wfurt wfurt self-assigned this Dec 9, 2022
@ghost
Copy link

ghost commented Dec 9, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

As noted in #27727, there is no publicly documented API how to do this with the API we use. However, Apple publish relevant code and that is most accurate documentation :) This is somewhat tacky but similar to what we for OpenSSL on Linux.
With that everything works just fine (tested 11.6 on x85 & M1 on real HW and 13.0.1 in VM) and macOS gets on par with Linux & Windows e.g. it opens possibility for HTTP/2 in Kestrel + gRPC for Mac developers.

Here is the long story:

Fundamentally, coreTLS fail to read and process ALPN extension from ClientHello. That is OK and easy to fix.
With small changes to existing TlsFrameHelper we can get the data in SslStream. Conveniently, there is option to break the handshake when ClientHello arrives. It was mostly for SNI according to docs but does not matter. We can get back to managed code and run SelectApplicationProtocol to make selection.

Now is the tricky part. We can use same functions to set the selected protocol but it does not work because coreTLS is unaware of client's ALPN and therefore ignores the provided data. To fix that I added header stub from coreTLS so we can dereference so far opaque SSLContextRef. While this is generally problematic this code did not changed several years and it is deprecated by Apple so it feels like it is unlikely to change ever. I put in extra sanity check to verify that the ALPN data written by public API goes to place where we expect it. If the layout ever change we should be able to detect it and avoid writing to wrong location.

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security, os-mac-os-x

Milestone: -

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

The fact that we rely on layouts that are not guaranteed to be stable makes me slightly uncomfortable. Can we be reasonably sure this will not cause process crashes when something changes in coreTLS?

@wfurt
Copy link
Member Author

wfurt commented Dec 9, 2022

The fact that we rely on layouts that are not guaranteed to be stable makes me slightly uncomfortable. Can we be reasonably sure this will not cause process crashes when something changes in coreTLS?

I agree. This is why I added the

if (tls != NULL && tls->alpnOwnData.length == length + 1)

If the TLS layout change (at least before ALPN portion) the alpnOwnData would move a the condition would fail.
I don't know if there is good way how to verify the first pointer. That is a least just read and the field we need is at beginning of the struct.

@filipnavara
Copy link
Member

The fact that we rely on layouts that are not guaranteed to be stable makes me slightly uncomfortable.

Same concern here.

Is there any validation for the scenario that runs BOTH on osx-arm64 and osx-x64? I know the layout is the same; specifically asking only for the validation of the fact. How does this affect iOS and tvOS platforms and is the code path tested there?

Lastly, this may be worth an escape hatch (eg. environment variable?) that would disable the behavior with peeking into private structures. If Apple ever decides to change it (unlikely) then we should have a way to keep applications running without patching framework or forced updates.

@wfurt
Copy link
Member Author

wfurt commented Dec 9, 2022

I did test on M1 arm macBook @filipnavara and all tests pass e.g. the layout is identical. As far as iOS I don't really know. There are some conditional fragments in structs so it can be different. I would still expect the check I mentioned would fail and we would basically end up in current situation.
Running server is not typical case IMHO so we can perhaps disable this on mobile platforms.

@filipnavara
Copy link
Member

I did test on M1 arm macBook @filipnavara and all tests pass e.g. the layout is identical.

I know it is identical [now] :-) I'm just worried it may diverge in future, however unlikely, and it would not be caught by the CI.

Running server is not typical case IMHO so we can perhaps disable this on mobile platforms.

I'd be definitely in favour of that.

@wfurt
Copy link
Member Author

wfurt commented Dec 13, 2022

The RawApplicationProtocol is gated by isMacOS(). Without it, iOS (and others) will not parse the inbound ALPN and the new logic would be skipped.

@wfurt wfurt merged commit 2d76178 into dotnet:main Dec 13, 2022
@davidfowl
Copy link
Member

Does this have any diagnostics in case things fail? How would we debug this on customer machines if things go wrong?

@wfurt
Copy link
Member Author

wfurt commented Dec 14, 2022

If the guard I put in kicks in it would not select any ALPN e.g. it would behave as it does now. I can certainly put in more diagnostic. We could throw (PNSE?) if preferable to make it more visible. The expectation is that would see that coming during testing beta versions of new major updated as it would fail our existing tests.

Now the difficult part is case when the structure would change in a way we fail to detect. In that case we would write to native memory of coreTLS. That would likely result in some crypto errors visible as TLS failures or possibly in weird crashes with stack to coreTLS if happen to mangle some pointers.

@davidfowl
Copy link
Member

I'm more looking for some event source event that we can fire as a diagnostic to tell the customer to turn on (use dotnet trace and turn on these events to see why it might be failing) if it fails.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 13, 2023
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server side ALPN support for SslStream on Mac
5 participants