Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add ALPN support to SslStream#4685

Closed
jubarat wants to merge 1 commit intodotnet:masterfrom
jubarat:sslstream_alpn
Closed

Add ALPN support to SslStream#4685
jubarat wants to merge 1 commit intodotnet:masterfrom
jubarat:sslstream_alpn

Conversation

@jubarat
Copy link

@jubarat jubarat commented Nov 26, 2015

Support for passing ALPN application protocol list to SChannel
and querying for the negotiated protocol post handshake on client
and server.

Support for passing ALPN application protocol list to SChannel
and querying for the negotiated protocol post handshake on client
and server.
@stephentoub
Copy link
Member

cc: @bartonjs, @vijaykota, @CIPop, @davidsh

@stephentoub
Copy link
Member

Copy link
Member

Choose a reason for hiding this comment

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

I assume this is just a temporary stub until the functionality is actually implemented on Unix? is there an issue tracking it?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. So do I assume correctly I just create a separate issue for Unix which could be implemented later ?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd implement it at the same time. If that's not possible, then yes, opening an issue to track the Unix implementation, and including a TODO comment here citing that issue number. Then there's a question of what the temporary behavior here should be. I've not yet seen how this is consumed, but often the right behavior is "throw new PlatformNotSupportedException();"... but it depends on the usage.

@davidsh
Copy link
Contributor

davidsh commented Nov 26, 2015

Thx for the contribution. I think there is already an open GitHub issue for ALPN support. But this PR will not be able to reviewed or merged until after the API review process is used.

@davidsh davidsh added enhancement Product code improvement that does NOT require public API changes/additions 0 - Backlog api-needs-work API needs work before it is approved, it is NOT ready for implementation needs more info The issue is not actionable yet, more details are needed and removed Needs extra testing before merge labels Nov 26, 2015
@davidsh davidsh added this to the 1.0.0-rtm milestone Nov 26, 2015
@davidsh
Copy link
Contributor

davidsh commented Nov 26, 2015

See #2928

@ghost
Copy link

ghost commented Nov 29, 2015

#2928 has up for grabs tag.

@davidsh
Copy link
Contributor

davidsh commented Nov 29, 2015

@jasonwilliams200OK Yes, #2928 is 'up for grabs'. As part of solving that, new methods/properties will need to be added to the public API surface for SslStream. We have a separate process for APi review so we'll want to use that first to discuss/refine the new APIs and we'll want to consider how this API change affects the Desktop SslStream class as well.

API Review Process:
https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

@jubarat
Copy link
Author

jubarat commented Nov 30, 2015

API Review ticket created: #4721

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api-needs-work API needs work before it is approved, it is NOT ready for implementation needs more info The issue is not actionable yet, more details are needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants