refactor(auth): secure non-OAuth2 admin credentials with keyring-first flow and runtime JWT key - #1161
refactor(auth): secure non-OAuth2 admin credentials with keyring-first flow and runtime JWT key#1161nbmaiti wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1161 +/- ##
==========================================
- Coverage 50.16% 50.13% -0.04%
==========================================
Files 147 148 +1
Lines 13574 13830 +256
==========================================
+ Hits 6810 6933 +123
- Misses 6172 6283 +111
- Partials 592 614 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a827972 to
d29099e
Compare
There was a problem hiding this comment.
Pull request overview
This PR strengthens non-OAuth2 authentication by moving admin credential handling into a keyring-first flow, hashing admin passwords with bcrypt, and ensuring the JWT signing key is generated at runtime when not explicitly configured.
Changes:
- Introduces a centralized admin secret-store flow (keyring →
.env→ env vars →config.yml→ prompt) plus CLI helpers for managing stored admin credentials. - Switches basic-auth verification from plaintext comparison to bcrypt hash verification.
- Removes placeholder JWT keys from templates and generates an in-memory JWT key at startup when none is configured.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.env.example |
Removes placeholder auth secrets and documents keystore-first resolution. |
cmd/app/main.go |
Wires admin CLI handling and replaces legacy admin password setup with the new credential flow. |
cmd/app/main_test.go |
Adds unit tests for the new secret-store resolution, CLI commands, and hashing behavior. |
cmd/app/secret_store.go |
Implements keyring-first admin credential resolution, interactive prompting, and CLI workflows. |
config/config.go |
Makes AUTH_JWT_KEY optional and generates a runtime JWT key when missing; adds helpers to clear/persist admin creds. |
config/config.yml |
Removes default admin/JWT placeholders and adds guidance comments for secure configuration. |
config/config_test.go |
Adds tests around runtime JWT key behavior. |
internal/controller/httpapi/v1/login.go |
Uses bcrypt hash comparison for basic-auth login verification. |
internal/controller/httpapi/v1/login_test.go |
Updates tests to use bcrypt-hashed admin password. |
Comments suppressed due to low confidence (3)
internal/controller/httpapi/v1/login.go:83
- The
||short-circuit means bcrypt is only evaluated when the username matches, which makes username enumeration possible via response-time differences (bcrypt is much slower than a simple string compare). Compute the password check first so the handler always does the bcrypt work regardless of username match.
if creds.Username != lr.Config.AdminUsername || bcrypt.CompareHashAndPassword([]byte(lr.Config.AdminPassword), []byte(creds.Password)) != nil {
c.JSON(http.StatusUnauthorized, gin.H{errorKey: "invalid credentials", messageKey: "Incorrect Username and/or Password!"})
return
}
cmd/app/secret_store.go:174
- If only one of the two keystore writes succeeds, the keystore can end up in a partial state that later causes confusing credential resolution (e.g., username from keyring + password from another source). Consider a best-effort rollback when either write fails.
usernameSaveErr := keyringStore.SetKeyValue(keyringAdminUsername, cfg.AdminUsername)
passwordSaveErr := keyringStore.SetKeyValue(keyringAdminPassword, cfg.AdminPassword)
if usernameSaveErr == nil && passwordSaveErr == nil {
// Credentials safely in keystore; scrub any plaintext from config.yml.
if err := config.ClearAdminCredentials(); err != nil {
log.Printf("Warning: failed to clear admin credentials from config.yml: %v", err)
} else {
log.Print("Admin credentials cleared from config.yml (stored in keystore).")
}
return
}
cmd/app/main_test.go:183
- This test uses
t.Setenv, which mutates process-global environment variables and should not run in parallel. Add an explicit//nolint:paralleltest(as used elsewhere in this repo) to avoid paralleltest linter failures.
func TestResolveAdminCredentialsFromSources_FallbackToDotEnvThenConfig(t *testing.T) {
t.Setenv(authAdminUsernameEnv, "")
t.Setenv(authAdminPasswordEnv, "")
4ee2bf3 to
f914325
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
cmd/app/main_test.go:164
- This test uses t.Setenv but doesn't call t.Parallel(). With paralleltest enabled, new tests are expected to either call t.Parallel() or explicitly opt out. Since environment mutation isn't safe to run in parallel, add a //nolint:paralleltest on the test declaration (per CLAUDE.md:245).
func TestResolveAdminCredentialsFromSources_PriorityOrder(t *testing.T) {
t.Setenv(authAdminUsernameEnv, "env-user")
t.Setenv(authAdminSecretEnv, "env-pass")
cmd/app/main_test.go:183
- This test uses t.Setenv but doesn't call t.Parallel(). With paralleltest enabled, new tests should either call t.Parallel() or explicitly opt out. Since environment mutation isn't safe to run in parallel, add a //nolint:paralleltest on the test declaration (per CLAUDE.md:245).
func TestResolveAdminCredentialsFromSources_FallbackToDotEnvThenConfig(t *testing.T) {
t.Setenv(authAdminUsernameEnv, "")
t.Setenv(authAdminSecretEnv, "")
cmd/app/secret_store.go:178
- handleAdminCredentials runs even when OAuth2 is configured (auth.clientId set). In that mode Console doesn't use basic-auth admin credentials, but this function can still prompt/exit on EOF in non-interactive environments, breaking OAuth2 deployments unnecessarily. Skip admin credential resolution when OAuth2 is enabled (same condition used in login.go: ClientID != "").
func handleAdminCredentials(cfg *config.Config) {
if cfg.Disabled {
log.Print("Auth is disabled; skipping admin credential resolution.")
return
go.mod:74
- go.mod marks golang.org/x/term as indirect, but it's imported directly (cmd/app/secret_store.go). This usually indicates go.mod wasn't tidied; leaving it indirect can cause churn on the next
go mod tidyrun.
golang.org/x/term v0.45.0 // indirect
| var configPathFlag string | ||
| if f := flag.Lookup("config"); f != nil { | ||
| configPathFlag = f.Value.String() | ||
| } | ||
|
|
f914325 to
f108807
Compare
f108807 to
fa6e135
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cmd/app/main_test.go:164
- This test uses
t.Setenvand intentionally does not callt.Parallel(), but this file otherwise follows the repo’sparalleltestconvention (tests either callt.Parallel()or add//nolint:paralleltest). Add an explicit//nolint:paralleltestto avoid lint failures for a missingt.Parallel()call.
func TestResolveAdminCredentialsFromSources_PriorityOrder(t *testing.T) {
t.Setenv(authAdminUsernameEnv, "env-user")
t.Setenv(authAdminSecretEnv, "env-pass")
cmd/app/main_test.go:183
- Same as above: this test uses
t.Setenvand does not callt.Parallel(). Add//nolint:paralleltestso theparalleltestlinter doesn’t report a missingt.Parallel()in this file.
func TestResolveAdminCredentialsFromSources_FallbackToDotEnvThenConfig(t *testing.T) {
t.Setenv(authAdminUsernameEnv, "")
t.Setenv(authAdminSecretEnv, "")
cmd/app/secret_store.go:86
- The PR description lists an
--add-adminworkflow, but the CLI parsing here only recognizes--remove-adminand--show-admin(and the error message also references only those). This is a discrepancy: either implement--add-admin(e.g., prompt for username/password, store to keyring, clear config.yml, then exit) or update the PR description/docs to match the actual supported commands.
func selectedAdminCommand(args []string) string {
selected := 0
command := ""
if hasArg(args, "--remove-admin") {
selected++
command = "remove"
}
if hasArg(args, "--show-admin") {
selected++
command = "show"
}
cmd/app/main.go:79
main()now useshandleAdminCredentials, but the oldhandleAdminPasswordflow (random password +SaveAdminPasswordto config.yml) still exists in this file and is only referenced by tests. Keeping two admin-credential mechanisms around (one of which persists plaintext) is confusing and risks accidentally reintroducing the insecure path later. Consider removinghandleAdminPasswordand its tests (or making it call into the new secret-store logic).
l := logger.New(cfg.Level)
handleEncryptionKey(cfg)
handleAdminCredentials(cfg)
|
I am yet to review the code, but did give a try with your PR and when I run console with the following I see console is hung, this could be due to keyring being locked or some other issue on my setup. APP_DISABLE_CIRA=false LOG_LEVEL=debug HTTP_PORT=8181 HTTP_HOST=10.190.213.14 AUTH_ADMIN_USERNAME=admin AUTH_ADMIN_PASSWORD= APP_ENCRYPTION_KEY= ./console_linux_x64 So you bring some initial check on keyring accessibility with some time-out and exit with the message. Something like this 2026/08/14 16:44:26 migrate: environment variable not declared: DB_URL -- using embedded database |
|
./console_linux_x64 --help does not show the options of |
fa6e135 to
81bdcd3
Compare
sudhir-intc
changed the title
divneil-intel
left a comment
divneil-intel
left a comment
There was a problem hiding this comment.
I believe the credential and JWT workflow needs further discussion before merging.
My concerns are:
config.ymlis user configured, but the code rewrites it to remove the admin username and password. In my opinion, the code should open config.yml in read-only mode.- bcrypt hash is stored in the OS keyring, instead could be stored in a protected file. The container based deployment would work seamlessly with it.
.envis also parsed with this PR, making it's usage at-par withconfig.yml, thus duplicating the purpose of it- It looks inconsistent to have
--show-adminand--remove-adminwithout implementing--add-admin - For
AUTH_JWT_KEYI believe we should move away from generating keys if not provided as it should be user owned.
Since, it looks a new user flow is getting added, so, I propose the below categorization of phases which could be expanded further:
Provisioning
- Admin enrollment
- JWT provisioning
- and so on
Runtime
- Admin login: the credentials are never persisted in clear. bcrypt hash is used to compare the incoming credentials.
- The runtime always uses the data but never creates it, this makes the separation of duties apparent and flow more cleaner.
Please look into secret creds ADR , please provide feedback if you have |
Migrate the admin username/password out of config.yml and env vars into the OS keyring on startup, with .env and config.yml as fallback sources, per the centralize-secret-storage ADR (standalone mode). Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
Hash admin passwords with bcrypt before they are written to the keystore, and compare bcrypt hashes on login instead of plaintext. Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>
81bdcd3 to
d27d09d
Compare
rsdmike
left a comment
rsdmike
left a comment
There was a problem hiding this comment.
On the right track. Don't think it makes sense to have any user management cli flags, those need to be removed. that opens a door we currently dont have open right now. If they need to rotate the credential they can do so via their appropriate OS credential store or their deployment secret provider, they shouldnt want or need console to do it and we dont want to be in that workflow.
Summary
This PR implements secure credential handling for non-OAuth2 Console deployments, aligned with issue #1067.
It centralizes admin secret handling, prioritizes keyring-backed storage, removes default JWT key placeholders from config templates, and ensures runtime JWT key generation when no explicit key is configured.
Related Issue
What Changed
Admin credential flow (non-OAuth2)
.envconfig.yml--add-admin--remove-admin--show-admin(username only; password is never displayed)config.yml.Password security
JWT key behavior
AUTH_JWT_KEYis no longer treated as required.Files Changed
.env.examplecmd/app/main.gocmd/app/main_test.gocmd/app/secret_store.goconfig/config.goconfig/config.ymlconfig/config_test.gointernal/controller/httpapi/v1/login.gointernal/controller/httpapi/v1/login_test.goValidation
Executed:
go test ./configgo test ./cmd/app ./internal/controller/httpapi/v1All passed.
Notes