Adarsh-Me commented on code in PR #3192:
URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4173766941
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -70,12 +74,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
+}
+
+# What the top-level authentication mapping of gremlin-server.yaml says about
+# authentication, as one of three states:
+#
+# none no such mapping
+# named the mapping carries an authenticator
+# nameless the mapping exists but names no authenticator
+#
+# Only presence is asked for, never the class: the entrypoint does not copy a
+# value between the two files any more, so quotes, inline comments and flow
+# mappings stay snakeyaml's business instead of becoming a parser here. The
+# key must start at column 0 — an `authentication:` nested under another
+# mapping belongs to that feature, not to the Gremlin server, and reading it as
+# the Gremlin one would let an unrelated class decide whether REST is
+# authenticated while Gremlin stayed on TinkerPop's AllowAllAuthenticator.
+yaml_auth_state() {
+ local yaml="./conf/gremlin-server.yaml"
+
+ [[ -f "${yaml}" ]] || { echo "none"; return 0; }
+ awk '
+ /^authentication[ \t]*:/ {
+ inblk = 1
+ have = 1
+ # A flow mapping keeps the authenticator on the same line as the
+ # key, so it has to count there too; missing it would report a
+ # configured mapping as nameless and refuse a valid deployment.
+ if (match($0, /authenticator[ \t]*:/)) { named = 1; exit }
Review Comment:
No longer reproduces at head 0be7fde. The scanner strips an unquoted
trailing comment before any line can count as a child, so your shape prints
nameless, byte-identical to the comment-free control; SnakeYAML 1.27 on the
same bytes also says nameless. With REST holding that class the wrapper exits 1
instead of passing parity.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -70,12 +74,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
+}
+
+# What the top-level authentication mapping of gremlin-server.yaml says about
+# authentication, as one of three states:
+#
+# none no such mapping
+# named the mapping carries an authenticator
+# nameless the mapping exists but names no authenticator
+#
+# Only presence is asked for, never the class: the entrypoint does not copy a
+# value between the two files any more, so quotes, inline comments and flow
+# mappings stay snakeyaml's business instead of becoming a parser here. The
+# key must start at column 0 — an `authentication:` nested under another
+# mapping belongs to that feature, not to the Gremlin server, and reading it as
+# the Gremlin one would let an unrelated class decide whether REST is
+# authenticated while Gremlin stayed on TinkerPop's AllowAllAuthenticator.
+yaml_auth_state() {
+ local yaml="./conf/gremlin-server.yaml"
+
+ [[ -f "${yaml}" ]] || { echo "none"; return 0; }
+ awk '
+ /^authentication[ \t]*:/ {
+ inblk = 1
+ have = 1
+ # A flow mapping keeps the authenticator on the same line as the
+ # key, so it has to count there too; missing it would report a
+ # configured mapping as nameless and refuse a valid deployment.
+ if (match($0, /authenticator[ \t]*:/)) { named = 1; exit }
+ next
+ }
+ # Any other column-0 key ends the mapping. A blank or whitespace-only
+ # line does not, because YAML does not close a mapping on an empty
line.
+ inblk && /^[^ \t]/ { inblk = 0 }
Review Comment:
No longer reproduces at head 0be7fde. A comment-only line strips to empty
before the root-sibling test, so it does not close the mapping: your fixture
answers named, and with a matching REST authenticator check_auth_sides returns
0, so startup is not blocked. Measured on gawk 5.0.0 here.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -70,12 +74,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
+}
+
+# What the top-level authentication mapping of gremlin-server.yaml says about
+# authentication, as one of three states:
+#
+# none no such mapping
+# named the mapping carries an authenticator
+# nameless the mapping exists but names no authenticator
+#
+# Only presence is asked for, never the class: the entrypoint does not copy a
+# value between the two files any more, so quotes, inline comments and flow
+# mappings stay snakeyaml's business instead of becoming a parser here. The
+# key must start at column 0 — an `authentication:` nested under another
+# mapping belongs to that feature, not to the Gremlin server, and reading it as
+# the Gremlin one would let an unrelated class decide whether REST is
+# authenticated while Gremlin stayed on TinkerPop's AllowAllAuthenticator.
+yaml_auth_state() {
+ local yaml="./conf/gremlin-server.yaml"
+
+ [[ -f "${yaml}" ]] || { echo "none"; return 0; }
+ awk '
+ /^authentication[ \t]*:/ {
+ inblk = 1
+ have = 1
+ # A flow mapping keeps the authenticator on the same line as the
+ # key, so it has to count there too; missing it would report a
+ # configured mapping as nameless and refuse a valid deployment.
+ if (match($0, /authenticator[ \t]*:/)) { named = 1; exit }
+ next
+ }
+ # Any other column-0 key ends the mapping. A blank or whitespace-only
+ # line does not, because YAML does not close a mapping on an empty
line.
+ inblk && /^[^ \t]/ { inblk = 0 }
+ inblk && /^[ \t]+authenticator[ \t]*:/ { named = 1; exit }
Review Comment:
No longer reproduces at head 0be7fde. Only a child at the mapping own
indentation counts, so a class nested under config is invisible: both the flow
spelling and the block spelling print nameless, and the wrapper then refuses to
arm one side alone. Verified against the fetched head file, not the scratch
copy.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -147,6 +221,9 @@ elif [[ -n "${AUTH_TOKEN_SECRET_ENCODED}" ]]; then
fi
if [[ -n "${PASSWORD:-}" ]]; then
set_prop "auth.admin_pa" "${PASSWORD}" "${REST_SERVER_CONF}"
+ # A refusal here exits the entrypoint under set -e, so enable-auth.sh can
+ # never run one-sided after it.
+ check_auth_sides
Review Comment:
Agreed, and this one is fixed: head 0be7fde calls the parity check
unconditionally at line 284 of docker-entrypoint.sh, ahead of the password
branch. With PASSWORD unset and a REST-only mounted config it exits 1 and
writes nothing; a nameless mapping with REST set is refused the same way.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh:
##########
@@ -41,16 +41,61 @@ if [ ! -d "$BAK_CONF" ]; then
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"
+fi
+
+# The appends below are guarded per file and match only an absent or still
+# commented-out definition, so they are no-ops on any config that already
+# carries authentication (e.g. a mounted one, or a re-run of this script).
+# The guards accept every spelling java.util.Properties reads as the key —
+# '=' or ':' or bare-whitespace separators, leading whitespace and
+# backslash-escaped dots — and the gremlin.graph flip tolerates CRLF
+# endings, which a mounted config saved on Windows carries. 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.
+
+# 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 below silently did nothing. Neither the REST
+# `auth.authenticator` nor the yaml `authentication:` block was written,
+# while the entrypoint had already applied PASSWORD and init-store had run
+# in auth mode — the servers then came up unauthenticated with no error.
+# `sed -i '$a'` also closed the previous last line for us, which `>>` does
+# not, so a file without a trailing newline gets one first.
+append_lines() {
+ local file="$1"
+ shift
+ if [[ -s "${file}" && -n "$(tail -c 1 "${file}")" ]]; then
+ printf '\n' >> "${file}"
+ fi
+ printf '%s\n' "$@" >> "${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}
+if ! grep -Eq '^[[:blank:]]*authentication[[:blank:]]*:'
"${CONF}/${GREMLIN_SERVER_CONF}"; then
+ append_lines "${CONF}/${GREMLIN_SERVER_CONF}" \
+ 'authentication: {' \
+ " authenticator: ${AUTHENTICATOR_CLASS}," \
+ ' authenticationHandler:
org.apache.hugegraph.auth.WsAndHttpBasicAuthHandler,' \
+ ' config: {tokens: conf/rest-server.properties}' \
+ '}'
+fi
+
+if ! grep -Eq
'^[[:blank:]]*auth[\\]?\.authenticator[[:blank:]]*([:=]|[[:blank:]])'
"${CONF}/${REST_SERVER_CONF}"; then
+ append_lines "${CONF}/${REST_SERVER_CONF}"
"auth.authenticator=${AUTHENTICATOR_CLASS}"
+fi
- sed -i -e
'$a\auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator' \
- -e '$a\auth.graph_store=hugegraph' ${CONF}/${REST_SERVER_CONF}
+if ! grep -Eq
'^[[:blank:]]*auth[\\]?\.graph_store[[:blank:]]*([:=]|[[:blank:]])'
"${CONF}/${REST_SERVER_CONF}"; then
+ append_lines "${CONF}/${REST_SERVER_CONF}" 'auth.graph_store=hugegraph'
+fi
- sed -i
's/gremlin.graph=org.apache.hugegraph.HugeFactory/gremlin.graph=org.apache.hugegraph.auth.HugeFactoryAuthProxy/g'
${CONF}/graphs/${GRAPH_CONF}
+# GNU grep reads \r in a pattern as the letter r, so the carriage return a
+# CRLF line ends with is embedded as a byte: without it the anchored guard
+# misses a mounted CRLF config and the factory is never wrapped for auth
+# although both servers already believe authentication is on.
+CR=$'\r'
+if grep -Eq
"^[[:blank:]]*gremlin[\\\\]?\\.graph[[:blank:]]*([:=]|[[:blank:]])[[:blank:]]*org\\.apache\\.hugegraph\\.HugeFactory[[:blank:]]*${CR}?$"
"${CONF}/graphs/${GRAPH_CONF}"; then
Review Comment:
No longer reproduces at head 0be7fde. The grep/sed pair is gone and the
rewrite goes through the properties helper, which decodes that Unicode escape
when matching keys. Ran your spelling on the head script: the factory value
became the auth proxy, neighbouring keys stayed intact, exit 0.
##########
hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh:
##########
@@ -41,16 +41,61 @@ if [ ! -d "$BAK_CONF" ]; then
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"
+fi
+
+# The appends below are guarded per file and match only an absent or still
+# commented-out definition, so they are no-ops on any config that already
+# carries authentication (e.g. a mounted one, or a re-run of this script).
+# The guards accept every spelling java.util.Properties reads as the key —
+# '=' or ':' or bare-whitespace separators, leading whitespace and
+# backslash-escaped dots — and the gremlin.graph flip tolerates CRLF
+# endings, which a mounted config saved on Windows carries. 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.
+
+# 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 below silently did nothing. Neither the REST
+# `auth.authenticator` nor the yaml `authentication:` block was written,
+# while the entrypoint had already applied PASSWORD and init-store had run
+# in auth mode — the servers then came up unauthenticated with no error.
+# `sed -i '$a'` also closed the previous last line for us, which `>>` does
+# not, so a file without a trailing newline gets one first.
+append_lines() {
+ local file="$1"
+ shift
+ if [[ -s "${file}" && -n "$(tail -c 1 "${file}")" ]]; then
+ printf '\n' >> "${file}"
+ fi
+ printf '%s\n' "$@" >> "${file}"
Review Comment:
No longer reproduces at head 0be7fde. errexit is on and every append is
checked, with writability guarded before any file is touched: a read-only yaml
file fails with exit 1 and nothing written, and the read-only REST and graph
cases are refused the same way.
--
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]
