Copilot commented on code in PR #3192: URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4099734960
########## hugegraph-server/Dockerfile: ########## @@ -70,6 +70,9 @@ RUN sed -i "s/^restserver.url.*$/restserver.url=http:\/\/0.0.0.0:8080/g" ./conf/ COPY hugegraph-server/hugegraph-dist/docker/scripts/remote-connect.groovy ./scripts COPY hugegraph-server/hugegraph-dist/docker/scripts/detect-storage.groovy ./scripts COPY hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh . +# props.awk needs no COPY: it ships in the assembly bin/ above, which is also +# where bin/enable-auth.sh finds it. yamlscan.awk serves only the entrypoint. +COPY hugegraph-server/hugegraph-dist/docker/yamlscan.awk . Review Comment: These helpers are copied inside `/hugegraph-server`, but the Dockerfile declares that entire path as a volume (line 79). A user upgrading a container with an existing named volume or bind-mounting the install directory will hide the newly copied `bin/props.awk` and root `yamlscan.awk`; the new entrypoint then exits at startup with “not found” before it can process the mounted config. Keep the runtime helpers in a path outside the application volume (and point both scripts there), or provide an equivalent upgrade-safe fallback. ########## hugegraph-server/Dockerfile-hstore: ########## @@ -72,6 +72,9 @@ RUN cd /hugegraph-server/conf/graphs \ COPY hugegraph-server/hugegraph-dist/docker/scripts/remote-connect.groovy ./scripts #COPY hugegraph-server/hugegraph-dist/docker/scripts/detect-storage.groovy ./scripts COPY hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh . +# props.awk needs no COPY: it ships in the assembly bin/ above, which is also +# where bin/enable-auth.sh finds it. yamlscan.awk serves only the entrypoint. +COPY hugegraph-server/hugegraph-dist/docker/yamlscan.awk . Review Comment: These helpers are copied inside `/hugegraph-server`, but the Dockerfile declares that entire path as a volume (line 81). A user upgrading a container with an existing named volume or bind-mounting the install directory will hide the newly copied `bin/props.awk` and root `yamlscan.awk`; the new entrypoint then exits at startup with “not found” before it can process the mounted config. Keep the runtime helpers in a path outside the application volume (and point both scripts there), or provide an equivalent upgrade-safe fallback. ########## hugegraph-server/hugegraph-dist/docker/yamlscan.awk: ########## @@ -0,0 +1,405 @@ +# 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 sit at the indentation of the document root -- an +# `authentication:` nested under some other key belongs to that feature, +# not to the Gremlin server, while a root mapping written indented below a +# document marker still counts because the server reads it as the root; +# 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; +# 6. a mapping is read to its end before it is answered, because the server +# sees the whole node: a direct `authenticator` defined twice is refused +# rather than settled by whoever met it first; +# 6b. the file is read to its end, not to the first mapping, because two +# top-level `authentication` mappings resolve to the last one (or are +# rejected outright). Answering from the first reported `named` for a +# server left on AllowAllAuthenticator by the empty second mapping, so a +# duplicate root mapping is refused rather than guessed at; +# 7. YAML ends a line at CR, LF or CRLF, so a CR that has survived the +# comment being stripped is line noise, not part of a key or a value. +# Reading `authentication:\r` as no key at all reported a Gremlin mapping +# that names a class as absent, which is the direction that leaves REST +# open while Gremlin authenticates. +# +# 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, and the +# answer has to be what snakeyaml hands the server rather than what the bytes +# look like. The unsafe direction is `named` for a config that leaves Gremlin +# on AllowAllAuthenticator while REST enforces, so anything this scanner cannot +# resolve to a class is refused instead of guessed at: +# +# - a plain scalar that resolves to null in any spelling, and YAML resolves +# null case-insensitively (null, Null, NULL, nUll) as well as to ~, names +# no class; +# - a leading `!` makes the tag, not the text, decide the type: !!null is the +# explicit spelling of empty and every other tag is a type not resolvable +# here, so neither counts; +# - a quoted scalar is a string and never null, but `""` and the empty single +# quoted form are the empty string, and loadAuthenticator("") returns null, +# which is the same no-authenticator state; +# - `&label value` is an anchor: the label is not part of the value, so the +# text after it decides, and `&label` alone anchors an empty node, which is +# the explicit spelling of null; +# - `*label` is an alias whose class lives in another node. This scanner +# does not resolve nodes, so an alias is refused rather than read as a +# class name -- `authenticator: &noAuth null` is a valid document whose +# value is null, and calling it named is the exact mistake this guards. +# - an unterminated quote is not a scalar at all. +function names_class(v, first, last, body, rest) { + v = trim(v) + if (v == "") return 0 + first = substr(v, 1, 1) + if (first == "!") return 0 + if (first == "*") return 0 + if (first == "&") { + rest = trim(substr(v, 2)) + sub(/^[^ \t]*/, "", rest) + return names_class(trim(rest)) + } + if (first == apos() || first == dquo()) { + if (length(v) < 2) return 0 + last = substr(v, length(v), 1) + if (last != first) return 0 + body = trim(substr(v, 2, length(v) - 2)) + return body != "" + } + if (v == "~") return 0 + if (tolower(v) == "null") return 0 + return 1 Review Comment: A YAML block scalar marker is treated as a named authenticator, although `authenticator: |` or `authenticator: >` with no content resolves to an empty string and leaves Gremlin on its no-authenticator default. If REST has an authenticator, the boolean parity check then accepts this one-sided effective configuration. Treat block scalars as unresolved and refuse them unless the scanner can parse their content. ########## hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh: ########## @@ -243,7 +358,15 @@ 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 `=`. Decoded, because this is compared +# against a literal: the JVM reads `backend=h\u0073tore` as hstore while the +# on-disk bytes are not that string, and the comparison deciding to skip +# wait-partition.sh is how startup continued before partitions were assigned. +# Trailing whitespace is dropped here rather than in the reader, which reports +# the value verbatim apart from the escapes java.util.Properties resolves. +ACTUAL_BACKEND=$(get_prop_decoded "backend" "${GRAPH_CONF}" | tr -d '[:space:]' || true) Review Comment: The `|| true` converts every `props.awk` error into an empty backend. For example, a graph properties file containing the explicitly refused `include` directive makes this assignment succeed with `ACTUAL_BACKEND=''`, so an HStore server skips `wait-partition.sh` and can start before its partitions are assigned. Do not swallow this reader error; propagate it (or fail explicitly) instead of treating an unreadable config as RocksDB. ########## hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh: ########## @@ -70,12 +107,82 @@ 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 +} + +# The value as java.util.Properties hands it to the server, escapes resolved. +# Compare against this, not the on-disk bytes: `backend=h\u0073tore` is a legal +# spelling of hstore that the JVM reads as hstore and a string compare against +# the raw text does not. +get_prop_decoded() { + local key="$1" file="$2" + + PROPS_MODE=get PROPS_DECODED=1 PROPS_KEY="${key}" PROPS_FILE="${file}" \ + awk -f "${PROPS_AWK}" /dev/null +} + +# What the top-level authentication mapping of gremlin-server.yaml says about +# authentication, as one of three states: none, named, nameless. +# +# The question and its answer live in yamlscan.awk, which reads the mapping the +# way snakeyaml presents it to the server: only a column-0 `authentication` +# mapping counts, only its direct `authenticator` child names a class, comment +# text never counts as content, and a nested `config.authenticator` belongs to +# the config map rather than to the server. Those distinctions are the whole +# decision -- an earlier grep-shaped version of this function reported `named` +# for `authentication: {} # authenticator: X` and for a class nested under +# `config:`, which passed the REST/Gremlin parity check while Gremlin was +# running on AllowAllAuthenticator. +yaml_auth_state() { + local yaml="./conf/gremlin-server.yaml" + + [[ -f "${yaml}" ]] || { echo "none"; return 0; } + awk -f "${YAMLSCAN}" "${yaml}" +} + +# Authentication has to be configured on both sides or on neither. A mounted +# config carrying only one is refused rather than completed: the entrypoint +# cannot know which class the operator means, and finishing the other side from +# a guessed default is how Gremlin ends up on AllowAllAuthenticator while REST +# enforces StandardAuthenticator. A mapping that names no authenticator is +# refused by itself, because enable-auth.sh guards on the presence of that +# mapping and would otherwise write only the REST side. +check_auth_sides() { + local rest=0 yaml=0 state rest_value + + state=$(yaml_auth_state) + if [[ "${state}" == "nameless" ]]; then + log "ERROR: gremlin-server.yaml carries a top-level authentication" \ + "mapping that names no authenticator; add an authenticator entry" \ + "to it or remove the mapping, then restart." + return 1 + fi + # A nonzero status here means the reader could not answer at all -- props.awk + # exits 2 rather than guess, e.g. for a file that splices another one with an + # commons-configuration `include`. Calling that "configured on one side" + # would send the operator to the wrong file, and calling it absent is the + # direction that lets REST start open beside a Gremlin that authenticates. + if ! rest_value=$(get_prop_encoded "auth.authenticator" "${REST_SERVER_CONF}"); then + log "ERROR: cannot read auth.authenticator from ${REST_SERVER_CONF};" \ + "see the reason above, fix it, then restart." + return 1 + fi + if [[ -n "${rest_value}" ]]; then + rest=1 + fi + if [[ "${state}" == "named" ]]; then + yaml=1 + fi + if (( rest == yaml )); then + return 0 Review Comment: This check only compares whether each side is non-empty, so `auth.authenticator=ClassA` and a YAML `authentication.authenticator: ClassB` return success with no warning. The PR contract says genuinely different authenticators must be reported with a WARN and left explicitly visible; please compare the effective class names (or update the stated behavior/tests) before accepting this parity result. This issue also appears on line 178 of the same file. -- 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]
