bitflicker64 commented on code in PR #3192:
URL: https://github.com/apache/hugegraph/pull/3192#discussion_r4084165117
##########
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:
Important: `^[ \t]+authenticator[ \t]*:` matches at any depth inside the
block, and the same-line `match()` on line 107 matches anywhere on the line. An
authenticator that sits under `config` therefore counts as the Gremlin one.
Reproduced at b8801a6: both `authentication:` / ` authenticationHandler:
...` / ` config:` / ` authenticator: com.example.X` and `authentication:
{config: {authenticator: X}}` make `yaml_auth_state` print `named`. With a REST
`auth.authenticator` present, `check_auth_sides` passes. TinkerPop reads
`authentication.authenticator` only, so Gremlin falls back to
AllowAllAuthenticator while REST authenticates.
Please count only a direct child. Record the indentation of the first key
inside the block and accept `authenticator:` only at that indentation, and for
the flow form only at the top level of the braces. Add both layouts to the
`want_state` cases as `nameless`.
##########
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:
Important: `check_auth_sides` treats an `auth.authenticator` with an empty
value as "REST not configured", but this guard has no single meaning for that
case. With `auth.authenticator=` it counts the key as present and skips the
append. With a bare `auth.authenticator` line (no separator) it counts the key
as absent and appends a second definition, which props.awk and HugeConfig
ignore because the first definition wins. In both cases the yaml block is
appended and REST keeps an empty authenticator.
Reproduced at b8801a6 with `host: 0.0.0.0` in gremlin-server.yaml:
- rest-server.properties has `auth.authenticator=`. The check passes, the
yaml gets `authenticator: org.apache.hugegraph.auth.StandardAuthenticator`, and
rest-server.properties keeps `auth.authenticator=`.
- rest-server.properties has a bare `auth.authenticator` line. The check
passes, and the file now holds `auth.authenticator` followed by
`auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator`.
`get_prop_encoded auth.authenticator` returns an empty string.
`HugeAuthenticator.loadAuthenticator` returns null for an empty class, so
REST serves without authentication while Gremlin requires it, and
`auth.admin_pa` has already been written from PASSWORD.
Please handle the defined-but-empty case one way on both sides. Either
`check_auth_sides` refuses a definition with an empty value, or the REST write
goes through `set_prop` (first definition, in place) instead of an append
guarded by grep. Add a test for both spellings.
##########
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:
Important: This guard accepts `authentication:` at any indentation, while
`yaml_auth_state` in the entrypoint now only accepts column 0. A nested mapping
therefore passes `check_auth_sides` as "neither side configured" and then stops
this append, so only the REST side gets written.
Reproduced at b8801a6 with the layout from the new `want_state none` test:
```yaml
host: 0.0.0.0
someFeature:
authentication:
authenticator: com.example.Nested
```
and a rest-server.properties without `auth.authenticator`. `yaml_auth_state`
prints `none`, `check_auth_sides` returns 0, and `enable-auth.sh` appends
`auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator` and
`auth.graph_store=hugegraph` to rest-server.properties but leaves
gremlin-server.yaml unchanged. REST then enforces StandardAuthenticator and
Gremlin stays on TinkerPop's AllowAllAuthenticator. The reply on the column-0
thread says this case lets enable-auth.sh "write both from its own default"; it
writes only one.
Please make this guard match the entrypoint: `grep -Eq
'^authentication[[:blank:]]*:'`. Add a test that runs the real enable-auth.sh
on the nested layout and asserts both files get an authenticator.
--
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]
