Adarsh-Me commented on code in PR #3192: URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4080559102
########## hugegraph-server/hugegraph-dist/docker/props.awk: ########## @@ -0,0 +1,300 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# props.awk — read and rewrite Java ".properties" files with the grammar +# HugeConfig (commons-configuration over JDK Properties) applies, so the +# entrypoint and the server agree on what a mounted file means. grep/sed +# rewrites do not: they see `\`-escaped keys, `:` separators, continuation +# lines and duplicate definitions differently, which is how a mounted +# config ends up with two definitions of one key. +# +# One invocation, selected with the `PROPS_MODE` environment variable: +# +# PROPS_MODE=get PROPS_KEY=K PROPS_FILE=F +# print the value of K's first logical definition +# PROPS_MODE=set PROPS_KEY=K PROPS_FILE=F +# replace K's first definition in place, drop every other +# definition of K, append one when the file has none. The new +# value arrives pre-encoded in PROPS_VALUE_ENCODED (an environment +# variable, so secrets never appear in `ps` output or in awk's +# argv), and -v is not used for it so awk cannot mangle its +# backslash escapes. +# +# Grammar implemented (java.util.Properties line reader + the +# first-definition-wins rule Configuration.getString applies): +# - '#' / '!' comments and blank lines +# - '=' / ':' / whitespace separators, with whitespace then an optional +# single '=' or ':' accepted as one separator +# - continuations: a physical line ending in an odd number of +# backslashes joins the next line (its leading whitespace stripped) +# - backslash escapes in keys and values, including \uXXXX +# - duplicate logical keys resolve to the first definition +# +# Rewrites keep every untouched line byte-for-byte (comments, blank +# lines, unrelated entries), and replace the first definition where it +# stands, so mounted configs stay reviewable in git diffs. + +function die(msg) { + printf "props.awk: %s\n", msg > "/dev/stderr" + exit 1 +} + +function hex_digit(c) { + return index("0123456789abcdef", tolower(c)) - 1 +} + +# \uXXXX is a UTF-16 code unit in Java. Values here are effectively +# ISO-8859-1, so codes above 0xFF are kept as their literal escape text +# rather than being mangled through a single-byte sprintf. +function unescape(s, out, i, n, c, code, j, d, ok) { + out = "" + n = length(s) + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (c != "\\") { out = out c; continue } + if (i == n) break + i++ + c = substr(s, i, 1) + if (c == "u" && i + 4 <= n) { + code = 0 + ok = 1 + for (j = 1; j <= 4; j++) { + d = hex_digit(substr(s, i + j, 1)) + if (d < 0) { ok = 0; break } + code = code * 16 + d + } + if (ok) { + i += 4 + if (code <= 255) out = out sprintf("%c", code) + else out = out substr(s, i - 5, 6) + continue + } + } + if (c == "t") out = out "\t" + else if (c == "n") out = out "\n" + else if (c == "r") out = out "\r" + else if (c == "f") out = out "\f" + else out = out c + } + return out +} + +# A physical line is continued when it ends in an odd number of +# backslashes (an even count escapes itself). +function trailing_backslashes(s, n, k) { + n = length(s) + k = 0 + while (k < n && substr(s, n - k, 1) == "\\") k++ + return k +} + +function is_skipped(raw) { + return raw ~ /^[ \t]*([#!]|$)/ +} + +# Split a logical line into its raw (still-escaped) key and value parts. +# Results land in K_RAW / V_RAW because awk returns one value. +function split_kv(s, n, i, c, esc, sep_at, rest) { + n = length(s) + esc = 0 + sep_at = 0 + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (esc) { esc = 0; continue } + if (c == "\\") { esc = 1; continue } + if (c == "=" || c == ":" || c == " " || c == "\t") { sep_at = i; break } + } + if (sep_at == 0) { + K_RAW = s + V_RAW = "" + return + } + K_RAW = substr(s, 1, sep_at - 1) + rest = substr(s, sep_at) + c = substr(rest, 1, 1) + if (c == "=" || c == ":") { + rest = substr(rest, 2) + } else { + sub(/^[ \t]+/, "", rest) + c = substr(rest, 1, 1) + if (c == "=" || c == ":") rest = substr(rest, 2) + } + sub(/^[ \t]+/, "", rest) + V_RAW = rest +} + +function shquote(s) { + gsub(/'/, "'\\''", s) + return "'" s "'" +} + +# Load `file` into per-block arrays: one block per comment/blank line or +# logical entry, spanning exactly the physical lines it occupies. +function props_load(file, raw, rc, nl, stripped, next_raw, start, logical) { + NLINES = 0 + while ((rc = (getline raw < file)) > 0) { + NLINES++ + RAW[NLINES] = raw + } + if (rc == -1) + die("cannot read " file) + close(file) + + NBLOCK = 0 + for (nl = 1; nl <= NLINES; nl++) { + raw = RAW[nl] + # CRLF: java.util.Properties drops the line terminator, so one + # trailing CR is stripped for parsing only. RAW[] keeps the byte + # so props_set replays untouched lines byte-for-byte. + stripped = raw + sub(/\r$/, "", stripped) + if (is_skipped(stripped)) { + NBLOCK++ + BTYPE[NBLOCK] = "skip" + BFIRST[NBLOCK] = nl + BLAST[NBLOCK] = nl + continue + } + start = nl + logical = stripped + while (trailing_backslashes(logical) % 2 == 1 && nl < NLINES) { Review Comment: Fixed in `b8801a6` as requested — `props_set` now refuses rather than picking a target: ```awk 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. ########## hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh: ########## @@ -70,12 +74,141 @@ set_prop() { get_prop_encoded() { local key="$1" file="$2" - local esc_key - esc_key=$(printf '%s' "$key" | sed -e 's/[][(){}.^$*+?|\\/]/\\&/g') - sed -nE \ - "s~^[[:space:]]*${esc_key}([[:space:]]*[:=][[:space:]]*|[[:space:]]+)(.*)$~\\2~p" \ - "${file}" | head -n 1 + PROPS_MODE=get PROPS_KEY="${key}" PROPS_FILE="${file}" \ + awk -f "${PROPS_AWK}" /dev/null +} + +# Decoded read: unescapes the on-disk value the way java.util.Properties +# does, so it compares equal with the snakeyaml-decoded scalar from +# get_yaml_authenticator. The raw get_prop_encoded mode stays for the +# secret round trip, which must replay backslashes byte-for-byte. +get_prop() { + local key="$1" file="$2" + + PROPS_MODE=get-decoded PROPS_KEY="${key}" PROPS_FILE="${file}" \ + awk -f "${PROPS_AWK}" /dev/null +} + +# First uncommented `authenticator:` inside the gremlin-server.yaml +# authentication block, or on the `authentication:` line itself (a flow +# mapping). snakeyaml resolves duplicate top-level keys to the last one, +# but a mounted file carrying two authentication blocks is pathological; +# report the first and let the mismatch WARN handle it. The scalar is +# cleaned the way snakeyaml reads it — an inline comment (a '#' preceded +# by whitespace), surrounding quotes and padding are stripped — because +# java.util.Properties keeps all of those in the class name. +get_yaml_authenticator() { + local yaml="./conf/gremlin-server.yaml" + + [[ -f "${yaml}" ]] || return 0 + awk ' + function scalar(s, out, i, n, c, q) { + out = "" + q = "" + n = length(s) + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (q != "") { + if (c == q) q = "" + else out = out c + continue + } + if (c == "\"" || c == "\047") { q = c; continue } + if (c == "#" && + (out == "" || substr(out, length(out), 1) ~ /[ \t]/)) + break + if (c == "," || c == "}" || c == "]") break + out = out c + } + sub(/^[ \t\r]+/, "", out) + sub(/[ \t\r]+$/, "", out) + return out + } + /^[ \t]*#/ { next } + /^[ \t]*authentication[ \t]*:/ { Review Comment: Fixed in `b8801a6`, taking your first option: the key must start at column 0. ```awk /^authentication[ \t]*:/ { inblk = 1 have = 1 ``` so the layout you ran no longer counts as the Gremlin mapping at all: ```yaml 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. ########## hugegraph-server/hugegraph-dist/docker/props.awk: ########## @@ -0,0 +1,300 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# props.awk — read and rewrite Java ".properties" files with the grammar +# HugeConfig (commons-configuration over JDK Properties) applies, so the +# entrypoint and the server agree on what a mounted file means. grep/sed +# rewrites do not: they see `\`-escaped keys, `:` separators, continuation +# lines and duplicate definitions differently, which is how a mounted +# config ends up with two definitions of one key. +# +# One invocation, selected with the `PROPS_MODE` environment variable: +# +# PROPS_MODE=get PROPS_KEY=K PROPS_FILE=F +# print the value of K's first logical definition +# PROPS_MODE=set PROPS_KEY=K PROPS_FILE=F +# replace K's first definition in place, drop every other +# definition of K, append one when the file has none. The new +# value arrives pre-encoded in PROPS_VALUE_ENCODED (an environment +# variable, so secrets never appear in `ps` output or in awk's +# argv), and -v is not used for it so awk cannot mangle its +# backslash escapes. +# +# Grammar implemented (java.util.Properties line reader + the +# first-definition-wins rule Configuration.getString applies): +# - '#' / '!' comments and blank lines +# - '=' / ':' / whitespace separators, with whitespace then an optional +# single '=' or ':' accepted as one separator +# - continuations: a physical line ending in an odd number of +# backslashes joins the next line (its leading whitespace stripped) +# - backslash escapes in keys and values, including \uXXXX +# - duplicate logical keys resolve to the first definition +# +# Rewrites keep every untouched line byte-for-byte (comments, blank +# lines, unrelated entries), and replace the first definition where it +# stands, so mounted configs stay reviewable in git diffs. + +function die(msg) { + printf "props.awk: %s\n", msg > "/dev/stderr" + exit 1 +} + +function hex_digit(c) { + return index("0123456789abcdef", tolower(c)) - 1 +} + +# \uXXXX is a UTF-16 code unit in Java. Values here are effectively +# ISO-8859-1, so codes above 0xFF are kept as their literal escape text +# rather than being mangled through a single-byte sprintf. +function unescape(s, out, i, n, c, code, j, d, ok) { + out = "" + n = length(s) + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (c != "\\") { out = out c; continue } + if (i == n) break + i++ + c = substr(s, i, 1) + if (c == "u" && i + 4 <= n) { + code = 0 + ok = 1 + for (j = 1; j <= 4; j++) { + d = hex_digit(substr(s, i + j, 1)) + if (d < 0) { ok = 0; break } + code = code * 16 + d + } + if (ok) { + i += 4 + if (code <= 255) out = out sprintf("%c", code) + else out = out substr(s, i - 5, 6) + continue + } + } + if (c == "t") out = out "\t" + else if (c == "n") out = out "\n" + else if (c == "r") out = out "\r" + else if (c == "f") out = out "\f" + else out = out c + } + return out +} + +# A physical line is continued when it ends in an odd number of +# backslashes (an even count escapes itself). +function trailing_backslashes(s, n, k) { + n = length(s) + k = 0 + while (k < n && substr(s, n - k, 1) == "\\") k++ + return k +} + +function is_skipped(raw) { + return raw ~ /^[ \t]*([#!]|$)/ +} + +# Split a logical line into its raw (still-escaped) key and value parts. +# Results land in K_RAW / V_RAW because awk returns one value. +function split_kv(s, n, i, c, esc, sep_at, rest) { + n = length(s) + esc = 0 + sep_at = 0 + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (esc) { esc = 0; continue } + if (c == "\\") { esc = 1; continue } + if (c == "=" || c == ":" || c == " " || c == "\t") { sep_at = i; break } + } + if (sep_at == 0) { + K_RAW = s + V_RAW = "" + return + } + K_RAW = substr(s, 1, sep_at - 1) + rest = substr(s, sep_at) + c = substr(rest, 1, 1) + if (c == "=" || c == ":") { + rest = substr(rest, 2) + } else { + sub(/^[ \t]+/, "", rest) + c = substr(rest, 1, 1) + if (c == "=" || c == ":") rest = substr(rest, 2) + } + sub(/^[ \t]+/, "", rest) + V_RAW = rest +} + +function shquote(s) { + gsub(/'/, "'\\''", s) + return "'" s "'" +} + +# Load `file` into per-block arrays: one block per comment/blank line or +# logical entry, spanning exactly the physical lines it occupies. +function props_load(file, raw, rc, nl, stripped, next_raw, start, logical) { + NLINES = 0 + while ((rc = (getline raw < file)) > 0) { + NLINES++ + RAW[NLINES] = raw + } + if (rc == -1) + die("cannot read " file) + close(file) + + NBLOCK = 0 + for (nl = 1; nl <= NLINES; nl++) { + raw = RAW[nl] + # CRLF: java.util.Properties drops the line terminator, so one + # trailing CR is stripped for parsing only. RAW[] keeps the byte + # so props_set replays untouched lines byte-for-byte. + stripped = raw + sub(/\r$/, "", stripped) + if (is_skipped(stripped)) { + NBLOCK++ + BTYPE[NBLOCK] = "skip" + BFIRST[NBLOCK] = nl + BLAST[NBLOCK] = nl + continue + } + start = nl + logical = stripped + while (trailing_backslashes(logical) % 2 == 1 && nl < NLINES) { + logical = substr(logical, 1, length(logical) - 1) + nl++ + next_raw = RAW[nl] + sub(/\r$/, "", next_raw) + sub(/^[ \t]+/, "", next_raw) + logical = logical next_raw + } + # java.util.Properties ignores whitespace before the key; strip it + # so split_kv's separator scan agrees (an indented key used to be + # read as a key whose name started with a space, and a set then + # appended a second definition of the real key). + sub(/^[ \t]+/, "", logical) + split_kv(logical) + NBLOCK++ + BTYPE[NBLOCK] = "entry" + BFIRST[NBLOCK] = start + BLAST[NBLOCK] = nl + BKEY[NBLOCK] = unescape(K_RAW) + # Values stay in their on-disk escaped form. get Prop callers feed + # the result straight back into set, which would corrupt a decoded + # value by re-writing its backslashes as literals; keys are + # unescaped because they are matched against plain names. + BVAL[NBLOCK] = V_RAW + } +} + +function props_set(file, key, enc_val, tmp, bak, cmd, b, first, ln, msg) { + props_load(file) + first = 0 + for (b = 1; b <= NBLOCK; b++) { + if (BTYPE[b] == "entry" && BKEY[b] == key) { + if (first == 0) first = b + else BDROP[b] = 1 + } + } + # Staged rewrite: everything lands in a sibling temp file first, so a + # failure before the copy-back leaves the original untouched. The temp + # file can hold secrets, so it is pre-created 0600 before the first + # write: awk's `>` below would otherwise create it under the process + # umask (usually 0644), leaving auth.admin_pa or auth.token_secret + # briefly group- and world-readable. Truncating an existing file keeps + # its mode, and the chmod after close repairs a stale tmp left behind + # by a crashed run. + tmp = file ".tmp" + system("umask 077 && : > " shquote(tmp)) + for (b = 1; b <= NBLOCK; b++) { + if (BDROP[b]) continue + if (b == first) { + printf "%s=%s\n", key, enc_val > tmp + } else { + for (ln = BFIRST[b]; ln <= BLAST[b]; ln++) + print RAW[ln] > tmp + } + } + if (first == 0) + printf "%s=%s\n", key, enc_val > tmp + close(tmp) + # Copy the completed temp file back onto the original instead of + # renaming it: a rename replaces the inode, which would lose the + # file's permissions (a 0600 config holding secrets would come back + # umask-world-readable), turn a symlinked config into a regular file, + # and fail with EBUSY on a config bind-mounted as a single file — the + # mounted case this path exists for. The copy keeps the inode, mode, + # symlink and mount point. + # + # The copy itself is not atomic and the shell's `>` truncates the + # destination before cat writes a byte, so an ENOSPC or I/O error + # mid-copy used to leave a truncated config on disk — a truncated + # rest-server.properties loses `auth.authenticator` and boots the + # server with authentication off. Snapshot the original first (under + # umask 077 so a backup of a 0644 mounted config never ends up more + # permissive than it started, and chmodded in case a crashed run left + # one behind) and put it back when the copy fails. + system("chmod 600 -- " shquote(tmp)) + 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) { + # Best effort: the destination is already damaged, so restoring it + # from the snapshot comes first, and the temp file is kept for an + # operator who wants to inspect what was being written. + 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") +} + +function props_get(file, key, b) { + props_load(file) + for (b = 1; b <= NBLOCK; b++) { + if (BTYPE[b] == "entry" && BKEY[b] == key) { + print BVAL[b] + return + } + } +} + +function props_get_decoded(file, key, b) { + props_load(file) + for (b = 1; b <= NBLOCK; b++) { + if (BTYPE[b] == "entry" && BKEY[b] == key) { + print unescape(BVAL[b]) Review Comment: 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. ########## hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh: ########## @@ -70,12 +74,141 @@ set_prop() { get_prop_encoded() { local key="$1" file="$2" - local esc_key - esc_key=$(printf '%s' "$key" | sed -e 's/[][(){}.^$*+?|\\/]/\\&/g') - sed -nE \ - "s~^[[:space:]]*${esc_key}([[:space:]]*[:=][[:space:]]*|[[:space:]]+)(.*)$~\\2~p" \ - "${file}" | head -n 1 + PROPS_MODE=get PROPS_KEY="${key}" PROPS_FILE="${file}" \ + awk -f "${PROPS_AWK}" /dev/null +} + +# Decoded read: unescapes the on-disk value the way java.util.Properties +# does, so it compares equal with the snakeyaml-decoded scalar from +# get_yaml_authenticator. The raw get_prop_encoded mode stays for the +# secret round trip, which must replay backslashes byte-for-byte. +get_prop() { Review Comment: Fixed in `b8801a6`, and it came along with the `get-decoded` removal rather than needing the trailing-whitespace gap closed first: ```bash 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. ########## hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh: ########## @@ -70,12 +74,141 @@ set_prop() { get_prop_encoded() { local key="$1" file="$2" - local esc_key - esc_key=$(printf '%s' "$key" | sed -e 's/[][(){}.^$*+?|\\/]/\\&/g') - sed -nE \ - "s~^[[:space:]]*${esc_key}([[:space:]]*[:=][[:space:]]*|[[:space:]]+)(.*)$~\\2~p" \ - "${file}" | head -n 1 + PROPS_MODE=get PROPS_KEY="${key}" PROPS_FILE="${file}" \ + awk -f "${PROPS_AWK}" /dev/null +} + +# Decoded read: unescapes the on-disk value the way java.util.Properties +# does, so it compares equal with the snakeyaml-decoded scalar from +# get_yaml_authenticator. The raw get_prop_encoded mode stays for the +# secret round trip, which must replay backslashes byte-for-byte. +get_prop() { + local key="$1" file="$2" + + PROPS_MODE=get-decoded PROPS_KEY="${key}" PROPS_FILE="${file}" \ + awk -f "${PROPS_AWK}" /dev/null +} + +# First uncommented `authenticator:` inside the gremlin-server.yaml +# authentication block, or on the `authentication:` line itself (a flow +# mapping). snakeyaml resolves duplicate top-level keys to the last one, +# but a mounted file carrying two authentication blocks is pathological; +# report the first and let the mismatch WARN handle it. The scalar is +# cleaned the way snakeyaml reads it — an inline comment (a '#' preceded +# by whitespace), surrounding quotes and padding are stripped — because +# java.util.Properties keeps all of those in the class name. +get_yaml_authenticator() { Review Comment: 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 ```yaml 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
