Conversation
439e185 to
d38890a
Compare
|
@maximthomas, rebased onto the current The merge of #1091 made this PR conflict in |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: The fix is at the source of #1093, and the new CI step pins it.
- Both LDIF loops bind with
-j "$PASSWORD_FILE"and quote-f "$file"(setup.sh:94,:114) instead of the unquoted-w $ROOT_PASSWORD -f $file, so the password reachesldapmodifyas a single value. set-password-policy-propnow names-h localhost -p $ADMIN_PORT(setup.sh:103-104), which fixes the silent19 Constraint Violationon pre-encoded passwords behind a non-default admin port.Docker test bootstrap LDIFsturns both bugs red:ROOT_PASSWORD='p@ss b*'makes thebootstrapProbegrep fail, andADMIN_PORT=5444makes the{SSHA}bind ofuid=prefail.
issue (non-blocking): A container killed mid-bootstrap leaves the root password in clear text in /tmp.
opendj-packages/opendj-docker/bootstrap/setup.sh:28-30
The password file sits in the container's writable layer, and only setup.sh's EXIT trap removes it. The trap does not run when the container is stopped or killed during the bootstrap (setup, dsconfig, import-ldif take tens of seconds): run.sh is PID 1 with no trap, so SIGTERM is dropped and the SIGKILL at the end of the grace period takes setup.sh down with it. After that, run.sh:44 goes straight to start-ds once data/config exists. Nothing sweeps /tmp, so the file (mode 0600) stays for the container's life and shows in docker diff, docker cp and docker export. The description says this is done "the same way #1094 does it", but #1094 has since moved to mktemp -p /dev/shm … || mktemp. The review there also asks for a fixed prefix and a sweep at start, because /dev/shm is pod-wide on Kubernetes. The password is also in the container's Config.Env, which limits the extra exposure.
# setup.sh
PASSWORD_FILE=$(mktemp -p /dev/shm opendj-setup.XXXXXXXXXX 2>/dev/null || mktemp /tmp/opendj-setup.XXXXXXXXXX) || exit 1
# run.sh, before the data/config check at :44
rm -f /dev/shm/opendj-setup.* /tmp/opendj-setup.*Or: whatever #1094 settles for replicate.sh, applied to both scripts.
suggestion (non-blocking): Drop --rm from the new step's docker run.
.github/workflows/build.yml:574, :762
The ERR trap's docker logs test_bootstrap only helps while the container still exists. Suppose setup rejects --rootUserPasswordFile, or setup.sh fails before data/config exists. Then run.sh:105 execs start-ds --nodetach, which exits, and --rm removes the container. The step goes red after the 5-minute timeout, and the trap prints only "No such container". That is the one road this PR changes. The pattern is copied from the steps above; runners are ephemeral, so the stopped container docker kill leaves behind costs nothing.
docker run -it -d --memory="512m" -e ADD_BASE_ENTRY="--addBaseEntry" -e ROOT_PASSWORD="$ROOT_PASSWORD" -e ADMIN_PORT=5444 ... --name=test_bootstrap ...suggestion (non-blocking): The "password not in the container log" check cannot fail for any change this PR makes.
.github/workflows/build.yml:579, :767
Neither version of setup.sh or run.sh prints the password. setup runs --no-prompt, and dsconfig has no --displayCommand. So reverting any --rootUserPasswordFile / --bindPasswordFile / -j "$PASSWORD_FILE" to the quoted --bindPassword "$ROOT_PASSWORD" keeps the whole step green, and so does deleting the EXIT trap at setup.sh:29. The claim that the password is off the command line rests on the local docker top sample. The HEALTHCHECK needs $BOOTSTRAP_COMPLETE, which run.sh:93 touches only after setup.sh has exited. So once the container is healthy, the trap has run and can be pinned:
docker exec test_bootstrap sh -c '! grep -rqF -e "$ROOT_PASSWORD" /tmp'
docker exec test_bootstrap sh -c '! grep -nE -e "--(bind|rootUser)Password[ =]|-w " /opt/opendj/bootstrap/setup.sh'Pin: the first line goes red when the trap is deleted. The second is a static guard against the argv revert, which has no cheap runtime observable.
suggestion (non-blocking): The schema LDIF name has no space, so the -f "$file" quoting in the schema loop is unpinned.
.github/workflows/build.yml:568, :756, opendj-packages/opendj-docker/bootstrap/setup.sh:94
With 10-schema.ldif, reverting setup.sh:94 to -f $file passes the same single word, and grep bootstrapProbe stays green. Only the data loop is pinned, through 10 probe.ldif.
printf "dn: cn=schema\n..." > "$BOOTSTRAP_DIR/schema/10 schema.ldif"Pin: with the space, the unquoted mutant splits the path, bootstrapProbe never loads, and the cn=schema grep goes red.
|
@maximthomas, thank you. All four points are taken in 99e627b. Password file left by a killed bootstrap. Confirmed: The removal is pinned: once the container is healthy, the step creates
Log check cannot fail. Right. Two checks are added, with two changes from the snippet:
The log check stays as a guard against a future Schema LDIF name. It is Local run of the step, taken from
|
99e627b to
1cac805
Compare
|
@maximthomas, rebased onto the current The merge of #1094 made both commits conflict:
The commits are unchanged apart from that context. The "Out of scope" notes in the description about #1094 and the conflicts are updated. |
maximthomas
left a comment
maximthomas
left a comment
There was a problem hiding this comment.
praise: Both round-1 points are fixed at the source, and the new checks turn red on the mutants they target.
- The password file is on the tmpfs of
/dev/shm(setup.sh:33-34), andrun.sh:39sweeps what a killed bootstrap leaves. Both steps drop--rm(build.yml:571,:772), so the ERR trap'sdocker logsstill has a container to read. - The command-line check accepts only exit status
1fromgrep(build.yml:586-587), so an unreadablesetup.shfails the step instead of passing it. The leftover grep (:582) goes red when theEXITtrap is removed, as the PR's table shows. 10 schema.ldifand10 probe.ldifpin the quoted-f "$file"in both LDIF loops.
issue (non-blocking): A ROOT_PASSWORD with a CR or LF is cut at the line break for every tool that reads the password file, while the HEALTHCHECK and replicate.sh still get the whole value.
opendj-packages/opendj-docker/bootstrap/setup.sh:36, :61, opendj-packages/opendj-docker/Dockerfile:82, opendj-packages/opendj-docker/bootstrap/replicate.sh:39-57
FileBasedArgument reads the file with BufferedReader.readLine(), and readLine() ends the line at \r as well as \n. A probe on JDK 26 gives "abc\r\n" -> abc and "ab\rc\n" -> ab. Take -e ROOT_PASSWORD="$(cat crlf-file)" (value abc\r) or a Kubernetes secret with a trailing newline (value abc\n). The root password becomes abc, but the HEALTHCHECK's --bindPassword "${ROOT_PASSWORD:-password}" and replicate.sh bind with the whole value, so the container never turns healthy. At the base every tool got the value on argv, and the same container turned healthy. Failing loudly beats the silent mismatch. setup.sh runs under sh (run.sh:71), so this is POSIX:
cr=$(printf '\r')
nl='
'
case $ROOT_PASSWORD in
*"$cr"*|*"$nl"*)
echo "ROOT_PASSWORD must not contain a line break: the tools read only the first line of the password file" >&2
exit 1 ;;
esacissue (non-blocking): run.sh removes every opendj-setup-password.* in /dev/shm, including the live file of another container of this image that shares /dev/shm and is still bootstrapping.
opendj-packages/opendj-docker/run.sh:37-39, opendj-packages/opendj-docker/bootstrap/setup.sh:30-34
The comment's "nothing is bootstrapping yet at this point" is true only inside one container, and setup.sh:30-32 itself says that /dev/shm is shared by the containers of a pod. Take two OpenDJ containers in one pod (distinct ports), or --ipc=host, or --ipc=container:X. Both run as uid 1001, so the 1777 sticky bit does not help. If B starts while A's setup runs, B's rm deletes A's file. A's next dsconfig create-backend --bindPasswordFile then fails with ERR_FILEARG_NO_SUCH_FILE, and || exit 1 ends A's bootstrap. After a restart, A comes up healthy without its userRoot backend. Plain Docker and the in-repo OpenShift template (one container per pod) do not reach this. Key the file to the container: ADMIN_PORT is an image ENV, and it must differ within a pod, whose containers share one network namespace. --ipc=host across network namespaces with equal ports stays unkeyed, so the comment should say that:
# setup.sh:33-34
PASSWORD_FILE=$(mktemp -p /dev/shm "opendj-setup-password.$ADMIN_PORT.XXXXXX" 2>/dev/null \
|| mktemp "/tmp/opendj-setup-password.$ADMIN_PORT.XXXXXX") || exit 1
# run.sh:37-39
# A container killed during its bootstrap leaves the password file of setup.sh behind, as the
# EXIT trap that removes it does not run then. /dev/shm may be shared with the other containers
# of a pod, so only the file of this container is removed: the containers of a pod share one
# network namespace, hence differ in ADMIN_PORT
rm -f /dev/shm/opendj-setup-password."$ADMIN_PORT".* /tmp/opendj-setup-password."$ADMIN_PORT".*The restart check then plants mktemp /tmp/opendj-setup-password.5444.XXXXXX (build.yml:589, :790).
issue (non-blocking): The bootstrap/config/schema copy loop still splits a file name that contains a space: $(basename -- $file) is unquoted.
opendj-packages/opendj-docker/bootstrap/setup.sh:46-49
This is the same whitespace class the PR fixes in the two ldapmodify loops, on a line the PR does not touch (it is the same at the base). For bootstrap/config/schema/10 foo.ldif, basename -- /opt/.../10 foo.ldif takes foo.ldif as a suffix and prints 10. cp then writes template/config/schema/10, and the schema loader takes only *.ldif, so the schema is silently not loaded while the container turns healthy.
target_file="/opt/opendj/template/config/schema/$(basename -- "$file")"suggestion (non-blocking): BASE_TEMPLATE is removed only when the bootstrap succeeds, so a failed makeldif or import-ldif leaves it in /tmp.
opendj-packages/opendj-docker/bootstrap/setup.sh:76-88, :35
The || exit 1 at :79, :81 and :86 skips the rm at :88. The EXIT trap removes only the password file, and run.sh:39 matches only opendj-setup-password.*. The file stays in the writable layer. It holds no secret, but with SAMPLE_DATA it holds the generated entries. This was already so at the base; the PR fixes the other leak, the double mktemp.
trap 'rm -f "$PASSWORD_FILE" ${BASE_TEMPLATE:+"$BASE_TEMPLATE"}' EXITsuggestion (non-blocking): No check pins that the password file is created in /dev/shm: a mutant that always uses /tmp keeps both docker jobs green.
opendj-packages/opendj-docker/bootstrap/setup.sh:33-34, .github/workflows/build.yml:582, :589, :783, :790
Replace :33-34 with PASSWORD_FILE=$(mktemp /tmp/opendj-setup-password.XXXXXX). The file is then back in the writable layer, which is what the commit title says the change prevents. The leftover grep still passes, because it runs after the healthy wait, when the EXIT trap has already removed the file from either directory. The restart check still passes, because it plants its own /tmp file. The file lives for the whole bootstrap (tens of seconds), so polling it from a second container is deterministic. Run this before the step's own container starts or after it is killed:
docker run -d --memory="512m" --name test_bootstrap_shm localhost:5000/${GITHUB_REPOSITORY,,}:${{ env.release_version }}
timeout 2m bash -c 'until docker exec test_bootstrap_shm sh -c "ls /dev/shm/opendj-setup-password.*" >/dev/null 2>&1; do sleep 1; done'
docker exec test_bootstrap_shm sh -c '! ls /tmp/opendj-setup-password.* 2>/dev/null'
docker kill test_bootstrap_shmPin: the always-/tmp mutant never creates the /dev/shm file, so the loop times out and the step goes red. On build-docker-alpine it also shows that busybox mktemp -p works.
suggestion (non-blocking): The /dev/shm half of run.sh's leftover removal has no pin: the restart check plants its file only under /tmp.
opendj-packages/opendj-docker/run.sh:39, .github/workflows/build.yml:589-592, :790-793
If the /dev/shm glob is dropped from run.sh:39, both jobs stay green, yet that glob is the Kubernetes road the commit message gives as the reason for the removal. A /dev/shm plant in the current step would pass either way, because a plain container gets a fresh /dev/shm on every start. The fixture needs a /dev/shm that outlives the restart, which a holder container gives:
docker run -d --ipc=shareable --name shm_holder --entrypoint sleep localhost:5000/${GITHUB_REPOSITORY,,}:${{ env.release_version }} 600
# the step's docker run gets --ipc=container:shm_holder; then, next to the /tmp plant:
shm_left=$(docker exec test_bootstrap mktemp -p /dev/shm opendj-setup-password.XXXXXX)
docker restart test_bootstrap
timeout 5m bash -c 'until docker inspect --format="{{json .State.Health.Status}}" test_bootstrap | grep -q \"healthy\"; do sleep 10; done'
docker exec test_bootstrap test ! -e "$shm_left" || { echo "::error::$shm_left is still there after a restart"; false; }
docker kill test_bootstrap shm_holderPin: the dropped-glob mutant leaves $shm_left in the holder's /dev/shm, and the step goes red. With the $ADMIN_PORT key above, plant opendj-setup-password.5444.XXXXXX.
…strap tools in a file setup.sh passed ROOT_PASSWORD unquoted to ldapmodify, so a password with whitespace or a glob character failed the bind and the schema and data LDIFs under /opt/opendj/bootstrap were skipped. Every tool of the script now reads the password from a file of its own, which also keeps it off the command line of the processes the bootstrap runs. The dsconfig call that allows pre-encoded passwords named no port and reached 4444 whatever ADMIN_PORT was, so the entries of the data LDIFs carrying one were refused while the container reported itself healthy. It now goes to ADMIN_PORT. The base entry template is no longer created twice, which left an empty file in /tmp. CI runs the image with schema and data LDIFs, a password with a space and a glob character, and a non-default admin port. Fixes OpenIdentityPlatform#1093
…ff the writable layer, and pin it in CI A setup.sh killed before its EXIT trap left the password file in /tmp, in the writable layer of the container. It now goes to the tmpfs of /dev/shm where there is one, and run.sh removes what a killed bootstrap left behind, which on Kubernetes also covers /dev/shm, shared by the pod. The bootstrap step no longer removes its container on exit, so its log stays available to the ERR trap. It names the schema LDIF with a space, and checks that no file under /tmp or /dev/shm holds the root password, that setup.sh passes no password on a command line, and that a planted password file is gone after a restart.
… and remove only the password files of this container The tools read only the first line of a password file, and a CR ends it as an LF does, so a ROOT_PASSWORD with a line break was set up cut short. setup.sh now refuses it. /dev/shm is shared by the containers of a Kubernetes pod, so run.sh removed the live password file of another container still bootstrapping. The password files of setup.sh and replicate.sh now carry ADMIN_PORT in their name, which the containers of a pod cannot share, and run.sh removes only those of its own port there. The copy of bootstrap/config/schema quotes the file name, and the EXIT trap of setup.sh also removes the base entry template a failed import leaves in /tmp. The bootstrap step checks that both a CR and an LF in the root password are refused, that the password file is on /dev/shm while the bootstrap runs, and that a schema file of bootstrap/config/schema with a space in its name is loaded. The container shares /dev/shm with a holder container, as in a pod, so the removal after a restart is checked there too, together with a file of another admin port that must be kept. The replication step checks the latter for replicate.sh.
1cac805 to
d2e63ac
Compare
|
@maximthomas, all six points are taken in d2e63ac, and the branch is rebased onto the current Line break in Removal in
No pin for No pin for the Local run of the steps, taken from
|
2 participants
Fixes #1093
Problem
bootstrap/setup.shloads the optional LDIFs under/opt/opendj/bootstrap/schema/and/opt/opendj/bootstrap/data/withldapmodify -w $ROOT_PASSWORDunquoted, so a password with whitespace, or with*,?or[that matches files in/opt/opendj, is split or expanded and the bind fails (49 Invalid Credentials).setup,dsconfigandimport-ldifquote the password, but all of them still take it on the command line, readable from/proc/<pid>/cmdline(andpson the Docker host) while they run.What the skipped load leads to depends on the image. The released
5.1.2image reports itself healthy without the data, as the issue shows. Onmaster, since #898, the exit status ofsetup.shis that of its last command, so a failed bind on the last data LDIF keeps the container from ever turning healthy, while a failed bind on a schema LDIF, or on an earlier data LDIF, is skipped silently.Found while reproducing: the
dsconfig set-password-policy-propcall that allows pre-encoded passwords for the data LDIFs names no host or port, so it goes to4444whateverADMIN_PORTthe server listens on. With a non-defaultADMIN_PORTit fails (Unable to connect to the server at "localhost" on port 4444, not checked), and every entry carrying a pre-encoded password is refused (19 Constraint Violation) while the container reports itself healthy.Change
setup.sh:mktempfile (mode0600, removed by anEXITtrap). Every tool reads it from there:setup --rootUserPasswordFile,dsconfig/import-ldif --bindPasswordFile,ldapmodify -j. The tools read the first line of the file as is, so spaces and glob characters survive.$fileis quoted, in bothldapmodifyloops and in the copy ofbootstrap/config/schema.ROOT_PASSWORDwith a line break would be set up cut short without a word.setup.shrefuses it instead, and the README says so.opendj-setup-password.$ADMIN_PORT.XXXXXXon the tmpfs of/dev/shmwhere there is one, and in/tmpotherwise, so that a container killed during the bootstrap, before theEXITtrap runs, does not keep the password in its writable layer.dsconfig set-password-policy-propgets-h localhost -p $ADMIN_PORT, likecreate-backendabove it.mktempwhenSAMPLE_DATAis unset, which left an empty file in/tmp; it is created once, and theEXITtrap also removes it when an import fails.run.sh(Report the OpenDJ container healthy only once its bootstrap has succeeded #898) is unchanged.run.shremoves/dev/shm/opendj-setup-password."$ADMIN_PORT".*and/tmp/opendj-setup-password.*before anything else. That is what a killed bootstrap leaves behind: in/tmpof a restarted Docker container, and in/dev/shmon Kubernetes, where it is shared by the pod and outlives a restart of the container. The containers of a pod share one network namespace, hence differ inADMIN_PORT, so the removal leaves alone the file of another container that is still bootstrapping. Containers in distinct network namespaces that share/dev/shmthrough--ipc=hoston the sameADMIN_PORTare not told apart.replicate.sh([#1084] Keep the root password out of the log and off the command line when a Docker container joins replication #1094) had the same flaw in its removal: its password file is nowopendj-replicate.$ADMIN_PORT.XXXXXX, andrun.shremoves only/dev/shm/opendj-replicate."$ADMIN_PORT".*.CI: no step mounted bootstrap LDIFs until now.
Docker test bootstrap LDIFsis added to bothbuild-dockerandbuild-docker-alpine. It first starts the image twice, withab\ncdand withab\rcdas the root password, and waits for the refusal in each log. Then it starts the image withROOT_PASSWORD='p@ss b*',ADMIN_PORT=5444, a schema file ofbootstrap/config/schema, a schema LDIF and a data LDIF, all with a space in their names, the data LDIF holding a user with an{SSHA}password. The container shares/dev/shmwith a holder container, as the containers of a pod do. It checks that/dev/shm, with the admin port in its name, while the bootstrap runs,healthy,cn=schema,/tmpor/dev/shmholds the root password once the bootstrap is over, apart from the perf data of the HEALTHCHECK's ownldapsearchJVM (Docker image: the HEALTHCHECK binds with the initial ROOT_PASSWORD, so the container turns unhealthy once the root password is changed #1092),setup.shin the image passes no password on a command line (-w,--bindPassword,--rootUserPassword, also in the--option=valueform);grepmust exit with1, so a file it cannot read fails the step,/tmpand in/dev/shmare gone after adocker restart, while one planted in/dev/shmfor another admin port is kept.Docker test replication([#1084] Keep the root password out of the log and off the command line when a Docker container joins replication #1094) plants its leftover password file under the admin port of the replica, and a second one under another port, which has to be kept.The container is started without
--rm, so that the ERR trap still prints its log when the container exits early.Verification
The step was run locally as a script taken from
build.yml, against images built from release 5.1.2 with the scripts swapped in:masterbind 49on both LDIFs, never healthymasterbind 49, plusUnable to connect … port 4444Docker test replicationEXITtrap/dev/shm/opendj-setup-password.*-f $filein the schema loop…/schema/10not found,bootstrapProbenot incn=schemarun.sh/tmp/opendj-setup-password.*is still there after the restarttest_bootstrap_lf"$cr"test_bootstrap_cr/tmp/dev/shm/opendj-setup-password.5444.*never shows up/dev/shmremoval inrun.shwithout the portrun.sh removed /dev/shm/opendj-setup-password.4444.…, the password file of another containerrun.shwithout its/dev/shmhalf/dev/shm/opendj-setup-password.5444.… is still there after a restartbasename -- $file99 config.ldifis copied as…/schema/99, andbootstrapConfigProbeis not incn=schemaopendj-replicate.*removal without the port (replication step)run.sh of test_replica removed the password file of another containerThe same check was also run on
setup.shwith-w "$ROOT_PASSWORD",--bindPassword="$ROOT_PASSWORD"and--rootUserPassword "$ROOT_PASSWORD"put back, and it finds each of them.The command-line check finds 0 lines in
setup.shof this branch and 7 in that ofmaster, with GNUgrep, with the busyboxgrepof the Alpine image and with that of the Debian one. Withdocker topsampled during the bootstrap, thesetupprocess ofmastershows--rootUserPassword p@ss b*; the one of this branch shows only the file path.Out of scope
replicate.shis Docker image: replicate.sh prints its environment, ROOT_PASSWORD included, to the container log #1084, fixed by [#1084] Keep the root password out of the log and off the command line when a Docker container joins replication #1094 (merged); this PR changes only the name of its password file, as above.HEALTHCHECKstill binds withROOT_PASSWORDon its command line: Docker image: the HEALTHCHECK binds with the initial ROOT_PASSWORD, so the container turns unhealthy once the root password is changed #1092.build.ymltheirDocker test arbitrary uidandDocker test replicationsteps come right beforeDocker test bootstrap LDIFs, and inrun.shthe removal of the password file ofsetup.shcomes right after the one [#1084] Keep the root password out of the log and off the command line when a Docker container joins replication #1094 added forreplicate.sh. Docker image: the HEALTHCHECK binds with the initial ROOT_PASSWORD, so the container turns unhealthy once the root password is changed #1092 and [#1087] Copy the secret volume on every start of the Docker image, and run the server as PID 1 after the bootstrap #1100 add steps at the same place inbuild.yml, so whichever is merged later gets a trivial conflict there.