Skip to content

Commit 8619376

Browse files
committed
refactor(blobmanager/s3accesspoint): tighten SessionPolicyARN validation
Replace the loose prefix-and-substring check with a proper arn.Parse plus service and resource-type assertions, so configuration mistakes that previously slipped through (role ARN whose path embeds ":policy/", policy ARN with an empty name, malformed ARNs) are now rejected at credential-load time instead of surfacing as opaque STS errors during upload. Adds AWS-managed-policy ARNs (arn:aws:iam::aws:policy/...) to the accepted set and locks down the previously-permissive shapes with table-driven test cases. Assisted-by: Claude Signed-off-by: Jose I. Paris <jiparis@chainloop.dev> Chainloop-Trace-Sessions: ee4385fa-8011-4750-87cd-502097dda8b9
1 parent 3f09fbd commit 8619376

2 files changed

Lines changed: 39 additions & 5 deletions

File tree

‎pkg/blobmanager/s3accesspoint/provider.go‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ import (
5656
"strings"
5757
"time"
5858

59+
"github.com/aws/aws-sdk-go-v2/aws/arn"
5960
backend "github.com/chainloop-dev/chainloop/pkg/blobmanager"
6061
"github.com/chainloop-dev/chainloop/pkg/credentials"
6162
)
@@ -152,12 +153,18 @@ func (c *Credentials) Validate() error {
152153
return fmt.Errorf("%w: base_role_arn %q is not a valid IAM role ARN", backend.ErrValidation, c.BaseRoleARN)
153154
}
154155
}
155-
// SessionPolicyARN is optional. When set it must look like a managed
156-
// IAM policy ARN — anything else (an S3 ARN, a role ARN) would be
157-
// silently rejected by STS at request time, surfacing as opaque
158-
// errors deep in the upload path. Fail loudly here instead.
156+
// SessionPolicyARN is optional. When set it must be a syntactically
157+
// valid IAM managed policy ARN — anything else (an S3 ARN, a role
158+
// ARN, a policy ARN with no name, a role ARN that happens to embed
159+
// ":policy/" in its path) would be silently rejected by STS at
160+
// request time, surfacing as opaque errors deep in the upload path.
161+
// Fail loudly here instead.
159162
if c.SessionPolicyARN != "" {
160-
if !strings.HasPrefix(c.SessionPolicyARN, "arn:aws:iam::") || !strings.Contains(c.SessionPolicyARN, ":policy/") {
163+
a, err := arn.Parse(c.SessionPolicyARN)
164+
if err != nil ||
165+
a.Service != "iam" ||
166+
!strings.HasPrefix(a.Resource, "policy/") ||
167+
a.Resource == "policy/" {
161168
return fmt.Errorf("%w: session_policy_arn %q is not a valid IAM managed policy ARN", backend.ErrValidation, c.SessionPolicyARN)
162169
}
163170
}

‎pkg/blobmanager/s3accesspoint/provider_test.go‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,33 @@ func TestCredentials_Validate(t *testing.T) {
9292
mutate: func(c *Credentials) { c.SessionPolicyARN = "not-an-arn-at-all" },
9393
wantErr: "not a valid IAM managed policy ARN",
9494
},
95+
{
96+
name: "session policy arn for AWS-managed policy accepted",
97+
mutate: func(c *Credentials) { c.SessionPolicyARN = "arn:aws:iam::aws:policy/AdministratorAccess" },
98+
wantErr: "",
99+
},
100+
{
101+
// previously the loose substring check would accept this because
102+
// the string both starts with arn:aws:iam:: and contains :policy/
103+
name: "session policy arn embedded in role path rejected",
104+
mutate: func(c *Credentials) { c.SessionPolicyARN = "arn:aws:iam::123456789012:role/foo:policy/bar" },
105+
wantErr: "not a valid IAM managed policy ARN",
106+
},
107+
{
108+
name: "session policy arn with empty name rejected",
109+
mutate: func(c *Credentials) { c.SessionPolicyARN = "arn:aws:iam::123456789012:policy/" },
110+
wantErr: "not a valid IAM managed policy ARN",
111+
},
112+
{
113+
name: "session policy arn missing service rejected",
114+
mutate: func(c *Credentials) { c.SessionPolicyARN = "arn:aws::us-east-1:123456789012:policy/foo" },
115+
wantErr: "not a valid IAM managed policy ARN",
116+
},
117+
{
118+
name: "session policy arn structurally malformed rejected",
119+
mutate: func(c *Credentials) { c.SessionPolicyARN = "arn:aws:iam" },
120+
wantErr: "not a valid IAM managed policy ARN",
121+
},
95122
}
96123
for _, tc := range tests {
97124
t.Run(tc.name, func(t *testing.T) {

0 commit comments

Comments
 (0)