aws/credential: Increase credential_process provider default buffer size

[bodhi]

Thanks @YakDriver and everyone who got #2217 merged!

I tried out this feature with our internal auth delegation tool, but it didn't work out of the box, even though the AWS CLI works fine, because the session token returned by the AWS API is much longer (at 560 bytes) than the code expects.

I note that DefaultBufSize is exported, so it could be set larger in client code, but it seems a bit unfriendly to me that the default size doesn't accept data that is being returned from the AWS API.

Copy of commit message:

The default buffer size of 512 bytes is not long enough to contain credentials with long session tokens, like those returned by sts.STS.AssumeRoleWithWebIdentity, which appears to return session tokens of ~560 bytes.

Using 560 bytes as a starting point, with the rest of the credential data being ~160-170 bytes, 1024 bytes will fit these, without being excessive.

[jasdel]

Thanks for the feedback @bodhi. A 1024 default buffer seems reasonable. We can do more research and see if there is a more appropriate size.

[bodhi]

@jasdel thanks!

I note that DefaultBufSize is exported, so it could be set larger in client code

One of my team-mates just pointed out to me that DefaultBufSize is const, not var 🙀 .

[YakDriver]

@jasdel 512 is ultra conservative 😞 . In my opinion, even 2048 would be safe and accommodate most situations.

@bodhi DefaultBufSize is a const, as it should be because it's a default. However, MaxBufSize is exported and can be set programmatically for an individual ProcessProvider. (This doesn't help much if, like us, there's a layer or two between you and the aws-sdk-go and you need it to work out of the box.)

    creds := processcreds.NewCredentials(
        "/path/to/command",
        func(opt *ProcessProvider) {
            opt.Timeout = time.Duration(2) * time.Minute
            opt.Duration = time.Duration(60) * time.Minute
            opt.MaxBufSize = 2048
        })

[YakDriver]

Shared config has been tested already so no need to add another profile.

[YakDriver]

Shared config has been tested already so no need to add another profile.

[YakDriver]

Instead of using a profile and shared config file, this can be tested more directly:

creds := processcreds.NewCredentials(
		fmt.Sprintf(
			"%s %s",
			getOSCat(),
			strings.Join(
				[]string{"testdata", "longsessiontoken.json"},
				string(os.PathSeparator))))
	_, err := creds.Get()
	if err != nil {
		t.Errorf("expected %v, got %v", "no error", err)
	}

This way you can get rid of the two changes to the shared config files.

[mikkeloscar]

I also hit this issue now when testing with https://github.com/mikkeloscar/kube-aws-iam-controller

[bodhi]

@YakDriver I just updated the tests to not use the profile configuration. It's done as a separate commit, but I'm fine to squash them if that's preferred.

[jasdel]

Separate commit is fine, the PR will be squashed when its merged into master so no worries about the separate commits in the PR.

[YakDriver]

LGTM! 👍

[jasdel]

Thanks for putting this fix together. The change looks good.