Fix/13 improve auth security #14
Merged
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
added 5 commits
April 3, 2022 16:27
added 21 commits
April 4, 2022 10:53
…ss token if refresh token is invalid.
|
Todo:
|
added 6 commits
August 31, 2022 12:08
nick-verida
requested changes
Sep 1, 2022
There was a problem hiding this comment.
A few minor suggestions.
Also there are some conflicts that need fixing. I haven't looked at them properly but some seem to have been better logging/exception handling we added when trying to work out what was going wrong in some circumstances so it is important to keep those changes.
.env.example
Outdated
| @@ -0,0 +1,22 @@ | |||
| HASH_KEY=wb4f9qf6789bqjqcj0cq4897cqn890tq0 | |||
There was a problem hiding this comment.
-
Rename to
sample.envso it is visible by default in the file system (and also the docs mention sample.env) -
Change this key for the sample to
HASH_KEY=this_is_not_the_prod_hash_key
We've had 2 separate production issues caused by people not updating this.
There was a problem hiding this comment.
Would you mind to add more info on how to actually get it similar to this?
There was a problem hiding this comment.
| @@ -40,6 +57,8 @@ A `sample.env` is included. Copy this to `.env` and update the configuration: | |||
| - `DB_REJECT_UNAUTHORIZED_SSL`: Boolean indicating if unauthorized SSL certificates should be rejected (`true` or `false`). Defaults to `false` for development testing. Must be `true` for production environments otherwise SSL certificates won't be verified. | |||
| - `DB_PUBLIC_USER`: Alphanumeric string for a public database user. These credentials can be requested by anyone and provide access to all databases where the permissions have been set to `public`. | |||
| - `DB_PUBLIC_PASS`: Alphanumeric string for a public database password. | |||
| - `ACCESS_TOKEN_EXPIRY`: Number of seconds before an access token expires. The protocol will use the refresh token to obtain a new access token. CouchDB does not support a way to force the expiry of an issued token, so the access token expiry should always be set to 5 minutes (300) | |||
There was a problem hiding this comment.
How does this relate to REFRESH_JWT_SIGN_PK in authManager.js?
There was a problem hiding this comment.
That link doesn't point to a specific line.
Can you expand on your question, I don't follow.
There was a problem hiding this comment.
In authManager.js there is:
process.env.REFRESH_JWT_SIGN_PK, {
// expies in 1 minute
expiresIn: 60
})
It's not immediately clear what this token expiry is for and why it is a different value to ACCESS_TOKEN_EXPIRY
There was a problem hiding this comment.
@nick-verida It's explained here, is that not clear?
src/controllers/user.js
Outdated
| }); | ||
| } | ||
| else { | ||
| return res.status(400).send({ |
There was a problem hiding this comment.
500 error more appropriate
src/controllers/user.js
Outdated
| try { | ||
| success = await DbManager.updateDatabase(username, databaseHash, contextName, options); | ||
| } catch (err) { | ||
| return res.status(400).send({ |
There was a problem hiding this comment.
500 error more appropriate
| @@ -0,0 +1,22 @@ | |||
| import { EnvironmentType } from "@verida/account" | |||
|
|
|||
| export default { | |||
There was a problem hiding this comment.
Need a way to set these from the .env file.
There was a problem hiding this comment.
Why? These are configs just for testing which serve a different purpose.
There was a problem hiding this comment.
Because I had to change the URL of the server to post 5000 (which is possible using the PORT config setting) but there was no way to point the tests at it without changing the sourcecode.
Perhaps there should be .tests.env or something separately.
There was a problem hiding this comment.
Changed to load port from process.env for tests.
|
Added fixes for @nick-verida review. Need to:
|
|
Branch conflicts resolved, tests all passing. |
dmty
reviewed
Sep 1, 2022
| ``` | ||
| [httpd] | ||
| WWW-Authenticate = Basic realm="administrator" | ||
| enable_cors = true |
There was a problem hiding this comment.
Does it work alright without enabling cors?
There was a problem hiding this comment.
No. Web browsers will throw CORS errors when they access CouchDB directly.
This shouldn't have been removed, re-added to README.
dmty
reviewed
Sep 1, 2022
dmty
reviewed
Sep 1, 2022
added 2 commits
September 2, 2022 10:01
…orized user. More HTTP status improvements and better checking for valid request params.
|
Updated this PR to also close #4 |
tahpot
added a commit
that referenced
this pull request
Jan 14, 2023
* Fix/13 improve auth security (#14) * Support running a single test or multiple. Add JWT dependency. * Implement JWT authorization. Use a new consent message for signature verification. * Update permission tests to support JWT auth. * Implement JWT tests * Add README instructions for enabling JWT auth in CouchDb * Fix incorrect merge issues. * Add support for generating and verifying refresh token * More refresh token unit tests. Add access token support. Db refactor. * Fix permission unit tests with new refactor. * Update server endpoints to use new auth. Create server integration tests for auth. * Hash database name server side before creating * Verify context name when verifying refresh token. Don't generate access token if refresh token is invalid. * Confirm expected user is admin of database before deleting it. * Ensure database names begin with "v" * Return an access token and host when requesting a refresh token. * Support deleteDatabase endpoint * Expand unit tests * Split controller into auth and user. * Fix broken test * Fix broken delete database calls * Remove JWT test no longer required * Support invalidating device ID * Invalidating devices now working correctly with tests * Implement garbage collection * Fix building DSN for public credentials * Add CouchDB configuration note around basic auth * Add more docs * Fix edge case issues identified during testing * Fix yarn dependencies * Fix example .env files and remove duplicate. * Describe how authentication works * Rename to authorization * Add missing account-node dev dependency * Fix minor issues with tests. Better config docs in README. * Resolve feedback for further review. * Fix missed merge issue * Support running single test * Add support for saving user databases and getting the info as an authorized user. More HTTP status improvements and better checking for valid request params. * Add missing require for PouchDB * Cleanup didsToUsernames * Destructure some vars * Add cors requirement to README * Add isTokenValid endpoint to verify a refresh token is still valid and obtain the expiry. * Support the new decentralized did-client implementation (#32) * adding acacia testnet * Update sample.env * Fix server tests. Add support for deleting all databases for a user. (#35) * Update dependencies to acacia rc2 * Update account node dependency for tests * Fix issue with documenting how to configure HMAC key in couchdb * Add better comments * Update config to use testnet storage node * Add docker and docker-compose (#27) * Remove body-parser as deprecated * Add Dockerfile * Replace require with import for the js work without babel * Add docker-compose * Update node to latest 14 * Add gh workflow to build and publish docker image * Rearrange dockerfile to improve image size * Uncomment dockerhub * Add docker info and single node config to readme * Update to latest verida dependencies. Remove module so tests run. * Remove lambda support * Details on how to do the Docker build and push it. * Update REARME.md: enable_cors = true * update Docker build command * Changes to make deployment work. Yarn build must happen in Docker container * Support signing all responses using the storage node VDA private key. (#42) * Add support for /user/usage details (#43) * add logging to make debugging Docker image somewhat possible * fix logging message * update README to correct authentication info. Add logging to dbInit exception handler * logging for failure of /auth/public; clean up pakage.json * update docs on HASH_KEY not being needed * Feature/37 implement vip 3 (#44) * Add scaffolding for new did storage support. Refactor routes. * Initial progress on create and get endpoints * Updates working. Get all versions working. * Deletion now working. All tests passing. * Support field validation for create, including proof verification * Tests destroy database before starting. Fix create and update tests to work with new verification. * Fix missing versionId field on index * Upgrade to latest ethers version * Update error messages. Wrap versions response in a versions parameter. * Fix handling of all versions being wrapped in a versions parameter. * Ensure signature verification for update, delete * Implement storage limits and status information for users and the system (#45) * Remove redundant hash key Co-authored-by: Chris <chris@verida.io> * Update README.md Remove Docker compose (moved to infrastructure) Remove redundant value in windows config. * Force lowercase username when generating usernames * Add comments about running tests * Include permissions in database info * Update dependencies to next release candidate * delay start of docker image until $DB_HOST reolves * Add support for a NODE_URI * typo * remove checking for network now SN supports different endpoint * Move signature tests to the end to avoid issues with testing servers with invalid signatures. * Support internal and external DB hostnames * Fix hostname generation. * Fix build external host * Add console errors and warnings to help with debugging * Support looking up a DID document and caching it * Include public key in system status * Feature/49 server replication (#50) * Rename applicationName to contextName. Add replicator permissions. * Add generateHash utility method * Refactor so context databases are in their own database that will be replicated, instead of all stored in a single database. Other minor fixes and test improvements. * Implement untested auth/replicationCreds * Implementation complete, untested. * Add comments for next steps * First pass at replication test * Fix missing endpoint variable * Bug fixes and enhanced logging * Bug fixes from testing * Bug fixes * Support generating correct CouchURI for server * Bug fixes * Fix syntax error * First tests passing * Fix clearing databases. Add warnings. * Ensure replicator user has did context role * Bug fixes * Try basic auth header * Update replication document creation * Create new replicater user for replication instead of using admin * Bug fix replication * Update working tests * All tests written so far, pass! * Remove commented test code * Code cleanup * Code cleanup. Replication test cleanup. Verify timestamp when fetching replication creds. * Add comments in .env file Co-authored-by: Chris <chris@verida.io> * Feature/49 server replication (#51) * Rename applicationName to contextName. Add replicator permissions. * Add generateHash utility method * Refactor so context databases are in their own database that will be replicated, instead of all stored in a single database. Other minor fixes and test improvements. * Implement untested auth/replicationCreds * Implementation complete, untested. * Add comments for next steps * First pass at replication test * Fix missing endpoint variable * Bug fixes and enhanced logging * Bug fixes from testing * Bug fixes * Support generating correct CouchURI for server * Bug fixes * Fix syntax error * First tests passing * Fix clearing databases. Add warnings. * Ensure replicator user has did context role * Bug fixes * Try basic auth header * Update replication document creation * Create new replicater user for replication instead of using admin * Bug fix replication * Update working tests * All tests written so far, pass! * Remove commented test code * Code cleanup * Code cleanup. Replication test cleanup. Verify timestamp when fetching replication creds. * Replication tests all passing with remote 3x servers * Remove console output Co-authored-by: Chris <chris@verida.io> * Feature/52 user db list replicated (#54) * Ensure user databases are replicated * Add checkReplication tests confirming databases are re-created * Fix user database issues. Add debug logging. * Bug fix database creation via checkReplication() * Basic replication unit tests passing * Remove excess debug output * Fix trailing slash issue * Fix trailing slash issues * Adding cache checks for DID documents * Fix database list database incorrect config * Handle missing DID document * Throw error if unknown check databse error * Improve service endpoint checks on checkReplication() * Ensure user database list database has replicator local role * Fix issue with user database list database not being recreated. Minor refactor. * Handle garbage collection where token already deleted * Handle user database list database document conflict * Fix caching of couch instance not respecting external v internal * Add debug output to nodes about cache status * Add error message when unable to resolve DID * Add debug logging * Unify storage node checks and hashes to use the hostname only * Make endpointUri more resiliant * Upgrade encryption utils to latest * Support recoverying from replication credential errors * Add debug logging * Add storage node HTTP timeouts. Add more debugging. * Fix replicater creds to have replicaterUsername has id * Fix replicater username to only use hostname * Fix incorrect username being loaded * Fix replication to start working upon creation of new databases, only update password if it changes, re-use existing password where possible. * Check replication credentials after all databases have been checked. * Refactor how endpoint security checks occur. Cleanup console output. * Improve debug output * More debug output for testing * Remove redundant record variable * Add delete failed replication meethod * Fix incorrect delete replication code * More debugging * More debug output * Update order of code * Change order so that failed replication entries are removed first, so they are auto created later. * Add more debug output for testing= * Force update of remote credentials if they are definitely invalid * Add debug output * Add more debug output * Fix issue with fetchReplicaterCredential setting the wrong key for the credential cache * Fix incorrect username * Restructure how replication fails are fixed * Add more debug output, fix replication status id parameter * More debug output * More debugging * Fix sample.env comment * Reduce debug output * d * Refactor replication and improve debug output * Handle unable to fetch DID error * Add checkreplication debug header * Lots of bug fixes and refactor * Bug fix incorrect instance reference * Force cache refresh if service endpoint not found * Handle missing did document * set winning_revs_only: true * Remove winning_revs_only * Remove permissions check * Add debug logging * More debug output * Debug to find error * Support ignore cache option when fetching DID document * Correctly handle empty DIDs * Update CHANGELOG in preparation for v2.0.0 release
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.
Closes verida/security-issues#2.
Don't merge until verida/security-issues#1 is also resolved and merged.
Closes #4