pkg/system/phase4_configuring.go (2)

218-227: READY path: same signing-key guard as above; prefer r.NooBaa.Name; add TTL

• Hydrate/validate server JWT before signing (same as metrics).
• Minor: use r.NooBaa.Name consistently for the "system" claim (less surprise than r.Request.Name).
• Add exp/iat to avoid never-expiring operator tokens.

@@
-			token, err := util.MakeAuthToken(map[string]any{
-				"system": r.Request.Name,
-				"role":   "operator",
-				"email":  options.OperatorAccountEmail,
-			}, []byte(r.SecretServer.StringData["jwt"]))
+			util.SecretResetStringDataFromData(r.SecretServer)
+			serverJWT := r.SecretServer.StringData["jwt"]
+			if serverJWT == "" {
+				return fmt.Errorf("server JWT secret is missing/empty")
+			}
+			token, err := util.MakeAuthToken(map[string]any{
+				"system": r.NooBaa.Name,
+				"role":   "operator",
+				"email":  options.OperatorAccountEmail,
+				/* requires util.MakeAuthToken to allow std claims:
+				"iat": time.Now().Unix(),
+				"exp": time.Now().Add(24*time.Hour).Unix(),
+				*/
+			}, []byte(serverJWT))

Confirm core PR 9211 accepts the "role", "system", and "email" claims exactly as used here for operator auth.

---

266-276: Endpoints admin token: add signing-key guard + TTL

• Same signing-key hydration/empty check.
• Add exp/iat for admin token as well.

@@
-	token, err := util.MakeAuthToken(map[string]any{
-		"system": r.Request.Name,
-		"role":   "admin",
-		"email":  options.AdminAccountEmail,
-	}, []byte(r.SecretServer.StringData["jwt"]))
+	util.SecretResetStringDataFromData(r.SecretServer)
+	serverJWT := r.SecretServer.StringData["jwt"]
+	if serverJWT == "" {
+		return fmt.Errorf("server JWT secret is missing/empty")
+	}
+	token, err := util.MakeAuthToken(map[string]any{
+		"system": r.NooBaa.Name,
+		"role":   "admin",
+		"email":  options.AdminAccountEmail,
+		/* requires util.MakeAuthToken to allow std claims:
+		"iat": time.Now().Unix(),
+		"exp": time.Now().Add(24*time.Hour).Unix(),
+		*/
+	}, []byte(serverJWT))

pkg/noobaaaccount/reconciler.go (2)

48-50: SecretServer field is unused

Either wire this into the reconcile flow or remove to avoid confusion.

---

372-377: JWT best-practices: set issuer and TTL via MakeAuthToken (follow-up to util change)

Once MakeAuthToken adds standard claims, pass issuer/TTL as needed (or rely on defaults). No change required here if defaults are acceptable.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/nb/types.go (1)

568-577: API break: UpdateAccountParams.Name tag must remain name (not username)
This will break consumers of account_api.update_account() expecting name.

-type UpdateAccountParams struct {
-    Name       *string `json:"username,omitempty"`
+type UpdateAccountParams struct {
+    Name       *string `json:"name,omitempty"`
     Email      string  `json:"email"`
     NewEmail   *string `json:"new_email,omitempty"`
     AllowedIPs *[]struct {
         Start string `json:"start"`
         End   string `json:"end"`
     } `json:"ips,omitempty"`
     RoleConfig       interface{} `json:"role_config,omitempty"`
     RemoveRoleConfig bool        `json:"remove_role_config,omitempty"`
 }

Also update the struct comment above to reference account_api.update_account() instead of update_account_s3_access().

pkg/noobaaaccount/reconciler.go (2)

365-371: Guard missing/empty JWT key and prefer Secret.Data over StringData
Currently, if the secret lacks "jwt", token gets signed with an empty key. Use Data and validate presence.

-    if !util.KubeCheck(secretServer) {
-        return fmt.Errorf("cannot create an auth token for remote operator - server secret not found")
-    }
+    if !util.KubeCheck(secretServer) {
+        return fmt.Errorf("cannot create an auth token for remote operator - server secret not found")
+    }
+    jwtBytes := secretServer.Data["jwt"]
+    if len(jwtBytes) == 0 {
+        return fmt.Errorf("cannot create an auth token for remote operator - missing \"jwt\" key in server secret %q", secretServer.Name)
+    }

---

380-384: Prevent panics: check AccessKeys length and init StringData before writes
Indexing [0] can panic; StringData may be nil.

-    accessKeys := accountInfo.AccessKeys[0]
-    r.Secret.StringData["auth_token"] = token
-    r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey)
-    r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey)
+    var accessKeys nb.S3AccessKeys
+    if len(accountInfo.AccessKeys) == 0 {
+        log.Info("CreateAccountAPI did not return access keys. calling ReadAccountAPI to get keys..")
+        readAccountReply, err := r.NBClient.ReadAccountAPI(nb.ReadAccountParams{Email: r.NooBaaAccount.Name})
+        if err != nil {
+            return err
+        }
+        accessKeys = readAccountReply.AccessKeys[0]
+    } else {
+        accessKeys = accountInfo.AccessKeys[0]
+    }
+    if r.Secret.StringData == nil {
+        r.Secret.StringData = map[string]string{}
+    }
+    r.Secret.StringData["auth_token"] = token
+    r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey)
+    r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey)

pkg/system/phase4_configuring.go (1)

156-166: Hydrate server JWT secret and add exp/iat (TTL) to metrics token.

Load r.SecretServer.StringData from Data, ensure the signing key is non-empty, and set standard JWT timing claims to avoid never-expiring tokens.

Apply:

-	token, err := util.MakeAuthToken(map[string]any{
-		"system": r.NooBaa.Name,
-		"role":   "metrics",
-		"email":  options.OperatorAccountEmail,
-	}, []byte(r.SecretServer.StringData["jwt"]))
+	// Ensure signing key is loaded and non-empty
+	util.SecretResetStringDataFromData(r.SecretServer)
+	serverJWT := r.SecretServer.StringData["jwt"]
+	if serverJWT == "" {
+		return fmt.Errorf("server JWT secret is missing/empty")
+	}
+	token, err := util.MakeAuthToken(map[string]any{
+		"system": r.NooBaa.Name,
+		"role":   "metrics",
+		"email":  options.OperatorAccountEmail,
+		// token hardening
+		"iat": time.Now().Unix(),
+		"exp": time.Now().Add(1 * time.Hour).Unix(),
+	}, []byte(serverJWT))

Supporting change outside this file (to let util keep these std claims):

--- a/pkg/util/util.go
+++ b/pkg/util/util.go
@@
-	allowedFields := []string{"account_id", "system_id", "role", "extra", "authorized_by", "email", "system"}
+	// include standard timing claims
+	allowedFields := []string{"account_id", "system_id", "role", "extra", "authorized_by", "email", "system", "iat", "exp", "nbf"}

Note: plan rotation for metrics_token on JWT key change/expiry (re-mint token and update SecretMetricsAuth to trigger reloads).

pkg/backingstore/backingstore.go (1)

947-949: Nit: simplify interval expression

Use a duration literal to avoid multiplying by time.Second at the call site.

-   interval := time.Duration(30)
-   err := wait.PollUntilContextCancel(ctx, interval*time.Second, true, func(ctx context.Context) (bool, error) {
+   interval := 30 * time.Second
+   err := wait.PollUntilContextCancel(ctx, interval, true, func(ctx context.Context) (bool, error) {

pkg/noobaaaccount/reconciler.go (1)

48-50: SecretServer field is unused

Either wire it into the token generation path or drop it to avoid confusion.

pkg/system/phase4_configuring.go (1)

415-426: Minor typos in comments/logs.

Low-impact polish for operator logs and comments.

@@
-						// Ignore mailformed addresses
+						// Ignore malformed addresses
@@
-						// Ignore mailformed addresses
+						// Ignore malformed addresses

@@
-	if _, err := url.Parse(endpoint); err != nil {
-		r.Logger.Errorf("Invalid formate URL %q", endpoint)
-		return fmt.Errorf("Invalid formate URL %q", endpoint)
+	if _, err := url.Parse(endpoint); err != nil {
+		r.Logger.Errorf("Invalid URL format %q", endpoint)
+		return fmt.Errorf("invalid URL format %q", endpoint)

@@
-	// make sure the name is ended with a valid charecter
+	// make sure the name ends with a valid character

Also applies to: 1329-1332, 1499-1501

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/noobaaaccount/reconciler.go (2)

364-369: Guard: verify server secret contains “jwt” and use Secret.Data bytes.

Avoid signing with a missing/empty key and prefer Data over StringData.

Apply:

 			secretServer := util.KubeObject(bundle.File_deploy_internal_secret_empty_yaml).(*corev1.Secret)
 			secretServer.Namespace = r.Request.Namespace
 			secretServer.Name = options.SystemName
 			if !util.KubeCheck(secretServer) {
 				return fmt.Errorf("cannot create an auth token for remote operator - server secret not found")
 			}
-
+			jwtBytes, ok := secretServer.Data["jwt"]
+			if !ok || len(jwtBytes) == 0 {
+				return fmt.Errorf("cannot create an auth token for remote operator - missing %q key in server secret %q", "jwt", secretServer.Name)
+			}

---

379-383: Fix potential panic and ensure StringData is initialized.

AccessKeys[0] can be empty on older cores; also StringData may be nil.

Apply:

-			accessKeys := accountInfo.AccessKeys[0]
-			r.Secret.StringData["auth_token"] = token
-			r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey)
-			r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey)
+			var accessKeys nb.S3AccessKeys
+			if len(accountInfo.AccessKeys) == 0 {
+				log.Info("CreateAccountAPI did not return access keys. calling ReadAccountAPI to get keys..")
+				readAccountReply, err := r.NBClient.ReadAccountAPI(nb.ReadAccountParams{Email: r.NooBaaAccount.Name})
+				if err != nil {
+					return err
+				}
+				if len(readAccountReply.AccessKeys) == 0 {
+					return fmt.Errorf("no access keys found for account %q", r.NooBaaAccount.Name)
+				}
+				accessKeys = readAccountReply.AccessKeys[0]
+			} else {
+				accessKeys = accountInfo.AccessKeys[0]
+			}
+			if r.Secret.StringData == nil {
+				r.Secret.StringData = map[string]string{}
+			}
+			r.Secret.StringData["auth_token"] = token
+			r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey)
+			r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey)

test/cli/test_cli_flow.sh (1)

34-34: Don’t silently drop a destructive test; gate it behind a flag.

Keep coverage while avoiding accidental data wipes in CI.

Apply:

-    # check_default_backingstore #It deletes all the buckets and non-default accounts, creates new backingstore and attach it to default admin account and then deletes default backingstore
+    # It deletes all the buckets and non-default accounts, creates new backingstore and attach it to default admin account and then deletes default backingstore
+    if [[ "${RUN_DESTRUCTIVE_TESTS:-false}" == "true" ]]; then
+        check_default_backingstore
+    fi

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/util/util.go (1)

2393-2410: JWTs lack exp/iat and accept weak/empty keys

Tokens never expire and HS256 signs even with very short secrets. Enforce a minimum key length and add standard time claims by default. Also allow callers to override/augment with exp/iat if needed.

Apply:

-func MakeAuthToken(payload map[string]any, secret []byte) (string, error) {
-	if len(secret) == 0 {
-		return "", fmt.Errorf("jwt signing secret missing")
-	}
-
-	filtered := jwt.MapClaims{}
-
-	allowedFields := []string{"account_id", "system_id", "role", "extra", "authorized_by", "email", "system"}
-	for _, field := range allowedFields {
-		val, ok := payload[field]
-		if ok {
-			filtered[field] = val
-		}
-	}
-
-	return jwt.NewWithClaims(jwt.SigningMethodHS256, filtered).SignedString(secret)
-}
+func MakeAuthToken(payload map[string]any, secret []byte) (string, error) {
+	if len(secret) < 32 {
+		return "", fmt.Errorf("jwt signing secret missing or too short")
+	}
+	filtered := jwt.MapClaims{}
+	// allow selected custom and standard claims
+	allowedFields := []string{
+		"account_id", "system_id", "role", "extra", "authorized_by", "email", "system",
+		"iss", "sub", "iat", "exp", "nbf",
+	}
+	for _, field := range allowedFields {
+		if val, ok := payload[field]; ok {
+			filtered[field] = val
+		}
+	}
+	// set defaults if not provided
+	now := time.Now().UTC()
+	if _, ok := filtered["iat"]; !ok {
+		filtered["iat"] = now.Unix()
+	}
+	if _, ok := filtered["exp"]; !ok {
+		filtered["exp"] = now.Add(15 * time.Minute).Unix()
+	}
+	if _, ok := filtered["iss"]; !ok {
+		filtered["iss"] = "noobaa-operator"
+	}
+	if _, ok := filtered["sub"]; !ok {
+		if v, ok := filtered["account_id"]; ok {
+			filtered["sub"] = v
+		} else if v, ok := filtered["email"]; ok {
+			filtered["sub"] = v
+		}
+	}
+	token := jwt.NewWithClaims(jwt.SigningMethodHS256, filtered)
+	return token.SignedString(secret)
+}

pkg/noobaaaccount/reconciler.go (2)

364-375: Guard jwt key and add token TTL; sign with validated bytes

Ensure the server secret contains a non-empty "jwt" key before signing. Also make tokens short-lived.

Apply:

-            if !util.KubeCheck(secretServer) {
+            if !util.KubeCheck(secretServer) {
                 return fmt.Errorf("cannot create an auth token for remote operator - server secret not found")
             }
-
-            token, err := util.MakeAuthToken(map[string]any{
-                "system": r.NooBaa.Name,
-                "role":   "operator",
-                "email":  options.OperatorAccountEmail,
-            }, []byte(secretServer.StringData["jwt"]))
+            // hydrate and validate JWT key
+            util.SecretResetStringDataFromData(secretServer)
+            jwtKey := strings.TrimSpace(secretServer.StringData["jwt"])
+            if jwtKey == "" {
+                return fmt.Errorf("cannot create an auth token for remote operator - missing \"jwt\" key in server secret %q", secretServer.Name)
+            }
+            token, err := util.MakeAuthToken(map[string]any{
+                "system": r.NooBaa.Name,
+                "role":   "operator",
+                "email":  options.OperatorAccountEmail,
+                "iat":    time.Now().UTC().Unix(),
+                "exp":    time.Now().UTC().Add(24 * time.Hour).Unix(),
+            }, []byte(jwtKey))

---

379-383: Possible panic: AccessKeys[0] without length check

When AccessKeys is empty, indexing [0] panics. Mirror the fallback used in the non-remote path.

Apply:

-            accessKeys := accountInfo.AccessKeys[0]
-            r.Secret.StringData["auth_token"] = token
-            r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey)
-            r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey)
+            var accessKeys nb.S3AccessKeys
+            if len(accountInfo.AccessKeys) == 0 {
+                log.Info("CreateAccountAPI did not return access keys. calling ReadAccountAPI to get keys..")
+                readAccountReply, err := r.NBClient.ReadAccountAPI(nb.ReadAccountParams{Email: r.NooBaaAccount.Name})
+                if err != nil || len(readAccountReply.AccessKeys) == 0 {
+                    return fmt.Errorf("failed to obtain access keys for account %q", r.NooBaaAccount.Name)
+                }
+                accessKeys = readAccountReply.AccessKeys[0]
+            } else {
+                accessKeys = accountInfo.AccessKeys[0]
+            }
+            r.Secret.StringData["auth_token"] = token
+            r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey)
+            r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey)

pkg/system/phase4_configuring.go (3)

157-166: Metrics token: guard empty signing key and add exp/iat

Avoid minting with an empty key; add short TTL for metrics.

Apply:

-    token, err := util.MakeAuthToken(map[string]any{
-        "system": r.NooBaa.Name,
-        "role":   "metrics",
-        "email":  options.OperatorAccountEmail,
-    }, []byte(r.SecretServer.StringData["jwt"]))
+    util.SecretResetStringDataFromData(r.SecretServer)
+    serverJWT := strings.TrimSpace(r.SecretServer.StringData["jwt"])
+    if serverJWT == "" {
+        return fmt.Errorf("server JWT secret is missing/empty")
+    }
+    token, err := util.MakeAuthToken(map[string]any{
+        "system": r.NooBaa.Name,
+        "role":   "metrics",
+        "email":  options.OperatorAccountEmail,
+        "iat":    time.Now().UTC().Unix(),
+        "exp":    time.Now().UTC().Add(1 * time.Hour).Unix(),
+    }, []byte(serverJWT))

---

218-227: Operator token: guard signing key and add exp/iat

Same concern as metrics; make operator tokens time-bound.

Apply:

-            token, err := util.MakeAuthToken(map[string]any{
-                "system": r.Request.Name,
-                "role":   "operator",
-                "email":  options.OperatorAccountEmail,
-            }, []byte(r.SecretServer.StringData["jwt"]))
+            util.SecretResetStringDataFromData(r.SecretServer)
+            serverJWT := strings.TrimSpace(r.SecretServer.StringData["jwt"])
+            if serverJWT == "" {
+                return fmt.Errorf("server JWT secret is missing/empty")
+            }
+            token, err := util.MakeAuthToken(map[string]any{
+                "system": r.Request.Name,
+                "role":   "operator",
+                "email":  options.OperatorAccountEmail,
+                "iat":    time.Now().UTC().Unix(),
+                "exp":    time.Now().UTC().Add(24 * time.Hour).Unix(),
+            }, []byte(serverJWT))

---

266-276: Admin/endpoints token: guard signing key and add exp/iat

Prevent empty-key signing; include expiry.

Apply:

-    token, err := util.MakeAuthToken(map[string]any{
-        "system": r.Request.Name,
-        "role":   "admin",
-        "email":  options.AdminAccountEmail,
-    }, []byte(r.SecretServer.StringData["jwt"]))
+    util.SecretResetStringDataFromData(r.SecretServer)
+    serverJWT := strings.TrimSpace(r.SecretServer.StringData["jwt"])
+    if serverJWT == "" {
+        return fmt.Errorf("server JWT secret is missing/empty")
+    }
+    token, err := util.MakeAuthToken(map[string]any{
+        "system": r.Request.Name,
+        "role":   "admin",
+        "email":  options.AdminAccountEmail,
+        "iat":    time.Now().UTC().Unix(),
+        "exp":    time.Now().UTC().Add(24 * time.Hour).Unix(),
+    }, []byte(serverJWT))

pkg/nb/types.go (1)

568-577: JSON tag change breaks API: use name, not username

account_api.update_account expects "name"; switching to "username" will break clients.

Apply:

-    Name       *string `json:"username,omitempty"`
+    Name       *string `json:"name,omitempty"`

Also consider updating the struct comment to reference account_api.update_account().

pkg/util/util.go (1)

1851-1862: Optional: use duration/context for polling

Hard-coding 5*60 seconds is fine; consider a time.Duration or context.WithTimeout to make this reusable/configurable.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/util/util.go (1)

2394-2410: JWTs lack exp/iat and accept trivially short secrets — add standard claims and enforce key length

Tokens are long-lived if leaked and can be signed with an empty/weak key. Add exp/iat (and optional iss/sub defaults) and require a minimum HS256 key length.

Apply:

 func MakeAuthToken(payload map[string]any, secret []byte) (string, error) {
-	if len(secret) == 0 {
-		return "", fmt.Errorf("jwt signing secret missing")
-	}
+	if len(secret) < 32 {
+		return "", fmt.Errorf("jwt signing secret missing or too short")
+	}

 	filtered := jwt.MapClaims{}

 	allowedFields := []string{"account_id", "system_id", "role", "extra", "authorized_by", "email", "system"}
 	for _, field := range allowedFields {
 		val, ok := payload[field]
 		if ok {
 			filtered[field] = val
 		}
 	}

-	return jwt.NewWithClaims(jwt.SigningMethodHS256, filtered).SignedString(secret)
+	now := time.Now().UTC()
+	filtered["iat"] = now.Unix()
+	filtered["exp"] = now.Add(15 * time.Minute).Unix()
+	if _, ok := filtered["iss"]; !ok {
+		filtered["iss"] = "noobaa-operator"
+	}
+	if _, ok := filtered["sub"]; !ok {
+		if v, ok := filtered["account_id"]; ok {
+			filtered["sub"] = v
+		} else if v, ok := filtered["email"]; ok {
+			filtered["sub"] = v
+		}
+	}
+	return jwt.NewWithClaims(jwt.SigningMethodHS256, filtered).SignedString(secret)
 }

Follow-up:

• If noobaa-core expects specific iss/aud, set them accordingly; otherwise consider adding aud and verifying at the receiver.
• If different TTLs are needed (operator vs metrics), we can add a TTL parameter variant while keeping this default.

I can thread TTL/issuer across call sites if you want.

pkg/noobaaaccount/noobaaaccount.go (1)

265-275: Differentiate NotFound from other RPC failures

Replace the simple err == nil checks with explicit handling of “not found” RPC codes and fail fast on other errors:

-_, err = sysClient.NBClient.ReadPoolAPI(nb.ReadPoolParams{Name: newDefaultResource})
-isResourceBackingStore := err == nil
+isResourceBackingStore := false
+if _, err := sysClient.NBClient.ReadPoolAPI(nb.ReadPoolParams{Name: newDefaultResource}); err == nil {
+    isResourceBackingStore = true
+} else if rpcErr, ok := err.(*nb.RPCError); ok && rpcErr.RPCCode == "NO_SUCH_POOL" {
+    // backing store does not exist
+} else {
+    log.Fatalf("❌ ReadPoolAPI failed: %v", err)
+}

-_, err = sysClient.NBClient.ReadNamespaceResourceAPI(nb.ReadNamespaceResourceParams{Name: newDefaultResource})
-isResourceNamespaceStore := err == nil
+isResourceNamespaceStore := false
+if _, err := sysClient.NBClient.ReadNamespaceResourceAPI(nb.ReadNamespaceResourceParams{Name: newDefaultResource}); err == nil {
+    isResourceNamespaceStore = true
+} else if rpcErr, ok := err.(*nb.RPCError); ok && rpcErr.RPCCode == "NO_SUCH_NAMESPACE_RESOURCE" {
+    // namespace store does not exist
+} else {
+    log.Fatalf("❌ ReadNamespaceResourceAPI failed: %v", err)
+}

RPC codes “NO_SUCH_POOL” and “NO_SUCH_NAMESPACE_RESOURCE” are confirmed in the codebase. Also consider reusing the existing sysClient in RunUpdate rather than reconnecting.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/system/phase4_configuring.go (3)

157-161: Critical: Validate JWT secret before token generation.

The code directly uses r.SecretServer.StringData["jwt"] without checking if it exists or is empty. If the JWT secret is missing or empty, util.MakeAuthToken will fail or create invalid tokens.

+// Ensure the JWT secret is available and non-empty
+util.SecretResetStringDataFromData(r.SecretServer)
+jwtSecret := r.SecretServer.StringData["jwt"]
+if jwtSecret == "" {
+    return fmt.Errorf("server JWT secret is missing or empty")
+}
 token, err := util.MakeAuthToken(map[string]any{
     "system": r.NooBaa.Name,
     "role":   "metrics",
     "email":  options.OperatorAccountEmail,
-}, []byte(r.SecretServer.StringData["jwt"]))
+}, []byte(jwtSecret))

---

218-222: Critical: Validate JWT secret before operator token generation.

Similar to the metrics token generation, the JWT secret should be validated before use.

+// Ensure the JWT secret is available and non-empty
+util.SecretResetStringDataFromData(r.SecretServer)
+jwtSecret := r.SecretServer.StringData["jwt"]
+if jwtSecret == "" {
+    return fmt.Errorf("server JWT secret is missing or empty")
+}
 token, err := util.MakeAuthToken(map[string]any{
     "system": r.Request.Name,
     "role":   "operator",
     "email":  options.OperatorAccountEmail,
-}, []byte(r.SecretServer.StringData["jwt"]))
+}, []byte(jwtSecret))

---

266-270: Critical: Validate JWT secret before admin token generation.

The JWT secret validation is needed here as well for consistency and safety.

+// Ensure the JWT secret is available and non-empty
+util.SecretResetStringDataFromData(r.SecretServer)
+jwtSecret := r.SecretServer.StringData["jwt"]
+if jwtSecret == "" {
+    return fmt.Errorf("server JWT secret is missing or empty")
+}
 token, err := util.MakeAuthToken(map[string]any{
     "system": r.Request.Name,
     "role":   "admin",
     "email":  options.AdminAccountEmail,
-}, []byte(r.SecretServer.StringData["jwt"]))
+}, []byte(jwtSecret))

test/cli/test_cli_functions.sh (1)

816-816: Centralize & document default-resource normalization ('backingstores' → 'noobaa-default-backing-store').

Normalization exists only in test/cli/test_cli_functions.sh (line 816); many tests/code reference the literal "noobaa-default-backing-store". Create a shared helper (or document the conversion) and update callers to use it.

Key locations: test/cli/test_cli_functions.sh:816 (normalization), test/cli/test_cli_functions.sh:703,1152,1166,1198–1206; pkg/cosi/cosi_cli_test.go; pkg/cosi/cosi_test.go; pkg/admission/test/integ/admission_integ_test.go.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

pkg/noobaaaccount/reconciler.go (2)

364-376: Guard missing/empty JWT key and use validated bytes; add token expiry
If "jwt" is absent/blank, you’ll sign with an empty key. Also add exp/iat to avoid indefinite tokens. Use trimmed, validated bytes for signing.

Apply:

-			secretServer := util.KubeObject(bundle.File_deploy_internal_secret_empty_yaml).(*corev1.Secret)
+			secretServer := util.KubeObject(bundle.File_deploy_internal_secret_empty_yaml).(*corev1.Secret)
 			secretServer.Namespace = r.Request.Namespace
 			secretServer.Name = options.SystemName
 			if !util.KubeCheck(secretServer) {
 				return fmt.Errorf("cannot create an auth token for remote operator - server secret not found")
 			}
 
-			token, err := util.MakeAuthToken(map[string]any{
-				"system": r.NooBaa.Name,
-				"role":   "operator",
-				"email":  options.OperatorAccountEmail,
-			}, []byte(secretServer.StringData["jwt"]))
+			jwtKey := strings.TrimSpace(secretServer.StringData["jwt"])
+			if jwtKey == "" {
+				return fmt.Errorf("cannot create an auth token for remote operator - missing \"jwt\" key in server secret %q", secretServer.Name)
+			}
+			jwtBytes := []byte(jwtKey)
+
+			token, err := util.MakeAuthToken(map[string]any{
+				"system": r.NooBaa.Name,
+				"role":   "operator",
+				"email":  options.OperatorAccountEmail,
+				"iat":    time.Now().Unix(),
+				"exp":    time.Now().Add(24 * time.Hour).Unix(),
+			}, jwtBytes)

Additionally update util.MakeAuthToken allowlist to include exp/iat:

-	allowedFields := []string{"account_id", "system_id", "role", "extra", "authorized_by", "email", "system"}
+	allowedFields := []string{"account_id", "system_id", "role", "extra", "authorized_by", "email", "system", "exp", "iat"}

---

379-383: Nil map write and potential panic on AccessKeys[0]
StringData may be nil; writing to it panics. Also AccessKeys can be empty on older cores.

Apply:

-			accessKeys := accountInfo.AccessKeys[0]
-			r.Secret.StringData["auth_token"] = token
-			r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey)
-			r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey)
+			if r.Secret.StringData == nil {
+				r.Secret.StringData = map[string]string{}
+			}
+			var accessKeys nb.S3AccessKeys
+			if len(accountInfo.AccessKeys) == 0 {
+				log.Info("CreateAccountAPI did not return access keys. calling ReadAccountAPI to get keys..")
+				readAccountReply, err := r.NBClient.ReadAccountAPI(nb.ReadAccountParams{Email: r.NooBaaAccount.Name})
+				if err != nil || len(readAccountReply.AccessKeys) == 0 {
+					return fmt.Errorf("failed retrieving access keys for %q: %v", r.NooBaaAccount.Name, err)
+				}
+				accessKeys = readAccountReply.AccessKeys[0]
+			} else {
+				accessKeys = accountInfo.AccessKeys[0]
+			}
+			r.Secret.StringData["auth_token"] = token
+			r.Secret.StringData["AWS_ACCESS_KEY_ID"] = string(accessKeys.AccessKey)
+			r.Secret.StringData["AWS_SECRET_ACCESS_KEY"] = string(accessKeys.SecretKey)

pkg/backingstore/backingstore.go (1)

947-949: 30s poll hurts UX; prefer shorter or configurable interval

30s between status checks makes CLI feel stalled and slows tests. Suggest 10s (or make it flag/env-configurable).

Apply:

-	interval := time.Duration(30)
+	// Poll every ~10s for a snappier UX; keep overall controller-side backoff unchanged.
+	interval := time.Duration(10)

test/cli/test_cli_functions.sh (2)

816-816: Avoid hardcoded alias “backingstores”; normalize via actual default resource
Literal “backingstores” is brittle across core versions/config. Prefer querying the system’s default and mapping once.

Apply:

-        if [ "${default_resource}" == "backingstores" ]; then default_resource="noobaa-default-backing-store"; fi
+        # Normalize default_resource from legacy alias to actual name
+        if [ "${default_resource}" == "backingstores" ] || [ -z "${default_resource}" ]; then
+            default_resource=$(test_noobaa silence api account list_accounts {} -o json | jq -r '.accounts[0].default_resource')
+        fi

---

1195-1195: Operator account may not exist — gate update or create first
If operator@noobaa.io isn’t present, this line fails the suite. Guard or ensure creation.

Apply:

-    test_noobaa account update operator@noobaa.io --new_default_resource=new-default-backing-store
+    if test_noobaa silence account list | awk '{print $1}' | grep -qx "operator@noobaa.io"; then
+        test_noobaa account update operator@noobaa.io --new_default_resource=new-default-backing-store
+    else
+        echo_time "ℹ️  Skipping operator account update (operator@noobaa.io not found)"
+    fi

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro