bitflicker64 commented on code in PR #3192:
URL: https://github.com/apache/hugegraph/pull/3192#discussion_r3999406016
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -70,12 +74,119 @@ 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]*:/ {
+ inblk = 1
+ line = $0
+ sub(/^[ \t]*authentication[ \t]*:[ \t]*/, "", line)
+ if (match(line, /authenticator[ \t]*:/)) {
+ print scalar(substr(line, RSTART + RLENGTH))
+ exit
+ }
+ next
+ }
+ inblk && /^[ \t]+authenticator[ \t]*:/ {
+ line = $0
+ sub(/^[ \t]*authenticator[ \t]*:[ \t]*/, "", line)
+ print scalar(line)
+ exit
+ }
+ ' "${yaml}"
+}
+
+# A mounted yaml can carry an authentication block whose authenticator
+# cannot be read (an empty or unparseable one). That is not the
+# both-empty case: exporting the default would override an explicit
+# choice that snakeyaml does resolve, so callers treat it as a mismatch.
+has_yaml_authentication_block() {
+ local yaml="./conf/gremlin-server.yaml"
+
+ [[ -f "${yaml}" ]] || return 1
+ grep -Eq '^[[:blank:]]*authentication[[:blank:]]*:' "${yaml}"
+}
+
+# enable-auth.sh appends definitions to files it did not write. On a
+# mounted config those appended definitions are duplicates the two parsers
+# resolve in opposite directions — HugeConfig (commons-configuration) takes
+# the first, snakeyaml takes the last — so Gremlin and REST can land on
+# different authenticators with no error from either. Normalize both sides
+# to one definition of the same authenticator here; enable-auth.sh's
+# per-file guards then make its appends no-ops on anything already set.
+align_auth_config() {
+ local rest_auth yaml_auth
+
+ rest_auth=$(get_prop "auth.authenticator" "${REST_SERVER_CONF}")
+ yaml_auth=$(get_yaml_authenticator)
+ if [[ -z "${yaml_auth}" ]] && has_yaml_authentication_block; then
Review Comment:
⚠️ 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.
##########
hugegraph-server/hugegraph-dist/docker/props.awk:
##########
@@ -0,0 +1,274 @@
+# 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, cmd, b, first, ln) {
+ 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 created 0600 regardless of the umask.
+ tmp = file ".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, and since the temp file is fully written
+ # before the original is truncated, a failed copy still leaves the
+ # previous content on disk.
+ system("chmod 600 -- " shquote(tmp))
Review Comment:
🧹 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.
--
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]
