Repository navigation
atelet: only ate-api-server may call AteomHerder - #2516
Conversation
632e492 to
837130a
Compare
| GetCertificate: serverCert, | ||
| ClientAuth: tls.RequireAndVerifyClientCert, | ||
| ClientCAs: clientCAs, | ||
| VerifyConnection: verifyClient, |
There was a problem hiding this comment.
I think in the long run it's probably better to verify the certificate per-request --- atelet will probably eventually need a richer authorization policy than "only ate-api-server can call any RPC", and the error message for clients is much better if the cert is checked during request processing, rather than connection establishment.
This is fine for now, though.
There was a problem hiding this comment.
Ack.
| return nil, fmt.Errorf("load CA bundle %s: %w", clientCAPath, err) | ||
| } | ||
| serverCert := credbundle.Loader(servingBundlePath) | ||
| return &tls.Config{ |
There was a problem hiding this comment.
There was a problem hiding this comment.
🤖 Thanks. Whichever lands second can carry the callback through: credbundle.ServerConfig would need a VerifyConnection field set on the config that GetConfigForClient returns. I'm happy to rebase onto #2237 if it lands first.
| tlsCfg, err := ateletServerTLSConfig(*grpcServerCredBundle, *clientCACerts) | ||
| // The flag defaults to installdefaults.APIServerServiceAccount | ||
| // ("ate-api-server"), so this only rejects an explicit empty override: | ||
| // path.Join would drop the empty segment, yielding an identity that parses |
There was a problem hiding this comment.
I feel like we're heavy on the agent comments here. Can you slim them down?
There was a problem hiding this comment.
🤖 Trimmed the comments in main.go, atelet.yaml, and installdefaults.
|
|
||
| grpcServerCredBundle = pflag.String("grpc-server-cred-bundle", "/run/podidentity.podcert.ate.dev/credential-bundle.pem", "Credential bundle atelet presents as its gRPC serving certificate.") | ||
| clientCACerts = pflag.String("client-ca-certs", "/run/podidentity.podcert.ate.dev/trust-bundle.pem", "CA bundle used to verify gRPC client certificates.") | ||
| ateapiServiceAccount = pflag.String("ateapi-service-account", installdefaults.APIServerServiceAccount, "ServiceAccount ate-api-server runs as, in atelet's namespace. It is the service-account segment of the only SPIFFE ID allowed to call AteomHerder, so a deployment that prefixes resource names needs it set.") |
There was a problem hiding this comment.
I feel like we should just take the whole SPIFFE ID that's allowed to call? That way ate-setup can handle all the stuff with figuring out namespaces and so on.
There was a problem hiding this comment.
🤖 Done. Replaced --ateapi-service-account with --herder-allowed-spiffe-id. It defaults to spiffe://cluster.local/ns//sa/ate-api-server, so ate-setup can pass the full identity.
be3df77 to
c5c9417
Compare
|
FYI The test failure is due to a bad test that is hitting "example.com" #2513 |
c5c9417 to
81e3f74
Compare
|
Rebased to fix merge conflicts and pick up the CI testing fix. Should be OK to merge directly now. |
092d6d3 to
7d73d28
Compare
| - configMapRef: | ||
| name: ate-otel-config | ||
| ports: | ||
| # No hostPort: ate-api-server dials atelet's Pod IP. |
There was a problem hiding this comment.
lets remove this line, feels redundant
There was a problem hiding this comment.
removed
| func APIServerSPIFFEID(namespace string) string { | ||
| return SPIFFEID(namespace, APIServerServiceAccount) | ||
| } |
There was a problem hiding this comment.
why do we need a special helper for this, vs just use SPIFFEID(namespace, APIServerServiceAccount)?
There was a problem hiding this comment.
I see other function do the same... feels like a lot of redundant code.
There was a problem hiding this comment.
I agree. I asked gemini exactly the same thing and it told me it was following the convention set by all the other functions, so I left it in 🤷 . I'll take it out.
| func TestAPIServerSPIFFEID(t *testing.T) { | ||
| // Matches the --herder-allowed-spiffe-id default atelet ships with. | ||
| const want = "spiffe://cluster.local/ns/ate-system/sa/ate-api-server" | ||
| if got := APIServerSPIFFEID(SystemNamespace); got != want { | ||
| t.Errorf("APIServerSPIFFEID(%q) = %q, want %q", SystemNamespace, got, want) | ||
| } | ||
| } |
There was a problem hiding this comment.
similar comment to above, test and function feels very redundant.
There was a problem hiding this comment.
Removed along with the helper.
Benjamin Elder (BenTheElder)
added
area/security
7d73d28 to
3ddbf61
Compare
|
rebased and addressed comments |
The AteomHerder listener required a client certificate chaining to the pod-identity CA, but any pod in the cluster can obtain one, and the caller's identity was never checked. Any pod reaching atelet could drive actor lifecycles (Run/Checkpoint/Restore/Terminate) on the node.
--herder-allowed-spiffe-id(defaultspiffe://cluster.local/ns/<atelet ns>/sa/ate-api-server) during the TLS handshake.Fixes #2073
Breaking change
Only ate-api-server can call AteomHerder now. Anything else that called atelet on
:8085(including via<node-ip>:8085) is rejected at the TLS handshake. Installs that rename the ate-api-server ServiceAccount or run it in another namespace must pass--herder-allowed-spiffe-idto atelet.