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

New OpenSSL APIs for TLSv1.3 ciphersuites Support #4007

Closed
masaori335 opened this issue Jul 25, 2018 · 11 comments
Closed

New OpenSSL APIs for TLSv1.3 ciphersuites Support #4007

masaori335 opened this issue Jul 25, 2018 · 11 comments
Labels

Comments

@masaori335
Copy link
Contributor

OpenSSL has split configuration of TLSv1.3 ciphers from older ciphers since 1.1.1-pre3. ATS needs to use this new API for TLSv1.3 ciphers.

New APIs for TLSv1.3 ciphersuites

 int SSL_CTX_set_ciphersuites(SSL_CTX *ctx, const char *str);
 int SSL_set_ciphersuites(SSL *s, const char *str);

Details in https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_cipher_list.html
Commit of OpenSSL: openssl/openssl@f865b08

@masaori335 masaori335 added the TLS label Jul 25, 2018
@masaori335
Copy link
Contributor Author

IMO, we need a new config in records.config for TLSv1.3 cipher suites instead of mixing old and new ciphersuites in proxy.config.ssl.server.cipher_suite.
Currently I'm thinking below name, but it looks unclear. Anybody has good idea?

  • proxy.config.ssl.server.cipher_suite : for TLSv1.2 and below - pass to SSL_CTX_set_cipher_list() (current config)
  • proxy.config.tls.server.cipher_suites : for TLSv1.3 (and above) - pass to SSL_CTX_set_ciphersuites()

@zizhong
Copy link
Member

zizhong commented Jul 25, 2018

maybeproxy.config.ssl.server.cipher_suites_1_3?

@masaori335
Copy link
Contributor Author

@zizhong It looks TLSv1.3 specific. What will we do when new TLS version like TLSv1.4 or TLSv2 is published?

@masaori335
Copy link
Contributor Author

masaori335 commented Jul 25, 2018

Also, the name should be consistent with a config for (EC)DHE groups. ( #3604 )

@zizhong
Copy link
Member

zizhong commented Jul 25, 2018

@masaori335 good point. proxy.config.ssl.tls1_3_plus.server.cipher_suites?
Naming can never be perfect. The name of the new OpenSSL API is also pretty confusing to me.

@zwoop
Copy link
Contributor

zwoop commented Jul 28, 2018

Doh. Couldn't we just parse out the TLS v1.3 ciphers? I believe they are all prefixed with TLS13_, right? Presumably, future TLS versions will do the same, e.g. prefix with TLS14_ etc. It seems pretty unfortunate to have to introduce multiple configuration files here. What we likely need though is an option to enable / disable TLS v1.3 support, like we have for previous TLS and SSL versions.

@masaori335
Copy link
Contributor Author

masaori335 commented Jul 28, 2018

The prefix is removed. Now TLS v1.3 ciphers suite names are below.

TLS_AES_128_GCM_SHA256
TLS_AES_256_GCM_SHA384
TLS_CHACHA20_POLY1305_SHA256
TLS_AES_128_CCM_SHA256
TLS_AES_128_CCM_8_SHA256

Update: - are also changed to _. ( e.g. TLS13-AES-128-GCM-SHA256 -> TLS_AES_128_GCM_SHA256 )

@masaori335
Copy link
Contributor Author

We can parse, but mixing old and new ciphersuites looks messy. If we do that, proxy.config.ssl.server.cipher_suite like below means TLS_AES_256_GCM_SHA384 is the highest priority for TLSv1.3 and ECDHE-ECDSA-AES256-SHA384 is that for TLSv1.2. ( If I understand correctly )

ECDHE-ECDSA-AES256-SHA384:TLS_AES_256_GCM_SHA384:ECDHE-ECDSA-AES256-SHA:TLS_AES_128_GCM_SHA256

@zwoop
Copy link
Contributor

zwoop commented Jul 28, 2018

What happens if you give the same argument to both OpenSSL APIs? Meaning, could we just have one option and feed that to both, and let it figure things out?

I’m kinda baffled about this, there must be some reason why they separated the two cipher suites.

Also, what are the implications for BoringSSL support now? Do they add this new API too?

@masaori335
Copy link
Contributor Author

What happens if you give the same argument to both OpenSSL APIs? Meaning, could we just have one option and feed that to both, and let it figure things out?

I'll check the behavior.

I’m kinda baffled about this, there must be some reason why they separated the two cipher suites.

Before the split, when a server has TLSv1.3 enabled but no TLSv1.3 ciphersuites, all handshake immediately fail include TLSv1.2. It looks like they want to avoid that. Also the change of ciphersuite concept looks another motivation.

This new API is introduced by this commit.(openssl/openssl@f865b08).
More details are in openss/openssl PR5359.

Also, what are the implications for BoringSSL support now? Do they add this new API too?

BoringSSL doesn't have this API. They hard coded TLSv1.3 ciphers :)

// TLS 1.3 ciphers do not participate in this mechanism and instead have a
// built-in preference order. Functions to set cipher lists do not affect TLS
// 1.3, and functions to query the cipher list do not include TLS 1.3
// ciphers.

https://boringssl.googlesource.com/boringssl/+/master/include/openssl/ssl.h#1379
openss/openssl PR5359 (comment)

@masaori335
Copy link
Contributor Author

What happens if you give the same argument to both OpenSSL APIs? Meaning, could we just have one option and feed that to both, and let it figure things out?

I'll check the behavior.

Giving old ciphersuites to the new API returns error.
Even if it works, it will introduce the problem which is solved by separating the APIs. I mean when people upgrade to OpenSSL v1.1.1 (TLSv1.3 enabled) and use same records.config settings without TLSv1.3 ciphers, they will get trouble.

@zwoop zwoop added this to 8.0.0 backports in 8.x releases Aug 9, 2018
@zwoop zwoop moved this from 8.0.0 backports to Open Issues in 8.x releases Aug 9, 2018
@zwoop zwoop removed this from Open Issues in 8.x releases Oct 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants