You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/05/24 17:27:39 UTC
[GitHub] [couchdb-mochiweb] noahshaw11 opened a new pull request #5: Upgrade crypto functions to support OTP 23
noahshaw11 opened a new pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5
Upgrade crypto functions to support OTP 23 to eliminate deprecation warnings during building.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva edited a comment on pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva edited a comment on pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#issuecomment-849774446
@noahshaw11 I think I might have figured one error for Erlang 20. It looks like `ssl:cipher_suites/2,3` was added later in the Erlang 20 release cycle (20.3) but Travis CI is running Erlang 20.0. So I think for 20 we can probably move back to using 21 as the earliest release where new ssl is available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640799032
##########
File path: rebar.config
##########
@@ -2,6 +2,8 @@
{erl_opts, [debug_info,
{platform_define, "^R15", 'gen_tcp_r15b_workaround'},
{platform_define, "^(R14|R15|R16B-)", 'crypto_compatibility'},
+ {platform_define, "^(17|18|19|20|21)", new_crypto_unavailable},
Review comment:
Notice that we're declaring that new_crypto_unavailable is true for 14, 15, 16 here! So that means it will try to use the new functions on the really old libraries
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640101898
##########
File path: src/mochiweb_socket.erl
##########
@@ -44,11 +56,14 @@ filter_broken_cipher_suites(Ciphers) ->
Ciphers
end.
+%% Filter old tuple style cipher suites and new map style cipher suites
filter_unsecure_cipher_suites(Ciphers) ->
lists:filter(fun
({_,des_cbc,_}) -> false;
({_,_,md5}) -> false;
- (_) -> true
+ (_) -> true;
+ (#{key_exchange := des_cbc}) -> false;
+ (#{mac := md5}) -> false
Review comment:
These lines should be moved above the (_) "catchall" case. Otherwise the (_) will match first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] noahshaw11 commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r639083456
##########
File path: src/mochiweb_socket.erl
##########
@@ -30,7 +30,7 @@ listen(Ssl, Port, Opts, SslOpts) ->
end.
add_unbroken_ciphers_default(Opts) ->
- Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
+ Default = filter_unsecure_cipher_suites(ssl:cipher_suites(default, 'tlsv1.3')),
Review comment:
Need to add backward compatibility here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r643246804
##########
File path: src/mochiweb_socket.erl
##########
@@ -29,11 +29,37 @@ listen(Ssl, Port, Opts, SslOpts) ->
gen_tcp:listen(Port, Opts)
end.
+-ifdef(new_crypto_unavailable).
add_unbroken_ciphers_default(Opts) ->
Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
[{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+%% Filter old map style cipher suites
+filter_unsecure_cipher_suites(Ciphers) ->
+ lists:filter(fun
+ ({_,des_cbc,_}) -> false;
+ ({_,_,md5}) -> false;
+ (_) -> true
+ end,
+ Ciphers).
+
+-else.
+add_unbroken_ciphers_default(Opts) ->
+ %% add_safe_protocol_versions/1 must have been called to ensure a {versions, _} tuple is present
+ Versions = proplists:get_value(versions, Opts),
+ CipherSuites = lists:append([ssl:cipher_suites(all, Version) || Version <- Versions]),
+ Default = filter_unsecure_cipher_suites(CipherSuites),
+ Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
+ [{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+
+%% Filter new map style cipher suites
+filter_unsecure_cipher_suites(Ciphers) ->
+ ssl:filter_cipher_suites(Ciphers, [{key_exchange, fun(des_cbc) -> false; (_) -> true end},
+ {mac, fun(md5) -> false; (_) -> true end}]).
+
Review comment:
Minor nit, let's improve indentation a bit. Something like perhaps?:
```erlang
ssl:filter_cipher_suites(Ciphers, [
{key_exchange, fun(des_cbc) -> false; (_) -> true end},
{mac, fun(md5) -> false; (_) -> true end}
]).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] noahshaw11 closed pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
noahshaw11 closed pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640043407
##########
File path: src/mochiweb_socket.erl
##########
@@ -29,11 +29,22 @@ listen(Ssl, Port, Opts, SslOpts) ->
gen_tcp:listen(Port, Opts)
end.
+-ifdef(new_ssl_unavailable).
add_unbroken_ciphers_default(Opts) ->
Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
[{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+-else.
+add_unbroken_ciphers_default(Opts) ->
+ CipherSuitesListMap = ssl:cipher_suites(default, 'tlsv1.3'),
+ CipherSuitesList = [{KeyExchange, Cipher, Mac, Prf} || #{cipher := Cipher, key_exchange := KeyExchange, mac := Mac, prf := Prf} <- CipherSuitesListMap],
+ Default = filter_unsecure_cipher_suites(CipherSuitesList),
+ Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
+ [{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
Review comment:
It looks like technically ssl:connect/2,3.. would accept tuples as cipher suites even until OTP 25 at least.
https://github.com/erlang/otp/blob/master/lib/ssl/src/ssl.erl#L2580-L2582
But it's still probably better to switch to maps on versions of erlang which support maps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r641878643
##########
File path: src/mochiweb_session.erl
##########
@@ -150,13 +151,33 @@ decrypt_data(<<IV:16/binary, Crypt/binary>>, Key) ->
crypto:block_decrypt(aes_cfb128, Key, IV, Crypt).
-spec gen_key(iolist(), iolist()) -> binary().
-gen_key(ExpirationTime, ServerKey)->
+gen_key(ExpirationTime, ServerKey) ->
crypto:hmac(md5, ServerKey, [ExpirationTime]).
-spec gen_hmac(iolist(), binary(), iolist(), binary()) -> binary().
gen_hmac(ExpirationTime, Data, SessionKey, Key) ->
crypto:hmac(sha, Key, [ExpirationTime, Data, SessionKey]).
+-else. % new crypto available (OTP 22+)
Review comment:
It is 22 technically but we started with 23
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] noahshaw11 commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640818810
##########
File path: rebar.config
##########
@@ -2,6 +2,8 @@
{erl_opts, [debug_info,
{platform_define, "^R15", 'gen_tcp_r15b_workaround'},
{platform_define, "^(R14|R15|R16B-)", 'crypto_compatibility'},
+ {platform_define, "^(17|18|19|20|21)", new_crypto_unavailable},
Review comment:
Is 14, 15, 16, not covered by the `-ifdef(crypto_compatibility)` statement since it is in an `if/else` block?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] noahshaw11 commented on pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#issuecomment-850457992
Receiving another error with OTP 23:
```
Uncaught error in rebar_core: {'EXIT',
{undef,
[{rebar_utils,get_cwd,[],[]},
{rebar_config,new,0,[]},
{rebar,init_config,1,[]},
{rebar,run,1,[]},
{rebar,main,1,[]},
{escript,run,2,
[{file,"escript.erl"},{line,758}]},
{escript,start,1,
[{file,"escript.erl"},{line,277}]},
{init,start_em,1,[]}]}}
=ERROR REPORT==== 27-May-2021::23:50:25.081921 ===
beam/beam_load.c(1624): Error loading module rebar_utils:
please re-compile this module with an 23 compiler (old-style fun with indices: 3/6)
=ERROR REPORT==== 27-May-2021::23:50:25.081948 ===
Loading of /home/travis/build/apache/couchdb-mochiweb/rebar/rebar/ebin/rebar_utils.beam failed: badfile
=ERROR REPORT==== 27-May-2021::23:50:25.097954 ===
beam/beam_load.c(1624): Error loading module rebar_utils:
please re-compile this module with an 23 compiler (old-style fun with indices: 3/6)
=ERROR REPORT==== 27-May-2021::23:50:25.097987 ===
Loading of /home/travis/build/apache/couchdb-mochiweb/rebar/rebar/ebin/rebar_utils.beam failed: badfile
escript: exception error: undefined function rebar_utils:delayed_halt/1
in function escript:run/2 (escript.erl, line 758)
in call from escript:start/1 (escript.erl, line 277)
in call from init:start_em/1
in call from init:do_boot/3
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640036832
##########
File path: src/mochiweb_socket.erl
##########
@@ -29,11 +29,22 @@ listen(Ssl, Port, Opts, SslOpts) ->
gen_tcp:listen(Port, Opts)
end.
+-ifdef(new_ssl_unavailable).
add_unbroken_ciphers_default(Opts) ->
Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
[{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+-else.
+add_unbroken_ciphers_default(Opts) ->
+ CipherSuitesListMap = ssl:cipher_suites(default, 'tlsv1.3'),
+ CipherSuitesList = [{KeyExchange, Cipher, Mac, Prf} || #{cipher := Cipher, key_exchange := KeyExchange, mac := Mac, prf := Prf} <- CipherSuitesListMap],
+ Default = filter_unsecure_cipher_suites(CipherSuitesList),
+ Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
+ [{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
Review comment:
This is a bit tricky. I wonder if connect method on 23 (where ` ssl:cipher_suites/0` returns tuples) would accept ciphers in a tuple format for the connect method, I think it should or mochiweb would break. They just don't document it I think. On 24 it might not. So if we always translate them to tuples, it might not work.
I wonder if we can filter both tuples and maps with a few changes and pass in maps on Erlang 20+ and handle tuples on Erlang < 20.
I think for `case proplists:get_value(ssl_app, ssl:versions()) of "5.3" ++ _ ...` we don't have to worry about as ssh app version 5.3 only accepts tuples. Erlang 20 has ssl app version as 8.2.x.x.
`filter_unsecure_cipher_suites(Ciphers) ->` could be handled by adding extra clauses to the fun:
```
(#{key_exchange := des_cbc}) -> false;
(#{mac := md5}) -> false;
```
With a comment saying we are filtering both old style tuple cipher suites and new map ones
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] noahshaw11 commented on pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#issuecomment-848089491
PR made upstream: https://github.com/mochi/mochiweb/pull/231
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#issuecomment-849990624
Saw another failure in 22.0
```
::{function_clause,[{dtls_v1,corresponding_tls_version,
['tlsv1.3'],
[{file,"dtls_v1.erl"},{line,55}]},
{dtls_v1,all_suites,1,[{file,"dtls_v1.erl"},{line,41}]},
{ssl,cipher_suites,2,[{file,"ssl.erl"},{line,987}]},
```
I verified it with my own build of 22.0:
```
17> Vers = proplists:get_value(available, ssl:versions()).
['tlsv1.3','tlsv1.2','tlsv1.1',tlsv1,sslv3]
18> catch ssl:cipher_suites(default, 'tlsv1.3').
{'EXIT',{function_clause,[{ssl_cipher,suites,
['tlsv1.3'],
[{file,"ssl_cipher.erl"},{line,276}]},
{ssl,cipher_suites,2,[{file,"ssl.erl"},{line,987}]},
{erl_eval,do_apply,6,[{file,"erl_eval.erl"},{line,684}]},
{erl_eval,expr,5,[{file,"erl_eval.erl"},{line,437}]},
{shell,exprs,7,[{file,"shell.erl"},{line,686}]},
{shell,eval_exprs,7,[{file,"shell.erl"},{line,642}]},
{shell,eval_loop,3,[{file,"shell.erl"},{line,627}]}]}}
```
And noticed in later versions of 22 like 22.3.4.16 it works...:
```
3> _ = ssl:cipher_suites(default, 'tlsv1.3'), ok.
ok
```
So maybe we'd just end up using the same `new_crypto_unavailable` macro instead of the `new_ssl_available` as both seem to point to 23 being the cutoff release which handle new cipher and new crypto properly.
It seems that map-based cipher suites are supported in 20-21, and higher versions of 22, but in 22.0 the `ssl:cipher_suites(all | default, Version)` would sometimes fail. To avoid doing fine-grained special cases we might just make 23 the smallest version and call it a day.
But that means we would like to add 23 to the list of tests and hopefully soon enough Travis CI would add 24 to some repos too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] noahshaw11 commented on pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
noahshaw11 commented on pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#issuecomment-854918687
PR was merged upstream: https://github.com/mochi/mochiweb/pull/231
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r639106718
##########
File path: src/mochiweb_socket.erl
##########
@@ -30,7 +30,7 @@ listen(Ssl, Port, Opts, SslOpts) ->
end.
add_unbroken_ciphers_default(Opts) ->
- Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
+ Default = filter_unsecure_cipher_suites(ssl:cipher_suites(default, 'tlsv1.3')),
Review comment:
Also note that the format of the result is different too:
```
16> rp(ssl:cipher_suites(default, 'tlsv1')).
[#{cipher => aes_256_cbc,key_exchange => ecdhe_ecdsa,
mac => sha,prf => default_prf},
#{cipher => aes_256_cbc,key_exchange => ecdhe_rsa,mac => sha,
prf => default_prf},
```
```
19> rp(ssl:cipher_suites()).
[{any,aes_256_gcm,aead,sha384},
{any,aes_128_gcm,aead,sha256},
{any,chacha20_poly1305,aead,sha256},
{any,aes_128_ccm,aead,sha256},
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#issuecomment-849774446
@noahshaw11 I think I might have figured one error for Erlang 20. It looks like `ssl:cipher_suites/2,3` was added later in the Erlang 20 release cycle (20.3) but Travis CI is running Erlang 20. So I think for 20 we can probably move back to using 21 as the earliest release where new ssl is available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640036832
##########
File path: src/mochiweb_socket.erl
##########
@@ -29,11 +29,22 @@ listen(Ssl, Port, Opts, SslOpts) ->
gen_tcp:listen(Port, Opts)
end.
+-ifdef(new_ssl_unavailable).
add_unbroken_ciphers_default(Opts) ->
Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
[{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+-else.
+add_unbroken_ciphers_default(Opts) ->
+ CipherSuitesListMap = ssl:cipher_suites(default, 'tlsv1.3'),
+ CipherSuitesList = [{KeyExchange, Cipher, Mac, Prf} || #{cipher := Cipher, key_exchange := KeyExchange, mac := Mac, prf := Prf} <- CipherSuitesListMap],
+ Default = filter_unsecure_cipher_suites(CipherSuitesList),
+ Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
+ [{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
Review comment:
This is a bit tricky. I wonder if connect method on 23 (where ` ssl:cipher_suites/0` return tuples) would accept ciphers in a tuple format for the connect method. On 24 it might not. So if we always translate them to tuples, it might not work.
I wonder if we can filter both tuples and maps with a few changes and pass in maps on Erlang 20+ and handle tuples on Erlang < 20.
I think for `case proplists:get_value(ssl_app, ssl:versions()) of "5.3" ++ _ ...` we don't have to worry about as ssh app version 5.3 only accepts tuples. Erlang 20 has ssl app version as 8.2.x.x.
`filter_unsecure_cipher_suites(Ciphers) ->` could be handled by adding extra clauses to the fun:
```
(#{key_exchange := des_cbc}) -> false;
(#{mac := md5}) -> false;
```
With a comment saying we are filtering both old style tuple cipher suites and new map ones
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#issuecomment-850503025
Another hurdle! The problem is rebar was compiled on a too old version of Erlang OTP. Usually those should work with future versions but at some point they would break like you saw. To test locally you might do:
```
rm ./rebar ; rebar get-deps && rebar compile && rebar eunit
```
And then install your own rebar bootstrapped from scratch from https://github.com/rebar/rebar.git then run `./bootstrap` and link to or copy the rebar in your $PATH somewhere
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r643219360
##########
File path: src/mochiweb_socket.erl
##########
@@ -29,11 +29,43 @@ listen(Ssl, Port, Opts, SslOpts) ->
gen_tcp:listen(Port, Opts)
end.
+-ifdef(new_crypto_unavailable).
add_unbroken_ciphers_default(Opts) ->
Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
[{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+%% Filter old map style cipher suites
+filter_unsecure_cipher_suites(Ciphers) ->
+ lists:filter(fun
+ ({_,des_cbc,_}) -> false;
+ ({_,_,md5}) -> false;
+ (_) -> true
+ end,
+ Ciphers).
+
+-else.
+add_unbroken_ciphers_default(Opts) ->
+ %% add_safe_protocol_versions/1 must have been called to ensure a {versions, _} tuple is present
+ Versions = proplists:get_value(versions, Opts),
+ CipherSuites = lists:append([ssl:cipher_suites(all, Version) || Version <- Versions]),
+ Default = filter_unsecure_cipher_suites(CipherSuites),
+ Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
+ [{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+
+%% Filter new map style cipher suites
+filter_unsecure_cipher_suites(Ciphers) ->
+ lists:filter(fun
+ ({_,des_cbc,_}) -> false;
+ ({_,_,md5}) -> false;
Review comment:
Probably don't need to check for tuples here if this block is included in the `-else. ...` section with the new new crypto.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640025615
##########
File path: rebar.config
##########
@@ -2,6 +2,8 @@
{erl_opts, [debug_info,
{platform_define, "^R15", 'gen_tcp_r15b_workaround'},
{platform_define, "^(R14|R15|R16B-)", 'crypto_compatibility'},
+ {platform_define, "^(17|18|19|20|21)", new_crypto_unavailable},
+ {platform_define, "^(R14|R15|R16|17|18|19|20)", new_ssl_unavailable},
Review comment:
Technically I think cipher suites results as maps became available in 20.
```
% asdf shell erlang 20.3.8.26
% erl
Erlang/OTP 20 [erts-9.3.3.15] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:10] [hipe] [kernel-poll:false]
Eshell V9.3.3.15 (abort with ^G)
> ssl:cipher_suites(default, 'tlsv1.1').
[#{cipher => aes_256_cbc,key_exchange => ecdhe_ecdsa,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640024067
##########
File path: src/mochiweb_socket.erl
##########
@@ -29,11 +29,22 @@ listen(Ssl, Port, Opts, SslOpts) ->
gen_tcp:listen(Port, Opts)
end.
+-ifdef(new_ssl_unavailable).
add_unbroken_ciphers_default(Opts) ->
Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
[{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+-else.
+add_unbroken_ciphers_default(Opts) ->
+ CipherSuitesListMap = ssl:cipher_suites(default, 'tlsv1.3'),
Review comment:
This might not always be true. `tlsv1.3` might not be available. Notice in Erlang 22 on MacOS BigSur:
```
% asdf shell erlang 21.3.8.15
% erl
Erlang/OTP 21 [erts-10.3.5.11] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [hipe]
Eshell V10.3.5.11 (abort with ^G)
1> ssl:versions().
[{ssl_app,"9.2"},
{supported,['tlsv1.2','tlsv1.1',tlsv1]},
{supported_dtls,['dtlsv1.2',dtlsv1]},
{available,['tlsv1.2','tlsv1.1',tlsv1,sslv3]},
{available_dtls,['dtlsv1.2',dtlsv1]}]
2> ssl:cipher_suites(default, 'tlsv1.3').
** exception error: no function clause matching
ssl_cipher:suites('tlsv1.3') (ssl_cipher.erl, line 253)
in function ssl:cipher_suites/2 (ssl.erl, line 909)
```
In this case I think we'd want to look at the `versions` options after it had been filtered with `add_safe_protocol_version/1` os maybe switch the order of:
```
Opts1 = add_unbroken_ciphers_default(Opts ++ SslOpts),
Opts2 = add_safe_protocol_versions(Opts1)
```
to
```
Opts1 = add_safe_protocol_versions(Opts)
Opts2 = add_unbroken_ciphers_default(Opts1 ++ SslOpts),
```
Then query `ssl:cipher_suites(default, Version)` for each Version and gather the suites in a list that way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640065782
##########
File path: src/mochiweb_socket.erl
##########
@@ -29,11 +29,22 @@ listen(Ssl, Port, Opts, SslOpts) ->
gen_tcp:listen(Port, Opts)
end.
+-ifdef(new_ssl_unavailable).
add_unbroken_ciphers_default(Opts) ->
Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
[{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+-else.
+add_unbroken_ciphers_default(Opts) ->
+ [{version, Versions}] = Opts,
Review comment:
`Opts` would be a proplist and may have multiple items so would have to do:
```
Versions = proplists:get(versions, Opts)
```
And with a comment that we rely on having called `add_safe_protocol_versions/1` which ensures we'll have a `{versions, _}` tuple present.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640884555
##########
File path: rebar.config
##########
@@ -2,6 +2,8 @@
{erl_opts, [debug_info,
{platform_define, "^R15", 'gen_tcp_r15b_workaround'},
{platform_define, "^(R14|R15|R16B-)", 'crypto_compatibility'},
+ {platform_define, "^(17|18|19|20|21)", new_crypto_unavailable},
Review comment:
Good point!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r639087259
##########
File path: src/mochiweb_socket.erl
##########
@@ -30,7 +30,7 @@ listen(Ssl, Port, Opts, SslOpts) ->
end.
add_unbroken_ciphers_default(Opts) ->
- Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
+ Default = filter_unsecure_cipher_suites(ssl:cipher_suites(default, 'tlsv1.3')),
Review comment:
Also don't forget to add some documentation or references about the API differences. That is, what exactly changed and why the line is needed. I took a quick look at https://erlang.org/doc/man/ssl.html#cipher_suites-2 but couldn't immediately tell.
To make comparisons easier with older versions, there is https://erldocs.com/. There you can pick an older version of docs and it would be more obvious what has changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb-mochiweb] nickva commented on a change in pull request #5: Upgrade crypto functions to support OTP 23
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #5:
URL: https://github.com/apache/couchdb-mochiweb/pull/5#discussion_r640066761
##########
File path: src/mochiweb_socket.erl
##########
@@ -29,11 +29,22 @@ listen(Ssl, Port, Opts, SslOpts) ->
gen_tcp:listen(Port, Opts)
end.
+-ifdef(new_ssl_unavailable).
add_unbroken_ciphers_default(Opts) ->
Default = filter_unsecure_cipher_suites(ssl:cipher_suites()),
Ciphers = filter_broken_cipher_suites(proplists:get_value(ciphers, Opts, Default)),
[{ciphers, Ciphers} | proplists:delete(ciphers, Opts)].
+-else.
+add_unbroken_ciphers_default(Opts) ->
+ [{version, Versions}] = Opts,
+ CipherSuites = [ssl:cipher_suites(default, Version) || Version <- Versions],
Review comment:
This will be a list of lists I think. You can calls lists:append/1 on it I think:
```
2> lists:append([[1,2],[3,4]]).
[1,2,3,4]
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org