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

Sync users database to Postgres #137

Closed
kennsippell opened this issue Jun 22, 2023 · 10 comments
Closed

Sync users database to Postgres #137

kennsippell opened this issue Jun 22, 2023 · 10 comments
Assignees

Comments

@kennsippell
Copy link
Member

A common failure in CHT is that the roles in the users database do not match the user-settings database. We'd like to write assertions to detect these cases via cht-app-monitoring-data-ingest and request the user document in Postgres. medic/cht-app-monitoring-data-ingest#64

@kennsippell kennsippell self-assigned this Jun 22, 2023
@garethbowen
Copy link
Member

I'm a little concerned about the security implications of replicating the encrypted passwords. It's not dire, but still less than ideal. Is there another way to expose/detect these errors? Is there a way to have CHT automatically fix this issue rather than being a manual step? How does this error present?

@kennsippell
Copy link
Member Author

kennsippell commented Jun 22, 2023

Discussed the security implications of this today with @mrjones-plip. I'll defer to him re security talk.

We detect these issues today through scripts like this. They are run manually. We are trying to integrate this with monitoring efforts like watchdog.

My understanding is that some parts of CHT use the _users database to determine a user's role and permissions (CouchDB/API/etc), while others use the user-settings doc in medic to determine the user's roles and permissions (webapp). These docs are expected to be identical, but in reality they fall out of sync (eg. I saw it twice last week example).

Also the facility_id can be out of sync between the two.

I don't have a clear summary of how this manifests. In general, it leads to users with ambiguous roles and permissions. This can be a security issue, and likely to manifest as "general spookiness".

@garethbowen
Copy link
Member

Would you mind raising an issue in cht-core for the bug (as well as continuing with monitoring)? Essentially cht-core should consistently use the one in user-settings because that's the only one that's downloaded for offline use. However I suspect couchdb db level permissions will use the one in the _users db because that's the only one it knows about. The CHT has code to keep these in sync but obviously it's not working correctly.

@kennsippell
Copy link
Member Author

kennsippell commented Jun 22, 2023

I've opened medic/cht-core#8337.

as well as continuing with monitoring

Can you clarify what you mean here? Are you saying you have no blocking security concerns here and I can proceed with this issue as described (sync _users db to Postgres)? Or are you saying we should continue to manually monitor this via existing means + address a fix for it in core?

@mrjones-plip
Copy link
Contributor

mrjones-plip commented Jun 22, 2023

I'm a little concerned about the security implications of replicating the encrypted passwords. It's not dire, but still less than ideal.

Discussed the security implications of this today with @mrjones-plip. I'll defer to him re security talk.

Yes, agreed this is not ideal and should be done with care (not on by default). Some thoughts on why this seemed OK (enough):

  • Passwords are hashed (not encrypted) with pbkdf2 (@kennsippell and @eljhkrr: sorry I said bcrypt on our call today - I was wrong!). This is a modern hashing algorithm and considered secure
  • The Postgres DB (RDBMs in Medic's case) or is already full of PII and PHI and is considered a safe place to put sensitive data, like these hashes
  • The admin credentials to the CHT instance are already on the couch2pg system to facilitate logging in to CouchDB, a fundamental and mandatory function of couch2pg. If someone illicitly gains access to the system, they'll gain access to the admin password and they wouldn't bother with the user account hashes when the admin password is right there in plaintext.

A low cost remediation here would be to zero out the hash values in the postgres DB at some point in the sync process. This won't stop anyone per plaintext credentials mentioned above, but is so cheap there's no reason not to do it and it might help just a little.

As an aside, it looks like our iterations on pbkdf2 is low, and have filed an issue accordingly.

@garethbowen
Copy link
Member

Can you clarify what you mean here?

That the bug fix and monitoring effort should continue in parallel.

@garethbowen
Copy link
Member

A low cost remediation here would be to zero out the hash values in the postgres DB at some point in the sync process. This won't stop anyone per plaintext credentials mentioned above, but is so cheap there's no reason not to do it and it might help just a little.

This is the solution I was thinking of. If it's a low effort change it would make me feel better to know we're not storing the hashed passwords.

@kennsippell
Copy link
Member Author

kennsippell commented Jun 23, 2023

That will require a change in the underlying couch2pg repository and not just cht-couch2pg. Here is a proposal medic/couch2pg#45. It's maybe a bit loose since it is for docs with "type: user" and not exclusively for the _users database; but it is straight-forward without interface changes.

@mrjones-plip
Copy link
Contributor

@kennsippell - the speed at which you're moving is inspiring (EG PR to strip hashes in what looks like <60 min). I really appreciate your bold efforts here - thank you!!

kennsippell added a commit to medic/couch2pg that referenced this issue Jun 23, 2023
kennsippell added a commit that referenced this issue Jun 26, 2023
@kennsippell
Copy link
Member Author

Released as v3.6.2

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

No branches or pull requests

3 participants