strip out options unsupported in TLS1.3 before listening on socket #313

adrigonzo · 2021-02-08T13:15:06Z

These four options to secure earlier TLS versions are actually unsupported by TLS1.3 and are sometimes added by default (e.g. in Plug and Plug Cowboy):

secure_renegotiate
reuse_sessions
next_protocols_advertised
alpn_preferred_protocols

When earlier versions aren't being enabled, this gives an error from OTP SSL module. Remove these options if TLS1.3 is the only enabled protocol, to get around this error.

adrigonzo · 2021-02-08T13:16:28Z

Fixes also submitted for Plug and Plug Cowboy:
elixir-plug/plug#1011
elixir-plug/plug_cowboy#63

essen · 2021-02-08T13:39:39Z

I don't know if this is something we want, but that's definitely not the right place, TLS related code should only be in ranch_ssl.

adrigonzo · 2021-02-08T16:23:55Z

I don't know if this is something we want, but that's definitely not the right place, TLS related code should only be in ranch_ssl.

I'll take a look into moving it then. thank you.

…ocket" This reverts commit dc8ba08.

…oved from acceptors)

adrigonzo · 2021-02-26T12:16:38Z

I don't know if this is something we want, but that's definitely not the right place, TLS related code should only be in ranch_ssl.

I've moved it to ranch_ssl now. This may be redundant if the application using ranch doesn't force-add those options. It's where I made the change originally to get TLS1.3 working in a web application using Plug and Plug Cowboy over Ranch.

michaelklishin · 2021-03-09T00:13:47Z

This is not pretty but I personally think it's the right thing to do. For example, we had to document these limitations for RabbitMQ but many users don't read any docs until things fail, often too late in the process (e.g. at deployment time).

Maria-12648430

Comments inline :)

Maria-12648430 · 2021-03-09T08:57:01Z

src/ranch_ssl.erl

 	ok.
+
+-spec strip_usupported_options(opts()) -> opts().
+strip_usupported_options(SocketOpts) ->


Typo? usupported --> unsupported?

indeed, thank you for spotting that. The downside of autocomplete once it was written wrong.

Maria-12648430 · 2021-03-09T09:05:06Z

src/ranch_ssl.erl

+
+-spec strip_usupported_options(opts()) -> opts().
+strip_usupported_options(SocketOpts) ->
+    case lists:keyfind(versions, 1, SocketOpts) of


I think this is only part of it, ie the versions key in the socket options is not the only place influencing it. According to the ssl docs here, if absent it can also be set by an environment option for the ssl application, and if absent there it defaults to all versions that are supported (which can probably be found out by examining the supported key in the return from ssl:versions()).

I don't know if you mean that other options could give a value of 'tlsv1.3' for a version number, not only {versions, ['tlsv1.3']}; however, just to clarify, we found that as long as other options are specified in addition to 'tlsv1.3', .e.g. {versions, ['tlsv1.2', 'tlsv1.3']}, then the SSL error does not occur.

We mean that the versions could be implicitly ['tlsv1.3'] even if you don't specify them at all in the socket options, namely if they are specified in the ssl application environment. Like, try starting your node with erl -ssl protocol_versions "['tlsv1.3']". If you later give other versions in the socket options, that overrides those environment settings, but if you leave them out, the ssl application environment protocol_versions is taken for versions. If that is not defined either, versions default to all supported versions, which is probably more than only ['tlsv1.3'].
So, I guess what I mean is, if you don't find versions in the socket options, see if there is a protocol_versions key in the application environment of ssl, and if that is set to ['tlsv1.3].

Maria-12648430 · 2021-03-09T09:05:36Z

src/ranch_ssl.erl

+        _ ->
+            SocketOpts
+    end;
+strip_usupported_options(SocketOpts) ->


This clause is pointless, it will never be reached.

I'll remove this. It was inherited from the code, which I'd moved from the acceptors file.

Maria-12648430 · 2021-03-09T09:14:56Z

src/ranch_ssl.erl

+            Intermediate1 = lists:keydelete(secure_renegotiate, 1, SocketOpts),
+            Intermediate2 = lists:keydelete(reuse_sessions, 1, Intermediate1),
+            Intermediate3 = lists:keydelete(next_protocols_advertised, 1, Intermediate2),
+            lists:keydelete(alpn_preferred_protocols, 1, Intermediate3);


Minor detail probably, but this is not exhaustive either. lists:keydelete only removes the first occurrence, but in reality the same option may be given multiple times, pointless as that may be (if it follows the practice used in gen_tcp, not 100% sure for ssl). To be on the safe side, you'll have to lists:filter/2 the socket options.

essen · 2021-03-09T09:50:54Z

Please do not silently drop the values, instead we should call logger:warning to inform the user that these settings were dropped.

/cc @lhoguin

…f unsupported options

adrigonzo · 2021-04-14T13:47:01Z

I hope I resolved the above suggestions:

checking versions and protocol_versions for ['tlsv1.3'] option.
checking for the 4 listed options using lists:filter
added logger:warning to alert system of the changes

The only thing I wasn't quite sure of was, can protocol_versions be present with the ['tlsv1.3'] setting, while the user passes in a different versions option, which supersedes protocol_versions? If that were so, I think the clause of the if statement may need to be changed to:

    if
        (Versions1 == {versions, ['tlsv1.3']}) or ((Versions1 == false) and (Versions2 == {protocol_versions, ['tlsv1.3']})) ->

Maria-12648430 · 2021-04-15T11:56:30Z

src/ranch_ssl.erl

+-spec strip_unsupported_options(opts()) -> opts().
+strip_unsupported_options(SocketOpts) ->
+    Versions1 = lists:keyfind(versions, 1, SocketOpts),
+    Versions2 = lists:keyfind(protocol_versions, 1, SocketOpts),


protocol_versions is not in SocketOpts but in the environment of the ssl application

application:get_env(ssl, protocol_versions)

If versions is given in SocketOpts, it overrides protocol_versions from the application environment.

Maria-12648430 · 2021-04-15T12:01:13Z

src/ranch_ssl.erl

+            if
+                NewSocketOpts /= SocketOpts ->
+                    error_logger:warning_msg("~p~n dropping options unsupported by TLS1.3-only ssl sockets: " ++
+                    "secure_renegotiate, reuse_sessions, next_protocols_advertised and/or alpn_preferred_protocols from ~p~n", [?MODULE, SocketOpts])


You should not use error_logger directly here (or logger, for that matter), but ranch:log/3 with the value from the logger key in TransOpts (as given to ranch_ssl: listen).

Also, the message is at the same time very verbose and doesn't tell very much, ie which options were actually dropped.

Maria-12648430 · 2021-04-15T12:03:38Z

src/ranch_ssl.erl

+    if
+        (Versions1 == {versions, ['tlsv1.3']}) or (Versions2 == {protocol_versions, ['tlsv1.3']}) ->
+            NewSocketOpts = lists:filter(fun({X, _}) ->
+                    (X /= secure_renegotiate) and (X /= reuse_sessions) and (X /= next_protocols_advertised) and (X /= alpn_preferred_protocols);


If you want to do it this way (and I'm pretty sure that @essen will object 😅), you should use andalso. But it may be better to have individual clauses for each, so you could log what you found and dropped along the way.

It would be better to log what is dropped indeed.

Maria-12648430 · 2021-08-30T07:58:44Z

This PR seems to have dried up. I can take it on if you want?

adrigonzo · 2021-08-30T08:06:10Z

This PR seems to have dried up. I can take it on if you want?

Hi, I believe you had a PR se-apc#3 for this. It's probably in more correct format as I'm not an Erlang programmer. It's ok if you want to go ahead and merge it in. The fix is working for us on a local branch, so I've not had to go back and touch it again (if it ain't broke, etc.... )
Thanks for checking in!

essen · 2021-08-30T08:10:44Z

@Maria-12648430 Please take over the PR then. I think it would be good to release it in Ranch 2.1 along with the other ones.

Maria-12648430 · 2021-08-30T11:20:54Z

@Maria-12648430 Please take over the PR then. I think it would be good to release it in Ranch 2.1 along with the other ones.

@essen see #324

essen · 2021-08-31T12:48:00Z

Closed in favor of #324.

strip out options unsupported in TLS1.3 before listening on socket

dc8ba08

Adriana Gonzalez added 2 commits February 26, 2021 11:00

Revert "strip out options unsupported in TLS1.3 before listening on s…

4c8e58f

…ocket" This reverts commit dc8ba08.

strip out options unsupported in TLS1.3 before listening on socket (m…

8aef9e6

…oved from acceptors)

adrigonzo mentioned this pull request Feb 26, 2021

Issue/fix for tls1.3 se-apc/ranch#2

Merged

Maria-12648430 suggested changes Mar 9, 2021

View reviewed changes

Adriana Gonzalez added 2 commits April 14, 2021 14:33

check for protocol_versions in Socket Options, drop all occurrences o…

67d26b4

…f unsupported options

log warning for dropping TLS1.3-only unsupported Socket Options

40c11ff

Maria-12648430 suggested changes Apr 15, 2021

View reviewed changes

adrigonzo mentioned this pull request Jun 2, 2021

Disallow unsupported TLSv1.3 socket options se-apc/ranch#3

Open

Maria-12648430 mentioned this pull request Aug 30, 2021

Disallow unsupported options for TLSv1.3 #324

Closed

essen closed this Aug 31, 2021

Maria-12648430 mentioned this pull request Sep 8, 2022

Add selectable alphabets (standard vs urlsafe) to base64 erlang/otp#6280

Merged

Uh oh!

strip out options unsupported in TLS1.3 before listening on socket #313

strip out options unsupported in TLS1.3 before listening on socket #313

Uh oh!

Conversation

adrigonzo commented Feb 8, 2021

Uh oh!

adrigonzo commented Feb 8, 2021

Uh oh!

essen commented Feb 8, 2021

Uh oh!

adrigonzo commented Feb 8, 2021

Uh oh!

adrigonzo commented Feb 26, 2021

Uh oh!

michaelklishin commented Mar 9, 2021

Uh oh!

Maria-12648430 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juhlig Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maria-12648430 Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

essen commented Mar 9, 2021

Uh oh!

adrigonzo commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maria-12648430 commented Aug 30, 2021

Uh oh!

adrigonzo commented Aug 30, 2021

Uh oh!

essen commented Aug 30, 2021

Uh oh!

Maria-12648430 commented Aug 30, 2021

Uh oh!

essen commented Aug 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

juhlig Mar 9, 2021 •

edited

Loading

Maria-12648430 Mar 9, 2021 •

edited

Loading

adrigonzo commented Apr 14, 2021 •

edited

Loading