Auth password config rework #3463
Conversation
- 'scram_iterations' moved to 'password' - There was an undocumented check which prevented SHA hashes from being specified when format was 'plain', it is removed now: - For consistency , as there was no such check for scram_iterations. - To be able to specify the defaults. - There is no default for 'hash' as this would add some lists:member checks to the code because of the current implementation. In the future we might want to rework 'cyrsasl:listmech' to avoid this problem, e.g. by pre-filtering sasl mechanisms for each host type.
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #3463 +/- ##
==========================================
+ Coverage 80.82% 80.84% +0.02%
==========================================
Files 415 415
Lines 32263 32255 -8
==========================================
+ Hits 26075 26077 +2
+ Misses 6188 6178 -10
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
small_tests_24 / small_tests / c7574bb small_tests_23 / small_tests / c7574bb dynamic_domains_mysql_redis_24 / mysql_redis / c7574bb dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / c7574bb dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / c7574bb dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / c7574bb ldap_mnesia_23 / ldap_mnesia / c7574bb ldap_mnesia_24 / ldap_mnesia / c7574bb internal_mnesia_24 / internal_mnesia / c7574bb pgsql_mnesia_23 / pgsql_mnesia / c7574bb pgsql_mnesia_24 / pgsql_mnesia / c7574bb mysql_redis_24 / mysql_redis / c7574bb elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / c7574bb mssql_mnesia_24 / odbc_mssql_mnesia / c7574bb riak_mnesia_24 / riak_mnesia / c7574bb |
| {scram_iterations, 64}. | ||
| {auth_password_opts, "format = \"scram\" | ||
| hash = [\"sha256\"] | ||
| scram_iterations = 64"}. |
There was a problem hiding this comment.
This reminds me, if you want to shave off a few more miliseconds in tests, you can set this to 2 or 3 or so, the fact that the iterations really work is thoroughly tested in the fast_scram repo, here in MIM we only need authentication to be correct, not necessarily safe 😄
| case mongoose_config:get_opt([{auth, HostType}, password]) of | ||
| #{format := scram, hash := ConfiguredSha} -> lists:member(Sha, ConfiguredSha); | ||
| #{format := _PlainOrScram} -> true |
There was a problem hiding this comment.
Nice, that's very nice, good optimisation against the common case, less lists:member/2 👏🏽
The goal of this PR is to rework the
auth.passwordconfig section to:hashoption if the password format isplainis removed as it was undocumented, inconsistent with no such check forscram_iterationsand prevented the defaults from being set. It seems enough to inform the user that theplainformat just ignores these options.hashoption is left without a default. Putting the default list of hashes there might lead to suboptimal code, aslists:memberwould be called on each call tolistmechfor each method. This can be changed when we rework thecyrsaslimplementation, e.g. by precomputing effective filtered SASL mechanisms per host type, but it falls out of scope of this PR.scram_iterationsoption, which is password-related, but was outside of this section.