Adarsh-Me commented on code in PR #3192:
URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4113277082
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh:
##########
@@ -34,23 +36,226 @@ 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}"
+}
+
+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}"
+}
+
+AUTHENTICATOR_CLASS="${AUTHENTICATOR_CLASS:-org.apache.hugegraph.auth.StandardAuthenticator}"
+
+# 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
+
+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
- 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}" \
+ 'authentication: {' \
+ " authenticator: ${AUTHENTICATOR_CLASS}," \
Review Comment:
Fixed in `a5de9f6` — the character check is now `check_class_name`, called
as soon as `AUTHENTICATOR_CLASS` is set and before any config is touched;
`props_set` still calls it. Your case exits 1 with both files unchanged and the
yaml still `none`, and the rerun then arms both sides with one class. New
both-layout assertions fail against `b42b1ba`. Suite exits 0; CI is still
`action_required`.
--
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]
