update iam password generation #3989

jbardin · 2018-03-29T18:59:43Z

There are 2 major issues with the previous generatePassword() function:

It does not use a cryptographically-secure RNG, meaning that its output
is not guaranteed to be secret;
It is seeded only from the current time, making it very easy to bruteforce
the password for an adversary with (limited) information on the creation
time of the account.

resource_aws_iam_user_login_profile: Make uniformly random passwords

This maximises the entropy of the resulting password, given a fixed
charset and length; given that the charset contains 2*26 + 10 + 20
distinct charactes, using length-21 passwords results in password
entropy above 128 bits.

Check that generated IAM passwords pass any specified
iam.PasswordPolicy. IAM password policies cannot set specific numbers of each characters
classes, nor can the forbid specific characters. Since the policy
restrictions are minimal, rejection sampling will quickly find even the
shortest passwords containning all characters classes. This can be
extended to lookup the account policy, but in practice that is probably
not required. The only real unknown in this case is the minimum password
length, which will be validated when setting the password, and catching
it earlier won't do any good.

Update the validation for password length, since the minimum is 6
characters.

There are 2 major issues with the previous generatePassword() function: - It does not use a cryptographically-secure RNG, meaning that its output is not guaranteed to be secret; - It is seeded only from the current time, making it very easy to bruteforce the password for an adversary with (limited) information on the creation time of the account. resource_aws_iam_user_login_profile: Make uniformly random passwords This maximises the entropy of the resulting password, given a fixed charset and length; given that the charset contains 2*26 + 10 + 20 distinct charactes, using length-21 passwords results in password entropy above 128 bits.

KellerFuchs · 2018-03-29T19:57:05Z

@jbardin FWIW, Github has this nice feature where one can allow project maintainers to push to a PR's branch (and that box was ticked)

jbardin · 2018-03-29T20:00:52Z

@KellerFuchs, thanks for reminding me! I only moved it out of habit, and I need to remember that feature ;)

KellerFuchs · 2018-03-29T20:01:09Z

aws/resource_aws_iam_user_login_profile.go

+
+// Check the generated password contains all character classes listed in the
+// IAM password policy.
+func checkPwdPolicy(pass []byte) bool {


Is “the password contains all character classes” actually required by AWS?
If not, please do not enforce that, as it reduces password entropy.

We also need to consider the requirement that we need prevent breaking existing users configurations, where the profile running terraform may not have access to read the policy.

In practice this rejects very few samples, since the probability of having each of the 4 classes in a given random password is quite high.

If people feel strongly about this we can attempt to request the policy and fall back to this as the default.

KellerFuchs · 2018-03-29T20:02:59Z

aws/resource_aws_iam_user_login_profile.go

+	// Even in the worst case, this tends to take less than 10 tries to find a
+	// matching password. Any sufficiently long password is likely to succeed
+	// on the first try
+	for n := 0; n < 100000; n++ {


I would rather rename the function from the previous commit to generateOnePassword (or whatever is appropriate), add a datatype describing password policies, and make generatePassword take a policy argument.

KellerFuchs · 2018-03-29T20:10:00Z

aws/resource_aws_iam_user_login_profile_test.go

+func TestGenerateIAMPassword(t *testing.T) {
+	p := generatePassword(6)
+
+	if !(strings.ContainsAny(p, charLower) &&


Why is the code copied over from checkPwdPolicy?
Why do you expect checkPwdPolicy(generatePassword(6)) to be a useful test?

No, it doesn't check much. It was just a minor addition after verifying that we can generated the worst case password successfully.

KellerFuchs · 2018-03-29T20:20:12Z

Should generatePassword check whether the requested length and policy yield at least a (configurable?) level of entropy, and error-out if not?
Otherwise, you could easily run into the case where you want a 10 characters password, and the policy states it should have at least 4 lowercase characters, 3 uppercases, 3 digits, and 3 symbols, and run into the same results as the current code (minus the broken PRNG)

jbardin · 2018-03-29T20:50:19Z

@KellerFuchs,

The IAM PasswordPolicy has very few configuration options. The only options relevant to password generation are the boolean fields: RequireLowercaseCharacters, RequireUppercaseCharacters, RequireNumbers, and RequireSymbols, which correspond to the checks in the checkPwdPolicy function. There is no way to attempt to enforce number or placement of each type of character, so any randomly generated password by default matches all of these checks more often than not.

KellerFuchs · 2018-03-29T22:48:46Z

OK, I did not realise the options in the policy were Booleans, and not arbitrary numbers; should be Good Enough© then, though I would rather run the numbers on the entropy of generated passwords, but I am not doing that at 1am.

paultyng · 2018-03-30T00:32:51Z

aws/resource_aws_iam_user_login_profile_test.go

@@ -20,6 +21,17 @@ import (
 	"github.com/hashicorp/vault/helper/pgpkeys"
 )

+func TestGenerateIAMPassword(t *testing.T) {
+	p := generatePassword(6)
+


add a check here that it is actually 6 chars :)

Check that generated IAM passwords pass any specified iam.PasswordPolicy. IAM password policies cannot set specific numbers of each characters classes, nor can the forbid specific characters. Since the policy restrictions are minimal, rejection sampling will quickly find even the shortest passwords containing all characters classes. This can be extended to lookup the account policy, but in practice that is probably not required. The only real unknown in this case is the minimum password length, which will be validated when setting the password, and catching it earlier won't do any good. Update the validation for password length, since the minimum is 6 characters.

bflad · 2018-04-06T19:11:58Z

This has been released in version 1.14.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

KellerFuchs · 2018-04-14T11:46:32Z

@jbardin @paultyng Thanks for getting this finished while I was unavailable :)

FYI, I did a back-of-the-enveloppe computation, and the entropy of the generated passwords (with the policy) should be >= 6 bits/character, which isn't much lower than when generating without constraints (where it's ≃6.36b/char)

ghost · 2020-04-06T17:47:10Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

jbardin added the bug Addresses a defect in current functionality. label Mar 29, 2018

jbardin requested a review from paultyng March 29, 2018 18:59

ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 29, 2018

jbardin mentioned this pull request Mar 29, 2018

Fix password generation in resourceAwsIamUserLoginProfile #3934

Closed

bflad added the service/iam Issues and PRs that pertain to the iam service. label Mar 29, 2018

bflad added this to the v1.14.0 milestone Mar 29, 2018

KellerFuchs suggested changes Mar 29, 2018

View reviewed changes

paultyng suggested changes Mar 30, 2018

View reviewed changes

jbardin force-pushed the jbardin/password branch from 7ce1ea3 to 74334db Compare March 30, 2018 12:53

ghost added the size/M Managed by automation to categorize the size of a PR. label Mar 30, 2018

paultyng approved these changes Mar 30, 2018

View reviewed changes

jbardin added 2 commits March 30, 2018 11:30

add iam password policy test

78d50b1

rename functions to be more descriptive

e804aed

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/M Managed by automation to categorize the size of a PR. labels Mar 30, 2018

paultyng approved these changes Mar 30, 2018

View reviewed changes

jbardin merged commit efa8cd4 into master Mar 30, 2018

jbardin deleted the jbardin/password branch March 30, 2018 15:47

bflad added a commit that referenced this pull request Apr 2, 2018

Update CHANGELOG for #3989

9a3f6a3

danielcompton mentioned this pull request Jul 21, 2018

Use cryptographically secure RNG for random_string, or don't recommend them for passwords? hashicorp/terraform-provider-random#35

Closed

ghost locked and limited conversation to collaborators Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update iam password generation #3989

update iam password generation #3989

jbardin commented Mar 29, 2018

KellerFuchs commented Mar 29, 2018

jbardin commented Mar 29, 2018

KellerFuchs Mar 29, 2018 •

edited

Loading

jbardin Mar 29, 2018

KellerFuchs Mar 29, 2018

KellerFuchs Mar 29, 2018

jbardin Mar 29, 2018

KellerFuchs commented Mar 29, 2018

jbardin commented Mar 29, 2018

KellerFuchs commented Mar 29, 2018

paultyng Mar 30, 2018

bflad commented Apr 6, 2018

KellerFuchs commented Apr 14, 2018

ghost commented Apr 6, 2020

update iam password generation #3989

update iam password generation #3989

Conversation

jbardin commented Mar 29, 2018

KellerFuchs commented Mar 29, 2018

jbardin commented Mar 29, 2018

KellerFuchs Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

jbardin Mar 29, 2018

Choose a reason for hiding this comment

KellerFuchs Mar 29, 2018

Choose a reason for hiding this comment

KellerFuchs Mar 29, 2018

Choose a reason for hiding this comment

jbardin Mar 29, 2018

Choose a reason for hiding this comment

KellerFuchs commented Mar 29, 2018

jbardin commented Mar 29, 2018

KellerFuchs commented Mar 29, 2018

paultyng Mar 30, 2018

Choose a reason for hiding this comment

bflad commented Apr 6, 2018

KellerFuchs commented Apr 14, 2018

ghost commented Apr 6, 2020

KellerFuchs Mar 29, 2018 •

edited

Loading