Skip to content

fix(registry/handlers/app): redis CAs#4668

Merged
milosgajdos merged 2 commits intodistribution:mainfrom
ChandonPierre:cpierre/redis-rootcas
Sep 3, 2025
Merged

fix(registry/handlers/app): redis CAs#4668
milosgajdos merged 2 commits intodistribution:mainfrom
ChandonPierre:cpierre/redis-rootcas

Conversation

@ChandonPierre
Copy link
Copy Markdown
Contributor

configureRedis currently sets RequireAndVerifyClientCert and ClientCAs, however these are server side mTLS configurations, and do not apply for the client initiating the handshake.

Since we never actually set client side RootCAs, connecting to Redis with a self-signed CA results in:

"error": "tls: failed to verify certificate: x509: certificate signed by unknown authority",

Fixed by switching Redis TLS config to use RootCAs instead, and updating configuration accordingly.

@github-actions github-actions bot added area/config Related to registry config area/docs area/api labels Jul 5, 2025
@ChandonPierre ChandonPierre force-pushed the cpierre/redis-rootcas branch 2 times, most recently from 77731c4 to ec2e369 Compare July 5, 2025 19:24
`configureRedis` currently sets `RequireAndVerifyClientCert` and `ClientCAs`, however these are server side mTLS configurations, and do not apply for the client initiating the handshake.

Since we never actually set client side `RootCAs`, connecting to Redis with a self-signed CA results in:

```
"error": "tls: failed to verify certificate: x509: certificate signed by unknown authority",
```
Fixed by switching Redis TLS config to use `RootCAs` instead, and updating configuration accordingly.

Signed-off-by: ChandonPierre <cpierre@coreweave.com>
@ChandonPierre ChandonPierre force-pushed the cpierre/redis-rootcas branch from ec2e369 to 02b1f6e Compare July 5, 2025 19:25
Copy link
Copy Markdown
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

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

PTAL @thaJeztah

Signed-off-by: ChandonPierre <cpierre@coreweave.com>
@ChandonPierre
Copy link
Copy Markdown
Contributor Author

Let me know if any changes are needed 👍

@milosgajdos milosgajdos requested a review from thaJeztah August 8, 2025 05:19
@milosgajdos
Copy link
Copy Markdown
Member

@thaJeztah PTAL

Copy link
Copy Markdown
Collaborator

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

ClientCAs []string `yaml:"clientcas,omitempty"`
// RootCAs specifies a list of root certificate authorities that clients use when
// verifying server certificates. If RootCAs is nil, TLS uses the host's root CA set.
RootCAs []string `yaml:"rootcas,omitempty"`
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.

one thing Im wondering here...this will break existing deployments...so we either release this as 3.1 or we'll have to change this field to the original ClientCAs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, it’s a breaking change, but it shouldn’t impact users since the functionality doesn’t work with ClientCAs anyway. We can treat it as a fix to correct the behavior.

@milosgajdos milosgajdos merged commit 979a074 into distribution:main Sep 3, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/api area/config Related to registry config area/docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants