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: add Argon2 calibration CLI #803

Merged
merged 26 commits into from
Nov 6, 2020
Merged

feat: add Argon2 calibration CLI #803

merged 26 commits into from
Nov 6, 2020

Conversation

zepatrik
Copy link
Member

@zepatrik zepatrik commented Nov 2, 2020

Related issue

closes #723
closes #572
closes #647

Proposed changes

This adds a CLI that conducts measurements to allow choosing the best argon2 parameters.

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
    security@ory.sh) 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 changed the title Argon2 value cli feat: add Argon2 calibration CLI Nov 2, 2020
@zepatrik zepatrik marked this pull request as ready for review November 2, 2020 15:20
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.

This looks great already! I have some ideas for improvement! :)

cmd/hashers/argon2/calibrate.go Outdated Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Outdated Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Outdated Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Outdated Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Outdated Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Outdated Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Outdated Show resolved Hide resolved
cmd/hashers/argon2/calibrate.go Outdated Show resolved Hide resolved
@zepatrik
Copy link
Member Author

zepatrik commented Nov 3, 2020

Do you think we should ditch the --verbose flag? I would either remove it or make it a quiet flag. Users will be confused if there is no output for a minute when they just try to run the command.

@aeneasr
Copy link
Member

aeneasr commented Nov 3, 2020

I think also that a quiet flag makes more sense!

@zepatrik zepatrik requested a review from aeneasr November 4, 2020 13:14
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.

Great! Almost done! We need a page in the docs (maybe in guide?) which explains: "Choosing Argon2 Parameters". You could probably re-use the blog post!

@aeneasr
Copy link
Member

aeneasr commented Nov 5, 2020

Note to self:

Commit message:

feat: add helper for choosing argon2 parameters

This patch adds the new command "hashers argon2 calibrate" which allows one to pick the desired hashing time for password hashing and then chooses the optimal parameters for the hardware the command is running on:

```
$ kratos hashers argon2 calibrate 500ms
Increasing memory to get over 500ms:
    took 2.846592732s in try 0
    took 6.006488824s in try 1
  took 4.42657975s with 4.00GB of memory
[...]
Decreasing iterations to get under 500ms:
    took 484.257775ms in try 0
    took 488.784192ms in try 1
  took 486.534204ms with 3 iterations
Settled on 3 iterations.

{
  "memory": 1048576,
  "iterations": 3,
  "parallelism": 32,
  "salt_length": 16,
  "key_length": 32
}
```

@zepatrik
Copy link
Member Author

zepatrik commented Nov 5, 2020

Did you see the page I added in the troubleshooting/debug section?

docs/docs/debug/csrf.mdx Show resolved Hide resolved
docs/docs/debug/argon2.md Outdated Show resolved Hide resolved
docs/docs/debug/argon2.md Outdated Show resolved Hide resolved
docs/docs/debug/argon2.md Outdated Show resolved Hide resolved
docs/docs/debug/argon2.md Outdated Show resolved Hide resolved
docs/docs/debug/argon2.md Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Nov 5, 2020

Did you see the page I added in the troubleshooting/debug section?

I actually missed that and that page is very important. I still think it would make sense to have a guide for this also as it probably makes sense to first understand how to choose those parameters before the server crashes :)

1 similar comment
@aeneasr
Copy link
Member

aeneasr commented Nov 5, 2020

Did you see the page I added in the troubleshooting/debug section?

I actually missed that and that page is very important. I still think it would make sense to have a guide for this also as it probably makes sense to first understand how to choose those parameters before the server crashes :)

@zepatrik zepatrik requested a review from aeneasr November 5, 2020 12:54
docs/docs/debug/argon2.md Outdated Show resolved Hide resolved
@@ -90,7 +97,7 @@
]
}
],
"Debug & Help": ["debug/csrf"],
"Debug & Help": ["debug/csrf", "debug/argon2"],
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect ID

Copy link
Member Author

Choose a reason for hiding this comment

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

damn, always forget the sidebar 🤦

docs/sidebar.json Show resolved Hide resolved
@aeneasr aeneasr merged commit ca5a69b into master Nov 6, 2020
@aeneasr aeneasr deleted the argon2-value-cli branch November 6, 2020 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants