mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-15 09:01:31 +00:00
fix: carry the trust-policy validation reason to the caller (#1046)
ValidateTrustPolicyDocument builds precise errors naming the statement index and the offending element, and both callers discarded them into slog.Debug and returned a bare MalformedPolicyDocument. Debug is below the default level, so the reason reached nobody: not the caller, and not the operator reading the gateway log. The contrast with the identity-policy path was stark. CreatePolicy names the operator and the key exactly; CreateRole answered only 'The policy document is malformed.', so an operator could not tell whether Principal, Action, a size limit or a Condition block was at fault. The trust gate is the one the terraform ECS module hits, which is what made this worth fixing on its own. Both sites now use awserrors.Errorf, the same mechanism the identity path uses. The separate Debug line is dropped rather than duplicated: the gateway already logs a returned error at Error level, so one message now serves both the client and the operator.
This commit is contained in:
@@ -56,9 +56,12 @@ func (s *IAMServiceImpl) CreateRole(accountID string, input *iam.CreateRoleInput
|
||||
}
|
||||
}
|
||||
|
||||
// Carry the reason: a bare code cannot tell a caller which statement or key
|
||||
// was refused, and the gateway logs the returned error, so one message
|
||||
// serves both the client and the operator.
|
||||
if _, err := ValidateTrustPolicyDocument(*input.AssumeRolePolicyDocument); err != nil {
|
||||
slog.Debug("CreateRole: invalid trust policy", "roleName", roleName, "err", err)
|
||||
return nil, errors.New(awserrors.ErrorIAMMalformedPolicyDocument)
|
||||
return nil, awserrors.Errorf(awserrors.ErrorIAMMalformedPolicyDocument,
|
||||
"trust policy for role %q: %w", roleName, err)
|
||||
}
|
||||
|
||||
maxSession := defaultMaxSessionDuration
|
||||
@@ -245,8 +248,8 @@ func (s *IAMServiceImpl) UpdateAssumeRolePolicy(accountID string, input *iam.Upd
|
||||
roleName := *input.RoleName
|
||||
|
||||
if _, err := ValidateTrustPolicyDocument(*input.PolicyDocument); err != nil {
|
||||
slog.Debug("UpdateAssumeRolePolicy: invalid trust policy", "roleName", roleName, "err", err)
|
||||
return nil, errors.New(awserrors.ErrorIAMMalformedPolicyDocument)
|
||||
return nil, awserrors.Errorf(awserrors.ErrorIAMMalformedPolicyDocument,
|
||||
"trust policy for role %q: %w", roleName, err)
|
||||
}
|
||||
|
||||
role, err := s.getRole(ctx, accountID, roleName)
|
||||
|
||||
@@ -158,6 +158,66 @@ func TestCreateRole_PermissionsBoundaryAbsent(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// The code alone cannot tell a caller which statement or element was refused,
|
||||
// so these assert the reason survives to the client, not only the code.
|
||||
func TestCreateRole_MalformedTrustPolicy_NamesTheReason(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
cases := []struct {
|
||||
name string
|
||||
doc string
|
||||
want []string
|
||||
}{
|
||||
{
|
||||
name: "condition-on-a-non-web-identity-action",
|
||||
doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceAccount":"000000000001"}}}]}`,
|
||||
want: []string{"statement 0", STSActionAssumeRoleWithWebIdentity},
|
||||
},
|
||||
{
|
||||
name: "bad-effect",
|
||||
doc: `{"Version":"2012-10-17","Statement":[{"Effect":"Maybe","Principal":{"Service":"ec2.amazonaws.com"},"Action":"sts:AssumeRole"}]}`,
|
||||
want: []string{"statement 0"},
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
svc := setupTestIAMService(t)
|
||||
|
||||
_, err := svc.CreateRole(testAccountID, &iam.CreateRoleInput{
|
||||
RoleName: aws.String("reason-" + tc.name),
|
||||
AssumeRolePolicyDocument: aws.String(tc.doc),
|
||||
})
|
||||
require.Error(t, err)
|
||||
code, message, ok := awserrors.ResolveErrorDetail(err)
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, awserrors.ErrorIAMMalformedPolicyDocument, code)
|
||||
for _, want := range tc.want {
|
||||
assert.Contains(t, message, want)
|
||||
}
|
||||
assert.Contains(t, message, "reason-"+tc.name)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestUpdateAssumeRolePolicy_MalformedTrustPolicy_NamesTheReason(t *testing.T) {
|
||||
t.Parallel()
|
||||
svc := setupTestIAMService(t)
|
||||
createTestRole(t, svc, "update-reason")
|
||||
|
||||
_, err := svc.UpdateAssumeRolePolicy(testAccountID, &iam.UpdateAssumeRolePolicyInput{
|
||||
RoleName: aws.String("update-reason"),
|
||||
PolicyDocument: aws.String(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"ecs-tasks.amazonaws.com"},"Action":"sts:AssumeRole","Condition":{"StringEquals":{"aws:SourceAccount":"000000000001"}}}]}`),
|
||||
})
|
||||
require.Error(t, err)
|
||||
code, message, ok := awserrors.ResolveErrorDetail(err)
|
||||
require.True(t, ok)
|
||||
assert.Equal(t, awserrors.ErrorIAMMalformedPolicyDocument, code)
|
||||
assert.Contains(t, message, "update-reason")
|
||||
assert.Contains(t, message, STSActionAssumeRoleWithWebIdentity)
|
||||
}
|
||||
|
||||
func TestCreateRole_MalformedTrustPolicy_InvalidJSON(t *testing.T) {
|
||||
t.Parallel()
|
||||
svc := setupTestIAMService(t)
|
||||
|
||||
Reference in New Issue
Block a user