Skip to content

config: only require the OIDC client secret for serve - #230

Merged
davekempe merged 1 commit into
sol1:mainfrom
joshsol1:main
Sep 29, 2026
Merged

davekempe merged 1 commit into
sol1:mainfrom
joshsol1:main

Conversation

@joshsol1

Copy link
Copy Markdown
Contributor

Summary

Config::load exited with [oidc] is configured but no client_secret was provided for every subcommand, including add-admin, list-admins and map-group. On a fresh install the secret lives in /opt/rustguac/env, which only systemd's EnvironmentFile= loads, so running the admin CLI from a shell failed until the secret was also copied into config.toml. That defeats the documented reason for the env file.

This keeps the OIDC_CLIENT_SECRET override in Config::load, but moves the presence check into a new Config::validate_oidc_secret() that returns a Result, and calls it only on the serve path. Admin subcommands only touch the SQLite database and never contact the IdP, so they no longer need the secret.

Changes

  • src/config.rs: Config::load no longer exits on a missing OIDC secret; new validate_oidc_secret() carries the same operator-facing message. Four unit tests added.
  • src/main.rs: serve runs the validation before starting; other subcommands skip it.

Testing

Built an unpatched and a patched binary and ran both against a throwaway config with an [oidc] block and no client_secret:

Scenario Before After
add-admin, no secret in env exit 1 with the error admin created
list-admins, no secret in env exit 1 with the error lists admins
serve, no secret exit 1 with the error exit 1 with the error (unchanged)
serve, secret sourced from env file starts starts

Also cargo fmt --check, cargo clippy -- -D warnings and cargo test (310 passed) are clean.

Note: CI doesn't currently run cargo test, so the new unit tests only run locally.

`Config::load` exited with "[oidc] is configured but no client_secret
was provided" for every subcommand, including `add-admin`. The secret
normally lives in /opt/rustguac/env, which only systemd's
EnvironmentFile loads, so running the admin CLI from an interactive
shell on a freshly installed host failed unless the secret was also
copied into config.toml, which defeats the point of the env file.

Keep the OIDC_CLIENT_SECRET override in `Config::load`, but move the
presence check into `Config::validate_oidc_secret` and call it only on
the `serve` path. Admin subcommands touch just the SQLite database and
never contact the IdP. Add unit tests for the validation function.

@davekempe davekempe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Verified rather than taken on trust:

  • cargo fmt --check, cargo clippy --all-targets -- -D warnings clean
  • cargo test = 310 passed (33 + 277), matching your figure
  • Behaviour confirmed against a throwaway config with an [oidc] block and no secret in config or env: add-admin and map-group now exit 0, and serve still refuses with the identical message

The scoping is right. I checked every subcommand (AddAdmin, DeleteAdmin, DeleteUser, DisableAdmin, DisableUser, EnableAdmin, GenerateCert, ImportGuacamole, ListAdmins, ListUsers, MapGroup, RotateKey, SetRole, VaultMigrate) and they only touch SQLite, Vault or the filesystem. serve is genuinely the only path that contacts the IdP, so gating the check there is not under-scoped.

Nice detail that the operator-facing text is preserved byte for byte, with the continuation prefixes carried in the message and [config] ERROR: added by the caller.

This also fixes a papercut we shipped ourselves: map-group landed in v1.10.0 and was unusable from a shell on any OIDC install keeping the secret in /opt/rustguac/env.

Your note about CI is correct and, if anything, understated: ci.yml runs fmt, clippy, build and audit but has no cargo test step at all, so none of the 310 tests have ever run there. Adding it now as a separate change.

@davekempe
davekempe merged commit 8db1d7e into sol1:main Sep 29, 2026
davekempe added a commit that referenced this pull request Sep 29, 2026
CI ran fmt, clippy, build and audit but never `cargo test`, so none of the
310 tests had ever run on a pull request. That included the regression
tests written specifically to guard past incidents: the recording rotation
that deleted in-progress files, and the credential-variable folder scan
that hid subfolder-granted entries from operators. Both could have been
reintroduced without CI noticing.

Runs the debug profile rather than --release on purpose. debug_assertions
and integer overflow checks are compiled in there and switched off in a
release build, and the wire-protocol parser is where an overflow would
otherwise pass silently.

Noted by @joshsol1 on #230.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants