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.

Sign up for GitHub

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

Jump to bottom

"Certificate required" alert should not be mapped to "handshake failure" #6804

Closed
p-mongo opened this issue Jul 29, 2018 · 3 comments
Closed

"Certificate required" alert should not be mapped to "handshake failure" #6804

p-mongo opened this issue Jul 29, 2018 · 3 comments
Milestone
1.1.1

Comments

@p-mongo
Copy link

p-mongo commented Jul 29, 2018

42c28b6 added this line:

   case SSL_AD_CERTIFICATE_REQUIRED:
        return SSL_AD_HANDSHAKE_FAILURE;

And same for TLS.

My understanding is this causes servers to report "handshake failure" to clients when a client did not provide a certificate to the server and the server required a certificate to be provided. These are distinct conditions as far as I can tell, according to the description of "handshake failure" in https://github.com/tlswg/tls13-spec/blob/master/draft-ietf-tls-tls13.md. A handshake failure error is very generic and usually difficult/impossible to debug.

https://github.com/tlswg/tls13-spec/blob/master/draft-ietf-tls-tls13.md also specifies a "certificate_required" alert; it seems to me that this should be sent to clients when a certificate is required and not supplied.
@mattcaswell
Copy link
Member

Looks like a bug in tls13_alert_code which should have an exception for this TLSv1.3 specific alert.

@mattcaswell mattcaswell added this to the 1.1.1 milestone Jul 30, 2018
mattcaswell added a commit to mattcaswell/openssl that referenced this issue Jul 30, 2018
@mattcaswell
Fix some TLSv1.3 alert issues
8d30c48 
Ensure that the certificate required alert actually gets sent (and doesn't
get translated into handshake failure in TLSv1.3).

Ensure that proper reason codes are given for the new TLSv1.3 alerts.

Remove an out of date macro for TLS13_AD_END_OF_EARLY_DATA. This is a left
over from an earlier TLSv1.3 draft that is no longer used.

Fixes openssl#6804
@mattcaswell mattcaswell mentioned this issue Jul 30, 2018
Closed
@mattcaswell
Copy link
Member

Fix in #6809

@p-mongo
Copy link
Author

p-mongo commented Jul 30, 2018

Thank you for the quick fix. TLS 1.2 (https://www.ietf.org/rfc/rfc5246.txt) seems to not have anything like a "certificate required" alert, thus using TLS 1.3+ is desired for better error reporting. Good to know.

facebook-github-bot pushed a commit to facebook/sapling that referenced this issue Jul 30, 2020
@kulshrax @facebook-github-bot
tests: allow multiple curl error codes in test-lfs-server-https.t
d691e06 
Summary: When a TLS connection fails due to a missing client certificate, the `curl` command may fail with either code 35 or 56 depending on the TLS version used. With TLS v1.3, the error is explicitly reported as a missing client certificate, whereas in TLS v1.2, it is reported as a generic handshake failure. This is because TLS v1.3 defines an explicit [`certificate_required`](https://tools.ietf.org/html/rfc8446#section-4.4.2.4) alert, which is [not present](openssl/openssl#6804) in earlier TLS versions.

Reviewed By: krallin

Differential Revision: D22834527

fbshipit-source-id: a15d6a169d35ece6ed5a54b37b8ca9bbc506b3da
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.1
Development

No branches or pull requests
2 participants
@mattcaswell @p-mongo