Skip to content
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 #3586

Merged
merged 1 commit into from Jun 1, 2021

Conversation

nickva
Copy link
Contributor

@nickva nickva commented May 28, 2021

Previously, there were two ways to pass in basic auth credentials for
endpoints -- using URL's userinfo part and encoding the them in an
"Authorization": "basic ..." header. Neither one is ideal for these reasons:

  • Passwords in userinfo doesn't allow using ":", "@" and other characters.
    However, even after switching to always unquoting them like we did recently
    [1], would 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_utils module.

[1] f672b91

The original idea for this scheme belongs to @jaydoane

Checklist

  • Code is written and works correctly
  • Changes are covered by tests
  • Any new configurable parameters are documented in rel/overlay/etc/default.ini
  • A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation (will create a docs pr after the review)

@nickva nickva force-pushed the improve-basic-auth-creds-passing-in-replicator-3.x branch from b018bfa to cc5baed Compare May 28, 2021 23:12
@nickva nickva requested a review from jaydoane May 30, 2021 21:11
Copy link
Contributor

@jaydoane jaydoane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! Only found one minor formatting issue. Tests pass:

$ make eunit apps=couch_replicator
...
=======================================================
  All 301 tests passed.

src/couch_replicator/src/couch_replicator_utils.erl Outdated Show resolved Hide resolved
@nickva nickva force-pushed the improve-basic-auth-creds-passing-in-replicator-3.x branch 2 times, most recently from edc353d to 0c6ca57 Compare June 1, 2021 19:50
Previously, there were two ways to pass in basic auth credentials for
endpoints -- using URL's userinfo part and encoding the them in an
`"Authorization": "basic ..."` header. Neither one is ideal for these reasons:

 * Passwords in userinfo doesn't allow using ":", "@" and other characters.
   However, even after switching to always unquoting them like we did recently
   [1], would 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_utils` module.

[1] f672b91
@nickva nickva force-pushed the improve-basic-auth-creds-passing-in-replicator-3.x branch from 0c6ca57 to 914b032 Compare June 1, 2021 20:38
@nickva nickva merged commit ecd266b into 3.x Jun 1, 2021
@nickva nickva deleted the improve-basic-auth-creds-passing-in-replicator-3.x branch June 1, 2021 21:07
nickva added a commit to apache/couchdb-documentation that referenced this pull request Jun 7, 2021
nickva added a commit to apache/couchdb-documentation that referenced this pull request Jun 7, 2021
nickva added a commit to apache/couchdb-documentation that referenced this pull request Jun 7, 2021
nickva added a commit to nickva/couchdb that referenced this pull request Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants