fix: harden JWT and X.509 SVID validation and error handling#375
Conversation
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thank you @maxlambrecht, this is a great improvement!
I have some comments / suggestions. My main concern is around the validation logic for URI SAN.
spiffe/src/spiffe/svid/x509_svid.py
Outdated
| 'Certificate contains multiple SPIFFE IDs in the URI SAN' | ||
| ) | ||
|
|
||
| return SpiffeId(spiffe_uris[0]) |
There was a problem hiding this comment.
This could raise an exception if the SPIFFE URI is malformed. Maybe it could be wrapped in a try-catch to provide clearer error context?
spiffe/src/spiffe/svid/x509_svid.py
Outdated
| ) | ||
| return SpiffeId(sans[0]) | ||
|
|
||
| if len(spiffe_uris) > 1: |
There was a problem hiding this comment.
It doesn't seem like we have test coverage for this new validation. I think that it would be great to have it covered by tests.
There was a problem hiding this comment.
Added test coverage.
spiffe/src/spiffe/svid/x509_svid.py
Outdated
| uri_sans = ext.value.get_values_for_type(x509.UniformResourceIdentifier) | ||
| spiffe_uris = [uri for uri in uri_sans if uri.startswith(spiffe_id.SCHEME_PREFIX)] | ||
|
|
||
| if len(spiffe_uris) == 0: | ||
| raise InvalidLeafCertificateError( | ||
| 'Certificate does not contain a SPIFFE ID in the URI SAN' | ||
| ) | ||
| return SpiffeId(sans[0]) | ||
|
|
||
| if len(spiffe_uris) > 1: | ||
| raise InvalidLeafCertificateError( | ||
| 'Certificate contains multiple SPIFFE IDs in the URI SAN' | ||
| ) |
There was a problem hiding this comment.
I think that this validation logic checks only SPIFFE URIs, not all URI SANs. This is a problem because the spec says that "An X.509 SVID MUST contain exactly one URI SAN, and by extension, exactly one SPIFFE ID.".
The way I see this code, a certificate with spiffe://example.org/service + https://example.org would still be accepted by py-spiffe.
There was a problem hiding this comment.
That's correct. Fixed
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
|
Thanks for the great review, @amartinezfayo! |
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thank you @maxlambrecht, looks great! 🎉
2 participants
Summary
This PR fixes several correctness and validation issues in the JWT-SVID and X.509-SVID implementations, with the goal of aligning behavior more closely with the SPIFFE specifications and avoiding silent acceptance of invalid identity material.
On the JWT-SVID side, the changes ensure that a token is rejected early if the
subclaim is missing or empty, rather than relying on downstream SPIFFE ID parsing to fail implicitly. The JWT decoding path is also tightened to use an already-validated algorithm when verifying signatures, and expiration (exp) is now treated explicitly as a numeric value during validation to avoid type-related edge cases.On the X.509-SVID side, validation is made stricter and more explicit. The code now fails with clear validation errors when required extensions (SubjectAlternativeName, BasicConstraints, KeyUsage) are missing, instead of propagating low-level exceptions. The leaf certificate validation enforces that exactly one SPIFFE URI SAN is present, as required by the SPIFFE X.509-SVID specification, and rejects certificates that contain multiple SPIFFE IDs.
These changes do not introduce API changes. They are intended to improve security, correctness, and spec compliance, and to make invalid SVIDs fail fast with clear, intentional errors.