bitflicker64 commented on code in PR #3192:
URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4132468807
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh:
##########
@@ -34,23 +36,244 @@ 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.
+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}"
+}
+
+AUTHENTICATOR_CLASS="${AUTHENTICATOR_CLASS:-org.apache.hugegraph.auth.StandardAuthenticator}"
- 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}
+# 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}"
- sed -i -e
'$a\auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator' \
- -e '$a\auth.graph_store=hugegraph' ${CONF}/${REST_SERVER_CONF}
+# 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
's/gremlin.graph=org.apache.hugegraph.HugeFactory/gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy/g'
${CONF}/graphs/${GRAPH_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}"
+
+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
+
+if [[ "${GREMLIN_AUTH}" == "unverifiable" ]] &&
+ [[ -z "$(props_get "auth.authenticator" "${CONF}/${REST_SERVER_CONF}")"
]]; 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
+
+# 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}" \
+ 'authentication: {' \
+ " authenticator: ${AUTHENTICATOR_CLASS}," \
+ ' authenticationHandler:
org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler,' \
+ ' config: {tokens: conf/rest-server.properties}' \
+ '}'
+fi
+
+ensure_rest_prop "auth.authenticator" "${AUTHENTICATOR_CLASS}"
"${CONF}/${REST_SERVER_CONF}"
Review Comment:
Minor: The yaml block is appended at line 249 before anything reads
rest-server.properties, so a REST file that props.awk refuses leaves the
one-sided tree that the comment above `append_lines` says this script must not
leave. The early read does not catch it: `ensure_rest_prop` tests `[[ -n
"$(props_get ...)" ]]` (line 124), and `[[` discards the status of the command
substitution, so `props_get`'s `fail` only exits the subshell. Only
`props_set`'s own load then refuses the file. The new props.awk header (lines
30-32) says `props_get` relies on get's nonzero status to refuse, which is not
what happens on this path.
Reproduced at 9ae26c8 in the tarball layout (bin/enable-auth.sh and
bin/props.awk, no yamlscan.awk), over a copy of the shipped conf/ with
`include=other.properties` appended to rest-server.properties:
```
$ bash bin/enable-auth.sh
props.awk: refusing to read or rewrite .../rest-server.properties: it uses
an include directive (line 37), ...
enable-auth.sh: cannot read .../rest-server.properties
props.awk: refusing to read or rewrite .../rest-server.properties: ...
enable-auth.sh: cannot update .../rest-server.properties (exit 1)
gremlin-server.yaml gains the authentication block naming
StandardAuthenticator
rest-server.properties no auth.authenticator
```
After the include line is removed, a rerun in the tarball layout exits 1
with "gremlin-server.yaml has a top-level authentication mapping and this
layout has no yaml reader ...", so the operator has to take the appended block
out of the yaml by hand. The image layout repairs itself on rerun. The same
ordering applies to graphs/hugegraph.properties, which is read last at line
272: with an include there, the run writes both auth sides, leaves
`gremlin.graph=org.apache.hugegraph.HugeFactory` unwrapped, and exits 1. The
Docker entrypoint is not affected, because it reads both files before it calls
this script.
Requested change: read every value the script decides on
(`auth.authenticator`, `auth.graph_store`, `gremlin.graph`) before the first
write, using plain assignments so errexit sees a nonzero status, then do the
yaml append and the property writes. A test with an include line in
rest-server.properties that asserts gremlin-server.yaml is unchanged after the
refusal 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]
