Switch password hashing algorithm to argon2#2608
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v7.0.0 #2608 +/- ##
==================================================
- Coverage 79.15% 78.10% -1.05%
==================================================
Files 661 473 -188
Lines 51740 33660 -18080
Branches 735 735
==================================================
- Hits 40953 26289 -14664
+ Misses 10707 7291 -3416
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return true if @@session_cache and (time - @@session_cache_time) < SESSION_CACHE_TIMEOUT and @@session_cache[token] | ||
| token_hash = hash(token) | ||
| return true if @@token_cache and (time - @@token_cache_time) < TOKEN_CACHE_TIMEOUT and @@token_cache == token_hash | ||
| unless no_password |
There was a problem hiding this comment.
Missing this unless no_password is actually a bug in my last PR
| return false | ||
| # Check stored password hash | ||
| pw_hash = Store.get(PRIMARY_KEY) | ||
| raise "invalid password hash" unless pw_hash.start_with?("$argon2") # Catch users who didn't run the migration utility when upgrading to COSMOS 7 |
There was a problem hiding this comment.
This triggers an alert on the login page that tells them to look at the migration guide, which tells them to run the password migration utility
| @@session_cache = nil | ||
| @@session_cache_time = nil |
There was a problem hiding this comment.
sessions_cache typo was a bug that allowed sessions to persist for 5min after logging out
since github runners suck
This reverts commit 27e7e06.
|
Same as my last PR, only remaining test failure is pre-existing on v7 branch since we merged main |
| class AuthModel | ||
| PRIMARY_KEY = 'OPENC3__TOKEN' | ||
| SESSIONS_KEY = 'OPENC3__SESSIONS' | ||
| ARGON2_PROFILE = ENV["OPENC3_ARGON2_PROFILE"]&.to_sym || :rfc_9106_high_memory # More secure than default (:rfc_9106_low_memory) |
There was a problem hiding this comment.
The argon2 gem defaults to rfc_9106_low_memory, but we probably usually want to use rfc_9106_high_memory since it's the most secure built-in profile. This environment variable lets people override that with a less secure but faster profile (e.g. for github runners)
clayandgen
left a comment
clayandgen
left a comment
There was a problem hiding this comment.
Tested and validated 👍
Only comment I'd make, is maybe in the migration documentation, mention that the OPENC3_API_PASSWORD can be in the .env as well -- I think most users won't want to type the API Password on the command line
| s.add_runtime_dependency 'rubyzip', '~> 3.0' | ||
| s.add_runtime_dependency 'uuidtools', '~> 2.2' | ||
| s.add_runtime_dependency 'yard', '~> 0.9' | ||
| s.add_runtime_dependency 'argon2', '~> 2.3' |
There was a problem hiding this comment.
@ryanmelt if the dependency is here does it also have to be the Gemfile? I think no because of the gemspec line in the Gemfile. But maybe the Gemfile has support the >= 2.3.2 and that's why it's in both? I also see that tzinfo-data is in both ... is that an oversight? We should probably add some comments one way or another.
3 participants
This change in practice:
OPENC3_API_PASSWORD=password openc3.sh cli migratepassword