-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Improve basic auth credentials handling in replicator #3599
Improve basic auth credentials handling in replicator #3599
Conversation
This is a port of commit ecd266b from 3.x to main. Including the same commit message from there for completeness and then towards the end, there is a description of changes required to port the PR to main. Previously, there were two ways to pass in basic auth credentials for endpoints -- using URL's userinfo part, and encoding them in an `"Authorization": "basic ..."` header. Neither one is ideal for these reasons: * Passwords in userinfo don't allow using ":", "@" and other characters. However, even after switching to always unquoting them like we did recently [1], it could break authentication for usernames or passwords previously containing "+" or "%HH" patterns, as "+" might now be decoded to a " ". * Base64 encoded headers need an extra step to encode them. Also, quite often these encoded headers are confused as being "encrypted" and shared in a clear channel. To improve this, revert the recent commit to unquote URL userinfo parts to restore backwards compatibility, and introduce a way to pass in basic auth credentials in the "auth" object. The "auth" object was already added a while back to allow authentication plugins to store their credentials in it. The format is: ``` "source": { "url": "https://host/db", "auth": { "basic": { "username":"myuser", "password":"mypassword" } } } ``` {"auth" : "basic" : {...}} object is checked first, and if credentials are provided, they will be used. If they are not then userinfo and basic auth header will be parsed. Internally, there was a good amount duplication related to parsing credentials from userinfo and headers in replication ID generation logic and in the auth session plugin. As a cleanup, consolidate that logic in the `couch_replicator_parse` module. The commit is quite different from the 3.x one for these two reasons: * `main` uses two types of replication endpoint "objects": `#httpdb` records and `HttpDb` maps. In most cases it uses maps which can be serialized and deserialized to and from json. But in lower level, connection handling code in couch_replicator_httpc, it uses `#httpdb` records. This explain the need to still handle both representations. Auth session plugin, for instance, uses the lower level #httpdb records while replicator ID handling code uses the map based one. * `main` has all the parsing of replication documents and `_replicate` request bodies in a separate `couch_replicator_parse`. So, most of the code which handles normalizing basic auth creds is there instead of `couch_replicator_docs` or `couch_replicator_utils` like it is in 3.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Only question I have is about whether using multiple different creds should result in silently dropping one or more of them, or immediately failing with an error?
<<"url">> => <<"http://h/db">>, | ||
<<"auth_props">> => #{}, | ||
<<"headers">> => maps:merge(DefaultHeaders, #{ | ||
<<"authorization">> => basic_b64("u", "p@") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay I can finally put @
in my password 🎉
This is a port of commit ecd266b from 3.x to main. Including the same commit message from there for completeness below. Then, towards the end, there is a description of changes required to port this PR to main.
Previously, there were two ways to pass in basic auth credentials for endpoints -- using URL's userinfo part, and encoding them in an
"Authorization": "basic..."
header. Neither one is ideal for these reasons:Passwords in userinfo don't allow using ":", "@" and other characters. However, even after switching to always unquoting them like we did recently [1], it could break authentication for usernames or passwords previously
containing "+" or "%HH" patterns, as "+" might now be decoded to a " ".
Base64 encoded headers need an extra step to encode them. Also, quite often these encoded headers are confused as being "encrypted" and shared in a clear channel.
To improve this, revert the recent commit to unquote URL userinfo parts to restore backwards compatibility, and introduce a way to pass in basic auth credentials in the "auth" object. The "auth" object was already added a while
back to allow authentication plugins to store their credentials in it. The format is:
{"auth" : "basic" : {...}} object is checked first, and if credentials are provided, they will be used. If they are not then userinfo and basic auth header will be parsed.
Internally, there was a good amount duplication related to parsing credentials from userinfo and headers in replication ID generation logic and in the auth session plugin. As a cleanup, consolidate that logic in the
couch_replicator_parse
module.The commit is quite different from the 3.x one for these two reasons:
main
uses two types of replication endpoint "objects":#httpdb
records andHttpDb
maps. In most cases it uses maps which can be serialized and deserialized to and from json. But in lower level, connection handling codein couch_replicator_httpc, it uses
#httpdb
records. This explain the need to still handle both representations. Auth session plugin, for instance, uses the lower level #httpdb records while replicator ID handling code uses the map based one.main
has all the parsing of replication documents and_replicate
request bodies in a separatecouch_replicator_parse
. So, most of the code which handles normalizing basic auth creds is there instead ofcouch_replicator_docs
orcouch_replicator_utils
like it is in 3.xThanks to @jaydoane for the original idea for this scheme