Skip to content

fix: Increase length of generated random password from 10 to 20 #222

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

fix: Increase length of generated random password from 10 to 20 #222

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2356d45
fix: Increase length of generated random password from 10 to 20
ryboe May 18, 2021
f689207
Merge branch 'master' into master
antonbabenko Sep 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

lets just turn this into a variable with the length that defaults to 10. something like var.password_length

Copy link
Author

@ryboe ryboe May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

  1. its not for us to dictate to users what their password length should be
  2. 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

Copy link
Author

@ryboe ryboe May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

  1. We're currently dictating a password length of 10. We're currently dictating that the password is cryptographically random.
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

special = false
}

Expand Down