imbajin commented on code in PR #3192: URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4088888173
########## hugegraph-server/hugegraph-dist/docker/yamlscan.awk: ########## @@ -0,0 +1,257 @@ +# 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. +# +# yamlscan.awk -- does the top-level `authentication` mapping of a Gremlin +# server YAML file name an authenticator? Prints exactly one of: +# +# none there is no top-level authentication mapping +# nameless the mapping exists but names no authenticator class +# named the mapping names an authenticator class +# +# The entrypoint asks this one question to decide whether +# rest-server.properties and gremlin-server.yaml configure authentication +# together. Getting it wrong toward "named" is how REST ends up enforcing +# StandardAuthenticator while Gremlin silently falls back to TinkerPop +# AllowAllAuthenticator, so the answer has to follow the same structure +# snakeyaml hands to the server, within the subset of YAML that shipped and +# mounted configs use: +# +# 1. the mapping must start at column 0 -- an `authentication:` nested under +# some other key belongs to that feature, not to the Gremlin server; +# 2. only a direct child `authenticator` counts -- a class reached through +# `authentication.config`, or through any other nested mapping, is not +# the server authenticator, because TinkerPop keeps `config` as its own +# map; +# 3. `#` outside quotes starts a comment: text behind one is not content, +# and a comment-only line is neither a child nor the end of the mapping; +# 4. in a flow mapping the key must sit at depth one between the braces, so +# `{authenticator: X}` names a class while `{config: {authenticator: X}}` +# does not; +# 5. a direct `authenticator` whose value is empty, `null` or `~` names no +# class -- the server reads the key, gets nothing and leaves +# authentication off, which is the nameless case that must be refused. +# +# Quote characters come from sprintf so this file holds no literal apostrophe: +# an awk program written into a single-quoted shell string breaks on one, and +# that has cost this repo twice already. + +function apos() { return sprintf("%c", 39) } +function dquo() { return sprintf("%c", 34) } + +function ltrim(s) { sub(/^[ \t]+/, "", s); return s } +function rtrim(s) { sub(/[ \t]+$/, "", s); return s } +function trim(s) { return rtrim(ltrim(s)) } + +function is_quote(c) { return c == apos() || c == dquo() } + +# Remove an unquoted trailing comment together with the whitespace that has to +# precede the `#` for it to be a comment rather than part of a scalar. +function strip_comment(s, i, n, c, q, prev) { + q = "" + prev = "" + n = length(s) + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (q != "") { + if (c == q) q = "" + } else if (is_quote(c)) { + q = c + } else if (c == "#" && (prev == "" || prev == " " || prev == "\t")) { + return rtrim(substr(s, 1, i - 1)) + } + prev = c + } + return s +} + +# How many whitespace characters open the line, i.e. its block nesting level. +function indent_of(s, i, n, c) { + n = length(s) + i = 1 + while (i <= n) { + c = substr(s, i, 1) + if (c != " " && c != "\t") break + i++ + } + return i - 1 +} + +# One layer of matching quotes off a key or scalar. +function unquote(s, f) { + s = trim(s) + if (length(s) >= 2) { + f = substr(s, 1, 1) + if ((f == apos() || f == dquo()) && substr(s, length(s), 1) == f) + return substr(s, 2, length(s) - 2) + } + return s +} + +# Split `name: value` at the first colon outside quotes that is followed by end +# of line or a space, which is what makes a colon inside `http://host` part of +# the scalar. Results go to K_TXT / V_TXT because awk returns one value. +function split_pair(s, i, n, c, q) { + q = "" + n = length(s) + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (q != "") { + if (c == q) q = "" + continue + } + if (is_quote(c)) { q = c; continue } + if (c != ":") continue + if (i == n || substr(s, i + 1, 1) ~ /^[ \t]/) { + K_TXT = rtrim(substr(s, 1, i - 1)) + V_TXT = ltrim(substr(s, i + 1)) + return 1 + } + } + return 0 +} + +function names_authenticator(k) { return unquote(k) == "authenticator" } + +# An authenticator entry only counts when it actually names a class. +function names_class(v) { + v = trim(v) + return v != "" && v != "null" && v != "~" Review Comment: ‼️ `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. ########## hugegraph-server/hugegraph-dist/docker/yamlscan.awk: ########## @@ -0,0 +1,257 @@ +# 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. +# +# yamlscan.awk -- does the top-level `authentication` mapping of a Gremlin +# server YAML file name an authenticator? Prints exactly one of: +# +# none there is no top-level authentication mapping +# nameless the mapping exists but names no authenticator class +# named the mapping names an authenticator class +# +# The entrypoint asks this one question to decide whether +# rest-server.properties and gremlin-server.yaml configure authentication +# together. Getting it wrong toward "named" is how REST ends up enforcing +# StandardAuthenticator while Gremlin silently falls back to TinkerPop +# AllowAllAuthenticator, so the answer has to follow the same structure +# snakeyaml hands to the server, within the subset of YAML that shipped and +# mounted configs use: +# +# 1. the mapping must start at column 0 -- an `authentication:` nested under +# some other key belongs to that feature, not to the Gremlin server; +# 2. only a direct child `authenticator` counts -- a class reached through +# `authentication.config`, or through any other nested mapping, is not +# the server authenticator, because TinkerPop keeps `config` as its own +# map; +# 3. `#` outside quotes starts a comment: text behind one is not content, +# and a comment-only line is neither a child nor the end of the mapping; +# 4. in a flow mapping the key must sit at depth one between the braces, so +# `{authenticator: X}` names a class while `{config: {authenticator: X}}` +# does not; +# 5. a direct `authenticator` whose value is empty, `null` or `~` names no +# class -- the server reads the key, gets nothing and leaves +# authentication off, which is the nameless case that must be refused. +# +# Quote characters come from sprintf so this file holds no literal apostrophe: +# an awk program written into a single-quoted shell string breaks on one, and +# that has cost this repo twice already. + +function apos() { return sprintf("%c", 39) } +function dquo() { return sprintf("%c", 34) } + +function ltrim(s) { sub(/^[ \t]+/, "", s); return s } +function rtrim(s) { sub(/[ \t]+$/, "", s); return s } +function trim(s) { return rtrim(ltrim(s)) } + +function is_quote(c) { return c == apos() || c == dquo() } + +# Remove an unquoted trailing comment together with the whitespace that has to +# precede the `#` for it to be a comment rather than part of a scalar. +function strip_comment(s, i, n, c, q, prev) { + q = "" + prev = "" + n = length(s) + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (q != "") { + if (c == q) q = "" + } else if (is_quote(c)) { + q = c + } else if (c == "#" && (prev == "" || prev == " " || prev == "\t")) { + return rtrim(substr(s, 1, i - 1)) + } + prev = c + } + return s +} + +# How many whitespace characters open the line, i.e. its block nesting level. +function indent_of(s, i, n, c) { + n = length(s) + i = 1 + while (i <= n) { + c = substr(s, i, 1) + if (c != " " && c != "\t") break + i++ + } + return i - 1 +} + +# One layer of matching quotes off a key or scalar. +function unquote(s, f) { + s = trim(s) + if (length(s) >= 2) { + f = substr(s, 1, 1) + if ((f == apos() || f == dquo()) && substr(s, length(s), 1) == f) + return substr(s, 2, length(s) - 2) + } + return s +} + +# Split `name: value` at the first colon outside quotes that is followed by end +# of line or a space, which is what makes a colon inside `http://host` part of +# the scalar. Results go to K_TXT / V_TXT because awk returns one value. +function split_pair(s, i, n, c, q) { + q = "" + n = length(s) + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (q != "") { + if (c == q) q = "" + continue + } + if (is_quote(c)) { q = c; continue } + if (c != ":") continue + if (i == n || substr(s, i + 1, 1) ~ /^[ \t]/) { + K_TXT = rtrim(substr(s, 1, i - 1)) + V_TXT = ltrim(substr(s, i + 1)) + return 1 + } + } + return 0 +} + +function names_authenticator(k) { return unquote(k) == "authenticator" } + +# An authenticator entry only counts when it actually names a class. +function names_class(v) { + v = trim(v) + return v != "" && v != "null" && v != "~" +} + +# Report and stop. Output happens in END only, because awk runs END after +# `exit` and a second print there would emit two states on one run. +function finish(r) { RESULT = r; exit } + +# Feed one line of a flow collection to the brace scanner. DEPTH counts open +# collections; keys and values are only read at depth one, which is what makes +# a nested mapping under `config` invisible to it. FSET records a direct +# authenticator that names a class. Returns 1 once the outermost collection +# has closed. +function scan_flow(s, i, n, c, q) { + n = length(s) + q = "" + for (i = 1; i <= n; i++) { + c = substr(s, i, 1) + if (q != "") { + if (FST == "key") CUR = CUR c Review Comment: ⚠️ 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. ########## hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh: ########## @@ -243,7 +331,11 @@ fi ./bin/start-hugegraph.sh -j "${JAVA_OPTS:-}" -t "${SERVER_STARTUP_TIMEOUT_S}" # Post-startup cluster stabilization check (hstore only — rocksdb has no partitions) -ACTUAL_BACKEND=$(grep -E '^[[:space:]]*backend[[:space:]]*=' "${GRAPH_CONF}" | head -n 1 | sed 's/.*=//' | tr -d '[:space:]' || true) +# Read through props.awk so a mounted config using the `:` or bare-whitespace +# separator is seen at all, and first-definition-wins matches HugeConfig; the +# grep this replaces only ever accepted `=`. Trailing whitespace is dropped +# here rather than in the reader, which reports the on-disk bytes verbatim. +ACTUAL_BACKEND=$(get_prop_encoded "backend" "${GRAPH_CONF}" | tr -d '[:space:]' || true) Review Comment: ⚠️ `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. ########## hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh: ########## @@ -34,23 +36,142 @@ GREMLIN_SERVER_CONF="gremlin-server.yaml" REST_SERVER_CONF="rest-server.properties" GRAPH_CONF="hugegraph.properties" +fail() { + echo "enable-auth.sh: $*" >&2 + exit 1 +} + +# Reading and writing .properties files goes through props.awk, the same helper +# the docker entrypoint uses, because the keys below can be spelled in every way +# java.util.Properties accepts: `=`/`:`/bare-whitespace separators, a form feed +# as whitespace, `\.` or `\u002e` for the dots, and LF, CRLF or CR line +# terminators. grep and sed see a different file. A legal +# `gremlin\u002egraph=org.apache.hugegraph.HugeFactory` matched no pattern at +# all, so the factory was never wrapped for auth even though both servers were +# told authentication was on -- and the CR byte that the previous pattern had +# to be handed a carriage return for is now handled by the reader itself. +# +# props.awk is packaged in this same bin/ directory by the release assembly, so +# it is present in the tarball and in the image; the entrypoint also exports +# PROPS_AWK when it calls this script. +for candidate in "${PROPS_AWK:-}" "${BIN}/props.awk" "${TOP}/props.awk"; do + if [[ -n "${candidate}" && -f "${candidate}" ]]; then + PROPS_AWK="${candidate}" + break + fi +done +[[ -n "${PROPS_AWK:-}" ]] || fail "props.awk not found beside this script" + +# props_get is the only reader used here, and it treats any nonzero status from +# props.awk as an error: 2 means the file could not be read at all, which must +# not be mistaken for "the key is not there" and answered with a write. +props_get() { + local status=0 value + value=$(PROPS_MODE=get PROPS_DECODED=1 PROPS_KEY="$1" PROPS_FILE="$2" \ + awk -f "${PROPS_AWK}" /dev/null) || status=$? + if (( status > 0 )); then + fail "cannot read $2" + fi + printf '%s' "${value}" +} + +props_set() { + # The only values written here are Java class names, whose characters need + # no properties escaping; anything else would have to go through the + # entrypoint's encoder first. + case "$2" in + *[!A-Za-z0-9_\.\$]*) fail "refusing to write an unescaped value: $2" ;; + esac + PROPS_MODE=set PROPS_KEY="$1" PROPS_VALUE_ENCODED="$2" PROPS_FILE="$3" \ + awk -f "${PROPS_AWK}" /dev/null || fail "cannot update $3" +} + +# Give `$3` its default `$2` for key `$1`, in place, unless it already has a +# value. Guarding with props_has and appending was not the same question: +# `auth.authenticator=` and a bare `auth.authenticator` line both parse to the +# empty string (measured against java.util.Properties, which also strips the +# trailing blanks of `auth.authenticator= `), so props_has reported them as +# answered and the append was skipped -- while the entrypoint's +# check_auth_sides, which asks for the value rather than the key, counted the +# same file as unconfigured. `loadAuthenticator("")` returns null, so REST then +# served without authentication next to a Gremlin that required it. +# +# props_set covers both shapes the guard had to split: with a definition +# present it replaces the first one where it stands (no duplicate for +# first-definition-wins to bury), with none present it appends. +ensure_rest_prop() { + [[ -n "$(props_get "$1" "$3")" ]] && return 0 + props_set "$1" "$2" "$3" +} + # make a backup BAK_CONF="$TOP/conf-bak" if [ ! -d "$BAK_CONF" ]; then - mkdir -p "$BAK_CONF" - cp "${CONF}/${GREMLIN_SERVER_CONF}" "${BAK_CONF}/${GREMLIN_SERVER_CONF}.bak" - cp "${CONF}/${REST_SERVER_CONF}" "${BAK_CONF}/${REST_SERVER_CONF}.bak" - cp "${CONF}/graphs/${GRAPH_CONF}" "${BAK_CONF}/${GRAPH_CONF}.bak" + mkdir -p "$BAK_CONF" || fail "cannot create ${BAK_CONF}" + cp "${CONF}/${GREMLIN_SERVER_CONF}" "${BAK_CONF}/${GREMLIN_SERVER_CONF}.bak" || + fail "cannot back up ${GREMLIN_SERVER_CONF}" + cp "${CONF}/${REST_SERVER_CONF}" "${BAK_CONF}/${REST_SERVER_CONF}.bak" || + fail "cannot back up ${REST_SERVER_CONF}" + cp "${CONF}/graphs/${GRAPH_CONF}" "${BAK_CONF}/${GRAPH_CONF}.bak" || + fail "cannot back up ${GRAPH_CONF}" +fi +# Both writes below skip a side that already carries a real value, so they are +# no-ops on a mounted config or a re-run. Appending unconditionally used to +# create duplicate definitions that the properties parser (first definition +# wins) and the yaml parser (last wins) resolved in opposite directions, leaving +# Gremlin and REST on different authenticators. That is why the REST side goes +# through ensure_rest_prop rather than a presence guard plus an append: a +# presence guard also lets a defined-but-empty key count as answered, and the +# appended default would then be the definition the server never reads. +# +# Appended with `>>` rather than `sed -i '$a\...'`: GNU sed's `$` address never +# matches when the file has no lines, so on an empty mounted config every append +# silently did nothing. `sed -i '$a'` also closed the previous last line for us, +# which `>>` does not, so a file without a trailing newline gets one first. +# +# Every write here has to be seen to succeed. The docker entrypoint runs this +# script and trusts its exit status, and a partially updated tree -- REST +# configured, yaml append refused by a read-only mounted file -- is exactly the +# one-sided state the entrypoint refuses to start with. Without errexit and +# these checks the script exited 0 on that half-done job. +append_lines() { + local file="$1" + shift + if [[ ! -w "${file}" ]]; then + fail "cannot append to ${file}: not writable" + fi + if [[ -s "${file}" && -n "$(tail -c 1 "${file}")" ]]; then + printf '\n' >> "${file}" || fail "cannot append to ${file}" + fi + printf '%s\n' "$@" >> "${file}" || fail "cannot append to ${file}" +} - sed -i -e '$a\authentication: {' \ - -e '$a\ authenticator: org.apache.hugegraph.auth.StandardAuthenticator,' \ - -e '$a\ authenticationHandler: org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler,' \ - -e '$a\ config: {tokens: conf/rest-server.properties}' \ - -e '$a\}' ${CONF}/${GREMLIN_SERVER_CONF} +AUTHENTICATOR_CLASS="${AUTHENTICATOR_CLASS:-org.apache.hugegraph.auth.StandardAuthenticator}" + +# Only a column-0 `authentication` mapping is the Gremlin server's, which is the +# rule yamlscan.awk applies to decide the same thing for check_auth_sides. With +# `[[:blank:]]*` here the two disagreed on a config that nests `authentication` +# under another feature: the entrypoint read it as `none`, so parity held and it +# called this script, but this guard saw the nested key and skipped the append, +# writing the REST side only -- StandardAuthenticator on REST, TinkerPop's +# AllowAllAuthenticator on Gremlin. +if ! grep -Eq '^authentication[[:blank:]]*:' "${CONF}/${GREMLIN_SERVER_CONF}"; then Review Comment: ⚠️ `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. -- 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]
