Conversation
Update the crypto spiffe handler to support for per-audience JWT SVIDs. This allows for the requester to only request for the specific audience it needs, rather than having to accept all audiences in the SVID. Signed-off-by: joshvanl <me@joshvanl.dev>
cicoyle
left a comment
cicoyle
left a comment
There was a problem hiding this comment.
lgtm, just one outstanding comment that needs to be addressed
Signed-off-by: joshvanl <me@joshvanl.dev>
|
@cicoyle done, PTAL |
Co-authored-by: Albert Callarisa <albert@acroca.com> Signed-off-by: Josh van Leeuwen <me@joshvanl.dev>
There was a problem hiding this comment.
Pull request overview
This PR extends the SPIFFE credential handler to support returning JWT SVIDs scoped to a single requested audience, using a per-audience cache when available.
Changes:
- Added per-audience JWT SVID storage (
currentPerAudJWTSVID) alongside the existing “base” JWT SVID. - Updated
FetchJWTSVIDto prefer a per-audience JWT SVID for the requested audience, falling back to the base JWT SVID. - Added unit tests covering per-audience selection behavior in the JWT SVID source.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
crypto/spiffe/svidsource.go |
Selects per-audience JWT SVIDs by requested audience, with fallback to the base JWT SVID. |
crypto/spiffe/spiffe.go |
Adds per-audience JWT support in identity storage and parsing from SVIDResponse. |
crypto/spiffe/svidsource_test.go |
Expands tests to cover per-audience JWT SVID selection and mismatch cases. |
Comments suppressed due to low confidence (1)
crypto/spiffe/svidsource_test.go:145
- The "PER: should return error when audience doesn't match" subtest largely duplicates the prior "should return error when audience doesn't match" case (same setup/expectations, with the per-audience map not affecting the outcome because the requested key is missing). Consider consolidating these into a table-driven test or adjusting the PER case to exercise a distinct per-audience-specific behavior.
t.Run("PER: should return error when audience doesn't match", func(t *testing.T) {
// Create a mock SVID with a specific audience
mockJWTSVID, err := createMockJWTSVID([]string{"actual-audience"})
require.NoError(t, err)
s := &svidSource{
spiffe: &SPIFFE{
readyCh: make(chan struct{}),
lock: sync.RWMutex{},
currentBaseJWTSVID: mockJWTSVID,
currentPerAudJWTSVID: map[string]*jwtsvid.SVID{
"actual-audience": mockJWTSVID,
},
},
}
close(s.spiffe.readyCh) // Mark as ready
svid, err := s.FetchJWTSVID(t.Context(), jwtsvid.Params{
Audience: "requested-audience",
})
require.Nil(t, svid)
require.Error(t, err)
// Verify the specific error type and contents
audienceErr, ok := err.(*audienceMismatchError)
require.True(t, ok, "Expected audienceMismatchError")
require.Equal(t, "JWT SVID has different audiences than requested: expected requested-audience, got actual-audience", audienceErr.Error())
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s.lock.RLock() | ||
| cert := s.currentX509SVID.Certificates[0] | ||
| jwtSVID := s.currentJWTSVID | ||
| jwtSVID := s.currentBaseJWTSVID | ||
| s.lock.RUnlock() |
There was a problem hiding this comment.
The rotation scheduler still computes renewal time using only the base JWT SVID. Per-audience JWT SVIDs may have earlier expiries, which could leave expired per-audience tokens being served until the next cert/base-JWT renewal. Consider incorporating the earliest expiry among all JWT SVIDs (base + per-audience) when calculating renewTime.
| // SVIDResponse represents the response from the SVID request function, | ||
| // containing both X.509 certificates and a JWT token. | ||
| type SVIDResponse struct { | ||
| X509Certificates []*x509.Certificate | ||
| JWT *string | ||
| PerAudienceJWT map[string]string | ||
| } | ||
|
|
||
| // Identity contains both X.509 and JWT SVIDs for a workload. | ||
| type Identity struct { | ||
| X509SVID *x509svid.SVID | ||
| JWTSVID *jwtsvid.SVID | ||
| X509SVID *x509svid.SVID | ||
| JWTSVID *jwtsvid.SVID | ||
| PerAudienceJWTSVID map[string]*jwtsvid.SVID | ||
| } |
There was a problem hiding this comment.
The struct comments for SVIDResponse/Identity mention only a single JWT token/SVID, but the types now also support per-audience JWTs. Updating these comments would help avoid confusion for implementers of RequestSVIDFn and future maintainers.
| for aud, token := range svidResponse.PerAudienceJWT { | ||
| jwtSvid, err := jwtsvid.ParseInsecure(token, []string{aud}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse JWT SVID: %w", err) | ||
| } |
There was a problem hiding this comment.
When parsing per-audience JWTs, the returned error doesn't include which audience key failed. Including the audience (and possibly whether it's from PerAudienceJWT vs the base JWT) would make troubleshooting malformed tokens significantly easier.
| for aud, token := range svidResponse.PerAudienceJWT { | ||
| jwtSvid, err := jwtsvid.ParseInsecure(token, []string{aud}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse JWT SVID: %w", err) | ||
| } | ||
|
|
||
| if identity.PerAudienceJWTSVID == nil { | ||
| identity.PerAudienceJWTSVID = make(map[string]*jwtsvid.SVID) | ||
| } | ||
| identity.PerAudienceJWTSVID[aud] = jwtSvid | ||
| s.log.Infof("Successfully received per-audience JWT SVID for audience %s with expiry: %s", aud, jwtSvid.Expiry.String()) | ||
| } |
There was a problem hiding this comment.
This new loop adds behavior to parse and store per-audience JWT SVIDs, but there doesn't appear to be test coverage validating that (1) PerAudienceJWT values from RequestSVIDFn are parsed into Identity.PerAudienceJWTSVID and (2) those entries are then served by FetchJWTSVID. Adding a focused unit test around fetchIdentity/Run would help prevent regressions.
3 participants
Update the crypto spiffe handler to support for per-audience JWT SVIDs. This allows for the requester to only request for the specific audience it needs, rather than having to accept all audiences in the SVID.