Moved couch_httpd_auth options to chttpd_auth main #3583
Conversation
jiahuili430
changed the title
Solved conflicts from "cherry-pick" 3.x commit with kdiff3 merge tool.
jiahuili430
changed the title
| <<"unauthorized">> -> | ||
| case config:get("couch_httpd_auth", "authentication_redirect", undefined) of | ||
| case chttpd_util:get_chttpd_auth_config( | ||
| "authentication_redirect", "/_utils/session.html") of |
There was a problem hiding this comment.
please use undefined as default here to avoid breaking users installations during upgrade.
There was a problem hiding this comment.
Actually never mind. We can update the default on main because it was not released yet. However for 3.x we should preserve existent behavior.
| <<"unauthorized">> -> | ||
| case config:get("couch_httpd_auth", "authentication_redirect", undefined) of | ||
| case chttpd_util:get_chttpd_auth_config( | ||
| "authentication_redirect", "/_utils/session.html") of |
There was a problem hiding this comment.
please use undefined as default here to avoid breaking users installations during upgrade.
There was a problem hiding this comment.
We can update the default on main because it was not released yet.
| ?l2b("-hashed-" ++ couch_util:to_hex(Hash) ++ "," ++ ?b2l(Salt)); | ||
| hash_admin_password("pbkdf2", ClearPassword) -> | ||
| Iterations = config:get("couch_httpd_auth", "iterations", "10000"), | ||
| Iterations = chttpd_util:get_chttpd_auth_config("iterations", "10"), |
There was a problem hiding this comment.
Please change the default value back to "10000"
There was a problem hiding this comment.
On the https://github.com/apache/couchdb/pull/3577/files#diff-47040d0b65ca2cdd2c1a8eea22187c860de02e51bd67ebf14c8287f5b56b26e8R82 the chttpd_util:get_chttpd_auth_config_integer is used. Is it intentional to not use it here?
There was a problem hiding this comment.
Iterations is used in the DerivedKey (line 51 & 54), I need to keep Iterations return String "10" instead of Integer 10.
Thanks for the review, @iilyak
| {ClearPassword, "pbkdf2"} -> | ||
| ok = validate_password(ClearPassword), | ||
| Iterations = list_to_integer(config:get("couch_httpd_auth", "iterations", "1000")), | ||
| Iterations = chttpd_util:get_chttpd_auth_config_integer( |
There was a problem hiding this comment.
Please change the default value back to 1000.
There was a problem hiding this comment.
Default work factor was deliberately set to 10 f726bc4. We can switch it to something even higher than 1000 now as replication is using the cookie auth by default, but it probably shouldn't be part of this commit?
If users updated the config value, that value would be in their local.ini or local.d/*.ini files, and it would still be used after this commit.
default.ini file is shipped and replaced by the couchdb packages and users should expect that file to be overwritten [1]. As I understand it, for a while it was the place to store default config settings. Later on we started adding only commented defaults in there and using code default as primary default values. But before that, in some cases, we just forgot to update the code defaults when we updated default.ini value so they got unsynchronized.
[1] https://docs.couchdb.org/en/stable/config/intro.html
Warning: The default.ini file may be overwritten during an upgrade or re-installation, so localised
changes should be made to the local.ini file or files within the local.d directory.
|
Thank you for the review @iilyak ! |
3 participants
Overview
Solved conflicts from "cherry-pick" 3.x commit with kdiff3 merge tool.
Testing recommendations
make eunit apps=chttpd suites=chttpd_util_test
Related Issues or Pull Requests
This fixes #3472
Checklist
rel/overlay/etc/default.ini