spiffeid: enforce 2048-byte maximum SPIFFE ID length#393
Conversation
The SPIFFE specification recommends that implementations not generate SPIFFE IDs longer than 2048 bytes (SPIFFE-ID spec section 2.3, Maximum SPIFFE ID Length). The spiffeid package previously applied no length limit, so constructors and parsers happily produced or accepted IDs of arbitrary size. Add a maxIDLen (2048) check at the points where a SPIFFE ID string is finalized: - makeID, the convergence point for FromPath, FromPathf, FromSegments, ReplacePath, ReplacePathf, ReplaceSegments, and TrustDomain.ID(). - FromString, which parses directly without going through makeID and so needs its own check (also covers FromStringf and FromURI). - AppendPath, AppendPathf, and AppendSegments, which mutate id.id directly and bypass makeID. A new errIDTooLong sentinel is returned for over-length IDs. Valid IDs up to and including 2048 bytes are unaffected. Adds TestIDMaxLength covering the boundary (exactly 2048 accepted, 2049 rejected) across FromPath, FromString, FromSegments, and the Append* methods, plus a sanity check that a normal ID still constructs. Fixes spiffe#384 Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
arpitjain099
requested review from
MarcosDY,
amartinezfayo,
azdagron and
evan2645
as code owners
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @arpitjain099!
| return ID{}, err | ||
| } | ||
|
|
||
| if len(id) > maxIDLen { |
There was a problem hiding this comment.
I think we should be careful here and scope the enforcement to generation only, dropping this guard in FromString. FromString is our acceptance path: it backs FromURI in x509svid/verify.go and the subject parse in jwtsvid/svid.go, so a length check here becomes a verification rule and would make us reject a peer's cryptographically-valid, properly-signed SVID solely because its ID is long. The spec frames 2048 as "SHOULD NOT generate" plus "MUST support up to 2048 bytes," with no requirement to reject longer IDs on parse, so rejecting them trades interoperability for no security gain and runs against being liberal in what we accept.
I think that the generation-side checks in makeID and the Append* methods cover what #384 is really after, since those are paths where local code constructs IDs it controls and erroring early can't cause an interop regression. If we ever do want the verify side to reject over-length IDs, I'd prefer we make that its own deliberate, documented behavior change rather than fold it into this one.
What do you think?
2 participants
Fixes #384
Problem
The
spiffeidpackage never enforces the SPIFFE specification's 2048-byte maximum length for a SPIFFE ID. An ID well over the limit can be constructed and parsed without error, so a library consumer can produce IDs that other SPIFFE implementations are not required to accept.FromPath(td, "/" + strings.Repeat("a", 3000))and the equivalentFromString(...)both return a 3021-byte ID with a nil error today.Root cause
makeIDis the convergence point for most constructors but has no length check.FromStringparses directly and bypassesmakeID, and theAppend*methods mutateid.iddirectly and also bypass it.Fix
Added
const maxIDLen = 2048and anerrIDTooLongsentinel, with a length check in:makeID(coversFromPath,FromPathf,FromSegments,ReplacePath/f,ReplaceSegments,TrustDomain.ID())FromString(coversFromStringf,FromURI, and the trust-domain URI path transitively)AppendPath,AppendPathf,AppendSegmentsTrust-domain constructors do not need a separate guard (a bare trust domain is at most a few hundred bytes once wrapped; the RFC 3986 255-byte trust-domain limit is a distinct concern and out of scope for this issue). Valid IDs (<= 2048 bytes) are unaffected.
Tests
Added
TestIDMaxLengthwith explicit boundaries (exactly 2048 accepted, 2049 rejected) acrossFromPath,FromString,FromSegments,AppendPath,AppendPathf, andAppendSegments, plus a normal-ID sanity check. Before the change the over-length construction succeeds; after it returns the error.go test ./spiffeid/...and the whole-modulego test ./...pass;go vetand gofmt are clean.A note on spec wording
The SPIFFE ID spec says a SPIFFE Server "SHOULD NOT generate URIs of length greater than 2048 bytes" and that implementations "MUST support URIs up to 2048 bytes." Returning a hard error on construction is therefore slightly stricter than the literal "SHOULD NOT," and it prevents this library from generating IDs that are not guaranteed to interoperate. If you would prefer to keep this non-fatal (for example gated behind an option, or a warning), I am happy to adjust; I went with a hard error since #384 asks for the limit to be enforced.