bitflicker64 commented on code in PR #3327:
URL: https://github.com/apache/hugegraph/pull/3327#discussion_r4252976654
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -75,6 +69,24 @@ set_prop() {
set_prop_encoded "$key" "$(encode_prop_value "$val")" "$file"
}
+# Auth values are strings, not typed/list options. Commons keeps escaped spaces
+# and trims a physical trailing space before detecting line continuation.
+encode_auth_prop_value() {
+ local value="$1" encoding=UTF-8
+ if (( $(printf '%s' "${value}" | LC_ALL=C tr -d '\000-\177' | wc -c) > 0
)); then
+ # Retain legacy byte values that the ISO-8859-1 reader already
accepted.
+ if ! printf '%s' "${value}" | iconv -f UTF-8 -t UTF-8 >/dev/null 2>&1;
then
+ encoding=ISO-8859-1
+ fi
+ # The Server properties reader is ISO-8859-1. Standard UTF-16 escapes
+ # preserve UTF-8 input, including supplementary characters and
backslashes.
+ printf '%s' "${value}" | iconv -f "${encoding}" -t UTF-16BE | od -An
-v -tx1 | \
+ tr -d ' \n' | sed 's/..../\\u&/g'
+ else
+ encode_prop_value "${value}" | sed 's/\\ /\\u0020/g'
Review Comment:
Important: An ASCII value that starts or ends with a C0 control byte still
loads as a different string. This branch only escapes backslash, space, tab,
LF, CR and FF (through `encode_prop_value`), so bytes such as 0x01, 0x0B or
0x1F are written raw, and the Commons Configuration 2.10.1 properties reader
trims them at either end of the value.
I sourced `encode_prop_value` and `encode_auth_prop_value` from this head
and read the result back with `new
Configurations().properties(file).getProperty("auth.token_secret")`:
- `KEY` + 0x01, 0x01 + `KEY`, `KEY` + 0x0B and `KEY` + 0x1F all load as
`KEY`, one character short.
- 0x01 in the middle, a trailing 0x7F, and the same control byte in a value
that also has a non-ASCII character (the `\uXXXX` branch) all round-trip.
So `HG_SERVER_AUTH_TOKEN_SECRET` or `PASSWORD` with such a byte at an edge
is stored as a different key or password with no error, and a 32-byte key
passes the length check at line 114 but loads as 31 characters. The raw write
is the same as before this PR, so this is not a regression, but the README text
added here says authentication strings are retained through the reader and the
new test comment says both files load exactly what the caller supplied.
Requested change: escape every byte below 0x20 (and ideally 0x7F) as
`\u00XX` in this branch, or send every value through the `iconv | od` branch,
which already handles them, and add one such key to `auth_secrets` in
docker-entrypoint-test.sh. Rejecting control bytes with an error at startup
would also work.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint-test.sh:
##########
@@ -67,6 +67,27 @@ exit 0
EOF
chmod +x "${TEST_HOME}/bin/"*.sh
+# CI supplies current HugeConfig and the POM-pinned Commons dependency
classpath.
+if [[ -n "${HG_CONFIG_TEST_CLASSPATH:-}" ]]; then
Review Comment:
Minor: When `HG_CONFIG_TEST_CLASSPATH` is unset, `assert_auth_readback` does
nothing and prints nothing, so the new loop at the end only checks that a
second run leaves the files unchanged. That is the case for a local run and for
`.github/workflows/server-tests.yml` line 79, which calls this script without
the variable. Only the `docker / deployment-contracts` job runs the readback
(its log at this head has the `PASS: exact authentication property readback`
lines). A later change that breaks the encoding while that job is skipped or
its classpath step is edited would still print the final PASS here.
Requested change: print a `SKIP: auth readback needs
HG_CONFIG_TEST_CLASSPATH` line in the unset case so the gap is visible in logs,
and consider exporting the classpath in server-tests.yml too, since that job
has already compiled hugegraph-common.
##########
hugegraph-server/hugegraph-dist/docker/docker-entrypoint.sh:
##########
@@ -26,13 +26,7 @@ mkdir -p "${DOCKER_FOLDER}"
log() { echo "[hugegraph-server-entrypoint] $*"; }
-# TODO: write values so the Server reads them back unchanged. A space is
escaped as "\ " here,
-# which java.util.Properties would unescape, but the Server reads these files
through Commons
-# Configuration's PropertiesConfiguration, which keeps the backslash before a
space: PASSWORD
-# "a b" becomes auth.admin_pa "a\ b" and the admin login with "a b" answers
401. Emit interior
-# spaces as-is (they round-trip) and add a write-then-read test through
HugeConfig for the keys
-# this script sets. The Helm chart (helm/hugegraph) refuses admin passwords
containing spaces in
-# its schema and Server wrapper; drop that guard once this is fixed.
+# Keep typed/list mappings on their existing encoder; auth strings use
native-reader escapes below.
Review Comment:
Minor: The TODO removed here was the tracking point for the Helm guard, and
the chart still carries it. `helm/hugegraph/templates/server-deployment.yaml`
lines 115-128 reject an admin password with a space, say the image stores each
space with a backslash before it, and point to the `TODO at encode_prop_value
in docker-entrypoint.sh`. After this PR that message is no longer true for
images built from this head and the TODO it names is gone.
Requested change: either drop the space case in the chart wrapper and the
space exclusion in the values schema in this PR, or keep a one-line TODO here
(or update the chart comment) that says the guard can be removed once the
chart's minimum image version includes this fix.
--
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]
