feat(provider): support for SPIFFE backed token exchange - #1970
Conversation
697b181 to
0378b29
Compare
|
@mrunalp @TaylorMutch just fyi, I have verified that this works with both currently supported supervisor topologies. |
|
Context from the Okta side: I have a live-tested RFC 8693 OBO implementation in #1681. That path exchanges the logged-in user subject token at Okta using the Okta client ID/secret, audience, and scope. Before I rework it around this PR: is #1970 intended to support that kind of Okta token endpoint/client-secret exchange, or does the new two-stage flow require the IdP to accept SPIFFE JWT-SVID client assertions? If the latter, I will need to validate/configure that separately before wiring OBO into the NemoClaw reference. |
|
@afourniernv At present it does indeed require the IdP to accept SPIFFE JWT-SVIDs. That could certainly be expanded, but I do think that the intention for this path (i.e. the current dynamic token grant path) is for it to use some 'platform provided' credential that is (or can be) scoped to the sandbox. If the user is providing a client secret for the exchange through a provider, then in my view the approach in your PR seems like the simplest/cleanest. In that case the gateway can handle it in much the same way as a token refresh (as your PR shows). |
11f2a20 to
85ebd89
Compare
|
/ok |
e154f4d to
2123487
Compare
| /// | ||
| /// When set, the gateway can fetch its own JWT-SVID for provider token exchange | ||
| /// client assertions. | ||
| pub const GATEWAY_SPIFFE_WORKLOAD_API_SOCKET: &str = "OPENSHELL_GATEWAY_SPIFFE_WORKLOAD_API_SOCKET"; |
There was a problem hiding this comment.
Nit: this module's doc comment describes it as "the shared protocol between the compute drivers (which set the variables when launching a sandbox container/VM) and the sandbox supervisor process (which reads them on startup)."
GATEWAY_SPIFFE_WORKLOAD_API_SOCKET doesn't fit that contract. No compute driver sets it, and no supervisor reads it. It's set by the Helm template on the gateway pod and read by the gateway's gRPC handler (fetch_gateway_jwt_svid). Placing it here blurs the module's scope and could confuse someone auditing which env vars drivers must provide.
Consider moving it to the server crate (e.g., a gateway_env module or alongside the handler that reads it).
There was a problem hiding this comment.
Thank you! I agree and have moved that definition to a private server-local constant in crates/openshell-server/src/grpc/provider.rs where it is used.
rhuss
left a comment
There was a problem hiding this comment.
Nice work on this, Gordon. The two-stage exchange design is solid, and the security model (subject token never leaves gateway, audience-scoped intermediates, full SVID verification via SPIRE JWT bundles) is well thought through.
A few observations beyond the inline comments:
Consider splitting out the demo. The 59-file diff is a lot to review in one pass. The examples/spiffe-token-exchange-demo/ directory (README, demo.sh, K8s manifests, token-issuer, protected-service) is self-contained and could ship as a follow-up PR without blocking the core feature. That would trim roughly 8 files and make the core diff less intimidating for reviewers.
Extract a shared token exchange helper. perform_token_exchange and perform_intermediate_token_exchange share ~80% structure: URL parsing, client assertion type resolution, form POST assembly, error handling, response parsing, and access token validation. The differences are just which form parameters get added. A common helper that takes a pluggable form-params builder would reduce the duplication and ensure future fixes in one path don't miss the other.
Gateway env var placement. I left an inline comment earlier about GATEWAY_SPIFFE_WORKLOAD_API_SOCKET not fitting the sandbox_env.rs module contract (see thread on that file).
| // Supervisor JWT-SVID. The gateway verifies this and uses its `sub` claim | ||
| // as the requested audience for the intermediate token. | ||
| string supervisor_jwt_svid = 4; | ||
| } |
There was a problem hiding this comment.
supervisor_jwt_svid carries a JWT-SVID credential but is missing the [(openshell.options.v1.secret) = true] annotation. Every other credential-bearing field in this proto uses it (e.g., IssueSandboxTokenResponse.token at line 288, TcpForwardInit.authorization_token at line 819). Without it, secret-scrubbing interceptors won't redact this value in debug logs.
There was a problem hiding this comment.
Fixed (I merged this into the original commit)
|
|
||
| message ExchangeProviderSubjectTokenResponse { | ||
| string access_token = 1; | ||
| int64 expires_in = 2; |
There was a problem hiding this comment.
Same issue: access_token should have [(openshell.options.v1.secret) = true] for consistency with every other token field in the proto.
There was a problem hiding this comment.
Fixed (I merged this into the original commit as above)
| pub async fn obtain_provider_token(request: ObtainProviderTokenRequest<'_>) -> Result<String> { | ||
| let grant_type = ProviderCredentialTokenGrantType::try_from(request.grant_type) | ||
| .unwrap_or(ProviderCredentialTokenGrantType::ClientCredentials); | ||
| obtain_provider_token_with_grant( |
There was a problem hiding this comment.
If a newer gateway sends an unrecognized grant_type enum variant, this silently falls back to ClientCredentials instead of reporting the misconfiguration. This could mask a broken token exchange setup where the profile says token_exchange but the proto value doesn't round-trip correctly.
Consider logging a warning on the fallback, or returning an error for unknown values (keeping the Unspecified -> ClientCredentials mapping as-is).
There was a problem hiding this comment.
Fixed (also merged this into the original commit)
Additional observations (lower severity)A few more things I noticed while reviewing. None of these are blocking, but worth considering: Possible double-skew on intermediate token cache expiry
No unit test for Cache eviction is non-deterministic SELinux label
Helm values.yaml comment scope |
|
@TaylorMutch @mrunalp @johntmyers, this PR could use your eyes. I went through the implementation in detail (agent-assisted) and I think it's ready for maintainer review. The design is non-intrusive by construction. Token exchange is a new I reviewed the driver integration (agent-assisted) and the abstraction boundaries are clean. The supervisor logic is fully driver-agnostic. It reads an env var and doesn't know whether it's running under Kubernetes, Podman, or anything else. Each driver opts in independently (K8s via CSI volume, Podman via bind mount). Docker and VM drivers simply don't set the env var and everything works as before. (but nothing prevents those drivers to add support for SPIFFE later, too). The Podman support is a real bonus for developer workflow. You can run SPIRE locally, test the full token exchange on your laptop, then deploy the same provider profiles to a K8s cluster with proper per-pod attestation. The e2e test validates this path end-to-end. Strategically, this is the foundation for agent delegation chains, proving both the user's identity and which specific sandbox is acting on their behalf. That capability becomes essential once multi-agent scenarios need real identity propagation. The PR is 59 files but I think the demo ( would be great to come to a decission whether we could add this functionality to the beta roadmap as well. Our customers are asking actively for this feature, so there is a real-world value backing for this, too. |
| validate_access_token(&intermediate.access_token)?; | ||
| let intermediate_subject_token_type = | ||
| final_exchange_subject_token_type(request.requested_token_type); | ||
| perform_token_exchange( |
There was a problem hiding this comment.
Question: intermediate_subject_token_type is derived from request.requested_token_type (the output type the caller wants), but the intermediate token from Stage 1 is always an access token (that's what the gateway returns). In the Stage 2 exchange, the subject_token_type should describe what the intermediate token is, not what the final output should be.
If requested_token_type differs from the default access token type, this would send an incorrect subject_token_type in the RFC 8693 form POST. Should this use DEFAULT_TOKEN_TYPE (or the token_type from the gateway's ExchangeProviderSubjectTokenResponse) instead?
The final_exchange_subject_token_type wrapper uses effective_token_type which falls back to DEFAULT_TOKEN_TYPE when empty, so in practice it may work today if requested_token_type is always empty or matches. But the semantic coupling seems fragile.
There was a problem hiding this comment.
^^ is a coderabbit finding, @grs I hope it makes sense to you (but could be a false alarm, too). I found it interesting enough to share this.
There was a problem hiding this comment.
Thanks @rhuss! I agree and have made this fix along with the oauth refactor, with a renamed constant to make it clearer.
…nt credentials as OAuth grant_type Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
Signed-off-by: Gordon Sim <gsim@redhat.com>
…ervisor Signed-off-by: Gordon Sim <gsim@redhat.com>
…cache Signed-off-by: Gordon Sim <gsim@redhat.com>
|
Thank you very much for the great review @rhuss!
I can certainly do that, but at present I kept it as a separate commit on the branch as it is a useful smoke test for anyone trying things out. If you review commit-by-commit you can skip that one.
Agreed. I have made this change as an additional commit and included the original token grant in the refactor as well, as there is a reasonable amount of duplication there also.
Fixed earlier. |
The allowance is applied to the tokens own expiry first, then to the expiry of other tokens (the subject token and the jwt-svid), with the minimum taken between these. So I believe it is correct as is at present, but challenge me further if you disagree or I am misunderstanding your point.
Agreed, I have done this in the new oauth refactor commit.
This is also covered in the oauth refactor.
I have added a new commit for this that evicts the closest to expiry which is more intuitive.
Agreed, I have removed the label and merged that fix into the podman support commit.
Fixed (merged into the first commit)
I think the comment is correct. That value is used to create the driver config. So from the users perspective its one switch to enable spiffe on an install, that will then affect the gateway and the driver config (and through that the supervisors). Thanks again for all the great feedback @rhuss ! |
Summary
This PR adds the ability to specify token_exchange instead of a simple token_grant in a provider profile, with the goal of automatically obtaining a token that combines the agents identity with that of the user on whose behalf it is acting.
As with the existing token grants, the token exchange is initiated by the supervisor proxy. In the case of an exchange however it requires a subject token that has the supervisor clients id in the audience. To get this, a two stage exchange is required.
The supervisor first requests a token from the gateway. The gateway does the first exchange, using the identified credential in a provider as the subject and requesting the audience needed for the supervisor (which is based on the supervisors SPIFFE ID). The token from that exchange is then returned to the supervisor which uses it as the subject for a further exchange. The resulting token than has the subject of the original provider credential but the sandbox (as identified by its SPIFFE ID) as the authorized party.
An option was also added to the CLI for provider create|update, allowing the current OIDC token to be stored.
Related Issue
Fixes #1987
Changes
Testing
mise run pre-commitpassesChecklist