From b61d0ba24491801ae5bacc341e121c0acc79c9bc Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Wed, 22 Sep 2021 17:00:37 -0700 Subject: [PATCH] more WIP: lots of fun testing --- go.mod | 2 +- go.sum | 2 + plugin/plugin.go | 28 ++- plugin/plugin_test.go | 349 +++++++++++++++++++++++++++++++ plugin/testdata/basic/iam.tf | 41 ++-- plugin/testdata/basic/vpc_ec2.tf | 10 + testing/testing.go | 48 +++++ 7 files changed, 463 insertions(+), 17 deletions(-) create mode 100644 plugin/plugin_test.go diff --git a/go.mod b/go.mod index 5925bba..90b8072 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.16 require ( github.com/aws/aws-sdk-go v1.40.44 github.com/hashicorp/boundary/sdk v0.0.11-0.20210917171013-480e801d6447 - github.com/hashicorp/go-secure-stdlib/awsutil v0.1.3 + github.com/hashicorp/go-secure-stdlib/awsutil v0.1.4 github.com/hashicorp/terraform-json v0.12.0 // indirect google.golang.org/protobuf v1.27.1 ) diff --git a/go.sum b/go.sum index ac74f87..3b5e906 100644 --- a/go.sum +++ b/go.sum @@ -223,6 +223,8 @@ github.com/hashicorp/go-rootcerts v1.0.2/go.mod h1:pqUvnprVnM5bf7AOirdbb01K4ccR3 github.com/hashicorp/go-secure-stdlib/awsutil v0.1.2/go.mod h1:QRJZ7siKie+SZJB9jLbfKrs0Gd0yPWMtbneg0iU1PrY= github.com/hashicorp/go-secure-stdlib/awsutil v0.1.3 h1:IC4hCiNbbfG0jZBIU8/KTHXKfkdukQtx5kTjyo6Nkn0= github.com/hashicorp/go-secure-stdlib/awsutil v0.1.3/go.mod h1:MpCPSPGLDILGb4JMm94/mMi3YysIqsXzGCzkEZjcjXg= +github.com/hashicorp/go-secure-stdlib/awsutil v0.1.4 h1:aCPwC2WPyomaw5gk5ZZ7DPevN8d9DS3igkmq6DxGMjU= +github.com/hashicorp/go-secure-stdlib/awsutil v0.1.4/go.mod h1:MpCPSPGLDILGb4JMm94/mMi3YysIqsXzGCzkEZjcjXg= github.com/hashicorp/go-secure-stdlib/configutil v0.1.2/go.mod h1:EN1DJMjv9y5e/uRAuP2WPibAox5KHEiq2BjZl00aNPQ= github.com/hashicorp/go-secure-stdlib/listenerutil v0.1.1/go.mod h1:irE9ILwhE8VooWdhHhBy0qXzOMRlHKoWoMlr31mAFCE= github.com/hashicorp/go-secure-stdlib/parseutil v0.1.1/go.mod h1:QmrqtbKuxxSWTN3ETMPuB+VtEiBJ/A9XhoYGv8E1uD8= diff --git a/plugin/plugin.go b/plugin/plugin.go index 1a1b64a..5386e86 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -17,6 +17,11 @@ import ( "google.golang.org/protobuf/types/known/structpb" ) +// rotationWaitTimeout controls the time we wait for credential rotation to +// succeed. This is important to ensure that rotated credentials can be used +// right away. +const rotationWaitTimeout = time.Second * 30 + // AwsPlugin implements the HostPluginServiceServer interface for the // AWS plugin. type AwsPlugin struct { @@ -74,6 +79,11 @@ func (p *AwsPlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalog if err := state.RotateCreds(); err != nil { return nil, fmt.Errorf("error during credential rotation: %w", err) } + } else { + // Simply validate if we aren't rotating. + if err := state.ValidateCreds(); err != nil { + return nil, fmt.Errorf("error during credential validation: %w", err) + } } persistedProto, err := state.ToProto() @@ -456,6 +466,22 @@ func (s *awsCatalogPersistedState) ToProto() (*pb.HostCatalogPersisted, error) { return &pb.HostCatalogPersisted{Data: data}, nil } +func (s *awsCatalogPersistedState) ValidateCreds() error { + c, err := awsutil.NewCredentialsConfig( + awsutil.WithAccessKey(s.AccessKeyId), + awsutil.WithSecretKey(s.SecretAccessKey), + ) + if err != nil { + return fmt.Errorf("error loading credentials: %w", err) + } + + if _, err := c.GetCallerIdentity(); err != nil { + return fmt.Errorf("error validating credentials: %w", err) + } + + return nil +} + func (s *awsCatalogPersistedState) RotateCreds() error { c, err := awsutil.NewCredentialsConfig( awsutil.WithAccessKey(s.AccessKeyId), @@ -465,7 +491,7 @@ func (s *awsCatalogPersistedState) RotateCreds() error { return fmt.Errorf("error loading credentials: %w", err) } - if err := c.RotateKeys(); err != nil { + if err := c.RotateKeys(awsutil.WithValidityCheckTimeout(rotationWaitTimeout)); err != nil { return fmt.Errorf("error rotating credentials: %w", err) } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go new file mode 100644 index 0000000..74b6c56 --- /dev/null +++ b/plugin/plugin_test.go @@ -0,0 +1,349 @@ +package plugin + +import ( + "context" + "os" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws/awserr" + pluginTesting "github.com/hashicorp/boundary-host-plugin-aws/testing" + "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostcatalogs" + pb "github.com/hashicorp/boundary/sdk/pbs/plugin" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/structpb" +) + +func TestPlugin(t *testing.T) { + region := os.Getenv("AWS_REGION") + if region == "" { + t.Skip("set AWS_REGION to use this test") + } + if envAccessKeyId := os.Getenv("AWS_ACCESS_KEY_ID"); envAccessKeyId == "" { + t.Skip("set AWS_ACCESS_KEY_ID to use this test") + } + if envSecretAccessKey := os.Getenv("AWS_SECRET_ACCESS_KEY"); envSecretAccessKey == "" { + t.Skip("set AWS_SECRET_ACCESS_KEY to use this test") + } + + require := require.New(t) + tf, err := pluginTesting.NewTestTerraformer("testdata/basic") + require.NoError(err) + require.NotNil(tf) + + err = tf.Deploy() + require.NoError(err) + + defer func() { + if err := tf.Destroy(); err != nil { + t.Logf("WARNING: could not run Terraform destroy: %s", err) + } + }() + + iamUserName, err := tf.GetOutputString("iam_user_name") + require.NoError(err) + require.NotZero(iamUserName) + + iamUserArn, err := tf.GetOutputString("iam_user_arn") + require.NoError(err) + require.NotZero(iamUserArn) + + iamAccessKeyId, err := tf.GetOutputString("iam_access_key_id") + require.NoError(err) + require.NotZero(iamAccessKeyId) + + iamSecretAccessKey, err := tf.GetOutputString("iam_secret_access_key") + require.NoError(err) + require.NotZero(iamSecretAccessKey) + + ec2InstanceIds, err := tf.GetOutputSlice("instance_ids") + require.NoError(err) + require.Len(ec2InstanceIds, 5) + + ec2InstanceAddrs, err := tf.GetOutputMap("instance_addrs") + require.NoError(err) + require.Len(ec2InstanceAddrs, 5) + + ec2InstanceTags, err := tf.GetOutputMap("instance_tags") + require.NoError(err) + require.Len(ec2InstanceTags, 5) + + // Start the workflow now. Set up the host catalog. Note that this + // will cause the state to go out of drift above in the sense that + // the access key ID/secret access key will no longer be valid. We + // will assert this through the returned state. + p := new(AwsPlugin) + ctx := context.Background() + + // ******************** + // * OnCreateCatalog + // ******************** + // + // Test rotation first. + iamAccessKeyId, iamSecretAccessKey = testPluginOnCreateCatalog(ctx, t, p, region, iamAccessKeyId, iamSecretAccessKey, true) + // Test non-rotation next. + iamAccessKeyId, iamSecretAccessKey = testPluginOnCreateCatalog(ctx, t, p, region, iamAccessKeyId, iamSecretAccessKey, false) + + // ******************** + // * OnUpdateCatalog + // ******************** + // + // Test no-op non-rotation. + iamAccessKeyId, iamSecretAccessKey = testPluginOnCreateCatalog(ctx, t, p, region, iamAccessKeyId, iamSecretAccessKey, "", "", false, false) + // Switch to rotation. + iamAccessKeyId, iamSecretAccessKey = testPluginOnCreateCatalog(ctx, t, p, region, iamAccessKeyId, iamSecretAccessKey, "", "", false, true) + // Test no-op with rotation. + iamAccessKeyId, iamSecretAccessKey = testPluginOnCreateCatalog(ctx, t, p, region, iamAccessKeyId, iamSecretAccessKey, "", "", true, true) + // Switch credentials to other user. +} + +func testPluginOnCreateCatalog(ctx context.Context, t *testing.T, p *AwsPlugin, region, accessKeyId, secretAccessKey string, rotate bool) (string, string) { + t.Helper() + require := require.New(t) + + reqAttrs, err := structpb.NewStruct(map[string]interface{}{ + constRegion: region, + constDisableCredentialRotation: !rotate, + }) + require.NoError(err) + reqSecrets, err := structpb.NewStruct(map[string]interface{}{ + constAccessKeyId: accessKeyId, + constSecretAccessKey: secretAccessKey, + }) + require.NoError(err) + request := &pb.OnCreateCatalogRequest{ + Catalog: &hostcatalogs.HostCatalog{ + Attributes: reqAttrs, + Secrets: reqSecrets, + }, + } + response, err := p.OnCreateCatalog(ctx, request) + require.NoError(err) + require.NotNil(response) + persisted := response.GetPersisted() + require.NotNil(persisted) + persistedData := persisted.GetData() + require.NotNil(persistedData) + persistedDataMap := persistedData.AsMap() + require.NotNil(persistedDataMap) + persistedAccessKeyId, ok := persistedDataMap[constAccessKeyId] + require.True(ok) + require.NotZero(persistedAccessKeyId) + if rotate { + require.NotEqual(accessKeyId, persistedAccessKeyId) + } else { + require.Equal(accessKeyId, persistedAccessKeyId) + } + + persistedSecretAccessKey, ok := persistedDataMap[constSecretAccessKey] + require.True(ok) + require.NotZero(persistedSecretAccessKey) + if rotate { + require.NotEqual(secretAccessKey, persistedSecretAccessKey) + } else { + require.Equal(secretAccessKey, persistedSecretAccessKey) + } + + persistedCredsLastRotatedTime, ok := persistedDataMap[constCredsLastRotatedTime] + require.True(ok) + if rotate { + require.NotZero(persistedCredsLastRotatedTime) + } else { + require.Zero(persistedCredsLastRotatedTime) + } + + return persistedAccessKeyId.(string), persistedSecretAccessKey.(string) +} + +func testPluginOnUpdateCatalog( + ctx context.Context, t *testing.T, p *AwsPlugin, + region, currentAccessKeyId, currentSecretAccessKey, newAccessKeyId, newSecretAccessKey string, + rotated, rotate bool, +) (string, string) { + t.Helper() + require := require.New(t) + + // Take a timestamp of the current time to get a point in time to + // reference, ensuring that we are updating credential rotation + // timestamps. + currentLastRotationTime = time.Now() + + reqCurrentAttrs, err := structpb.NewStruct(map[string]interface{}{ + constRegion: region, + constDisableCredentialRotation: !rotated, + }) + reqNewAttrs, err := structpb.NewStruct(map[string]interface{}{ + constRegion: region, + constDisableCredentialRotation: !rotate, + }) + require.NoError(err) + var reqSecrets *structpb.Struct + if newAccessKeyId != "" && newSecretAccessKey != "" { + reqSecrets, err = structpb.NewStruct(map[string]interface{}{ + constAccessKeyId: newAccessKeyId, + constSecretAccessKey: newSecretAccessKey, + }) + } + reqPersistedData, err := structpb.NewStruct(map[string]interface{}{ + constAccessKeyId: currentAccessKeyId, + constSecretAccessKey: currentSecretAccessKey, + constCredsLastRotatedTime: func() time.Time { + if rotated { + return currentLastRotationTime + } + + return time.Time{} + }(), + }) + require.NoError(err) + onUpdateCatalogRequest := &pb.OnUpdateCatalogRequest{ + CurrentCatalog: &hostcatalogs.HostCatalog{ + Attributes: reqCurrentAttrs, + }, + NewCatalog: &hostcatalogs.HostCatalog{ + Attributes: reqNewAttrs, + Secrets: reqSecrets, + }, + Persisted: &pb.HostCatalogPersisted{ + Data: reqPersistedData, + }, + } + response, err := p.OnCreateCatalog(ctx, request) + require.NoError(err) + require.NotNil(response) + persisted := response.GetPersisted() + require.NotNil(persisted) + persistedData := persisted.GetData() + require.NotNil(persistedData) + persistedDataMap := persistedData.AsMap() + require.NotNil(persistedDataMap) + + // Complex checks based on the scenarios. + persistedAccessKeyId, ok := persistedDataMap[constAccessKeyId] + require.True(ok) + require.NotZero(persistedAccessKeyId) + persistedSecretAccessKey, ok := persistedDataMap[constSecretAccessKey] + require.True(ok) + require.NotZero(persistedSecretAccessKey) + persistedCredsLastRotatedTime, ok := persistedDataMap[constCredsLastRotatedTime] + require.True(ok) + + // Our test scenarios are complex due the multi-dimensional nature + // of criteria, so we lay them out in a switch below. + switch { + case newAccessKeyId != "" && rotated && rotate: + // The new access key ID was provided, we had previously rotated + // the credentials before, and the new credential set is to be + // rotated as well. In this case, the old credentials should have + // been deleted, and the new credentials should have been rotated, + // hence, should not match the new credentials initially + // provided. Rotation time should be non-zero and updated. + require.CredentialsInvalid(t, currentAccessKeyId, currentSecretAccessKey) + require.NotEqual(persistedAccessKeyId, newAccessKeyId) + require.NotEqual(persistedSecretAccessKey, newSecretAccessKey) + require.NotZero(persistedCredsLastRotatedTime) + require.True(persistedCredsLastRotatedTime.After(currentCredsLastRotatedTime)) + + case newAccessKeyId != "" && rotated && !rotate: + // The new access key ID was provided, we had previously rotated + // the credentials before, and the new credential is *not* + // rotated. In this case, the old credentials should have + // been deleted, But the new credentials should have not been + // rotated, and hence should be the same. Rotation time should be + // zero. + require.CredentialsInvalid(t, currentAccessKeyId, currentSecretAccessKey) + require.Equal(persistedAccessKeyId, newAccessKeyId) + require.Equal(persistedSecretAccessKey, newSecretAccessKey) + require.Zero(persistedCredsLastRotatedTime) + + case newAccessKeyId != "" && !rotated && rotate: + // The new access key ID was provided, we *have not* previously + // rotated the credentials, and the new credential set is to be + // rotated. In this case, the old credentials should have been + // left alone, and the new credentials should have been rotated, + // hence, should not match the new credentials initially + // provided. Rotation time should be non-zero, but updated. + require.CredentialsValid(t, currentAccessKeyId, currentSecretAccessKey) + require.NotEqual(persistedAccessKeyId, newAccessKeyId) + require.NotEqual(persistedSecretAccessKey, newSecretAccessKey) + require.NotZero(persistedCredsLastRotatedTime) + require.True(persistedCredsLastRotatedTime.After(currentCredsLastRotatedTime)) + + case newAccessKeyId != "" && !rotated && !rotate: + // The new access key ID was provided, but we have not rotated + // the credentials previously and we still don't plan on rotating + // them. In this case, the old credentials should still be valid, + // and the persisted ones should match the new ones provided. + // Rotation time should be zero. + require.CredentialsValid(t, currentAccessKeyId, currentSecretAccessKey) + require.Equal(persistedAccessKeyId, newAccessKeyId) + require.Equal(persistedSecretAccessKey, newSecretAccessKey) + require.Zero(persistedCredsLastRotatedTime) + + case newAccessKeyId == "" && rotated && rotate: + // No new credentials have been provided, but we have previously + // rotated and are still rotating credentials. This is a no-op. + // Existing credentials should still be valid and match the ones + // persisted to state. Rotation time should be identical since + // no new rotation occurred. + require.CredentialsValid(t, currentAccessKeyId, currentSecretAccessKey) + require.Equal(persistedAccessKeyId, currentAccessKeyId) + require.Equal(persistedSecretAccessKey, currentSecretAccessKey) + require.NotZero(persistedCredsLastRotatedTime) + require.Equal(currentCredsLastRotatedTime, persistedCredsLastRotatedTime) + + case newAccessKeyId == "" && rotated && !rotate: + // No new credentials have been provided, and we have previously + // rotated the credentials. This is actually an error. + // + // TODO: validate this scenario through unit testing. For now if + // we for some reason try to test this with this function, return + // an error. + require.FailNow("testing rotated-to-not-rotated scenario not implemented by this helper") + + case newAccessKeyId == "" && !rotated && rotate: + // No new credentials have been provided, and while we did not + // rotate before, we want to switch to rotation. In this case, + // the existing persisted credentials should have been rotated, + // with a new non-zero timestamp. + require.CredentialsInvalid(t, currentAccessKeyId, currentSecretAccessKey) + require.NotEqual(persistedAccessKeyId, currentAccessKeyId) + require.NotEqual(persistedSecretAccessKey, currentSecretAccessKey) + require.NotZero(persistedCredsLastRotatedTime) + + case newAccessKeyId == "" && !rotated && !rotate: + // No new credentials have been provided and we have not, nor do + // not, plan on rotating the credentials. This is a no-op. + // Existing credentials should still be valid and match the ones + // persisted to state. Rotation time should remain at zero. + require.CredentialsValid(t, currentAccessKeyId, currentSecretAccessKey) + require.Equal(persistedAccessKeyId, currentAccessKeyId) + require.Equal(persistedSecretAccessKey, currentSecretAccessKey) + require.Zero(persistedCredsLastRotatedTime) + + default: + // Scenario was reached that was not covered by this function. + require.FailNow("unknown test scenario") + } + + return persistedAccessKeyId.(string), persistedSecretAccessKey.(string) +} + +func requireCredentailsInvalid(t *testing.T, accessKeyId, secretAccessKey string) { + t.Helper() + require := require.New(t) + + err := c.GetCallerIdentity() + require.NotNil(err) + awsErr, ok := err.(awserr.Error) + require.True(ok) + require.Equal("InvalidClientTokenId", awsErr.Code()) +} + +func requireCredentailsvValid(t *testing.T, accessKeyId, secretAccessKey string) { + t.Helper() + require := require.New(t) + + err := c.GetCallerIdentity() + require.NoError(err) +} diff --git a/plugin/testdata/basic/iam.tf b/plugin/testdata/basic/iam.tf index d186b56..c71f1ea 100644 --- a/plugin/testdata/basic/iam.tf +++ b/plugin/testdata/basic/iam.tf @@ -2,19 +2,27 @@ variable "iam_user_prefix" { default = "boundary-plugin-host-aws-test-iam-user" } +variable "iam_user_count" { + default = 2 +} + resource "random_id" "aws_iam_user_name" { - prefix = var.iam_user_prefix + count = var.iam_user_count + prefix = "${var.iam_user_prefix}-${count.index}" byte_length = 4 } resource "aws_iam_user" "user" { - name = random_id.aws_iam_user_name.dec + count = var.iam_user_count + name = random_id.aws_iam_user_name[count.index].dec + force_destroy = true tags = var.project_path_tags } resource "aws_iam_access_key" "user_initial_key" { - user = aws_iam_user.user.name + count = var.iam_user_count + user = aws_iam_user.user[count.index].name } resource "aws_iam_policy" "ec2_describeinstances" { @@ -39,12 +47,14 @@ EOF } resource "aws_iam_user_policy_attachment" "user_ec2_describeinstances_attachment" { - user = aws_iam_user.user.name + count = var.iam_user_count + user = aws_iam_user.user[count.index].name policy_arn = aws_iam_policy.ec2_describeinstances.arn } resource "aws_iam_policy" "user_self_manage_policy" { - name = "BoundaryPluginHostAwsTestUserSelfManagePolicy" + count = var.iam_user_count + name = "BoundaryPluginHostAwsTestUserSelfManagePolicy-${count.index}" policy = < r.private_ip + } +} + output "instance_tags" { value = { for _, r in aws_instance.instances : r.id => r.tags diff --git a/testing/testing.go b/testing/testing.go index 915f430..d816d97 100644 --- a/testing/testing.go +++ b/testing/testing.go @@ -134,6 +134,54 @@ func (t *TestTerraformer) GetOutput(key string) (interface{}, error) { return output.Value, nil } +// GetOutputString wraps GetOutput and returns the value as a string. It's an +// error if the return value is actually not a string. +func (t *TestTerraformer) GetOutputString(key string) (string, error) { + value, err := t.GetOutput(key) + if err != nil { + return "", err + } + + str, ok := value.(string) + if !ok { + return "", fmt.Errorf("expected output value at key %q to be string, got %T", key, value) + } + + return str, nil +} + +// GetOutputMap wraps GetOutput and returns the value as a map. It's an +// error if the return value is actually not a map. +func (t *TestTerraformer) GetOutputMap(key string) (map[string]interface{}, error) { + value, err := t.GetOutput(key) + if err != nil { + return nil, err + } + + m, ok := value.(map[string]interface{}) + if !ok { + return nil, fmt.Errorf("expected output value at key %q to be map, got %T", key, value) + } + + return m, nil +} + +// GetOutputSlice wraps GetOutput and returns the value as a slice. It's an +// error if the return value is actually not a slice. +func (t *TestTerraformer) GetOutputSlice(key string) ([]interface{}, error) { + value, err := t.GetOutput(key) + if err != nil { + return nil, err + } + + m, ok := value.([]interface{}) + if !ok { + return nil, fmt.Errorf("expected output value at key %q to be slice, got %T", key, value) + } + + return m, nil +} + // init runs "terraform init" in the project's working directory. // // This is only ever done once.