Use returned SPIFFE ID returned with JWT-SVID for caching #6501
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
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) |
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?
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) |
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()) |
There was a problem hiding this comment.
I think we should have fmt.Errorf with %w for error wrapping here.
| 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
2 participants
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.