-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat(#9706): use Proxy Auth for communicating with Couch #9740
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Robert <[email protected]>
await request.put({ url: securityUrl.toString(), json: true, body: securityObject }); | ||
}; | ||
|
||
module.exports.addRoleAsAdmin = (dbname, role) => addRoleToSecurity(dbname, role, true); |
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.
This function was not being used anywhere.
@@ -143,14 +144,19 @@ const updateServiceWorker = () => { | |||
}); | |||
}; | |||
|
|||
const load = () => { | |||
const addSystemUserToDbs = () => Promise.all([...DATABASES, { name: `${environment.db}-purged-cache` }] |
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.
Technically this is not currently "config related", so it probably does not belong in here. I just stuffed it in here for now since it was the right place in the startup and nowhere else seemed better.
Also probably need a more centralized approach to listing all the DBs that the system user should have member access too....
@@ -19,6 +19,10 @@ bind_address = 0.0.0.0 | |||
server_options = [{recbuf, 262144}] | |||
socket_options = [{sndbuf, 262144}, {nodelay, true}] | |||
require_valid_user = true | |||
authentication_handlers = {chttpd_auth, cookie_authentication_handler}, {chttpd_auth, proxy_authentication_handler}, {chttpd_auth, default_authentication_handler} |
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.
The default value is just:
authentication_handlers = {chttpd_auth, cookie_authentication_handler}, {chttpd_auth, default_authentication_handler}
@@ -18,18 +20,19 @@ const methods = { | |||
HEAD: 'HEAD' | |||
}; | |||
|
|||
const promisedAdminAuthHeaders = getAuthHeaders(environment.username, '_admin'); |
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.
This needs to be changed to not set the _admin
role by default. I have it like this so everything runs, but the intended approach is to just use the default username with its "member" access. Then, for requests that need admin access, they will have to explicitly set the _admin
role header.
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.
I put the code for dealing with the COUCHDB_SECRET
in here. It could probably be its own shared-lib if we wanted but it seemed at least adjacent to the couch-request functionality so I started with it here.
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.
Actually, after more digging, it seems like shared-libs/settings
might be a the most logical place to put this. However seems like it could cause a circular dependency since that lib already depends on couch-request
. I need to look more into this.
@dianabarsan Could you please let me know what you think about the approach in this prototype? 🙏 The code is not done yet, but I think it proves enough of the functionality for us to be able to make a decision on if we want to pursue this approach. The api/sentinel dev instances do run locally for me and I have done some smoke testing, but there are definitely still broken edge cases and incomplete flows.... |
Hi @jkuester I requested a review, so I get reminded about this. I had read the email over the weekend and it skipped my memory today. Apologies for the delay. |
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.
I left a few questions inline.
Nice work on this. Overall I think this could be useful to overcome the difficulties in handling sso, however I'm still not convinced about how secure this will be, because it seems that right now everyone gets a free pass.
return envarSecret; | ||
} | ||
|
||
// Need to look up the secret (assuming the COUCH_URL contains user/pass) |
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.
I think that, in some other file here you had edited the export COUCH_URL documentation to not include basic auth.
So, we would still have a COUCH_URL with basic auth?
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.
In order to make things passive, we need to support the case where no COUCHDB_SECRET
is set, but the COUCH_URL
contains the username/password (this is likely the case for many existing single-node servers). With these changes the "recommended" case would be where COUCHDB_SECRET
is set and COUCH_URL
does not contain username/password.
The case we do not support is where COUCHDB_SECRET
is NOT set and COUCH_URL
does not contain username/password. (Since in this case we have no way of actually connecting to Couch...)
}; | ||
}; | ||
|
||
const getAuthHeaders = async (username, roles) => { |
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.
This basically means that everyone has a free pass? Since we are getting the token ourselves? How are we authenticating people?
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.
"everyone" here would be "all the calls coming from api/sentinel" and "free pass" would be "member-level access to Couch dbs (upgradeable to admin by changing the roles header value)".
The goal is to use proxy auth for all Couch traffic after the user making the request is authenticated. (More on this in my response to your next comment.)
|
||
const getAuthHeaders = async (username, roles) => { | ||
if (!username) { | ||
return createHeaders(username, roles); |
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.
So, if the username is null
we'll create headers with the username null
? I don't follow.
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.
Yeah, so this really needs its own code path instead of the confusing overloading I am doing here. But practically, yes. When you pass in null
, you get null
values for the headers which means that the proxy auth headers are not actually set on the request.
So, when doing the _session
checks to authenticate the user making the api REST call, we pass null
in here to make sure the _session
call is done without any proxy auth headers (and is just authenticated via the users username/password or their session cookie).
For all the normal traffic between api/sentinal and Couch we want to use Proxy Auth. But, for the specific user authorization checks (made against _session
) we explicitly disable the proxy auth (by passing null
), so we can actually check the user. For SSO flows, instead of calling _session
to validate the user, we would call the 3rd party identity provider. Either way, once the user is authenticated, the subsequent communication between api and couch should be the same and should happen via proxy auth.
Description
Closes #9706
This is my prototype attempt at "simplifying" communication between api/sentinel and CouchDB. As noted in the ticket, there are currently several different user modes used when api/sentinel connect to Couch. Significantly, there is currently not really any way for api/sentinel to connect to Couch with just "member" access. Currently all access (not using creds provided by a user) is full "admin". Most of the time, admin access is unnecessary.
This PR tries to address both challenges by introducing Couch Proxy Auth as the sole method used to authenticate Couch traffic from api/sentinel (except for the actual user auth checks to authenticate users). The username
cht-system
gets added as a "member" user to the various Couch dbs that api/sentinel need to access. Then, all traffic from api/sentinel includes the proxy auth headers needed to authenticate thischt-system
user (including a token calculated via theCOUCHDB_SECRET
). This means that by default all traffic has "member" access. For requests that need admin access (or no access, e.g. for the auth requests) this must be explicitly configured in the request. (A request for thecht-system
user is made "admin" simply by specifying_admin
in the role header.) IMHO this gives a nice balance of security and convenience.Including a
COUCHDB_SECRET
envar when deploying via Docker Compose is part of our prod hosting instructions (not positive about the k8s setup). However, it does not seem to actually be required to run the compose config unless you are doing a multi-node Couch cluster. Otherwise, if no secret is configured, then the Couch container will generate one when it runs. This means that even if we update the compose config to inject theCOUCHDB_SECRET
envar, there is not a 100% guarantee it will be there. So, I added some fallback code to ping CouchDB and lookup the secret. If theCOUCHDB_SECRET
envar is set, then theCOUCH_URL
envar for api/sentinel does not actually need to include the user/pass.The overall goal of simplifying Couch communication is to hopefully make the implementation of SSO Auth in #9735 straightforward so that SSO users can be authenticated against the 3rd party identity provider and then the api server does all the talking to Couch with no need of an actual Couch user session.
TODO
couch-request
functionality to not use_admin
role by default on the non-pouch requests. We should be able to make "member" level requests by default. The_admin
requests should be made by explicitly setting that header.COUCHDB_SECRET
in to the api and sentinel containersCode review checklist
can_view_old_navigation
permission to see the old design.License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.