Repository navigation
feat(server): read the initial admin password from a file - #3334
arshilkxwork wants to merge 5 commits into
Conversation
auth.admin_pa goes through the properties grammar, which trims the value, processes backslash escapes and decodes it as ISO-8859-1. A padded, backslashed or non-ASCII password can be stored differently from what the operator wrote. auth.admin_pa_file names a file that is read as raw UTF-8, with one trailing line break dropped. When set, it replaces auth.admin_pa on the PD startup path and the in-memory backend path, and it replaces the stdin prompt in init-store. A missing, empty or non-UTF-8 file stops startup with an error naming the option; the server never falls back to the 'pa' default. With init-store disabled, init-store reads the file up front so a bad path fails before the server starts. Fixes apache#3285
|
Paired website docs: apache/hugegraph-doc#526 (apache/hugegraph-doc#526). It adds |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3334 +/- ##
============================================
+ Coverage 54.32% 56.91% +2.58%
+ Complexity 2619 130 -2489
============================================
Files 770 514 -256
Lines 67937 49791 -18146
Branches 9210 7215 -1995
============================================
- Hits 36908 28338 -8570
+ Misses 27445 18550 -8895
+ Partials 3584 2903 -681 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
init-store prompts for the admin password on stdin when the backend is persistent. Pin that a configured auth.admin_pa_file replaces the prompt and the admin is created with the file's password.
Strip a leading UTF-8 BOM. PowerShell and some Windows editors write one, and if kept it becomes an invisible first character of the admin password. Read at most 4096 bytes, so a path that points at a log or a device fails instead of being loaded whole, and report an invalid path as a ConfigException. When the file is refused, init-store now closes the auth graph and keeps the file error even if the close also fails. The BackendException raised when a non-persistent backend can't create the admin now carries its cause. Document that PD and non-persistent backends read the file on every start, and that a disabled init-store still needs this option or an explicit auth.admin_pa. The init-store tests run with an empty stdin, so falling back to the prompt fails instead of hanging. New tests cover the BOM, the size cap, closing the graph on a refused file, and the GraphManager non-persistent path.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The file reader, its precedence over auth.admin_pa and the three bootstrap call sites match the description, and I found no correctness or compatibility problem in them; one minor note on password length. Evidence: full diff of c1e7aba..128c0b3 read with the surrounding StandardAuthenticator, GraphManager, InitStore and AuthenticationFilter code; the 72-byte behaviour was reproduced with jbcrypt 0.4 (BCrypt.checkpw). At review time the HugeGraph-Server CI jobs cluster-test, docker-build, docker-bake-check, deployment-contracts and pd_store/struct were still queued, and the PD startup call in GraphManager.loadMetaFromPD has no unit test, so that path rests on reading the code.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The password reader opens arbitrary paths before enforcing its size bound; a FIFO with no writer can hang startup. Evidence: Files.newInputStream() opens the configured path before the bounded read. Reject non-regular sources while following mounted-secret symlinks, then apply the size limit.
Check the path's attributes before opening it and refuse anything that is not a regular file. Opening a FIFO blocks until a writer connects, so a wrong path could hang startup before the size limit applied. Symlinks are followed, so a Kubernetes Secret mounted through its ..data links still works. Refuse a password longer than 72 bytes in UTF-8. jBCrypt hashes only the first 72 bytes, so a longer secret would also accept any password sharing that prefix. The 4096-byte limit still bounds how much of the file is read. Tests cover a directory, a FIFO (with a timeout so a regression fails instead of hanging), a symlink to a regular file, and the 72-byte limit for ASCII and multi-byte passwords.
apache/hugegraph#3334 now requires a regular file, strips a leading BOM, and caps the file at 4096 bytes and the password at 72 UTF-8 bytes. Add those to the option rows and the rest-server.properties comments. The pages also called the option first-startup only. The password is applied only when the admin is created, but PD (hstore) and non-persistent backends read the file on every start, so removing it after the first boot stops the Server. Say so in the option rows, the properties comments and the hstore init-store paragraph.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The non-persistent backend path reads the new password file even when remote authentication disables local admin bootstrap, so an unused missing file can prevent startup. Evidence: GraphManager.checkBackendVersionOrExit resolves the file before StandardAuthenticator.initAdminUser; remote AuthManager makes requireInitAdminUser() return false, while the PD startup path already guards this read.
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The file reader, its precedence over auth.admin_pa and the three bootstrap call sites behave as described at this head, and the two earlier points are fixed (non-regular paths are refused before they are opened, passwords over 72 UTF-8 bytes are refused). One minor gap remains: no test runs the PD call site with the file option set. Evidence: full diff c1e7aba..7ba00bd read with StandardAuthenticator, GraphManager, InitStore, HugeFactoryAuthProxy, HugeSecurityManager, AuthenticationFilter and the Docker entrypoint; on Linux x86_64 with OpenJDK 17.0.20.1, mvn test -pl hugegraph-server/hugegraph-test -am -P unit-test -Dtest=AdminPasswordFileTest,GraphManagerAdminInitTest,InitStoreConfigTest passes the 42 tests of those three classes at 7ba00bd, and the same 42 with the PD call put back to conf.get(ServerOptions.ADMIN_PA); CI run 38446074869 at this head is green (UnitTestSuite: 1146 run, 0 failed in server_memory), and the only red status is codecov/project (54.16%, -0.16% against c1e7aba). The remote-auth comment already open on GraphManager.java line 1883 matches the code and is not repeated here.
| this.initAdminUserIfNeeded( | ||
| StandardAuthenticator.initialAdminPassword(conf)); |
There was a problem hiding this comment.
Minor: no test runs this call with auth.admin_pa_file set, and it is the only link between the option and the PD bootstrap, the path it mostly serves (hstore with a mounted Secret). The review of 128c0b3 noted the gap; it still holds at 7ba00bd. loadMetaFromPD() runs only from the constructor when usePD=true, no unit test builds a GraphManager that way, and the hstore API job starts the Server with the option unset.
Evidence (Linux x86_64, OpenJDK 17.0.20.1): with these two lines put back to this.initAdminUserIfNeeded(conf.get(ServerOptions.ADMIN_PA));, AdminPasswordFileTest, GraphManagerAdminInitTest and InitStoreConfigTest still pass all 42 of their tests.
Requested change: move the shouldBootstrapAdmin guard and this call into a small method that takes the config, and cover it in GraphManagerAdminInitTest, which already swaps in an AuthMetaManager over a mock driver: one case with a password file whose content StringEncoding.checkPassword accepts against the stored admin hash, and one with auth.remote_url set and a missing file, which must not be read.
With auth.remote_url set, StandardAuthenticator uses a remote AuthManager and never creates a local admin, but the non-persistent backend path still resolved auth.admin_pa_file first. A missing file that would never be used stopped startup. Skip the lookup in that case, as the PD startup path already does. Custom authenticators still receive initAdminUser().
|
@arshilkxwork After looking more closely, I don't think adding For any remaining encoding issues, please prefer a narrowly scoped fix to the existing input path, with round-trip tests and clear documentation. If there is a concrete Kubernetes Secret integration requirement, we can consider a focused adaptation that reuses the existing configuration/authentication flow, rather than introducing a separate general-purpose password-file mechanism. I would defer this feature until that need is demonstrated. |
|
Thanks for taking a closer look. Agreed: #3327 already covers the Docker case, and the remaining inputs are uncommon, so I am closing this along with the docs PR, apache/hugegraph-doc#526. If a concrete Kubernetes Secret requirement comes up, it can start from an issue and reuse the existing configuration and authentication flow, as you suggest. |
3 participants
Fixes #3285.
auth.admin_pais read through commons-configuration's properties reader. It trims the value, processes backslash escapes and decodes the file as ISO-8859-1, sopadded,two\\backorpässwordcan end up as a different admin password from the one the operator set.This adds
auth.admin_pa_file. It points at a file that the Server reads as raw UTF-8, dropping a leading BOM and one trailing\nor\r\nso files written byecho, a Windows editor or a secret mount work as expected.auth.admin_pastays as it is for compatibility.When the file option is set it is used on all three bootstrap paths:
GraphManager.loadMetaFromPD), in place ofauth.admin_paGraphManager.checkBackendVersionOrExit), in place ofauth.admin_painit-storewith a persistent backend, in place of the stdin promptThe first two read the file on every start, so it has to stay readable while the option is set. The option description and the README say so.
The reader refuses a missing, unreadable, empty or non-UTF-8 file, anything that is not a regular file (symlinks are followed, so mounted Kubernetes Secrets work), a file over 4096 bytes, and a password longer than 72 bytes in UTF-8, which is all that jBCrypt hashes. Each case is a
ConfigExceptionthat names the option. The file type is checked before the file is opened, so a FIFO cannot block startup. The reader never falls back toauth.admin_paor thepadefault, because that would create an admin whose password nobody chose. The non-UTF-8 case is refused for the same reason: guessing another charset is howpässwordbecamepässwordin the first place. Wheninit_store.enabled=false, the existing check inInitStore.checkAdminBootstrapReachablenow accepts the file option as an explicit password and reads it right away, so a bad path fails before the Server starts rather than during PD bootstrap.If init-store refuses the file, it still closes the auth graph. A failure while closing is added to the file error as a suppressed exception. The
BackendExceptionthrown when a non-persistent backend can't create the admin now keeps its cause.initialAdminPasswordandreadAdminPasswordFileare added to the Gremlin reflection filter next toinitAdminUserandinputPassword.Docs:
docker/README.mdand the commented example in the shippedrest-server.properties. The Docker entrypoint already encodesPASSWORDsafely since #3327, so it is unchanged. The website config-option table also listsauth.admin_pa; the matching entry is in apache/hugegraph-doc#526.Left for a follow-up: the Helm chart still refuses padded, backslash and non-ASCII admin passwords. Relaxing that means switching its Server wrapper to mount the Secret and set
auth.admin_pa_file, and it depends on the minimum Server image the chart supports, so it belongs in its own PR. The TODO inServerOptionsnow says that.Tests:
AdminPasswordFileTestcovers precedence, the strings from the issue table, line-break and BOM handling, the 4096-byte and 72-byte limits, directories, FIFOs, symlinks and the other failure cases. Its init-store tests check that the file replaces the prompt and that a refused file still closes the auth graph; they run with an empty stdin, so a fallback to the prompt fails instead of hanging.GraphManagerAdminInitTestandInitStoreConfigTesteach get two new cases, with a valid file and with a missing one, for the non-persistent backend path and the disabled init-store check.Conceptual overview
The file option takes precedence, strictly decodes UTF-8 and rejects invalid input without fallback. It feeds PD, non-persistent backend and init-store bootstrap paths.
Conceptual illustration; not test or release-acceptance evidence.