From e6f2386a0f062e6ed3ebd0a0ce23e55e94b2a766 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sat, 5 Sep 2026 13:16:56 -0700 Subject: [PATCH] admin: redact S3 secret keys for read-only sessions (#11189) The Admin UI documents its read-only account as view-only and blocks its write requests, but the authenticated read routes returned object-store users with plaintext access and secret keys. A read-only admin user could retrieve another user's live S3 credential pair from GET /api/users and GET /api/users/{username} and use it directly against the S3 endpoint, converting view-only access into the victim identity's object-store authority. Redact the reusable secret_key in GetUsers, GetUserDetails, and the rendered users page whenever the requesting session has the read-only role. The public access_key identifier is retained so identities remain browsable; only the reusable secret is stripped. Admin and no-auth sessions are unaffected. --- weed/admin/dash/admin_data.go | 28 +++++++ weed/admin/dash/auth_middleware.go | 2 +- .../dash/readonly_secret_redaction_test.go | 75 +++++++++++++++++++ weed/admin/handlers/user_handlers.go | 23 ++++++ 4 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 weed/admin/dash/readonly_secret_redaction_test.go diff --git a/weed/admin/dash/admin_data.go b/weed/admin/dash/admin_data.go index aa5b517d9..c57b10a56 100644 --- a/weed/admin/dash/admin_data.go +++ b/weed/admin/dash/admin_data.go @@ -113,6 +113,34 @@ type UserDetails struct { Groups []string `json:"groups"` } +// RoleReadOnly is the session role assigned to read-only (view-only) admin +// accounts. It matches the value stored by HandleLogin when the read-only +// credentials are used. +const RoleReadOnly = "readonly" + +// IsReadOnlyRole reports whether the given admin session role grants only +// view-only access. Any other role (admin, or the empty role used when auth +// is disabled) is treated as non-read-only. +func IsReadOnlyRole(role string) bool { + return role == RoleReadOnly +} + +// RedactSecretKey clears the plaintext S3 secret key from an object-store +// user record. The access key (a public identifier) is retained so the +// identity can still be listed; only the reusable secret is removed. +func (u *ObjectStoreUser) RedactSecretKey() { + u.SecretKey = "" +} + +// RedactSecretKeys clears the plaintext S3 secret keys from a user's access +// key records. Access key identifiers are retained so the set of keys remains +// visible; only the reusable secrets are removed. +func (d *UserDetails) RedactSecretKeys() { + for i := range d.AccessKeys { + d.AccessKeys[i].SecretKey = "" + } +} + type FilerNode struct { Address string `json:"address"` DataCenter string `json:"datacenter"` diff --git a/weed/admin/dash/auth_middleware.go b/weed/admin/dash/auth_middleware.go index d2c3ee595..c3b039c61 100644 --- a/weed/admin/dash/auth_middleware.go +++ b/weed/admin/dash/auth_middleware.go @@ -44,7 +44,7 @@ func (s *AdminServer) HandleLogin(store sessions.Store, adminUser, adminPassword authenticated = true } else if readOnlyPassword != "" && loginUsername == readOnlyUser && subtle.ConstantTimeCompare([]byte(loginPassword), []byte(readOnlyPassword)) == 1 { // Check read-only credentials. - role = "readonly" + role = RoleReadOnly authenticated = true } diff --git a/weed/admin/dash/readonly_secret_redaction_test.go b/weed/admin/dash/readonly_secret_redaction_test.go new file mode 100644 index 000000000..cf8e66d57 --- /dev/null +++ b/weed/admin/dash/readonly_secret_redaction_test.go @@ -0,0 +1,75 @@ +package dash + +import ( + "testing" +) + +// TestIsReadOnlyRole verifies that only the read-only session role is treated +// as view-only. The admin role and the empty role (used when auth is +// disabled) must not be treated as read-only, otherwise admins and no-auth +// deployments would lose legitimate access to credentials. +func TestIsReadOnlyRole(t *testing.T) { + if !IsReadOnlyRole(RoleReadOnly) { + t.Errorf("IsReadOnlyRole(%q) = false, want true", RoleReadOnly) + } + if !IsReadOnlyRole("readonly") { + t.Errorf("IsReadOnlyRole(\"readonly\") = false, want true") + } + if IsReadOnlyRole("admin") { + t.Errorf("IsReadOnlyRole(\"admin\") = true, want false") + } + if IsReadOnlyRole("") { + t.Errorf("IsReadOnlyRole(\"\") = true, want false; no-auth mode must not be redacted") + } +} + +// TestObjectStoreUserRedactSecretKey verifies that redaction clears the +// reusable secret while preserving the public access key identifier. +func TestObjectStoreUserRedactSecretKey(t *testing.T) { + u := ObjectStoreUser{ + Username: "victim", + AccessKey: "AKIAEXAMPLEKEY", + SecretKey: "super-secret-value", + } + u.RedactSecretKey() + if u.SecretKey != "" { + t.Errorf("SecretKey = %q, want empty after redaction", u.SecretKey) + } + if u.AccessKey == "" { + t.Errorf("AccessKey was emptied; only the secret should be redacted") + } + if u.Username == "" { + t.Errorf("Username was emptied; only the secret should be redacted") + } +} + +// TestUserDetailsRedactSecretKeys verifies that redaction clears every +// access key's secret while preserving the access key identifiers. +func TestUserDetailsRedactSecretKeys(t *testing.T) { + d := &UserDetails{ + Username: "victim", + AccessKeys: []AccessKeyInfo{ + {AccessKey: "AKIAONE", SecretKey: "secret-one"}, + {AccessKey: "AKIATWO", SecretKey: "secret-two"}, + }, + } + d.RedactSecretKeys() + for i, ak := range d.AccessKeys { + if ak.SecretKey != "" { + t.Errorf("AccessKeys[%d].SecretKey = %q, want empty after redaction", i, ak.SecretKey) + } + if ak.AccessKey == "" { + t.Errorf("AccessKeys[%d].AccessKey was emptied; only the secret should be redacted", i) + } + } +} + +// TestUserDetailsRedactSecretKeys_Empty verifies redaction is a no-op +// (and does not panic) when there are no access keys. +func TestUserDetailsRedactSecretKeys_Empty(t *testing.T) { + d := &UserDetails{Username: "victim"} + d.RedactSecretKeys() + if len(d.AccessKeys) != 0 { + t.Errorf("expected no access keys, got %d", len(d.AccessKeys)) + } +} diff --git a/weed/admin/handlers/user_handlers.go b/weed/admin/handlers/user_handlers.go index e439b3ad8..dfd327cc1 100644 --- a/weed/admin/handlers/user_handlers.go +++ b/weed/admin/handlers/user_handlers.go @@ -55,6 +55,14 @@ func (h *UserHandlers) GetUsers(w http.ResponseWriter, r *http.Request) { writeJSONError(w, http.StatusInternalServerError, "Failed to get users: "+err.Error()) return } + // Read-only (view-only) admin sessions must never receive reusable + // object-store secrets. Redact the secret key before serializing so a + // viewer cannot lift another user's live S3 credential pair. + if dash.IsReadOnlyRole(dash.RoleFromContext(r.Context())) { + for i := range users { + users[i].RedactSecretKey() + } + } writeJSON(w, http.StatusOK, map[string]interface{}{"users": users}) } @@ -156,6 +164,13 @@ func (h *UserHandlers) GetUserDetails(w http.ResponseWriter, r *http.Request) { return } + // Read-only (view-only) admin sessions must never receive reusable + // object-store secrets. Redact the secret keys before serializing so a + // viewer cannot lift another user's live S3 credential pair. + if dash.IsReadOnlyRole(dash.RoleFromContext(r.Context())) { + user.RedactSecretKeys() + } + writeJSON(w, http.StatusOK, user) } @@ -321,6 +336,14 @@ func (h *UserHandlers) getObjectStoreUsersData(r *http.Request) dash.ObjectStore } } + // Read-only (view-only) admin sessions must never receive reusable + // object-store secrets, including via the rendered HTML users page. + if dash.IsReadOnlyRole(dash.RoleFromContext(r.Context())) { + for i := range users { + users[i].RedactSecretKey() + } + } + hasAnonymous := false for _, u := range users { if u.Username == "anonymous" {