feat(auth): support private_key_jwt registration and authentication#1379
Open
albertnusouo wants to merge 4 commits into
Open
feat(auth): support private_key_jwt registration and authentication#1379albertnusouo wants to merge 4 commits into
albertnusouo wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds secretless private_key_jwt end-to-end: keysigner contracts and registry, JWT attestation/assertion builders, app-registration auth-method negotiation, ClientAuth device-flow wiring, assertion-based TAT/UAT flows, config persistence and interactive selection, macOS keychain signer, and comprehensive tests. ChangesPrivate Key JWT — Single Cohort
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@40de8a44dc4f0171ca083f182038fb8dca2e3f78🧩 Skill updatenpx skills add larksuite/cli#feat/app_registration_v3 -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1379 +/- ##
==========================================
+ Coverage 71.98% 72.07% +0.08%
==========================================
Files 700 707 +7
Lines 66293 66888 +595
==========================================
+ Hits 47721 48207 +486
- Misses 14867 14944 +77
- Partials 3705 3737 +32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/config/init_interactive.go`:
- Around line 243-257: Add a short clarifying comment next to the init guard
that checks initResp.SupportedAuthMethods (the block starting with if
len(initResp.SupportedAuthMethods) > 0 && !slices.Contains(...)) describing that
an empty SupportedAuthMethods slice is intentionally treated as “older server /
unknown” and therefore allows the requested private_key_jwt to proceed (matching
resolveFinalAuthMethod’s back-compat behavior). Then add a unit test for
RequestAppRegistrationInit handling an empty SupportedAuthMethods array: mock
larkauth.RequestAppRegistrationInit to return SupportedAuthMethods: [] and
assert that the code path allows private_key_jwt (no rejection) and that the
comment’s documented behavior is covered; reference the init logic and
resolveFinalAuthMethod in the test to show alignment.
In `@extension/keysigner/signer_keychain_darwin.go`:
- Line 372: This file uses direct os.* filesystem calls (e.g., os.ReadFile at
the shown diff and additional uses of
ReadFile/WriteFile/Stat/MkdirAll/Remove/CreateTemp/Executable elsewhere in
signer_keychain_darwin.go) which violates the forbidigo rule; replace all these
os.X calls with the corresponding internal/vfs helpers (vfs.ReadFile,
vfs.WriteFile, vfs.Stat, vfs.MkdirAll, vfs.Remove, vfs.CreateTemp,
vfs.Executable) and import the internal/vfs package, ensuring each call site
(for example the function that reads key data where os.ReadFile(path) is used)
uses vfs.* instead; do not add a //nolint:forbidigo—migrate the calls instead to
satisfy lint/build checks.
In `@internal/auth/app_registration.go`:
- Around line 183-193: The fallback construction for verificationUriComplete
appends "?user_code=..." without handling existing query parameters; update the
logic in internal/auth/app_registration.go where verificationUriComplete is
built (variable verificationUriComplete, values verificationUri and userCode,
and ep.Open) to use the same query-joining logic as BuildVerificationURL: either
detect whether verificationUri (or base) contains a '?' and choose '?' vs '&'
accordingly, or better, construct the URL via net/url (url.URL and url.Values)
to add the user_code parameter safely so you never produce an invalid query
string when verification_uri already has parameters.
In `@internal/auth/jwt/jwt.go`:
- Around line 1-153: Add a unit test to jwt_test.go that triggers json.Marshal
failures by passing an unmarshalable value (e.g. a func or channel) in the
header or claims to exercise the error paths in buildSignedJWT; call
buildSignedJWT directly (or via SignClientAssertion/SignAttestation if you
prefer) with signer nil-check satisfied (use a stub signer) and assert the
returned error is non-nil and contains the marshal-related prefix ("jwt: marshal
header" or "jwt: marshal claims") so the test covers those failure branches.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3a6a530-a2c3-42bb-9e6a-302a413baa72
📒 Files selected for processing (30)
cmd/auth/login.gocmd/auth/login_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_auth_method_test.gocmd/config/init_interactive.goextension/keysigner/keysigner.goextension/keysigner/keysigner_test.goextension/keysigner/registry.goextension/keysigner/signer_keychain_darwin.goextension/keysigner/signer_keychain_darwin_test.gointernal/auth/app_registration.gointernal/auth/app_registration_test.gointernal/auth/client_auth.gointernal/auth/client_auth_test.gointernal/auth/device_flow.gointernal/auth/device_flow_test.gointernal/auth/jwt/jwt.gointernal/auth/jwt/jwt_test.gointernal/auth/uat_client.gointernal/auth/uat_client_options_test.gointernal/core/config.gointernal/core/types.gointernal/core/types_test.gointernal/credential/default_provider.gointernal/credential/tat_fetch.gointernal/credential/tat_fetch_test.gointernal/credential/types.gointernal/identitydiag/diagnostics.gosidecar/server-multi-tenant-demo/auth_bridge.go
albertnusouo
force-pushed
the
feat/app_registration_v3
branch
from
4d38935 to
7575d72
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/config.go`:
- Around line 288-292: The resolver currently copies app.AuthMethod directly
into cfg (AuthMethod: app.AuthMethod) and only conditionally copies
app.KeyRef.ID, allowing unsupported auth methods and permitting private_key_jwt
without a key handle; update the resolution logic to validate and normalize
app.AuthMethod into cfg.AuthMethod (reject unknown values) and enforce that when
the resolved/auth method is "private_key_jwt" there is a non-nil app.KeyRef
(otherwise return an error during resolution), and add resolver unit tests
covering an invalid authMethod and the missing keyRef for private_key_jwt to
ensure failures occur at resolution time.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 387f9896-f74c-4cf0-987e-32b943a94f96
📒 Files selected for processing (30)
cmd/auth/login.gocmd/auth/login_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_auth_method_test.gocmd/config/init_interactive.goextension/keysigner/keysigner.goextension/keysigner/keysigner_test.goextension/keysigner/registry.goextension/keysigner/signer_keychain_darwin.goextension/keysigner/signer_keychain_darwin_test.gointernal/auth/app_registration.gointernal/auth/app_registration_test.gointernal/auth/client_auth.gointernal/auth/client_auth_test.gointernal/auth/device_flow.gointernal/auth/device_flow_test.gointernal/auth/jwt/jwt.gointernal/auth/jwt/jwt_test.gointernal/auth/uat_client.gointernal/auth/uat_client_options_test.gointernal/core/config.gointernal/core/types.gointernal/core/types_test.gointernal/credential/default_provider.gointernal/credential/tat_fetch.gointernal/credential/tat_fetch_test.gointernal/credential/types.gointernal/identitydiag/diagnostics.gosidecar/server-multi-tenant-demo/auth_bridge.go
🚧 Files skipped from review as they are similar to previous changes (29)
- cmd/auth/login_test.go
- internal/core/types.go
- cmd/config/config_test.go
- sidecar/server-multi-tenant-demo/auth_bridge.go
- internal/auth/uat_client_options_test.go
- extension/keysigner/registry.go
- internal/identitydiag/diagnostics.go
- internal/credential/tat_fetch.go
- internal/auth/uat_client.go
- internal/credential/default_provider.go
- extension/keysigner/signer_keychain_darwin_test.go
- internal/auth/client_auth.go
- extension/keysigner/keysigner.go
- internal/core/types_test.go
- internal/credential/tat_fetch_test.go
- cmd/auth/login.go
- internal/auth/device_flow_test.go
- cmd/config/init_auth_method_test.go
- internal/auth/client_auth_test.go
- internal/auth/jwt/jwt.go
- internal/credential/types.go
- extension/keysigner/keysigner_test.go
- internal/auth/app_registration_test.go
- extension/keysigner/signer_keychain_darwin.go
- internal/auth/device_flow.go
- cmd/config/init_interactive.go
- internal/auth/jwt/jwt_test.go
- cmd/config/init.go
- internal/auth/app_registration.go
…ack-compat - ResolveConfigFromMulti: reject unknown authMethod and require keyRef for private_key_jwt at resolution time (fail-fast vs. silent client_secret degrade or later token-signing failure) - init_interactive: comment why an empty SupportedAuthMethods intentionally allows the requested private_key_jwt (older-server back-compat, mirrors resolveFinalAuthMethod) - tests: invalid authMethod & missing keyRef resolution errors; empty SupportedAuthMethods init parse; explicit empty-slice resolveFinalAuthMethod
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/core/config_test.go (2)
159-192: ⚡ Quick winStrengthen error assertions for consistency.
The test correctly validates both the failure case (missing
KeyRef) and the success case (withKeyRef, verifyingKeyLabelderivation). However, the error assertions only check the error type. Following the existing pattern at lines 110-112, consider also asserting thatcfgErr.HintandcfgErr.Messageare non-empty to ensure users receive actionable error guidance.📋 Proposed enhancement
var cfgErr *ConfigError if !errors.As(err, &cfgErr) { t.Fatalf("expected ConfigError, got %T: %v", err, err) } + if cfgErr.Hint == "" { + t.Error("expected non-empty hint in ConfigError") + } // Control: same config WITH a keyRef resolves cleanly and sets KeyLabel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/config_test.go` around lines 159 - 192, In TestResolveConfigFromMulti_PrivateKeyJWTRequiresKeyRef, after asserting the error is a ConfigError via errors.As into cfgErr, add assertions to verify cfgErr.Hint and cfgErr.Message are non-empty (e.g., use if cfgErr.Hint == "" { t.Fatalf(...) } and similarly for cfgErr.Message) so the failure case from ResolveConfigFromMulti yields actionable guidance; keep the checks consistent with the existing pattern used elsewhere (see other tests that assert cfgErr.Hint and cfgErr.Message).
135-157: ⚡ Quick winStrengthen error assertions to match existing test pattern.
The test correctly validates that an unknown
AuthMethodfails with a*ConfigError, but doesn't assert any of the error's fields. The existing pattern in this file (lines 110-112 inTestResolveConfigFromMulti_RejectsSecretKeyMismatch) also checks thatcfgErr.Hintis non-empty. Consider asserting onMessageandHintto ensure the error provides actionable guidance and to improve regression protection.📋 Proposed enhancement
var cfgErr *ConfigError if !errors.As(err, &cfgErr) { t.Fatalf("expected ConfigError, got %T: %v", err, err) } + if cfgErr.Hint == "" { + t.Error("expected non-empty hint in ConfigError") + } + if cfgErr.Message == "" { + t.Error("expected non-empty message in ConfigError") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/core/config_test.go` around lines 135 - 157, Update TestResolveConfigFromMulti_RejectsUnknownAuthMethod to assert the returned *ConfigError contains actionable fields: after confirming err is a *ConfigError (cfgErr), add checks that cfgErr.Message and cfgErr.Hint are non-empty (or otherwise validate expected substrings about unknown AuthMethod) so the test mirrors the pattern used in TestResolveConfigFromMulti_RejectsSecretKeyMismatch and guards against regressions in ResolveConfigFromMulti.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/core/config_test.go`:
- Around line 159-192: In
TestResolveConfigFromMulti_PrivateKeyJWTRequiresKeyRef, after asserting the
error is a ConfigError via errors.As into cfgErr, add assertions to verify
cfgErr.Hint and cfgErr.Message are non-empty (e.g., use if cfgErr.Hint == "" {
t.Fatalf(...) } and similarly for cfgErr.Message) so the failure case from
ResolveConfigFromMulti yields actionable guidance; keep the checks consistent
with the existing pattern used elsewhere (see other tests that assert
cfgErr.Hint and cfgErr.Message).
- Around line 135-157: Update
TestResolveConfigFromMulti_RejectsUnknownAuthMethod to assert the returned
*ConfigError contains actionable fields: after confirming err is a *ConfigError
(cfgErr), add checks that cfgErr.Message and cfgErr.Hint are non-empty (or
otherwise validate expected substrings about unknown AuthMethod) so the test
mirrors the pattern used in TestResolveConfigFromMulti_RejectsSecretKeyMismatch
and guards against regressions in ResolveConfigFromMulti.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79e50d94-b61c-450a-b4a2-f0fd1aa4accf
📒 Files selected for processing (5)
cmd/config/init_auth_method_test.gocmd/config/init_interactive.gointernal/auth/app_registration_test.gointernal/core/config.gointernal/core/config_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/core/config.go
- internal/auth/app_registration_test.go
- cmd/config/init_auth_method_test.go
- cmd/config/init_interactive.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a secretless
private_key_jwt(RFC 7523) path for app registration and token acquisition, alongside the existingclient_secretflow. The client holds a non-exportable private key in a platform key store and proves possession with signed JWTs, so no app secret is stored or transmitted. The auth method is gated on device capability and server support;client_secretstays the default and its behavior is unchanged.Changes
extension/keysigner): stdlib-onlySignerinterface + registry; key-store backends register frominit(). Ships a macOS Keychain signer behind thekeychain_signerbuild tag (CGO + Security.framework) that imports a software-generated RSA-2048 key as non-extractable (security import -x) — note: software-level, not Secure Enclave / hardware-backed. The default build stays CGO-free with no signer registered (client_secret only).internal/auth/jwt): stdlib-only registration attestation (public key in thejwkheader) and RFC 7523client_assertion; no new module dependencies (go.mod/go.sumunchanged).config init --auth-method client_secret|private_key_jwt. The private_key_jwt flow runs init (server nonce +supported_auth_methods) → signed attestation → begin, persists no secret (only an opaque key handle), and honors the authoritativeauth_methodreturned at user confirmation.jwt-bearergrant +client_assertion; user device-login and token refresh substituteclient_assertionforclient_secret. All existingclient_secretcode paths are unchanged.auth_method+ key handle;auth statusreports a secretless private_key_jwt app as configured.Test Plan
make unit-test) — added coverage for the signer, JWS, registration init/begin, client-assertion, and tenant/user token pathsgolangci-lint run --new-from-rev=origin/main→ 0 issues;go.mod/go.sumunchanged-tags keychain_signer)Related Issues
Summary by CodeRabbit
New Features
Tests