feat: Add PKCE and JWKS support for upstream OIDC providers#5993
feat: Add PKCE and JWKS support for upstream OIDC providers#5993assimilat wants to merge 1 commit intonetbirdio:mainfrom
Conversation
|
|
📝 WalkthroughWalkthroughThis PR adds Proof Key for Code Exchange (PKCE) and JWKS URL configuration support to identity provider settings. The changes propagate new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/store/sql_store.go (1)
2716-2744:⚠️ Potential issue | 🟠 MajorRead-modify-write without a transaction can clobber concurrent updates and risks double-encryption.
A few concerns with the new flow:
TOCTOU / lost updates.
s.db.Take(&user, ...)followed bys.db.Save(userCopy).Erroris not wrapped in a transaction (and isn't using a row lock).Savewrites every column, so any field updated between theTakehere and theSave(Role, Blocked, AutoGroups, Email/Name change from another flow, etc.) is silently overwritten with the version this function loaded.last_loginupdates happen on a hot path and frequently race with profile updates.Double-encryption interaction with the
util/crypt/crypt.gochange in this same PR. With the new Decrypt that returns the original ciphertext ongcm.Openfailure, any user record whose Email/Name was encrypted under a different key (or got corrupted) will:
- "Decrypt" to ciphertext-as-plaintext (no error).
- Be re-encrypted by
EncryptSensitiveData→ ciphertext-of-ciphertext.- Be persisted via
Save. Each subsequent login deepens the nesting.
This is a silent data-corruption path triggered by a routine login-time write.Unnecessary work for a single-column update. Decrypting and re-encrypting Email/Name on every login just to set
last_loginis heavy and only exists to compensate for theSaveoverwriting all columns. A targeted update avoids both the race-clobber and the encryption round-trip.Minor:
userCopy.Email = user.Email/userCopy.Name = user.Nameare redundant —(*User).Copy()(seemanagement/server/types/user.go~L200-225) already preserves both fields. Not a bug, just dead code.🛡️ Suggested fix: targeted column update
- if lastLogin.IsZero() { - return nil - } - - if err := user.DecryptSensitiveData(s.fieldEncrypt); err != nil { - return fmt.Errorf("decrypt user: %w", err) - } - - user.LastLogin = &lastLogin - - userCopy := user.Copy() - userCopy.Email = user.Email - userCopy.Name = user.Name - if err := userCopy.EncryptSensitiveData(s.fieldEncrypt); err != nil { - return fmt.Errorf("encrypt user: %w", err) - } - - return s.db.Save(userCopy).Error + if lastLogin.IsZero() { + return nil + } + + result = s.db.Model(&types.User{}). + Where(accountAndIDQueryCondition, accountID, userID). + Update("last_login", lastLogin) + if result.Error != nil { + return result.Error + } + if result.RowsAffected == 0 { + return status.NewUserNotFoundError(userID) + } + return nilThis avoids the race, the all-column overwrite, and the encrypt/decrypt round-trip entirely. The initial
Takecan also be removed since the targetedUpdatealready validates existence viaRowsAffected.Side note: like the
util/crypt/crypt.gochange, this doesn't appear to relate to the PR's stated scope ("PKCE and JWKS support for upstream OIDC providers"). Worth double-checking these changes belong in this PR vs. a separate one focused on encryption/persistence behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store.go` around lines 2716 - 2744, SaveUserLastLogin currently reads the whole user row, decrypts/re-encrypts sensitive fields and calls Save which causes TOCTOU lost-updates and double-encryption; replace this with a targeted single-column update: in SaveUserLastLogin, keep the early lastLogin.IsZero() return, then perform a DB update like s.db.Model(&types.User{}).Where(accountAndIDQueryCondition, accountID, userID).Update("last_login", lastLogin) (or UpdateColumn to avoid hooks), check result.Error and if result.RowsAffected == 0 return status.NewUserNotFoundError(userID), and remove the s.db.Take, DecryptSensitiveData, Copy/EncryptSensitiveData and Save logic to avoid decrypt/encrypt and overwrite of other columns (references: function SaveUserLastLogin and the query condition symbol accountAndIDQueryCondition).
🧹 Nitpick comments (5)
management/server/http/handlers/idp/idp_handler.go (1)
180-181: Always emitting pointers forPkce/JwksUrlproduces"jwks_url": ""for unset values.
toAPIResponseunconditionally wires&idp.PKCEand&idp.JWKSURL, so the response always contains these fields even when no JWKS URL was configured. If the OpenAPI schema marks them optional, prefer omitting the empty string so clients can distinguish "not configured" from "configured as empty":Proposed adjustment
- Pkce: &idp.PKCE, - JwksUrl: &idp.JWKSURL, + Pkce: &idp.PKCE, + } + if idp.JWKSURL != "" { + resp.JwksUrl = &idp.JWKSURL }(Adjust placement to fit the existing struct literal.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/idp/idp_handler.go` around lines 180 - 181, toAPIResponse currently always sets Pkce and JwksUrl to pointers of idp.PKCE and idp.JWKSURL which emits empty strings for unset values; change the struct construction in toAPIResponse to only include Pkce: &idp.PKCE when idp.PKCE is non-empty and only include JwksUrl: &idp.JWKSURL when idp.JWKSURL is non-empty (omit the fields entirely otherwise) so clients receive nil/missing fields for "not configured" instead of "".management/server/http/handlers/idp/idp_handler_test.go (1)
427-449: Consider also testing the nil-pointer (omitted) case forPkce/JwksUrl.The new test only exercises the path where both pointers are set. Adding a sub-case where
req.Pkce/req.JwksUrlarenilwould lock in the conditional mapping behavior infromAPIRequestand guard against regressions (especially given the PKCE-on-update preservation concern raised onidp_handler.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/idp/idp_handler_test.go` around lines 427 - 449, Add a subtest in TestFromAPIRequest that covers the nil-pointer case for api.IdentityProviderRequest.Pkce and .JwksUrl: create req with those fields omitted (nil), call fromAPIRequest(req), and assert that the resulting idp.PKCE is false (or the zero value used by fromAPIRequest) and idp.JWKSURL is the empty string (or the zero value used by fromAPIRequest); this verifies fromAPIRequest correctly handles nil pointers and guards against regressions.management/server/types/identity_provider.go (1)
116-121: Use a sentinel error forJWKSURLvalidation, consistent with other fields.Other validation failures in this file use predefined sentinel errors (e.g.,
ErrIdentityProviderIssuerInvalid), which makes them easy to match witherrors.Isin callers/tests. The new JWKS validation uses a one-offfmt.Errorf, breaking that pattern.Suggested change
var ( ... ErrIdentityProviderClientIDRequired = errors.New("identity provider client ID is required") + ErrIdentityProviderJWKSURLInvalid = errors.New("identity provider JWKS URL must be a valid URL") )- if idp.JWKSURL != "" { - parsedURL, err := url.Parse(idp.JWKSURL) - if err != nil || parsedURL.Scheme == "" || parsedURL.Host == "" { - return fmt.Errorf("invalid JWKS URL: %s", idp.JWKSURL) - } - } + if idp.JWKSURL != "" { + parsedURL, err := url.Parse(idp.JWKSURL) + if err != nil || parsedURL.Scheme == "" || parsedURL.Host == "" { + return ErrIdentityProviderJWKSURLInvalid + } + }If
fmtis no longer needed after this change, drop the import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/identity_provider.go` around lines 116 - 121, Replace the ad-hoc fmt.Errorf used for JWKS URL validation in the idp.JWKSURL block with the package's predefined sentinel error (the same style as ErrIdentityProviderIssuerInvalid) so callers can use errors.Is to detect this validation failure; update the return to return ErrIdentityProviderJWKSURLInvalid (or the existing sentinel that matches other identity-provider validations) and remove the fmt import if it becomes unused.idp/dex/connector.go (1)
224-229: Consider conditionally settingjwksURLfor consistency with thecfg.PKCEpattern.While Dex treats an explicit empty
jwksURLthe same as omitting it (both result in using the default discovered JWKS URI), adding a conditional check to only set the key whencfg.JWKSURLis populated aligns with the existing code pattern and improves clarity:Suggested change
oidcConfig := map[string]interface{}{ "issuer": cfg.Issuer, "clientID": cfg.ClientID, "clientSecret": cfg.ClientSecret, "redirectURI": redirectURI, "scopes": []string{"openid", "profile", "email"}, "insecureEnableGroups": true, //some providers don't return email verified, so we need to skip it if not present (e.g., Entra, Okta, Duo) "insecureSkipEmailVerified": true, - "jwksURL": cfg.JWKSURL, } - + if cfg.JWKSURL != "" { + oidcConfig["jwksURL"] = cfg.JWKSURL + } if cfg.PKCE { oidcConfig["pkceChallenge"] = "S256" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@idp/dex/connector.go` around lines 224 - 229, The oidcConfig map currently always sets "jwksURL" to cfg.JWKSURL even when empty; follow the existing cfg.PKCE pattern by only adding oidcConfig["jwksURL"] = cfg.JWKSURL when cfg.JWKSURL is non-empty—locate the oidcConfig construction near the cfg.PKCE check and add a conditional that checks cfg.JWKSURL before setting the "jwksURL" key to keep behavior consistent and clearer.shared/management/http/api/openapi.yml (1)
2946-2949: Constrainjwks_urlas a URI in the OpenAPI contract.
jwks_urlis documented as a URL, but the schema only declarestype: string. Addingformat: uriin both request and response models will make validation and generated clients stricter and more predictable.📌 Suggested OpenAPI diff
jwks_url: description: URL to the JSON Web Key Set of the identity provider type: string + format: uri example: https://accounts.google.com/o/oauth2/v1/certs @@ jwks_url: description: URL to the JSON Web Key Set of the identity provider type: string + format: uri example: https://accounts.google.com/o/oauth2/v1/certsAlso applies to: 2980-2983
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/management/http/api/openapi.yml` around lines 2946 - 2949, The jwks_url property is declared as type: string but should be constrained as a URI; update the OpenAPI schemas that define the jwks_url property (both the request and response models that include the jwks_url field) to add format: uri alongside type: string so generated clients and validators treat it as a URI; ensure you update every schema instance of jwks_url in this file (including the other occurrence mentioned) so both request and response models consistently use type: string and format: uri.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@idp/dex/connector.go`:
- Around line 136-143: The merge currently loses "field-omitted vs false"
semantics because dex.ConnectorConfig.PKCE is a plain bool and
mergeConnectorConfig treats empty JWKSURL specially, so updates can silently
disable PKCE or prevent clearing JWKSURL; update fromAPIRequest
(management/server/http/handlers/idp/idp_handler.go) and the types by changing
types.IdentityProvider.PKCE and dex.ConnectorConfig.PKCE to *bool (and
optionally JWKSURL to *string) and adjust fromAPIRequest/mergeConnectorConfig to
only overwrite cfg.PKCE or cfg.JWKSURL when the incoming pointer is non-nil (or
alternatively implement merging in the Update* path to read the existing
IdentityProvider, overlay only non-nil request fields, and pass the merged
struct down) so omitted fields are preserved and explicit false/empty values are
honored.
In `@management/server/http/handlers/idp/idp_handler.go`:
- Around line 198-203: The handler currently overwrites idp.PKCE with the zero
value when req.Pkce is nil; to avoid flipping existing PKCE you should load the
existing IdP first and only set idp.PKCE when req.Pkce != nil (leave existing
value intact otherwise) before calling
identityProviderToConnectorConfig/UpdateConnector (or alternatively change
types.IdentityProvider and dex.ConnectorConfig to carry *bool and pass the
pointer through to mergeConnectorConfig). Concretely, in fromAPIRequest/this
handler: fetch the current IdentityProvider, initialize idp from that, then
apply fields from req only when their pointers (req.Pkce, req.JwksUrl, etc.) are
non-nil so mergeConnectorConfig/UpdateConnector receives preserved PKCE unless
explicitly changed.
In `@util/crypt/crypt.go`:
- Around line 85-90: The GCM auth-failure branch currently returns the base64
ciphertext as plaintext after gcm.Open fails; change this so the decryption
function (the function that calls gcm.Open and uses variables nonce,
ciphertextBytes and plaintext) returns an explicit error instead of returning
ciphertext, e.g., return "", fmt.Errorf("authenticated decryption failed: %w",
err) (or just return "", err) so callers see the failure (this will satisfy
TestDecrypt_WrongKey); keep the earlier non-base64 and nonce-size migration
fallbacks as-is, and if you need a plaintext-migration mode make it an explicit
option/flag rather than silently accepting gcm.Open failures.
---
Outside diff comments:
In `@management/server/store/sql_store.go`:
- Around line 2716-2744: SaveUserLastLogin currently reads the whole user row,
decrypts/re-encrypts sensitive fields and calls Save which causes TOCTOU
lost-updates and double-encryption; replace this with a targeted single-column
update: in SaveUserLastLogin, keep the early lastLogin.IsZero() return, then
perform a DB update like
s.db.Model(&types.User{}).Where(accountAndIDQueryCondition, accountID,
userID).Update("last_login", lastLogin) (or UpdateColumn to avoid hooks), check
result.Error and if result.RowsAffected == 0 return
status.NewUserNotFoundError(userID), and remove the s.db.Take,
DecryptSensitiveData, Copy/EncryptSensitiveData and Save logic to avoid
decrypt/encrypt and overwrite of other columns (references: function
SaveUserLastLogin and the query condition symbol accountAndIDQueryCondition).
---
Nitpick comments:
In `@idp/dex/connector.go`:
- Around line 224-229: The oidcConfig map currently always sets "jwksURL" to
cfg.JWKSURL even when empty; follow the existing cfg.PKCE pattern by only adding
oidcConfig["jwksURL"] = cfg.JWKSURL when cfg.JWKSURL is non-empty—locate the
oidcConfig construction near the cfg.PKCE check and add a conditional that
checks cfg.JWKSURL before setting the "jwksURL" key to keep behavior consistent
and clearer.
In `@management/server/http/handlers/idp/idp_handler_test.go`:
- Around line 427-449: Add a subtest in TestFromAPIRequest that covers the
nil-pointer case for api.IdentityProviderRequest.Pkce and .JwksUrl: create req
with those fields omitted (nil), call fromAPIRequest(req), and assert that the
resulting idp.PKCE is false (or the zero value used by fromAPIRequest) and
idp.JWKSURL is the empty string (or the zero value used by fromAPIRequest); this
verifies fromAPIRequest correctly handles nil pointers and guards against
regressions.
In `@management/server/http/handlers/idp/idp_handler.go`:
- Around line 180-181: toAPIResponse currently always sets Pkce and JwksUrl to
pointers of idp.PKCE and idp.JWKSURL which emits empty strings for unset values;
change the struct construction in toAPIResponse to only include Pkce: &idp.PKCE
when idp.PKCE is non-empty and only include JwksUrl: &idp.JWKSURL when
idp.JWKSURL is non-empty (omit the fields entirely otherwise) so clients receive
nil/missing fields for "not configured" instead of "".
In `@management/server/types/identity_provider.go`:
- Around line 116-121: Replace the ad-hoc fmt.Errorf used for JWKS URL
validation in the idp.JWKSURL block with the package's predefined sentinel error
(the same style as ErrIdentityProviderIssuerInvalid) so callers can use
errors.Is to detect this validation failure; update the return to return
ErrIdentityProviderJWKSURLInvalid (or the existing sentinel that matches other
identity-provider validations) and remove the fmt import if it becomes unused.
In `@shared/management/http/api/openapi.yml`:
- Around line 2946-2949: The jwks_url property is declared as type: string but
should be constrained as a URI; update the OpenAPI schemas that define the
jwks_url property (both the request and response models that include the
jwks_url field) to add format: uri alongside type: string so generated clients
and validators treat it as a URI; ensure you update every schema instance of
jwks_url in this file (including the other occurrence mentioned) so both request
and response models consistently use type: string and format: uri.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c39018c-4fc8-4f79-accd-73f64830084f
📒 Files selected for processing (12)
idp/dex/connector.gomanagement/server/http/handlers/idp/idp_handler.gomanagement/server/http/handlers/idp/idp_handler_test.gomanagement/server/identity_provider.gomanagement/server/store/sql_store.gomanagement/server/store/sql_store.go.origmanagement/server/types/identity_provider.goshared/management/http/api/openapi.ymlshared/management/http/api/openapi.yml.origshared/management/http/api/types.gen.goshared/management/http/api/types.gen.go.origutil/crypt/crypt.go
| if cfg.JWKSURL == "" { | ||
| cfg.JWKSURL = oldCfg.JWKSURL | ||
| } | ||
| // Note: for boolean PKCE, we don't have an easy way to detect if it was missing in the request | ||
| // if we don't use pointers. Given the current pattern, we assume if it's false it might be intentional | ||
| // or it might be missing. However, the API request uses *bool, so we could have used that if we wanted. | ||
| // For now, we'll just let it be updated as is. | ||
| } |
There was a problem hiding this comment.
Root cause of the PKCE/JWKS silent-reset risk on partial updates.
The acknowledged limitation here is the underlying cause of a real data-loss bug: because cfg.PKCE is a plain bool, this merge cannot distinguish "client omitted the field" from "client set it to false". Any update from a client that doesn't echo pkce (curl/scripted updates, future UIs that only patch some fields, etc.) will flip a previously-enabled PKCE configuration off without the user's knowledge.
The cleanest fix is to plumb *bool (and *string for JWKSURL if you want the same semantics) through the call chain — api.IdentityProviderRequest already uses *bool/*string, so the information is currently being thrown away in fromAPIRequest (management/server/http/handlers/idp/idp_handler.go). Either:
- Make
types.IdentityProvider.PKCEanddex.ConnectorConfig.PKCE*bool, and only overwrite when non-nil; or - Have the manager
Update*path read the existingIdentityProvider, copy over only fields explicitly set in the request, and pass the merged struct down (so this merge becomes unnecessary for these fields).
Also note JWKSURL has the inverse asymmetry: mergeConnectorConfig preserves it on empty input, so a user can never clear a previously-set JWKS URL via update once the field is set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@idp/dex/connector.go` around lines 136 - 143, The merge currently loses
"field-omitted vs false" semantics because dex.ConnectorConfig.PKCE is a plain
bool and mergeConnectorConfig treats empty JWKSURL specially, so updates can
silently disable PKCE or prevent clearing JWKSURL; update fromAPIRequest
(management/server/http/handlers/idp/idp_handler.go) and the types by changing
types.IdentityProvider.PKCE and dex.ConnectorConfig.PKCE to *bool (and
optionally JWKSURL to *string) and adjust fromAPIRequest/mergeConnectorConfig to
only overwrite cfg.PKCE or cfg.JWKSURL when the incoming pointer is non-nil (or
alternatively implement merging in the Update* path to read the existing
IdentityProvider, overlay only non-nil request fields, and pass the merged
struct down) so omitted fields are preserved and explicit false/empty values are
honored.
| if req.Pkce != nil { | ||
| idp.PKCE = *req.Pkce | ||
| } | ||
| if req.JwksUrl != nil { | ||
| idp.JWKSURL = *req.JwksUrl | ||
| } |
There was a problem hiding this comment.
PKCE cannot be distinguished from "not provided" on update.
Because req.Pkce is *bool, fromAPIRequest correctly only sets idp.PKCE when the pointer is non-nil. However, when the request omits pkce, idp.PKCE is left at its zero value (false) and is later passed through identityProviderToConnectorConfig/UpdateConnector. In idp/dex/connector.go::mergeConnectorConfig, there is no preservation logic for PKCE (the comment at lines 139-142 acknowledges this), so an existing PKCE=true configuration will be silently flipped to false on any partial update that omits the field.
Consider propagating the pointer (or a tri-state) all the way down to the merge step, e.g. by carrying *bool on types.IdentityProvider/dex.ConnectorConfig, or by having the handler load the existing IdP and apply only fields that were provided in the request before delegating to the manager.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@management/server/http/handlers/idp/idp_handler.go` around lines 198 - 203,
The handler currently overwrites idp.PKCE with the zero value when req.Pkce is
nil; to avoid flipping existing PKCE you should load the existing IdP first and
only set idp.PKCE when req.Pkce != nil (leave existing value intact otherwise)
before calling identityProviderToConnectorConfig/UpdateConnector (or
alternatively change types.IdentityProvider and dex.ConnectorConfig to carry
*bool and pass the pointer through to mergeConnectorConfig). Concretely, in
fromAPIRequest/this handler: fetch the current IdentityProvider, initialize idp
from that, then apply fields from req only when their pointers (req.Pkce,
req.JwksUrl, etc.) are non-nil so mergeConnectorConfig/UpdateConnector receives
preserved PKCE unless explicitly changed.
| plaintext, err := gcm.Open(nil, nonce, ciphertextBytes, nil) | ||
| if err != nil { | ||
| return "", fmt.Errorf("decrypt: %w", err) | ||
| // Decryption failed, likely plain text or wrong key. | ||
| // We return the original string to avoid breaking systems when encryption is newly enabled. | ||
| return ciphertext, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every call site of FieldEncrypt.Decrypt and inspect whether it relies on the error
# return for control flow / fallback behavior that the new silent-fallback breaks.
rg -nP -C3 '\bDecrypt\s*\(' --type=go -g '!**/vendor/**'Repository: netbirdio/netbird
Length of output: 16091
🏁 Script executed:
cat -n util/crypt/crypt.go | sed -n '62,95p'Repository: netbirdio/netbird
Length of output: 1242
🏁 Script executed:
# Check the wrong-key decryption test
cat -n util/crypt/crypt_test.go | sed -n '130,140p'Repository: netbirdio/netbird
Length of output: 358
🏁 Script executed:
# Find SaveUserLastLogin and understand the re-encryption claim
rg -n 'SaveUserLastLogin' --type=go -A 15Repository: netbirdio/netbird
Length of output: 9080
🏁 Script executed:
# Check the complete SaveUserLastLogin implementation
cat -n management/server/store/sql_store.go | sed -n '2715,2745p'Repository: netbirdio/netbird
Length of output: 1157
🏁 Script executed:
# Find where EncryptSensitiveData is called after DecryptSensitiveData
rg -n 'EncryptSensitiveData' --type=go -B 2 -A 2 | head -60Repository: netbirdio/netbird
Length of output: 4127
🏁 Script executed:
# Check if the wrong-key test actually passes or fails
cd management/server && grep -r "TestDecryptWrongKey\|TestFieldEncryptWrongKey" . || echo "Test not found by name"Repository: netbirdio/netbird
Length of output: 84
🏁 Script executed:
# Run the crypt_test to see if it actually passes
cd util/crypt && go test -v 2>&1 | grep -A 5 "wrong\|Wrong" || echo "Test output not available"Repository: netbirdio/netbird
Length of output: 809
Silent fallback on gcm.Open failure breaks decryption contract and corrupts data.
The implementation currently returns the ciphertext bytes as plaintext when authenticated decryption fails (line 89), instead of surfacing the error. This directly contradicts the test expectations: TestDecrypt_WrongKey fails with the current code—it expects an error and empty payload, but receives the base64 ciphertext as plaintext instead.
The first two fallbacks (non-base64 and nonce-size checks) are reasonable migration aids. The GCM auth failure branch is different: a successful base64 decode followed by gcm.Open failure almost always means real ciphertext encrypted under a different key, tampered, or corrupted—not plaintext.
Downstream consequences:
-
All callers that check
err != nilare now broken. They wrap errors withfmt.Errorfinuser.go(~L292),user_invite.go(~L170), return errors inreverseproxy/service.go(~L1212), and aggregate decryption errors inactivity/store/sql_store.go(~L92-124). None of these error paths will trigger on wrong-key decryption, silently treating ciphertext as plaintext. -
Data corruption via re-encryption: In
SaveUserLastLogin(sql_store.go ~L2730-2741), the code decrypts, mutates LastLogin, and re-encrypts. If Decrypt returns base64 ciphertext as plaintext due to a wrong key, EncryptSensitiveData will encrypt that ciphertext string itself, producing progressively corrupted (double/triple encrypted) records on every login attempt. -
reverseproxy/service.go(~L1210): wrong-key decryption stores raw ciphertext bytes back intoSessionPrivateKey, silently breaking sessions.
Fix: Surface the GCM auth failure. If a true migration path from plaintext is required, gate it behind an explicit option and apply only to the base64/length pre-checks, not GCM auth failures.
Proposed approach
nonce, ciphertextBytes := data[:nonceSize], data[nonceSize:]
plaintext, err := gcm.Open(nil, nonce, ciphertextBytes, nil)
if err != nil {
- // Decryption failed, likely plain text or wrong key.
- // We return the original string to avoid breaking systems when encryption is newly enabled.
- return ciphertext, nil
+ // Authenticated decryption failed on data that looked like valid ciphertext.
+ // This is not plaintext; it indicates wrong key, tampering, or corruption.
+ return "", fmt.Errorf("gcm open: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| plaintext, err := gcm.Open(nil, nonce, ciphertextBytes, nil) | |
| if err != nil { | |
| return "", fmt.Errorf("decrypt: %w", err) | |
| // Decryption failed, likely plain text or wrong key. | |
| // We return the original string to avoid breaking systems when encryption is newly enabled. | |
| return ciphertext, nil | |
| } | |
| plaintext, err := gcm.Open(nil, nonce, ciphertextBytes, nil) | |
| if err != nil { | |
| // Authenticated decryption failed on data that looked like valid ciphertext. | |
| // This is not plaintext; it indicates wrong key, tampering, or corruption. | |
| return "", fmt.Errorf("gcm open: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@util/crypt/crypt.go` around lines 85 - 90, The GCM auth-failure branch
currently returns the base64 ciphertext as plaintext after gcm.Open fails;
change this so the decryption function (the function that calls gcm.Open and
uses variables nonce, ciphertextBytes and plaintext) returns an explicit error
instead of returning ciphertext, e.g., return "", fmt.Errorf("authenticated
decryption failed: %w", err) (or just return "", err) so callers see the failure
(this will satisfy TestDecrypt_WrongKey); keep the earlier non-base64 and
nonce-size migration fallbacks as-is, and if you need a plaintext-migration mode
make it an explicit option/flag rather than silently accepting gcm.Open
failures.
2 participants
This PR adds support for configuring PKCE and JWKS URLs for upstream OIDC identity providers.
Summary by CodeRabbit