Skip to content

egress: add an example credential provider which reads from k8s secret - #1335

Merged
Eitan Yarmush (EItanya) merged 4 commits into
agent-substrate:mainfrom
yufan-su:cred-provider-plugin
Sep 22, 2026
Merged

Eitan Yarmush (EItanya) merged 4 commits into
agent-substrate:mainfrom
yufan-su:cred-provider-plugin

Conversation

@yufan-su

@yufan-su Yufan Su (yufan-su) commented Aug 31, 2026 •

Copy link
Copy Markdown
Collaborator

Add an example gRPC credential-provider plugin which reads from k8s secrets for the egress
credential-injection path.

  • It resolves ate-secret://k8s.io/default/<namespace>/<secret>/<key>
    URIs to Kubernetes Secret values, so Substrate never stores secrets — it only
    brokers a read that the provider is authorized to perform.
  • It is the only
    component in the injection path with Kubernetes secerts access; the egress gateway and
    its injector never read Secrets directly.

What's included

  • cmd/credential-provider/kubernetes-secrets — the gRPC service:
    • Parses and validates ate-secret:// URIs, resolving the requested Secret
      (with single-key fallback when the URI omits a key).
    • Enforces an atespace→namespace authorization policy (default-deny),
      derived from the caller's attested actor SPIFFE ID.
    • Serves over mutual TLS, requiring the caller's client cert to chain to
      the trust bundle and carry the egress injector's SPIFFE SAN.
  • Manifests (manifests/egress-credential-injection/) — Deployment,
    Service, ServiceAccount + RBAC, a sample namespace-policy ConfigMap, and a
    sample Secret.
  • Renames the default provider address/service from credprovider to
    k8s-credential-provider across ate-setup, install scripts, and the egress
    injection overlay.

atespace → namespace authorization

Beyond the mTLS check that only the egress injector may call the provider, each
request is authorized against a default-deny atespace→namespace policy. The
provider derives the requesting actor's atespace from its attested SPIFFE ID and
resolves a Secret only if that atespace is explicitly granted access to the URI's
namespace.

@EItanya

Eitan Yarmush (EItanya) commented Aug 31, 2026 •

Copy link
Copy Markdown
Collaborator

I think this PR is honestly a bit noisy. There's both the credential provider, and the implementation of yet another extproc server which uses it. Can we instead review the credential provider in isolation, especially given that there are still open questions in the doc, and then we can tackle the thing which calls it?

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

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.

We discussed offline but for posterity, Eitan Yarmush (@EItanya) 's idea to split the pr sounds good.

@yufan-su
Yufan Su (yufan-su) force-pushed the cred-provider-plugin branch 2 times, most recently from 02a01c3 to d2731aa Compare September 1, 2026 03:59
@yufan-su Yufan Su (yufan-su) changed the title egress: add credential injector and credential provider egress: add credential provider which reads from k8s secret Sep 1, 2026
@yufan-su

Yufan Su (yufan-su) commented Sep 1, 2026 •

Copy link
Copy Markdown
Collaborator Author

Split into two PRs #1335 and #1360 and this one only contains the credential provider change.

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.

Comment thread internal/proto/nsauthzpb/nsauthz.proto Resolved
Comment thread internal/proto/nsauthzpb/nsauthz.proto Resolved
Comment thread internal/proto/nsauthzpb/nsauthz.proto Resolved
Comment thread internal/actorspiffe/actorspiffe.go Resolved
Comment thread cmd/credprovider/main.go Resolved
@yufan-su
Yufan Su (yufan-su) force-pushed the cred-provider-plugin branch 7 times, most recently from d4669d5 to 9587727 Compare September 9, 2026 04:04
@yufan-su Yufan Su (yufan-su) changed the title egress: add credential provider which reads from k8s secret egress: add an example credential provider which reads from k8s secret Sep 17, 2026

@LiorLieberman Lior Lieberman (LiorLieberman) left a comment •

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.

One of the comments Tim mentioned is multi cluster. E.g I am storing a secret in a different cluster.

Not a requirement for this PR but the (code) design is important to support that. E.g

"I install the kubernetes.io provider (and have it in as a registration flag), and together with this I either (a) provide a config map that has my cluster-name/dns/control-plane ip to a kubeconfig entry (b) store that mapping somewhere else not in a config map?"

how is the plugin authorized to read secrets? is it a service account only? can we support what I mentioned above to support fetching secrets from a different cluster?

@EItanya

Copy link
Copy Markdown
Collaborator

One of the comments Tim mentioned is multi cluster. E.g I am storing a secret in a different cluster.

Not a requirement for this PR but the (code) design is important to support that. E.g

"I install the kubernetes.io provider (and have it in as a registration flag), and together with this I either (a) provide a config map that has my cluster-name/dns/control-plane ip to a kubeconfig entry (b) store that mapping somewhere else not in a config map?"

how is the plugin authorized to read secrets? is it a service account only? can we support what I mentioned above to support fetching secrets from a different cluster?

Should that be in scope for this initial round of this plugin. I still don't think we've decided what the support level is for this component, if it's official or just an example etc.

Maybe let's start with this as the default local secret impl and then we can expand or let others bring their own multicluster vairant?

@bowei

Bowei Du (bowei) commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

I think we should not get too into the weeds on the multicluster aspect of it right now. We could leave a space in the path for later expansion if we really want to:

We have:

	ate-secret://kubernetes.io/<namespace>/<secret>[/<key>]

Maybe

	ate-secret://kubernetes.io/default/<namespace>/<secret>[/<key>]

Later we can add

	ate-secret://kubernetes.io/clusters/<cluster name>/<namespace>/<secret>[/<key>]

@yufan-su

Yufan Su (yufan-su) commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator Author

Updated the substrate uri format to be

ate-secret://k8s.io/default/<namespace>/<secret>[/<key>] - for local secrets

replaced kubernetes.io with k8s.io to make the uri shorter

@bowei Bowei Du (bowei) added area/security Security related issue/pr area/demos labels Sep 17, 2026
@bowei

Copy link
Copy Markdown
Collaborator

Minor comments -- generally looks good.

Should clarify URI escaping is not supported in the string.

@LiorLieberman

Copy link
Copy Markdown
Collaborator

Updated the substrate uri format to be


ate-secret://k8s.io/default/<namespace>/<secret>[/<key>] - for local secrets

replaced kubernetes.io with k8s.io to make the uri shorter

Wait why not starting with clusters now?

What are we loosing?

/clusters/self

@LiorLieberman

Copy link
Copy Markdown
Collaborator

I think we should not get too into the weeds on the multicluster aspect of it right now. We could leave a space in the path for later expansion if we really want to:

We have:


	ate-secret://kubernetes.io/<namespace>/<secret>[/<key>]

Maybe


	ate-secret://kubernetes.io/default/<namespace>/<secret>[/<key>]

Later we can add


	ate-secret://kubernetes.io/clusters/<cluster name>/<namespace>/<secret>[/<key>]

We should start with clusters/cluster-name now. Adding another element later just complicates that.

Why not just go with clusters/'self' and support only local now or something

(Or self instead or underscore self)

@LiorLieberman

Copy link
Copy Markdown
Collaborator

Errr

_self_

@EItanya Eitan Yarmush (EItanya) left a comment

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.

🤖 AI-generated review.

Three findings below. The four focused test packages passed, and local TLS and request-load probes reproduced the first two findings on 55072e64. I checked the changes through 6fba1c3; the client trust loading, Kubernetes client limits, and image build lists are unchanged. No live Kubernetes deployment was tested.

Comment thread cmd/ate-setup/internal/images/images.go Resolved

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.

LGTM. I really prefer having the same element list in the uri and start with /clusters/_self_ or some other preserved thing but... not going to block on that, so good with whatever you decide

@bowei

Bowei Du (bowei) commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

What are the requested changes that are still pending? Eitan Yarmush (@EItanya) I can't find any pending items from your comments.

@EItanya
Eitan Yarmush (EItanya) added this pull request to the merge queue Sep 22, 2026
Merged via the queue into agent-substrate:main with commit 57234a8 Sep 22, 2026
11 checks passed
Yuan Gao (ygao-g) pushed a commit to ygao-g/substrate that referenced this pull request Sep 22, 2026
…nfigurable (agent-substrate#350)

# Description

**Substrate assumes the canonical install layout, and every deviation
fails closed.** The namespace `ate-system`, the Services `api` /
`atenet-router`, and the ServiceAccounts `atelet` / `atenet-router` are
compiled-in constants. An install in a per-developer namespace, or under
a deployment that prefixes resource names, breaks — and no failure
points at the naming.

This series makes the namespace, the Service names, and the
ServiceAccount names configurable. Every option defaults to the
canonical value. A canonical install is byte-for-byte unaffected.

| Hardcoded assumption | Failure | What it looks like instead |
|---|---|---|
| atelet's namespace in ateapi's SPIFFE check | ateapi rejects every
atelet | an mTLS handshake failure, not a naming error |
| atelet's namespace in the worker's broker check | no actor obtains a
certificate | `credential broker is not atelet` |
| NetworkPolicy's ingress namespace | the CNI drops every request to the
pool | a silent network fault |
| ateapi Service name and namespace in the client | the client cannot
authenticate | `services "api" not found`, then `invalid bearer token` |
| Resource names in `hack/install-ate.sh` and the e2e harness |
authorities land in the wrong namespace | suites die in preflight |

**A SPIFFE ID breaks on two axes.** It names a namespace and a
ServiceAccount. Both were constants, and the failure surfaces as a
rejected peer, not a missing object.

## Design notes

- **ate-controller passes the worker-side identities.** ateom already
took `--atunnel-client-identity` as a flag but relied on its default. A
new `--atunnel-broker-identity` flag alone would be inert: correct only
where the constant was already correct. The controller knows the control
plane's namespace, so it supplies both.
- **ateapi takes the whole expected identity, not a namespace.** The
dialer and `ateletauth` used the namespace only to build one string. One
place decides how atelet's identity is spelled.
- **`ateletauth` deduplicates without taking the dependency it was
avoiding.** Its constants are duplicated rather than imported so the
package does not depend on `controlapi` for three strings. `ateletdial`
would make a third copy, so the strings move to
`internal/installdefaults` instead — a leaf package of constants that
imports only the standard library, so consuming it does not reintroduce
the coupling the duplication was there to prevent.
- **The client reads environment variables, not flags.** It runs outside
the cluster. It has no downward API and nothing to discover from.
- **Namespaces come from the downward API, not flags.** Every supported
topology co-locates these components. Only Service and ServiceAccount
names, which a deployment may legitimately rename, get flags.
- **`hack/install-ate.sh` and the e2e harness are in scope.** A
relocated install cannot be bootstrapped or exercised without them. The
script refuses `ATE_NAMESPACE` or `ATE_API_SERVICE_NAME` overrides on
the manifest-applying subcommands, which would half-install:
`manifests/ate-install/` names `ate-system` and `api` literally.

## Why this is one PR

The series is stacked, not parallel. Commit 1 creates
`internal/installdefaults` and seeds seven imports; commit 2 adds
fourteen more; every later commit builds on those constants. Split into
separate PRs, each blocks on the previous merging and none reviews
independently.

The guard test cannot land first. Applied to `main` it fails on eleven
hardcoded literals across `ateletauth`, `controlapi/informer.go`,
`networkpolicy_controller.go`, both `ateom` mains, `ateclient`,
`ateletdial` and three e2e files. It is green only because the preceding
commits removed them.

A partial series still fails closed. Each commit's audit turned up more
places assuming `ate-system`, so landing commits 1-3 without 4-5 leaves
a relocated install broken later and less legibly than before.

`CONTRIBUTING.md` covers this case: when the intermediate steps are not
useful on their own, keep the change as one PR split into commits at
logical break points. Review commit by commit, and preserve the commits
on merge. If you would still rather split, the one defensible cut is by
axis — relocation (commits 1-3) then renaming (commits 4-5) with the
guard test rebased on top.

## Rebase notes (2026-09-15)

The series is rebased onto `main` at `d0d85c39`, ninety-two commits on
from the base the PR first carried:

- `main` moved actor JWT and certificate minting out of
`cmd/ateapi/internal/actoridentity` into `controlapi`, deleting the
package and dropping its inline atelet authorization check in favor of
the unified authorizer. This series previously threaded the expected
atelet identity into that check; the check no longer exists, so that
adaptation is gone. `ateletauth` survives with one caller,
`workerservice.SetWorkerCapacity`, which still takes the configured
identity.
- The same refactor replaced `BrokerConfig.ExpectedActorUID` with an
`ActorAtespace` / `ActorName` / `ActorUID` triple. `AteletSPIFFEID` is
additive and still required by `ateletdial.TLSConfig`.
- `ateletauth` (ateapi's side) and `ateletdial` (the worker's side) each
re-declared the hardcoded SPIFFE ID. Both take the expected identity as
a parameter.
- `main` deleted the atenet DNS subsystem. The series no longer touches
it, and `installdefaults` carries no DNS Service name.
- The controller passes `--atunnel-broker-identity` only when it differs
from the canonical default. An ateom old enough to predate the flag
exits on it, and `docs/upgrade.md` keeps such a pool serving during a
rolling upgrade. A relocated install gets the flag and necessarily runs
an image that accepts it.
- `main` added `internal/e2e/collector_metrics.go` (agentgateway CI
support), which addresses the router through the package-level
`routerNamespace` and `routerService` that this series replaces. Its two
call sites become `SystemNamespace()` and
`ResourceName("atenet-router")`, matching `router_client.go` and
`statusz.go`. A reviewer diffing against the older base sees those call
sites move; nothing else in that file changes.
- `main` added `cmd/credential-provider/kubernetes-secrets` (`agent-substrate#1335`),
whose `injectorSPIFFEID` constant is the mTLS peer check on the only
caller permitted to read Secrets. The comparison is an exact string
match, so a renamed install rejects every fetch at TLS. It becomes
`--injector-identity`, defaulting to
`installdefaults.EgressSPIFFEID(SystemNamespace)`, alongside a new
`EgressServiceAccount` constant and helper. The flag name follows
`--atunnel-client-identity`; the manifest beside it still names
`ate-system`, so a renaming deployment configures both.
- `main` replaced the dialer's worker indexer with
`DialForAteletOnNode`, dropped `WorkerPodInformer` from `controlapi`,
and removed `kataConfig` from ateom-microvm's `NewService`. The series
adapts to each narrower signature and keeps only its own added
parameter.
- The guard test's allowlist entry for the two inert `ate-system`
literals follows the code from `actoridentity.go` to
`controlapi/actor.go`. The refactor re-introduced exactly the class of
constant this series removes, so the guard earns its place.

# Testing

- `go test -race ./...` passes. `make verify` passes boilerplate,
codegen, go-modules, gofmt, golangci-lint, kube-api-linter, licenses,
metrics and postgresql-migrations. `proto-fmt` needs `clang-format`,
which is absent locally; this series touches no `.proto` files.
- Every commit builds and vets individually, via `git rebase -x 'go
build ./... && go vet ./...'`.
- On a fresh kind cluster, all twelve e2e suites pass: `capabilities`,
`combinedvolumes`, `demo`, `egressauthz`, `egressmitm`, `example`,
`identity`, `metrics`, `networking`, `networkpolicy`, `parking`,
`sizing`. The `demo`, `metrics`, `networkpolicy`, `parking` and
`networking` suites need the `--deploy-demo-counter` and
`--deploy-demo-egress` fixtures, and the MITM path needs
`--deploy-demo-egress-mitm`; without them they fail on `actor template
not found`, which reads as a control-plane fault rather than a missing
fixture.
- With the sdsmint egress gateway (`--deploy-atenet
--experimental-use-sdsmint`, `E2E_EGRESS_MITM=1`),
`TestActorEgressMITMTrust` and `TestActorEgressHTTPSByHostnameMITM` both
pass, and the `networking` suite is green at 25 passed. The two modes
are mutually exclusive by design:
`TestActorEgressHTTPSByHostnamePassthrough` covers the plain gateway and
skips under MITM, and the two MITM tests skip without it. Both modes
were run, so every case executed in one of them.
- The new unit tests use a relocated namespace and renamed
ServiceAccounts. Reintroducing each hardcoding makes them fail.

One e2e caveat that predates this series and misleads: the `identity`
suite calls `ReplaceEgressTrustPool`, which takes over the shared
`egress-mitm-ca-pool` Secret, overwrites it and registers no cleanup.
Any MITM test running afterwards fails with `certificate signed by
unknown authority`, which reads as a broken interception path rather
than a mutated fixture. Reinstall the gateway before running
`egressmitm`.

# Additional Notes

**Credential contents stay canonical.** `controlapi/actor.go` still
mints credentials naming `api.ate-system.svc`: the JWT issuer and the
certificate's Issuer CN. Relying parties validate these, so changing
them is a compatibility decision, not a lookup fix. The code carries a
TODO to make the issuer a globally unique, OIDC-compliant name.
@zlammerts-svg Zheng Lammerts (zlammerts-svg) added this to the M3 milestone Sep 23, 2026
pull Bot pushed a commit to Future-Outlier/substrate that referenced this pull request Sep 23, 2026
)

Follow-up change on a comment in agent-substrate#1335 to add a statusz page in the
credential provider service.

- [x] Tests pass
- [ ] Appropriate changes to documentation are included in the PR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/demos area/network area/security Security related issue/pr kind/feature An enhancement / feature request or implementation

Development

Successfully merging this pull request may close these issues.

5 participants