Use returned SPIFFE ID returned with JWT-SVID for caching#6501
Merged
sorindumitru merged 3 commits intospiffe:mainfrom Jan 6, 2026
Merged
Use returned SPIFFE ID returned with JWT-SVID for caching#6501sorindumitru merged 3 commits intospiffe:mainfrom
sorindumitru merged 3 commits intospiffe:mainfrom
Conversation
We use our cached entry for requesting a new svid which may be different from the one that the server has. It may happen that entry gets updated around the same time that a cached SVID is renewed, including changing the SPIFFE ID. In this case the SPIFFE ID of the returned SVID may be different from the one of the entry we have. Because of the above we shouldn't use the SPIFFE ID of our entry for caching the SVIDs. The NewJWTSVID API also returns the SPIFFE ID of the issued SVID so we can use that for caching purposes. X509-SVID doesn't have this issue since there we cache using the entry ID. Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
sorindumitru requested review from MarcosDY, amartinezfayo, evan2645 and rturner3 as code owners 
Member
amartinezfayo left a comment
amartinezfayo left a comment There was a problem hiding this comment.
Thank you @sorindumitru for this!
If the SPIFFE ID changed, it seems like the old cached entry will remain in the cache forever? We may consider explicitly removing the old cache entry when the SPIFFE ID changes.
pkg/agent/client/client.go Outdated
| RenewSVID(ctx context.Context, csr []byte) (*X509SVID, error) | ||
| NewX509SVIDs(ctx context.Context, csrs map[string][]byte) (map[string]*X509SVID, error) | ||
| NewJWTSVID(ctx context.Context, entryID string, audience []string) (*JWTSVID, error) | ||
| NewJWTSVID(ctx context.Context, entryID string, audience []string) (*JWTSVID, string, error) |
Member
There was a problem hiding this comment.
Have you considered returning spiffeid.ID directly instead of string to avoid the double parsing and improve type safety?
Collaborator Author
There was a problem hiding this comment.
Yeah, that's probably best. I think I had in mind that the SPIFFE ID gets converted to string when it's used in the cache, so string is probably going to be better for this. But I can see how that goes in a separate PR.
pkg/agent/manager/manager.go Outdated
| return cachedSVID, nil | ||
| } | ||
| | ||
| spiffeID, err = spiffeid.FromString(spiffeIdString) |
Member
There was a problem hiding this comment.
Overwriting the spiffeID var may be confusing. The initial parsing is still used for cache lookup, but it's replaced with the server's SPIFFE ID. If the two differ, the cache lookup might miss?
We should probably have two different var names.
pkg/agent/manager/manager.go Outdated
| | ||
| spiffeID, err = spiffeid.FromString(spiffeIdString) | ||
| if err != nil { | ||
| return nil, errors.New("Invalid SPIFFE ID: " + err.Error()) |
Member
There was a problem hiding this comment.
I think we should have fmt.Errorf with %w for error wrapping here.
Suggested change
| return nil, errors.New("Invalid SPIFFE ID: " + err.Error()) | |
| return nil, fmt.Errorf("invalid SPIFFE ID: %w", err) |
Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
sorindumitru force-pushed the spiffeid-for-cache branch from ed166a1 to b42a0f0 Compare jsnctl pushed a commit to jsnctl/spire that referenced this pull request Jan 15, 2026
* Use returned SPIFFE ID returned with JWT-SVID for caching We use our cached entry for requesting a new svid which may be different from the one that the server has. It may happen that entry gets updated around the same time that a cached SVID is renewed, including changing the SPIFFE ID. In this case the SPIFFE ID of the returned SVID may be different from the one of the entry we have. Because of the above we shouldn't use the SPIFFE ID of our entry for caching the SVIDs. The NewJWTSVID API also returns the SPIFFE ID of the issued SVID so we can use that for caching purposes. X509-SVID doesn't have this issue since there we cache using the entry ID. Signed-off-by: Sorin Dumitru <sorin@returnze.ro> * Address review comments Signed-off-by: Sorin Dumitru <sorin@returnze.ro> --------- Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
MikeZappa87 pushed a commit to MikeZappa87/spire that referenced this pull request Mar 3, 2026
* Use returned SPIFFE ID returned with JWT-SVID for caching We use our cached entry for requesting a new svid which may be different from the one that the server has. It may happen that entry gets updated around the same time that a cached SVID is renewed, including changing the SPIFFE ID. In this case the SPIFFE ID of the returned SVID may be different from the one of the entry we have. Because of the above we shouldn't use the SPIFFE ID of our entry for caching the SVIDs. The NewJWTSVID API also returns the SPIFFE ID of the issued SVID so we can use that for caching purposes. X509-SVID doesn't have this issue since there we cache using the entry ID. Signed-off-by: Sorin Dumitru <sorin@returnze.ro> * Address review comments Signed-off-by: Sorin Dumitru <sorin@returnze.ro> --------- Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We use our cached entry for requesting a new svid which may be different from the one that the server has. It may happen that entry gets updated around the same time that a cached SVID is renewed, including changing the SPIFFE ID. In this case the SPIFFE ID of the returned SVID may be different from the one of the entry we have.
Because of the above we shouldn't use the SPIFFE ID of our entry for caching the SVIDs. The NewJWTSVID API also returns the SPIFFE ID of the issued SVID so we can use that for caching purposes.
X509-SVID doesn't have this issue since there we cache using the entry ID.