fix(docker): make auth bootstrap safe for mounted and upgraded configs

[Adarsh-Me]

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

[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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 41.35%. Comparing base (83ef9f3) to head (c1dde5e).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #3192   +/-   ##
=========================================
  Coverage     41.35%   41.35%           
- Complexity     7299     7313   +14     
=========================================
  Files           802      802           
  Lines         69688    69770   +82     
  Branches       9291     9309   +18     
=========================================
+ Hits          28816    28850   +34     
- Misses        37576    37627   +51     
+ Partials       3296     3293    -3     

☔ 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.

[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.

[Adarsh-Me]

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.

[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]

⚠️ 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.

[Adarsh-Me]

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]

🧹 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.

[Adarsh-Me]

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.

[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]

⚠️ 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.

[Adarsh-Me]

Confirmed, and the fixture turned out to be the least interesting part of this — the empty file is a real input the entrypoint supports (props.awk happily appends to a zero-byte config, and auth.admin_pa lands in it), so I fixed enable-auth.sh rather than seeding the fixture. Seeding it would have made CI green while leaving the bug in.

Root cause, isolated on GNU sed 4.9:

$ printf '' > empty.txt
$ sed -i -e '$a\hello=world' empty.txt; echo "exit=$?"   # exit=0
$ wc -c < empty.txt                                       # 0

$ never matches without at least one line, so every append in the script was a silent no-op on an empty config. Run against the script as of bf2718f with an empty rest-server.properties and an empty gremlin-server.yaml:

enable-auth rc=0
rest bytes: 0
yaml bytes: 0
1                     # gremlin.graph -> HugeFactoryAuthProxy

That is the shape worth the ⚠️: exit code 0, the factory flipped to the authenticating proxy, and neither server told to authenticate. The entrypoint has already written auth.admin_pa and run init-store in auth mode by the time this returns, so a deployment that asked for auth serves unauthenticated and reports success.

The appends now go through a helper that writes with >> and first closes a missing trailing newline (sed -i '$a' used to do that for us, >> does not):

append_lines() {
    local file="$1"
    shift
    if [[ -s "${file}" && -n "$(tail -c 1 "${file}")" ]]; then
        printf '\n' >> "${file}"
    fi
    printf '%s\n' "$@" >> "${file}"
}

Same fixture, same command, after the change:

enable-auth rc=0
rest:
auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator
auth.graph_store=hugegraph
yaml:
authentication: {
  authenticator: org.apache.hugegraph.auth.StandardAuthenticator,
  ...
}

The new case keeps the empty fixture and asserts all of it, plus a re-run for idempotence and a config whose last line has no terminator. Reverting only enable-auth.sh reproduces the original EXIT=1 at line 221, so docker-build (hugegraph-server/Dockerfile) on run 35101931936 should go green.

Two things I cannot claim from here, so you know what this CI run is actually first-testing:

• My host is Git Bash, and MSYS stdio is text mode — awk and grep never see a CR byte, chmod is a no-op (stat -c %a reports 644 for a 600 file), and ln -s writes a copy. So the CRLF case (lines 230-249), the mode-preservation case and the symlink case cannot be executed here; I ran the file with exactly those four assertions neutralised and it exits 0. They are correct assertions, they are just untestable on Windows.
• Because the script died at line 221, everything after it has never run in CI either — this push is the first execution of lines 224 onward, including the four cases I added. If any of them is red I will get to it immediately.

[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]

⚠️ 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.

[Adarsh-Me]

Fixed in b93b52e:

if [[ -z "${rest_auth}" && -z "${yaml_auth}" ]]; then
    # Only fill in a default: an operator-supplied AUTHENTICATOR_CLASS
    # is the intent for a config that names no authenticator yet, and
    # assigning here would turn it back into StandardAuthenticator
    # before enable-auth.sh ever saw it.
    export AUTHENTICATOR_CLASS="${AUTHENTICATOR_CLASS:-org.apache.hugegraph.auth.StandardAuthenticator}"

AUTHENTICATOR_CLASS is the only channel a caller has for naming a class — it is what enable-auth.sh interpolates into the yaml block and what the other two branches of this if export — and on a mounted config that names no authenticator anywhere this branch is the only one that ever runs, so the value could not survive a start with PASSWORD set.

Left deliberately asymmetric: the else branch still exports rest_auth over the caller's value. There the config does name a class, and propagating the file is the whole point of align_auth_config; a value already in rest-server.properties beats an env var, and enable-auth.sh's per-file guard would refuse to append a second definition anyway. The both-empty branch is the only one where the env var is the sole signal.

New case covers both directions — an operator value survives, and with the variable unset the default still lands:

AUTHENTICATOR_CLASS=com.example.OperatorAuth → com.example.OperatorAuth
unset AUTHENTICATOR_CLASS                    → org.apache.hugegraph.auth.StandardAuthenticator

Restoring the unconditional assignment makes the first assertion fail (EXIT=1).

[imbajin]

⚠️ 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.

[Adarsh-Me]

Fixed in b93b52e. Reproduced exactly as described: with the block open, the first authenticator: below authentication: wins no matter which mapping it belongs to, and align_auth_config then writes that class into rest-server.properties — so a class only ever mentioned to an unrelated mapping ends up authenticating REST.

The block now tracks the key's own column and closes on the next key at or left of it:

/^[ \t]*authentication[ \t]*:/ {
    inblk = 1
    indent = match($0, /[^ \t]/)
    ...
}
# A blank line does not close a YAML mapping.
/^[ \t\r]*$/ { next }
inblk && match($0, /[^ \t]/) <= indent { inblk = 0 }
inblk && /^[ \t]+authenticator[ \t]*:/ { ... }

The blank-line rule has to precede the closing rule, otherwise a CR-only "blank" line in a CRLF-saved yaml (column 1) would end the block early. Comment lines were already skipped by the existing /^[ \t]*#/ rule, and that rule stays ahead of the closer so a commented-out authentication: cannot close the block either.

Four cases added: sibling mapping after the block returns nothing; the block's own authenticator is still found when a sibling follows it; a blank line and a commented authenticator inside the block don't break it; and a block that is itself indented ( authentication: / authenticator:) still resolves, because the comparison is against the key's column rather than zero.

Against the two real layouts in the tree rather than fixtures — the entrypoint function sourced and run on the actual files:

conf-raft1 -> 'org.apache.hugegraph.auth.StandardAuthenticator'
conf       -> ''

conf-raft1 is the case worth checking: its block is the last thing in the file, line 122 inside it is a commented-out #authenticationHandler: (so the comment rule has to stay ahead of the closer), and the authenticator is on line 121 — it still resolves.

Deleting only the inblk = 0 rule makes the new case fail (EXIT=1), so the coverage is load-bearing.

[imbajin]

‼️ 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.

[Adarsh-Me]

Fixed in b93b52e. You are right, and the comment above it was the part that was wrong: cat -- tmp > file truncates the destination in the shell before cat runs, so an error mid-copy left a partial file and the "previous content is still on disk" claim did not hold.

The copy-back is now preceded by a snapshot and followed by a repair:

bak = file ".bak"
cmd = "umask 077 && cp -- " shquote(file) " " shquote(bak)
if (system(cmd " && chmod 600 -- " shquote(bak)) != 0)
    die("cannot back up " file " before the copy-back")
cmd = "cat -- " shquote(tmp) " > " shquote(file)
if (system(cmd) != 0) {
    msg = "cannot copy " tmp " over " file
    cmd = "cat -- " shquote(bak) " > " shquote(file)
    if (system(cmd) == 0) die(msg "; the previous content is restored")
    die(msg "; " file " is damaged, previous content is in " bak)
}
if (system("rm -f -- " shquote(tmp) " " shquote(bak)) != 0)
    die("cannot remove " tmp " and " bak " after the copy-back")

Notes on the choices:

• Still a copy, not a rename, so the inode / mode / symlink / bind-mount properties the previous commit added stay true.
• umask 077 plus the explicit chmod 600 means a backup taken of a 0644 mounted config is never more permissive than its source, and repairs a stale .bak from a crashed run the same way the tmp path already does. The snapshot can hold auth.admin_pa in clear.
• Both staging files survive a failure on purpose: the .tmp is what was being written and the .bak is the way back. The success path removes both, which is now asserted.

Evidence — awk -f props.awk driven by the entrypoint's set_prop, with the staging cat replaced through PATH so the copy fails the way ENOSPC would (stdout is the already-truncated destination, so the fake writes 23 bytes and exits 1):

fake cat invoked:  -- .../config-rollback.tmp
                   -- .../config-rollback.bak
RESULT: set_prop failed as intended
--- config after rollback ---
auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator
auth.token_secret=s3cr3t
unrelated=true
MATCH: content restored

Two cases carry this; bf83032 adds the second one: the restore path above, and the case where the restore also fails — then the config is left damaged (0 bytes, measured; the honest limit of a copy that cannot be made atomic without replacing the inode), props.awk names <file>.bak in its error, and the snapshot is asserted byte-identical to the original. Reverting only props.awk to bf2718f makes the first case fail with a failed copy-back must leave the previous content in place, so the test does discriminate.

[Adarsh-Me]

One correction to what I wrote in the empty-fixture thread: I said docker-build (hugegraph-server/Dockerfile) "should go green", and it cannot tell us anything yet — CI has not run on either new commit. Every workflow on b93b52e and bf83032 is sitting at action_required, i.e. GitHub is holding the runs for this fork PR until a committer approves them (Docker Build CI on bf83032). Could someone with write access approve the workflows? The last approved run is 35101931936, which failed at test-docker-entrypoint.sh:221 and never reached anything after it.

Where the four open findings landed:

finding	fix	case that fails without it
copy-back is not failure-safe (props.awk:240)	b93b52e — 0600 snapshot before the copy, restore on failure, staging files removed only on success	injected cat fails the copy; cmp against a pristine copy
inblk never cleared (docker-entrypoint.sh:139)	b93b52e — block closes at the next key at or left of authentication:'s column	sibling mapping after the block must return nothing; also checked against the shipped conf-raft1 yaml
AUTHENTICATOR_CLASS overwritten (docker-entrypoint.sh:190)	b93b52e — default fills in only when unset	operator value survives / unset still defaults
empty fixture reds the build (test-docker-entrypoint.sh:207)	b93b52e — the appends move off sed -i '$a\...', which cannot match an empty file; the fixture is kept because the empty config is a real input	the empty-config case, plus the original line-221 assertion

What I could not execute here, so the approved run is the first real signal for it: my host is Git Bash, and MSYS stdio is text mode (awk/grep never see a CR byte), chmod is a no-op (stat -c %a says 644 for a 600 file) and ln -s writes a copy instead of linking. The CRLF case, the mode-preservation case and the symlink case therefore had to be neutralised to run the file locally — they exit 0 with those four assertions removed. Everything else, including all four cases added for the findings above, runs and passes locally, three times in a row.

[bitflicker64]

Blocking: no. Summary: The parser premise holds, HugeConfig extends commons-configuration2 PropertiesConfiguration and props.awk agrees with it on escaped keys, continuations, indented keys, bare-whitespace separators and the first-definition-wins duplicate rule; four findings remain, two of which can misconfigure or fail a mounted deployment. Evidence: probed commons-configuration2-2.8.0 and java.util.Properties side by side against props.awk on the same fixtures, and reproduced each finding through the entrypoint's own helpers at bf83032; both shell suites pass at this head. Not verified: CI has not run at this head (no check runs, combined status pending, workflows held at action_required), and props.awk was not exercised under the mawk the runtime image ships.

[bitflicker64]

⚠️ A value ending in an odd number of backslashes on the file's last line keeps that backslash, and writing it back makes the next line part of that value.

The continuation loop stops at nl < NLINES, so the trailing backslash survives into V_RAW. Measured at this head against commons-configuration2-2.8.0, the library HugeConfig extends:

rest-server.properties, last line:  auth.token_secret=abc\

props.awk  get         -> [abc\]
cc2 2.8.0  getString   -> [null]    (property dropped entirely)
JDK Props  getProperty -> [abc]

Line 243 reads that value and lines 275-279 write it into conf/graphs/hugegraph.properties, where it is no longer the last line:

BEFORE                          AFTER                           re-read
gremlin.graph=...HugeFactory    gremlin.graph=...HugeFactory
auth.token_secret=old           auth.token_secret=abc\          [abcbackend=rocksdb]
backend=rocksdb                 backend=rocksdb                 []
serializer=binary               serializer=binary

The bytes of the untouched lines are preserved as promised, but backend is now a continuation of the secret, so the server sees no backend property at all and neither side reports anything.

Requested change: have props_set refuse a value whose encoded form ends in an odd number of backslashes. Matching java.util.Properties here (dropping the backslash to yield abc) would be the wrong target: cc2 reads no property at all for that input, so the entrypoint would propagate a secret the server never had.

[Adarsh-Me]

Fixed in b8801a6 as requested — props_set now refuses rather than picking a target:

    nbs = 0
    while (nbs < length(enc_val) && substr(enc_val, length(enc_val) - nbs, 1) == "\\")
        nbs++
    if (nbs % 2 == 1)
        die("refusing to write " key ": the encoded value ends in an odd number of backslashes")

Agreed on the target being cc2 rather than java.util.Properties: matching the JDK here would drop the backslash and hand the server a secret that differs from the one on disk, which is worse than not writing it. The refusal is the only answer that cannot be wrong.

Covered by a new case in test-docker-entrypoint.sh that also asserts the config is left byte-for-byte untouched after the refusal, and that an escaped backslash (two of them, even) is still writable and replays unchanged. Checked by reverting the guard: the suite goes red on its own.

Note this arrives as a side effect of the other thread — get-decoded is gone, so the unescape path that produced your abc\ reading no longer exists for a write, but the hazard was in replaying untouched bytes, which props_set still does, so the refusal is what closes it.

[bitflicker64]

⚠️ authentication: is matched at any indentation, so a mapping nested under another key is taken as the Gremlin server's authentication block.

Line 166 has the same loose match. Run against the extracted helpers at this head:

someFeature:
  authentication:
    authenticator: com.example.Nested

get_yaml_authenticator -> com.example.Nested
rest-server.properties after align_auth_config:
  restserver.url=http://0.0.0.0:8080
  auth.authenticator=com.example.Nested

A class the operator only ever gave to an unrelated nested mapping now authenticates REST, while Gremlin stays on TinkerPop's AllowAllAuthenticator default. The same nested mapping without a readable authenticator: reaches has_yaml_authentication_block and fires the new refusal, so the container fails to start:

ERROR: gremlin-server.yaml carries an authentication block without a readable authenticator ...
(rc=1, entrypoint exits under set -e)

This is the sibling of the thread the inblk indent tracking fixed: that change scopes the block once it is open, but never checks that the key itself is top level. The test at test/test-docker-entrypoint.sh deliberately accepts an indented authentication:, so the two requirements conflict and need a decision.

Requested change: either require the key at column 0 (dropping the indented-block case), or track the parent path so an authentication: under another key is not treated as the Gremlin one.

[Adarsh-Me]

Fixed in b8801a6, taking your first option: the key must start at column 0.

        /^authentication[ \t]*:/ {
            inblk = 1
            have = 1

so the layout you ran no longer counts as the Gremlin mapping at all:

someFeature:
  authentication:
    authenticator: com.example.Nested

→ none, which means check_auth_sides sees neither side configured and lets enable-auth.sh write both from its own default, instead of putting com.example.Nested into rest-server.properties or refusing the boot.

On the conflict you spotted — yes, and this is the decision: the indented-authentication: case is dropped. The old test asserted authentication: opened the block, and that assertion cannot coexist with rejecting a nested mapping, since the two are the same bytes. conf/gremlin-server.yaml as shipped puts the key at column 0, and TinkerPop's own settings loader resolves it as a top-level key, so column 0 is the faithful reading and the indented case was this script being more permissive than the thing it is modelling. That test is removed; the nested case is asserted instead, both in the committed suite and as a want_state none line.

[bitflicker64]

🧹 get-decoded unescapes but does not trim, so a trailing space still reads as a mismatch.

commons-configuration2 trims the value; props.awk keeps trailing whitespace, which is java.util.Properties behaviour rather than the server's. Measured at this head with auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator followed by three spaces:

cc2 2.8.0 getString   -> [org.apache.hugegraph.auth.StandardAuthenticator]
props.awk get-decoded -> [org.apache.hugegraph.auth.StandardAuthenticator   ]

With gremlin-server.yaml naming the same class, align_auth_config then logs (one line, wrapped here):

WARN: REST and Gremlin name different authenticators ('org.apache.hugegraph.auth.StandardAuthenticator   ' vs 'org.apache.hugegraph.auth.StandardAuthenticator'); leaving both untouched

Same symptom as the escape thread this mode was added for, different mechanism. Nothing breaks because both sides do name the same class, but the WARN is wrong and the alignment is skipped.

Requested change: strip trailing whitespace in the get-decoded path so the comparison matches what HugeConfig reads.

[Adarsh-Me]

Moot in b8801a6, in the direction of your bigger thread on this file: get-decoded is deleted.

The mode existed so the decoded REST value could be compared against a decoded yaml scalar. Once the entrypoint stops reading which class the yaml names, there is no cross-file comparison left to trim for — check_auth_sides only asks whether auth.authenticator is present, and presence is not affected by trailing whitespace.

So the trailing-space mismatch cannot happen any more rather than being fixed: no WARN: REST and Gremlin name different authenticators, because there is no longer anything that compares the two classes. props_get_decoded and the unescape-based read are gone; unescape itself stays, since keys are still unescaped for matching (auth\.authenticator is the same key), which is now asserted directly by a new case.

[bitflicker64]

🧹 Worth a follow-up rather than a change here: this new reader is not used for the backend read at line 359, which still has the grep shape the PR replaces everywhere else.

ACTUAL_BACKEND=$(grep -E '^[[:space:]]*backend[[:space:]]*=' "${GRAPH_CONF}" | head -n 1 | sed 's/.*=//' | tr -d '[:space:]' || true)

It accepts only the = separator, so with HG_SERVER_BACKEND unset (when it is set, line 259 rewrites the entry as backend=... and the grep works) a mounted graph config using the other separators is missed:

conf/graphs/hugegraph.properties: 'backend : hstore'
  cc2 2.8.0 getString     -> [hstore]
  line 359 ACTUAL_BACKEND -> []

The hstore wait-partition.sh stabilization check is then skipped with no message.

Note it cannot simply become get_prop "backend" "${GRAPH_CONF}" while the trailing-whitespace gap above is open: backend=hstore with trailing spaces would return them and fail the == "hstore" test that tr -d '[:space:]' currently survives. The two belong together, or a TODO here pointing at it.

[Adarsh-Me]

Fixed in b8801a6, and it came along with the get-decoded removal rather than needing the trailing-whitespace gap closed first:

ACTUAL_BACKEND=$(get_prop_encoded "backend" "${GRAPH_CONF}" | tr -d '[:space:]' || true)

get_prop_encoded reports the on-disk bytes, so the trim happens in bash where the == "hstore" test already lives — which is the coupling you flagged, resolved by not needing a trimmed reader at all. backend : hstore with HG_SERVER_BACKEND unset now reaches the partition-wait check, and duplicate definitions resolve to the first one the way HugeConfig does instead of the last one grep found.

[bitflicker64]

Blocking: no. Summary: The biggest cut available is to replace the awk YAML reader and the cross-side authenticator propagation with a refusal of any one-sided auth config, which keeps REST and Gremlin from splitting and drops roughly 250 lines across the entrypoint, props.awk and the test suite. Evidence: read the full diff at bf83032; ran the head get_prop_encoded and has_yaml_authentication_block helpers with the proposed check against neither-side, both-side (including auth\.authenticator : A with a quoted yaml scalar), REST-only, yaml-only and authenticator-less-block fixtures.

[bitflicker64]

⚠️ This reader, and the propagation it feeds in align_auth_config, can go. Keep the guarantee (REST and Gremlin never end up on different authenticators) and drop the YAML parsing.

The PR already refuses one case, a yaml block with no readable authenticator, because carrying on would leave auth half-applied. Refuse every one-sided case the same way and the entrypoint no longer needs to know which class the yaml names, only whether each side names one. Both inputs already exist in this diff:

check_auth_sides() {
    local rest=0 yaml=0
    [[ -n "$(get_prop_encoded auth.authenticator "${REST_SERVER_CONF}")" ]] && rest=1
    has_yaml_authentication_block && yaml=1
    if (( rest != yaml )); then
        log "ERROR: authentication is configured in only one of" \
            "rest-server.properties and gremlin-server.yaml;" \
            "configure both or neither, then restart."
        return 1
    fi
}

Neither side: enable-auth.sh appends both, with its own AUTHENTICATOR_CLASS default. Both sides: its per-file guards make it a no-op. One side: stop.

That deletes get_yaml_authenticator (quotes, flow mappings, inline comments, indent scoping), get_prop and the get-decoded mode in props.awk, most of align_auth_config, and the test blocks for yaml scalars, sibling scope, escaped authenticators and the AUTHENTICATOR_CLASS default: roughly 250 lines. The trailing-space thread on props.awk line 279 goes away with get-decoded.

What you give up: an operator with a one-sided mounted config gets an error telling them to finish it, instead of the entrypoint finishing it for them, and two sides naming different classes are left alone without the WARN, as they were before this PR. Checked with the head helpers: neither and both (including auth\.authenticator : A next to a quoted yaml scalar) pass; REST-only, yaml-only and a block without an authenticator all refuse.

[Adarsh-Me]

Taken in b8801a6 — this is a better shape than what I had, and I have deleted the parser: get_yaml_authenticator, get_prop, the get-decoded mode in props.awk, the class comparison and the AUTHENTICATOR_CLASS export all go, and check_auth_sides decides by presence.

One change to your sketch, because the version as written reopens the fail-open this PR exists for. has_yaml_authentication_block is true whenever the mapping is present, so a mounted file with

authentication:
  authenticationHandler: org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler

and auth.authenticator=com.example.MyAuth in the REST file gives rest=1, yaml=1 → passes. But snakeyaml resolves that mapping to no authenticator, Gremlin stays on AllowAllAuthenticator, and REST enforces — one side configured, silently. The nameless case is exactly the one your verification list expected to refuse, and it only refuses when the REST side is empty too.

So the read is three-valued (none / named / nameless) and a nameless mapping is refused on its own, before the comparison. It still never reads the class. A mapping that names nothing cannot be left to enable-auth.sh either, because its guard looks for the presence of authentication: and would then write only the REST file.

Two other things that came out of implementing this:

• the key now has to be at column 0 (separate thread), so a nested authentication: under another feature reads as none rather than deciding the REST side;
• the both sides, different classes case passes through untouched with no WARN, as you described. Left as is deliberately: with the per-file guards already in enable-auth.sh, that config is one the operator wrote on both sides, and guessing which of their two classes to propagate is the thing the parser existed to do badly.

AUTHENTICATOR_CLASS is no longer exported at all, so the operator-supplied value reaches enable-auth.sh untouched. That behaviour moved, so the test moved with it: two fresh trees, one with the variable set and one without, asserting which class lands in both files.

Suite is green apart from the four assertions this host cannot execute (MSYS text-mode stdio, chmod no-op, ln -s copying). Each of the four changes above was checked by reverting it on its own — backslash refusal, column 0, the nameless refusal, and the refusal path — and each turns the suite red by itself. Production delta is net -56 lines of shell and awk; I added net +105 lines of tests, so the overall diff is not the ~250-line reduction you estimated.

[Adarsh-Me]

b8801a6 takes @bitflicker64's simplification and answers all five threads. Short version:

finding	change	what fails without it
props_set can write a value ending in an odd number of backslashes, which then swallows the next line	refuse it; the file is left byte-for-byte untouched	reverting the guard alone turns the suite red
authentication: matched at any indentation, so a nested mapping decided the REST side	key must start at column 0; the indented-authentication: test is dropped and the nested case asserted instead	same
get-decoded did not trim, so a trailing space produced a wrong WARN	the mode is deleted — nothing compares the two classes any more	n/a
the post-startup backend read accepted only =	goes through props.awk and trims in bash, so backend : hstore reaches the partition-wait check	n/a (behaviour asserted in the suite)
parse which authenticator the yaml names	don't: check_auth_sides decides by presence and refuses one-sided layouts	making the check always pass turns the suite red

Two things worth flagging rather than leaving in the diff:

The proposal as sketched reopened the fail-open. has_yaml_authentication_block is true for any mapping that exists, so a mounted file with an authentication: block that names no authenticator plus auth.authenticator set in the REST file reads as rest=1, yaml=1 → passes → snakeyaml resolves no authenticator → Gremlin on AllowAllAuthenticator, REST enforcing. The read is therefore three-valued (none / named / nameless) and nameless is refused by itself, before the comparison — still without ever reading the class.

Column 0 costs a test that used to pass. The old suite asserted authentication: opened the block; that and rejecting a nested mapping are the same bytes, so they cannot both hold. I dropped the permissive one: the shipped config puts the key at column 0 and TinkerPop resolves it as a top-level key, so the indented form was this script being more generous than what it models. Say so if you would rather keep the lenient reading and reject only the nested case by tracking the parent path.

Honest state of the run, same caveat as before: this host is Git Bash, so the four assertions that need a real CR byte, a working chmod, or a symlink cannot execute here — the suite passes with exactly those four neutralised in a scratch copy, and the committed file is unchanged. Production delta is net -56 lines of shell and awk; tests net +105, so the overall diff is not the ~250-line reduction the thread estimated.

CI on b8801a6 is action_required like every previous head on this fork PR, so nothing has executed it yet — could someone with write access approve the workflows, and take a look at the auth decision above?

[imbajin]

Blocking: yes. Summary: The auth bootstrap still accepts mismatched REST and Gremlin authentication states and can mishandle mounted Java properties. Evidence: exact-head static review of docker-entrypoint.sh, enable-auth.sh, and props.awk; latest-head workflows await maintainer approval.

[imbajin]

‼️ Blocking: yes. Summary: This match also counts YAML comment text, so authentication: {} # authenticator: com.example.Fake is reported as named while Gremlin has no authenticator. Evidence: yaml_auth_state finds the comment text and enable-auth.sh skips appending because the line starts with authentication:. Please ignore comments and scalar text and count only a real direct authenticator key.

[imbajin]

⚠️ Blocking: no. Summary: A column-zero YAML comment ends this mapping scan, so authentication: followed by # note and an indented authenticator is reported as nameless and blocks startup when PASSWORD is set. Evidence: this condition clears inblk on any first nonblank byte, including #. Please ignore comment lines when detecting the end of the mapping.

[imbajin]

‼️ Blocking: yes. Summary: This accepts an authenticator at any depth, so authentication.config.authenticator counts even when the direct authentication.authenticator is absent and Gremlin uses AllowAllAuthenticator. Evidence: the exact-head flow marks the nested key named and lets REST/YAML parity pass; TinkerPop keeps config as a separate map. Please count only the direct child authenticator key.

[imbajin]

‼️ Blocking: yes. Summary: The parity check only runs when PASSWORD is nonempty, so a mounted REST-only authenticator bypasses validation and the server can start with Gremlin unauthenticated. Evidence: check_auth_sides is called only inside this conditional and execution proceeds when PASSWORD is unset. Please validate mounted authentication state regardless of whether bootstrap creates an admin password.

[imbajin]

‼️ Blocking: yes. Summary: A legal Java-properties key written as gremlin\u002egraph is not matched here, so its HugeFactory value is left unwrapped when authentication is enabled. Evidence: Java Properties decodes the Unicode escape to gremlin.graph, but this grep/sed pair matches only literal or backslash-escaped dots. Please use the properties-aware helper or handle Unicode escapes before rewriting the graph factory.

[imbajin]

‼️ Blocking: yes. Summary: If appending the YAML auth block fails while the other config files remain writable, this function returns nonzero but the script continues and can exit successfully after updating REST and graph settings. Evidence: enable-auth.sh has no errexit or append-status checks, while the entrypoint trusts its exit status. Please stop on every failed append and preserve a nonzero script result.

[imbajin]

⚠️ Blocking: yes. Summary: LF-only getline reads a bare-CR properties file as one record, so updating its first key can replace that record and drop every later key. Evidence: props_load strips only a final CR after getline, while java.util.Properties treats bare CR as a line ending. Please split LF, CRLF, and CR records before rewriting.

[imbajin]

⚠️ Blocking: no. Summary: Java properties treats form-feed as separator whitespace, but split_kv recognizes only equals, colon, space, and tab; auth.authenticator=... is therefore missed and a valid mounted configuration is refused. Evidence: the exact-head separator scan has no form-feed case. Please handle form-feed consistently in key scanning, leading whitespace, and separator trimming.

[bitflicker64]

Blocking: yes. Summary: props.awk and the per-file guards fix the duplicate-definition cases they target, but enable-auth.sh and check_auth_sides still disagree about what counts as a configured side, so three mounted layouts pass the check and still end with only one of REST and Gremlin authenticating. Evidence: extracted get_prop_encoded, yaml_auth_state and check_auth_sides from docker-entrypoint.sh at b8801a6 and ran them, then the real enable-auth.sh, against temp conf trees (bash 5, gsed as sed); read HugeAuthenticator.loadAuthenticator and InitStore for how an empty auth.authenticator is treated. CI on this head is held at action_required, so the new shell suites have not run in CI.

[bitflicker64]

Important: This guard accepts authentication: at any indentation, while yaml_auth_state in the entrypoint now only accepts column 0. A nested mapping therefore passes check_auth_sides as "neither side configured" and then stops this append, so only the REST side gets written.

Reproduced at b8801a6 with the layout from the new want_state none test:

host: 0.0.0.0
someFeature:
  authentication:
    authenticator: com.example.Nested

and a rest-server.properties without auth.authenticator. yaml_auth_state prints none, check_auth_sides returns 0, and enable-auth.sh appends auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator and auth.graph_store=hugegraph to rest-server.properties but leaves gremlin-server.yaml unchanged. REST then enforces StandardAuthenticator and Gremlin stays on TinkerPop's AllowAllAuthenticator. The reply on the column-0 thread says this case lets enable-auth.sh "write both from its own default"; it writes only one.

Please make this guard match the entrypoint: grep -Eq '^authentication[[:blank:]]*:'. Add a test that runs the real enable-auth.sh on the nested layout and asserts both files get an authenticator.

[Adarsh-Me]

Confirmed, and still live at 53a5edb — this one was not covered by the 16:10Z push. Reproduced against the real enable-auth.sh:

before: state=none  check_auth_sides=PASS | after: gremlin=none  rest=[...StandardAuthenticator] => ONE-SIDED

Guard anchored to column 0 in 2c9ebaf so the two files answer the same question:

if ! grep -Eq '^authentication[[:blank:]]*:' "${CONF}/${GREMLIN_SERVER_CONF}"; then

Test added at docker/test/test-docker-entrypoint.sh:1066, on your layout, running the real script: it first asserts check_auth_sides accepts the tree (a case the entrypoint refuses no longer tests anything), then requires yaml_auth_state to come back named, the REST class to read back through props.awk, exactly one auth.authenticator definition, and check_auth_sides to still hold afterwards. someFeature:'s own block is asserted byte-identical, so the fix cannot "resolve" this by rewriting the nested mapping.

Red before the change, green after: nested authentication mapping: gremlin-server.yaml is none, not named.

I did not check a CR-only or CRLF gremlin-server.yaml here — MSYS text mode hides CR bytes, so ^authentication against those shapes is CI's to confirm, not mine.

[bitflicker64]

Important: check_auth_sides treats an auth.authenticator with an empty value as "REST not configured", but this guard has no single meaning for that case. With auth.authenticator= it counts the key as present and skips the append. With a bare auth.authenticator line (no separator) it counts the key as absent and appends a second definition, which props.awk and HugeConfig ignore because the first definition wins. In both cases the yaml block is appended and REST keeps an empty authenticator.

Reproduced at b8801a6 with host: 0.0.0.0 in gremlin-server.yaml:

• rest-server.properties has auth.authenticator=. The check passes, the yaml gets authenticator: org.apache.hugegraph.auth.StandardAuthenticator, and rest-server.properties keeps auth.authenticator=.
• rest-server.properties has a bare auth.authenticator line. The check passes, and the file now holds auth.authenticator followed by auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator. get_prop_encoded auth.authenticator returns an empty string.

HugeAuthenticator.loadAuthenticator returns null for an empty class, so REST serves without authentication while Gremlin requires it, and auth.admin_pa has already been written from PASSWORD.

Please handle the defined-but-empty case one way on both sides. Either check_auth_sides refuses a definition with an empty value, or the REST write goes through set_prop (first definition, in place) instead of an append guarded by grep. Add a test for both spellings.

[Adarsh-Me]

Confirmed, still live at 53a5edb — also not covered by the 16:10Z push. All three spellings reproduce against the real script:

auth.authenticator=    -> gremlin=named  rest=[]  ONE-SIDED
auth.authenticator     -> gremlin=named  rest=[]  ONE-SIDED
auth.authenticator=    (trailing blanks) -> also rest=[]

To your framing of the two options, I took the second (write through set_prop) because the first would leave a hole: check_auth_sides already asks for the value (get_prop_encoded non-empty), so it counts an empty definition as unconfigured and lets the tree through — refusing it in the guard would fix the docker path only, while bin/enable-auth.sh from the tarball, which never runs the guard, would still write one side.

Worth adding to your measurement: Java also strips the trailing blanks, so auth.authenticator= parses to the empty string too. Compared against java.util.Properties directly (jdk 17), for auth.authenticator:

auth.authenticator=com.example.Foo -> PRESENT len=15
auth.authenticator=                -> PRESENT len=0
auth.authenticator                 -> PRESENT len=0   (bare, no separator)
auth.authenticator=   /  \t / ':' forms -> PRESENT len=0
(no key)                            -> ABSENT

props.awk's get agrees on every row, so the reader was already right and only the caller's question was wrong.

Fix in 2c9ebaf: both REST defaults go through props_set, which replaces the first definition where it stands and appends when there is none, so the empty placeholder is rewritten rather than followed by a definition first-definition-wins buries. props_has lost its last caller here and is removed; PROPS_MODE=has stays, since it answers a different question and is covered directly.

Tests at test-docker-entrypoint.sh:1076 loop all three empty spellings and assert the class reads back through props.awk, exactly one raw definition, the placeholder replaced on line 1 (not appended), unrelated lines kept — plus one case that auth.authenticator=com.example.OperatorAuth survives the default write, so the new props_set path cannot clobber an operator. Red before: empty definition [auth.authenticator=]: rest-server.properties reads back [].

[bitflicker64]

Important: ^[ \t]+authenticator[ \t]*: matches at any depth inside the block, and the same-line match() on line 107 matches anywhere on the line. An authenticator that sits under config therefore counts as the Gremlin one.

Reproduced at b8801a6: both authentication: / authenticationHandler: ... / config: / authenticator: com.example.X and authentication: {config: {authenticator: X}} make yaml_auth_state print named. With a REST auth.authenticator present, check_auth_sides passes. TinkerPop reads authentication.authenticator only, so Gremlin falls back to AllowAllAuthenticator while REST authenticates.

Please count only a direct child. Record the indentation of the first key inside the block and accept authenticator: only at that indentation, and for the flow form only at the top level of the braces. Add both layouts to the want_state cases as nameless.

[Adarsh-Me]

Already fixed, and your test asks are in — but not by the push you were reviewing. 5afbb4a landed 16:10Z, 45 minutes after this review, and I had not replied on the thread, so nothing told you the code had moved. That is on me.

yaml_auth_state is now docker/yamlscan.awk, and both rules you asked for are there: the indentation of the first key inside the block is recorded and only that column counts (yamlscan.awk:248-249), and for the flow form keys are read at depth one only (yamlscan.awk:173), so {config: {authenticator: X}} is invisible to it. config: staying its own map is what the server sees.

Both layouts you named are want_state cases as nameless already: test-docker-entrypoint.sh:902 (block under config:) and :906 (flow inside a flow), with :911 keeping {config: {...}, authenticator: X} as named so the direct child does not get thrown out with the nested one.

The half of this that was not fixed was the mirror image in enable-auth.sh — its guard still accepted authentication: at any indentation, so a nested mapping reached the disagreement from the other side. Covered in 2c9ebaf on the column-0 thread.

[Adarsh-Me]

All eight reproduced before touching anything, and every fix was reverted to confirm its own test goes red. Pushed in 5afbb4a; master merged in 53a5edb (mergeable: true again -- #3194 and the startup-timeout change had landed in the same files).

You were right on all eight. Table of what each one was, measured against java.util.Properties where the question was properties grammar:

thread	what it was	at b8801a6	now
r4083528783	authentication: {} # authenticator: X counted as named because the scanner matched the comment text	named	nameless
r4083528796	a column-zero comment line ended the mapping, refusing a valid deployment	nameless	named
r4083528809	authenticator accepted at any depth, so authentication.config.authenticator counted	named	nameless
r4083528814	check_auth_sides only ran inside the PASSWORD branch	exit 0, server starts	exit 1, refused before init-store
r4083528824	gremlin\u002egraph matched no grep pattern, factory left unwrapped	org.apache.hugegraph.HugeFactory	HugeFactoryAuthProxy
r4083528831	a failed yaml append on a writable REST file still exited 0	exit 0, half-configured	exit 1
r4083528843	bare-CR file read as one record; rewriting its first key dropped every later one	auth.authenticator gone	read and survives
r4083528855	form feed not treated as separator whitespace	key invisible	parsed as Java sees it

The first, second and third were the same root cause and the worst of them: two of the three produced a false named, which is a passed parity check with Gremlin on AllowAllAuthenticator.

properties/yaml readers

• props.awk now splits records on \r\n, \n and \r and remembers each line's terminator, so a rewrite replays untouched bytes. Your r4083528843 description was exactly right, and the rewrite is worse than "drop every later key" in one respect worth naming: auth.authenticator is usually the later key, so the surviving file is the unauthenticated one.
• \f is now separator whitespace on both sides of the separator, in the leading-whitespace strip and in the comment/blank-line test.
• The yaml reader moved out of the shell into yamlscan.awk. It answers about the mapping, not the text: column-zero mapping only, direct child only, in block and flow form alike (a flow key counts at depth one, so {config: {authenticator: X}} does not), comments stripped outside quotes, and a direct authenticator with an empty/null/~ value treated as nameless rather than named. The last one is not in your list -- it is the same fail-open shape as r4083528783, so I closed it while I was in there.
• enable-auth.sh reads and writes through props.awk instead of grep/sed. The embedded-CR workaround the old pattern needed goes with it, since the reader now handles terminators.

One packaging change to look at: props.awk moved from docker/ to src/assembly/static/bin/, because bin/enable-auth.sh now needs it and the tarball does not ship docker/. That directory's assembly fileSet (<include>*</include>) picks it up with no descriptor change, and the image gets it from the same place it gets every other bin/ file, so the COPY .../props.awk . line added earlier in this PR is gone. yamlscan.awk stays in docker/ with an explicit COPY -- only the entrypoint uses it.

Two smaller changes the review pushed me into: PROPS_MODE=has, which answers "is this key defined" without confusing auth.authenticator= (present, empty) with absent -- appending a default on top of an empty first definition would have left the empty one in force; and exit status 2 now means an error while 1 means absent, so a caller wearing errexit cannot read an unreadable file as "not there".

Verification

• props.awk against real java.util.Properties (javac/java 17) over 15 file shapes -- LF, CRLF, CR-only, mixed, no-final-newline, \f on either side of the separator, \u002e and \. in keys, continuations, indentation, :/whitespace separators. Two assertions per shape: every property Java sees must be visible to the reader with the same value, and rewriting one key must leave every other key intact and the key set unchanged. 21 failing assertions at b8801a6, 0 now.
• yamlscan.awk over 21 yaml shapes (the three findings, the positive forms a wrong reader must keep accepting, and the negatives).
• The eight findings as a standalone before/after table: 8/8 fail at b8801a6, 8/8 pass now.
• test-docker-entrypoint.sh and docker-entrypoint-test.sh both green on the merged tree, which includes the startup-timeout cases from fix(docker): make the Server startup timeout configurable #3187.

What did not run here, so please read the above with this attached: no Linux container on this host, so mawk never executed -- the image's /usr/bin/awk is mawk on jammy, and I kept the new awk to substr/index/match, no RT, no regex RS, no gensub, and gawk --posix --lint is clean apart from the parameter-shadowing warnings this file already had. The Java oracle is 17, not the 11 the image runs; the grammar cases here are identical between them, but that is reasoning, not a measurement. snakeyaml was never run either, so the yaml rules are from the spec plus a written trace, not from the parser that will actually read these files. Three assertions in the unit suite -- CRLF byte preservation, config-mode preservation, symlinked config -- cannot be evaluated on a Windows host at all (MSYS gawk drops the CR of a CRLF pair on read, chmod does not affect what stat reports, ln -s copies), so they are gated behind host probes that print a skip line rather than pass quietly; they will genuinely run in CI.

CI at 53a5edb: six workflows registered, all completed / action_required -- so none of them have run, and nothing here is green. A maintainer with write access still needs to approve the pending deployments; mergeable_state is blocked on that, not on anything in the diff.

[imbajin]

Blocking: yes. Summary: The auth parity check still accepts YAML null nodes as authenticated, and valid mounted forms can be rejected or rewritten inconsistently. Evidence: exact-head static review of yamlscan.awk, enable-auth.sh, and docker-entrypoint.sh; latest-head workflows are action_required.

[imbajin]

‼️ names_class() treats only the lowercase text null and ~ as empty. SnakeYAML accepts !!null null (and case variants such as NULL) as a null node, so this scanner reports it as named and check_auth_sides() can start REST authentication while Gremlin has no authenticator. Please recognize YAML null tags/case variants or fail closed on unsupported tags, and cover this in the parity tests.

[Adarsh-Me]

Fixed in d5ccb94. names_class() now refuses null in any case, plus !!null and an empty quoted scalar; a value whose type comes from an unresolvable tag fails closed instead of guessing. A quoted "null" deliberately stays named, since quoting makes it a string. Verified by reverting: NULL reported named before, nameless after. Cases added to the parity group.

[imbajin]

⚠️ In a quoted flow value, this branch never appends characters while FST == "val"; CUR_VAL stays empty and {authenticator: "org.example.Auth"} is classified as nameless. A valid mounted configuration with matching REST authentication is therefore refused before server startup. Preserve quoted value contents with escape handling and add a quoted flow authenticator case.

[Adarsh-Me]

Fixed in d5ccb94. Quoted bytes now accumulate for flow values as they already did for keys, with the double-quoted backslash escape honoured, so {authenticator: "org.example.Auth"} reads named. Reproduced first: it returned nameless. Reverted to confirm the new quoted-flow cases go red; single-quoted, sibling and nested-under-config forms are all covered.

[imbajin]

⚠️ yamlscan.awk accepts and unquotes a quoted top-level key such as "authentication":, but this guard matches only the bare spelling. With PASSWORD set, an existing valid block is missed and a second block is appended; YAML can reject the duplicate or select the appended default while REST retains its existing authenticator. Reuse the same YAML-aware check here and cover quoted top-level keys.

[Adarsh-Me]

Fixed in d5ccb94. The guard now asks yamlscan.awk the same question check_auth_sides asks, so anything other than none leaves the operator block alone. The release tarball ships no yamlscan.awk, and there the fallback grep learned the quoted spellings too. Measured before: one top-level key in, two out; now one. Covered in both layouts.

[imbajin]

⚠️ get_prop_encoded() returns the on-disk escaped value; props.awk decodes values only when PROPS_DECODED=1. For a valid properties entry backend=h\u0073tore, the JVM reads hstore but this comparison skips wait-partition.sh, allowing startup to continue before HStore partitions are assigned. Read this key in decoded mode before comparing it.

[Adarsh-Me]

Fixed in d5ccb94 through a new get_prop_decoded(). Measured against java.util.Properties: the escaped spelling is hstore to the JVM, so wait-partition.sh is now reached, and the rocksdb case still skips it - asserted end to end in docker-entrypoint-test.sh. The REST presence check was measured as well and does not need decoding, so it stays encoded.

[imbajin]

Blocking: yes. Summary: Several valid or accepted configuration forms can bypass REST/Gremlin authenticator parity; the properties writer also follows predictable staging symlinks. Evidence: Exact-head static control-flow review of yamlscan.awk, props.awk, and HugeConfig.loadConfigFile().

[imbajin]

‼️ names_class() treats YAML anchor/alias syntax as a non-empty class name. For valid YAML with authentication: {authenticator: &noAuth null}, SnakeYAML resolves the value to null but the scanner reports named; with REST authentication configured, check_auth_sides() accepts the mismatch and Gremlin can remain unauthenticated. Resolve aliases with the runtime YAML parser or reject anchors/aliases here, and cover null/empty aliases.

[Adarsh-Me]

Confirmed: authenticator: &noAuth null answered named. Fixed in cc9cc8c - the anchor label comes off and the text behind it decides, an anchor with nothing behind it is empty, and an alias is refused rather than guessed. &cls com.example.Anchored still names its class. Reproduced against d5ccb94 first; the new group is red without the fix.

[imbajin]

‼️ A later duplicate direct child can replace the value this scanner has already accepted. In block YAML, authenticator: org.apache.hugegraph.auth.StandardAuthenticator followed by authenticator: null hits finish("named") on the first row even though SnakeYAML keeps the last value; the parity check can pass while Gremlin has no authenticator. Scan the full mapping with the server's last-wins behavior or reject duplicate keys, and cover block and flow forms.

[Adarsh-Me]

Reproduced in both forms, fixed in cc9cc8c: block and flow mappings are read to their end, and a direct authenticator defined twice is refused - reason on stderr - instead of being settled by the first row seen. snakeyaml keeps the last value or rejects the document, so first-match was never the server's answer. Group red without the fix.

[imbajin]

⚠️ split_pair() only recognizes a colon followed by end of line, space, or tab. On CRLF input, the top-level line is authentication:\r, so this check misses it; its indented children are skipped and yamlscan.awk reports none. With Gremlin authentication present and REST authentication absent, check_auth_sides() accepts the config and leaves REST open. Strip the carriage return before parsing and add a CRLF parity case.

[Adarsh-Me]

Confirmed: authentication:\r matched no key, so a CRLF gremlin-server.yaml read as none and check_auth_sides accepted REST-open beside an authenticating Gremlin. Fixed in cc9cc8c - trailing CR is line noise, taken off before a line is split. The fixture writes one CR for the host (MSYS text mode needs two) and skips itself when it cannot.

[imbajin]

‼️ props_load() reads only PROPS_FILE, while HugeConfig loads properties through Commons Configuration with includes enabled by default. If the main file has include=rest-auth.properties and the included file defines auth.authenticator, this helper reports REST authentication as absent; with no YAML auth mapping, the entrypoint passes the parity check and starts a REST-protected/Gremlin-open server. Resolve includes consistently or fail closed when an include is present.

[Adarsh-Me]

Fixed in cc9cc8c by failing closed: a file carrying a live include is refused in get, has and set (exit 2), nothing is written, and check_auth_sides says the REST side could not be read instead of reporting a one-sided config. included.foo and a commented #include= still read. Resolving includes needs the parser HugeConfig uses, which I could not run here.

[Adarsh-Me]

Fixed in 5940fa5. props.awk now refuses both include and includeOptional, case-insensitively, in every mode (exit 2), so INCLUDE= or IncludeOptional= no longer read as ordinary properties. Controls kept: included.filter and a commented #include still read. Extended the test to every spelling; it fails without the fix. Did not run commons-configuration2 here.

[imbajin]

⚠️ This predictable .tmp path is opened with truncating redirection without checking for a symlink; the later AWK redirection follows it too. If a writable mounted config directory contains rest-server.properties.tmp as a symlink, the default-root Docker entrypoint can overwrite a file elsewhere in the container while writing credentials. Use an exclusive 0600 temporary file with no-follow semantics, and secure the .bak destination the same way.

[Adarsh-Me]

Fixed in cc9cc8c: both staging names come from an exclusive mktemp create (0600 whatever the umask), so there is no predictable path to arrange and nothing to follow. The rollback group follows the paths props.awk reports instead of guessing names, and a planted <file>.tmp/.bak is asserted untouched. The symlink case is host-gated and did not run here.

[bitflicker64]

Blocking: yes. Summary: props.awk reads properties with the java.util.Properties grammar, but HugeConfig loads rest-server.properties and hugegraph.properties through commons-configuration2 2.8.0, which trims each line and keeps \ escapes. At d5ccb94 that difference means a mounted gremlin.graph=...HugeFactory with a trailing blank is no longer wrapped in HugeFactoryAuthProxy (master wraps it), and a PASSWORD ending in a space leaves REST with no authenticator while check_auth_sides passes. Evidence: ran head enable-auth.sh, props.awk and the entrypoint helpers (encode_prop_value, set_prop, yaml_auth_state, check_auth_sides) against copies of the shipped conf/ tree, and read every result back with commons-configuration2 2.8.0 PropertiesConfiguration and java.util.Properties (JDK 17). Base enable-auth.sh at 83ef9f3 (the PR base) was run the same way with GNU sed for the gremlin.graph comparison. Not covered: images were not built and mawk was not exercised (no Docker daemon here), and CI has not run at this head: all six workflow runs are action_required.

[bitflicker64]

Important: This compares the decoded value without trimming it, so a gremlin.graph line with trailing whitespace is no longer wrapped, and the server still opens the graph with the plain HugeFactory. commons-configuration2 2.8.0 (what HugeConfig loads with) trims the line. props.awk keeps the trailing blank, which is java.util.Properties behaviour.

I copied the shipped conf/ tree, added one trailing space to gremlin.graph=org.apache.hugegraph.HugeFactory, and ran enable-auth.sh:

base (sed s/gremlin.graph=...HugeFactory/.../g)  -> gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy 
                                                   cc2 getProperty -> [org.apache.hugegraph.auth.HugeFactoryAuthProxy]
head d5ccb94 (props_get + exact ==)              -> gremlin.graph=org.apache.hugegraph.HugeFactory 
                                                   cc2 getProperty -> [org.apache.hugegraph.HugeFactory]

enable-auth.sh exits 0 in both runs. At this head, authentication is enabled on both sides but the graph is not behind HugeGraphAuthProxy. GraphManager only logs a warning about that (GraphManager.java:1851). So a mounted config that master wrapped now runs without per-user access control on the graph. The same trim gap was raised for the old get-decoded path in an earlier round, and it came back when this comparison moved to PROPS_DECODED=1.

Requested change: strip trailing space, tab and form feed from GRAPH_FACTORY before the comparison, or make the decoded read trim the way commons-configuration2 does. Add a test case with a trailing blank after HugeFactory that asserts the factory is wrapped.

[Adarsh-Me]

Confirmed and fixed in cc9cc8c, at the comparison site rather than in the decoded read: trailing blanks come off before gremlin.graph is compared, so a mounted ...HugeFactory is wrapped again while a factory this script does not own is left alone. The test runs enable-auth.sh over both the raw and \ spellings, red without the fix.

[bitflicker64]

Important: This guard exists so that a written value never turns the next line into a continuation under commons-configuration2. It misses \ at the end of a value, which is how encode_prop_value writes a trailing space. cc2 trims the line before its continuation check, so abc\ becomes abc\ and swallows the following line.

I ran the entrypoint order (check_auth_sides, set_prop auth.admin_pa "abc ", then enable-auth.sh) on the shipped default conf/. The run ends like this:

rest-server.properties tail:
  auth.admin_pa=abc\ 
  auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator
  auth.graph_store=hugegraph
check_auth_sides                -> passes (props.awk: rest named, yaml named)
cc2 getProperty auth.authenticator -> null
cc2 getProperty auth.admin_pa      -> [abcauth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator]

The REST server then loads with no authenticator while Gremlin requires one. That is the one-sided state check_auth_sides exists to refuse. The same thing happens to whatever line follows a auth.token_secret that ends in a space. The encoder is older than this PR, so master behaves the same way. But this PR adds the parity check and this cc2 guard for exactly this hazard, and neither catches it. cc2 also keeps \ inside a value: auth.admin_pa=Strong\\Pass\ 9!, the fixture in docker-entrypoint-test.sh, reads back as Strong\Pass\ 9! rather than Strong\Pass 9!.

Requested change: have encode_prop_value write a space as \u0020. Both readers decode that the same way: cc2 and java.util.Properties both return abc for abc\u0020. Also extend this guard to refuse an encoded value that ends in a backslash followed by whitespace. Add a round-trip test that checks the line after a trailing-space secret is still a separate property.

[Adarsh-Me]

Done as suggested in cc9cc8c: encode_prop_value writes a space as \u0020, and the guard judges the right-trimmed form, refusing abc\ and abc\\\ while still accepting abc\\ . Round trip added - auth.admin_pa=abc\u0020 on disk, java.util.Properties on JDK 17 returns abc with the next property intact. commons-configuration2 was not run here; no jar on this host.

[bitflicker64]

Minor: chmod 600 -- file fails with BSD chmod, which reads -- as a file name after the mode. This file now ships in the release tarball's bin/, so bin/enable-auth.sh fails on macOS. On macOS (/bin/chmod), enable-auth.sh against the shipped conf/ printed chmod: --: No such file or directory and props.awk: cannot back up .../rest-server.properties before the copy-back, then exited 1. By then it had already appended the authentication block to gremlin-server.yaml and left rest-server.properties.tmp and .bak in conf/. The master script also failed on macOS (BSD sed rejects $a\), but it wrote nothing. Requested change: drop the -- from the two chmod calls here and on line 312. The paths are already quoted by shquote and never start with - in these calls.

[Adarsh-Me]

Fixed in cc9cc8c - and both chmod calls are gone, not just the --: the staged files are created 0600 by an exclusive mktemp now, so no mode is left to repair. Nothing passes -- to chmod. A config whose name starts with - is asserted to still write. BSD chmod itself could not be run on this host.

[imbajin]

Review at cc9cc8c

Blocking: yes. Score: 6.5/10. Five independent review lanes completed, followed by independent reproduction of the parser findings.

• The existing include finding remains partially unresolved. Commons Configuration 2.8.0 also accepts includeOptional and case-insensitive forms of both directives. props.awk rejects only exact lowercase include. A real-parser fixture loads a REST authenticator from the included file while check_auth_sides returns success with REST absent and YAML none. Please reject all supported include directive spellings, or resolve them with the actual parser.
• The inline findings cover effective YAML authentication detection and a reproducible CI environment leak.

Evidence used real Commons Configuration 2.8.0 and TinkerPop 3.5.1 Settings.read() with SnakeYAML 1.27. Shell checks passed in isolated copies with macOS limitations; this is not a complete Linux image validation. The current rocksdb CI job fails at the escaped-backend assertion, not an unrelated Java test.

[imbajin]

‼️ [P1] Fail closed when the effective root authentication mapping is not identified. With two top-level authentication mappings, this early return accepts the first named authenticator, while TinkerPop Settings.read() uses the final empty mapping and defaults to AllowAllAuthenticator. An indented root mapping is also valid to Settings.read() but is skipped at line 314 and reported as none, allowing the opposite REST/Gremlin mismatch. Both cases were reproduced with TinkerPop 3.5.1 / SnakeYAML 1.27. Detect duplicate root mappings through EOF and recognize or explicitly reject unsupported root layouts; test against the runtime parser.

[Adarsh-Me]

Fixed in 5940fa5. The scanner now reads to EOF and refuses a duplicate root authentication mapping (nameless, count on stderr), and anchors on the first key's indentation, so an indented root mapping resolves instead of reporting none. A nested authentication still reports none. Tests added; each fails without the fix, confirmed by reverting. Did not run SnakeYAML here.

[imbajin]

⚠️ [P2] Isolate this config-driven invocation from the CI backend environment. Server CI exports BACKEND=rocksdb; the entrypoint maps it to HG_SERVER_BACKEND and overwrites this fixture's escaped hstore, so the assertion at line 436 fails. This reproduces with BACKEND=rocksdb and passes when BACKEND and HG_SERVER_BACKEND are unset. Clear inherited backend variables for these config-driven cases rather than changing production environment precedence.

[Adarsh-Me]

Fixed in 5940fa5. The harness now unsets inherited BACKEND/HG_SERVER_BACKEND/PD_PEERS/HG_SERVER_PD_PEERS at the top, so the escaped hstore fixture decides the stabilization check instead of the CI matrix value. Production env precedence is unchanged; env-driven cases set it per invocation. Reproduced: the test failed under BACKEND=rocksdb before, passes after.

[Copilot]

Copilot review overview

🟡 Changes recommended

One or more issues must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 1

Open (5)

• Runtime helpers hidden by mounted application volume · New
• Runtime helpers hidden by mounted application volume · New
• props.awk errors are swallowed as empty backend · New
• Block scalar authenticators are misclassified as named values · New
• Authenticator parity check ignores differing class names · New

What changed in this PR

Hardens Docker authentication bootstrap for mounted and upgraded HugeGraph configurations.

Changes:

• Adds grammar-aware properties parsing and safe in-place rewriting.
• Adds YAML authentication-state scanning and idempotent auth configuration.
• Packages helpers and expands Docker entrypoint coverage.

File	Description
hugegraph-server/​hugegraph-dist/​src/​assembly/​static/​bin/​props.awk	Updated as part of this pull request.
hugegraph-server/​hugegraph-dist/​src/​assembly/​static/​bin/​enable-auth.sh	Updated as part of this pull request.
hugegraph-server/​hugegraph-dist/​docker/​yamlscan.awk	Updated as part of this pull request.
hugegraph-server/​hugegraph-dist/​docker/​test/​test-docker-entrypoint.sh	Updated as part of this pull request.
hugegraph-server/​hugegraph-dist/​docker/​docker-entrypoint.sh	Updated as part of this pull request.
hugegraph-server/​hugegraph-dist/​docker/​docker-entrypoint-test.sh	Updated as part of this pull request.
hugegraph-server/​Dockerfile-hstore	Updated as part of this pull request.
hugegraph-server/​Dockerfile	Updated as part of this pull request.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[imbajin]

Blocking: yes. Summary: Valid Gremlin authentication configurations can bypass the parity check, and an ambiguous bootstrap path can leave REST and Gremlin with different authentication states. Evidence: exact-head static review of yamlscan.awk, enable-auth.sh, and the entrypoint checks.

[imbajin]

‼️ Critical: A valid root flow mapping such as { host: 8182, authentication: { authenticator: org.example.Auth } } is accepted by TinkerPop 3.5.1's Settings.read()/SnakeYAML, but split_pair() captures only { host at the first colon. AUTH_BLOCKS stays 0 and this scanner returns none, so check_auth_sides() passes when REST has no authenticator even though Gremlin does. Please parse root flow mappings or fail closed whenever the root form cannot be classified.

[imbajin]

⚠️ Important: In a valid multiline flow mapping (authentication: { with an indented authenticator: com.example.Auth and a closing }), the closing brace is at ROOT_IND. This branch clears flow before scan_flow() can commit the final pair, so the scanner reports nameless and check_auth_sides() refuses a valid authenticated configuration. Please let an open flow mapping consume its closing line before block-sibling handling, including a final item without a trailing comma.

[imbajin]

‼️ Critical: AWK splits records on LF, but this only strips CR characters at the end of an LF-delimited record. A valid bare-CR YAML file remains one record, so the root authentication: key is not recognized and AUTH_BLOCKS stays 0; REST can be left open beside authenticated Gremlin. Normalize CR, LF, and CRLF before parsing, or fail closed when the scanner cannot separate records.

[imbajin]

‼️ Critical: YAML resolves double-quoted key escapes, so "authentic\u0061tion" is the server's authentication key. unquote() only removes surrounding quotes, so this comparison misses the key and returns none; with REST auth unset, the parity check accepts an authenticated Gremlin config beside open REST. Decode YAML escapes or reject quoted key forms the scanner cannot resolve.

[imbajin]

‼️ Critical: This treats nameless as a usable authentication block. For an existing mapping without authenticator (for example, one that only sets authenticationHandler), enable-auth.sh skips the Gremlin update and still writes StandardAuthenticator to REST; TinkerPop 3.5.1 defaults an omitted authenticator to AllowAllAuthenticator. Please fail before changing REST or safely complete the mapping; the release-tarball fallback must also reject mappings it cannot verify.

[Adarsh-Me]

Fixed in b42b1ba. The guard now answers none/named/nameless through the shared reader and refuses before writing auth.authenticator when the mapping names no class, including the tarball grep fallback. New cases fail against c1dde5e; both CI-wired suites exit 0. No Docker or Java here, so no boot was observed.

[Adarsh-Me]

Fixed in c1dde5e — spread-flow closing brace, CR/LF/CRLF records, escaped quoted keys, empty block scalars. Each input run against 5940fa5 and the new head; table in the commit. Both shell suites exit 0. Root flow mapping is refused, not parsed. Dockerfile volume-mount and class-name parity untouched — no Docker here. gawk only; snakeyaml not run.

[bitflicker64]

Blocking: no. Summary: The b42b1ba change holds up. enable-auth.sh now refuses a nameless or unverifiable Gremlin mapping before it writes REST, in both the image layout and the tarball fallback. One new Minor item: an invalid AUTHENTICATOR_CLASS is written into the yaml before it is validated, so a refused run leaves a one-sided config. The open Copilot threads from 5940fa5 (helpers hidden by the /hugegraph-server volume, || true on the backend read, class-name parity) are still unanswered at this head and are not repeated. Evidence: full diff 83ef9f3..b42b1ba and c1dde5e..b42b1ba read; enable-auth.sh run against a copy of the shipped conf/ with AUTHENTICATOR_CLASS='com.example.My Auth' (exit 1, yaml block appended, REST untouched, yamlscan reports named); docker/test/test-docker-entrypoint.sh exits 0 on macOS with BWK awk; all six workflow runs at this head are action_required, so CI has not run.

[bitflicker64]

Minor: The yaml block is appended with AUTHENTICATOR_CLASS before anything checks that value, and props_set (line 97) only rejects it later, at line 239. A value that the character check refuses therefore leaves the one-sided tree that the comment above append_lines says this script must not leave.

Reproduced at b42b1ba on a copy of the shipped conf/ with the image layout (yamlscan.awk present):

$ AUTHENTICATOR_CLASS='com.example.My Auth' bin/enable-auth.sh
enable-auth.sh: refusing to write an unescaped value: com.example.My Auth    (exit 1)
gremlin-server.yaml   authentication: { authenticator: com.example.My Auth, ... }   yamlscan -> named
rest-server.properties  no auth.authenticator
hugegraph.properties    gremlin.graph=org.apache.hugegraph.HugeFactory

On the next container start check_auth_sides sees yaml named and REST empty and refuses to boot, and the operator has to edit the yaml by hand. A rerun of the script with the variable fixed or unset does not repair it either: the yaml is now named, so it keeps the bad class and REST gets StandardAuthenticator.

Requested change: validate AUTHENTICATOR_CLASS with the same character check right after line 164, before any file is touched, and add a case asserting that a refused class leaves both files unchanged.

[Adarsh-Me]

Fixed in a5de9f6 — the character check is now check_class_name, called as soon as AUTHENTICATOR_CLASS is set and before any config is touched; props_set still calls it. Your case exits 1 with both files unchanged and the yaml still none, and the rerun then arms both sides with one class. New both-layout assertions fail against b42b1ba. Suite exits 0; CI is still action_required.

[bitflicker64]

Blocking: yes. Summary: yamlscan.awk reports named for a valid gremlin-server.yaml whose only authenticator sits inside a multi-line config flow mapping, so enable-auth.sh arms REST while Gremlin runs on AllowAllAuthenticator and check_auth_sides accepts it; two other valid flow spellings are reported as nameless and now refuse to boot. Evidence: ran head yamlscan.awk and enable-auth.sh against the shipped conf/ with the shapes in the inline comments, and parsed the same files with TinkerPop 3.5.1 Settings.read() on snakeyaml 1.27; test-docker-entrypoint.sh and docker-entrypoint-test.sh (GNU sed) both exit 0 locally; all six workflow runs at a5de9f6 are action_required, so CI has not run on this head.

[bitflicker64]

Critical: The block-mapping branch reads each line on its own, so a child value that opens a flow collection and continues on later lines is not tracked. A line inside that collection at the child indentation is then counted as a direct child. Example:

authentication:
  config: {tokens: conf/rest-server.properties,
  authenticator: org.apache.hugegraph.auth.StandardAuthenticator}

yamlscan.awk prints named for this file. TinkerPop 3.5.1 Settings.read() (snakeyaml 1.27) on the same file gives authentication.authenticator=org.apache.tinkerpop.gremlin.server.auth.AllowAllAuthenticator and config={tokens=conf/rest-server.properties, authenticator=org.apache.hugegraph.auth.StandardAuthenticator}, because flow content ignores indentation. Run against the shipped conf/ plus this mapping, enable-auth.sh exits 0, skips the yaml append and writes auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator to rest-server.properties. check_auth_sides then passes on every boot while REST enforces and Gremlin accepts unauthenticated requests. A double-quoted scalar that spans lines inside the mapping has the same effect.

Please track open flow collections and quoted scalars in child values and skip their continuation lines, or refuse through refuse() whenever a child value leaves one open. Add this case to test-docker-entrypoint.sh.

[Adarsh-Me]

Fixed in be7f5e2. Child values now track an open flow collection or quoted scalar and their continuation lines are skipped at any indentation, so config: {tokens: x, no longer lends its second line to the mapping; a span that never closes is refused. Test added to test-docker-entrypoint.sh; fails without the fix.

[bitflicker64]

Important: Only a value that starts with { on the key line is read as a flow mapping. Two valid spellings fall through to the block branch and are reported as nameless:

• authentication: &auth {authenticator: org.apache.hugegraph.auth.StandardAuthenticator, authenticationHandler: org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler} (an anchor before the flow mapping)
• authentication: with {authenticator: org.apache.hugegraph.auth.StandardAuthenticator, authenticationHandler: ...} on the next line

TinkerPop 3.5.1 Settings.read() loads both with authenticator=org.apache.hugegraph.auth.StandardAuthenticator, and master boots them. At this head check_auth_sides runs on every start, so a container with either file and a matching auth.authenticator now refuses to boot, and the log says the mapping names no authenticator, which points the operator at the wrong fix.

Please drop a leading anchor (&name) before the { test and accept a flow mapping that opens on the first child line, or send these shapes through refuse() so the message says the shape is unsupported.

[Adarsh-Me]

Also fixed in be7f5e2: unanchor() strips a leading &label before the brace test, and a flow collection opening on the first child line goes to the same scan_flow() reader, so both spellings answer named the way Settings.read() loads them. Both shapes asserted in test-docker-entrypoint.sh.

[bitflicker64]

Blocking: yes. Summary: A gremlin-server.yaml that master's enable-auth.sh appended to twice (bind-mounted conf, fresh /hugegraph-server volume, conf-bak missing) now stops the container on every boot, although TinkerPop 3.5.1 reads it as StandardAuthenticator. A valid mapping whose authenticator value starts on the next line is also refused, and an explicit-key ? authentication mapping reads as none. Evidence: ran the base enable-auth.sh twice over the shipped conf/, then the head docker-entrypoint.sh with PASSWORD set (exit 1, duplicate-mapping refusal); compared yamlscan.awk with TinkerPop 3.5.1 Settings.read() on snakeyaml 1.27 for 26 yaml shapes; test-docker-entrypoint.sh and docker-entrypoint-test.sh both exit 0 on macOS (BWK awk, GNU sed); all six workflow runs at be7f5e2 are action_required, so CI has not run on this head. The open Copilot threads (volume, || true on the backend read, class parity) are not repeated. The PR description still says the entrypoint propagates an authenticator to the missing side and warns on differing classes; the code now refuses one-sided configs and does not compare classes.

[bitflicker64]

Important: Refusing every file with two top-level authentication mappings stops containers that master's own enable-auth.sh produced. The base script appended a block whenever conf-bak/ was missing, and conf-bak lives in the /hugegraph-server volume, so each fresh container over a bind-mounted conf/ appended another block (the #3133 bug this PR fixes).

Reproduced: base enable-auth.sh run twice over the shipped conf/ with conf-bak removed in between gives 2 identical blocks. TinkerPop 3.5.1 Settings.read() (snakeyaml 1.27) on that file gives authenticator=org.apache.hugegraph.auth.StandardAuthenticator, and master boots it. At this head docker-entrypoint.sh with PASSWORD set prints 2 top-level authentication mappings and exits 1. The entrypoint's ERROR line then says the mapping "names no authenticator; add an authenticator entry", which points the operator at the wrong fix. check_auth_sides runs on every boot, so this also hits containers started without PASSWORD.

The test comment at test-docker-entrypoint.sh:1281 already records that Settings.read() takes the last mapping, and handle_line resets AUTH_SEEN/AUTH_NAMED when a later block opens. Requested change: answer from the last mapping here (else RESULT = auth_state()), which still refuses the existing test case whose last mapping is empty, and add a case with two identical StandardAuthenticator blocks that must read named. If you keep the refusal, make check_auth_sides print the duplicate-mapping reason instead of the "names no authenticator" text.

[Adarsh-Me]

Fixed in 664dcac. Reproduced with the base enable-auth.sh run twice over the shipped conf/ (2 identical blocks), then TinkerPop 3.5.1 Settings.read() on snakeyaml 1.27 under JDK 17: authenticator=StandardAuthenticator. Now answers from the last mapping; a class-less last mapping still reads nameless. Entrypoint reaches start-hugegraph.sh instead of exiting 1.

[bitflicker64]

Important: A block authenticator whose value starts on the next, deeper line is reported as nameless, so a valid config that master boots now refuses to start. names_class gets the empty key-line text, and the deeper continuation line is skipped because its indentation is not child.

Measured against TinkerPop 3.5.1 Settings.read() on snakeyaml 1.27:

authentication:
  authenticator:
    org.apache.hugegraph.auth.StandardAuthenticator

yamlscan: nameless. Settings.read(): authenticator=org.apache.hugegraph.auth.StandardAuthenticator. The same holds with the value double-quoted on the next line. check_auth_sides then exits 1 with "names no authenticator", which is wrong for this file.

Requested change: when the key line carries no value, read the first deeper non-comment line as the value (plain or quoted scalar), or route this shape through refuse() so the message says the spelling is unsupported. Add both spellings to test-docker-entrypoint.sh.

[Adarsh-Me]

Fixed in 664dcac. Both spellings measured against Settings.read(): plain and double-quoted value on the deeper line return StandardAuthenticator; the scanner said nameless and stopped the boot. It now waits for that line. A value there that is a mapping or collection names no class (the server throws) and goes through refuse(). Both added to the tests.

[bitflicker64]

Minor: An explicit-key root mapping is not recognized, so a Gremlin config that authenticates reads as none. For ? authentication followed by : authenticator: org.apache.hugegraph.auth.StandardAuthenticator, yamlscan prints none, while TinkerPop 3.5.1 Settings.read() gives authenticator=org.apache.hugegraph.auth.StandardAuthenticator. With no auth.authenticator in rest-server.properties, check_auth_sides passes and REST starts open beside an authenticating Gremlin, the direction the entrypoint comments call unsafe. Master had no check here, so this is not a regression. Requested change: refuse any root line that starts with ? through refuse(), and add a test case.

[Adarsh-Me]

Fixed in 664dcac, and the unsafe direction is confirmed: with rest-server.properties unconfigured this file reached start-hugegraph.sh at be7f5e2 while Settings.read() returns StandardAuthenticator. A root line beginning question-mark-space now goes through refuse(), so check_auth_sides stops the boot. Test added.

[Adarsh-Me]

All three fixed in 664dcac; both suites green here under gawk 5.0.0, and reverting just the scanner makes the new assertions fail. One asymmetry left deliberately: a duplicate authenticator inside one mapping still refuses, though the server takes the last of those too -- say if it should follow the root rule. CI is still action_required at this head.

[bitflicker64]

Blocking: no. Summary: The awk YAML reader still gives the unsafe none answer for a root authentication key with a leading BOM, a !!str tag or an &anchor, and it refuses a valid CRLF block-scalar authenticator. Neither is a regression from master, and neither repeats an open thread (Dockerfile volume, the || true backend read, class-name parity, the stale PR description). Evidence: compared yamlscan.awk at 664dcac with snakeyaml 1.27 Yaml.load on nine yaml shapes; ran the head enable-auth.sh over the shipped conf/ with a BOM-prefixed authentication line; test-docker-entrypoint.sh and docker-entrypoint-test.sh both exit 0 locally (BWK awk 20200816, GNU sed 4.x); all six workflow runs at 664dcac are action_required, so CI has not run on this head.

[bitflicker64]

Important: A root authentication key that SnakeYAML resolves but this comparison does not recognize reads as none, so the parity check passes while Gremlin authenticates. K_TXT is compared after unquote() only, so a UTF-8 BOM before a first-line key, a !!str tag on the key, or an &anchor on the key all miss it.

Measured at 664dcac with snakeyaml 1.27 (new Yaml().load) against awk -f yamlscan.awk:

<BOM>authentication: {authenticator: com.example.Auth}        snakeyaml: com.example.Auth   yamlscan: none
!!str authentication: {authenticator: com.example.Auth}       snakeyaml: com.example.Auth   yamlscan: none
&k authentication: {authenticator: com.example.Auth}          snakeyaml: com.example.Auth   yamlscan: none

Without PASSWORD, check_auth_sides sees REST unconfigured and yaml none, so the server starts with REST open beside an authenticating Gremlin. With PASSWORD, enable-auth.sh appends a second default mapping. I ran it over the shipped conf/ with the BOM line prepended: it exited 0, the yaml gained a second authentication: block, and SnakeYAML now resolves org.apache.hugegraph.auth.StandardAuthenticator, so the operator's class is replaced without a message. This matches what master does today, so it is not a regression, but it is the one-sided and duplicate-mapping outcome this reader exists to prevent.

Requested change: strip a leading U+FEFF from the first record, and send any root key whose text starts with !, & or * to refuse(), the same way the ? explicit-key form is handled now. Add the three rows above to test-docker-entrypoint.sh.

[Adarsh-Me]

Fixed in 364d42f. BOM stripped from the first record; a node property before the authentication key goes to refuse(); an empty CR-split segment no longer closes a block scalar. Six assertions added, all checked against Settings.read(). Note: only that key is refused -- !!str host: beside a working mapping is an ordinary sibling, and refusing it stops a boot that works.

[bitflicker64]

Minor: On a CRLF file a block-scalar authenticator always reads as nameless, so a valid config fails to boot. The main rule splits each LF record on \r, so authenticator: >-\r produces an empty segment right after the indicator line. That empty segment reaches this branch with indent 0, fails the > BLOCK_IND test, and finish_block() runs with an empty BLOCK_TXT before the content line is read.

Measured at 664dcac:

printf 'host: 0.0.0.0\r\nauthentication:\r\n  authenticator: >-\r\n    com.example.Auth\r\n'
snakeyaml 1.27: authenticator=com.example.Auth    yamlscan: nameless

The same file with LF endings reads named. check_auth_sides refuses every nameless answer, so the CRLF copy stops the container even with REST configured. It fails closed, so this is not a security problem.

Requested change: skip empty segments while BLOCK is open, or treat a blank line inside a block scalar as part of it the way YAML does. Add a CRLF block-scalar case next to the existing CRLF yaml test.

[bitflicker64]

Blocking: no. Summary: Two small documentation and dead-code issues in props.awk at 364d42f; no new correctness problem found beyond the threads already open on this PR. Evidence: read the full diff at 364d42f, ran docker/test/test-docker-entrypoint.sh locally (passes), and ran props.awk get against a missing file and an include file (both exit 2).

[bitflicker64]

Minor: The header says get always exits 0, but it exits 2 whenever props_load calls die: for an unreadable file (line 237) and for a file with an include directive (line 288). check_auth_sides in docker-entrypoint.sh and props_get in enable-auth.sh both rely on that nonzero status to refuse instead of treating the key as absent, so a reader who trusts this header could remove those checks.

Measured at 364d42f: PROPS_MODE=get PROPS_KEY=a PROPS_FILE=/nonexistent awk -f props.awk /dev/null prints props.awk: cannot read /nonexistent and exits 2. The same call on a file whose first line is include=x.properties also exits 2.

Please change the sentence to say that get exits 0 when the key is absent and 2 when the file cannot be read or uses an include directive.

[Adarsh-Me]

Fixed in 9ae26c8: get now states exit 0 when the key is absent, 2 when the file cannot be read or uses an include — both reproduced at 364d42f. has, props_has and their tests are gone; nothing outside the suite called them. Its assertions became three get status checks, mutation-tested. Suite green here; CI has not run.

[bitflicker64]

Minor: No production code calls PROPS_MODE=has any more. enable-auth.sh replaced its props_has guard with ensure_rest_prop, which reads the value instead (its comment at enable-auth.sh lines 109-113 explains why the presence check was wrong), and the entrypoint only uses get and set. At 364d42f, git grep 'PROPS_MODE=has' matches only test-docker-entrypoint.sh (lines 1053, 1058, 1067, 1794).

The header text for this mode (lines 29-31 and 395-398) still presents it as the right guard before appending a default, which is the approach enable-auth.sh moved away from. Please remove the has mode, props_has and their tests, or point to the caller that needs them.