sec(auth): fail-closed MFA, CSRF HMAC derivation, bounded last-used, errors.Is (#1018 #1027 #1028 #1029) by cristim · Pull Request #1044 · LeanerCloud/cloud-commitments-cli

cristim · 2026-06-07T10:07:35Z

Summary

Four security fixes across internal/auth and internal/credentials, implemented together because they share the same auth primitives and test infrastructure.

#1018 - MFA primitives fail closed on empty input

verifyTOTP("","") returned true before this fix: generateTOTP returns "" on base32-decode failure, and ConstantTimeCompare("","") == 1. An unguarded caller (MFARegenerateRecoveryCodes) could regenerate recovery codes with code="".

verifyTOTP: early-return false when code=="" or secret==""
consumeRecoveryCode: early-return false when normalized input is empty
Fix misleading comment at service.go:~212

#1027 - API key last-used update bounded via singleflight

ValidateUserAPIKey spawned one go func() per request to update last_used_at, with no concurrency bound. A burst of authenticated requests (or a revoked key being presented) spawned unbounded goroutines and logged Warn on every miss.

Replaced raw goroutine with singleflight.Group.Do per key ID: at most one DB write in-flight per key; concurrent requests are coalesced
Demoted "failed to update last-used" log from Warn to Debug

#1028 - credentials/LoadRaw uses errors.Is for pgx.ErrNoRows

store.go:LoadRaw used err == pgx.ErrNoRows (pointer equality). Pool and transaction wrappers wrap the sentinel; the check returned false and LoadRaw propagated a hard error instead of the documented (nil, nil) "not found" contract.

Changed to errors.Is(err, pgx.ErrNoRows) to unwrap the chain

#1029 - CSRF token derived as HMAC, not stored cleartext

The CSRF token was a random hex string stored plaintext in sessions.csrf_token. A DB read (SQLi, backup, replica) yielded a directly usable CSRF token.

createSession now derives CSRF as HMAC-SHA256(csrfKey, rawSessionToken)
ValidateCSRFToken recomputes the HMAC from the raw session token; the stored column is NOT read during validation
csrfKey added to Service/ServiceConfig; if omitted, a random ephemeral key is generated with a startup warning
No DB migration needed; old sessions will require re-login after deploy

Verification per issue

Issue	Pre-fix behavior	Post-fix behavior	Regression test
#1018	`verifyTOTP("","")` returns `true`	returns `false`	`TestVerifyTOTP_FailClosed`
#1018	`consumeRecoveryCode(user, "")` calls bcrypt on empty	returns `false` immediately	`TestConsumeRecoveryCode_EmptyInput`
#1018	`MFARegenerateRecoveryCodes(..., "")` calls `UpdateUser`	returns error, no mutation	`TestMFARegenerateRecoveryCodes_EmptyCode`
#1027	Unbounded goroutines per request	singleflight: 1 DB write per burst	`TestValidateUserAPIKey_LastUsedSingleflight`
#1028	Wrapped `ErrNoRows` returns hard error	`errors.Is` unwraps correctly	`TestLoadRaw_ErrorsIs_WrappedNoRows`
#1029	Stored plaintext CSRF readable from DB	HMAC; DB read cannot forge token	`TestCSRFToken_DerivedFromSessionToken`, `TestCSRFToken_CrossSessionRejected`

Build: go build ./... clean.
Vet: go vet ./internal/auth/... ./internal/credentials/... clean.
Tests: 631 pass; 2 pre-existing failures in TestLogin_WithMFA_NoSecret (unrelated to this PR, present on base branch).

Closes #1018
Closes #1027
Closes #1028
Closes #1029

Scope expanded

Additional commits from FOLD-1044 (docs/code-review/16-remaining-findings-plan.md): 14 Medium/Low/Nit findings from docs/code-review/03-auth-secrets.md added as separate atomic commits.

03-M1 - Explicit AES-256 key-length guard

Encrypt/Decrypt now reject non-32-byte keys with a clear error before passing them to aes.NewCipher. Prevents silent AES-128/192 downgrade when a caller supplies a key directly.

03-M2 - Multi-key env var config is now a hard error

Setting more than one CREDENTIAL_ENCRYPTION_KEY_* env var simultaneously now returns ErrMultipleKeys instead of logging a warning and using the first. Ambiguous key selection on a data-encryption key is a startup misconfiguration.

03-M3 - GCP ListSecrets uses client-side HasPrefix

GCPResolver.ListSecrets was forwarding the caller's filter to GCP's expression-language Filter field, violating the Resolver interface doc which states all backends use strings.HasPrefix. Fixed to list all secrets and apply prefix client-side.

03-M5 - Password reset rate-limit per email

RequestPasswordReset now silently no-ops when a token was issued within PasswordResetRateLimit (1 minute). Prevents a griefing attack where repeated calls perpetually invalidate the victim's reset link.

03-M6 - Azure exponent overflow guard

AzureKeyVaultSigner.resolveOnce now validates the public exponent fits int64 and is in the valid range before int() conversion. A truncated exponent would produce a broken JWKS.

03-M7 - Precise error for half-configured Azure OIDC

NewSignerFromEnv now returns a precise "set both vars or neither" error when exactly one of CUDLY_SIGNING_KEY_VAULT_URL / CUDLY_SIGNING_KEY_NAME is set, instead of deferring to the constructor's generic error.

03-L1 through 03-L6 - Low-severity hardening

L1: redactEmail masks domain hostname in addition to local part
L2: apiKey[:8] guarded with len check
L3: containsSequentialChars renamed to containsRepeatedChars (was checking repeated, not sequential, chars)
L4: SetIssuerURL validates absolute https:// before caching; callers updated
L5: IsUnrestrictedAccess doc extended with fail-open WARNING
L6: matchStringListConstraints doc extended explaining empty-list semantics

03-N4 and 03-N5 - Nit improvements

N4: generateToken encodes raw bytes directly; SHA-256 of CSPRNG output adds nothing
N5: AWS access key and Azure client secret payloads validate required fields non-empty after unmarshal

Build: go build ./... clean.
Vet: go vet ./internal/auth/... ./internal/credentials/... ./internal/oidc/... ./internal/secrets/... clean.
Tests: pre-existing 2 failures in TestLogin_WithMFA_NoSecret unchanged; all new regression tests pass.

Also closes #1058

…put (#1018) verifyTOTP("","") previously returned true because generateTOTP("") returns "" on base32-decode failure, and ConstantTimeCompare("","") == 1. This created an MFA-bypass primitive callable by any session holder via MFARegenerateRecoveryCodes with code="". - verifyTOTP: early-return false when code=="" or secret=="" - consumeRecoveryCode: early-return false when normalized input is empty - Fix misleading comment at verifyPasswordAndMFA (service.go:212) that described the old panic-safe behaviour; the function now fails closed Closes #1018

ValidateUserAPIKey fired one unbounded goroutine per request to update the last-used timestamp. Under a burst of authenticated requests (or a revoked key being presented repeatedly), this spawned an unbounded number of goroutines writing to the DB with context.Background() and logged a Warn for every "key not found" event. - Replace raw go func() with singleflight.Group.Do: at most one DB write per key is in-flight at any moment; concurrent requests for the same key are coalesced rather than each spawning a writer - Demote "failed to update last-used" log from Warn to Debug Closes #1027

…#1028) store.go:LoadRaw used direct pointer equality (err == pgx.ErrNoRows). Pool and transaction wrappers wrap the sentinel, so the check returned false and LoadRaw propagated a hard error instead of the documented (nil, nil) "not found" contract, breaking callers like resolveAccessKeyProvider. - Change to errors.Is(err, pgx.ErrNoRows) to unwrap the error chain - Add errors import Closes #1028

…dom (#1029) The CSRF token was a random hex string stored plaintext in the sessions row alongside the hashed session token. A DB read (SQLi, backup, replica) yielded a directly usable CSRF token, undermining the double-submit protection. New approach (no DB migration needed): - createSession derives CSRF as HMAC-SHA256(csrfKey, rawSessionToken) - The MAC is written to csrf_token for diagnostics/observability but is never used for validation - ValidateCSRFToken recomputes the HMAC from the raw session token and compares with the presented token; the stored column is not read - csrfKey added to Service struct and ServiceConfig; if not supplied, NewService generates a random ephemeral key and logs a warning The token is now cryptographically bound to the session token AND the server-side key. A DB read alone cannot produce a forgeable CSRF token. Closes #1029

…1029 - TestVerifyTOTP_FailClosed: verifyTOTP("","") must return false (was true) - TestConsumeRecoveryCode_EmptyInput: empty code must be rejected before bcrypt - TestMFARegenerateRecoveryCodes_EmptyCode: empty code must return error without calling UpdateUser - TestCSRFToken_DerivedFromSessionToken: CSRF equals HMAC(csrfKey, rawToken); ValidateCSRFToken accepts derived token, rejects cleartext legacy tokens - TestCSRFToken_CrossSessionRejected: CSRF token from session A is rejected for B - TestValidateUserAPIKey_LastUsedSingleflight: background last-used update runs; done channel replaces time.Sleep per feedback_no_sleep_in_tests - TestLoadRaw_ErrorsIs_WrappedNoRows: errors.Is unwraps a wrapped pgx.ErrNoRows where == would return false (the pre-fix bug)

coderabbitai · 2026-06-07T10:07:41Z

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 48 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 83e2a776-34fe-4e74-b150-4443683c8ea1

📥 Commits

Reviewing files that changed from the base of the PR and between cbdc4be and 04b9a99.

📒 Files selected for processing (28)

internal/api/handler_oidc.go
internal/auth/service.go
internal/auth/service_apikeys.go
internal/auth/service_group.go
internal/auth/service_helpers.go
internal/auth/service_mfa.go
internal/auth/service_password.go
internal/auth/service_password_test.go
internal/auth/service_security_test.go
internal/auth/service_test.go
internal/auth/test_helpers.go
internal/auth/types.go
internal/credentials/cipher.go
internal/credentials/cipher_extra_test.go
internal/credentials/resolver.go
internal/credentials/resolver_extra_test.go
internal/credentials/store.go
internal/credentials/store_test.go
internal/oidc/azure_factory_test.go
internal/oidc/azure_signer.go
internal/oidc/factory.go
internal/oidc/issuer_cache.go
internal/oidc/lambda_issuer.go
internal/oidc/lambda_issuer_test.go
internal/secrets/gcp_resolver.go
internal/secrets/gcp_resolver_grpc_test.go
internal/secrets/gcp_resolver_test.go
internal/server/app.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch sec/auth-mfa-crypto

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cristim · 2026-06-07T10:07:41Z

@coderabbitai review

coderabbitai · 2026-06-07T10:07:46Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…y config 03-M1: add `len(key) != 32` guard at the top of Encrypt and Decrypt so a 16- or 24-byte key returns a clear error rather than silently selecting AES-128/192. AES spec accepts shorter keys without error; the guard is needed because callers can supply a key directly, bypassing LoadKey. 03-M2: treat multiple key-source env vars simultaneously set as a hard startup error (ErrMultipleKeys) instead of a WARN + first-wins. Ambiguous key selection on a data-encryption key is a misconfiguration that must not proceed silently, especially given the rekey workflow. Regression tests updated/extended for both changes. Closes #1058 (partial)

…ter expression 03-M3: The Resolver interface documents GCP as using strings.HasPrefix filtering, but GCPResolver.ListSecrets was forwarding the caller's filter directly as a GCP expression-language Filter field, making the three implementations (Azure/env use HasPrefix; GCP used expression-language; AWS does API substring) inconsistent. Fix: remove the Filter from the ListSecretsRequest; apply strings.HasPrefix on the short name (path.Base) client-side so all implementations honour the documented contract. Updated regression tests confirm the API request carries no Filter expression. Closes #1058 (partial)

…error 03-M6: guard `new(big.Int).SetBytes(E).Int64()` in AzureKeyVaultSigner.resolveOnce with an IsInt64 + range check before the int() conversion. An oversized or non-positive exponent now returns an explicit error instead of silently corrupting the rsa.PublicKey and producing an unusable JWKS. 03-M7: NewSignerFromEnv Azure branch now emits a precise error when exactly one of CUDLY_SIGNING_KEY_VAULT_URL and CUDLY_SIGNING_KEY_NAME is set, naming the missing variable, instead of deferring to the constructor's generic error. Regression tests added for both cases. Closes #1058 (partial)

03-M5: RequestPasswordReset now silently no-ops when a reset token was issued within PasswordResetRateLimit (1 minute). An adversary who knows a victim's email could previously call the endpoint repeatedly to perpetually invalidate the victim's legitimate reset link. Detection is via the existing PasswordResetExpiry field (no schema change): if the token's age is less than PasswordResetRateLimit, the function returns nil without overwriting the token or sending a new email. The existing token remains valid. The response is indistinguishable from a successful request to preserve email-enumeration protection. Regression test: TestService_RequestPasswordReset_RateLimit. Closes #1058 (partial)

@ex

03-L1: redactEmail now masks both local prefix and domain hostname (was "us***@example.com", now "us***@ex***.com") to reduce PII exposure for low-entropy addresses in debug logs. 03-L2: apiKey[:8] slice in CreateAPIKey guarded with len check so a future change in key-generation length cannot panic. 03-L3: containsSequentialChars renamed to containsRepeatedChars and sequentialCharThreshold to repeatedCharThreshold; the function counts identical consecutive chars, not sequential runs -- the "sequential" naming was misleading. 03-L4: SetIssuerURL now validates that the URL is an absolute https:// URL and returns an error for non-https, relative, or unparseable URLs. Callers updated (lambda_issuer, handler_oidc, server/app). 03-L5: IsUnrestrictedAccess doc extended with a prominent fail-open WARNING; empty allowed-list = unrestricted is documented as intentional backward-compat but flagged so new callers cannot accidentally rely on it for "deny all" semantics. 03-L6: matchStringListConstraints doc extended to explain the empty-list semantics (empty either side = satisfied) and the authz rationale. Closes #1058 (partial)

…idate unmarshal fields 03-N4: generateToken now encodes 32 CSPRNG bytes directly as base64url instead of hashing them through SHA-256. Hashing CSPRNG output adds no entropy -- the raw bytes already carry 256 bits. The SHA-256 step could mislead readers into thinking the input was sensitive pre-image material. 03-N5: resolveAccessKeyProvider and ResolveAzureCredentials now validate that required payload fields (access_key_id, secret_access_key, client_secret) are non-empty after JSON unmarshal. Previously an empty field would flow through to the AWS SDK or Azure SDK and fail with a less descriptive error deep in the credential chain. Regression tests added for both N5 paths. Closes #1058 (partial)

cristim · 2026-06-07T11:30:40Z

FOLD-1044 additional commits (from 16-remaining-findings-plan.md)

Six new commits adding the Medium/Low/Nit findings from docs/code-review/03-auth-secrets.md to this PR, per the FOLD-1044 routing. Issue #1058 tracks them.

`ef6314c` - 03-M1 + 03-M2 (credentials/cipher.go)

M1: Encrypt/Decrypt now guard len(key)!=32 before aes.NewCipher; prevents silent AES-128/192 downgrade.
M2: Multiple CREDENTIAL_ENCRYPTION_KEY_* env vars simultaneously set is now ErrMultipleKeys (hard error).

`dfb66de` - 03-M3 (secrets/gcp_resolver.go)

GCPResolver.ListSecrets removed Filter from API request; applies strings.HasPrefix client-side to match the Resolver interface doc.

`93f352d` - 03-M6 + 03-M7 (oidc/azure_signer.go, oidc/factory.go)

M6: resolveOnce validates IsInt64() and range before int() conversion of Azure RSA exponent.
M7: NewSignerFromEnv Azure branch returns a precise "set both or neither" error when exactly one var is set.

`7434c11` - 03-M5 (auth/service_password.go)

RequestPasswordReset now checks PasswordResetRateLimit (1 min) before overwriting the existing token.

`47c3b76` - 03-L1 through 03-L6 (auth, oidc, server, api)

L1: redactEmail masks domain hostname.
L2: apiKey[:8] guarded with len check.
L3: containsSequentialChars renamed to containsRepeatedChars.
L4: SetIssuerURL returns error for non-https/relative URLs; callers updated.
L5: IsUnrestrictedAccess doc extended with fail-open WARNING.
L6: matchStringListConstraints doc extended with semantics rationale.

`04b9a99` - 03-N4 + 03-N5 (auth/service_helpers.go, credentials/resolver.go)

N4: generateToken encodes raw bytes directly; SHA-256 of CSPRNG output removed.
N5: resolveAccessKeyProvider and ResolveAzureCredentials validate required fields non-empty after unmarshal.

Note: 03-N1/N2 deferred to #1035 per the plan's "whichever lands first" instruction.

@coderabbitai review

coderabbitai · 2026-06-07T11:30:55Z

@cristim: Got it! Triggering a review of the six new commits covering the Medium/Low/Nit findings from 03-auth-secrets.md (03-M1 through 03-M7, 03-L1 through 03-L6, 03-N4, 03-N5), with 03-N1/N2 noted as deferred to #1035.

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T07:04:28.213Z
Learning: In LeanerCloud/CUDly, PR `#907` (`feat(auth): group-membership-only authorization, remove roles, require >=1 group`) landed on `feat/multicloud-web-frontend` and removed `Session.Role` and `User.Role` entirely. Any `Principal` struct must use `GroupIDs []string` (or similar group-based field) instead of `Role string`. The `*auth.User` type no longer exposes `GetRole()`, `GetID()`, or `GetEmail()` accessors via interface assertion — using those in a type assertion causes a silent always-false fail-closed regression at runtime. The group-membership model also requires >=1 group for a valid user session; zero-groups users must be rejected at the authentication layer. Follow-up design issue is `#1010`.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…ation - Replace TestAzureSigner_ExponentRange (RSA exponent tests) with TestAzureSigner_ECKeyCompleteness: the azure_factory_test.go added on main (#1044) tested RSA exponent validation removed by this PR; new test covers EC key nil-field rejection to keep coverage parity. - Apply fieldalignment ordering (govet) to aws_signer.go, azure_signer.go, gcp_signer.go, and the updated azure_factory_test.go struct literals. - Suppress staticcheck SA1019 on elliptic.Marshal in ComputeKeyID with an inline nolint; elliptic.Marshal is the only stdlib path from *ecdsa.PublicKey to the uncompressed point without converting through crypto/ecdh. - Fix staticcheck QF1008 (remove unnecessary .PublicKey embed selector) in aws_signer_test.go and backend_jws_test.go. - Fix shadow declarations in signer_test.go (json.Unmarshal err vars). - Replace deprecated ecdsa.Sign with ecdsa.SignASN1 + derToRawECDSASignature in the fakeAzureKVClient test stub (backend_jws_test.go). - Fix gofmt import ordering in backend_jws_test.go (cloud.google.com before github.com).

* sec(oidc): switch JWT signing from PKCS1v15 to ES256 (closes #422) Replace RSA-PKCS1v15 (RS256) with ECDSA P-256 (ES256) across the entire oidc package: - LocalSigner: ecdsa.GenerateKey(P256) + ecdsa.SignASN1 instead of rsa.GenerateKey + rsa.SignPKCS1v15 - Signer interface: PublicKey() returns crypto.PublicKey (was *rsa.PublicKey) - Algorithm constant: "ES256" (was "RS256") - ComputeKeyID: accepts crypto.PublicKey, hashes uncompressed EC point - JWK: EC fields (kty=EC, crv=P-256, x, y); RSA fields (n, e) removed - AWSKMSSigner: SigningAlgorithmSpecEcdsaSha256 + *ecdsa.PublicKey assertion - AzureKeyVaultSigner: SignatureAlgorithmES256 + EC key (X/Y) extraction - GCPKMSSigner: *ecdsa.PublicKey assertion (digest call is key-type-agnostic) - All tests: positive assertions on ES256 algorithm and EC JWK fields * fix(oidc): emit RFC7518 raw R||S ES256 JWS signatures across all signer backends (refs #422) Mint base64url-encoded the signer's raw output directly into the JWS signature segment, but the Local/AWS/GCP signers return DER/ASN.1 ECDSA signatures (ecdsa.SignASN1, AWS ECDSA_SHA_256, GCP EC sign all yield ASN.1 DER). RFC 7518 section 3.4 requires an ES256 JWS signature to be the raw fixed-length R || S concatenation (32 bytes each = 64 bytes for P-256), NOT DER. As a result AWS/GCP/Local minted tokens that real OIDC consumers (the stated Azure AD target) reject. Azure Key Vault already returns raw R||S, so only Azure was correct, leaving the four backends mutually inconsistent. The Azure Sign comment also wrongly claimed Key Vault returns DER. Contract: Signer.Sign now returns the RFC 7518 raw R||S form. DER backends (Local/AWS/GCP) convert via a single shared derToRawECDSASignature helper; Azure passes through unchanged (no double-conversion). Mint encodes the result directly and defensively rejects any signature that is not 64 bytes. - signer.go: add derToRawECDSASignature; LocalSigner.Sign converts; Mint enforces the 64-byte contract; document the contract on the Signer interface. - aws_signer.go, gcp_signer.go: convert KMS DER output to raw R||S. - azure_signer.go: fix the wrong "DER-encoded" comment; keep passthrough. - tests: add assertRawES256JWS asserting the JWS sig is exactly 64 bytes, splitting R||S for ecdsa.Verify, and parsing with a real golang-jwt ES256 parser. Cover Local, AWS (DER fake), GCP (DER fake) and Azure (raw fake). The DER-path tests fail on the pre-fix code (71/69-byte signatures) and pass after; the Azure raw path stays correct. - go.mod: promote golang-jwt/jwt/v5 to a direct dependency (test use). * fix(oidc): rebase on main, fix test/lint regressions after ES256 migration - Replace TestAzureSigner_ExponentRange (RSA exponent tests) with TestAzureSigner_ECKeyCompleteness: the azure_factory_test.go added on main (#1044) tested RSA exponent validation removed by this PR; new test covers EC key nil-field rejection to keep coverage parity. - Apply fieldalignment ordering (govet) to aws_signer.go, azure_signer.go, gcp_signer.go, and the updated azure_factory_test.go struct literals. - Suppress staticcheck SA1019 on elliptic.Marshal in ComputeKeyID with an inline nolint; elliptic.Marshal is the only stdlib path from *ecdsa.PublicKey to the uncompressed point without converting through crypto/ecdh. - Fix staticcheck QF1008 (remove unnecessary .PublicKey embed selector) in aws_signer_test.go and backend_jws_test.go. - Fix shadow declarations in signer_test.go (json.Unmarshal err vars). - Replace deprecated ecdsa.Sign with ecdsa.SignASN1 + derToRawECDSASignature in the fakeAzureKVClient test stub (backend_jws_test.go). - Fix gofmt import ordering in backend_jws_test.go (cloud.google.com before github.com). * fix(oidc): replace deprecated elliptic.Marshal with ECDH().Bytes() in ComputeKeyID Use (*ecdsa.PublicKey).ECDH() and (*ecdh.PublicKey).Bytes() to obtain the uncompressed EC point, replacing the SA1019-deprecated elliptic.Marshal. The wire format is identical (0x04 || X || Y), so kid stability is preserved. Removes the staticcheck nolint directive. * fix(oidc): update stubOIDCSigner in credentials tests to crypto.PublicKey The oidc.Signer interface PublicKey method was changed from *rsa.PublicKey to crypto.PublicKey as part of the ES256 migration. Update the test stub in internal/credentials to match, fixing the go vet type-assertion failure. * fix(oidc): avoid deprecated ecdsa.PublicKey.X/Y in ES256 tests Rebasing onto main (Go 1.26.5) surfaces staticcheck SA1019: the raw ecdsa.PublicKey.X/Y fields are deprecated since Go 1.26. Replace the two uses introduced by the ES256 test code: - aws_signer_test.go: compare public keys via (*ecdsa.PublicKey).Equal instead of X.Cmp/Y.Cmp. - azure_factory_test.go: derive the fixed-width JWK coordinates from the uncompressed SEC 1 point via crypto/ecdh (ECDH().Bytes()), matching ComputeKeyID and PublicJWK. No behaviour change; the Azure fake now emits left-padded coordinates, which big.Int.SetBytes reconstructs identically. * fix(oidc): replace deprecated ecdsa.PublicKey X/Y fields with ParseUncompressedPublicKey Staticcheck SA1019 flagged direct struct-literal initialization of ecdsa.PublicKey.X and ecdsa.PublicKey.Y (deprecated since Go 1.23). azure_signer.go: build the 65-byte SEC 1 uncompressed point from the JWK X/Y bytes (right-aligned to 32 bytes each, per JWK spec), then call ecdsa.ParseUncompressedPublicKey(elliptic.P256(), ...) which validates point membership and produces a PublicKey without touching the deprecated fields. Drop the now-unused math/big import. Add a guard for coordinate lengths > 32 bytes. backend_jws_test.go: replace f.key.X.Bytes()/f.key.Y.Bytes() with key.PublicKey.ECDH().Bytes() (uncompressed point), then slice X from [1:33] and Y from [33:65]. Add "fmt" import for the new error path.

cristim added 5 commits June 7, 2026 03:03

cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/high Significant harm urgency/now Drop other things impact/few Limited audience effort/m Days type/security Security finding labels Jun 7, 2026

This was referenced Jun 7, 2026

refactor(arch): provider-module fragmentation forces copy-paste (fan-out, env reader, savings formula); + pkg/errors orphan, logging split #1035

Closed

sec(auth,credentials,oidc,secrets): remaining FOLD-1044 hardening (M1-M7, L1-L6, N4-N5) #1058

Closed

cristim added 6 commits June 7, 2026 04:16

cristim merged commit 8f20d76 into feat/multicloud-web-frontend Jun 7, 2026
4 checks passed

cristim deleted the sec/auth-mfa-crypto branch July 27, 2026 11:10

Navigation Menu

Repository navigation

sec(auth): fail-closed MFA, CSRF HMAC derivation, bounded last-used, errors.Is (#1018 #1027 #1028 #1029) - #1044

sec(auth): fail-closed MFA, CSRF HMAC derivation, bounded last-used, errors.Is (#1018 #1027 #1028 #1029)#1044
cristim merged 11 commits into
feat/multicloud-web-frontendfrom
sec/auth-mfa-crypto

cristim commented Jun 7, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Review limit reached

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cristim commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

#1018 - MFA primitives fail closed on empty input

#1027 - API key last-used update bounded via singleflight

#1028 - credentials/LoadRaw uses errors.Is for pgx.ErrNoRows

#1029 - CSRF token derived as HMAC, not stored cleartext

Verification per issue

Scope expanded

03-M1 - Explicit AES-256 key-length guard

03-M2 - Multi-key env var config is now a hard error

03-M3 - GCP ListSecrets uses client-side HasPrefix

03-M5 - Password reset rate-limit per email

03-M6 - Azure exponent overflow guard

03-M7 - Precise error for half-configured Azure OIDC

03-L1 through 03-L6 - Low-severity hardening

03-N4 and 03-N5 - Nit improvements

Uh oh!

coderabbitai Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

cristim commented Jun 7, 2026

Uh oh!

coderabbitai Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 7, 2026

FOLD-1044 additional commits (from 16-remaining-findings-plan.md)

ef6314c - 03-M1 + 03-M2 (credentials/cipher.go)

dfb66de - 03-M3 (secrets/gcp_resolver.go)

93f352d - 03-M6 + 03-M7 (oidc/azure_signer.go, oidc/factory.go)

7434c11 - 03-M5 (auth/service_password.go)

47c3b76 - 03-L1 through 03-L6 (auth, oidc, server, api)

04b9a99 - 03-N4 + 03-N5 (auth/service_helpers.go, credentials/resolver.go)

Uh oh!

coderabbitai Bot commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cristim commented Jun 7, 2026 •

edited

Loading

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading

`ef6314c` - 03-M1 + 03-M2 (credentials/cipher.go)

`dfb66de` - 03-M3 (secrets/gcp_resolver.go)

`93f352d` - 03-M6 + 03-M7 (oidc/azure_signer.go, oidc/factory.go)

`7434c11` - 03-M5 (auth/service_password.go)

`47c3b76` - 03-L1 through 03-L6 (auth, oidc, server, api)

`04b9a99` - 03-N4 + 03-N5 (auth/service_helpers.go, credentials/resolver.go)

coderabbitai Bot commented Jun 7, 2026 •

edited

Loading