Skip to content

Commit 8446a90

Browse files
committed
feat(provider): selective credential passthrough
Add an opt-in passthrough mechanism for provider credentials that bypass the canonical openshell:resolve:env:* placeholder and L7 proxy substitution. Provider gains a repeated string passthrough_credentials field listing credential keys whose real value should be injected directly into the agent process environment. Required for SDKs that validate credential format in-process (Slack xoxb-*/xapp-*), credentials consumed by in-process crypto (HMAC signing secrets, signature verification), and transports the proxy cannot rewrite (WebSocket payloads after HTTP 101 upgrade). Passthrough drops the "agent never sees the real secret" invariant for the listed keys; documented as a per-credential security trade-off. Server: validation rejects keys not in credentials, invalid env var names, duplicates, and over-cap. resolve_provider_environment returns a ProviderEnvironment struct with the merged env map and the union of passthrough keys; passthrough flag follows the winning value on duplicate keys across providers. Update merge auto-prunes orphaned passthrough entries when their credential is deleted in the same call (proto3 cannot distinguish "field unset" from "set to empty list"), preserving the "recover via update" path. Explicit non-empty incoming list still rejects orphans as a caller bug. Sandbox: SecretResolver::from_provider_env_with_passthrough handles the new mode — passthrough keys go to child env as real values, resolver does not learn them; non-passthrough keys still get the canonical placeholder. CLI: provider create/update accept --passthrough KEY (repeatable); provider get annotates passthrough keys, provider list adds a PASSTHROUGH column. Refs #894 (selective passthrough lands; custom-format placeholders deferred to a follow-up). Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
1 parent 9751872 commit 8446a90

17 files changed

Lines changed: 929 additions & 49 deletions

File tree

architecture/sandbox-providers.md

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ Provider is defined in `proto/datamodel.proto`:
3535
- `type`: canonical provider slug (`claude`, `gitlab`, `github`, etc.)
3636
- `credentials`: `map<string, string>` for secret values
3737
- `config`: `map<string, string>` for non-secret settings
38+
- `passthrough_credentials`: `repeated string` listing credential keys whose
39+
real value is injected directly into the agent process, bypassing the
40+
canonical placeholder substitution (see [Selective Passthrough](#selective-passthrough)).
3841

3942
The gRPC surface is defined in `proto/openshell.proto`:
4043

@@ -363,6 +366,92 @@ CLI: openshell sandbox create -- claude
363366
+-- Proxy rewrites outbound auth header placeholders -> real secrets
364367
```
365368

369+
## Selective Passthrough
370+
371+
The canonical placeholder model breaks for credentials that:
372+
373+
- are validated **in-process** by an SDK before any network call (Slack
374+
`@slack/web-api` checks the `xoxb-` prefix; `@slack/socket-mode` checks
375+
`xapp-`; OAuth libraries parse JWT structure; AWS SDKs validate access-key
376+
format),
377+
- are sent over **transports the L7 proxy cannot rewrite** (WebSocket payloads
378+
after HTTP `101 Switching Protocols` upgrade — for example Discord's
379+
IDENTIFY frame), or
380+
- are consumed by **in-process crypto** that never reaches the wire (Slack
381+
signing-secret HMAC, Discord interaction-signature verification).
382+
383+
For these cases the operator can opt specific credential keys into
384+
**passthrough**: the supervisor injects the **real value** into the agent's
385+
environment instead of the canonical
386+
`openshell:resolve:env:<KEY>` placeholder, and the resolver does not learn
387+
that key. The L7 proxy therefore performs no substitution for it.
388+
389+
### Configuration
390+
391+
Each entry in `Provider.passthrough_credentials` must:
392+
393+
- be a valid environment-variable name (matches `^[A-Za-z_][A-Za-z0-9_]*$`),
394+
- be present as a key in `Provider.credentials`,
395+
- appear at most once.
396+
397+
The gateway rejects updates that violate any of these conditions.
398+
399+
**Update merge semantics.** Proto3 cannot distinguish "field unset" from
400+
"field set to empty list" for repeated strings, so the gateway uses a
401+
two-mode merge:
402+
403+
- **Empty incoming list** (caller is not declaring passthrough) — preserve
404+
the existing list, but auto-prune any entries whose credential was deleted
405+
in the same update. Without this prune, marking a credential passthrough
406+
and then deleting that credential would lock the provider in an invalid
407+
state.
408+
- **Non-empty incoming list** — replace the stored list verbatim. Strict
409+
validation still applies, so an explicit entry naming a non-existent
410+
credential is rejected (treated as a caller bug, not a state-rescue
411+
request).
412+
413+
**Clearing the list.** Because proto3 cannot signal "set to empty" for
414+
repeated strings, there is no direct update that wipes the passthrough
415+
list while leaving credentials intact. Available paths:
416+
417+
- delete the underlying credentials (auto-prune removes the corresponding
418+
passthrough entries),
419+
- replace the list with a non-empty subset (send the entries you want to
420+
keep), or
421+
- recreate the provider.
422+
423+
### Resolution and merge across providers
424+
425+
`resolve_provider_environment()` collects credentials and the union of
426+
passthrough keys across all providers attached to a sandbox. When two providers
427+
declare the same credential key, the first provider's value wins (existing
428+
semantics) and the passthrough flag follows the **winning** value: a
429+
passthrough opt-in declared on the second provider has no effect.
430+
431+
### Supervisor injection
432+
433+
`SecretResolver::from_provider_env_with_passthrough()` consumes both the env
434+
map and the passthrough key list. For each credential:
435+
436+
- key in passthrough list → child env receives the real value; resolver does
437+
not register a placeholder entry;
438+
- otherwise → child env receives the canonical placeholder and the resolver
439+
maps it to the real value for on-the-wire substitution.
440+
441+
If every key is passthrough the resolver is `None` and no L7 substitution
442+
runs.
443+
444+
### Security trade-off
445+
446+
Passthrough drops the "agent never sees the real secret" invariant for the
447+
listed keys. The real value is at rest in `/proc/<agent-pid>/environ` and any
448+
descendant process inherits it. Use sparingly:
449+
450+
- prefer canonical placeholders whenever the consumer permits it;
451+
- only opt in keys whose consumer demonstrably fails with the placeholder
452+
(in-process format check, opaque transport, in-process crypto);
453+
- document each opt-in in the provider's deployment notes.
454+
366455
## Persistence and Validation
367456

368457
The gateway enforces:

crates/openshell-cli/src/main.rs

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,15 @@ enum ProviderCommands {
709709
/// Provider config key/value pair.
710710
#[arg(long = "config", value_name = "KEY=VALUE")]
711711
config: Vec<String>,
712+
713+
/// Credential key whose real value is injected directly into the
714+
/// agent process, bypassing the canonical placeholder substitution
715+
/// and L7 proxy rewriting. Each key must also appear in `--credential`
716+
/// (or be discovered via `--from-existing`). Drops the "agent never
717+
/// sees the real secret" invariant for that key — see provider docs
718+
/// for the security trade-off.
719+
#[arg(long = "passthrough", value_name = "KEY")]
720+
passthrough: Vec<String>,
712721
},
713722

714723
/// Fetch a provider by name.
@@ -757,6 +766,15 @@ enum ProviderCommands {
757766
/// Provider config key/value pair.
758767
#[arg(long = "config", value_name = "KEY=VALUE")]
759768
config: Vec<String>,
769+
770+
/// Credential key whose real value is injected directly into the
771+
/// agent process, bypassing the canonical placeholder substitution
772+
/// and L7 proxy rewriting. Replaces the existing passthrough list
773+
/// when non-empty. Each key must also be present in the merged
774+
/// credentials. Drops the "agent never sees the real secret"
775+
/// invariant for that key.
776+
#[arg(long = "passthrough", value_name = "KEY")]
777+
passthrough: Vec<String>,
760778
},
761779

762780
/// Delete providers by name.
@@ -2742,6 +2760,7 @@ async fn main() -> Result<()> {
27422760
from_existing,
27432761
credentials,
27442762
config,
2763+
passthrough,
27452764
} => {
27462765
run::provider_create(
27472766
endpoint,
@@ -2750,6 +2769,7 @@ async fn main() -> Result<()> {
27502769
from_existing,
27512770
&credentials,
27522771
&config,
2772+
&passthrough,
27532773
&tls,
27542774
)
27552775
.await?;
@@ -2769,13 +2789,15 @@ async fn main() -> Result<()> {
27692789
from_existing,
27702790
credentials,
27712791
config,
2792+
passthrough,
27722793
} => {
27732794
run::provider_update(
27742795
endpoint,
27752796
&name,
27762797
from_existing,
27772798
&credentials,
27782799
&config,
2800+
&passthrough,
27792801
&tls,
27802802
)
27812803
.await?;

crates/openshell-cli/src/run.rs

Lines changed: 85 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3576,6 +3576,7 @@ async fn auto_create_provider(
35763576
r#type: provider_type.to_string(),
35773577
credentials: discovered.credentials.clone(),
35783578
config: discovered.config.clone(),
3579+
passthrough_credentials: Vec::new(),
35793580
}),
35803581
};
35813582

@@ -3616,6 +3617,7 @@ async fn auto_create_provider(
36163617
r#type: provider_type.to_string(),
36173618
credentials: discovered.credentials.clone(),
36183619
config: discovered.config.clone(),
3620+
passthrough_credentials: Vec::new(),
36193621
}),
36203622
};
36213623

@@ -3676,6 +3678,50 @@ fn parse_key_value_pairs(items: &[String], flag: &str) -> Result<HashMap<String,
36763678
Ok(map)
36773679
}
36783680

3681+
/// Parse `--passthrough KEY` flags into a deduplicated list. Validates that
3682+
/// each entry is non-empty and that no key is supplied more than once.
3683+
///
3684+
/// Cross-referencing against the credential map is intentionally not done
3685+
/// here — call [`ensure_passthrough_in_credentials`] when the caller has the
3686+
/// full local credential set (e.g. on `provider create` after
3687+
/// `--from-existing` discovery). On `provider update` the credential set
3688+
/// lives only on the server, and the server re-validates anyway.
3689+
fn parse_passthrough_keys(items: &[String]) -> Result<Vec<String>> {
3690+
let mut seen = HashSet::new();
3691+
let mut out = Vec::with_capacity(items.len());
3692+
for item in items {
3693+
let key = item.trim();
3694+
if key.is_empty() {
3695+
return Err(miette::miette!("--passthrough key cannot be empty"));
3696+
}
3697+
if !seen.insert(key.to_string()) {
3698+
return Err(miette::miette!(
3699+
"--passthrough key '{key}' was supplied more than once"
3700+
));
3701+
}
3702+
out.push(key.to_string());
3703+
}
3704+
Ok(out)
3705+
}
3706+
3707+
/// Reject any passthrough key that is not present in `credential_map`. The
3708+
/// caller is responsible for assembling the full local credential set before
3709+
/// invoking — typically explicit `--credential` entries merged with anything
3710+
/// `--from-existing` discovered.
3711+
fn ensure_passthrough_in_credentials(
3712+
passthrough: &[String],
3713+
credential_map: &HashMap<String, String>,
3714+
) -> Result<()> {
3715+
for key in passthrough {
3716+
if !credential_map.contains_key(key) {
3717+
return Err(miette::miette!(
3718+
"--passthrough '{key}' is not present in resolved credentials"
3719+
));
3720+
}
3721+
}
3722+
Ok(())
3723+
}
3724+
36793725
fn parse_credential_pairs(items: &[String]) -> Result<HashMap<String, String>> {
36803726
let mut map = HashMap::new();
36813727

@@ -3712,13 +3758,15 @@ fn parse_credential_pairs(items: &[String]) -> Result<HashMap<String, String>> {
37123758
Ok(map)
37133759
}
37143760

3761+
#[allow(clippy::too_many_arguments)] // user-facing CLI command
37153762
pub async fn provider_create(
37163763
server: &str,
37173764
name: &str,
37183765
provider_type: &str,
37193766
from_existing: bool,
37203767
credentials: &[String],
37213768
config: &[String],
3769+
passthrough: &[String],
37223770
tls: &TlsOptions,
37233771
) -> Result<()> {
37243772
if from_existing && !credentials.is_empty() {
@@ -3735,6 +3783,7 @@ pub async fn provider_create(
37353783

37363784
let mut credential_map = parse_credential_pairs(credentials)?;
37373785
let mut config_map = parse_key_value_pairs(config, "--config")?;
3786+
let passthrough_credentials = parse_passthrough_keys(passthrough)?;
37383787

37393788
if from_existing {
37403789
let registry = ProviderRegistry::new();
@@ -3762,6 +3811,11 @@ pub async fn provider_create(
37623811
));
37633812
}
37643813

3814+
// The full credential set is known locally on create (--credential plus
3815+
// anything --from-existing discovered), so cross-check passthrough keys
3816+
// here for fast feedback. The server re-validates regardless.
3817+
ensure_passthrough_in_credentials(&passthrough_credentials, &credential_map)?;
3818+
37653819
let response = client
37663820
.create_provider(CreateProviderRequest {
37673821
provider: Some(Provider {
@@ -3774,6 +3828,7 @@ pub async fn provider_create(
37743828
r#type: provider_type,
37753829
credentials: credential_map,
37763830
config: config_map,
3831+
passthrough_credentials,
37773832
}),
37783833
})
37793834
.await
@@ -3806,8 +3861,25 @@ pub async fn provider_get(server: &str, name: &str, tls: &TlsOptions) -> Result<
38063861
.provider
38073862
.ok_or_else(|| miette::miette!("provider missing from response"))?;
38083863

3809-
let credential_keys = provider.credentials.keys().cloned().collect::<Vec<_>>();
3810-
let config_keys = provider.config.keys().cloned().collect::<Vec<_>>();
3864+
let passthrough: HashSet<&str> = provider
3865+
.passthrough_credentials
3866+
.iter()
3867+
.map(String::as_str)
3868+
.collect();
3869+
let mut credential_keys = provider
3870+
.credentials
3871+
.keys()
3872+
.map(|k| {
3873+
if passthrough.contains(k.as_str()) {
3874+
format!("{k} (passthrough)")
3875+
} else {
3876+
k.clone()
3877+
}
3878+
})
3879+
.collect::<Vec<_>>();
3880+
credential_keys.sort();
3881+
let mut config_keys = provider.config.keys().cloned().collect::<Vec<_>>();
3882+
config_keys.sort();
38113883

38123884
println!("{}", "Provider:".cyan().bold());
38133885
println!();
@@ -3878,19 +3950,21 @@ pub async fn provider_list(
38783950
.max(4);
38793951

38803952
println!(
3881-
"{:<name_width$} {:<type_width$} {:<16} {}",
3953+
"{:<name_width$} {:<type_width$} {:<16} {:<13} {}",
38823954
"NAME".bold(),
38833955
"TYPE".bold(),
38843956
"CREDENTIAL_KEYS".bold(),
3957+
"PASSTHROUGH".bold(),
38853958
"CONFIG_KEYS".bold(),
38863959
);
38873960

38883961
for provider in providers {
38893962
println!(
3890-
"{:<name_width$} {:<type_width$} {:<16} {}",
3963+
"{:<name_width$} {:<type_width$} {:<16} {:<13} {}",
38913964
provider.object_name().to_string(),
38923965
provider.r#type,
38933966
provider.credentials.len(),
3967+
provider.passthrough_credentials.len(),
38943968
provider.config.len(),
38953969
);
38963970
}
@@ -3904,6 +3978,7 @@ pub async fn provider_update(
39043978
from_existing: bool,
39053979
credentials: &[String],
39063980
config: &[String],
3981+
passthrough: &[String],
39073982
tls: &TlsOptions,
39083983
) -> Result<()> {
39093984
if from_existing && !credentials.is_empty() {
@@ -3916,6 +3991,11 @@ pub async fn provider_update(
39163991

39173992
let mut credential_map = parse_credential_pairs(credentials)?;
39183993
let mut config_map = parse_key_value_pairs(config, "--config")?;
3994+
// On update the local --credential set may legitimately be empty (the
3995+
// operator is only changing the passthrough list against existing stored
3996+
// credentials), so the cross-check against credentials is delegated to
3997+
// server-side validation, which sees the post-merge state.
3998+
let passthrough_credentials = parse_passthrough_keys(passthrough)?;
39193999

39204000
if from_existing {
39214001
// Fetch the existing provider to discover its type for credential lookup.
@@ -3960,6 +4040,7 @@ pub async fn provider_update(
39604040
r#type: String::new(),
39614041
credentials: credential_map,
39624042
config: config_map,
4043+
passthrough_credentials,
39634044
}),
39644045
})
39654046
.await

crates/openshell-cli/tests/ensure_providers_integration.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ impl TestOpenShell {
115115
r#type: provider_type.to_string(),
116116
credentials: HashMap::new(),
117117
config: HashMap::new(),
118+
passthrough_credentials: Vec::new(),
118119
},
119120
);
120121
}
@@ -293,6 +294,11 @@ impl OpenShell for TestOpenShell {
293294
r#type: existing.r#type,
294295
credentials: merge(existing.credentials, provider.credentials),
295296
config: merge(existing.config, provider.config),
297+
passthrough_credentials: if provider.passthrough_credentials.is_empty() {
298+
existing.passthrough_credentials
299+
} else {
300+
provider.passthrough_credentials
301+
},
296302
};
297303
let updated_name = updated.object_name().to_string();
298304
providers.insert(updated_name, updated.clone());

0 commit comments

Comments
 (0)