Skip to content

quic: enable TLS client authentication support#39766

Closed
agrawroh wants to merge 3 commits into
envoyproxy:mainfrom
agrawroh:quic-client-cert
Closed

quic: enable TLS client authentication support#39766
agrawroh wants to merge 3 commits into
envoyproxy:mainfrom
agrawroh:quic-client-cert

Conversation

@agrawroh

@agrawroh agrawroh commented Jun 6, 2025

Copy link
Copy Markdown
Member

Description

This PR implements TLS client authentication (mTLS) support for QUIC connections, addressing the open TODO comment we have in QuicServerTransportSocketConfigFactory::createTransportSocketFactory().

We have modified the existing test to expect successful creation of transport socket factory with client authentication enabled and added new test cases to verify that the client certificate configuration is properly accepted and parsed.

This change aligns with RFC 9001 Section 4.4, which explicitly allows client authentication during the TLS handshake for QUIC connections.


Commit Message: quic: enable TLS client authentication support
Additional Description: Removed the TODO and added support for TLS client authentication for QUIC.
Risk Level: Low
Testing: Unit tests and integration tests added
Docs Changes: Added
Release Notes: Added

@repokitteh-read-only

Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #39766 was opened by agrawroh.

see: more, trace.

@agrawroh agrawroh marked this pull request as ready for review June 6, 2025 04:46
@agrawroh agrawroh assigned agrawroh and ggreenway and unassigned agrawroh Jun 6, 2025
@agrawroh agrawroh requested a review from ggreenway June 6, 2025 04:47

@ggreenway ggreenway left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this end up going through the existing validator in the TLS server context?

This change requires more tests. At a minimum, we need an integration test for:

  • happy path (client provides cert, and it validates)
  • cert with wrong SANs compared to config (connection gets denied)
  • no cert provided and validation configuration requires a cert (connection gets denied)
  • no cert provided and validation configuration allows no cert (connection gets allowed, and http/rbac features to check for that work as they do for h2)
    /wait

@ggreenway

Copy link
Copy Markdown
Member

cc @RyanTheOptimist and @danzh2010

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
@danzh2010

danzh2010 commented Jun 10, 2025

Copy link
Copy Markdown
Contributor

Thanks for working on this missing piece! Can you elaborate more about how the implementation is like? It seems that removing a TODO and early return in QUIC integration code doesn't actually do client cert verification. Is QUICHE exposing client certs somewhere? How is it wired up with Envoy async cert validator?

@agrawroh agrawroh marked this pull request as draft June 10, 2025 17:07
@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 24, 2025
@github-actions

Copy link
Copy Markdown

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants