fix: Increase length of generated random password from 10 to 20 #222
Conversation
When the user passes create_random_password = true, we're currently generating a 10 char alphanumeric password (no special characters). That's *much* too short. It opens the db to brute force password cracking. This commit increases the password length to 20, which will firmly shut the door on this attack method.
| @@ -21,7 +21,7 @@ data "aws_partition" "current" {} | |||
| resource "random_password" "master_password" { | |||
| count = var.create_cluster && var.create_random_password ? 1 : 0 | |||
|
|
|||
| length = 10 | |||
| length = 20 | |||
There was a problem hiding this comment.
lets just turn this into a variable with the length that defaults to 10. something like var.password_length
There was a problem hiding this comment.
Why give people a footgun to shoot themselves with? Do you want to allow people to set shorter passwords than 10 chars? Fewer variables == a better API.
There's no cost to having a long password, unless someone is trying to memorize the password instead of storing it in a secure place 😱. That's crazy in 2021.
A 20 char random password is good enough for everybody. It's long enough to resist brute force attacks from password crackers for at least a century.
There was a problem hiding this comment.
- its not for us to dictate to users what their password length should be
- this change as its written will be a breaking change, essentially rotating the DB master password for all users of the module
I agree in principle that a longer password is a better posture, but we try to not break things for users nor dictate how they should setup their resources. we try to balance between providing a sane set of defaults and good documentation/examples to help them get up and running quickly, but also allowing them flexibility to customize as it suites their setup
There was a problem hiding this comment.
- We're currently dictating a password length of 10. We're currently dictating that the password is cryptographically random.
- The breaking change is a big consideration. I think fixing this insecure default justifies bumping the major version to 6.0.0. If people aren't using version constraints, then they really can't complain about a change like this.
There was a problem hiding this comment.
@bryantbiggs I think the master password is only used during the creation of the DB and not verified during updates, so it is safe to change it and it won't break anything for existing users except recreation of the random_password resource itself. Right?
There was a problem hiding this comment.
@antonbabenko I am fairly certain it will force the update. The length change will trigger the resource update for the random_password which means the input for the password of the cluster will be different and show up as a diff and will get updated on the next apply. should be easy to confirm with the examples
@ryboe this is what we have for the RDS module, it should be a variable https://github.com/terraform-aws-modules/terraform-aws-rds/blob/66336d9c489c4e318799219a7ec5584efdae70be/main.tf#L15
There was a problem hiding this comment.
It's your call, but I still think it should not be a variable. APIs with no knobs are generally better than APIs with lots of knobs. If the rds module exposes it, then it's making a mistake too. The only thing var.password_length gives the user is the opportunity to misconfigure the db.
There was a problem hiding this comment.
What is the status of this PR, is it abandoned? At the moment I "workaround" this by creating my own random_password resource of 32 symbol length and password = random_password.aurora_master_password.result
|
I think we can merge this one and bump a major release (due to a breaking change discussed in the comments). Maybe there are some other PRs with breaking changes we can merge as well? @bryantbiggs What do you think? |
|
I don't think it needs to be a breaking change if we just parameterize this value with a variable, set the default to the current hard coded value of 10 (this avoids the breaking change) and then users are free to set the length they desire. @tehleet just commented that they are setting 32 characters - why not let users decide the length they desire? |
|
Even better! :) @ryboe please update and we merge it right away. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
This commit increases the generated password length to 20.
Motivation and Context
When the user passes
create_random_password = true, we're currently generating a 10 char alphanumeric password (no special characters). That's much too short. It opens the db to brute force password cracking. Increasing the length to 20 will firmly shut the door on brute force password cracking attempts.Breaking Changes
N/A
How Has This Been Tested?