fix(docker): make auth bootstrap safe for mounted and upgraded configs by Adarsh-Me · Pull Request #3192 · apache/hugegraph

Adarsh-Me · 2026-09-03T15:50:23Z

What is changing

Closes #3133 (the parts still open on master 3681148, since #3119 landed the rest).

Properties rewriting now implements the Java grammar. docker-entrypoint.sh previously rewrote rest-server.properties / hugegraph.properties with grep/sed, which disagrees with HugeConfig on mounted or upgraded configs: backslash-escaped keys, :/whitespace separators, line continuations, and duplicate definitions are all parsed differently. The property logic moves to a new props.awk loaded by the entrypoint, which implements the java.util.Properties line grammar (comments, both separators, continuations, backslash escapes, first-definition-wins duplicates) and rewrites the first definition in place while keeping every untouched line byte-for-byte.

PASSWORD no longer appears in ps output. The old sed rewrite interpolated the encoded value into sed's command line, so when a key already existed (mounted or persisted config) the password was visible in ps. Values now travel through an environment variable into awk, never through argv.

enable-auth.sh appends are per-file guarded. The old script appended authentication definitions whenever conf-bak/ was absent. On a config it did not write, that created duplicate definitions that the properties parser (first definition wins) and snakeyaml (last definition wins) resolved in opposite directions — Gremlin and REST could land on different authenticators with no error from either. Now each append runs only when its file lacks the definition (or still has it commented out), re-runs are idempotent, custom gremlin.graph factories are preserved, and the authenticator class can be overridden via AUTHENTICATOR_CLASS.

The entrypoint aligns both sides before enabling auth. If the yaml declares an authenticator but the properties file does not (or vice versa), the entrypoint propagates it to the other side instead of letting the default StandardAuthenticator split the pair. When both sides name genuinely different authenticators, it logs a WARN and leaves both untouched instead of silently splitting them.

Implementation notes

I chose a shell/awk implementation of the properties grammar over the issue's proposed Java ConfigTool CLI: it delivers the same parser-agreement contract with a much smaller footprint and no new build artifact. Happy to rework toward the ConfigTool if reviewers prefer that direction.
props.awk ships in both server images (Dockerfile COPY) and in the test sandbox; CI already runs the unit suite via docker-build-ci.yml.

How was this tested

docker/test/test-docker-entrypoint.sh extended with cases for: escaped-key definitions rewritten in place, continuation lines consumed with the key they belong to, get-mode separator/continuation/duplicate semantics, and appends when the key only exists commented out. All pass.
docker-entrypoint-test.sh full harness passes end-to-end (secret round-trips incl. backslash/space/trailing-space secrets, enable-auth call counting unchanged).
enable-auth.sh manually exercised against: fresh default config, idempotent re-run, mounted config with custom authenticator (no duplicates), and custom gremlin.graph factory (preserved).

Code Review Handbook

props.awk is the core: block model (comment lines and logical entries), first-definition-wins, raw-value round-trip (get returns the on-disk escaped form so feeding it back into set is byte-exact).
The value of get is intentionally not unescaped: the entrypoint re-writes secrets it just read, and unescape-then-re-encode would double-escape backslashes (caught by the complex-secret round-trip in docker-entrypoint-test.sh).
The escaped-key unit case is the reported auth\.admin_pa scenario: the old grep could not match it, so the append created a duplicate and HugeConfig silently kept pa.

Visual summary

The entrypoint's grep/sed property rewriting disagrees with HugeConfig on mounted or upgraded configs: escaped keys, ':'/whitespace separators, line continuations, and duplicate definitions are all read differently, so a mounted config could end up with two logical definitions of one key. Property reading/writing now goes through props.awk, which implements the java.util.Properties grammar (comments, both separators, continuations, backslash escapes, first-definition-wins duplicates) and keeps every untouched line byte-for-byte. Values travel through environment variables instead of command arguments, so a PASSWORD no longer shows up in 'ps' output when a key is rewritten in place. enable-auth.sh appended authentication definitions whenever conf-bak/ was absent, which on a mounted config created duplicate definitions that the properties parser (first definition wins) and the yaml parser (last definition wins) resolved in opposite directions -- Gremlin and REST could land on different authenticators with no error from either. Its appends are now guarded per file, only an absent or still commented-out definition triggers an append, re-runs are idempotent, and the authenticator class is overridable through AUTHENTICATOR_CLASS. The entrypoint aligns both sides before calling it: it copies a yaml authenticator into rest-server.properties, or exports the REST one for the yaml append, and warns without touching anything when the two name genuinely different authenticators. The unit test suite covers escaped keys, continuations, get-mode semantics, and comment-guarded appends; the entrypoint harness now ships props.awk into its sandbox, and both server Dockerfiles COPY it next to the entrypoint. Fixes apache#3133

bitflicker64

Blocking: yes. Summary: props.awk is careful work and both shell suites this PR touches pass under the image's own mawk, but the new alignment layer does not hold on the mounted configs the PR targets. The yaml authenticator is copied into rest-server.properties without unquoting, a flow-style authentication: block reads as absent so REST silently falls back to the default, enable-auth.sh's guards accept only the key= spelling so duplicates are still appended, and the narrowed gremlin.graph guard no longer converts a CRLF config the old sed did convert. Two doc fixes as well. Evidence: measured at 698b0c3 in ubuntu:22.04 (GNU grep 3.7, GNU sed, mawk 1.3.4), the same toolchain eclipse-temurin:11-jre-jammy ships; test/test-docker-entrypoint.sh and docker-entrypoint-test.sh both exit 0 there; each finding below quotes the config the run produced. All seven workflow runs on this head are action_required and the combined status is pending with zero statuses, so there is no CI evidence for the image builds.

imbajin

Blocking: yes. Summary: The new properties parser still mishandles valid indented keys, so mounted authentication values can remain stale after an environment override. Evidence: reproduced at the exact head with the PR helper; existing current-head comments cover other findings and are not duplicated.

codecov · 2026-09-04T15:33:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 37.92%. Comparing base (3681148) to head (bf2718f).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3192      +/-   ##
============================================
+ Coverage     37.77%   37.92%   +0.14%     
- Complexity     6560     6606      +46     
============================================
  Files           800      800              
  Lines         68960    69029      +69     
  Branches       9166     9184      +18     
============================================
+ Hits          26052    26176     +124     
+ Misses        39841    39777      -64     
- Partials       3067     3076       +9

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Review follow-ups on the props.awk bootstrap: the yaml authenticator scalar now goes through a snakeyaml-shaped cleanup (inline comments, quotes and padding stripped) instead of only cutting at the first comma or colon; a flow mapping on the authentication line itself is read, and an authentication block without a readable authenticator takes the WARN branch instead of the both-empty default. props.awk strips leading whitespace before the key the way java.util.Properties does, so an indented key is rewritten in place rather than duplicated. enable-auth.sh's append guards now accept the ':', bare-whitespace and backslash-escaped spellings with [[:blank:]] classes (the '[ \t]' bracket matched space, backslash and the letter t), and the gremlin.graph flip embeds the carriage return as a byte because GNU grep reads \r in a pattern as the letter r, which made the anchored guard drop mounted CRLF configs. Test docs name the environment variables and the function count they rely on, and new regression tests cover indented keys, yaml scalar cleanup, flow mappings and the block-without-authenticator WARN.

bitflicker64

Blocking: no. Summary: The fresh-default path works and is idempotent at this head (one auth.authenticator, one authentication: block, gremlin.graph flipped to HugeFactoryAuthProxy, stable across three re-runs), and the fixes for the earlier review round all check out. Four gaps remain between what the new code promises for mounted or upgraded configs and what it does, three of them around CRLF and separator spellings that java.util.Properties accepts. Evidence: entrypoint helpers extracted the way test/test-docker-entrypoint.sh does, run against verbatim head copies of props.awk and bin/enable-auth.sh with GNU sed; props.awk output compared byte for byte against java.util.Properties.load on the same files; the gremlin.graph guard and its sed run against eight legal spellings. Not covered: no Docker daemon on this host, so the images were not built and the runtime mawk path was not exercised. props.awk uses only POSIX awk features and awk is already a dependency of the shipped bin/*.sh, but CI's docker-build-ci.yml run of the suite remains the authority there.

Address review 5185689081 on the auth bootstrap alignment: - props.awk: strip one trailing CR while assembling logical lines so CRLF configs parse like java.util.Properties, without touching the RAW bytes replayed on rewrite; add get-decoded mode. - props.awk: die when getline fails and rewrite atomically through a sibling temp file renamed over the original. - enable-auth.sh: widen the gremlin.graph guard and flip together for colon, equals, bare-whitespace, leading-blank and escaped-dot spellings with optional CR, still skipping proxied/commented lines. - docker-entrypoint.sh: compare the unescaped authenticator with the yaml scalar and write the yaml side through the encoding setter. Add CRLF plus escaped-authenticator regression cases to test-docker-entrypoint.sh.

Adarsh-Me · 2026-09-12T09:08:40Z

All 4 inline threads of review 5185689081 addressed in 5f50511 (branch fix-entrypoint-auth-bootstrap): (A) CRLF stripped while assembling logical lines, RAW replay untouched + regression test; (B) die on getline -1 plus atomic sibling-temp rewrite with quoted rename; (C) gremlin.graph guard and sed widened together (colon/equals/bare-whitespace, leading blank, escaped dot, optional CR), proxied/commented still skipped, CR preserved; (D) decoded authenticator comparison via new get-decoded mode with encoding setter on write. Verification: bash test-docker-entrypoint.sh passes (exit 0), bash -n clean on all three shell files, guard/sed exercised against 8 spellings + CRLF/proxied/commented cases. shellcheck not installed on this host, so that step was skipped. No Docker daemon here, so image build/mawk paths remain for CI (docker-build-ci.yml).

bitflicker64

Blocking: no. Summary: The earlier review rounds are addressed at this head: CRLF parsing, the unreadable-file guard, the widened gremlin.graph guard and sed, and the decoded authenticator comparison all check out, and the unit suite passes. One new problem comes from the atomic rewrite added in this round. props_set now swaps in a new file with mv, so the rewritten config loses its original permissions and any symlink, and a config bind-mounted as a single file can no longer be updated at all. Evidence: bash hugegraph-server/hugegraph-dist/docker/test/test-docker-entrypoint.sh exits 0 at 5f50511. A direct PROPS_MODE=set run against a 0600 file left it 0644 with auth.admin_pa in it, and a symlinked config was replaced by a regular file. The bind-mount failure follows from rename(2) returning EBUSY when the target is a mount point; there is no Docker daemon on this host, so that case was not run. CI has not run yet: all seven workflow runs on this head are action_required.

The staged temp file was renamed over the config, replacing its inode: a 0600 config holding secrets came back umask-world-readable, a symlinked config was replaced by a regular file, and a config bind-mounted as a single file could not be renamed over at all (rename(2) returns EBUSY on a mount point), aborting the entrypoint on exactly the mounted configs this path exists for. The temp file is now copied back onto the original instead, which keeps the inode, mode, symlink and mount point, and is created 0600 itself since it can hold secrets while it exists. Regression tests check that a 0600 file keeps its mode and that a symlink survives a set with its target rewritten.

bitflicker64

Blocking: no. Summary: The fixes from the earlier rounds are in place at this head. One gap is left: when the yaml has an authentication: block with no readable authenticator, the WARN branch says it leaves both sides alone, but enable-auth.sh still writes the REST authenticator, so REST and Gremlin end up on different authenticators. There is also a small ordering problem with the temp-file mode in props.awk. Evidence: helpers pulled from docker-entrypoint.sh the way test/test-docker-entrypoint.sh does it, run against head copies of props.awk and bin/enable-auth.sh; TinkerPop 3.5.1 default checked with javap on Settings$AuthenticationSettings. Not covered: images not built and the suite not run under mawk (no Docker daemon here). All seven workflow runs on bedc21e are action_required, so CI has not run.

bitflicker64 · 2026-09-13T10:45:41Z

+
+    rest_auth=$(get_prop "auth.authenticator" "${REST_SERVER_CONF}")
+    yaml_auth=$(get_yaml_authenticator)
+    if [[ -z "${yaml_auth}" ]] && has_yaml_authentication_block; then


⚠️ This branch logs "leaving both sides untouched", but enable-auth.sh runs right after it and only touches the REST side.

At bedc21e, with a yaml block that has no authenticator and a rest-server.properties without auth.authenticator:

gremlin-server.yaml: authentication: authenticationHandler: org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler config: {tokens: conf/rest-server.properties} align_auth_config -> WARN ... leaving both sides untouched (AUTHENTICATOR_CLASS unset) ./bin/enable-auth.sh rest: auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator auth.graph_store=hugegraph yaml: unchanged (its guard sees `authentication:`) graph: gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy

In TinkerPop 3.5.1 Settings.AuthenticationSettings.authenticator defaults to AllowAllAuthenticator, so REST is on StandardAuthenticator and Gremlin is on AllowAllAuthenticator. That is the split this function is meant to prevent. Before this change a first run appended a second authentication: block, and snakeyaml's last-wins rule put both sides on the default. The test at test/test-docker-entrypoint.sh:618-625 stops at align_auth_config and never runs enable-auth.sh.

Requested change: make this branch keep the bootstrap from writing just one side. Skip enable-auth.sh here, fail the entrypoint, or add the default authenticator to the yaml block as well. Please also extend the test to run enable-auth.sh after this branch.

Fixed in bf2718f: the branch now fails the entrypoint (ERROR + nonzero exit under set -e) instead of warning while enable-auth.sh writes REST alone. Chose fail-fast over skip/add-to-yaml: skipping would start a PASSWORD deployment with auth silently half-applied, and appending an authenticator line into YAML by shell surgery risks corrupting the user's block. The unit test now asserts the refusal and runs enable-auth.sh against the same layout to prove it would write only REST (yaml untouched) — i.e. what the refusal prevents. Both shell suites pass locally.

bitflicker64 · 2026-09-13T10:45:41Z

+    # symlink and mount point, and since the temp file is fully written
+    # before the original is truncated, a failed copy still leaves the
+    # previous content on disk.
+    system("chmod 600 -- " shquote(tmp))


🧹 The mode is set after the secret has already been written. The comment on line 210 says the temp file "is created 0600 regardless of the umask", but lines 215 and 222 create tmp with awk's > under the process umask (usually 0644), and this chmod only runs after close(tmp). Until then auth.admin_pa or auth.token_secret sits in a group- and world-readable file.

Requested change: create the file 0600 before the first write, for example system("umask 077 && : > " shquote(tmp)) ahead of the loop (awk's > then truncates it and keeps the mode), or correct the comment.

Fixed in bf2718f along the suggested lines: props_set pre-creates the tmp file with umask 077 && : > tmp before the first write, so secrets never sit umask-readable; the chmod after close is kept to repair a stale tmp left by a crashed run. Both shell suites pass locally.

Unreadable-authenticator yaml block: align_auth_config now fails the entrypoint instead of logging 'leaving both sides untouched' while enable-auth.sh goes on to write the REST side alone (REST on StandardAuthenticator vs Gremlin on AllowAllAuthenticator). The error tells the operator to add an 'authenticator:' entry or remove the block. props.awk: pre-create the rewrite temp file 0600 (umask 077) before the first write so secrets never sit briefly umask-readable; the chmod after close is kept for stale tmp files from crashed runs. Tests: the unreadable-block case now asserts refusal, and a new case runs enable-auth.sh against the same layout to prove it would write only REST (yaml untouched, graph flipped) — i.e. what the refusal prevents.

bitflicker64

Blocking: yes. Summary: The earlier review rounds are addressed at this head, and the fail-fast refusal plus the 0600 pre-create in props.awk both look right. But the one-sided regression test added in bf2718f fails, so Docker Build CI / docker-build (hugegraph-server/Dockerfile) is red on this head. The test seeds an empty rest-server.properties, and GNU sed '$a\...' appends nothing to an empty file. Evidence: the failed job log for run 35101931936 stops right after the second refusal ERROR, inside the yaml-onesided block. Locally, GNU sed 4.10 left a zero-byte file untouched after sed -i -e '$a\auth.authenticator=...', and the same enable-auth.sh run against the test layout left rest-server.properties empty, so the grep -q '^auth\.authenticator=...StandardAuthenticator$' assertion exits 1. Not covered: the full suite was not run under the image's mawk/GNU toolchain (no Docker daemon here).

bitflicker64 · 2026-09-16T20:11:24Z

+(
+    cd "${onesided_dir}" || exit 1
+    REST_SERVER_CONF="./conf/rest-server.properties"
+    : > conf/rest-server.properties


⚠️ This empty fixture makes the new one-sided test fail, which is why docker-build (hugegraph-server/Dockerfile) is red at bf2718f.

enable-auth.sh appends with sed -i -e '$a\...'. On an empty file GNU sed has no last line, so $a never runs and nothing is written. The grep -q '^auth\.authenticator=org\.apache\.hugegraph\.auth\.StandardAuthenticator$' on line 221 then exits 1 and set -e stops the suite. Checked with GNU sed 4.10:

$ : > empty.properties $ sed -i -e '$a\auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator' empty.properties $ wc -c < empty.properties 0

In CI (run 35101931936), the log stops right after the second refusal ERROR, which comes from this block.

Requested change: seed the fixture with one unrelated line so it matches a real rest-server.properties, for example printf '%s\n' 'restserver.url=http://0.0.0.0:8080' > conf/rest-server.properties. If an empty mounted file should also work, switch the two REST appends in enable-auth.sh to printf '%s\n' ... >> "${CONF}/${REST_SERVER_CONF}", which also writes to an empty file.

imbajin

Blocking: yes. Summary: The current head still has auth bootstrap correctness issues for custom and mounted configurations, while the Docker Build CI failure is already covered by the existing exact-head inline finding. Evidence: exact-head static verification against docker-entrypoint.sh, props.awk, enable-auth.sh, and the failed docker-build (hugegraph-server/Dockerfile) check in run 35101931936.

imbajin · 2026-09-16T22:27:01Z

+        return
+    fi
+    if [[ -z "${rest_auth}" && -z "${yaml_auth}" ]]; then
+        export AUTHENTICATOR_CLASS="org.apache.hugegraph.auth.StandardAuthenticator"


⚠️ This branch overwrites an operator-supplied AUTHENTICATOR_CLASS. When both REST and YAML have no authenticator, it unconditionally exports StandardAuthenticator before enable-auth.sh reads the variable, so a fresh deployment with AUTHENTICATOR_CLASS=com.example.CustomAuthenticator silently uses the default. Preserve a non-empty environment value (only default when unset) and add a fresh-config regression case.

imbajin · 2026-09-16T22:27:01Z

+            }
+            next
+        }
+        inblk && /^[ \t]+authenticator[ \t]*:/ {


⚠️ inblk is never cleared when the YAML mapping leaves authentication:. For example, after authentication: has no authenticator, a later other: authenticator: com.example.Other is accepted here and then written into REST by align_auth_config, although it is not authentication.authenticator and Gremlin still uses its default. Track the block indentation or use the production YAML parser, and cover a following top-level mapping.

imbajin · 2026-09-16T22:27:01Z

+    # before the original is truncated, a failed copy still leaves the
+    # previous content on disk.
+    system("chmod 600 -- " shquote(tmp))
+    cmd = "cat -- " shquote(tmp) " > " shquote(file) " && rm -f -- " shquote(tmp)


‼️ The copy-back is not failure-safe despite the comment above it. cat -- tmp > file truncates the original before the copy completes, so an interruption, ENOSPC, or I/O error can leave a mounted rest-server.properties partially written; system() then reports the error but cannot restore the old contents. Preserve the old file on copy failure (for example with a tested backup/rollback path for inode-preserving mounts) and add failure-injection coverage.

This was referenced Sep 3, 2026

[Bug] Docker entrypoint auth bootstrap is unsafe for mounted and upgraded configs #3133

Open

feat(helm): add HStore deployment chart #3132

Draft

bitflicker64 reviewed Sep 3, 2026

View reviewed changes

imbajin reviewed Sep 3, 2026

View reviewed changes

Comment thread hugegraph-server/hugegraph-dist/docker/props.awk

bitflicker64 reviewed Sep 12, 2026

View reviewed changes

Comment thread hugegraph-server/hugegraph-dist/docker/props.awk Outdated

bitflicker64 reviewed Sep 13, 2026

View reviewed changes

bitflicker64 reviewed Sep 16, 2026

View reviewed changes

imbajin reviewed Sep 16, 2026

View reviewed changes

Uh oh!

Conversation

Adarsh-Me commented Sep 3, 2026 • edited by imbajin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changing

Implementation notes

How was this tested

Code Review Handbook

Visual summary

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Adarsh-Me commented Sep 12, 2026

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bitflicker64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imbajin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Adarsh-Me commented Sep 3, 2026 •

edited by imbajin

Loading

codecov Bot commented Sep 4, 2026 •

edited

Loading