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

NPN / ALPN extensions callbacks don't allow handshake failure #188

Closed
Scottmitch opened this Issue Oct 21, 2014 · 13 comments

Comments

Projects
None yet
2 participants
@Scottmitch

Scottmitch commented Oct 21, 2014

The interface for the ALPN/NPN protocol selection callbacks and protocol selection notification callbacks do not allow for the handshake to fail. It is requested that the return value support for these methods be expanded to allow the handshake to fail. This may be desirable (and explicitly called out in the ALPN specification) in the event there are no common protocols found during the selection process, or if the select protocol is not acceptable.

For example:
ALPN RFC Section 3.2 specifies a new fatal alert definition no_application_protocol(120) which can be used. This is currently not defined in openssl.

The NPN specification is not as explicit about the alert to be used in this case but it could just result in a generic handshake_failure alert (if the callbacks return the new failure value)?

@Scottmitch Scottmitch changed the title from NPN / ALPN extensions no common protocols found to NPN / ALPN extensions callbacks allow handshake failure Oct 21, 2014

@Scottmitch

This comment has been minimized.

Scottmitch commented Oct 21, 2014

@richsalz - FYI this is to continue our discussion about the ALPN / NPN failure behavior. The OpenJDK based implementations (jetty-alpn and jetty-npn) have been updated to support controlling the behavior at the granularity of each handshake (so not just a compile time flag or system property). I think this is equivalent to adding an additional return value to the openssl callbacks?

In the case of ALPN we used the no_application_protocol(120) alert and in the case of NPN we used the handshake_failure(40) alert to fail the handshake (if callbacks indicated a failure was desired).

@Scottmitch Scottmitch changed the title from NPN / ALPN extensions callbacks allow handshake failure to NPN / ALPN extensions callbacks don't allow handshake failure Oct 22, 2014

@Scottmitch

This comment has been minimized.

Scottmitch commented Dec 17, 2014

Any updates or thoughts on this?

@Scottmitch

This comment has been minimized.

Scottmitch commented May 22, 2015

Now that HTTP/2 has been published as an RFC (and ALPN is already an RFC)....is there any chance of this getting re-prioritized?

@richsalz

This comment has been minimized.

Contributor

richsalz commented May 22, 2015

yeah, it probably should be. can you bring it up on openssl-dev?

@Scottmitch

This comment has been minimized.

Scottmitch commented May 26, 2015

@richsalz - Done. Haven't received feedback that the post went through yet though. Not sure if I'm required to join before posting or if the post has to be reviewed before it makes it to the archives.

@richsalz

This comment has been minimized.

Contributor

richsalz commented May 26, 2015

ah, yeah, you have to be a member of the list to post. i think nabble has a UI that lets you post

@Scottmitch

This comment has been minimized.

Scottmitch commented May 26, 2015

@richsalz - Join request sent.

@Scottmitch

This comment has been minimized.

Scottmitch commented May 28, 2015

@richsalz

This comment has been minimized.

Contributor

richsalz commented Jun 13, 2015

See also http://rt.openssl.org/Ticket/Display.html?id=3463 (user/pass guest/guest)

@Scottmitch

This comment has been minimized.

Scottmitch commented Aug 14, 2015

@richsalz - I checked out the patch and it looks good. The only thing from your point of view is whether you want to make the fatal alert failure behavior in s_server optional or not. This would be to preserve existing behavior, even though it goes against the spec.

What is the timeline on evaluating / merging / targeting this for a release?

@richsalz

This comment has been minimized.

Contributor

richsalz commented Feb 25, 2016

Finally closing this in 1.1; we'll send the alert. code to land in repo shortly.

@richsalz richsalz closed this Feb 25, 2016

@Scottmitch

This comment has been minimized.

Scottmitch commented Feb 25, 2016

@richsalz - Thanks for the update! So this will not land in a 1.0.2 release?

@richsalz

This comment has been minimized.

Contributor

richsalz commented Feb 25, 2016

nope. sorry.

asfgit pushed a commit to apache/tomcat that referenced this issue Apr 28, 2016

Work around a known issue in OpenSSL (openssl/openssl#188) that does …
…not permit the TLS handshake to be failed if the ALPN negotiation fails.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1741501 13f79535-47bb-0310-9956-ffa450edef68

asfgit pushed a commit to apache/tomcat85 that referenced this issue Apr 28, 2016

Work around a known issue in OpenSSL (openssl/openssl#188) that does …
…not permit the TLS handshake to be failed if the ALPN negotiation fails.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/tc8.5.x/trunk@1741503 13f79535-47bb-0310-9956-ffa450edef68

christianpaquin added a commit to christianpaquin/openssl that referenced this issue Dec 21, 2017

Squashed 'vendor/liboqs/' changes from 04d7eaa..ff3986a
ff3986a removed hard paths (openssl#193)
f62bb02 Enabled and documented building on ARM32 (Raspberry Pi). (openssl#179)
9dab6f6 Flags for configured algorithms generated in config.h (openssl#177)
2d5eb13 Covscan defect fix (openssl#189)
a5b239d Updated README (openssl#191)
d7a72e2 Add checks to verify length of input data for McBits (openssl#186)
cbee5ef Vsoftco issue160 (openssl#188)
581fbbb Initialize out-parameters to NULL (openssl#183)
0d8a354 Properly separate SIDH CLN16 from SIDH CLN16 compressed (openssl#181)
8bc8cd9 Added VisualStudio DLL build configurations (openssl#182)
fc522d6 Embed SIDH IQC REFERENCE parameters (openssl#180)
40ffb4e Updated Windows build (added sig, fixed warnings, 2017 update) (openssl#169)
a329060 Update README.md (openssl#178)
fcbd0f3 KEX memory benchmarks (openssl#171)
b9854b4 Arm compilation (openssl#170)
f3e24e1 Link to algorithm data sheets.
28cc05a Added datasheets for SIDH and Picnic. (openssl#166)

git-subtree-dir: vendor/liboqs
git-subtree-split: ff3986ab9585e521462fb28d24ed024328f609b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment