Added mTLS between ate system components#237
Conversation
df3d9a0 to
dfb1db1
Compare
|
Can you talk a little bit about the background of splitting out separate CAs / signers for the workerpools and ate-system components? If at all possible, I would prefer to restrict us to two signers:
The reason is that we can be reasonably confident that signers like these will land in upstream K8s in the near-ish future. The certificates here aren't special or unique to substrate, and they aren't part of the value proposition of the project. So I think we should plan to rebase on native K8s capabilities as they become available. That's easiest if we don't get very custom with the signers. |
|
I think the main motivations for splitting out separate CAs / signers was, if I mount the "podidentity" CA bundle as trusted client CAs in ate-system services, workerpool pods (therefore actors) would also be able to connect to ate-system servers like the Valkey cluster and atelet server. But after typing it out loud - it seems this needs to be solved anyway (probably in a later milestone) by authorization inside servers such as valkey, instead of using separate CAs. Does that make sense to you? |
Yeah --- we should always draw a bright line between authentication and authorization. Assuming that a given client is authorized just because they were able to get a certificate from a particular CA has caused a lot of security problems in the past. Unfortunately, for the specific case of Valkey we might be limited by its capabilities, in which case it might require a separate signer. Let me check the docs. |
|
It looks like valkey can do it: https://valkey.io/topics/tls/#:~:text=Client%20certificate%20authentication |
683d085 to
2e34e49
Compare
Bowei Du (bowei)
left a comment
Bowei Du (bowei)
left a comment
There was a problem hiding this comment.
Router changes look ok.
|
needs rebase |
|
Ping on this? |
Rebased onto main and integrated with the ateapi-auth (mtls|jwt) mode that landed on main in agent-substrate#248. Router (client): - ateapi-auth=mtls now performs real mutual TLS: the router presents its podidentity credential bundle (--ateapi-client-cert) as a client cert and verifies ateapi's serving cert against the servicedns trust bundle (--ateapi-ca-certs), instead of the previous insecure TLS dial. No insecure fallback: missing material is a hard error. - ateapi-auth=jwt (from agent-substrate#248) is unchanged and still routes through ateapiauth.DialOptions; otel tracing/metrics are preserved. ateapi (server): - The ateapiauth server interceptor now injects an authenticated principal (internal/principal) into the request context in both modes: the mtls authenticator extracts the client's SPIFFE identity from its mTLS peer cert, and the jwt authenticator records the verified token subject (VerifyBearerToken now returns the principal ID). The principal carries a Kind (mtls|jwt) and is included in RPC request logs. Supporting changes: - Move credbundle to internal/credbundle and add ClientLoader for client certs. - podcertcontroller signers, valkey, and install-ate.sh updated to provision and mount the podidentity/servicedns bundles; manifests mount them into the router and apiserver pods.
The --ateapi-client-cert path is provided via the manifest instead of a code default, matching how the other cert-material flags are wired.
Drop --ateapi-ca-certs: it duplicated --ateapi-ca-file (both are the CA bundle used to verify ateapi's serving cert, split by auth mode). The router now dials ateapi via ateapiauth.DialOptions in both modes, like atecontroller, instead of hand-rolling the mtls TLS config. This also gains TLS 1.2 minimum and --ateapi-server-name support in mtls mode. Path defaults move to the manifest (--ateapi-ca-file previously defaulted to the SA ca.crt, which never matches ateapi's servicedns- signed serving cert), and the router's servicedns-ca trust bundle now mounts at /run/servicedns-ca, matching ate-controller and deduping the jwt overlay's second mount of the same bundle.
Port the file-layout refactors from PR agent-substrate#237 that were lost when the mtls branch was redone on a newer main: - Move NewRouterCmd from cmd/atenet/internal into the router package as cmd.go; root.go now calls router.NewRouterCmd(). - Move the config struct to config.go, unexport it (routerConfig), and group the ateapi client-auth fields under a nested authConfig. - Drop the self-signed Envoy cert fallback: --envoy-cert-path is now required for HTTPS serving; SetTlsConfig takes just (port, path) and xDS always references the cert by filename.
Zoe Zhao (zoez7)
changed the title
|
Sorry for the delay! Rebased and force-pushed this branch, also added the mTLS between other ate system components as well. |
| GetClientCertificate: credbundle.ClientLoader(clientBundlePath), | ||
| // Skip the default verification because the peer is dialed by IP and its | ||
| // certificate has no DNS/IP SAN. | ||
| InsecureSkipVerify: true, |
There was a problem hiding this comment.
Should we include DNS or IP SANs in the certs so we don't need to skip this? https://spiffe.io/docs/latest/spiffe-specs/x509-svid/#2-spiffe-id says:
An X.509 SVID MAY contain any number of other SAN field types, including DNS SANs.
There was a problem hiding this comment.
The problem is that kubelet first waits for the pod certificate volume mount https://github.com/kubernetes/kubernetes/blob/v1.34.0/pkg/kubelet/kubelet.go#L2066 before creating a pod, if we try to add IPAddress in cmd/podcertcontroller/internal/podidentitysigner/podidentitysigner.go, it will be empty.
| const CTBPrefix = "podidentity.podcert.ate.dev:identity:" | ||
|
|
||
| // oidPodUID represents the Pod UID within a certificate extension. | ||
| var oidPodUID = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11129, 2, 6, 1, 3} |
There was a problem hiding this comment.
How did we determine the object identifier? Is this a standard set of numbers?
There was a problem hiding this comment.
Yes, this is a standard set up numbers I registerered in cl/949739997. The implementation now mirrors the future upstream pod identity signer https://github.com/kubernetes/kubernetes/pull/127251/changes#diff-985739ccf0a8431134e42720fd9a285e18e9fe49a5586062ff405d205e648e06. All changes related to the extension are in 0003f76.
| ExtraExtensions: []pkix.Extension{ | ||
| { | ||
| Id: oidPodUID, | ||
| Critical: false, |
There was a problem hiding this comment.
If we always reject certs that don't have this extension, should we also mark it as critical so that any systems that don't recognize it also reject it, or is that going too far (e.g. would that break identity federation)?
There was a problem hiding this comment.
Reading https://datatracker.ietf.org/doc/html/rfc5280#section-4.2 I kept it as non-critical in case the cert will be used to authenticate to other system that is not the ate-apiserver.
| // bundle on each handshake so that in-place pod-certificate rotations are | ||
| // picked up. | ||
| func ClientLoader(path string) func(*tls.CertificateRequestInfo) (*tls.Certificate, error) { | ||
| // TODO: Introduce caching. |
There was a problem hiding this comment.
Would it be possible to fix this in this PR?
There was a problem hiding this comment.
I'll create an open issue for this, there are a couple places in internal/credbundle/credbundle.go that we need to do this for and I didn't want to make this PR even bigger.
| workerpoolCACerts = pflag.String("workerpool-ca-certs", "", "The file that contains the CA for verifying workerpool client certificates.") | ||
|
|
||
| ateletClientCredBundle = pflag.String("atelet-client-cred-bundle", "/run/podidentity.podcert.ate.dev/credential-bundle.pem", "Credential bundle presented as the client certificate when dialing atelet.") | ||
| ateletCACerts = pflag.String("atelet-ca-certs", "/run/podidentity.podcert.ate.dev/trust-bundle.pem", "CA bundle used to verify atelet serving certificates.") |
There was a problem hiding this comment.
I see worker pool certs above, does that also currently cover other system components like the router, controllers, etc that talk to ateapi?
There was a problem hiding this comment.
Yes. It does. Once we add authorization to the ate api, the workerpool pods should not be able to talk to the ateapi.
There was a problem hiding this comment.
Should we name it something more generic, then, to indicate the CA covers more than just the worker pool? Maybe just --ate-trust-bundle?
| // to ctx once ateapi has an authorization layer. | ||
| return ctx, nil | ||
| pInfo := principal.PrincipalInfo{ | ||
| ID: "anonymous", |
There was a problem hiding this comment.
Should we actually bother to support "anonymous" or should we just flat out reject anything that lacks credentials? We had a lot of issues with anonymous auth in K8s and have done our best to disable it wherever possible at this point. Personally I'd lean towards always requiring authn.
There was a problem hiding this comment.
Done in 620a215. Now we use a chained authenticator, if a client does not present either bear token or peer cert, we fail the authentication.
… use a chained authenticator
… future upstream impl
4 participants
Part of #170
Establishes mutual TLS between all ate system components, and updates the certificate plumbing it depends on.
Main changes:
InsecureSkipVerify.Minor bug fixes:
servicednssignerfrom signing a cert with no DNS SANs. Also updated valkey cluster's cert configuration, because it was relying on the cert with empty DNS.