imbajin commented on code in PR #3119:
URL: https://github.com/apache/hugegraph/pull/3119#discussion_r3678544703
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -54,29 +264,134 @@ migrate_env "PD_PEERS" "HG_SERVER_PD_PEERS"
# ── Map env → properties file ─────────────────────────────────────────
[[ -n "${HG_SERVER_BACKEND:-}" ]] && set_prop "backend"
"${HG_SERVER_BACKEND}" "${GRAPH_CONF}"
[[ -n "${HG_SERVER_PD_PEERS:-}" ]] && set_prop "pd.peers"
"${HG_SERVER_PD_PEERS}" "${GRAPH_CONF}"
+if [[ -n "${HG_SERVER_INIT_STORE_ENABLED:-}" ]]; then
+ # Canonicalize before writing, so the property file only ever holds `true`
+ # or `false` and cannot be read differently by the shell and the server
+ if ! HG_SERVER_INIT_STORE_ENABLED=$(to_bool
"${HG_SERVER_INIT_STORE_ENABLED}"); then
+ log "ERROR: HG_SERVER_INIT_STORE_ENABLED must be a boolean, got
'${HG_SERVER_INIT_STORE_ENABLED}'"
+ exit 1
+ fi
+ set_prop "init_store.enabled" "${HG_SERVER_INIT_STORE_ENABLED}"
"${REST_SERVER_CONF}"
+fi
# ── Build wait-storage env ─────────────────────────────────────────────
WAIT_ENV=()
[[ -n "${HG_SERVER_BACKEND:-}" ]] &&
WAIT_ENV+=("hugegraph.backend=${HG_SERVER_BACKEND}")
[[ -n "${HG_SERVER_PD_PEERS:-}" ]] &&
WAIT_ENV+=("hugegraph.pd.peers=${HG_SERVER_PD_PEERS}")
-# ── Init store (once) ─────────────────────────────────────────────────
-if [[ ! -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" ]]; then
+wait_storage() {
if (( ${#WAIT_ENV[@]} > 0 )); then
env "${WAIT_ENV[@]}" ./bin/wait-storage.sh
else
./bin/wait-storage.sh
fi
+}
+
+# ── Init store (once) ─────────────────────────────────────────────────
+# With `init_store.enabled=false` (distributed PD/HStore) init-store is a
no-op:
+# storage owns the metadata and the admin account is created on server startup
+# from `auth.admin_pa`. A requested PASSWORD is therefore written to that
+# property rather than piped into init-store.sh, where it would be read and
+# discarded without creating the account.
+#
+# The value is read back from the config file rather than from the env var, so
+# that a rest-server.properties mounted with the property already set behaves
+# the same as `HG_SERVER_INIT_STORE_ENABLED` (the env mapping above has already
+# been applied, so env still wins).
+INIT_STORE_ENABLED=$(get_prop "init_store.enabled" "${REST_SERVER_CONF}")
+if [[ -n "${INIT_STORE_ENABLED}" ]]; then
+ if ! INIT_STORE_ENABLED=$(to_bool "${INIT_STORE_ENABLED}"); then
+ log "ERROR: init_store.enabled in ${REST_SERVER_CONF} must be a
boolean," \
+ "got '${INIT_STORE_ENABLED}'"
+ exit 1
+ fi
+fi
+
+# A mounted configuration can enable REST authentication without carrying the
+# matching Gremlin handler or auth graph proxy. Complete all three configs for
+# every configured authenticator, whether or not Docker supplied a PASSWORD.
+AUTHENTICATOR=$(get_prop "auth.authenticator" "${REST_SERVER_CONF}")
+if [[ -n "${PASSWORD:-}" || -n "${AUTHENTICATOR}" ]]; then
+ ensure_auth_enabled
+ AUTHENTICATOR=$(get_prop "auth.authenticator" "${REST_SERVER_CONF}")
+fi
+
+AUTH_STATE=""
+AUTH_INIT_REQUIRED=false
+if [[ -n "${AUTHENTICATOR}" ]]; then
+ AUTH_STATE=$(printf '%s\n%s\n%s' \
+ "${AUTHENTICATOR}" \
+ "$(get_prop "auth.remote_url" "${REST_SERVER_CONF}")" \
+ "$(get_prop "auth.graph_store" "${REST_SERVER_CONF}")")
+ STORED_AUTH_STATE=$(cat \
+ "${DOCKER_FOLDER}/${AUTH_INIT_STATE_FILE}" 2>/dev/null || true)
+ if [[ -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" &&
+ "${STORED_AUTH_STATE}" != "${AUTH_STATE}" ]]; then
+ AUTH_INIT_REQUIRED=true
+ fi
+fi
+
+if [[ "${INIT_STORE_ENABLED:-true}" == "false" ]]; then
+ log "init-store disabled; validating the no-op configuration"
+
+ # Let InitStore make the type-aware decision about whether this effective
+ # authenticator needs the built-in admin and whether the configured auth
+ # graph can read the PD-created account. The gate returns before backend or
+ # plugin registration, so this invocation performs validation only.
+ ./bin/init-store.sh
+
+ # Still wait: the server needs the storage side reachable at startup even
+ # though nothing is initialized here
+ wait_storage
+
+ if [[ -n "${PASSWORD:-}" ]]; then
+ log "enabling auth mode, admin password applied via auth.admin_pa"
+ # TODO: auth.admin_pa only applies when the admin account is first
+ # created, so changing PASSWORD on a later restart keeps the old one.
+ if ! ESCAPED_PASSWORD=$(props_escape "${PASSWORD}"); then
+ log "ERROR: PASSWORD must be valid UTF-8"
+ exit 1
+ fi
+ if ! chmod 600 "${REST_SERVER_CONF}"; then
+ log "ERROR: cannot protect ${REST_SERVER_CONF} before writing
auth.admin_pa"
+ exit 1
+ fi
+ if ! set_prop "auth.admin_pa" "${ESCAPED_PASSWORD}" \
+ "${REST_SERVER_CONF}"; then
+ log "ERROR: cannot write auth.admin_pa to ${REST_SERVER_CONF}"
+ exit 1
+ fi
+ fi
+ # No init flag is written here: nothing was initialized, so a later run
+ # with init-store enabled must still perform the real initialization.
+elif [[ ! -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" ||
+ "${AUTH_INIT_REQUIRED}" == "true" ]]; then
+ if [[ -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" ]]; then
+ log "authentication configuration changed; running init-store once"
+ fi
+ wait_storage
if [[ -z "${PASSWORD:-}" ]]; then
- log "init hugegraph with non-auth mode"
- ./bin/init-store.sh
+ if [[ -n "${AUTHENTICATOR}" ]]; then
+ log "init hugegraph with configured auth.admin_pa"
+ ./bin/init-store.sh --use-configured-admin-password
Review Comment:
‼️ This path treats the option fallback as an explicitly configured secret.
With `auth.authenticator=StandardAuthenticator` but no `PASSWORD` or
`auth.admin_pa`, `InitStore` reads `ServerOptions.ADMIN_PA`, whose public
default is `pa`, and creates the built-in `admin/pa` account; the new smoke
test exercises exactly this missing-password case. Please require an explicitly
present, non-empty admin password before using
`--use-configured-admin-password` (or fail closed), and add a regression
proving the fallback cannot create an administrator.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -54,29 +264,134 @@ migrate_env "PD_PEERS" "HG_SERVER_PD_PEERS"
# ── Map env → properties file ─────────────────────────────────────────
[[ -n "${HG_SERVER_BACKEND:-}" ]] && set_prop "backend"
"${HG_SERVER_BACKEND}" "${GRAPH_CONF}"
[[ -n "${HG_SERVER_PD_PEERS:-}" ]] && set_prop "pd.peers"
"${HG_SERVER_PD_PEERS}" "${GRAPH_CONF}"
+if [[ -n "${HG_SERVER_INIT_STORE_ENABLED:-}" ]]; then
+ # Canonicalize before writing, so the property file only ever holds `true`
+ # or `false` and cannot be read differently by the shell and the server
+ if ! HG_SERVER_INIT_STORE_ENABLED=$(to_bool
"${HG_SERVER_INIT_STORE_ENABLED}"); then
+ log "ERROR: HG_SERVER_INIT_STORE_ENABLED must be a boolean, got
'${HG_SERVER_INIT_STORE_ENABLED}'"
+ exit 1
+ fi
+ set_prop "init_store.enabled" "${HG_SERVER_INIT_STORE_ENABLED}"
"${REST_SERVER_CONF}"
+fi
# ── Build wait-storage env ─────────────────────────────────────────────
WAIT_ENV=()
[[ -n "${HG_SERVER_BACKEND:-}" ]] &&
WAIT_ENV+=("hugegraph.backend=${HG_SERVER_BACKEND}")
[[ -n "${HG_SERVER_PD_PEERS:-}" ]] &&
WAIT_ENV+=("hugegraph.pd.peers=${HG_SERVER_PD_PEERS}")
-# ── Init store (once) ─────────────────────────────────────────────────
-if [[ ! -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" ]]; then
+wait_storage() {
if (( ${#WAIT_ENV[@]} > 0 )); then
env "${WAIT_ENV[@]}" ./bin/wait-storage.sh
else
./bin/wait-storage.sh
fi
+}
+
+# ── Init store (once) ─────────────────────────────────────────────────
+# With `init_store.enabled=false` (distributed PD/HStore) init-store is a
no-op:
+# storage owns the metadata and the admin account is created on server startup
+# from `auth.admin_pa`. A requested PASSWORD is therefore written to that
+# property rather than piped into init-store.sh, where it would be read and
+# discarded without creating the account.
+#
+# The value is read back from the config file rather than from the env var, so
+# that a rest-server.properties mounted with the property already set behaves
+# the same as `HG_SERVER_INIT_STORE_ENABLED` (the env mapping above has already
+# been applied, so env still wins).
+INIT_STORE_ENABLED=$(get_prop "init_store.enabled" "${REST_SERVER_CONF}")
+if [[ -n "${INIT_STORE_ENABLED}" ]]; then
+ if ! INIT_STORE_ENABLED=$(to_bool "${INIT_STORE_ENABLED}"); then
+ log "ERROR: init_store.enabled in ${REST_SERVER_CONF} must be a
boolean," \
+ "got '${INIT_STORE_ENABLED}'"
+ exit 1
+ fi
+fi
+
+# A mounted configuration can enable REST authentication without carrying the
+# matching Gremlin handler or auth graph proxy. Complete all three configs for
+# every configured authenticator, whether or not Docker supplied a PASSWORD.
+AUTHENTICATOR=$(get_prop "auth.authenticator" "${REST_SERVER_CONF}")
+if [[ -n "${PASSWORD:-}" || -n "${AUTHENTICATOR}" ]]; then
+ ensure_auth_enabled
+ AUTHENTICATOR=$(get_prop "auth.authenticator" "${REST_SERVER_CONF}")
+fi
+
+AUTH_STATE=""
+AUTH_INIT_REQUIRED=false
+if [[ -n "${AUTHENTICATOR}" ]]; then
+ AUTH_STATE=$(printf '%s\n%s\n%s' \
+ "${AUTHENTICATOR}" \
+ "$(get_prop "auth.remote_url" "${REST_SERVER_CONF}")" \
+ "$(get_prop "auth.graph_store" "${REST_SERVER_CONF}")")
+ STORED_AUTH_STATE=$(cat \
+ "${DOCKER_FOLDER}/${AUTH_INIT_STATE_FILE}" 2>/dev/null || true)
+ if [[ -f "${DOCKER_FOLDER}/${INIT_FLAG_FILE}" &&
+ "${STORED_AUTH_STATE}" != "${AUTH_STATE}" ]]; then
+ AUTH_INIT_REQUIRED=true
+ fi
+fi
+
+if [[ "${INIT_STORE_ENABLED:-true}" == "false" ]]; then
+ log "init-store disabled; validating the no-op configuration"
+
+ # Let InitStore make the type-aware decision about whether this effective
+ # authenticator needs the built-in admin and whether the configured auth
+ # graph can read the PD-created account. The gate returns before backend or
+ # plugin registration, so this invocation performs validation only.
+ ./bin/init-store.sh
+
+ # Still wait: the server needs the storage side reachable at startup even
+ # though nothing is initialized here
+ wait_storage
+
+ if [[ -n "${PASSWORD:-}" ]]; then
+ log "enabling auth mode, admin password applied via auth.admin_pa"
+ # TODO: auth.admin_pa only applies when the admin account is first
+ # created, so changing PASSWORD on a later restart keeps the old one.
+ if ! ESCAPED_PASSWORD=$(props_escape "${PASSWORD}"); then
+ log "ERROR: PASSWORD must be valid UTF-8"
+ exit 1
+ fi
+ if ! chmod 600 "${REST_SERVER_CONF}"; then
+ log "ERROR: cannot protect ${REST_SERVER_CONF} before writing
auth.admin_pa"
+ exit 1
+ fi
+ if ! set_prop "auth.admin_pa" "${ESCAPED_PASSWORD}" \
Review Comment:
⚠️ This persists every supplied `PASSWORD` as `auth.admin_pa`, even when the
effective authenticator is custom or uses `auth.remote_url`; `InitStore`
explicitly exempts those modes because they never consume HugeGraph's built-in
admin password, and the custom-auth test currently asserts the unused secret is
written. Please persist this value only for a local built-in
`StandardAuthenticator` (or subclass), and explicitly ignore or reject it for
custom/remote authentication.
##########
.github/workflows/docker-build-ci.yml:
##########
@@ -26,8 +26,31 @@ on:
paths:
- '**/Dockerfile*'
- '.dockerignore'
+ - 'hugegraph-server/hugegraph-dist/docker/**'
+ -
'hugegraph-server/hugegraph-dist/src/assembly/static/bin/enable-auth.sh'
+ - 'hugegraph-server/hugegraph-dist/src/assembly/static/bin/init-store.sh'
+ - '**/docker/docker-entrypoint.sh'
Review Comment:
⚠️ This path filter still lets the entrypoint contract job be skipped when
its Java-side contract changes. The smoke suite depends on `InitStore`,
`StandardAuthenticator`, and the consumed option definitions, but none of those
paths trigger this workflow; its shell stubs also do not exercise their real
status/validation behavior. Please include the relevant Java contract paths or
move this job to an unfiltered/reusable workflow.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/StandardAuthenticator.java:
##########
@@ -210,16 +217,40 @@ public SaslNegotiator newSaslNegotiator(InetAddress
remoteAddress) {
}
public static void initAdminUserIfNeeded(String confFile) throws Exception
{
+ initAdminUserIfNeeded(confFile, null, false);
+ }
+
+ public static void initAdminUserIfNeeded(String confFile,
+ String configuredPassword)
+ throws Exception {
+ initAdminUserIfNeeded(confFile, configuredPassword, true);
+ }
+
+ private static void initAdminUserIfNeeded(String confFile,
+ String password,
+ boolean fromConfig)
+ throws Exception {
StandardAuthenticator auth = new StandardAuthenticator();
HugeConfig config = new HugeConfig(confFile);
String authClass = config.get(ServerOptions.AUTHENTICATOR);
if (authClass.isEmpty()) {
return;
}
config.addProperty(INITING_STORE, true);
- auth.setup(config);
- if (auth.graph().backendStoreFeatures().supportsPersistence()) {
- auth.initAdminUser();
+ auth.initAdminUser(config, password, fromConfig);
+ }
+
+ private void initAdminUser(HugeConfig config, String password,
+ boolean fromConfig) throws Exception {
+ try {
+ this.setup(config);
+ if (this.graph().backendStoreFeatures().supportsPersistence()) {
+ this.initAdminUser(password, fromConfig);
+ }
+ } finally {
+ if (this.graph != null) {
+ this.graph.close();
Review Comment:
⚠️ If `setup()`, feature inspection, or admin creation throws and
`graph.close()` also throws, this `finally` replaces the primary bootstrap
failure with the cleanup exception. The new tests cover each primary failure
only when `close()` succeeds. Please preserve the original exception and attach
a close failure as suppressed (while still surfacing close failure when it is
the only error), and add a dual-failure regression.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -19,24 +19,234 @@ set -euo pipefail
DOCKER_FOLDER="./docker"
INIT_FLAG_FILE="init_complete"
+AUTH_INIT_STATE_FILE="auth_init_state"
GRAPH_CONF="./conf/graphs/hugegraph.properties"
+REST_SERVER_CONF="./conf/rest-server.properties"
+GREMLIN_SERVER_CONF="./conf/gremlin-server.yaml"
+
+# The only in-tree HugeAuthenticator that bootstraps HugeGraph's built-in admin
+# account. auth.authenticator accepts any implementation class, and a custom
one
+# (LDAP, OIDC, a plugin) manages its identities elsewhere, so the admin-account
+# requirement below must not be applied to it.
+BUILTIN_AUTHENTICATOR="org.apache.hugegraph.auth.StandardAuthenticator"
mkdir -p "${DOCKER_FOLDER}"
log() { echo "[hugegraph-server-entrypoint] $*"; }
+# Sets a property to exactly one canonical `key=value` line. Existing
+# definitions are matched on any separator a properties file allows (`=`, `:`
+# or whitespace) and collapsed into that single line, because leaving a second
+# definition behind would make the parser expose the key as a list and a scalar
+# read of it would then fail. Comment lines are left alone. Matching is
literal,
+# so no regex escaping of the key or value is needed.
set_prop() {
- local key="$1" val="$2" file="$3"
- local esc_key esc_val
+ local key="$1" val="$2" file="$3" tmp
+
+ # The scratch file holds auth.admin_pa, so keep it off the process umask
+ # and use an unpredictable same-directory name rather than following a
+ # pre-created predictable symlink.
+ if ! tmp=$(umask 077; mktemp "${file}.tmp.XXXXXX"); then
+ return 1
+ fi
+
+ if ! SET_PROP_KEY="$key" SET_PROP_VAL="$val" awk '
+ BEGIN { key = ENVIRON["SET_PROP_KEY"]; val = ENVIRON["SET_PROP_VAL"] }
+ {
+ line = $0
+ probe = line
+ sub(/^[[:space:]]+/, "", probe)
+ if (index(probe, key) == 1) {
+ rest = substr(probe, length(key) + 1)
+ if (rest ~ /^[[:space:]]*[=:]/ || rest ~ /^[[:space:]]+/) {
+ if (!done) { print key "=" val; done = 1 }
+ next
+ }
+ }
+ print line
+ }
+ END { if (!done) print key "=" val }
+ ' "${file}" > "${tmp}"; then
+ rm -f "${tmp}"
+ return 1
+ fi
+
+ # Truncate and rewrite in place rather than rename: a single-file bind
+ # mount cannot be replaced by rename, and a rename would also discard the
+ # original ownership and mode, which matters where auth.admin_pa is written
+ if ! cat "${tmp}" > "${file}"; then
+ rm -f "${tmp}"
+ return 1
+ fi
+ rm -f "${tmp}"
+}
+
+count_prop() {
+ local key="$1" file="$2"
+
+ [[ -f "${file}" ]] || { echo 0; return; }
+ SET_PROP_KEY="$key" awk '
+ BEGIN { key = ENVIRON["SET_PROP_KEY"] }
+ {
+ probe = $0
+ sub(/^[[:space:]]+/, "", probe)
+ if (index(probe, key) == 1) {
+ rest = substr(probe, length(key) + 1)
+ if (rest ~ /^[[:space:]]*[=:]/ || rest ~ /^[[:space:]]+/) {
+ count++
+ }
+ }
+ }
+ END { print count + 0 }
+ ' "${file}"
+}
+
+# Drops duplicate definitions while leaving a single valid definition
untouched.
+# Avoiding a needless rewrite lets complete read-only mounted configs start.
+canonicalize_prop() {
+ local key="$1" file="$2" count cur
+ count=$(count_prop "${key}" "${file}")
+ if (( count > 1 )); then
+ cur=$(get_prop "${key}" "${file}")
+ set_prop "${key}" "${cur}" "${file}"
+ fi
+}
+# Escapes a UTF-8 value for Java-properties serialization. Encoding every
+# UTF-16 code unit as a Unicode escape keeps separators, leading whitespace,
+# backslashes and embedded control characters out of the physical property
+# line while the Java parser reconstructs the exact original string.
+props_escape() {
+ printf '%s' "$1" | iconv -f UTF-8 -t UTF-16BE | \
+ od -An -v -t x1 | awk '
+ {
+ for (i = 1; i <= NF; i++) {
+ if (high == "") {
+ high = $i
+ } else {
+ printf "\\u%s%s", high, $i
+ high = ""
+ }
+ }
+ }
+ END { if (high != "") exit 1 }
+ '
+}
+
+# Echoes the value of a property, or nothing when the key or the file is
+# absent, so callers apply their own default. Accepts the `=`, `:` and
+# whitespace separators that properties files allow. On duplicate keys the last
+# one wins, matching how the properties parser reads the same file. Only
+# surrounding whitespace is trimmed, as the parser does; whitespace inside a
+# value is part of the value and deleting it would corrupt one.
+get_prop() {
+ local key="$1" file="$2"
+ local esc_key
+
+ [[ -f "${file}" ]] || return 0
esc_key=$(printf '%s' "$key" | sed -e 's/[][(){}.^$*+?|\\/]/\\&/g')
- esc_val=$(printf '%s' "$val" | sed -e 's/[&|\\]/\\&/g')
+ # '#' delimits the s command because the pattern itself contains '|'.
+ # '-E' rather than '-r': both GNU and BSD sed accept it
+ sed -En
"s#^[[:space:]]*${esc_key}([[:space:]]*[=:]|[[:space:]]+)[[:space:]]*(.*)\$#\\2#p"
\
Review Comment:
‼️ This physical-line reader does not implement the Java-properties grammar
used by `HugeConfig`. For example, Commons Configuration resolves
`auth\.authenticator=...` to `auth.authenticator` and joins a continued
`init_store.enabled=fal\` + next-line `se`, while this reader misses the
escaped key and reads the boolean as `fal\`. The former can enable REST auth
without triggering `ensure_auth_enabled()`, leaving Gremlin authentication and
`HugeFactoryAuthProxy` absent. Please read and rewrite these values through the
same Java/Commons parser, or fully support escaped keys and continuations, with
mounted-config regressions.
--
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]
