Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions spiffeid/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ var (
errMissingTrustDomain = errors.New("trust domain is missing")
errTrailingSlash = errors.New("path cannot have a trailing slash")
errWrongScheme = errors.New("scheme is missing or invalid")
errIDTooLong = errors.New("ID cannot be longer than 2048 bytes")
)
25 changes: 24 additions & 1 deletion spiffeid/id.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ import (
const (
schemePrefix = "spiffe://"
schemePrefixLen = len(schemePrefix)

// maxIDLen is the maximum length, in bytes, of a SPIFFE ID. The SPIFFE
// specification recommends that implementations not generate SPIFFE IDs
// longer than 2048 bytes so that they remain interoperable.
// See https://github.com/spiffe/spiffe/blob/main/standards/SPIFFE-ID.md#23-maximum-spiffe-id-length
maxIDLen = 2048
)

// FromPath returns a new SPIFFE ID in the given trust domain and with the
Expand Down Expand Up @@ -75,6 +81,10 @@ func FromString(id string) (ID, error) {
return ID{}, err
}

if len(id) > maxIDLen {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

return ID{}, errIDTooLong
}

return ID{
id: id,
pathidx: pathidx,
Expand Down Expand Up @@ -154,6 +164,9 @@ func (id ID) AppendPath(path string) (ID, error) {
return ID{}, err
}
id.id += path
if len(id.id) > maxIDLen {
return ID{}, errIDTooLong
}
return id, nil
}

Expand All @@ -170,6 +183,9 @@ func (id ID) AppendPathf(format string, args ...interface{}) (ID, error) {
return ID{}, err
}
id.id += path
if len(id.id) > maxIDLen {
return ID{}, errIDTooLong
}
return id, nil
}

Expand All @@ -186,6 +202,9 @@ func (id ID) AppendSegments(segments ...string) (ID, error) {
return ID{}, err
}
id.id += path
if len(id.id) > maxIDLen {
return ID{}, errIDTooLong
}
return id, nil
}

Expand Down Expand Up @@ -251,8 +270,12 @@ func makeID(td TrustDomain, path string) (ID, error) {
if td.IsZero() {
return ID{}, errors.New("trust domain is empty")
}
id := schemePrefix + td.name + path
if len(id) > maxIDLen {
return ID{}, errIDTooLong
}
return ID{
id: schemePrefix + td.name + path,
id: id,
pathidx: schemePrefixLen + len(td.name),
}, nil
}
75 changes: 75 additions & 0 deletions spiffeid/id_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"fmt"
"net/url"
"strings"
"testing"

"github.com/spiffe/go-spiffe/v2/spiffeid"
Expand Down Expand Up @@ -461,6 +462,80 @@ func TestIDTextUnmarshaler(t *testing.T) {
require.Equal(t, "spiffe://trustdomain/path", s.ID.String())
}

func TestIDMaxLength(t *testing.T) {
// The SPIFFE specification recommends that implementations not generate
// SPIFFE IDs longer than 2048 bytes.
// https://github.com/spiffe/spiffe/blob/main/standards/SPIFFE-ID.md#23-maximum-spiffe-id-length
const maxIDLen = 2048

maxTD := spiffeid.RequireTrustDomainFromString("example.org")
prefixLen := len(maxTD.IDString()) // "spiffe://example.org"

// pathForTotal returns a valid path whose bytes make the full ID exactly
// total bytes long.
pathForTotal := func(total int) string {
return "/" + strings.Repeat("a", total-prefixLen-1)
}

t.Run("FromPath at boundary", func(t *testing.T) {
// Exactly 2048 bytes is allowed.
id, err := spiffeid.FromPath(maxTD, pathForTotal(maxIDLen))
require.NoError(t, err)
require.Len(t, id.String(), maxIDLen)

// 2049 bytes is rejected.
_, err = spiffeid.FromPath(maxTD, pathForTotal(maxIDLen+1))
assertErrorContains(t, err, "ID cannot be longer than 2048 bytes")
})

t.Run("FromString at boundary", func(t *testing.T) {
ok := maxTD.IDString() + pathForTotal(maxIDLen)
id, err := spiffeid.FromString(ok)
require.NoError(t, err)
require.Len(t, id.String(), maxIDLen)

tooLong := maxTD.IDString() + pathForTotal(maxIDLen+1)
_, err = spiffeid.FromString(tooLong)
assertErrorContains(t, err, "ID cannot be longer than 2048 bytes")
})

t.Run("FromSegments rejects over-length", func(t *testing.T) {
_, err := spiffeid.FromSegments(maxTD, strings.Repeat("a", maxIDLen))
assertErrorContains(t, err, "ID cannot be longer than 2048 bytes")
})

t.Run("AppendPath rejects over-length", func(t *testing.T) {
// Start at exactly the limit, then append past it.
base, err := spiffeid.FromPath(maxTD, pathForTotal(maxIDLen))
require.NoError(t, err)
require.Len(t, base.String(), maxIDLen)
_, err = base.AppendPath("/x")
assertErrorContains(t, err, "ID cannot be longer than 2048 bytes")
})

t.Run("AppendPathf rejects over-length", func(t *testing.T) {
base, err := spiffeid.FromPath(maxTD, pathForTotal(maxIDLen))
require.NoError(t, err)
require.Len(t, base.String(), maxIDLen)
_, err = base.AppendPathf("/%s", "x")
assertErrorContains(t, err, "ID cannot be longer than 2048 bytes")
})

t.Run("AppendSegments rejects over-length", func(t *testing.T) {
base, err := spiffeid.FromPath(maxTD, pathForTotal(maxIDLen))
require.NoError(t, err)
require.Len(t, base.String(), maxIDLen)
_, err = base.AppendSegments("x")
assertErrorContains(t, err, "ID cannot be longer than 2048 bytes")
})

t.Run("normal ID still works", func(t *testing.T) {
id, err := spiffeid.FromPath(maxTD, "/workload")
require.NoError(t, err)
require.Equal(t, "spiffe://example.org/workload", id.String())
})
}

func BenchmarkIDFromString(b *testing.B) {
s := "spiffe://trustdomain/path"
for n := 0; n < b.N; n++ {
Expand Down
Loading