Implement pluggable authentication and session support for replicator #1176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
2 times, most recently
from
380848c
to
d24e194
Compare
iilyak
reviewed
Feb 19, 2018
iilyak
reviewed
Feb 19, 2018
iilyak
reviewed
Feb 19, 2018
iilyak
reviewed
Feb 19, 2018
iilyak
reviewed
Feb 19, 2018
| handle_call({update_headers, Headers, _Epoch}, _From, State) -> | ||
| case maybe_refresh(State) of | ||
| {ok, State1} -> | ||
| Cookie = "AuthSession=" ++ State1#state.cookie, |
There was a problem hiding this comment.
I am a bit concerned with this. Cookie value is not escaped and can be manipulated. I followed the code we use for maybe_update_cookie and it looks like there is a possibility to inject new headers.
CookieKVs = mochiweb_cookies:parse_cookie("AuthSession=\"see\nInjected=hack\";baz; Version=1").
Cookie = mochiweb_headers:get_value("AuthSession", CaseInsKVs).
40> "AuthSession=" ++ Cookie.
"AuthSession=see\nInjected=hack"
There was a problem hiding this comment.
But wondering what the threat model is here. The server somehow injecting extra headers into its own session cookie?
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
from
d24e194
to
bb8e8c7
Compare
|
Conducted a simple benchmark using couchdyno https://github.com/cloudant-labs/couchdyno Timed replicating 100k docs with PBKDF2 iterations set to 10k. Master took 1133 seconds and this PR took 574 seconds. This should allow increasing the work factor to improve security without sacrificing replication performance. |
jaydoane
reviewed
Feb 20, 2018
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
from
bb8e8c7
to
4bf464c
Compare
|
CouchDyno has a set of extended, long running replication tests. Those pass as well: |
jaydoane
reviewed
Feb 22, 2018
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
2 times, most recently
from
78bf2f4
to
fdaa3df
Compare
rnewson
reviewed
Feb 22, 2018
| % port and path as they were in the original. | ||
| Prefix = lists:concat([Proto, ":https://", User, ":", Pass, "@"]), | ||
| Suffix = lists:sublist(Url, length(Prefix) + 1, length(Url) + 1), | ||
| NoCreds = lists:concat([Proto, ":https://", Suffix]), |
There was a problem hiding this comment.
This is the url part, that's a string() based on ibrowse's url type:
https://github.com/cmullaparthi/ibrowse/blob/master/src/ibrowse.erl#L154
Also concat automatically does the stringification so don't have to do atom_to_list for Proto for example. Previously also had a Port that was an integer there.
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
3 times, most recently
from
727af7f
to
921609e
Compare
|
I made two updates to the PR over the weekend:
|
|
Debating whether to make this an opt-in feature at first and make the default plugin list = |
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
from
921609e
to
ebc7b6a
Compare
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
from
ebc7b6a
to
cbe0e5f
Compare
|
I've enabled this with; +auth_plugins = couch_replicator_auth_session,couch_replicator_auth_noop and used sys:get_state(Pid) to look at the state of a replicator task. auth_props remains []. Until I see this work, it's -1 from me. |
|
ok, figured out my mistake. It does acquire cookies. I'll finish the code review itself today. |
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
from
cbe0e5f
to
6a7d69d
Compare
|
Rebased on latest master |
Previously replicator used only basic authentication. It was simple and straightforward. However, with PBKDF2 hashing becoming the default it would be nice not to do all the password verification work with every single request, and instead take advantage of session (cookie) based authentication. This commit implements session based authentication via a plugin mechanism. The list of available replicator auth modules is configurable. For example: ``` [replicator] auth_plugins = couch_replicator_auth_session,couch_replicator_auth_basic ``` The plugins will be tried in order. The first one to successfully initialize will end up being used for that endpoint (source or target). During the initialization callback, a plugin could decide it cannot be used in the current context. In that case it signals to be "ignored". The plugin framework will then skip over it and try to initialize the next on in the list. `couch_replicator_auth_basic` effectively implements the old behavior. This plugin should normally be used as a default catch-all at the end of the plugin list. In some cases, it might be useful to enforce exclusive use of session-based auth and fail replication jobs if it is not available. `couch_replicator_auth_session` does most of the work of handling session based authentication. On initialization, it strips away basic auth credentials from headers and url to avoid basic auth being used on the server. Then it is in charge of periodically issuing POST requests to `_session`, updating the headers of each request with the latest cookie value, and possibly picking up new session cookie if the server can issue them along with reglar responses. Currently session based auth plugin is not enabled by default and is an opt-in feature. That is, users would have to explicitly add the session module to the list of auth_plugins. In a future, session might be used by default. As discussed in apache#1153 this work also removes OAuth 1.0 support. After server-side support was removed, it had stopped working anyway since the main oauth app was removed. However, with the plugin framework in place it would be possible for someone to implement it as a separate module not entangled with the rest of the replicator code. Fixes apache#1153
nickva
force-pushed
the
replicator-pluggable-auth-module
branch
from
6a7d69d
to
eedb540
Compare
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Implement pluggable authentication and session support for replicator
Previously replicator used only basic authentication. It was simple and
straightforward. However, with PBKDF2 hashing becoming the default it would be
nice not to do all the password verification work with every single request,
and instead take advantage of session (cookie) based authentication.
Description
This commit implements session based authentication via a plugin mechanism.
The list of available replicator auth modules is configurable:
The plugins will be tried in order. The first one to successfully initialize
will end up being used for that endpoint (source or target).
During the initialization callback, a plugin could decide it cannot be used in
the current context. In that case it signals to be "ignored". The plugin
framework will then skip over it and try to initialize the next on in the list.
couch_replicator_auth_basiceffectively implements the old behavior. Thisplugin should normally be used as a default catch-all at the end of the plugin
list. In some cases, it might be useful to enforce exclusive use of
session-based auth and fail replication jobs if it is not available.
couch_replicator_auth_sessiondoes most of the work of handling session basedauthentication. On initialization, it strips away basic auth credentials from
headers and url to avoid basic auth being used on the server. Then it is in
charge of periodically issuing POST requests to
_session, updating theheaders of each request with the latest cookie value, and possibly picking up
new session cookie if the server can issue them along with reglar responses.
As discussed in #1153 this work also removes OAuth 1.0 support. After
server-side support was removed, it had stopped working anyway since the main
oauth app was removed. However, with the plugin framework in place it would be
possible for someone to implement it as a separate module not entangled with
the rest of the replicator code.
Fixes #1153
Checklist
[*] Will do it after the review stage.