-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(drivers): inject local sandbox identity #2335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
98ef34f
c8f2917
3c5ec17
f1649ef
5cc88bf
0b74fdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,7 @@ const SUPERVISOR_PATH: &str = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin | |
| const HOST_OPENSHELL_INTERNAL: &str = "host.openshell.internal"; | ||
| const HOST_DOCKER_INTERNAL: &str = "host.docker.internal"; | ||
| const DOCKER_NETWORK_DRIVER: &str = "bridge"; | ||
| const DEFAULT_SANDBOX_UID: u32 = 10_001; | ||
|
|
||
| /// Queried by the Docker driver to decide when a sandbox's supervisor | ||
| /// relay is live. Implementations return `true` once a sandbox has an | ||
|
|
@@ -142,6 +143,14 @@ pub struct DockerComputeConfig { | |
| /// Set to `0` to leave Docker's runtime/default PID limit unchanged. | ||
| pub sandbox_pids_limit: i64, | ||
|
|
||
| /// Numeric identity used for the agent process. The root supervisor | ||
| /// retains UID 0 while preparing the sandbox. | ||
| pub sandbox_uid: u32, | ||
|
|
||
| /// Numeric group used for the agent process. Defaults to the resolved | ||
| /// sandbox UID when unset. | ||
| pub sandbox_gid: Option<u32>, | ||
|
|
||
| /// Allow sandbox requests to attach host bind mounts through | ||
| /// `template.driver_config`. | ||
| #[serde(default)] | ||
|
|
@@ -165,6 +174,8 @@ impl Default for DockerComputeConfig { | |
| host_gateway_ip: String::new(), | ||
| ssh_socket_path: "/run/openshell/ssh.sock".to_string(), | ||
| sandbox_pids_limit: DEFAULT_SANDBOX_PIDS_LIMIT, | ||
| sandbox_uid: DEFAULT_SANDBOX_UID, | ||
| sandbox_gid: None, | ||
| enable_bind_mounts: false, | ||
| } | ||
| } | ||
|
|
@@ -194,6 +205,8 @@ struct DockerDriverRuntimeConfig { | |
| supports_gpu: bool, | ||
| allow_all_default_gpu: bool, | ||
| sandbox_pids_limit: i64, | ||
| sandbox_uid: u32, | ||
| sandbox_gid: u32, | ||
| enable_bind_mounts: bool, | ||
| } | ||
|
|
||
|
|
@@ -314,6 +327,7 @@ impl DockerComputeDriver { | |
| docker_config: &DockerComputeConfig, | ||
| supervisor_readiness: Arc<dyn SupervisorReadiness>, | ||
| ) -> CoreResult<Self> { | ||
| let sandbox_identity = resolve_sandbox_identity(docker_config)?; | ||
| let socket_path = docker_config | ||
| .socket_path | ||
| .clone() | ||
|
|
@@ -391,6 +405,8 @@ impl DockerComputeDriver { | |
| supports_gpu, | ||
| allow_all_default_gpu, | ||
| sandbox_pids_limit: docker_config.sandbox_pids_limit, | ||
| sandbox_uid: sandbox_identity.0, | ||
| sandbox_gid: sandbox_identity.1, | ||
| enable_bind_mounts: docker_config.enable_bind_mounts, | ||
| }, | ||
| events: broadcast::channel(WATCH_BUFFER).0, | ||
|
|
@@ -2184,6 +2200,14 @@ fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig | |
| openshell_core::sandbox_env::TELEMETRY_ENABLED.to_string(), | ||
| openshell_core::telemetry::enabled_env_value().to_string(), | ||
| ); | ||
| environment.insert( | ||
| openshell_core::sandbox_env::SANDBOX_UID.to_string(), | ||
| config.sandbox_uid.to_string(), | ||
| ); | ||
| environment.insert( | ||
| openshell_core::sandbox_env::SANDBOX_GID.to_string(), | ||
| config.sandbox_gid.to_string(), | ||
| ); | ||
| // The root supervisor executes namespace helpers during bootstrap; keep | ||
| // their search path driver-owned even when the template/spec set PATH. | ||
| environment.insert("PATH".to_string(), SUPERVISOR_PATH.to_string()); | ||
|
|
@@ -2645,6 +2669,22 @@ fn validate_sandbox_pids_limit(value: i64) -> CoreResult<()> { | |
| Ok(()) | ||
| } | ||
|
|
||
| fn resolve_sandbox_identity(config: &DockerComputeConfig) -> CoreResult<(u32, u32)> { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The effective duplication of this across both drivers and the fact that it returns a This is what DetailsSince Docker and Podman have identical defaulting, range validation, tuple return values, and nearly identical tests, a shared implementation is preferable. The most natural current home is openshell-policy, because it already owns:Putting it in openshell-core::driver_utils would create a dependency problem: openshell-policy already depends on openshell-core, so core cannot depend back on policy. Iβd introduce a driver-neutral validated type: #[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct SandboxIdentity {
pub uid: u32,
pub gid: u32,
}
impl SandboxIdentity {
pub fn new(
uid: u32,
gid: Option<u32>,
) -> Result<Self, SandboxIdentityError> {
let identity = Self {
uid,
gid: gid.unwrap_or(uid),
};
identity.validate()?;
Ok(identity)
}
fn validate(&self) -> Result<(), SandboxIdentityError> {
for (field, value) in [
("sandbox_uid", self.uid),
("sandbox_gid", self.gid),
] {
if !(MIN_SANDBOX_UID..=MAX_SANDBOX_UID).contains(&value) {
return Err(SandboxIdentityError {
field,
value,
});
}
}
Ok(())
}
}With a shared error that does not mention Docker or Podman: #[derive(Debug, Clone, PartialEq, Eq)]
pub struct SandboxIdentityError {
field: &'static str,
value: u32,
}
impl std::fmt::Display for SandboxIdentityError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(
f,
"{} must be in [{MIN_SANDBOX_UID}, {MAX_SANDBOX_UID}], got {}",
self.field,
self.value,
)
}
}
impl std::error::Error for SandboxIdentityError {}Docker maps that into its configuration error: let sandbox_identity = openshell_policy::SandboxIdentity::new(
docker_config.sandbox_uid,
docker_config.sandbox_gid,
)
.map_err(|error| Error::config(format!("docker {error}")))?;Then: sandbox_uid: sandbox_identity.uid,
sandbox_gid: sandbox_identity.gid,Podman can keep a thin configuration method if that fits its current organization: pub fn sandbox_identity(
&self,
) -> Result<openshell_policy::SandboxIdentity, PodmanApiError> {
openshell_policy::SandboxIdentity::new(
self.sandbox_uid,
self.sandbox_gid,
)
.map_err(|error| PodmanApiError::InvalidInput(error.to_string()))
}And its environment construction becomes: let sandbox_identity = config
.sandbox_identity()
.expect("Podman configuration is validated before build_env");
env.insert(
openshell_core::sandbox_env::SANDBOX_UID.into(),
sandbox_identity.uid.to_string(),
);
env.insert(
openshell_core::sandbox_env::SANDBOX_GID.into(),
sandbox_identity.gid.to_string(),
);The defaulting and boundary tests should move into openshell-policy. Docker and Podman would retain only tests proving that their configuration and error mapping use the shared type correctly. That establishes one definition of the invariant across both drivers: a constructed SandboxIdentity always has a resolved GID and policy-valid UID/GID. |
||
| let uid = config.sandbox_uid; | ||
| let gid = config.sandbox_gid.unwrap_or(uid); | ||
| for (field, value) in [("sandbox_uid", uid), ("sandbox_gid", gid)] { | ||
| if !(openshell_policy::MIN_SANDBOX_UID..=openshell_policy::MAX_SANDBOX_UID).contains(&value) | ||
| { | ||
| return Err(Error::config(format!( | ||
| "docker {field} must be in [{}, {}]", | ||
| openshell_policy::MIN_SANDBOX_UID, | ||
| openshell_policy::MAX_SANDBOX_UID, | ||
| ))); | ||
| } | ||
| } | ||
| Ok((uid, gid)) | ||
| } | ||
|
|
||
| fn docker_pids_limit(value: i64) -> Result<Option<i64>, Status> { | ||
| if value < 0 { | ||
| return Err(Status::failed_precondition( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, but the UID is never "resolved"; it's always at least the default value of
10_001. The GID will be set to the UID unless otherwise set.