Skip to content

password authentication for local-only users#1841

Merged
davepacheco merged 12 commits intomainfrom
local-user-crud-passwords
Nov 1, 2022
Merged

password authentication for local-only users#1841
davepacheco merged 12 commits intomainfrom
local-user-crud-passwords

Conversation

@davepacheco
Copy link
Copy Markdown
Collaborator

@davepacheco davepacheco commented Oct 20, 2022

Adds support for password authentication for users in local-only Silos. See RFD 321 for details.

@davepacheco davepacheco marked this pull request as ready for review October 20, 2022 21:47
@davepacheco davepacheco requested a review from plotnick October 20, 2022 21:47
@davepacheco davepacheco mentioned this pull request Oct 20, 2022
69 tasks
Copy link
Copy Markdown
Contributor

@plotnick plotnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is fantastic work. Modern password hashing is gobsmackingly complex, and this PR goes to great lengths to ensure that we're not falling into any of the many traps that a naive implementation would.

I found only two issues, maybe only one of which is a true blocker. The critical one is the test failure due to timing differences; I think we need to be reasonably sure that even a developer with a faster-than-expected machine (to within some reasonable limit) can always successfully run the tests.

The other is the issue of local_idp_user_set_password not doing a password hash if the user doesn't exist. This is the only timing bug I could find from pure code inspection, and it could be that it's not an important case for some reason, but it definitely jumped out.

My last question is a policy one: is a full second too long for the expected hashing time? It should only be user-visible during account setup, password changes, and login, but it seems a bit long to me, especially for login. I understand that any target time we pick will be a security trade-off, but suspect that something like 1/10, 1/2, or even 1/2 a second would still provide a pretty robust security margin while feeling significantly snappier to an end-user. Long ago I learned (or at least heard) that roughly 1/10 second was the "user-visible threshold", and that anything longer than that involved the user consciously waiting for a thing to happen. Might be a question for more UI/UX focused folks, but I wanted to raise it here. Definitely no objection to merging this with that timing target, though, if we think it's appropriate and justified.

Comment thread nexus/src/db/datastore/silo_user.rs
Comment thread nexus/src/db/datastore/silo_user.rs
Comment thread nexus/src/app/silo.rs
Comment thread nexus/tests/integration_tests/password_login.rs
@davepacheco
Copy link
Copy Markdown
Collaborator Author

Thanks for the review and for the feedback!

My last question is a policy one: is a full second too long for the expected hashing time? It should only be user-visible during account setup, password changes, and login, but it seems a bit long to me, especially for login. I understand that any target time we pick will be a security trade-off, but suspect that something like 1/10, 1/2, or even 1/2 a second would still provide a pretty robust security margin while feeling significantly snappier to an end-user. Long ago I learned (or at least heard) that roughly 1/10 second was the "user-visible threshold", and that anything longer than that involved the user consciously waiting for a thing to happen. Might be a question for more UI/UX focused folks, but I wanted to raise it here. Definitely no objection to merging this with that timing target, though, if we think it's appropriate and justified.

This is a good question. RFD 321 includes the determination that "We’ll choose parameters to target 750ms to 1s per password hash on production hardware." The footnote mentions "the OWASP-recommended parameters result in password hash times around 20ms - 30ms...various sources suggest that 500ms - 1s is a more appropriate target for secure systems." The OWASP times are more consistent with your suggestions. I basically searched around for guidance and found a few blog posts mentioning numbers in the range of 500ms - 1s, but unfortunately I didn't save any of those sources. I picked the high end mainly out of fear and caution. My intuition was that this was a reasonable time cost for login, and I think the Oxide console represents a more sensitive interface than an ordinary web app. Since it sounds like you don't feel strongly, I'd like to stick with this for now, but I'm open to revisiting it! (There should be no compatibility implications to changing this later, and that's part of why there are explicit tests for specific hashes here -- to make sure we could always verify hashes that were created with different parameters, as would happen if we changed our standard parameters.)

@david-crespo
Copy link
Copy Markdown
Contributor

Random driveby UX opinion: login happens infrequently enough (especially username/password login) that taking 1s doesn’t matter. I feel like when I click login on sites I am never disappointed if it takes some time.

Copy link
Copy Markdown
Contributor

@plotnick plotnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for tackling this, for considering my wacky ideas, and for the timing fix! test_local_users passes for me now.

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

Successfully merging this pull request may close these issues.

3 participants