From 9a1fe9706530b50bbaec0909c91149680b82d69a Mon Sep 17 00:00:00 2001 From: Peter Van Bouwel Date: Fri, 15 Nov 2024 22:22:11 +0100 Subject: [PATCH 1/2] feature: implement the Deny effect for policies feature: process nested tag claims Allow IDPs to provide sessiont tags via the nested format. This is for https://github.com/VITObelgium/fakes3pp/issues/4 --- cmd/handler-builder.go | 5 +- cmd/jwt.go | 45 ++++++++++++--- cmd/policy-evaluation.go | 48 +++++++++++----- cmd/policy-evaluation_test.go | 67 +++++++++++++++++++++- cmd/policy-generation.go | 32 ++++++++--- cmd/policy-generation_test.go | 40 ++++++------- cmd/policy-iam-action.go | 76 +++++++++++++++++-------- cmd/policy-iam-action_test.go | 40 +++++++++---- cmd/proxysts_test.go | 104 ++++++++++++++++++++++++++++++++++ 9 files changed, 373 insertions(+), 84 deletions(-) diff --git a/cmd/handler-builder.go b/cmd/handler-builder.go index c32a083..aa47785 100644 --- a/cmd/handler-builder.go +++ b/cmd/handler-builder.go @@ -112,7 +112,8 @@ func authorizeS3Action(ctx context.Context, sessionToken string, action S3ApiAct return } - policyStr, err := pm.GetPolicy(sessionClaims.RoleARN, PolicyTemplateDataFromClaims(sessionClaims)) + policySessionData := GetPolicySessionDataFromClaims(sessionClaims) + policyStr, err := pm.GetPolicy(sessionClaims.RoleARN, policySessionData) if err != nil { slog.Error("Could not get policy for temporary credentials", "error", err, xRequestIDStr, getRequestID(ctx), "role_arn", sessionClaims.RoleARN) writeS3ErrorResponse(ctx, w, ErrS3InternalError, nil) @@ -125,7 +126,7 @@ func authorizeS3Action(ctx context.Context, sessionToken string, action S3ApiAct writeS3ErrorResponse(ctx, w, ErrS3InternalError, nil) return } - iamActions, err := NewIamActionsFromS3Request(action, r) + iamActions, err := newIamActionsFromS3Request(action, r, policySessionData) if err != nil { slog.Error("Could not get IAM actions from request", "error", err, xRequestIDStr, getRequestID(ctx), "policy", sessionClaims.RoleARN) writeS3ErrorResponse(ctx, w, ErrS3InternalError, nil) diff --git a/cmd/jwt.go b/cmd/jwt.go index 81fd663..6b51fa9 100644 --- a/cmd/jwt.go +++ b/cmd/jwt.go @@ -9,17 +9,20 @@ import ( "github.com/google/uuid" ) -type SessionClaims struct { - RoleARN string `json:"role_arn"` - //The issuer of the initial OIDC refresh token - IIssuer string `json:"initial_issuer"` +type AWSSessionTags struct { + PrincipalTags map[string][]string `json:"principal_tags"` + TransitiveTagKeys []string `json:"transitive_tag_keys,omitempty"` +} + +type IDPClaims struct { + //The optional session tags + Tags AWSSessionTags `json:"https://aws.amazon.com/tags,omitempty"` jwt.RegisteredClaims } -func createRS256PolicyToken(issuer, iIssuer, subject, roleARN string, expiry time.Duration) (*jwt.Token) { - claims := &SessionClaims{ - roleARN, - iIssuer, +func newIDPClaims(issuer, subject string, expiry time.Duration, tags AWSSessionTags) (*IDPClaims) { + return &IDPClaims{ + tags, jwt.RegisteredClaims{ ExpiresAt: jwt.NewNumericDate(time.Now().UTC().Add(expiry)), IssuedAt: jwt.NewNumericDate(time.Now().UTC()), @@ -30,6 +33,32 @@ func createRS256PolicyToken(issuer, iIssuer, subject, roleARN string, expiry tim }, } +} + +type SessionClaims struct { + RoleARN string `json:"role_arn"` + //The issuer of the initial OIDC refresh token + IIssuer string `json:"initial_issuer"` + IDPClaims +} + +func createRS256PolicyToken(issuer, iIssuer, subject, roleARN string, expiry time.Duration) (*jwt.Token) { + claims := &SessionClaims{ + roleARN, + iIssuer, + IDPClaims{ + AWSSessionTags{}, + jwt.RegisteredClaims{ + ExpiresAt: jwt.NewNumericDate(time.Now().UTC().Add(expiry)), + IssuedAt: jwt.NewNumericDate(time.Now().UTC()), + NotBefore: jwt.NewNumericDate(time.Now().UTC()), + Issuer: issuer, + Subject: subject, + ID: uuid.New().String(), + }, + }, + } + token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) return token } diff --git a/cmd/policy-evaluation.go b/cmd/policy-evaluation.go index 39406bb..2bb6893 100644 --- a/cmd/policy-evaluation.go +++ b/cmd/policy-evaluation.go @@ -45,6 +45,8 @@ func NewPolicyEvaluatorFromStr(policyContent string) (*PolicyEvaluator, error) type evalReason string const reasonActionIsAllowed evalReason = "Action is allowed" const reasonNoStatementAllowingAction evalReason = "No statement allows the action" +const reasonExplicitDeny evalReason = "Explicit deny" +const reasonErrorEncountered evalReason = "Error was encountered" //Allow wildcards like * and ? but escape other special characters //https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition_operators.html @@ -75,28 +77,42 @@ func areAllConditionValuesSingular(context map[string]*policy.ConditionValue) (b return true } +//Evaluate what a StringLike operation does +func evalStringLike(conditionDetails map[string]*policy.ConditionValue, context map[string]*policy.ConditionValue) (bool, error) { + if !areAllConditionValuesSingular(context) { + return false, fmt.Errorf("non-singular value got %v", context) + } + for sConditionKey, sConditionValue := range conditionDetails { + contextValue, exists := context[sConditionKey] + if !exists { + return false, fmt.Errorf("condition key '%s' was not set in request context", sConditionKey) + } + if !isConditionMetForStringLike(sConditionValue, contextValue) { + return false, nil + } + } + return true, nil +} + // See whether the condition defined by the conditionOperator and conditionDetails is met // for the given context func isConditionMetForOperator(conditionOperator string, conditionDetails map[string]*policy.ConditionValue, context map[string]*policy.ConditionValue) (bool, error) { switch conditionOperator { case "StringLike": - if !areAllConditionValuesSingular(context) { - return false, fmt.Errorf("non-singular value for %s, got %v", conditionOperator, context) + result, err := evalStringLike(conditionDetails, context) + if err != nil { + return false, fmt.Errorf("operator StringLike encountered %s", err) } - for sConditionKey, sConditionValue := range conditionDetails { - contextValue, exists := context[sConditionKey] - if !exists { - return false, fmt.Errorf("condition key '%s' was not set in request context", sConditionKey) - } - if !isConditionMetForStringLike(sConditionValue, contextValue) { - return false, nil - } + return result, err + case "StringNotLike": + result, err := evalStringLike(conditionDetails, context) + if err != nil { + return false, fmt.Errorf("operator StringLike encountered %s", err) } + return !result, err default: return false, fmt.Errorf("unsupported condition: '%s'", conditionOperator) } - //No unmet condition - return true, nil } @@ -164,7 +180,13 @@ func (e *PolicyEvaluator) Evaluate(a iamAction) (isAllowed bool, reason evalReas reason = reasonActionIsAllowed } case policy.EffectDeny: - panic("Not implemented yet") + relevant, err := isRelevantFor(s, a) + if err != nil { + return false, reasonErrorEncountered, err + } + if relevant { + return false, reasonExplicitDeny, err + } } } return diff --git a/cmd/policy-evaluation_test.go b/cmd/policy-evaluation_test.go index 330b4a3..9cc496a 100644 --- a/cmd/policy-evaluation_test.go +++ b/cmd/policy-evaluation_test.go @@ -54,7 +54,39 @@ var testPolScen2AllowListingBucketWithinPrefix = fmt.Sprintf(` } `, IAMActionS3ListBucket, testBucketARN, testAllowedPrefix) - +var testPolAllowAllIfTestDepartmentOtherwiseDenyAll = ` +{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "Allow all if test department", + "Effect": "Allow", + "Action": [ + "*" + ], + "Resource": "*", + "Condition" : { + "StringLike" : { + "aws:PrincipalTag/department": "test" + } + } + }, + { + "Sid": "Deny all if not test department", + "Effect": "Deny", + "Action": [ + "*" + ], + "Resource": "*", + "Condition" : { + "StringNotLike" : { + "aws:PrincipalTag/department": "test" + } + } + } + ] +} +` func TestPolicyEvaluations(t *testing.T) { @@ -121,6 +153,39 @@ func TestPolicyEvaluations(t *testing.T) { false, reasonNoStatementAllowingAction, }, + { + "Any action should be allowed if we run with test department session tag", + testPolAllowAllIfTestDepartmentOtherwiseDenyAll, + newIamAction( + IAMActionS3GetObject, + testBucketARN, + testSessionDataTestDepartment, + ), + true, + reasonActionIsAllowed, + }, + { + "Any action should be allowed if we run with test department session tag 2", + testPolAllowAllIfTestDepartmentOtherwiseDenyAll, + newIamAction( + IAMActionS3ListAllMyBuckets, + testBucketARN, + testSessionDataTestDepartment, + ), + true, + reasonActionIsAllowed, + }, + { + "Any action should be disallowed if we run with deparment session tag different from test", + testPolAllowAllIfTestDepartmentOtherwiseDenyAll, + newIamAction( + IAMActionS3GetObject, + testBucketARN, + testSessionDataQaDeparment, + ), + false, + reasonExplicitDeny, + }, } for _, policyTest := range policyTests { diff --git a/cmd/policy-generation.go b/cmd/policy-generation.go index 88349b0..45b5834 100644 --- a/cmd/policy-generation.go +++ b/cmd/policy-generation.go @@ -133,20 +133,36 @@ func (m *PolicyManager) getPolicyTemplate(arn string) (tmpl *template.Template, return } -type policyTemplateData struct { - Claims map[string]string + +type PolicySessionClaims struct { + Subject string + Issuer string +} + + +//This is the structure that will be made available during templating and +//thus is available to be used in policies. +type PolicySessionData struct { + Claims PolicySessionClaims + Tags AWSSessionTags } -func PolicyTemplateDataFromClaims(sc *SessionClaims) policyTemplateData{ - return policyTemplateData{ - Claims: map[string]string{ - "Issuer": sc.IIssuer, - "Subject": sc.Subject, +func GetPolicySessionDataFromClaims(claims *SessionClaims) *PolicySessionData { + issuer := claims.IIssuer + if issuer == "" { + issuer = claims.Issuer + } + return &PolicySessionData{ + Claims: PolicySessionClaims{ + Subject: claims.Subject, + Issuer: issuer, }, + Tags: claims.Tags, } } -func (m *PolicyManager) GetPolicy(arn string, data policyTemplateData) (string, error) { + +func (m *PolicyManager) GetPolicy(arn string, data *PolicySessionData) (string, error) { tmpl, err := m.getPolicyTemplate(arn) if err != nil { return "", err diff --git a/cmd/policy-generation_test.go b/cmd/policy-generation_test.go index 0f5131f..f872a05 100644 --- a/cmd/policy-generation_test.go +++ b/cmd/policy-generation_test.go @@ -26,7 +26,7 @@ var testPolicyRealistic = ` "Resource": "arn:aws:s3:::OpenEO-artifacts", "Condition" : { "StringLike" : { - "s3:prefix": "{{.Claims.Issuer}}/*" + "s3:prefix": "{{.Claims.Subject}}/*" } } } @@ -34,15 +34,7 @@ var testPolicyRealistic = ` } ` -func NewTestPolicyRetriever() *TestPolicyRetriever { - return &TestPolicyRetriever{ - testPolicies: map[string]string{ - "policyRealistic": testPolicyRealistic, - }, - } -} - -func NewTestPolicyManager() *PolicyManager { +func newTestPolicyManager() *PolicyManager { return NewPolicyManager( TestPolicyRetriever{ testPolicies: map[string]string{ @@ -77,43 +69,53 @@ func (r TestPolicyRetriever) retrieveAllIdentifiers() ([]string, error) { type policyGenerationTestCase struct { PolicyName string - Claims policyTemplateData + Claims *SessionClaims Expectedpolicy string } +func buildTestSessionClaimsNoTags(issuer, subject string) (*SessionClaims) { + idpClaims := newIDPClaims(issuer, subject, time.Hour * 1, AWSSessionTags{}) + return &SessionClaims{ + RoleARN: "", + IIssuer: "", + IDPClaims: *idpClaims, + } +} + func TestPolicyGeneration(t *testing.T) { testCases := []policyGenerationTestCase{ { PolicyName: "policyRealistic", - Claims: policyTemplateData{Claims: map[string]string{"Issuer": "https://SuperIssuer"}}, - Expectedpolicy: strings.Replace(testPolicyRealistic, "{{.Claims.Issuer}}", "https://SuperIssuer", -1), + Claims: buildTestSessionClaimsNoTags("", "userA"), + Expectedpolicy: strings.Replace(testPolicyRealistic, "{{.Claims.Subject}}", "userA", -1), }, { PolicyName: "now", - Claims: policyTemplateData{Claims: map[string]string{}}, + Claims: buildTestSessionClaimsNoTags("", ""), Expectedpolicy: YYYYmmdd(Now()), }, { PolicyName: "nowSlashed", - Claims: policyTemplateData{Claims: map[string]string{}}, + Claims: buildTestSessionClaimsNoTags("", ""), Expectedpolicy: YYYYmmddSlashed(Now()), }, { PolicyName: "tomorrow", - Claims: policyTemplateData{Claims: map[string]string{}}, + Claims: buildTestSessionClaimsNoTags("", ""), Expectedpolicy: YYYYmmdd(Now().Add(time.Hour * 24)), }, { PolicyName: "sha1", - Claims: policyTemplateData{Claims: map[string]string{"Issuer": "a", "Subject": "b"}}, + Claims: buildTestSessionClaimsNoTags("a", "b"), Expectedpolicy: sha1sum("a:b"), }, } - tpm := NewTestPolicyManager() + tpm := newTestPolicyManager() for _, tc := range testCases { - got, err := tpm.GetPolicy(tc.PolicyName, tc.Claims) + policyData := GetPolicySessionDataFromClaims(tc.Claims) + got, err := tpm.GetPolicy(tc.PolicyName, policyData) if err != nil { t.Errorf("Encountered for policy %s error %s", tc.PolicyName, err) } diff --git a/cmd/policy-iam-action.go b/cmd/policy-iam-action.go index cf97887..3229bc7 100644 --- a/cmd/policy-iam-action.go +++ b/cmd/policy-iam-action.go @@ -16,7 +16,10 @@ type iamAction struct{ Context map[string]*policy.ConditionValue `json:"context,omitempty"` } -func NewIamAction(action, resource string, context map[string]*policy.ConditionValue) iamAction{ +func newIamAction(action, resource string, session *PolicySessionData) iamAction{ + context := map[string]*policy.ConditionValue{} + addGenericSessionContextKeys(context, session) + return iamAction{ Action: action, Resource: resource, @@ -24,6 +27,14 @@ func NewIamAction(action, resource string, context map[string]*policy.ConditionV } } +// For a given IAM action add context specific for the action +func (a iamAction) addContext(context map[string]*policy.ConditionValue) (iamAction){ + for contextKey, ContextKeyValues := range context { + a.Context[contextKey] = ContextKeyValues + } + return a +} + func makeS3BucketArn(bucketName string) string { return fmt.Sprintf("arn:aws:s3:::%s", bucketName) } @@ -49,9 +60,22 @@ func getS3ObjectFromRequest(req *http.Request) (bucketName string, objectKey str } } +//Add context keys that are added to nearly all requests that contain information about the current session +func addGenericSessionContextKeys(context map[string]*policy.ConditionValue, session *PolicySessionData) { + addAwsPrincipalTagConditionKeys(context, session) +} + +//Add aws:PrincipalTag/tag-key keys that are added to nearly all requests that contain information about the current session +//https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html#condition-keys-principaltag +func addAwsPrincipalTagConditionKeys(context map[string]*policy.ConditionValue, session *PolicySessionData) { + for tagKey, tagValues := range session.Tags.PrincipalTags { + context[fmt.Sprintf("aws:PrincipalTag/%s", tagKey)] = policy.NewConditionValueString(true, tagValues...) + } +} + //Buid a new IAM action based out of an HTTP Request. The IAM action should resemble the required //Permissions. The api_action is passed in as a string argument -func NewIamActionsFromS3Request(api_action S3ApiAction, req *http.Request) (actions []iamAction, err error) { +func newIamActionsFromS3Request(api_action S3ApiAction, req *http.Request, session *PolicySessionData) (actions []iamAction, err error) { actions = []iamAction{} switch api_action { // https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html @@ -60,49 +84,55 @@ func NewIamActionsFromS3Request(api_action S3ApiAction, req *http.Request) (acti if err != nil { return nil, err } - a := iamAction{ - Action: IAMActionS3PutObject, - Resource: makeS3ObjectArn(bucket, key), - } + a := newIamAction( + IAMActionS3PutObject, + makeS3ObjectArn(bucket, key), + session, + ) actions = append(actions, a) case apiS3GetObject, apiS3HeadObject: bucket, key, err := getS3ObjectFromRequest(req) if err != nil { return nil, err } - a := iamAction{ - Action: IAMActionS3GetObject, - Resource: makeS3ObjectArn(bucket, key), - } + a := newIamAction( + IAMActionS3GetObject, + makeS3ObjectArn(bucket, key), + session, + ) actions = append(actions, a) case apiS3ListObjectsV2: bucket, _, err := getS3ObjectFromRequest(req) if err != nil { return nil, err } - a := iamAction{ - Action: IAMActionS3ListBucket, - Resource: makeS3BucketArn(bucket), - Context: map[string]*policy.ConditionValue{ + a := newIamAction( + IAMActionS3ListBucket, + makeS3BucketArn(bucket), + session, + ).addContext( + map[string]*policy.ConditionValue{ IAMConditionS3Prefix: policy.NewConditionValueString(true, req.URL.Query().Get("prefix")), }, - } + ) actions = append(actions, a) case apiS3AbortMultipartUpload: bucket, key, err := getS3ObjectFromRequest(req) if err != nil { return nil, err } - a := iamAction{ - Action: IAMActionS3AbortMultipartUpload, - Resource: makeS3ObjectArn(bucket, key), - } + a := newIamAction( + IAMActionS3AbortMultipartUpload, + makeS3ObjectArn(bucket, key), + session, + ) actions = append(actions, a) case apiS3ListBuckets: - a := iamAction{ - Action: IAMActionS3ListAllMyBuckets, - Resource: "*", //Can only be granted on * - } + a := newIamAction( + IAMActionS3ListAllMyBuckets, + "*", //Can only be granted on * + session, + ) actions = append(actions, a) default: return nil, errors.New("cannot get IAM actions due to unsupported api action") diff --git a/cmd/policy-iam-action_test.go b/cmd/policy-iam-action_test.go index f6ad456..af6c2e2 100644 --- a/cmd/policy-iam-action_test.go +++ b/cmd/policy-iam-action_test.go @@ -24,7 +24,7 @@ type StubJustReturnIamAction struct{ func (p *StubJustReturnIamAction) Build(action S3ApiAction, presigned bool) http.HandlerFunc{ return func (w http.ResponseWriter, r *http.Request) { - actions, err := NewIamActionsFromS3Request(action, r) + actions, err := newIamActionsFromS3Request(action, r, testSessionDataTestDepartment) if err != nil { p.t.Error(err) return @@ -240,6 +240,26 @@ type apiAndIAMActionTestCase struct { ExpectedActions []iamAction } +var testSessionDataTestDepartment = &PolicySessionData{ + Claims: PolicySessionClaims{}, + Tags: AWSSessionTags{ + PrincipalTags: map[string][]string{ + "department": {"test"}, + }, + TransitiveTagKeys: []string{"department"}, + }, +} + +var testSessionDataQaDeparment = &PolicySessionData{ + Claims: PolicySessionClaims{}, + Tags: AWSSessionTags{ + PrincipalTags: map[string][]string{ + "department": {"qa"}, + }, + TransitiveTagKeys: []string{"department"}, + }, +} + //For each supported API we should add test coverage. This is used in this //file for checking wether it is mapped to the expected IAMActions and in //policy_api_action_test.go it is used to see if it is the expected APIAction @@ -249,7 +269,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "ListObjectsV2", ApiCall: runListObjectsV2AndReturnError, ExpectedActions: []iamAction{ - NewIamAction(IAMActionS3ListBucket, testBucketARN, contextType{ + newIamAction(IAMActionS3ListBucket, testBucketARN, testSessionDataTestDepartment).addContext(contextType{ IAMConditionS3Prefix: policy.NewConditionValueString(true, ""), }), }, @@ -258,7 +278,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "ListObjectsV2", ApiCall: runListObjectsV2WithPrefixAndReturnError, ExpectedActions: []iamAction{ - NewIamAction(IAMActionS3ListBucket, testBucketARN, contextType{ + newIamAction(IAMActionS3ListBucket, testBucketARN, testSessionDataTestDepartment).addContext(contextType{ IAMConditionS3Prefix: policy.NewConditionValueString(true, listobjectv2_test_prefix), }), }, @@ -267,14 +287,14 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "PutObject", ApiCall: runPutObjectAndReturnError, ExpectedActions: []iamAction{ - NewIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), }, }, { ApiAction: "GetObject", ApiCall: runGetObjectAndReturnError, ExpectedActions: []iamAction{ - NewIamAction(IAMActionS3GetObject, putObjectFullObjectARN, nil), + newIamAction(IAMActionS3GetObject, putObjectFullObjectARN, testSessionDataTestDepartment), }, }, // TODO: HeadObject behaves different client side and overrides the error so our hacky way of testing does not work @@ -291,14 +311,14 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "ListBuckets", ApiCall: runListBucketsAndReturnError, ExpectedActions: []iamAction{ - NewIamAction(IAMActionS3ListAllMyBuckets, "*", nil), + newIamAction(IAMActionS3ListAllMyBuckets, "*", testSessionDataTestDepartment), }, }, { ApiAction: "AbortMultipartUpload", ApiCall: runAbortMultipartUploadAndReturnError, ExpectedActions: []iamAction{ - NewIamAction(IAMActionS3AbortMultipartUpload, putObjectFullObjectARN, nil), + newIamAction(IAMActionS3AbortMultipartUpload, putObjectFullObjectARN, testSessionDataTestDepartment), }, }, { @@ -307,7 +327,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - NewIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), }, }, { @@ -316,7 +336,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - NewIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), }, }, { @@ -325,7 +345,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - NewIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), }, }, } diff --git a/cmd/proxysts_test.go b/cmd/proxysts_test.go index e6c414c..e162e6b 100644 --- a/cmd/proxysts_test.go +++ b/cmd/proxysts_test.go @@ -12,6 +12,8 @@ import ( "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/config" "github.com/aws/aws-sdk-go-v2/service/sts" + jwt "github.com/golang-jwt/jwt/v5" + "github.com/google/uuid" "github.com/spf13/viper" ) @@ -58,6 +60,108 @@ func TestProxySts(t *testing.T) { } } +func createBasicRS256PolicyToken(issuer, subject string, expiry time.Duration) (*jwt.Token) { + claims := &jwt.RegisteredClaims{ + ExpiresAt: jwt.NewNumericDate(time.Now().UTC().Add(expiry)), + IssuedAt: jwt.NewNumericDate(time.Now().UTC()), + NotBefore: jwt.NewNumericDate(time.Now().UTC()), + Issuer: issuer, + Subject: subject, + ID: uuid.New().String(), + } + + token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) + return token +} + +//Test the most basic web identity token which only has the subject +func TestProxyStsAssumeRoleWithWebIdentityBasicToken(t *testing.T) { + //Given valid server config + BindEnvVariables("proxysts") + _, err := loadOidcConfig([]byte(testConfigFakeTesting)) + if err != nil { + t.Error(err) + } + + signingKey, err := getTestSigningKey() + if err != nil { + t.Error("Could not get test signing key") + t.FailNow() + } + token, err := CreateSignedToken(createBasicRS256PolicyToken(testFakeIssuer, testSubject, 20 * time.Minute), signingKey) + if err != nil { + t.Error("Could create signed token") + t.FailNow() + } + + //Given the policy Manager that has roleArn for the testARN + pm = *NewTestPolicyManagerAllowAll() + + + url := buildAssumeRoleWithIdentityTokenUrl(901, "mysession", testPolicyAllowAllARN, token) + req, err := http.NewRequest("POST", url, nil) + + if err != nil { + t.Fatal(err) + } + rr := httptest.NewRecorder() + processSTSPost(rr, req) + if rr.Result().StatusCode != http.StatusOK { + t.Errorf("Could not assume role with testing token: %v", rr) + } +} + +func createRS256PolicyTokenWithSessionTags(issuer, subject string, expiry time.Duration) (*jwt.Token) { + tags := AWSSessionTags{ + PrincipalTags: map[string][]string{ + "custom_id": {"idA"}, + }, + TransitiveTagKeys: []string{"custom_id"}, + } + claims := newIDPClaims(issuer, subject, expiry, tags) + + token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) + return token +} + +//Test the most basic web identity token which only has the subject +func TestProxyStsAssumeRoleWithWebIdentitySessionTagsToken(t *testing.T) { + //Given valid server config + BindEnvVariables("proxysts") + _, err := loadOidcConfig([]byte(testConfigFakeTesting)) + if err != nil { + t.Error(err) + } + + signingKey, err := getTestSigningKey() + if err != nil { + t.Error("Could not get test signing key") + t.FailNow() + } + token, err := CreateSignedToken(createRS256PolicyTokenWithSessionTags(testFakeIssuer, testSubject, 20 * time.Minute), signingKey) + if err != nil { + t.Error("Could create signed token") + t.FailNow() + } + + //Given the policy Manager that has roleArn for the testARN + pm = *NewTestPolicyManagerAllowAll() + + + url := buildAssumeRoleWithIdentityTokenUrl(901, "mysession", testPolicyAllowAllARN, token) + req, err := http.NewRequest("POST", url, nil) + + if err != nil { + t.Fatal(err) + } + rr := httptest.NewRecorder() + processSTSPost(rr, req) + if rr.Result().StatusCode != http.StatusOK { + t.Errorf("Could not assume role with testing token: %v", rr) + } +} + + // This works like a fixture see https://medium.com/nerd-for-tech/setup-and-teardown-unit-test-in-go-bd6fa1b785cd func setupSuiteProxySTS(t *testing.T) func(t *testing.T) { // Make sure OIDC config is for testing From 13251f572e8f78ab52c1009f23a6777852d3041d Mon Sep 17 00:00:00 2001 From: Peter Van Bouwel Date: Sun, 17 Nov 2024 15:43:25 +0100 Subject: [PATCH 2/2] tests: improve iamaction and api action tests A convoluted way was created where a stub is created that always returns an error and where the return value is encapsulated in the error message. Given to how error message are build it was tricky to extract them and while this case worked for most actions it did not work for HeadObject. Given that these unittest just run in one execution environment it is easier to just introduce a global which gets updated by the stub. So after calling the stub the global will contain the created value. So this approach is easier and more reliable. Since our stub is called by anonymous requests we should not use session data with valid tags as it makes the tests a bit confusing so changed those. --- cmd/policy-api-action_test.go | 22 +++---- cmd/policy-iam-action.go | 3 + cmd/policy-iam-action_test.go | 108 ++++++++++++++-------------------- 3 files changed, 55 insertions(+), 78 deletions(-) diff --git a/cmd/policy-api-action_test.go b/cmd/policy-api-action_test.go index b1c0a4a..721701f 100644 --- a/cmd/policy-api-action_test.go +++ b/cmd/policy-api-action_test.go @@ -3,10 +3,7 @@ package cmd import ( "errors" "net/http" - "strings" "testing" - - sg "github.com/aws/smithy-go" ) @@ -14,11 +11,14 @@ type StubJustReturnApiAction struct{ t *testing.T } +var globalLastApiActionStubJustReturnApiAction S3ApiAction = "" + func (p *StubJustReturnApiAction) Build(action S3ApiAction, presigned bool) http.HandlerFunc{ return func (w http.ResponseWriter, r *http.Request) { //AWS CLI expects certain structure for ok responses //For error we could use the message field to pass a message regardless //of the api action + globalLastApiActionStubJustReturnApiAction = action writeS3ErrorResponse( buildContextWithRequestID(r), w, @@ -41,18 +41,12 @@ func TestExpectedAPIActionIdentified(t *testing.T) { for _, tc := range getApiAndIAMActionTestCases() { //see policy_iam_action_test err := tc.ApiCall(t) - smityError, ok := err.(*sg.OperationError) - if !ok { - t.Errorf("err was not smithy error %s", err) + if err == nil { + t.Errorf("%s: an error should have been returned", tc.ApiAction) } - accessDeniedParts := strings.Split(smityError.Error(), "AccessDenied: ") - if len(accessDeniedParts) < 2 { - t.Errorf("Encountered unexpected error (not Access Denied) %s", smityError) - continue - } - msg := accessDeniedParts[1] - if msg != tc.ApiAction { - t.Errorf("Expected %s, got %s, bug in router code", tc.ApiAction, msg) + + if tc.ApiAction != string(globalLastApiActionStubJustReturnApiAction) { + t.Errorf("wrong APIAction identified; expected %s, got %s", tc.ApiAction, globalLastApiActionStubJustReturnApiAction) } } } \ No newline at end of file diff --git a/cmd/policy-iam-action.go b/cmd/policy-iam-action.go index 3229bc7..97858cd 100644 --- a/cmd/policy-iam-action.go +++ b/cmd/policy-iam-action.go @@ -68,6 +68,9 @@ func addGenericSessionContextKeys(context map[string]*policy.ConditionValue, ses //Add aws:PrincipalTag/tag-key keys that are added to nearly all requests that contain information about the current session //https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_condition-keys.html#condition-keys-principaltag func addAwsPrincipalTagConditionKeys(context map[string]*policy.ConditionValue, session *PolicySessionData) { + if session == nil { + return + } for tagKey, tagValues := range session.Tags.PrincipalTags { context[fmt.Sprintf("aws:PrincipalTag/%s", tagKey)] = policy.NewConditionValueString(true, tagValues...) } diff --git a/cmd/policy-iam-action_test.go b/cmd/policy-iam-action_test.go index af6c2e2..bc4d2de 100644 --- a/cmd/policy-iam-action_test.go +++ b/cmd/policy-iam-action_test.go @@ -8,13 +8,11 @@ import ( "fmt" "net/http" "reflect" - "strings" "testing" "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" - sg "github.com/aws/smithy-go" "github.com/micahhausler/aws-iam-policy/policy" ) @@ -22,13 +20,16 @@ type StubJustReturnIamAction struct{ t *testing.T } +var latestIamActionInStubReturnIamAction []iamAction = nil + func (p *StubJustReturnIamAction) Build(action S3ApiAction, presigned bool) http.HandlerFunc{ return func (w http.ResponseWriter, r *http.Request) { - actions, err := newIamActionsFromS3Request(action, r, testSessionDataTestDepartment) + actions, err := newIamActionsFromS3Request(action, r, nil) if err != nil { p.t.Error(err) return } + latestIamActionInStubReturnIamAction = actions bytes, err := json.Marshal(actions) if err != nil { p.t.Error(err) @@ -146,21 +147,20 @@ func runGetObjectAndReturnError(t *testing.T) error { return err } -// TODO: Check how to get this under test coverage -// func runHeadObjectAndReturnError(t *testing.T) error { -// client, max1Sec, cancel := getAnonymousS3TestClient(t) - -// input := s3.HeadObjectInput{ -// Bucket: &testBucketName, -// Key: &putObjectTestKey, -// } -// defer cancel() -// _, err := client.HeadObject(max1Sec, &input) -// if err == nil { -// t.Error("Should have encountered error but did not") -// } -// return err -// } +func runHeadObjectAndReturnError(t *testing.T) error { + client, max1Sec, cancel := getAnonymousS3TestClient(t) + + input := s3.HeadObjectInput{ + Bucket: &testBucketName, + Key: &putObjectTestKey, + } + defer cancel() + _, err := client.HeadObject(max1Sec, &input) + if err == nil { + t.Error("Should have encountered error but did not") + } + return err +} func runAbortMultipartUploadAndReturnError(t *testing.T) error { client, max1Sec, cancel := getAnonymousS3TestClient(t) @@ -269,7 +269,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "ListObjectsV2", ApiCall: runListObjectsV2AndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3ListBucket, testBucketARN, testSessionDataTestDepartment).addContext(contextType{ + newIamAction(IAMActionS3ListBucket, testBucketARN, nil).addContext(contextType{ IAMConditionS3Prefix: policy.NewConditionValueString(true, ""), }), }, @@ -278,7 +278,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "ListObjectsV2", ApiCall: runListObjectsV2WithPrefixAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3ListBucket, testBucketARN, testSessionDataTestDepartment).addContext(contextType{ + newIamAction(IAMActionS3ListBucket, testBucketARN, nil).addContext(contextType{ IAMConditionS3Prefix: policy.NewConditionValueString(true, listobjectv2_test_prefix), }), }, @@ -287,38 +287,37 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ApiAction: "PutObject", ApiCall: runPutObjectAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), }, }, { ApiAction: "GetObject", ApiCall: runGetObjectAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3GetObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3GetObject, putObjectFullObjectARN, nil), + }, + }, + { + ApiAction: "HeadObject", + ApiCall: runHeadObjectAndReturnError, + ExpectedActions: []iamAction{ + // https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html + // To use HEAD, you must have the s3:GetObject permission. + newIamAction(IAMActionS3GetObject, putObjectFullObjectARN, nil), }, }, - // TODO: HeadObject behaves different client side and overrides the error so our hacky way of testing does not work - // { - // ApiAction: "HeadObject", - // ApiCall: runHeadObjectAndReturnError, - // ExpectedActions: []iamAction{ - // // https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html - // // To use HEAD, you must have the s3:GetObject permission. - // NewIamAction(IAMActionS3GetObject, putObjectFullObjectARN, nil), - // }, - // }, { ApiAction: "ListBuckets", ApiCall: runListBucketsAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3ListAllMyBuckets, "*", testSessionDataTestDepartment), + newIamAction(IAMActionS3ListAllMyBuckets, "*", nil), }, }, { ApiAction: "AbortMultipartUpload", ApiCall: runAbortMultipartUploadAndReturnError, ExpectedActions: []iamAction{ - newIamAction(IAMActionS3AbortMultipartUpload, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3AbortMultipartUpload, putObjectFullObjectARN, nil), }, }, { @@ -327,7 +326,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), }, }, { @@ -336,7 +335,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), }, }, { @@ -345,7 +344,7 @@ func getApiAndIAMActionTestCases() ([]apiAndIAMActionTestCase) { ExpectedActions: []iamAction{ //https://docs.aws.amazon.com/AmazonS3/latest/userguide/mpuoverview.html //You must be allowed to perform the s3:PutObject action on an object to initiate multipart upload. - newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, testSessionDataTestDepartment), + newIamAction(IAMActionS3PutObject, putObjectFullObjectARN, nil), }, }, } @@ -365,34 +364,15 @@ func TestExpectedIamActionsAreReturned(t *testing.T) { for _, tc := range getApiAndIAMActionTestCases() { err := tc.ApiCall(t) - smityError, ok := err.(*sg.OperationError) - if !ok { - t.Errorf("err was not smithy error %s", err) - continue - } - accessDeniedParts := strings.Split(smityError.Error(), "AccessDenied: ") - if len(accessDeniedParts) < 2 { - t.Errorf("Encountered unexpected error (not Access Denied) %s", smityError) - continue + if err == nil { + t.Errorf("%s: by design the stub should return an error but we did not get one.", tc.ApiAction) + t.FailNow() } - msg := accessDeniedParts[1] - var actions []iamAction - err = json.Unmarshal([]byte(msg), &actions) - if err != nil { - t.Error(err) - } - if !reflect.DeepEqual(actions, tc.ExpectedActions) { - if len(actions) != len(tc.ExpectedActions) { - printPointerAndJSONStringComparison(t, tc.ApiAction, tc.ExpectedActions, actions) - } else { - //Same amount of actions string and pointer representations might not show the issue let's compare 1-by 1 - for i, action := range actions { - expectedAction := tc.ExpectedActions[i] - if !reflect.DeepEqual(action, expectedAction) { - printPointerAndJSONStringComparison(t, tc.ApiAction, expectedAction, action) - } - } - } + + if !reflect.DeepEqual(latestIamActionInStubReturnIamAction, tc.ExpectedActions) { + printPointerAndJSONStringComparison(t, tc.ApiAction, tc.ExpectedActions, latestIamActionInStubReturnIamAction) + t.Errorf("unexpected actions got %v, expected %v", latestIamActionInStubReturnIamAction, tc.ExpectedActions) } + } } \ No newline at end of file