Adarsh-Me commented on code in PR #3192:
URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4085381605
##########
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
Review Comment:
Confirmed, and still live at `53a5edb` — this one was not covered by the
16:10Z push. Reproduced against the real `enable-auth.sh`:
```
before: state=none check_auth_sides=PASS | after: gremlin=none
rest=[...StandardAuthenticator] => ONE-SIDED
```
Guard anchored to column 0 in 2c9ebaf so the two files answer the same
question:
```sh
if ! grep -Eq '^authentication[[:blank:]]*:'
"${CONF}/${GREMLIN_SERVER_CONF}"; then
```
Test added at `docker/test/test-docker-entrypoint.sh:1066`, on your layout,
running the real script: it first asserts `check_auth_sides` *accepts* the tree
(a case the entrypoint refuses no longer tests anything), then requires
`yaml_auth_state` to come back `named`, the REST class to read back through
props.awk, exactly one `auth.authenticator` definition, and `check_auth_sides`
to still hold afterwards. `someFeature:`'s own block is asserted
byte-identical, so the fix cannot "resolve" this by rewriting the nested
mapping.
Red before the change, green after: `nested authentication mapping:
gremlin-server.yaml is none, not named`.
I did not check a CR-only or CRLF `gremlin-server.yaml` here — MSYS text
mode hides CR bytes, so `^authentication` against those shapes is CI's to
confirm, not mine.
##########
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
Review Comment:
Confirmed, still live at `53a5edb` — also not covered by the 16:10Z push.
All three spellings reproduce against the real script:
```
auth.authenticator= -> gremlin=named rest=[] ONE-SIDED
auth.authenticator -> gremlin=named rest=[] ONE-SIDED
auth.authenticator= (trailing blanks) -> also rest=[]
```
To your framing of the two options, I took the second (write through
`set_prop`) because the first would leave a hole: `check_auth_sides` already
asks for the *value* (`get_prop_encoded` non-empty), so it counts an empty
definition as unconfigured and lets the tree through — refusing it in the guard
would fix the docker path only, while `bin/enable-auth.sh` from the tarball,
which never runs the guard, would still write one side.
Worth adding to your measurement: Java also strips the trailing blanks, so
`auth.authenticator= ` parses to the empty string too. Compared against
`java.util.Properties` directly (jdk 17), for `auth.authenticator`:
```
auth.authenticator=com.example.Foo -> PRESENT len=15
auth.authenticator= -> PRESENT len=0
auth.authenticator -> PRESENT len=0 (bare, no separator)
auth.authenticator= / \t / ':' forms -> PRESENT len=0
(no key) -> ABSENT
```
`props.awk`'s `get` agrees on every row, so the reader was already right and
only the caller's question was wrong.
Fix in 2c9ebaf: both REST defaults go through `props_set`, which replaces
the first definition where it stands and appends when there is none, so the
empty placeholder is rewritten rather than followed by a definition
`first-definition-wins` buries. `props_has` lost its last caller here and is
removed; `PROPS_MODE=has` stays, since it answers a different question and is
covered directly.
Tests at `test-docker-entrypoint.sh:1076` loop all three empty spellings and
assert the class reads back through props.awk, exactly one raw definition, the
placeholder replaced on line 1 (not appended), unrelated lines kept — plus one
case that `auth.authenticator=com.example.OperatorAuth` survives the default
write, so the new `props_set` path cannot clobber an operator. Red before:
`empty definition [auth.authenticator=]: rest-server.properties reads back []`.
##########
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:
Already fixed, and your test asks are in — but not by the push you were
reviewing. `5afbb4a` landed 16:10Z, 45 minutes after this review, and I had not
replied on the thread, so nothing told you the code had moved. That is on me.
`yaml_auth_state` is now `docker/yamlscan.awk`, and both rules you asked for
are there: the indentation of the first key inside the block is recorded and
only that column counts (`yamlscan.awk:248-249`), and for the flow form keys
are read at depth one only (`yamlscan.awk:173`), so `{config: {authenticator:
X}}` is invisible to it. `config:` staying its own map is what the server sees.
Both layouts you named are `want_state` cases as `nameless` already:
`test-docker-entrypoint.sh:902` (block under `config:`) and `:906` (flow inside
a flow), with `:911` keeping `{config: {...}, authenticator: X}` as `named` so
the direct child does not get thrown out with the nested one.
The half of this that was *not* fixed was the mirror image in
`enable-auth.sh` — its guard still accepted `authentication:` at any
indentation, so a nested mapping reached the disagreement from the other side.
Covered in 2c9ebaf on the column-0 thread.
--
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]
