fix: stop the WOPI keys from leaking and let the environment configure them - #546
Open
DeepDiver1975 wants to merge 3 commits into
Open
fix: stop the WOPI keys from leaking and let the environment configure them#546DeepDiver1975 wants to merge 3 commits into
DeepDiver1975 wants to merge 3 commits into
Conversation
…utput The xtrace hardening in #543 missed three secret-bearing variables that still tested their value with the plain "[[ -z ${VAR} ]]" form, so DEBUG=true printed them: OWNCLOUD_WOPI_TOKEN_KEY and OWNCLOUD_METRICS_SHARED_SECRET (v24.04) and OWNCLOUD_PROXY_USERPWD (both variants, a user:password pair). Test them as "${VAR:+x}" like the already covered secrets, so the trace shows "[[ -z x ]]" instead of the value. Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
The wopi app reads wopi.token.key and wopi.proxy.key through getSystemValue(), so both can come from the generated config file and never have to pass through "occ config:system:set" — which echoes the value back to stdout and is recorded verbatim by admin_audit. Add OWNCLOUD_WOPI_PROXY_KEY, and backport the whole OWNCLOUD_WOPI_* family to v22.04, which had no WOPI support at all even though ENVIRONMENT.md documents the variables without a variant caveat. Admins on 10.16.x therefore had to configure the keys from a custom hook. Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
Add OWNCLOUD_WOPI_PROXY_KEY to ENVIRONMENT.md and a README section for the hook directories, which were undocumented in this repo. It states that a documented environment variable is preferable to a hook, and shows how to keep a secret out of the log when a hook has to call occ anyway. Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Follow-up to #543, from the same 10.16.4 report. #543 got the eight secrets the
base image itself handles down to zero log hits, but the reporter's
wopi.token.keyand their ONLYOFFICE
jwt_secretstill showed up. Root cause: those two never wentthrough the image's own scripts at all — they were set from a custom hook, because
v22.04has no WOPI environment variables even thoughENVIRONMENT.mddocumentsthem. Three secret env vars were also missed by #543.
Scoped to
v22.04andv24.04;v20.04is dormant and untouched.1. The three variables #543 missed (#544)
OWNCLOUD_WOPI_TOKEN_KEYandOWNCLOUD_METRICS_SHARED_SECRET(v24.04) andOWNCLOUD_PROXY_USERPWD(both variants) still used the plain[[ -z "${VAR}" ]]test, soDEBUG=trueprinted them. They now use"${VAR:+x}"like the already covered secrets.
2. WOPI keys from the environment (#545)
The
wopiapp reads bothwopi.token.keyandwopi.proxy.keythroughgetSystemValue(), so they can come from the generated config file and never haveto touch
occ. AddedOWNCLOUD_WOPI_PROXY_KEY, which was missing in both variants,and backported the whole
OWNCLOUD_WOPI_*family tov22.04— the variant behind10.16.4,10.16,10andlatest, which implemented none of it.That is the actual fix for the reported disclosure: with the variables in place no
hook is needed, and the generated config file is never echoed or traced.
3. Docs
The hook directories were undocumented in this repo.
README.mdnow describesthem, says to prefer a documented environment variable over a hook, and shows how
to keep a secret out of the log when a hook has to call
occanyway:{ set +x; } 2>/dev/null # no shell trace occ config:app:set onlyoffice jwt_secret --value "${SECRET}" -q # no echo [[ "${DEBUG}" == "true" ]] && set -xThis is what admins of apps that are not bundled in the image (onlyoffice,
richdocuments) need, since they have no environment variable to use. It is
documented together with its limitation:
admin_auditstill records the fullargument list. Both remaining leaks are upstream and now filed —
owncloud/core#41779 (the command echoes the value back) and
owncloud/admin_audit#461 (the audit entry contains the argv).
Verification
Real images, not the overlay in isolation:
owncloud/basebuilt from this branchfor both variants, with
owncloud/server:10.16.4/11.0.0layered on top, againstMySQL 8 + Redis,
DEBUG=true, and eight secrets in the environment.status.phpinstalled:true,PROPFIND207status.phpinstalled:true,PROPFIND207docker logsowncloud.log[[ -z x ]][[ -z x ]]occ config:system:getreturns all six WOPI values plusmetrics_shared_secretand
proxyuserpwdfrom the environment on both variants.The keys are really consumed, not just stored —
occ wopi:get-tokenon 10.16.4returns two JWTs, and verifying their HMAC signatures shows which key signed what:
OWNCLOUD_WOPI_TOKEN_KEYOWNCLOUD_WOPI_PROXY_KEYwopi_srcSetting both variables to the same value makes the app raise
System configuration <wopi.proxy.key> and <wopi.token.key> are identical.(
TokenService.php:163), which confirms both arrive from the environment.The documented hook pattern was checked against the reporter's original hook,
running side by side in one container:
docker logsoccecho)set +xand-qand the residual limitation was confirmed rather than assumed — with the hardened
hook,
owncloud.logstill holdscommand_executed … config:app:set onlyoffice jwt_secret --value … -q.shellcheckclean on both hooks,php -lclean on both templates.Follow-up
owncloud-docker/serverneeds itsFROM owncloud/base:...digests bumped once thisis published (Renovate normally does this).
🤖 Generated with Claude Code