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

Improvements to password salt record #2460

Open
corrideat opened this issue Dec 15, 2024 · 1 comment · May be fixed by #2497
Open

Improvements to password salt record #2460

corrideat opened this issue Dec 15, 2024 · 1 comment · May be fixed by #2497
Assignees
Labels
App:Backend App:Frontend Kind:Enhancement Improvements, new features, performance upgrades, etc. Level:Advanced Note:Security

Comments

@corrideat
Copy link
Member

Problem

Working on the password change PR gave me two ideas. Currently, there are two minor issues with password salt storage:

  1. The salt record is indexed by username and not by contract ID. I don't think this is even an issue, but I think it might make more sense to index the record by contract ID. The plan all along was using contract IDs, but the challenge is that we need to set this before the contract ID is known.
  2. The salt record is set before the identity OP_CONTRACT event is stored. We 'need' to do this, but it's not ideal because we'd like the salt record to be set atomically with creation of the identity contract. Setting the salt first is safer than sending the event first, lest we create a contract that can never be used.

With the password change PR (#2446), I introduced atomic updates, so we could use the same mechanism to fix 2 above. Furthermore, if we do it atomically, we could also solve 1, meaning that the salt record can be indexed by contract ID (instead of by username).

However, fixing 1 (changing the index) means that the server DB will need to be updated, or else it won't be possible to log in to existing accounts. It will not mean that contracts need to be recreated, rather a one-time migration job on the SQLite DB would do.

Solution

  1. Implement atomic registration of password salts, using a token, similarly to what Password changes #2446 does for password updates.
  2. Implement indexing by contract ID instead of by username. (This would happen in POST /event upon redeeming the token).
  3. Update the frontend code (mostly in app/identity.js) to work with these changes.
  4. Write an update script to migrate existing password records (from username to contract ID).
@corrideat corrideat self-assigned this Dec 15, 2024
@corrideat corrideat added Kind:Enhancement Improvements, new features, performance upgrades, etc. App:Frontend App:Backend Note:Security Level:Advanced labels Dec 15, 2024
@corrideat corrideat added this to the Final breaking changes milestone Jan 5, 2025
@corrideat
Copy link
Member Author

If we do this before https://github.com/okTurtles/group-income/milestone/3, then we don't need to write an update script.

@corrideat corrideat linked a pull request Jan 7, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App:Backend App:Frontend Kind:Enhancement Improvements, new features, performance upgrades, etc. Level:Advanced Note:Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant