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

Enable TLSv1.3 ciphers #4725

Closed
wants to merge 2 commits into from
Closed

Enable TLSv1.3 ciphers #4725

wants to merge 2 commits into from

Conversation

evilaliv3
Copy link
Contributor

This pull request adds support for a set of TLSv1.3 ciphers and is actually necessary in order to enable support for TLSv1.3.

@codecov-io
Copy link

Codecov Report

Merging #4725 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #4725   +/-   ##
========================================
  Coverage    81.61%   81.61%           
========================================
  Files           49       49           
  Lines         3416     3416           
  Branches       391      391           
========================================
  Hits          2788     2788           
  Misses         535      535           
  Partials        93       93

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d151b91...15d5370. Read the comment docs.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

Mozilla's preference list is a bit different, I think we should follow that. @emkll what do you think?

@evilaliv3
Copy link
Contributor Author

Thank you for your feedback @kushaldas. Please let me know which is your choice and i will make the proper adjustments.

Actually i just think that for a strange reason the order in that instructions are wrong.
Typically the order of ciphers is set in relation to most common implementations and their performance, but in this case it seems to me that the suggestion is just in the reverse order (from the less secure to the most secure).

@garrettr what do you think? is there anybody in @mozilla that we could involve in this evaluation and eventually notify this error?

@redshiftzero
Copy link
Contributor

so from the wiki page it looks like Mozilla is intentionally prioritizing TLS_AES_128_GCM_SHA256 over TLS_AES_256_GCM_SHA384 for performance reasons since both ciphersuites are considered secure ("The cipher suites are all strong and so we allow the client to choose, as they will know best if they have support for hardware-accelerated AES").

@evilaliv3 you're right that we would need to include the TLS ciphersuites in our configuration in order to support TLSv1.3 since we list our ciphersuites explicitly. However: I just checked and the version of openssl included in Ubuntu Xenial does not support TLSv1.3 (it's 1.0.2) - version 1.1.1 or later is required for TLSv1.3, so I think we might need to wait a bit for upstream before making this change. What we should probably do here is file an issue for awareness to make sure we update these ciphersuites when upstream (ubuntu) openssl supports TLSv1.3. What do y'all think?

@evilaliv3
Copy link
Contributor Author

evilaliv3 commented Sep 4, 2019

Thank you for your feedback @redshiftzero, its now clear.

Actually 128 is prioritized over 256 for the reasons you mention.
Then CHACHA is left as last option but typical implementations use SSL_OP_PRIORITIZE_CHACHA to prioritize CHACHA over AES if the client declares CHACHA as first option.

As for OpenSSL the version 1.1.1 is included in the current Ubuntu LTS (bionic); is there any particular reason your codebase is still based on Ubuntu Xenial?

@redshiftzero
Copy link
Contributor

nope, no particular reason for lack of bionic support, the reason is basically that we haven't got around to it yet and we only recently transitioned to Xenial (shortly before the Trusty EOL, check out #3204 for the full details of what the Xenial support and migration entailed).

At some point we will migrate instances onto the next LTS (unless we make a larger architectural change between now and the migration) though realistically it'll be a while before we make the move to Bionic - getting v3 tor onion service support out to production and removing all Python 2 code paths by the end of the year will be done first (mostly will be resolved in our next release but we do still have a Python 2.7 only CLI tool used by admins).

@evilaliv3
Copy link
Contributor Author

Clear, thank you @redshiftzero

@redshiftzero
Copy link
Contributor

hey @evilaliv3, i'm gonna close this one for now since we can't merge it for Xenial, but keep the branch around - when we implement #4768 (though it'll be a while 😇) you can refile again and we can bring it in :-) I filed #4769 for tracking

@evilaliv3
Copy link
Contributor Author

It make sense. Thank you @redshiftzero

If completing the migration to bionic will take time probaby:

  • we could evaluate to backport openssl to xenial
  • maybe you could just plan to skip bionic and prepare for 20.04 that will arrive next April?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants