client/pkg/transport: Support SAN URIs in TLS peer verification

[LINKIWI]

This change proposes the addition of a config struct field AllowedURI, set at runtime via --client-cert-allowed-uri and --peer-cert-allowed-uri, to implement peer TLS verification based on the URIs specified in the peer certificate's subject alternative names.

This provides an alternative to the existing peer verification mechanisms based on CN and SAN DNS names. This change is primarily motivated by achieving compatibility with deployment environments that rely on SPIFFE validation.

I added an e2e test (with a corresponding new test fixture certificate with a SAN URI, which required regenerating all existing certs), though I'm not able to get it to run locally. I also plan to do some more manual testing, the results of which I'll paste in once they are ready.

[hexfusion]

Thanks for the contribution is there a design doc you can reference that gives the full details of SPIFFE validation?

[LINKIWI]

Thanks for taking a look. I don't have a design doc. SPIFFE validation is just one example use case; the patch itself is agnostic to whatever convention (or lack of convention) might be in place.

This change mostly aims to replicate the existing logic for CN and SAN DNS validation, but for URI field(s) as well.

Edit: To clarify, SPIFFE validation is not an objective of this PR.

[LINKIWI]

@hexfusion Is this feature something that would be desirable for etcd? If so, any feedback on the general direction would be appreciated before I investigate the e2e test breakage. Thanks.

[hexfusion]

I will try to take a look more this weekend but might take longer. We had issues in the past with CN SAN validation and our internal RBAC auth.

cc @mitake

[mitake]

@hexfusion got it, I'll also take a look during this weekend 👍

[mitake]

Thanks for your PR! I have 2 small comments, it's great if you can take a look @LINKIWI . I'll also take a look at test code changes later.

[mitake]

I think error message like exactly one of --peer-cert-allowed-cn, --peer-cert-allowed-hostname, or --peer-cert-allowed-uri can be defined might be clearer for users.

[LINKIWI]

This might be confusing for embedded etcd users since they would not be interfacing with any command line parameters. I'm happy to change it either way though.

[mitake]

@LINKIWI got it, then I think it's fine

[LINKIWI]

Thanks for your PR! I have 2 small comments, it's great if you can take a look @LINKIWI . I'll also take a look at test code changes later.

Thanks for the review. I pushed up some new commits to address the suggestions and also fixed the e2e test case (it is at least passing in my local environment now).

[mitake]

thanks for updating, let me check again later this week

[LINKIWI]

@mitake Wondering if you'd had a chance to make another pass through the change. Thanks

[mitake]

@LINKIWI sorry for my late update, I think the change should be fine if the existing tests are fine. I started the CIs.

[mitake]

LGTM, thanks! defer to @hexfusion

[mitake]

@hexfusion could you cross check when you have a time?

[mitake]

@hexfusion kindly ping?

[LINKIWI]

@mitake @hexfusion Any chance we can get this merged? Thanks.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

[fcantournet]

We're (Datadog) interested in upstreaming this PR.
We have a generic PKI for all services to get client certs with URI SANs (SPIFFE), and this would be very useful to us.
@LINKIWI can you re-open this ?

[LINKIWI]

Hi, we had an identical motivation for this PR. Unfortunately, we were unable to get further attention on this change from the maintainers as you can see above.

I don't work at the company anymore where we needed this; at this point I've lost context and the ability to verify this end-to-end. I believe we might have ended up just patching this in a fork. Please feel free to submit a new PR with this patch.