bitflicker64 commented on code in PR #3192: URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4137077577
########## hugegraph-server/hugegraph-dist/src/assembly/static/bin/props.awk: ########## @@ -0,0 +1,411 @@ +# 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, in the on-disk +# escaped form; with PROPS_DECODED=1 print it as java.util.Properties +# would hand it to the server. Exits 0 when K has no definition, which +# prints nothing, and 2 when the file cannot be read or uses an include +# directive. The guards that append a default -- `check_auth_sides` and +# `props_get` -- rely on that nonzero status to refuse rather than treat +# an unreadable file as an absent key. +# 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): +# - physical lines end at \r\n, \n or a bare \r, as in java.util.Properties +# - '#' / '!' comments and blank lines +# - '=' / ':' / whitespace separators, where the whitespace Java counts is +# space, tab and form feed, 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. +# +# A file that carries commons-configuration's `include` directive is refused in +# every mode, with exit status 2: the directive splices another file into this +# one, so "this key is absent" is a question that cannot be answered from this +# file alone, and writing into it could bury the definition the server reads. +# Rewrites stage through two exclusively created 0600 files (`mktemp`) rather +# than a predictable `<file>.tmp` / `<file>.bak`, which a writer in a mounted +# conf directory could have arranged as a symlink before the entrypoint, running +# as root, opened either of them. + +function die(msg) { + printf "props.awk: %s\n", msg > "/dev/stderr" + # 2 for an error. An absent key answers with empty output at exit 0, so an + # error needs a status of its own: a caller that read any nonzero as "not + # there" would append a definition on top of one it failed to read. + exit 2 +} + +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\f]*([#!]|$)/ +} + +# 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. +# Java treats form feed as whitespace on both sides of the separator, so +# `auth.authenticator<FF>=...` is one property here too; reading it as part of +# the key name made a valid mounted configuration invisible to the guards. +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" || c == "\f") { 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\f]+/, "", rest) + c = substr(rest, 1, 1) + if (c == "=" || c == ":") rest = substr(rest, 2) + } + sub(/^[ \t\f]+/, "", rest) + V_RAW = rest +} + +function shquote(s) { + gsub(/'/, "'\\''", s) + return "'" s "'" +} + +# A private temporary file, created exclusively, beside `file`. +# +# The name has to come from mktemp. With a fixed `<file>.tmp` anyone able to +# write in a mounted conf directory could leave that name as a symlink to a +# file elsewhere in the container, and neither the shell redirection that +# pre-created it nor awk's own `>` checks for that: both follow it, so the +# entrypoint, running as root by default, would write auth.admin_pa or +# auth.token_secret through the link and into whatever it points at. An +# exclusive create of an unpredictable name cannot be pre-arranged, and mktemp +# makes the file 0600 whatever the umask says, which is the reason no chmod +# follows it here. +# +# The template is quoted, which is also why no `--` is passed: the argument +# starts at a quote byte, so it can never read as an option. +function make_temp(file, kind, cmd, path) { + path = "" + cmd = "umask 077 && mktemp " shquote(file) "." kind ".XXXXXX" + if ((cmd | getline path) <= 0 || path == "") { + close(cmd) + die("cannot create a private " kind " file beside " file) + } + close(cmd) + return path +} + +# java.util.Properties ends a physical line at \r\n, \n or a bare \r, but +# getline splits on \n alone. A properties file saved with CR-only endings -- +# which java.util.Properties writes for a lone `store()` on some platforms, and +# which a mounted config can arrive with -- therefore reached the parser as one +# enormous record: only its first key was ever seen, and rewriting that key +# replaced the whole record and dropped every later entry, including +# auth.authenticator. So the file is re-scanned for terminators here. +# +# RAW[] keeps the exact bytes of each line and RAWTERM[] its terminator, so a +# rewrite still replays untouched lines byte-for-byte. A file whose last line +# carries no terminator gets a \n, which is what the replay did before. +function scan_records(s, i, n, c, start, term, len, cnt) { + n = length(s) + cnt = 0 + start = 1 + i = 1 + while (i <= n) { + c = substr(s, i, 1) + if (c != "\r" && c != "\n") { i++; continue } + if (c == "\r" && substr(s, i + 1, 1) == "\n") { + term = "\r\n" + len = 2 + } else { + term = c + len = 1 + } + cnt++ + RAW[cnt] = substr(s, start, i - start) + RAWTERM[cnt] = term + start = i + len + i = start + } + if (start <= n) { + cnt++ + RAW[cnt] = substr(s, start) + RAWTERM[cnt] = "" + } + return cnt +} + +# 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, content, nl, stripped, next_raw, start, logical, inc) { + content = "" + while ((rc = (getline raw < file)) > 0) + content = content raw "\n" + if (rc == -1) + die("cannot read " file) + close(file) + + NLINES = scan_records(content) + + NBLOCK = 0 + for (nl = 1; nl <= NLINES; nl++) { + # RAW[] holds one java.util.Properties physical line with its terminator + # already removed, so no CR stripping is needed here. + stripped = RAW[nl] + 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(/^[ \t\f]+/, "", 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\f]+/, "", logical) + split_kv(logical) + NBLOCK++ + BTYPE[NBLOCK] = "entry" + BFIRST[NBLOCK] = start + BLAST[NBLOCK] = nl + BKEY[NBLOCK] = unescape(K_RAW) + # An include directive is not an ordinary property to the server: commons + # configuration splices the named file into this one at this point, so + # auth.authenticator can be defined over there and be invisible from + # here, and which of the two definitions wins follows the spliced + # order rather than the order of this file. Answering that needs the + # parser the server uses, and answering it wrong is how a mounted + # config boots with REST open and Gremlin protected. Commons + # configuration 2 treats both `include` and `includeOptional` as + # directives, and matches the property name case-insensitively, so the + # guard below rejects every spelling a real loader would honour -- not + # just the exact lowercase `include` this first refused. A file that + # uses any of them is refused in every mode and nothing is written. + inc = tolower(BKEY[NBLOCK]) + if (inc == "include" || inc == "includeoptional") + die("refusing to read or rewrite " file ": it uses an include directive (line " start "), which this helper cannot resolve") + # 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, nbs, tail) { + props_load(file) + # A value whose written form leaves an odd number of backslashes at the end + # of the physical line turns the line after it into a continuation of that + # value. The line has to be judged as the server sees it: commons + # configuration trims the line before it looks for the continuation, so + # `abc\ ` -- the spelling encode_prop_value used to give a trailing space -- + # reaches the server as `abc\` and swallows whatever follows it. Measured + # against commons-configuration2 (what HugeConfig extends), the same input + # read back yields no property at all, so a secret written this way never + # reaches the server that is supposed to authenticate with it. The + # entrypoint has to refuse instead of guessing a target. + tail = enc_val + sub(/[ \t\f\r]+$/, "", tail) + nbs = 0 + while (nbs < length(tail) && substr(tail, length(tail) - nbs, 1) == "\\") + nbs++ + if (nbs % 2 == 1) + die("refusing to write " key ": trimmed of its trailing blanks the value ends in a backslash, which would swallow the next line") + 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 private temp file first, so a + # failure before the copy-back leaves the original untouched. The temp file + # holds secrets, so it is created 0600 and exclusively (see make_temp): a + # reused, predictable name is both a disclosure risk under the process umask + # and a path someone else can have arranged already. + tmp = make_temp(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++) { + # Replay the line with the terminator it was read with, so a + # CRLF or CR-only config keeps its endings on lines the + # rewrite does not touch. + msg = RAWTERM[ln] + if (msg == "") msg = "\n" + printf "%s%s", RAW[ln], msg > 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, into a + # second exclusively created 0600 file for the same reason as the temp, + # and put it back when the copy fails. + bak = make_temp(file, "bak") + cmd = "cp -- " shquote(file) " " shquote(bak) + if (system(cmd) != 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) Review Comment: Minor: When the destination can be read but not written, this branch tells the operator the config is damaged when it is untouched, and it leaves a temp file and a snapshot behind on every attempt. `cat -- tmp > file` fails when the shell opens the file for writing (EACCES or EROFS), so nothing is truncated. The restore at line 373 then fails for the same reason, and the code reaches this message. The temp file holds the full new content, including the value being written. Reproduced at f61a0f0 with a `chmod 444` copy of the shipped rest-server.properties, running `PROPS_MODE=set PROPS_KEY=auth.admin_pa PROPS_VALUE_ENCODED=s3cretVALUE` three times: ``` props.awk: cannot copy D/rest-server.properties.tmp.0hAsd8 over D/rest-server.properties; D/rest-server.properties is damaged, previous content is in D/rest-server.properties.bak.jc7JZd (same message twice more) cmp against the original: identical conf dir afterwards: 3 x rest-server.properties.tmp.* 3 x rest-server.properties.bak.* (all 0600) grep s3cretVALUE: all three tmp files ``` In the image this is the first `set_prop auth.token_secret` when rest-server.properties is mounted read-only into a writable conf/, so a container on a restart policy adds one tmp/bak pair per restart, each holding the token secret. Requested change: check that the file is writable before staging (for example `system("test -w " shquote(file))`) and die with "not writable; nothing written" before `make_temp` runs. On a copy-back failure, compare the file with the snapshot before calling it damaged, and remove the temp file when the original is intact. A test that runs set against a read-only file and asserts that the file is unchanged and no `.tmp.*` file remains would cover it. ########## hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh: ########## @@ -34,23 +36,267 @@ 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. PROPS_AWK lets a caller point +# at a different copy; the entrypoint reads that same variable for itself. +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" + +# The Gremlin half of the decision below has to be made by the same reader the +# entrypoint uses. yamlscan.awk is not in bin/: the Dockerfile places it in the +# install home, one above this script, which is where the image layout is +# mirrored in the test tree; PROPS_AWK and YAMLSCAN_AWK cover a caller that +# keeps it elsewhere. The release tarball carries no copy at all, so the +# fallback below has to answer on its own. +YAMLSCAN="" +for candidate in "${YAMLSCAN_AWK:-}" "${TOP}/yamlscan.awk" "${BIN}/yamlscan.awk"; do + if [[ -n "${candidate}" && -f "${candidate}" ]]; then + YAMLSCAN="${candidate}" + break + fi +done + +# 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}" +} + +# The character check for every class this script writes. Java class names need +# no properties escaping; anything else would have to go through the entrypoint's +# encoder first. It is one function rather than a pattern at each call site so +# the early refusal below and the write cannot drift apart. +check_class_name() { + case "$1" in + *[!A-Za-z0-9_\.\$]*) fail "refusing to write an unescaped value: $1" ;; + esac +} + +props_set() { + check_class_name "$2" + 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 on the key being present 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 a presence check 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. +# +# `$4` is the key's current value, read before the first write. Reading it +# here instead -- `[[ -n "$(props_get "$1" "$3")" ]]` -- discarded the status of +# the command substitution, because `[[` only looks at the text, so props_get's +# refusal of a file props.awk cannot read (an `include` directive) exited the +# subshell, the guard saw an empty string, and props_set then tried to write +# that same unreadable file. +ensure_rest_prop() { + [[ -n "$4" ]] && 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}" +} + +AUTHENTICATOR_CLASS="${AUTHENTICATOR_CLASS:-org.apache.hugegraph.auth.StandardAuthenticator}" + +# Refused here, before the first write, rather than at props_set's check when the +# value reaches rest-server.properties. That check ran one statement after the +# yaml block below carried the same class into gremlin-server.yaml, so a value +# with a space in it left the yaml naming a class no server can load beside an +# untouched REST config -- the one-sided tree the entrypoint's check_auth_sides +# then stops the next boot on. It did not self-repair either: once the yaml +# reads as `named` the append is skipped, so re-running with the variable fixed +# or unset wrote the default to REST only, exited 0, and left the two servers +# authenticating to different classes. +check_class_name "${AUTHENTICATOR_CLASS}" + +# Does the Gremlin config carry a top-level `authentication` mapping, and does +# that mapping name an authenticator? This is the same question +# check_auth_sides answers, so it has to go to the same reader: a mapping is +# the server's only at column 0, comment text is not content, and the key may +# be quoted. grep asks it differently -- it sees only the bare spelling, so an +# operator's `"authentication":` block read as absent and a second default +# block was appended beside it, after which the two servers can resolve the key +# in opposite directions while REST keeps its existing authenticator. +# +# The answer lands in GREMLIN_AUTH, and there are three of them because two +# were not enough: whether a mapping exists says nothing about whether it +# names a class, and only the second one decides what is safe to write. +# none no mapping, so the default block below is ours to append; +# named the operator's mapping names a class; +# nameless a mapping that names none, or a document the reader refuses +# rather than guess about -- yamlscan.awk reports both as +# nameless and check_auth_sides stops the boot on them. +# unverifiable only the tarball fallback can produce this: grep saw the key +# but has no reader to tell the three cases apart. +GREMLIN_AUTH="" +gremlin_auth_state() { + local file="$1" + GREMLIN_AUTH="none" + [[ -f "${file}" ]] || return 0 + if [[ -n "${YAMLSCAN}" ]]; then + GREMLIN_AUTH=$(awk -f "${YAMLSCAN}" "${file}") || fail "cannot read ${file}" + return 0 + fi + # No parser in this layout (the plain release tarball). Match what grep can + # honestly answer here: a column-0 key in either quote style or none. The + # nested-mapping and comment cases are the ones that need the real reader, + # and the image, where the entrypoint runs this script, always has it. + if grep -Eq "^[\"']?authentication[\"']?[[:blank:]]*:" "${file}"; then + GREMLIN_AUTH="unverifiable" + fi +} - 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} +# Writing `auth.authenticator` is the one-way door: REST starts enforcing on +# the next boot, and TinkerPop 3.5.1 resolves a mapping that names no +# authenticator to AllowAllAuthenticator, so Gremlin keeps answering without +# credentials. That is the same one-sided state this script exists to avoid, +# arrived at by a route the entrypoint does not guard -- enable-auth.sh ships +# in the release tarball, where nothing calls check_auth_sides first, so the +# refusal has to live here rather than lean on the caller. +gremlin_auth_state "${CONF}/${GREMLIN_SERVER_CONF}" - sed -i -e '$a\auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator' \ - -e '$a\auth.graph_store=hugegraph' ${CONF}/${REST_SERVER_CONF} +if [[ "${GREMLIN_AUTH}" == "nameless" ]]; then + fail "${GREMLIN_SERVER_CONF} carries an authentication mapping that names no authenticator, or a shape the reader refuses; writing ${REST_SERVER_CONF} beside it would enforce on REST and leave Gremlin on its default. Name authentication.authenticator in that mapping, or drop the mapping and let this script write both sides." +fi + +# Every value the decisions below act on is read here, before the first write. +# A plain assignment is what makes the read fatal: errexit sees the status of a +# command substitution on an assignment statement, while `[[ -n "$(props_get +# ...)" ]]` looked only at the text and discarded it. Reading rest-server.properties +# from ensure_rest_prop instead discovered an unreadable file one statement after +# the yaml block had been appended, leaving the yaml naming StandardAuthenticator +# beside a REST config with no `auth.authenticator` -- the one-sided tree this +# script exists to prevent, which in the tarball layout then refuses to repair +# itself: the appended block reads as a mapping, so a rerun after the operator +# fixes the include line stops on the unverifiable branch below. +# graphs/hugegraph.properties was read last of all, so an unreadable graph config +# let the run write both auth sides and leave the factory unwrapped. +REST_AUTHENTICATOR=$(props_get "auth.authenticator" "${CONF}/${REST_SERVER_CONF}") +REST_GRAPH_STORE=$(props_get "auth.graph_store" "${CONF}/${REST_SERVER_CONF}") +GRAPH_FACTORY=$(props_get "gremlin.graph" "${CONF}/graphs/${GRAPH_CONF}") + +if [[ "${GREMLIN_AUTH}" == "unverifiable" ]] && [[ -z "${REST_AUTHENTICATOR}" ]]; then + # The operator already naming a class on the REST side is the one answer + # this layout can act on without a reader: ensure_rest_prop then has + # nothing to write, so both sides stay as the operator left them. + fail "${GREMLIN_SERVER_CONF} has a top-level authentication mapping and this layout has no yaml reader to tell whether it names an authenticator, while ${REST_SERVER_CONF} names none. Set auth.authenticator there yourself, or run this from the server image, which ships the reader." +fi - sed -i 's/gremlin.graph=org.apache.hugegraph.HugeFactory/gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy/g' ${CONF}/graphs/${GRAPH_CONF} +# 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 +# a guard that disagreed on nesting, the entrypoint read the file as `none`, so +# parity held and it called this script, but the guard saw the nested key and +# skipped the append, writing the REST side only -- StandardAuthenticator on +# REST, TinkerPop's AllowAllAuthenticator on Gremlin. +if [[ "${GREMLIN_AUTH}" == "none" ]]; then + append_lines "${CONF}/${GREMLIN_SERVER_CONF}" \ Review Comment: Minor: f61a0f0 moves every read ahead of the first write, but a target that can be read and not written still fails after this append. `append_lines` checks only the yaml's own writability, so with a read-only rest-server.properties the yaml gains the block, `props_set` then fails, and the script exits 1 with the one-sided tree the comment above `append_lines` says it must not leave. Reproduced at f61a0f0 over a copy of the shipped conf/: ``` rest-server.properties chmod a-w, tarball and image layouts: exit 1, gremlin-server.yaml gains the authentication block, rest-server.properties has no auth.authenticator after chmod u+w and a rerun: image layout: exit 0, both sides written tarball layout: "gremlin-server.yaml has a top-level authentication mapping and this layout has no yaml reader ...", exit 1 graphs/hugegraph.properties chmod a-w, both layouts: exit 1, both auth sides written, gremlin.graph=org.apache.hugegraph.HugeFactory left unwrapped ``` In the tarball the operator has to remove the appended block by hand, which is the same outcome the include case had before f61a0f0. The Docker entrypoint is not affected: it writes `auth.token_secret` to both files before it calls this script, so an unwritable file stops the boot there. Requested change: after the reads at lines 253-255, fail if rest-server.properties is not writable and either REST value is empty, and if graphs/hugegraph.properties is not writable and the trimmed factory is HugeFactory. Do this before the append. Extending `refused_read` with a read-only case for each file would cover it. -- 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]
