egress: add credential provider which reads from k8s secret - #1335
egress: add credential provider which reads from k8s secret#1335yufan-su wants to merge 2 commits into
Conversation
|
I think this PR is honestly a bit noisy. There's both the credential provider, and the implementation of yet another Doc since it's not linked anywhere: https://docs.google.com/document/d/1-zQKvu-oYJlva_kwL3xbAOaFoJJaqO0qEqmqRlt1r0c/edit?resourcekey=0-TDrWohfRuYolcXYO-QPVTQ&tab=t.0#heading=h.rx0okwgtp5yu |
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
We discussed offline but for posterity, Eitan Yarmush (@EItanya) 's idea to split the pr sounds good.
02a01c3 to
d2731aa
Compare
Lior Lieberman (LiorLieberman)
left a comment
There was a problem hiding this comment.
Thanks yufan-su !
| namespace: ate-system | ||
| --- | ||
| apiVersion: apps/v1 | ||
| kind: Deployment |
There was a problem hiding this comment.
Creating YET another deployment for this feels like it spreads our deployment footprint out quite a bit. What do you think about running this as a sidecar for the egress policy, or from the control-plane somehow? I'm worried about:
- Latency
- deployment footprint
There was a problem hiding this comment.
I think we need to see if we consider this more of an example vs something that a lot of users will use.
Key things that would make some of the decisions:
- Would this be enabled by default (with opt-out)
- How we document the positioning of this (it's for demos vs it's for prod)
There was a problem hiding this comment.
I think that's a fair point, what do we consider it? How would a user actually deploy secrets and providers to their env?
| // The attested actor identity on whose behalf the secret is fetched. Today | ||
| // this is the actor's SPIFFE URI as verified by the egress gateway; a | ||
| // verifiable Actor JWT is the intended future form. | ||
| string actor_identity = 1; |
There was a problem hiding this comment.
Is this secure? Wouldn't we need the actual actor identity document?
| # atespace "team-a" may resolve secrets in namespace "ns1" (matches the | ||
| # sample policy's substrate-secret://kubernetes.io/team-secrets/ns1/example-api). | ||
| policy { | ||
| atespace: "team-a" | ||
| allowed_namespaces: "ns1" | ||
| } |
There was a problem hiding this comment.
Has this API been discussed at all? This seems like an important piece we need to come to agreement on which was not in the doc Bowei Du (@bowei) Lior Lieberman (@LiorLieberman)
There was a problem hiding this comment.
It's somewhat separate than the doc as this is specific to cred provider implementation.
I was thinking for this example, we do something very simple like a static YAML map of atespace(s) to k8s namespaces.
Nit: probably shouldn't use textproto, this is not a very popular format (outside of Google)
There was a problem hiding this comment.
Ok, so this who PR is really meant to be an example of what one of these plugins could be? I think overall it's not clear to me how various plugins will work, what their lifecycle will be, how they will be accessed by the PEP, etc
|
|
||
| // TrustDomain is the SPIFFE host an actor identity URI carries, matching the URI | ||
| // minted in cmd/ateapi/internal/actoridentity. | ||
| const TrustDomain = "substrate-actor.local" |
There was a problem hiding this comment.
This relates to my comment about the actual actor credential, I think this is a code smell from that
| metricsAddr = pflag.String("metrics-address", ":9090", "Prometheus/health HTTP listen address") | ||
| serverBundle = pflag.String("server-cred-bundle", "", "credential bundle (PEM key+chain) presented for serving TLS; empty serves plaintext (dev only)") | ||
| clientCAFile = pflag.String("client-ca-file", "", "CA bundle that caller (injector) client certificates must chain to; empty accepts any client when TLS is on") | ||
| defaultKey = pflag.String("default-secret-key", "", "Secret data key used when a credential URI omits one; empty requires a single-key Secret") |
There was a problem hiding this comment.
I don't think this is a good idea
775787a to
6e30f5a
Compare
6e30f5a to
150c6b2
Compare
First of a two-PR stack for atenet egress credential injection change. This PR adds the credential provider; the follow-up adds the egress injector that calls it.
What this adds
A
credprovidergRPC service implementing theCredentialProvider.RequestSecretplugin API (pkg/proto/credproviderpb), backed by Kubernetes Secrets. It resolvessubstrate-secret://kubernetes.io/<provider>/<namespace>/<secret>[/<key>]URIs to a Secret value — brokering a read it's authorized to perform rather than persisting the secret itself.Authorization
Included
cmd/credprovider(service + kubeprovider backend)pkg/proto/credproviderpb(plugin API),internal/proto/nsauthzpb(authz mapping),internal/actorspiffe(SPIFFE parser)namespace-policy/sample-secretfixtures