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

update iam password generation #3989

Merged
merged 4 commits into from
Mar 30, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
81 changes: 55 additions & 26 deletions aws/resource_aws_iam_user_login_profile.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
package aws

import (
"bytes"
"crypto/rand"
"fmt"
"log"
"math/rand"
"time"
"math/big"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
Expand Down Expand Up @@ -40,7 +41,7 @@ func resourceAwsIamUserLoginProfile() *schema.Resource {
Type: schema.TypeInt,
Optional: true,
Default: 20,
ValidateFunc: validation.IntBetween(4, 128),
ValidateFunc: validation.IntBetween(5, 128),
},

"key_fingerprint": {
Expand All @@ -55,35 +56,62 @@ func resourceAwsIamUserLoginProfile() *schema.Resource {
}
}

// generatePassword generates a random password of a given length using
// characters that are likely to satisfy any possible AWS password policy
// (given sufficient length).
const (
charLower = "abcdefghijklmnopqrstuvwxyz"
charUpper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
charNumbers = "0123456789"
charSymbols = "!@#$%^&*()_+-=[]{}|'"
)

// generatePassword generates a random password of a given length, matching the
// most restrictive iam password policy.
func generatePassword(length int) string {
charsets := []string{
"abcdefghijklmnopqrstuvwxyz",
"ABCDEFGHIJKLMNOPQRSTUVWXYZ",
"012346789",
"!@#$%^&*()_+-=[]{}|'",
}
const charset = charLower + charUpper + charNumbers + charSymbols

result := make([]byte, length)
charsetSize := big.NewInt(int64(len(charset)))

// rather than trying to artifically add specific characters from each
// class to the password to match the policy, we generate passwords
// randomly and reject those that don't match.
//
// 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++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

for i := range result {
r, err := rand.Int(rand.Reader, charsetSize)
if err != nil {
panic(err)
}
if !r.IsInt64() {
panic("rand.Int() not representable as an Int64")
}

result[i] = charset[r.Int64()]
}

// Use all character sets
random := rand.New(rand.NewSource(time.Now().UTC().UnixNano()))
components := make(map[int]byte, length)
for i := 0; i < length; i++ {
charset := charsets[i%len(charsets)]
components[i] = charset[random.Intn(len(charset))]
if !checkPwdPolicy(result) {
continue
}

return string(result)
}

// Randomise the ordering so we don't end up with a predictable
// lower case, upper case, numeric, symbol pattern
result := make([]byte, length)
i := 0
for _, b := range components {
result[i] = b
i = i + 1
panic("failed to generate acceptable password")
}

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

@KellerFuchs KellerFuchs Mar 29, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

if !(bytes.ContainsAny(pass, charLower) &&
bytes.ContainsAny(pass, charNumbers) &&
bytes.ContainsAny(pass, charSymbols) &&
bytes.ContainsAny(pass, charUpper)) {
return false
}

return string(result)
return true
}

func resourceAwsIamUserLoginProfileCreate(d *schema.ResourceData, meta interface{}) error {
Expand Down Expand Up @@ -114,6 +142,7 @@ func resourceAwsIamUserLoginProfileCreate(d *schema.ResourceData, meta interface
}

initialPassword := generatePassword(passwordLength)

fingerprint, encrypted, err := encryption.EncryptValue(encryptionKey, initialPassword, "Password")
if err != nil {
return err
Expand Down
12 changes: 12 additions & 0 deletions aws/resource_aws_iam_user_login_profile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,18 @@ import (
"github.com/hashicorp/vault/helper/pgpkeys"
)

func TestGenerateIAMPassword(t *testing.T) {
p := generatePassword(6)
if len(p) != 6 {
t.Fatalf("expected a 6 character password, got: %q", p)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

p = generatePassword(128)
if len(p) != 128 {
t.Fatalf("expected a 128 character password, got: %q", p)
}
}

func TestAccAWSUserLoginProfile_basic(t *testing.T) {
var conf iam.GetLoginProfileOutput

Expand Down