fix(x509svid): reject leaf SPIFFE IDs with root path#375
Conversation
maxlambrecht
requested review from
amartinezfayo,
azdagron and
evan2645
as code owners
strideynet
left a comment
strideynet
left a comment
There was a problem hiding this comment.
Implementation looks good and makes sense to me as per the spec.
However, I do worry that this could be a significant breaking change for those consuming a non-spec compliant implementation of SPIFFE. That being said, I don't think I have come across any implementations that are issuing root path SVIDs.
Do we need to at least flag this in the change-log as breaking? Is there any prior art for how we've handled changes in the SDK that could be breaking for non-compliant implementations of SPIFFE.
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
Thanks for the PR, @maxlambrecht. I also agree that the spec is clear here: Section 3.1 says leaf certificate SPIFFE IDs MUST have a non-root path component, so this is the right thing to do.
Regarding the breaking change concern that Noah raised, I don't think that we have a precedent for explicitly flagging breaking changes. I think we should document this clearly in the changelog under "Changed" with wording that makes it obvious this is now enforced, something like: "x509svid.Parse and x509svid.ParseRaw now reject leaf certificates with root-path SPIFFE IDs, in alignment with the X.509-SVID specification (Section 3.1)."
In practice, I think the risk is low. SPIRE and any spec-compliant SPIFFE implementation will not mint leaf SVIDs with root-path SPIFFE IDs, so this would only affect consumers using a non-compliant implementation as Noah pointed out.
| return nil, fmt.Errorf("cannot get leaf certificate SPIFFE ID: %v", err) | ||
| } | ||
|
|
||
| if leafID.Path() == "" || leafID.Path() == "/" { |
There was a problem hiding this comment.
nit: I think the leafID.Path() == "/" check is unnecessary here since spiffeid.FromURI already rejects URIs with a trailing slash like spiffe://example.org/. So Path() can never return "/" on a successfully parsed ID. Not harmful, but we could simplify this to just check for an empty path.
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
maxlambrecht
force-pushed
the
fix/x509svid-reject-root-path-spiffe-id
branch
from
a18d3f9 to
1381fdc
Compare
Signed-off-by: Max Lambrecht <maxlambrecht@gmail.com>
3 participants
This change updates
x509svid.Parseandx509svid.ParseRawto reject leaf certificates whose SPIFFE ID has a root/empty path (for example,spiffe://example.org). This aligns with the SPIFFE X.509-SVID spec (SPIFFE spec Section 3.1 "Leaf Certificates"), which requires that leaf certificate SPIFFE IDs MUST have a non-root path component.