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

fix for TLS 1.3 #1011

Closed
wants to merge 2 commits into from
Closed

Conversation

adrigonzo
Copy link

secure_renegotiate and reuse_sessions options are not supported by the OTP SSL module when earlier version of TLS are not being used. (i.e. when we specify only TSL1.3 version, without TLS1.2 or earlier versions).
It seems TLS1.2 or earlier must ALSO be specified for this to work, since it's not supported in TLS1.3. Hence, adding a check whether TLS1.3 is the ONLY version being used.

lib/plug/ssl.ex Outdated Show resolved Hide resolved
@adrigonzo
Copy link
Author

Please find similar changes required on Plug Cowboy elixir-plug/plug_cowboy#63

@josevalim
Copy link
Member

/cc @voltone

options
|> Keyword.put_new(:secure_renegotiate, true)
|> Keyword.put_new(:reuse_sessions, true)
if options[:versions] == [:"tlsv1.3"] do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it should check whether the options contain earlier versions instead, as I assume that if there will ever be TLSv1.4 it will not support these options as well.

Copy link
Contributor

@voltone voltone Jun 13, 2022

Choose a reason for hiding this comment

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

I agree, this should probably be something like:

Suggested change
if options[:versions] == [:"tlsv1.3"] do
if !Enum.any?([:tlsv1, :"tlsv1.1", :"tlsv1.2"], &(&1 in options[:versions])) do

(Or rather, swap the do and else parts, and remove the negation)

No point in doing a MapSet dance here, is there? This is typically checked just once on server startup...

Copy link
Member

Choose a reason for hiding this comment

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

Does :versions have to be set? I think that may not be the case.

Suggested change
if options[:versions] == [:"tlsv1.3"] do
versions = options[:versions]
if versions && not Enum.any?(...) do

WDYT?

Copy link
Contributor

@voltone voltone Jun 13, 2022

Choose a reason for hiding this comment

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

I was wondering the same thing. But if it's not set, then we don't know whether these options should be set or not. For that I think we'd have to pick up the default with :ssl.versions()[:supported]...

Suggested change
if options[:versions] == [:"tlsv1.3"] do
versions = Keyword.get(options, :versions, :ssl.versions()[:supported])
if not Enum.any?(...) do

Another thing: technically :ssl options are not a proplist, so not sure we're supposed to use Keyword on it?

@josevalim
Copy link
Member

Ping @adrigonzo! We only need to address @hauleth’s comment and we are good to go!

josevalim added a commit that referenced this pull request Jun 17, 2022
@josevalim josevalim closed this in 7b778c2 Jun 17, 2022
@adrigonzo
Copy link
Author

adrigonzo commented Jun 17, 2022

We haven't tested with any other version combinations. You're right though, if future versions come along, this would need another update. Will take a look at the option to revert the check for 1, 1.1 and 1.2. I think the :versions is always present, at least from what I've seen, but would need to double check.

oh, sorry, just realised you did another PR for that. Thank you!

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