Skip to content

Bug 2035315: fix passthrough test cases #438

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 2 commits into from
Dec 23, 2021
+181 −43
Merged

Bug 2035315: fix passthrough test cases #438

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7995e98
fix passthrough test cases
Dec 22, 2021
994d336
address PR feedback
Dec 23, 2021
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
93 changes: 81 additions & 12 deletions pkg/aws/actuator/actuator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,14 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR
}

// Various checks for the kinds of reasons that would trigger a needed update
_, accessKey, secretKey, credentialsKey := a.loadExistingSecret(cr)
awsClient, err := a.AWSClientBuilder([]byte(accessKey), []byte(secretKey), a.Client)
_, existingAccessKey, existingSecretKey, existingCredentialsKey := a.loadExistingSecret(cr)
awsClient, err := a.AWSClientBuilder([]byte(existingAccessKey), []byte(existingSecretKey), a.Client)
if err != nil {
return true, err
}

// Make sure we update old Secrets that don't have the new "credentials" field
if credentialsKey == "" || credentialsKey != string(generateAWSCredentialsConfig(accessKey, secretKey)) {
if existingCredentialsKey == "" || existingCredentialsKey != string(generateAWSCredentialsConfig(existingAccessKey, existingSecretKey)) {
logger.Infof("Secret %s key needs updating, will update Secret contents", constants.AWSSecretDataCredentialsKey)
return true, nil
}
Expand All @@ -177,14 +177,14 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR
return true, fmt.Errorf("unable to decode ProviderStatus: %v", err)
}

readAWSClient, err := a.buildReadAWSClient(cr)
if err != nil {
log.WithError(err).Error("error creating read-only AWS client")
return true, fmt.Errorf("unable to check whether AWS user is properly tagged")
}

// Minted-user-specific checks
if awsStatus.User != "" {
readAWSClient, err := a.buildReadAWSClient(cr)
if err != nil {
log.WithError(err).Error("error creating read-only AWS client")
return true, fmt.Errorf("unable to check whether AWS user is properly tagged")
}

// If AWS user defined (ie minted creds instead of passthrough) check whether user is tagged
user, err := readAWSClient.GetUser(&iam.GetUserInput{
UserName: aws.String(awsStatus.User),
Expand Down Expand Up @@ -225,7 +225,7 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR
logger.WithError(err).Error("error listing all access keys for user")
return false, err
}
accessKeyExists, err := a.accessKeyExists(logger, allUserKeys, accessKey)
accessKeyExists, err := a.accessKeyExists(logger, allUserKeys, existingAccessKey)
if err != nil {
logger.WithError(err).Error("error querying whether access key still valid")
}
Expand All @@ -241,6 +241,9 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR
}

policyEqual, err := a.awsPolicyEqualsDesiredPolicy(desiredUserPolicy, awsSpec, awsStatus, user.User, readAWSClient, logger)
if err != nil {
return true, err
}
if !policyEqual {
return true, nil
}
Expand Down Expand Up @@ -278,7 +281,7 @@ func (a *AWSActuator) needsUpdate(ctx context.Context, cr *minterv1.CredentialsR
Region: region,
}

goodEnough, err := ccaws.CheckPermissionsUsingQueryClient(readAWSClient, awsClient, awsSpec.StatementEntries, simParams, logger)
goodEnough, err := ccaws.CheckPermissionsAgainstStatementList(awsClient, awsSpec.StatementEntries, simParams, logger)
if err != nil {
return true, fmt.Errorf("error validating whether current creds are good enough: %v", err)
}
Expand Down Expand Up @@ -371,8 +374,74 @@ func (a *AWSActuator) syncPassthrough(ctx context.Context, cr *minterv1.Credenti
existingSecret, _, _, _ := a.loadExistingSecret(cr)
accessKeyID := string(cloudCredsSecret.Data[awsannotator.AwsAccessKeyName])
secretAccessKey := string(cloudCredsSecret.Data[awsannotator.AwsSecretAccessKeyName])

mode, _, err := utils.GetOperatorConfiguration(a.Client, logger)
if err != nil {
msg := "error getting operator configuration"
logger.WithError(err).Error(msg)
return &actuatoriface.ActuatorError{
ErrReason: minterv1.CredentialsProvisionFailure,
Message: fmt.Sprintf("%v: %v", msg, err),
}
}
if mode == operatorv1.CloudCredentialsModePassthrough {
logger.Debug("will not perform permissions simulation because operator in mode %q", mode)
} else {
Comment on lines +378 to +389

@akhil-rane akhil-rane Dec 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we again need to confirm the mode here if we are already syncing for passthrough?

@joelddiaz joelddiaz Dec 23, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So there's a difference between the Secret being annotated with "passthrough" and the CloudCredential Operator config saying that the mode is explicitly "Passthrough".
When the operator is explicitly configured with a non-default mode (eg not the empty string ""), then it is a way to tell CCO not to perform permissions validations. We came up with this behavior to address the case where a cluster is installed in an account where SCP policies would keep us from reliably being able to SimulatePolicy(). https://github.com/openshift/enhancements/blob/ce4d303db807622687159eb9d3248285a003fabb/enhancements/installer/aws-permissions-check-bypass.md
So in the mode = "" case (the default), CCO will see if the creds can do Mint, and use that if possible. If that fails, it will check for Passthrough, and use that if possible. But in either case where CCO dynamically determined Mint or Passthrough (ie CCO wasn't explicitly told), CCO will do the SimulatePolicy() calls to check whether the creds really are good enough.

@akhil-rane akhil-rane Dec 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok that makes sense

region, err := awsutils.LoadInfrastructureRegion(a.Client, logger)
if err != nil {
msg := "error reading region from Infrastructure CR"
logger.WithError(err).Error(msg)
return &actuatoriface.ActuatorError{
ErrReason: minterv1.CredentialsProvisionFailure,
Message: fmt.Sprintf("%v: %v", msg, err),
}
}

simParams := &ccaws.SimulateParams{
Region: region,
}

// build client with root secret and verify that the creds are good enough to pass through
awsClient, err := a.AWSClientBuilder([]byte(accessKeyID), []byte(secretAccessKey), a.Client)
if err != nil {
msg := "error building AWS client"
logger.WithError(err).Error(msg)
return &actuatoriface.ActuatorError{
ErrReason: minterv1.CredentialsProvisionFailure,
Message: fmt.Sprintf("%v: %v", msg, err),
}
}

awsSpec, err := DecodeProviderSpec(a.Codec, cr)
if err != nil {
msg := "error decoding AWS ProviderSpec"
logger.WithError(err).Error(msg)
return &actuatoriface.ActuatorError{
ErrReason: minterv1.CredentialsProvisionFailure,
Message: fmt.Sprintf("%v: %v", msg, err),
}
}
goodEnough, err := ccaws.CheckPermissionsAgainstStatementList(awsClient, awsSpec.StatementEntries, simParams, logger)
if err != nil {
msg := "error validating whether root creds are good enough"
logger.WithError(err).Error(msg)
return &actuatoriface.ActuatorError{
ErrReason: minterv1.CredentialsProvisionFailure,
Message: fmt.Sprintf("%v: %v", msg, err),
}
}
if !goodEnough {
msg := "root creds not sufficient"
Comment thread
joelddiaz marked this conversation as resolved.
Outdated
logger.Info("root creds not sufficient")
return &actuatoriface.ActuatorError{
ErrReason: minterv1.CredentialsProvisionFailure,
Message: fmt.Sprintf("%v", msg),
}
}
}

// userPolicy param empty because in passthrough mode this doesn't really have any meaning
err := a.syncAccessKeySecret(cr, accessKeyID, secretAccessKey, existingSecret, "", logger)
err = a.syncAccessKeySecret(cr, accessKeyID, secretAccessKey, existingSecret, "", logger)
if err != nil {
msg := "error creating/updating secret"
logger.WithError(err).Error(msg)
Expand Down
131 changes: 100 additions & 31 deletions pkg/operator/credentialsrequest/credentialsrequest_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package credentialsrequest
import (
"context"
"fmt"
"math/rand"
"net/url"
"testing"
"time"
Expand Down Expand Up @@ -446,6 +447,7 @@ func TestCredentialsRequestReconcile(t *testing.T) {
assert.True(t, cr.Status.Provisioned)
},
},

{
name: "cred exists access key missing",
existing: []runtime.Object{
Expand Down Expand Up @@ -599,10 +601,9 @@ func TestCredentialsRequestReconcile(t *testing.T) {
},
mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
mockAWSClient := mockaws.NewMockClient(mockCtrl)
return mockAWSClient
},
mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
mockAWSClient := mockaws.NewMockClient(mockCtrl)
mockGetUser(mockAWSClient)
// will simulate to check that the creds in the target secret actually satisfy the requested perms
mockSimulatePrincipalPolicyPagesSuccess(mockAWSClient)
return mockAWSClient
},
validate: func(c client.Client, t *testing.T) {
Expand All @@ -617,7 +618,8 @@ func TestCredentialsRequestReconcile(t *testing.T) {
},
},
{
name: "existing passthrough credential",
name: "new passthrough credential with insuffient root creds",
expectErr: true,
existing: []runtime.Object{
testOperatorConfig(""),
createTestNamespace(testNamespace),
Expand All @@ -626,33 +628,86 @@ func TestCredentialsRequestReconcile(t *testing.T) {
testPassthroughCredentialsRequest(t),
testPassthroughAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey),
testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey),
testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey),
},
mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
mockAWSClient := mockaws.NewMockClient(mockCtrl)
mockGetUser(mockAWSClient)
mockSimulatePrincipalPolicyPagesFailure(mockAWSClient)
return mockAWSClient
},
mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
validate: func(c client.Client, t *testing.T) {
targetSecret := getSecret(c)
assert.Nil(t, targetSecret)
cr := getCR(c)
assert.False(t, cr.Status.Provisioned)
},
expectedConditions: []ExpectedCondition{
{
conditionType: minterv1.CredentialsProvisionFailure,
reason: "CredentialsProvisionFailure",
status: corev1.ConditionTrue,
},
},
},
{
name: "existing up-to-date passthrough credential",
existing: []runtime.Object{
testOperatorConfig(""),
createTestNamespace(testNamespace),
testInfrastructure(testInfraName),
createTestNamespace(testSecretNamespace),
testPassthroughCredentialsRequestWithLastSyncResourceVersion(t, "12345"),
testPassthroughAWSCredsSecretWithResourceVersion("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey, "12345"),
testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey),
testAWSCredsSecret(testSecretNamespace, testSecretName, testRootAWSAccessKeyID, testRootAWSSecretAccessKey),
},
mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
mockAWSClient := mockaws.NewMockClient(mockCtrl)
mockGetUser(mockAWSClient)
// will simulate to check that the creds in the target secret actually satisfy the requested perms
mockSimulatePrincipalPolicyPagesSuccess(mockAWSClient)
return mockAWSClient
},
mockSecretAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
validate: func(c client.Client, t *testing.T) {
targetSecret := getSecret(c)
require.NotNil(t, targetSecret)
assert.Equal(t, testRootAWSAccessKeyID,
string(targetSecret.Data["aws_access_key_id"]))
assert.Equal(t, testRootAWSSecretAccessKey,
string(targetSecret.Data["aws_secret_access_key"]))
cr := getCR(c)
assert.True(t, cr.Status.Provisioned)
},
},
{
name: "existing out-of-date passthrough credential (rotating root creds)",
existing: []runtime.Object{
testOperatorConfig(""),
createTestNamespace(testNamespace),
testInfrastructure(testInfraName),
createTestNamespace(testSecretNamespace),
testPassthroughCredentialsRequestWithLastSyncResourceVersion(t, "0001"),
testPassthroughAWSCredsSecretWithResourceVersion("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey, "0002"),
testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey),
testAWSCredsSecret(testSecretNamespace, testSecretName, testAWSAccessKeyID, testAWSSecretAccessKey),
},
mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
mockAWSClient := mockaws.NewMockClient(mockCtrl)
mockGetUser(mockAWSClient)
// will simulate to check that the creds in the target secret actually satisfy the requested perms
mockSimulatePrincipalPolicyPagesSuccess(mockAWSClient)
return mockAWSClient
},
validate: func(c client.Client, t *testing.T) {
targetSecret := getSecret(c)
require.NotNil(t, targetSecret)
assert.Equal(t, testAWSAccessKeyID,
assert.Equal(t, testRootAWSAccessKeyID,
string(targetSecret.Data["aws_access_key_id"]))
assert.Equal(t, testAWSSecretAccessKey,
assert.Equal(t, testRootAWSSecretAccessKey,
string(targetSecret.Data["aws_secret_access_key"]))
cr := getCR(c)
assert.True(t, cr.Status.Provisioned)
assert.Equal(t, "0002", cr.Status.LastSyncCloudCredsSecretResourceVersion)
},
},
{
Expand All @@ -671,22 +726,12 @@ func TestCredentialsRequestReconcile(t *testing.T) {
mockAWSClient := mockaws.NewMockClient(mockCtrl)
return mockAWSClient
},
mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
mockAWSClient := mockaws.NewMockClient(mockCtrl)
mockGetUser(mockAWSClient)
return mockAWSClient
},
mockSecretAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient {
mockAWSClient := mockaws.NewMockClient(mockCtrl)
mockGetUser(mockAWSClient)
return mockAWSClient
},
validate: func(c client.Client, t *testing.T) {
targetSecret := getSecret(c)
require.NotNil(t, targetSecret)
assert.Equal(t, testAWSAccessKeyID,
assert.Equal(t, testRootAWSAccessKeyID,
string(targetSecret.Data["aws_access_key_id"]))
assert.Equal(t, testAWSSecretAccessKey,
assert.Equal(t, testRootAWSSecretAccessKey,
string(targetSecret.Data["aws_secret_access_key"]))
cr := getCR(c)
assert.True(t, cr.Status.Provisioned)
Expand Down Expand Up @@ -1329,7 +1374,7 @@ func TestCredentialsRequestReconcile(t *testing.T) {
mockSecretAWSClient = test.mockSecretAWSClient(mockCtrl)
}

fakeClient := fake.NewFakeClient(test.existing...)
fakeClient := fake.NewClientBuilder().WithRuntimeObjects(test.existing...).Build()
rcr := &ReconcileCredentialsRequest{
Client: fakeClient,
Actuator: &actuator.AWSActuator{
Expand Down Expand Up @@ -1451,6 +1496,12 @@ func testCredentialsRequestWithDeletionTimestamp(t *testing.T) *minterv1.Credent
return cr
}

func testPassthroughCredentialsRequestWithLastSyncResourceVersion(t *testing.T, resourceVersion string) *minterv1.CredentialsRequest {
cr := testPassthroughCredentialsRequest(t)
cr.Status.LastSyncCloudCredsSecretResourceVersion = resourceVersion
return cr
}

// passthrough credentialsrequest objects have no awsStatus
func testPassthroughCredentialsRequest(t *testing.T) *minterv1.CredentialsRequest {
codec, err := minterv1.NewCodec()
Expand Down Expand Up @@ -1542,20 +1593,29 @@ func testInsufficientAWSCredsSecret(namespace, name, accessKeyID, secretAccessKe
return s
}

func testPassthroughAWSCredsSecretWithResourceVersion(namespace, name, accessKeyID, secretAccessKey, resourceVersion string) *corev1.Secret {
s := testAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey)
s.Annotations[constants.AnnotationKey] = constants.PassthroughAnnotation
s.ObjectMeta.ResourceVersion = resourceVersion
return s
}

func testPassthroughAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey string) *corev1.Secret {
s := testAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey)
s.Annotations[constants.AnnotationKey] = constants.PassthroughAnnotation
return s
}

func testLegacyAWSCredsSecret(namespace, name, accessKeyID, secretAccessKey string) *corev1.Secret {
resourceVersion := fmt.Sprintf("%d", rand.Int())
s := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Annotations: map[string]string{
constants.AnnotationKey: constants.MintAnnotation,
},
ResourceVersion: resourceVersion,
},
Data: map[string][]byte{
"aws_access_key_id": []byte(accessKeyID),
Expand Down Expand Up @@ -1781,16 +1841,25 @@ func testClusterVersion() *configv1.ClusterVersion {
}
}

func mockSimulatePrincipalPolicyPagesSuccess(mockAWSClient *mockaws.MockClient) {
mockAWSClient.EXPECT().SimulatePrincipalPolicyPages(gomock.Any(), gomock.Any()).Return(nil)
// fake a successful permissions check but where the permissions aren't good enough
func mockSimulatePrincipalPolicyPagesFailure(mockAWSClient *mockaws.MockClient) {
mockAWSClient.EXPECT().SimulatePrincipalPolicyPages(gomock.Any(), gomock.Any()).Do(
func(input *iam.SimulatePrincipalPolicyInput, f func(resp *iam.SimulatePolicyResponse, lastPage bool) bool) {
response := &iam.SimulatePolicyResponse{
EvaluationResults: []*iam.EvaluationResult{
{
EvalActionName: aws.String("ec2:DeniedAction"),
EvalDecision: aws.String("notallowed"),
},
},
}

f(response, false)
}).Return(nil)
}

func mockSimulatePrincipalPolicySuccess(mockAWSClient *mockaws.MockClient) {
mockAWSClient.EXPECT().SimulatePrincipalPolicy(gomock.Any()).Return(&iam.SimulatePolicyResponse{
EvaluationResults: []*iam.EvaluationResult{
{EvalDecision: aws.String("allowed")},
},
}, nil)
func mockSimulatePrincipalPolicyPagesSuccess(mockAWSClient *mockaws.MockClient) {
mockAWSClient.EXPECT().SimulatePrincipalPolicyPages(gomock.Any(), gomock.Any()).Return(nil)
}

func testInfrastructure(infraName string) *configv1.Infrastructure {
Expand Down