-
Notifications
You must be signed in to change notification settings - Fork 152
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
AWS IAM role: Generate temporary security credentials from Role #350
Conversation
pkg/restic/restic_test.go
Outdated
{ | ||
profile: ¶m.Profile{ | ||
Location: v1alpha1.Location{ | ||
Type: v1alpha1.LocationTypeS3Compliant, | ||
Endpoint: "endpoint", // Also remove all of the trailing slashes | ||
}, | ||
Credential: param.Credential{ | ||
Type: param.CredentialTypeSecret, | ||
Secret: &v1.Secret{ | ||
Type: "secrets.kanister.io/aws", | ||
Data: map[string][]byte{ | ||
"access_key_id": []byte("id"), | ||
"secret_access_key": []byte("secret"), | ||
"session_token": []byte("token"), | ||
}, | ||
}, | ||
}, | ||
}, | ||
repo: "repo", | ||
password: "my-secret", | ||
expected: []string{ | ||
"export AWS_ACCESS_KEY_ID=id\n", | ||
"export AWS_SECRET_ACCESS_KEY=secret\n", | ||
"export AWS_SESSION_TOKEN=token\n", | ||
"export RESTIC_REPOSITORY=s3:endpoint/repo\n", | ||
"export RESTIC_PASSWORD=my-secret\n", | ||
"restic", | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding new test case in a followup commit!
{ | ||
secret: &v1.Secret{ | ||
Type: v1.SecretType(AWSSecretType), | ||
Data: map[string][]byte{ | ||
AWSAccessKeyID: []byte("key_id"), | ||
AWSSecretAccessKey: []byte("secret_key"), | ||
AWSSessionToken: []byte("session_token"), | ||
}, | ||
}, | ||
expected: &credentials.Value{ | ||
AccessKeyID: "key_id", | ||
SecretAccessKey: "secret_key", | ||
SessionToken: "session_token", | ||
}, | ||
errChecker: IsNil, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: add unit test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added unit test below.
pkg/config/aws/aws.go
Outdated
if err != nil { | ||
return nil, "", "", errors.Wrap(err, "Failed to get temporary security credentials") | ||
} | ||
return config, region, role, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to return role here even after assuming role?
b96630d
to
a26dbd9
Compare
@@ -33,23 +36,39 @@ const ( | |||
SecretAccessKey = "AWS_SECRET_ACCESS_KEY" | |||
// SessionToken represents AWS Session Key | |||
SessionToken = "AWS_SESSION_TOKEN" | |||
|
|||
assumeRoleDuration = 25 * time.Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Blockstorage 👆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assumption being that we'll automatically renew the blockstorage creds, right?
// ConfigRole represents the key for the ARN of the role which can be assumed. | ||
// It is optional. | ||
ConfigRole = "role" | ||
assumeRoleDuration = 90 * time.Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Restic and Kando 👆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include examples of running the tests you changes, especailly the ones that won't get run in CI due to the AWS key requirement.
pkg/config/aws/aws.go
Outdated
region, ok := config[ConfigRegion] | ||
if !ok { | ||
return nil, "", "", errors.New("region required for storage type EBS") | ||
return nil, "", errors.New("region required for storage type EBS") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this EBS specific?
|
||
// SwitchRole assumes role with given credentials. | ||
// Temporary credentials will be valid for 'duration'. | ||
func SwitchRole(ctx context.Context, accessKeyID string, secretAccessKey string, role string, duration time.Duration) (*credentials.Credentials, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't really describe the behavior. creds Is a creds provider that will automatically refresh credentials.
Co-Authored-By: Thomas Manville <tom@kasten.io>
Co-Authored-By: Thomas Manville <tom@kasten.io>
Updated the commit description with test results. |
"gopkg.in/check.v1" | ||
) | ||
|
||
func GetEnvOrSkip(c *check.C, varName string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this method already defined in testutil?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdmanv Yes, It creates a cycle if I import pkg/testutil
, hence I added the function to helpers.go
Also, this func is defined in 4 different places in kanister 😄 . Once this PR is merged, I can push another followup PR to remove the duplicate funcs and import only from helpers.go
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that works for now
) | ||
|
||
// GetConfig returns a configuration to establish AWS connection, connected region name and the role to assume if it exists. | ||
func GetConfig(config map[string]string) (awsConfig *aws.Config, region string, role string, err error) { | ||
func GetConfig(ctx context.Context, config map[string]string) (awsConfig *aws.Config, region string, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doc might also need update. It doesn't return the role but assumes it if it exists in config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment!
@@ -33,23 +36,39 @@ const ( | |||
SecretAccessKey = "AWS_SECRET_ACCESS_KEY" | |||
// SessionToken represents AWS Session Key | |||
SessionToken = "AWS_SESSION_TOKEN" | |||
|
|||
assumeRoleDuration = 25 * time.Minute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assumption being that we'll automatically renew the blockstorage creds, right?
@@ -272,6 +272,7 @@ func (s *BlockStorageProviderSuite) getConfig(c *C, region string) map[string]st | |||
} | |||
config[awsconfig.AccessKeyID] = accessKey | |||
config[awsconfig.SecretAccessKey] = secretAccessKey | |||
config[awsconfig.ConfigRole] = os.Getenv(awsconfig.ConfigRole) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdmanv this enables the tests with AWS assume role BUT it fails for me locally with the same error as we saw with before for storage_tests INFO[0000] Failed to create volume error="UnauthorizedOperation: You are not authorized to perform this operation. Encoded authorization failure message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested successfully with different role.
awsAccessKeyID := os.Getenv("AWS_ACCESS_KEY_ID") | ||
awsSecretAccessKey := os.Getenv("AWS_SECRET_ACCESS_KEY") | ||
var awsSessionToken string | ||
if role, ok := os.LookupEnv(aws.ConfigRole); ok { | ||
creds, err := aws.SwitchRole(ctx, awsAccessKeyID, awsSecretAccessKey, role, assumeRoleDuration) | ||
c.Check(err, IsNil) | ||
val, err := creds.Get() | ||
c.Check(err, IsNil) | ||
awsAccessKeyID = val.AccessKeyID | ||
awsSecretAccessKey = val.SecretAccessKey | ||
awsSessionToken = val.SessionToken | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tdmanv With these changes, We run objectstore tests with AWS IAM role, one of the 8 tests fails with my credentials:
go test
----------------------------------------------------------------------
FAIL: objectstore_test.go:423: ObjectStoreProviderSuite.TestBucketGetRegions
objectstore_test.go:429:
c.Check(err, IsNil)
... value *errors.withStack = Containers, listing the buckets: AccessDenied: Access Denied
status code: 403, request id: FC39088EE1A52416, host id: g9XaZ2ITdRbK4rDjjR+MCkv8sGlS8lLvubXbq4jnOZRyB/CQU7e0Gk/hyKW86wvffTo4EcQjO4M= ("Containers, listing the buckets: AccessDenied: Access Denied\n\tstatus code: 403, request id: FC39088EE1A52416, host id: g9XaZ2ITdRbK4rDjjR+MCkv8sGlS8lLvubXbq4jnOZRyB/CQU7e0Gk/hyKW86wvffTo4EcQjO4M=")
objectstore_test.go:439:
c.Check(err, IsNil)
... value *errors.withStack = Containers, listing the buckets: AccessDenied: Access Denied
status code: 403, request id: B02BD08131EADD44, host id: UTVEUGHfLkhv6ZUs07OHvc8D74LSMMT4sw5ZBQecbjkqVtf55jteT3+TSytkhxgQ1yHGBe6LNK4= ("Containers, listing the buckets: AccessDenied: Access Denied\n\tstatus code: 403, request id: B02BD08131EADD44, host id: UTVEUGHfLkhv6ZUs07OHvc8D74LSMMT4sw5ZBQecbjkqVtf55jteT3+TSytkhxgQ1yHGBe6LNK4=")
objectstore_test.go:439:
c.Check(err, IsNil)
... value *errors.withStack = Containers, listing the buckets: AccessDenied: Access Denied
status code: 403, request id: A34CE00D4BC213E7, host id: BtgIfwhLgvf+WCZDOvi6jG8zg63XVaHp1f4jUn8UL0qUMHyuFDTJhqqB4wXoiwnooF0NSy90zfE= ("Containers, listing the buckets: AccessDenied: Access Denied\n\tstatus code: 403, request id: A34CE00D4BC213E7, host id: BtgIfwhLgvf+WCZDOvi6jG8zg63XVaHp1f4jUn8UL0qUMHyuFDTJhqqB4wXoiwnooF0NSy90zfE=")
objectstore_test.go:439:
c.Check(err, IsNil)
... value *errors.withStack = Containers, listing the buckets: AccessDenied: Access Denied
status code: 403, request id: 9017B3A3E89EA391, host id: 6+JXjooSmq1FMlitzW787EsZSz8ktC3DWfvC64wRvsvBhrgvkKi9ikCgi2PpqR4KbeblH6KAq4w= ("Containers, listing the buckets: AccessDenied: Access Denied\n\tstatus code: 403, request id: 9017B3A3E89EA391, host id: 6+JXjooSmq1FMlitzW787EsZSz8ktC3DWfvC64wRvsvBhrgvkKi9ikCgi2PpqR4KbeblH6KAq4w=")
objectstore_test.go:439:
c.Check(err, IsNil)
... value *errors.withStack = Containers, listing the buckets: AccessDenied: Access Denied
status code: 403, request id: 9B88820B15FB881A, host id: 8oyRbM9NywaXjgQYISX3j1n+cTS3okFPb32nMt3fpjKXNH8TzwzcjfbQVYEj9DuNHQLwIWmZPjQ= ("Containers, listing the buckets: AccessDenied: Access Denied\n\tstatus code: 403, request id: 9B88820B15FB881A, host id: 8oyRbM9NywaXjgQYISX3j1n+cTS3okFPb32nMt3fpjKXNH8TzwzcjfbQVYEj9DuNHQLwIWmZPjQ=")
OOPS: 7 passed, 1 skipped, 1 FAILED
--- FAIL: Test (8.70s)
FAIL
exit status 1
FAIL github.com/kanisterio/kanister/pkg/objectstore 8.729s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SupriyaKasten is this failing before or after we get temporary credentials?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fail after we get the temp creds, although 7 of the tests pass with the temp credentials. The call to s.provider.ListBuckets(ctx)
fails with these temp credentials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, then this is an issue with the Role we're changing to in these tests, so won't this fail if we run this test with anyone's creds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
00b1445
to
161b297
Compare
pkg/blockstorage/awsebs/awsebs.go
Outdated
@@ -406,6 +441,10 @@ func setResourceTags(ctx context.Context, ec2Cli *EC2, resourceID string, tags m | |||
} | |||
|
|||
func (s *ebsStorage) VolumeCreateFromSnapshot(ctx context.Context, snapshot blockstorage.Snapshot, tags map[string]string) (*blockstorage.Volume, error) { | |||
err := s.refreshCreds() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The credentials should be automatically refreshed by the EC2 client
pkg/objectstore/objectstore_test.go
Outdated
var _ = Suite(&ObjectStoreProviderSuite{osType: ProviderTypeGCS, region: ""}) | ||
var _ = Suite(&ObjectStoreProviderSuite{osType: ProviderTypeAzure, region: ""}) | ||
|
||
// var _ = Suite(&ObjectStoreProviderSuite{osType: ProviderTypeGCS, region: ""}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uncomment prior to merge
Change Overview
AWS IAM role: Generate temporary security credentials from Role
Earlier, we added the temp credentials to profile directly but since profiles are persistent we do not want them to get invalid after the session expires, hence the change.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
pkg/kando/location_test.go
pkg/location/location_test.go
pkg/restic/restic_test.go
pkg/secrets/aws_test.go
pkg/blockstorage/blockstorage_test.go
Tested for auto refreshing temp credentials