Skip to content

Commit a65ec41

Browse files
committed
fix(network): narrow additional CA review scope
Signed-off-by: Jesse Jaggars <jjaggars@redhat.com>
1 parent 63b6086 commit a65ec41

7 files changed

Lines changed: 26 additions & 83 deletions

File tree

crates/openshell-supervisor-process/src/sandbox/linux/landlock.rs

Lines changed: 0 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -96,77 +96,6 @@ pub struct PreparedRuleset {
9696
compatibility: LandlockCompatibility,
9797
}
9898

99-
impl PreparedRuleset {
100-
/// Try to add a read-write rule for `/dev/tty` (the controlling terminal).
101-
///
102-
/// `/dev/tty` is a magic device that refers to the calling process's
103-
/// controlling terminal. The supervisor process has no controlling terminal,
104-
/// so `PathFd::new("/dev/tty")` fails with `ENXIO` during [`prepare`].
105-
/// After the child calls `setsid()` + `TIOCSCTTY`, `/dev/tty` becomes
106-
/// accessible. This method lets the child add the rule before
107-
/// `restrict_self()`.
108-
///
109-
/// Consumes and returns `self` because `RulesetCreated::add_rule` takes
110-
/// ownership. Failures are silently ignored — `/dev/tty` is only needed
111-
/// by interactive TUI programs (`prompt_toolkit`, ratatui, etc.).
112-
pub fn add_dev_tty_if_available(self) -> Self {
113-
let path = Path::new("/dev/tty");
114-
let Ok(path_fd) = PathFd::new(path) else {
115-
return self; // no controlling terminal — nothing to do
116-
};
117-
let abi = ABI::V2;
118-
let Ok(allowed_access) = access_for_path_fd(&path_fd, AccessFs::from_all(abi), abi) else {
119-
return self;
120-
};
121-
let compatibility = self.compatibility;
122-
// add_rule takes ownership of `self.ruleset`. On success we get
123-
// the updated ruleset back; on error it is consumed irreversibly.
124-
// The error requires a landlock_add_rule syscall failure (kernel
125-
// bug), so we treat it as unreachable and fall through without
126-
// the /dev/tty rule.
127-
match self
128-
.ruleset
129-
.add_rule(PathBeneath::new(path_fd, allowed_access))
130-
{
131-
Ok(ruleset) => Self {
132-
ruleset,
133-
compatibility,
134-
},
135-
Err(err) => {
136-
tracing::debug!(
137-
error = %err,
138-
"Landlock add_rule for /dev/tty failed (non-fatal)"
139-
);
140-
// Ruleset is consumed; build a fresh empty one so
141-
// enforce() can still call restrict_self(). The original
142-
// rules were already committed to the kernel fd, but that
143-
// fd is now gone, so enforce will apply a maximally
144-
// restrictive empty ruleset. This is preferable to
145-
// skipping Landlock entirely.
146-
let fallback = Ruleset::default()
147-
.set_compatibility(compat_level(&compatibility))
148-
.handle_access(AccessFs::from_all(abi))
149-
.and_then(Ruleset::create);
150-
fallback.map_or_else(
151-
|_| {
152-
// Cannot recover at all. The process will
153-
// continue without Landlock enforcement.
154-
// Return a dummy that enforce() can handle.
155-
unreachable!(
156-
"failed to create fallback Landlock ruleset \
157-
after add_rule failure for /dev/tty"
158-
)
159-
},
160-
|ruleset| Self {
161-
ruleset,
162-
compatibility,
163-
},
164-
)
165-
}
166-
}
167-
}
168-
}
169-
17099
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
171100
enum PathOpenMode {
172101
Privileged,

crates/openshell-supervisor-process/src/sandbox/linux/mod.rs

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,6 @@ pub fn prepare_current_user(
5151
/// Neither operation requires root privileges.
5252
pub fn enforce(prepared: PreparedSandbox) -> Result<()> {
5353
if let Some(ruleset) = prepared.landlock {
54-
// The child process now has a controlling terminal (setsid +
55-
// TIOCSCTTY ran before this point), so /dev/tty is accessible.
56-
// The supervisor couldn't add this rule during prepare() because
57-
// it has no controlling terminal (/dev/tty returns ENXIO).
58-
let ruleset = ruleset.add_dev_tty_if_available();
5954
landlock::enforce(ruleset)?;
6055
}
6156
seccomp::apply(&prepared.policy)?;

deploy/helm/openshell/README.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,13 @@ The driver manages the deterministic name
115115
`server.sandboxJwt.gatewayId`, or the chart fullname when unset). Its `get` and
116116
`patch` permissions must be restricted to that exact name; `create` must remain
117117
a separate unrestricted ConfigMap permission because Kubernetes cannot apply
118-
`resourceNames` to create authorization. If you render the TOML setting outside
119-
this Helm value, grant that same split RBAC policy in every sandbox namespace.
118+
`resourceNames` to create authorization. Enabling this therefore gives the
119+
gateway/driver service account namespace-wide ConfigMap-create authority. In
120+
managed/operator modes, the chart's ClusterRoleBinding makes that permission
121+
cluster-wide for the bound service account; use a dedicated service account and
122+
scoped bindings when that boundary matters. If you render the TOML setting
123+
outside this Helm value, grant that same split RBAC policy in every sandbox
124+
namespace.
120125

121126
Disabling the setting intentionally does not delete driver-managed ConfigMaps:
122127
the driver has no list/delete permissions, and a shared or operator namespace

deploy/helm/openshell/README.md.gotmpl

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,13 @@ The driver manages the deterministic name
115115
`server.sandboxJwt.gatewayId`, or the chart fullname when unset). Its `get` and
116116
`patch` permissions must be restricted to that exact name; `create` must remain
117117
a separate unrestricted ConfigMap permission because Kubernetes cannot apply
118-
`resourceNames` to create authorization. If you render the TOML setting outside
119-
this Helm value, grant that same split RBAC policy in every sandbox namespace.
118+
`resourceNames` to create authorization. Enabling this therefore gives the
119+
gateway/driver service account namespace-wide ConfigMap-create authority. In
120+
managed/operator modes, the chart's ClusterRoleBinding makes that permission
121+
cluster-wide for the bound service account; use a dedicated service account and
122+
scoped bindings when that boundary matters. If you render the TOML setting
123+
outside this Helm value, grant that same split RBAC policy in every sandbox
124+
namespace.
120125

121126
Disabling the setting intentionally does not delete driver-managed ConfigMaps:
122127
the driver has no list/delete permissions, and a shared or operator namespace

deploy/helm/openshell/templates/clusterrole.yaml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,10 @@ rules:
7878
# The Kubernetes driver reads and server-side-applies only its deterministic,
7979
# gateway-scoped destination CA ConfigMap in selected namespaces. CREATE
8080
# cannot use resourceNames because Kubernetes authorizes it before the name
81-
# is available.
81+
# is available. This is a namespace-wide ConfigMap-create capability when
82+
# bound through a Role, and becomes cluster-wide when this ClusterRole is
83+
# bound cluster-wide; use a dedicated service account and scoped bindings
84+
# when that boundary matters.
8285
- apiGroups:
8386
- ""
8487
resources:

deploy/helm/openshell/templates/role.yaml

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,9 @@ rules:
5151
{{- if $additionalCaEnabled }}
5252
# The Kubernetes driver reads and server-side-applies only its deterministic,
5353
# gateway-scoped destination CA ConfigMap. CREATE cannot use resourceNames:
54-
# Kubernetes authorizes creates before the object name is available.
54+
# Kubernetes authorizes creates before the object name is available. This is
55+
# a namespace-wide ConfigMap-create capability; use a dedicated gateway
56+
# service account when that boundary matters.
5557
- apiGroups:
5658
- ""
5759
resources:

skills/debug-openshell-cluster/SKILL.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,11 @@ chart fullname when it is unset. The managed ConfigMap must have
354354
`openshell.ai/managed-by=openshell` and the matching
355355
`openshell.ai/gateway-id` label. The chart grants `get`/`patch` only on this
356356
exact name and separate unrestricted `create` (Kubernetes cannot restrict a
357-
create with `resourceNames`). Check both the object and authorization:
357+
create with `resourceNames`). Enabling this gives the gateway/driver service
358+
account namespace-wide ConfigMap-create authority. In managed/operator modes,
359+
the chart's ClusterRoleBinding makes that permission cluster-wide for the bound
360+
service account; prefer a dedicated service account and scoped bindings when
361+
that boundary matters. Check both the object and authorization:
358362

359363
```bash
360364
kubectl -n <sandbox-namespace> get configmap \

0 commit comments

Comments
 (0)