Conversation
arndt-s
requested review from
amartinezfayo,
azdagron and
evan2645
as code owners
Signed-off-by: arndt-s <17650715+arndt-s@users.noreply.github.com>
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
This is great @arndt-s! Thank you so much for this contribution.
I have some comments/suggestions.
| return nil, err | ||
| } | ||
|
|
||
| if s.WitSvidKey != "" { |
There was a problem hiding this comment.
I'm wondering if the empty WitSvidKey case in parseWITSVIDs should error rather than produce an SVID with no PrivateKey. I think the proto declares the field as Required, and the WIT-SVID profile says that all WITSVID fields except hint are mandatory, and a WIT-SVID without the matching private key cannot be used for proof of possession (I believe that's the whole point of the credential).
A server returning one without the key looks more like a server bug worth surfacing than something that can be ignored. What do you think about returning an error when WitSvidKey is empty here?
| } | ||
| } | ||
|
|
||
| func TestWatchWITSVIDs_Unimplemented(t *testing.T) { |
There was a problem hiding this comment.
I think TestWatchWITSVIDs_Unimplemented may not actually exercise the codes.Unimplemented path. The fake fetchWITSVID returns noIdentityError (which is PermissionDenied) when no response is configured, and handleWatchError retries on PermissionDenied, so what the test ends up asserting is that the 200ms context timeout fires.
To exercise the early-exit behavior, would it make sense to teach the fake to return codes.Unimplemented from FetchWITSVID/FetchWITBundles and assert that the watch returns promptly with that status code?
|
|
||
| // FromJWTAuthorities creates a new bundle from a map of JWT authorities keyed | ||
| // by key ID. | ||
| func FromJWTAuthorities(trustDomain spiffeid.TrustDomain, jwtAuthorities map[string]crypto.PublicKey) *Bundle { |
There was a problem hiding this comment.
I wonder if the JWTAuthorities/FindJWTAuthority/AddJWTAuthority naming on witbundle.Bundle ends up being a bit confusing once spiffebundle.Bundle is in the picture, since the latter exposes the WIT-flavored counterparts as WITAuthorities/FindWITAuthority/etc. The mismatch is most visible in spiffebundle.FromWITBundle, which calls witBundle.JWTAuthorities() to populate bundle.witAuthorities, and in witsvid.ParseAndValidate, which calls bundle.FindJWTAuthority(keyID) on a witbundle to verify a WIT-SVID.
Would naming the witbundle methods WITAuthorities/FindWITAuthority, etc. make the two APIs feel more consistent?
| // WIT-SVIDs. The optional spiffeID filters updates to a specific identity. | ||
| // | ||
| // If the server returns codes.Unimplemented, the watch loop terminates without | ||
| // retrying per the WIMSE spec §7. |
There was a problem hiding this comment.
The Unimplemented-then-no-retry behavior is defined in the SPIFFE Workload API spec under §7 "WIT-SVID Profile", whereas the WIMSE documents specify the WIT token format itself rather than the workload API protocol.
Would it make sense to retitle the "WIMSE spec §7" citation here (and on WatchWITBundles)?
Pointing at "SPIFFE Workload API §7 (WIT-SVID Profile)" instead would attribute it correctly I think.
| } | ||
|
|
||
| // Verify expiry | ||
| if stdClaims.Expiry.Time().Before(time.Now()) { |
There was a problem hiding this comment.
Maybe we could also enforce the nbf claim in parse.
From what I understand in the WIT-SVID spec 3.5, it says nbf MAY be present and that "validators MUST reject WIT-SVIDs when the time indicated by nbf is in the future". The current parse only enforces exp, so a token whose nbf is in the future would be accepted.
Would extracting nbf alongside exp from stdClaims and rejecting when it lies after time.Now() work here?
| return nil, wrapErr(errors.New("cnf.jwk is not an object")) | ||
| } | ||
|
|
||
| if _, ok := jwkMap["alg"]; !ok { |
There was a problem hiding this comment.
What do you think about extending the check here from a presence test to a value check?
I think the spec lists the specific alg values the cnf.jwk.alg MUST take (the same RS/ES/PS suite already in allowedSignatureAlgorithms for the JOSE header). Without that enforcement, a token whose confirmation key declares an unsupported algorithm would parse here and only fail later when the workload tries to use it for proof of possession.
|
|
||
| type verifyFn func(*jwt.JSONWebToken, spiffeid.TrustDomain, string) error | ||
|
|
||
| func parse(token string, verify verifyFn) (*SVID, error) { |
There was a problem hiding this comment.
It could be worth adding direct _test.go files for the new witsvid and witbundle packages. It seems that the parse failure modes in witsvid.parse are only reached transitively through wit_client_test.go, and the witbundle helpers aren't directly covered at all.
The test coverage in svid/jwtsvid could be a useful template I think.
| func parseJWKPrivateKey(jwkStr string) (interface{}, error) { | ||
| var jwkKey jose.JSONWebKey | ||
| if err := jwkKey.UnmarshalJSON([]byte(jwkStr)); err != nil { | ||
| return nil, fmt.Errorf("witsvid: unable to parse private key JWK: %v", err) |
There was a problem hiding this comment.
nit: the "witsvid:" prefix in these errors looks a bit off. The function lives in the workloadapi package, and the rest of the WIT-related parsing in this file emits errors without that prefix, so the current message reads as if it came from a different layer than it actually does.
2 participants
This is the go-spiffe branch I've used for the KubeCon demo of WIT-SVIDs.