You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Part 1 of #471 — the Sharing list (owner + collaborators) becomes the SSH allowlist, evaluated by the manager on every connection. Part 2 (enrollment script for existing containers, run per Proxmox node) follows once this is merged.
How it works
sequenceDiagram
participant U as ssh client
participant S as sshd (container)
participant M as Manager
participant L as SSSD / LDAP
U->>S: connect as alice
Note over S: auth (publickey keys via sss_ssh_authorizedkeys, or password/MFA)
S->>S: PAM account: pam_exec ssh-access-check alice
S->>M: GET /api/v1/containers/:id/ssh-access/alice (Bearer container token)
M-->>S: 204 owner/collaborator · 403 otherwise
alt allowed (204)
S-->>U: session granted
else denied (403)
S-->>U: permission denied (account phase)
end
Loading
Enforcement is a single PAM account hook. Because UsePAM yes is in effect, sshd runs the account phase on every successful authentication — publickey and password/MFA alike — so one hook gates every path. Key serving stays the stock sss_ssh_authorizedkeys command (no manager round-trip during key negotiation).
Manager DB stays the single source of truth; share/unshare applies on the next connection. No timers, versions, or sshd reloads.
Authentication is unchanged (LDAP sshPublicKey); this only adds authorization.
Manager unreachable ⇒ recent cached allow is honoured (24 h), so the owner keeps access through an outage while strangers stay out.
Containers without a token (created before this) behave exactly as before and show SSH not enforced in the UI.
Container.sshAllowsUser(), rotateSshAccessToken(), ensureSshAccessToken(currentLxcEnv) (reuses the running token on reconfigure), verifySshAccessToken().
CONTAINER_ID / CONTAINER_SSH_TOKEN are reserved env keys — stripped from user/admin input, injected by buildLxcEnvConfig({ sshAccessToken }).
GET /api/v1/containers/:id/ssh-access/:username — container-token auth; 204 / 403 / 400 bad username / 401 bad token; Cache-Control: no-store; denials logged.
POST /sites/:siteId/containers/:id/ssh-access/token — owner/admin mints/rotates (used by the part-2 enrollment script).
Serializer: sshAccessEnforced.
Base image (images/base)
ssh-access-check — the sshd PAM account hook (pam_exec), covering both the key and password/MFA paths. Token is passed to curl via stdin config, never argv. Runs as root, so the token file is root-only (0600); no dedicated service user.
AuthorizedKeysCommand is the stock sss_ssh_authorizedkeys (as nobody) — unchanged; authorization is not done on the key path.
ssh-access-setup.service — copies the three env vars from PID 1 into /etc/ssh-access/ (0700 root:root); environment.sh filters CONTAINER_SSH_TOKEN out of /etc/environment.
Client
Sharing copy now says what it does; SshAccessBadge shows SSH enforced / SSH not enforced.
Owner + collaborators are now the SSH allowlist, evaluated by the manager
on every connection. Closes#471 (part 1 of 2; enrollment script for
existing containers follows).
Manager:
- Containers.sshAccessTokenHash + Container.rotate/ensure/verifySshAccessToken
- GET /api/v1/containers/:id/ssh-access/:username (container-token auth):
204 owner/collaborator, 403 otherwise
- POST /sites/:siteId/containers/:id/ssh-access/token (owner/admin) mints
- CONTAINER_ID / CONTAINER_SSH_TOKEN reserved env keys injected on create,
preserved across reconfigure; MANAGER_URL seeded as a default env var
- serializer: sshAccessEnforced
Base image:
- ssh-access-check (AuthorizedKeysCommand wrapper + PAM account hook) asks
the manager; cached allow honoured when the manager is unreachable;
unenrolled containers behave as before
- ssh-access-setup.service copies the token out of PID 1's env into
/etc/ssh-access (kept out of /etc/environment)
Client: Sharing copy + SshAccessBadge. Docs + OpenAPI updated.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Implements per-container SSH authorization based on the container’s Sharing list (owner + collaborators), enforced at SSH connect time by having the base image call back to the manager via a container-scoped token.
Changes:
Add manager endpoints + model/migrations to mint/verify per-container SSH-access tokens and answer “may user X SSH into container Y?”
Update the base image to gate both SSH key lookup (AuthorizedKeysCommand) and PAM account checks via the manager callback, with outage-safe cached allow.
Update UI, OpenAPI, and docs to reflect “SSH enforced / not enforced” and the new MANAGER_URL setting.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: single native PAM account hook instead of two enforcement points
I looked at whether there's a more Linux/LDAP-native way to do this before reviewing correctness. Two findings:
On the LDAP-group route (per-container authentik group + ldap_access_filter (memberOf=…)): I think your decision to not go this way is justified. The authentik LDAP outpost is read-only (memberships would have to be written via the authentik REST API, which the manager has no client for today), it adds a second source of truth to keep in sync, and it inherits SSSD access-cache latency. Keeping the manager DB authoritative and evaluating at connect time is reasonable. (Note: the outpost does expose memberOf, so the filter would technically work — the objection is complexity/consistency, not feasibility.)
On the enforcement mechanism — this is where I'd push for a change. The PR gates the same decision in two places (AuthorizedKeysCommand wrapper for the key path, pam_exec account for the password path). Because UsePAM yes is the default and isn't overridden, the PAM account phase already runs for publickey logins too, so a single account hook covers both paths. Reverting the AuthorizedKeysCommand to stock sss_ssh_authorizedkeys and keeping only the account hook is more native, removes a curl from every key negotiation, and halves the logic to maintain. Details in the two line comments.
This is a design suggestion, not a blocker — happy to discuss the one tradeoff I noted (denial surfaces at account after auth succeeds rather than as "no keys offered").
The reason will be displayed to describe this comment to others. Learn more.
Consolidate to a single PAM account hook; drop the AuthorizedKeysCommand wrapper.
The design currently enforces authorization at two points for the same decision:
AuthorizedKeysCommand wrapper (ssh-access-authorized-keys.sh) — gates the key path
this pam_execaccount hook — gates the password/MFA path
But the base image doesn't override UsePAM, so Debian's default UsePAM yes is in effect. That means the PAM account phase runs on every successful authentication, including publickey — not just password/MFA. So the account hook alone already covers both paths. The PR's own test-ssh-access-check.sh demonstrates this (the PAM_USER honoured case is the key path being gated by the same script).
Concretely, I'd suggest:
Keep this account required pam_exec.so … ssh-access-check line — it's the single, idiomatic choke point. account is exactly where "may this authenticated user log into this host" belongs, and it's method-agnostic.
Revert50-sss-ssh-authorizedkeys.conf back to stock AuthorizedKeysCommand /usr/bin/sss_ssh_authorizedkeys %u with AuthorizedKeysCommandUser nobody. Then ssh-access-authorized-keys.sh can be deleted entirely.
Benefits:
One enforcement path instead of two → half the surface to reason about, test, and keep in sync.
No curl on every key negotiation. Today a denied user still triggers a manager round-trip during AuthorizedKeysCommandand the LDAP key fetch; with account-only, the key exchange stays fully native (sss_ssh_authorizedkeys unchanged) and the single callback happens at account.
Restores the stock, well-understood AuthorizedKeysCommand rather than a shell wrapper that shells out to curl mid-handshake.
One thing to confirm if you take this: a denied user will now fail at accountafter key/password auth succeeds, so they'll see Permission denied at the account stage rather than "no keys offered." That's standard PAM behaviour and arguably clearer, but worth a note in the docs since the current copy describes "no keys → permission denied."
If there's a reason the key path must be denied beforeaccount (e.g. to avoid emitting any auth-success signal), that's the tradeoff to call out explicitly — but for owner/collaborator gating I don't see one.
and ssh-access-authorized-keys.sh can be removed. The account PAM hook already gates the key path because UsePAM yes (Debian default, not overridden here) runs the account phase for publickey logins too. Keeping the wrapper here means a curl round-trip on every key negotiation and a second copy of the enforcement logic to maintain.
A side benefit: reverting this also removes the need for the sshaccess user to be the AuthorizedKeysCommandUser. The only remaining reason for the sshaccess user is readable ownership of /etc/ssh-access/token for the pam_exec script, which runs as root anyway — so you may be able to drop the dedicated user entirely and keep the token 0600 root:root. Worth checking whether pam_exec (root) is the sole reader once the wrapper is gone.
…KeysCommand wrapper
UsePAM yes runs the PAM account phase on every successful auth (publickey
included), so a single pam_exec account hook gates both the key and
password/MFA paths. Revert AuthorizedKeysCommand to stock
sss_ssh_authorizedkeys (as nobody) and remove the ssh-access-authorized-keys
wrapper, eliminating a manager round-trip on every key negotiation and a
second copy of the enforcement logic.
With the wrapper gone, ssh-access-check runs only via pam_exec (as root), so
the dedicated sshaccess service user is no longer needed: drop the useradd and
make /etc/ssh-access and the token file root-only (0700 dir, 0600 token).
Applied the single-PAM-account-hook simplification in 25ce041.
AuthorizedKeysCommand reverted to stock sss_ssh_authorizedkeys (as nobody); ssh-access-authorized-keys.sh removed.
Enforcement is now the one pam_execaccount hook, which covers publickey and password/MFA alike since UsePAM yes runs the account phase on every successful auth.
With the wrapper gone, ssh-access-check runs only as root via pam_exec, so the dedicated sshaccess user is dropped and /etc/ssh-access/ + the token are root-only (0700 dir, 0600 token).
Removes a manager round-trip on every key negotiation and a second copy of the enforcement logic. test-ssh-access-check.sh still passes 19/19 (incl. the PAM_USER path); PR description/diagram updated to match.
User.uniqueUid() preserves periods (create-a-container/models/user.js:58-62), so a valid owner or collaborator such as jane.doe can exist in the manager, but this shared regex rejects it with 400. Include the same valid character set as the manager's usernames, and keep the shell validator in sync.
Report SSH enforcement only after successful enrollment
create-a-container/models/container.js:79
A non-null hash does not prove that sshd is enrolled: ssh-access-setup.sh skips enrollment when MANAGER_URL is blank, custom/legacy templates may not contain the PAM hook, and updateLxcConfig can fail after rotateSshAccessToken() has persisted the hash. The serializer will still report sshAccessEnforced: true while logins remain unrestricted; only set this state after successful enrollment or gate token issuance on a supported, configured image.
Apply no-store headers before authorization validation
The new no-store header is applied only after this validation and inside the route handler. Invalid usernames return 400 before it, and missing/invalid container tokens return from containerTokenAuth before reaching it, so those dynamic authorization responses do not satisfy the documented Cache-Control: no-store contract. Set the header at the start of the auth middleware (before any validation).
Allow periods in SSH username validation
images/base/ssh-access-check.sh:28
This preflight rejects UIDs containing ., even though User.uniqueUid() permits that character and the manager can store such owners/collaborators. For those users the script exits before contacting the manager, so relaxing only the API regex would not restore SSH access; keep this validation aligned with the manager's accepted login-name pattern.
This says the plaintext is only ever in the container environment, but the new owner/admin token endpoint returns the plaintext once to the enrollment caller (create-a-container/routers/api/v1/containers.js:936-947). Document that one-time handoff as well so operators understand the secret's lifecycle.
…manifesto
ssh-access is a resource (docs/mvc-manifesto.md §7.1: new endpoints use the
full layer stack in their own resources/<r>/ folder). Split the single-file
router into router/controller/service/repository/validator + a resource-local
container-token auth middleware:
- router.js: wiring only — validate(params) → containerTokenAuth → ctrl.check
- controller.js: HTTP translation, no model imports, no logic
- service.js: authenticateContainer() + assertUserAllowed(); throws ApiError
- repository.js: the sole model query (findByPk + collaborators)
- validator.js: zod params; reuses Container.USERNAME_RE as the source of truth
- auth.js: container-token (Bearer) auth, delegates verification to the service
Behaviour and wire contract unchanged (204/403/400/401, Cache-Control:
no-store, same path). The mint route (POST /sites/.../ssh-access/token) stays
with the not-yet-migrated containers resource per §7.2.
Tests: moved the api test into resources/ssh-access/__tests__/ (import depth
fixed) and added a service unit test with a mocked repository. ssh-access
suites: 18 passing. Only the pre-existing mcp-proxy failure remains on the
full run.
…tainers service seam
Move POST /sites/:siteId/containers/:id/ssh-access/token out of the legacy
containers god-route and into the ssh-access resource, keeping the URL and wire
contract identical.
To authorize the mint (owner/admin, site-scoped, 404-on-not-visible) without
duplicating container visibility logic, add a minimal containers service seam
per the manifesto (§3 service-to-service, §6 phase 7):
- resources/containers/repository.js: visibleToClauses, findSiteById,
findByIdForSession (Sequelize only, no ApiError).
- resources/containers/service.js: loadForSession() — the 404/403 decisions,
extracted verbatim from containers.js's loadContainerForSession.
- ssh-access service.mintToken() calls containersService.loadForSession()
(service-to-service), then rotates the token. The sites-scoped mint router
is mounted before /:siteId/containers so the deeper path matches first.
Also fixes the previous commit, which deleted routers/api/v1/ssh-access.js but
left index.js requiring './ssh-access' (the app would crash on boot); index.js
now points at resources/ssh-access/router.
Tests: mintToken unit test (containers service mocked) + a DB-backed
containers loadForSession test (owner/collaborator/stranger visibility,
requireManage 403, cross-site and unknown-site 404). ssh-access + containers
suites: 27 passing; only the pre-existing mcp-proxy failure remains.
User.uniqueUid accepts periods and does not impose a 32-character limit (create-a-container/models/user.js:58-62), and Users.uid is a 255-character string. This new regex therefore rejects valid directory usernames such as alice.smith or longer UIDs before owner/collaborator access is evaluated. Align the API and shell validation with the existing UID contract instead of introducing a narrower rule.
No-store header missing on validation and auth errors
The no-store header is set only in the controller, after validation and container-token authentication have run. Invalid usernames (400) and missing or invalid tokens (401) therefore return without Cache-Control: no-store, despite the endpoint contract. Set the header in middleware before validation/authentication.
Validation blocks valid UIDs on unenrolled containers
images/base/ssh-access-check.sh:28
This validation runs before the unenrolled fallback, so a legacy container with no callback files now rejects valid LDAP UIDs outside the narrower 32-character/no-period pattern, despite the documented behavior that unenrolled containers remain open. Apply the validation only when all enrollment files exist and use the same UID pattern as the manager.
…ixes
The check and mint endpoints are the same resource and already share one
service; only their mount prefix and auth differ. Fold the separate
sites-router.js back into router.js — one file owns both routes, each carrying
its own auth + validation middleware (as notifications/router.js does), and the
same module is mounted at both /containers/:id/ssh-access and
/sites/:siteId/containers/:id/ssh-access. No behavior or URL change.
ssh-access is one resource, so it gets one canonical mount. Move the mint
endpoint off the /sites tree onto the resource's own prefix:
POST /api/v1/containers/:id/ssh-access/token (was /sites/:siteId/...)
A container is identified by its globally unique id — siteId was only inherited
from the legacy containers route and added nothing (the GET check already
loaded by id alone). The containers service seam becomes id-based:
loadByIdForSession(id, session, { requireManage }); authorization is unchanged
(owner/collaborator visibility + owner/admin manage gate), and site-scoping was
redundant with the PK, so dropping it loses no protection. Removed the now-unused
findSiteById from the containers repository.
The resource's router now owns both routes under the single prefix (GET
/:username container-token auth, POST /token session auth), mounted once in
index.js. sites.js reverts to its original nested mounts.
Updated openapi.v1.yaml and the api test to the canonical path. The mint
endpoint is new on this branch (unreleased), so no external contract breaks.
Tests: ssh-access + containers suites green (26); only the pre-existing
mcp-proxy failure remains.
Align username validation with supported directory UIDs
create-a-container/models/container.js:13
Container.USERNAME_RE is narrower than the directory UID policy: User.uniqueUid preserves . and has no 32-character cap (create-a-container/models/user.js:58-62). An owner/collaborator such as alice.smith or a longer LDAP UID will therefore be rejected by this API and cannot pass the SSH allowlist. Align this shared regex with the actual supported UID rules.
Cache-Control: no-store is set only in ctrl.check, but validation and container-token authentication run before that controller. Malformed usernames and missing or wrong tokens therefore return without no-store, unlike successful/403 responses and contrary to this endpoint's cache policy. Set the header in middleware before validation and authentication.
Keep PAM username validation consistent with directory UIDs
images/base/ssh-access-check.sh:30
The PAM-side check repeats the same 32-character/alphanumeric-only restriction, so valid directory UIDs containing . (or longer than 32 characters) are denied before curl. Keep this validation in sync with User.uniqueUid and the API; otherwise owner/collaborator access is broken even when the manager would allow the user.
LXC container on a Proxmox node. Unique composite index on `(nodeId, containerId)`. `hostname`, `macAddress`, `ipv4Address` globally unique. `nvidiaRequested` indicates GPU passthrough was requested — the container is assigned to an NVIDIA-capable node and the nvidia hookscript is attached. Belongs to Node and optionally to a Job.
LXC container on a Proxmox node. Unique composite index on `(nodeId, containerId)`. `hostname`, `macAddress`, `ipv4Address` globally unique. `nvidiaRequested` indicates GPU passthrough was requested — the container is assigned to an NVIDIA-capable node and the nvidia hookscript is attached. `sshAccessTokenHash` is the argon2 hash of the token sshd inside the container presents to `GET /api/v1/containers/:id/ssh-access/:username`, which answers from `username` (owner) plus `ContainerCollaborators`; `NULL` means the container is not enrolled and SSH is not restricted. The plaintext is only ever in the container's env (`CONTAINER_SSH_TOKEN`). Belongs to Node and optionally to a Job.
SSH username validation rejects periods in valid UIDs
create-a-container/models/container.js:13
User.uniqueUid() preserves periods (models/user.js:58-62), so an existing owner or collaborator such as alice.smith is a valid account UID but this regex rejects it before the manager is queried. Align the SSH username contract with User.uid (at minimum allow .), and keep the PAM hook's copy in sync.
Failed access checks can be cached by intermediaries
Cache-Control: no-store is set only in ctrl.check, after validation and container-token authentication. 400/401 responses therefore omit it; a shared intermediary could cache a failed response for this URL and serve it after a valid token or username is presented. Set the header before validation/authentication for this GET route.
PAM validation rejects valid usernames containing periods
images/base/ssh-access-check.sh:28
The PAM hook repeats the username mismatch independently: alice.smith is accepted by the manager's UID generation but is denied locally and never reaches the allowlist endpoint. Keep this validation in sync with Container.USERNAME_RE, including periods.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
runleveldev
left a comment
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Part 1 of #471 — the Sharing list (owner + collaborators) becomes the SSH allowlist, evaluated by the manager on every connection. Part 2 (enrollment script for existing containers, run per Proxmox node) follows once this is merged.
How it works
sequenceDiagram participant U as ssh client participant S as sshd (container) participant M as Manager participant L as SSSD / LDAP U->>S: connect as alice Note over S: auth (publickey keys via sss_ssh_authorizedkeys, or password/MFA) S->>S: PAM account: pam_exec ssh-access-check alice S->>M: GET /api/v1/containers/:id/ssh-access/alice (Bearer container token) M-->>S: 204 owner/collaborator · 403 otherwise alt allowed (204) S-->>U: session granted else denied (403) S-->>U: permission denied (account phase) endaccounthook. BecauseUsePAM yesis in effect, sshd runs the account phase on every successful authentication — publickey and password/MFA alike — so one hook gates every path. Key serving stays the stocksss_ssh_authorizedkeyscommand (no manager round-trip during key negotiation).sshdreloads.sshPublicKey); this only adds authorization.Manager (
create-a-container)Containers.sshAccessTokenHash(argon2, nullable) +MANAGER_URLseeded into default container env vars.Container.sshAllowsUser(),rotateSshAccessToken(),ensureSshAccessToken(currentLxcEnv)(reuses the running token on reconfigure),verifySshAccessToken().CONTAINER_ID/CONTAINER_SSH_TOKENare reserved env keys — stripped from user/admin input, injected bybuildLxcEnvConfig({ sshAccessToken }).GET /api/v1/containers/:id/ssh-access/:username— container-token auth;204/403/400bad username /401bad token;Cache-Control: no-store; denials logged.POST /sites/:siteId/containers/:id/ssh-access/token— owner/admin mints/rotates (used by the part-2 enrollment script).sshAccessEnforced.Base image (
images/base)ssh-access-check— the sshd PAMaccounthook (pam_exec), covering both the key and password/MFA paths. Token is passed to curl via stdin config, never argv. Runs as root, so the token file is root-only (0600); no dedicated service user.AuthorizedKeysCommandis the stocksss_ssh_authorizedkeys(asnobody) — unchanged; authorization is not done on the key path.ssh-access-setup.service— copies the three env vars from PID 1 into/etc/ssh-access/(0700root:root);environment.shfiltersCONTAINER_SSH_TOKENout of/etc/environment.Client
Sharing copy now says what it does;
SshAccessBadgeshows SSH enforced / SSH not enforced.Tests
routers/api/v1/__tests__/ssh-access.api.test.js— owner/collaborator 204, stranger 403, unshare → 403, invalid usernames 400, missing/wrong/rotated/user-API-key tokens 401, unenrolled 401, mint permissions (collaborator 403, stranger 404), reserved keys not overridable, token reuse/rotation.images/base/test-ssh-access-check.sh— 19 checks with a stubbedcurl(allow/deny/cache/outage/401/invalid user/PAM_USER).npx jest: 118 pass; the one failure (mcp-proxy › not mounted when no MCP server is configured) is pre-existing onmain.Deploy notes
MANAGER_URLto the manager URL reachable from containers.Out of scope
Killing live sessions on unshare;
ldapuserspasswordless sudo (sharing = admin-level trust); Proxmox ACLs for collaborators.