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

feat: hasher load testing CLI #961

Merged
merged 38 commits into from
Jan 15, 2021
Merged

feat: hasher load testing CLI #961

merged 38 commits into from
Jan 15, 2021

Conversation

zepatrik
Copy link
Member

@zepatrik zepatrik commented Jan 5, 2021

Related issue

Closes #955

Proposed changes

This adds a load testing CLI that allows to adjust the hasher parameters under simulated load.

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

@zepatrik zepatrik marked this pull request as ready for review January 13, 2021 10:29
@zepatrik zepatrik requested a review from aeneasr January 13, 2021 11:36
cmd/hashers/argon2/loadtest.go Outdated Show resolved Hide resolved
cmd/hashers/argon2/root.go Outdated Show resolved Hide resolved
ViperKeyPasswordMaxBreaches = "password.max_breaches"
ViperKeyIgnoreNetworkErrors = "password.ignore_network_errors"
ViperKeyVersion = "version"
Argon2DefaultMemory uint32 = 4 * 1024 * 1024
Argon2DefaultIterations uint32 = 4
Argon2DefaultMemory = uint32(512 * bytesize.MB / bytesize.KB)
Copy link
Member

Choose a reason for hiding this comment

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

I think the default should be 64MB or 128MB as most instances of Kratos are deployed in a docker container with ~1GB of ram available.

Suggested change
Argon2DefaultMemory = uint32(512 * bytesize.MB / bytesize.KB)
Argon2DefaultMemory = uint32(128 * bytesize.MB / bytesize.KB)

Copy link
Member Author

Choose a reason for hiding this comment

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

Here the question remains: should we provide defaults that are secure or that will definitely work in every environment? Are such small docker containers considered for production environments? In a dev environment you will likely not have concurrent login requests. I went for 512MB as it should be always available (for non-concurrent settings at least).
Libsodium uses 256MB as the moderate level. Their guidelines are not very helpful as well and contradict the argon2 paper (recommend at least 3 iterations).
In my opinion the default should work with dev environments and be high enough to guarantee security. It is not possible to find good default values that work for everyone IMO. People will always have to calibrate and test different values for their requirements.

Copy link
Member

Choose a reason for hiding this comment

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

Libsodium uses 256MB as the moderate level.

From libsodium:

For interactive, online operations, crypto_pwhash_OPSLIMIT_INTERACTIVE and crypto_pwhash_MEMLIMIT_INTERACTIVE provide base line for these two parameters. This currently requires 64 MiB of dedicated RAM. Higher values may improve security (see below).

Alternatively, crypto_pwhash_OPSLIMIT_MODERATE and crypto_pwhash_MEMLIMIT_MODERATE can be used. This requires 256 MiB of dedicated RAM, and takes about 0.7 seconds on a 2.8 Ghz Core i7 CPU.

Most docker containers get about 128-256 MB ram and about 0.2-0.5 CPUs unless it's a Java application.

Their guidelines are not very helpful as well and contradict the argon2 paper (recommend at least 3 iterations).

This recommendation is for Argon2i, not Argon2id - therefore not contradicting imo:

opslimit, the number of passes, has to be at least 3 when using Argon2i.crypto_pwhash() and crypto_pwhash_str() will fail with a -1 return code for lower values.

I think it is unrealistic to scale a user system with 512MB ram per password hash. The value used here will be the default value for all users until the admin changes suddenly realizes the cost. It is then not easily possible to retrospectively change the RAM use of the existing hashes, meaning that the "old" hashes still use 512MB. It is therefore sensible to set something that reflects the real world. libsodium, a very popular and well maintained library, sets this to 64MB for online services. If we go with that, we will not be wrong, and we can still do a "bit more security" with 128MB.

Keep in mind that yes, making stuff expensive is the goal of password hashing, but

  • noone was ever able to find SHA-3 collisions as far as I know - and Argon2 is aeons more expensive than SHA familiy algorithms (BCrypt, Scrypt, PKBDF2 all are much more expensive).
  • security must be economical at scale

Copy link
Member Author

Choose a reason for hiding this comment

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

Collisions are not the main concern in my opinion (and increasing memory will not prevent collisions as well). It is more about preventing an attacker to bute force the clear text passwords when they happen to get access to the hashes.
I just assumed that production systems are way to different to have a somehow reliable default value. Setting this to a rather high value would force people to actually go and determine sensible values (instead of using defaults). We can also easily add a parameter update functionality in the hasher that recomputes the hash with the new parameters after a successful check and then overwrites it in the database (lazy computation after the request is handled). I will open an issue for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

The contradiction I was referring to was in

If this it is way too long for your application, reduce memlimit, but keep opslimit set to 3.

Their default "interactive" values are 64MB, 2 iterations

I would rather use more memory and less iterations (as all other best practices suggest). Therefore 128MB, 1 iteration should be good (and take about the same time).

driver/config/provider_viper.go Outdated Show resolved Hide resolved
driver/config/provider_viper.go Outdated Show resolved Hide resolved
.schema/config.schema.json Show resolved Hide resolved
.schema/config.schema.json Show resolved Hide resolved
zepatrik and others added 11 commits January 13, 2021 13:52
BREAKING: hashers.argon2.memory is now a string with unit
# Conflicts:
#	.schema/api.swagger.json
#	cmd/hashers/argon2/calibrate.go
#	driver/config/config.go
#	driver/config/config_test.go
#	hash/hasher_argon2.go
#	internal/httpclient/models/complete_self_service_login_flow_with_password_method.go
#	internal/httpclient/models/complete_self_service_recovery_flow_with_link_method.go
#	internal/httpclient/models/complete_self_service_settings_flow_with_password_method.go
#	internal/httpclient/models/complete_self_service_verification_flow_with_link_method.go
#	internal/httpclient/models/create_identity.go
#	internal/httpclient/models/create_recovery_link.go
#	internal/httpclient/models/generic_error_payload.go
#	internal/httpclient/models/id.go
#	internal/httpclient/models/identity.go
#	internal/httpclient/models/login_flow_method_config.go
#	internal/httpclient/models/message.go
#	internal/httpclient/models/messages.go
#	internal/httpclient/models/recovery_address.go
#	internal/httpclient/models/recovery_address_type.go
#	internal/httpclient/models/recovery_flow_method_config.go
#	internal/httpclient/models/revoke_session.go
#	internal/httpclient/models/session.go
#	internal/httpclient/models/state.go
#	internal/httpclient/models/traits.go
#	internal/httpclient/models/update_identity.go
#	internal/httpclient/models/verifiable_address.go
#	internal/httpclient/models/verifiable_address_status.go
#	internal/httpclient/models/verifiable_address_type.go
#	internal/httpclient/models/verification_flow_method.go
#	internal/httpclient/models/version.go
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Please test the quickstart - I assume it might not be working due to the memory changes. Please also provide a breaking change that follows conventional commit syntax and explain in the breaking change what the user has to do in order to upgrade!

@zepatrik
Copy link
Member Author

BREAKING CHANGE: The configuration value for hashers.argon2.memory is now a string representation of the memory amount including the unit of measurement. To convert the value divide your current setting (KB) by 1024 to get a result in MB or 1048576 to get a result in GB. Example: 131072 would now become 128MB.

@aeneasr aeneasr changed the base branch from master to v0.6 January 15, 2021 09:30
@aeneasr aeneasr merged commit ba6d5b2 into v0.6 Jan 15, 2021
@aeneasr aeneasr deleted the improve-argon2-value-cli branch January 15, 2021 09:30
@zepatrik
Copy link
Member Author

Should I update the blog post as well?

aeneasr added a commit that referenced this pull request Jan 25, 2021
This adds a load testing CLI that allows to adjust the hasher parameters under simulated load.

Closes #955

BREAKING CHANGE: The configuration value for `hashers.argon2.memory` is now a string representation of the memory amount including the unit of measurement. To convert the value divide your current setting (KB) by 1024 to get a result in MB or 1048576 to get a result in GB. Example: `131072` would now become `128MB`.

Co-authored-by: aeneasr <[email protected]>
Co-authored-by: aeneasr <[email protected]>
aeneasr added a commit that referenced this pull request Mar 9, 2021
This adds a load testing CLI that allows to adjust the hasher parameters under simulated load.

Closes #955

BREAKING CHANGE: The configuration value for `hashers.argon2.memory` is now a string representation of the memory amount including the unit of measurement. To convert the value divide your current setting (KB) by 1024 to get a result in MB or 1048576 to get a result in GB. Example: `131072` would now become `128MB`.

Co-authored-by: aeneasr <[email protected]>
Co-authored-by: aeneasr <[email protected]>
aeneasr added a commit that referenced this pull request Apr 9, 2021
This adds a load testing CLI that allows to adjust the hasher parameters under simulated load.

Closes #955

BREAKING CHANGE: The configuration value for `hashers.argon2.memory` is now a string representation of the memory amount including the unit of measurement. To convert the value divide your current setting (KB) by 1024 to get a result in MB or 1048576 to get a result in GB. Example: `131072` would now become `128MB`.

Co-authored-by: aeneasr <[email protected]>
Co-authored-by: aeneasr <[email protected]>
aeneasr added a commit that referenced this pull request Apr 9, 2021
This adds a load testing CLI that allows to adjust the hasher parameters under simulated load.

Closes #955

BREAKING CHANGE: The configuration value for `hashers.argon2.memory` is now a string representation of the memory amount including the unit of measurement. To convert the value divide your current setting (KB) by 1024 to get a result in MB or 1048576 to get a result in GB. Example: `131072` would now become `128MB`.

Co-authored-by: aeneasr <[email protected]>
Co-authored-by: aeneasr <[email protected]>
aeneasr added a commit that referenced this pull request Apr 9, 2021
This adds a load testing CLI that allows to adjust the hasher parameters under simulated load.

Closes #955

BREAKING CHANGE: The configuration value for `hashers.argon2.memory` is now a string representation of the memory amount including the unit of measurement. To convert the value divide your current setting (KB) by 1024 to get a result in MB or 1048576 to get a result in GB. Example: `131072` would now become `128MB`.

Co-authored-by: aeneasr <[email protected]>
Co-authored-by: aeneasr <[email protected]>
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.

Improve Argon2 helpers and defaults and documentation
2 participants