bitflicker64 commented on code in PR #3119:
URL: https://github.com/apache/hugegraph/pull/3119#discussion_r3684818207
##########
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:
Valid, fixed in commit 1434969c. The entrypoint now asks Java whether the
effective authenticator is a local StandardAuthenticator, including subclasses
and excluding auth.remote_url. Only that mode persists or pipes PASSWORD;
custom and remote auth explicitly log that it is ignored. Both custom and
remote cases assert that auth.admin_pa is absent.
##########
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:
Valid, fixed in commit 1434969c. The physical-line shell parser and rewriter
were replaced by ConfigTool, which uses Commons Configuration, the same grammar
as HugeConfig. Regressions cover escaped keys, continued booleans, duplicate
logical definitions, comments/layout, and exact complex password round trips.
Java passes 30/30 and the entrypoint suite passes 213/213.
##########
.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:
Valid, fixed in commit 1434969c. The Docker workflow filter now includes
config-tool.sh, ConfigTool.java, InitStore.java, StandardAuthenticator.java,
and ServerOptions.java, covering every Java-side dependency of the entrypoint
contract. The local entrypoint suite passes 213/213.
##########
hugegraph-server/hugegraph-dist/src/main/java/org/apache/hugegraph/cmd/InitStore.java:
##########
@@ -41,21 +41,64 @@
public class InitStore {
private static final Logger LOG = Log.logger(InitStore.class);
+ private static final String USE_CONFIGURED_ADMIN_PASSWORD =
+ "--use-configured-admin-password";
public static void main(String[] args) throws Exception {
- E.checkArgument(args.length == 1,
+ E.checkArgument(args.length == 1 ||
+ args.length == 2 &&
+ USE_CONFIGURED_ADMIN_PASSWORD.equals(args[1]),
"HugeGraph init-store need to pass the config file " +
- "of RestServer, like: conf/rest-server.properties");
+ "of RestServer, like: conf/rest-server.properties, " +
+ "with an optional %s flag",
+ USE_CONFIGURED_ADMIN_PASSWORD);
E.checkArgument(args[0].endsWith(".properties"),
"Expect the parameter is properties config file.");
String restConf = args[0];
+ boolean useConfiguredAdminPassword = args.length == 2;
- RegisterUtil.registerBackends();
- RegisterUtil.registerPlugins();
+ /*
+ * Only the server options are needed to read the gate below. Backend
+ * and plugin registration is deferred to the enabled path:
+ * registerPlugins() invokes every discovered plugin's register() and
+ * propagates their failures, which must not happen on a path that is
+ * documented to be a no-op.
+ */
RegisterUtil.registerServer();
HugeConfig restServerConfig = new HugeConfig(restConf);
+
+ /*
+ * Distributed deployments (PD/HStore) let the storage side own the
+ * metadata, so there is nothing for init-store to do. The option
+ * defaults to true, keeping standalone/tarball installs on the full
+ * init path.
+ *
+ * The loop below already skips hstore backends, so what this gate
+ * additionally avoids is scanning the graphs directory (which must
+ * otherwise exist), and, when auth is configured, opening the auth
+ * graph store in initAdminUserIfNeeded(). On Kubernetes that ran on
+ * every Server pod restart, since the entrypoint's init flag file does
+ * not survive one.
Review Comment:
Valid, fixed in commit 1434969c. The comment now distinguishes the full
initialization scan from the disabled-path validation: validation may scan
graph configuration files to prove the PD/HStore auth topology, but it does not
open those graphs. The focused Java suite passes 30/30.
##########
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:
Valid, fixed in commit 1434969c. InitStore.configuredAdminPassword now
requires auth.admin_pa to be explicitly present and non-empty before the
configured-password path can run, so the public pa fallback cannot bootstrap
admin. Both enabled and disabled Docker paths fail closed, with direct
default-password regressions in the Java and 213-assertion entrypoint suites.
##########
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:
Valid, fixed in commit 1434969c. Admin bootstrap now retains the primary
Exception or Error and attaches a simultaneous graph-close failure as
suppressed. A close-only failure is still thrown. Dedicated dual-failure and
close-only regressions pass in the 30-test focused suite.
--
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]
