Skip to content

fix: enforce SPIFFE 2048-byte ID cap at URI construction (§2.4)#106

Open
safayavatsal wants to merge 3 commits intohighflame-ai:mainfrom
safayavatsal:fix/spiffe-id-length-cap-48
Open

fix: enforce SPIFFE 2048-byte ID cap at URI construction (§2.4)#106
safayavatsal wants to merge 3 commits intohighflame-ai:mainfrom
safayavatsal:fix/spiffe-id-length-cap-48

Conversation

@safayavatsal
Copy link
Copy Markdown
Contributor

Summary

  • `domain.BuildWIMSEURI` was a `Sprintf` with no length check, so SPIFFE §2.4 ("MUST NOT exceed 2048 bytes") was unenforced.
  • Today's `varchar(255)` schema caps the assembled URI at ~1080 bytes, so this isn't reachable through the API surface — but the invariant belongs at the construction site so a future schema relaxation can't silently mint non-conformant SPIFFE IDs.
  • Changes `BuildWIMSEURI` to return `(string, error)`, introduces `domain.MaxSPIFFEIDBytes = 2048`, and propagates the error from `RegisterIdentity`.
  • Three domain tests cover rejection, the happy path, and the inclusive boundary at exactly 2048 bytes.

Caveat — public API signature change

`BuildWIMSEURI` is a top-level `domain` function, which `CONTRIBUTING.md` calls out as part of the public API. The single in-repo caller is updated; there are no other consumers (`pkg/authjwt` does not call it). Surfacing this so the maintainers can decide whether to ship the signature change or wrap with a `Must`-style helper.

Fixes #48

Test plan

  • `go vet ./...` clean
  • `go build ./...` clean
  • `go test ./domain/` (3 tests: rejection, happy path, inclusive boundary)
  • `make test` passes (full integration suite, ~42s)

BuildWIMSEURI was a Sprintf with no length check, so the SPIFFE §2.4
cap was unenforced. Today's varchar(255) schema bounds the URI to about
1080 bytes, so this isn't reachable through the API — but the invariant
belongs at the construction site so a future schema relaxation can't
silently mint non-conformant SPIFFE IDs.

Changes BuildWIMSEURI to return (string, error), introduces
MaxSPIFFEIDBytes = 2048, and propagates the error from RegisterIdentity.
Domain tests cover rejection, happy path, and the inclusive boundary at
exactly 2048 bytes.

Note: signature change to a public domain function. No external
consumers in the repo (pkg/authjwt does not call it).

Fixes highflame-ai#48
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements a 2048-byte size limit for SPIFFE IDs as per the SPIFFE specification, updating the BuildWIMSEURI function to return an error upon exceeding this limit. It also includes comprehensive unit tests for boundary conditions and updates the identity service to handle the new error return. Feedback was provided to improve the robustness of error message formatting by avoiding unsafe string slicing and to wrap errors in the service layer for better traceability.

Comment thread domain/identity.go
func BuildWIMSEURI(wimseDomain, accountID, projectID string, identityType IdentityType, externalID string) (string, error) {
uri := fmt.Sprintf("spiffe://%s/%s/%s/%s/%s", wimseDomain, accountID, projectID, identityType, externalID)
if n := len(uri); n > MaxSPIFFEIDBytes {
return "", fmt.Errorf("SPIFFE ID exceeds %d bytes (got %d): %s…", MaxSPIFFEIDBytes, n, uri[:64])
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.

medium

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 %.64q format specifier is more robust as it handles rune boundaries correctly, escapes special characters, and provides a clear, quoted representation in the error message.

Suggested change
return "", fmt.Errorf("SPIFFE ID exceeds %d bytes (got %d): %s…", MaxSPIFFEIDBytes, n, uri[:64])
return "", fmt.Errorf("SPIFFE ID exceeds %d bytes (got %d): %.64q...", MaxSPIFFEIDBytes, n, uri)

Comment on lines +160 to +163
wimseURI, err := domain.BuildWIMSEURI(s.wimseDomain, req.AccountID, req.ProjectID, req.IdentityType, req.ExternalID)
if err != nil {
return nil, err
}
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.

medium

To maintain consistency with other error handling in RegisterIdentity (e.g., line 196) and across the service, consider wrapping the error from domain.BuildWIMSEURI. This ensures that the error trace includes the context of the operation being performed.

Suggested change
wimseURI, err := domain.BuildWIMSEURI(s.wimseDomain, req.AccountID, req.ProjectID, req.IdentityType, req.ExternalID)
if err != nil {
return nil, err
}
wimseURI, err := domain.BuildWIMSEURI(s.wimseDomain, req.AccountID, req.ProjectID, req.IdentityType, req.ExternalID)
if err != nil {
return nil, fmt.Errorf("failed to build WIMSE URI: %w", err)
}

Copy link
Copy Markdown
Contributor

@saucam saucam left a comment

Choose a reason for hiding this comment

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

Please also resolve gemini comments

Comment thread domain/identity_test.go
t.Fatalf("URI of exactly %d bytes should be allowed: %v", MaxSPIFFEIDBytes, err)
}

overCap := atCap + "a"
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.

should we assert the error is actually the length exceeded ?

@rsharath
Copy link
Copy Markdown
Contributor

rsharath commented May 5, 2026

Thanks for the careful work on this — three asks before merge:

1. Apply Gemini's %.64q suggestion at domain/identity.go:353

Small safety win plus better operator output. uri[:64] byte-slicing is fine while every caller routes through RegisterIdentity (which #102 validates to ASCII), but BuildWIMSEURI is in the public domain/ surface and a future caller could pass non-ASCII; %.64q is rune-safe and quotes special characters in the error message.

2. Address @saucam's question with a sentinel error

Substring matching on the error string is fragile. Add:

// in domain/identity.go
var ErrSPIFFEIDTooLong = errors.New("SPIFFE ID exceeds maximum length")

Wrap it in BuildWIMSEURI:

return "", fmt.Errorf("%w: got %d bytes, max %d: %.64q",
    ErrSPIFFEIDTooLong, n, MaxSPIFFEIDBytes, uri)

Update the tests to use errors.Is(err, domain.ErrSPIFFEIDTooLong) instead of substring match.

Bonus: this also lets the handlers distinguish the cap-exceeded case from generic build failures and map it to 400 cleanly when the schema relaxes (today's varchar(255) makes the cap unreachable, but the invariant belongs at the construction site as you noted in the PR body).

3. Skip Gemini's error-wrap suggestion at internal/service/identity.go:177

The inner error from BuildWIMSEURI already self-describes ("SPIFFE ID exceeds 2048 bytes..."); wrapping it as fmt.Errorf("failed to build WIMSE URI: %w", err) adds verbosity without adding information. The "consistency with line 196" argument is weak — that line wraps a DB error which genuinely needs context, whereas this is a self-describing validation error.

The sentinel from #2 is the better wrap here — it preserves the existing message and lets callers branch on the typed error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SPIFFE: SPIFFE ID length not checked against 2048-byte limit

3 participants