-
Notifications
You must be signed in to change notification settings - Fork 12
fix: enforce SPIFFE 2048-byte ID cap at URI construction (§2.4) #106
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
18057c2
1381b2f
e291a8f
c07918a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package domain | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| // TestBuildWIMSEURIRejectsOver2048Bytes locks in SPIFFE §2.4. Today's | ||
| // varchar(255) schema makes this unreachable through the API surface — the | ||
| // test exists so a future schema bump can't silently mint over-cap IDs. | ||
| func TestBuildWIMSEURIRejectsOver2048Bytes(t *testing.T) { | ||
| // 2200-byte external_id forces the assembled URI past 2048 bytes | ||
| // regardless of any other field. | ||
| tooLong := strings.Repeat("a", 2200) | ||
|
|
||
| _, err := BuildWIMSEURI("highflame.ai", "acct", "proj", IdentityTypeAgent, tooLong) | ||
| if err == nil { | ||
| t.Fatalf("expected error for SPIFFE ID > %d bytes", MaxSPIFFEIDBytes) | ||
| } | ||
| if !strings.Contains(err.Error(), "exceeds 2048 bytes") { | ||
| t.Fatalf("error must name the cap so callers can act on it; got %q", err.Error()) | ||
| } | ||
| } | ||
|
|
||
| // TestBuildWIMSEURIAcceptsTypicalSize covers the happy path so we'd notice | ||
| // if a future change tightened the cap by accident. | ||
| func TestBuildWIMSEURIAcceptsTypicalSize(t *testing.T) { | ||
| uri, err := BuildWIMSEURI("highflame.ai", "acct-001", "proj-001", IdentityTypeAgent, "agent-1") | ||
| if err != nil { | ||
| t.Fatalf("unexpected error for short URI: %v", err) | ||
| } | ||
| want := "spiffe://highflame.ai/acct-001/proj-001/agent/agent-1" | ||
| if uri != want { | ||
| t.Fatalf("URI shape changed: got %q, want %q", uri, want) | ||
| } | ||
| } | ||
|
|
||
| // TestBuildWIMSEURIBoundary checks the inclusive boundary at exactly the | ||
| // cap. 2048 bytes must succeed; 2049 must fail. | ||
| func TestBuildWIMSEURIBoundary(t *testing.T) { | ||
| // Prefix length: "spiffe://" + "d" + "/" + "a" + "/" + "p" + "/" + | ||
| // "agent" + "/" = 9 + 1 + 1 + 1 + 1 + 1 + 1 + 5 + 1 = 21. | ||
| prefixLen := len("spiffe://d/a/p/agent/") | ||
| atCap := strings.Repeat("a", MaxSPIFFEIDBytes-prefixLen) | ||
|
|
||
| if _, err := BuildWIMSEURI("d", "a", "p", IdentityTypeAgent, atCap); err != nil { | ||
| t.Fatalf("URI of exactly %d bytes should be allowed: %v", MaxSPIFFEIDBytes, err) | ||
| } | ||
|
|
||
| overCap := atCap + "a" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we assert the error is actually the length exceeded ? |
||
| if _, err := BuildWIMSEURI("d", "a", "p", IdentityTypeAgent, overCap); err == nil { | ||
| t.Fatalf("URI of %d bytes should be rejected", MaxSPIFFEIDBytes+1) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -185,13 +185,18 @@ func (s *IdentityService) RegisterIdentity(ctx context.Context, req RegisterIden | |||||||||||||||||
| return nil, err | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| wimseURI, err := domain.BuildWIMSEURI(s.wimseDomain, req.AccountID, req.ProjectID, req.IdentityType, req.ExternalID) | ||||||||||||||||||
| if err != nil { | ||||||||||||||||||
| return nil, err | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+188
to
+191
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To maintain consistency with other error handling in
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| identity := &domain.Identity{ | ||||||||||||||||||
| ID: uuid.New().String(), | ||||||||||||||||||
| AccountID: req.AccountID, | ||||||||||||||||||
| ProjectID: req.ProjectID, | ||||||||||||||||||
| ExternalID: req.ExternalID, | ||||||||||||||||||
| Name: req.Name, | ||||||||||||||||||
| WIMSEURI: domain.BuildWIMSEURI(s.wimseDomain, req.AccountID, req.ProjectID, req.IdentityType, req.ExternalID), | ||||||||||||||||||
| WIMSEURI: wimseURI, | ||||||||||||||||||
| IdentityType: req.IdentityType, | ||||||||||||||||||
| SubType: req.SubType, | ||||||||||||||||||
| TrustLevel: req.TrustLevel, | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slicing a string by bytes (
uri[:64]) can be unsafe if the truncation happens in the middle of a multi-byte UTF-8 character. While SPIFFE IDs are expected to be ASCII-based URIs, using the%.64qformat specifier is more robust as it handles rune boundaries correctly, escapes special characters, and provides a clear, quoted representation in the error message.