feat(auth): support private_key_jwt registration and authentication - #1379
Open
albertnusouo wants to merge 36 commits into
Open
albertnusouo wants to merge 36 commits into
albertnusouo wants to merge 36 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)
✨ 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f8f29fce1e66bef016a33d88ec84a70ea1cdbba8🧩 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 76.67% 76.39% -0.29%
==========================================
Files 1126 1143 +17
Lines 129843 132627 +2784
==========================================
+ Hits 99562 101324 +1762
- Misses 22380 23113 +733
- Partials 7901 8190 +289 ☔ 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
June 10, 2026 11:47
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
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
albertnusouo
force-pushed
the
feat/app_registration_v3
branch
from
June 13, 2026 08:52
75910e8 to
e6c8fd5
Compare
albertnusouo
force-pushed
the
feat/app_registration_v3
branch
from
July 11, 2026 10:19
71be742 to
8c11b27
Compare
albertnusouo
force-pushed
the
feat/app_registration_v3
branch
from
August 10, 2026 09:39
92df3c0 to
6918b52
Compare
|
|
albertnusouo
force-pushed
the
feat/app_registration_v3
branch
from
September 9, 2026 08:32
47930a5 to
40560c8
Compare
kiraWangRuilong
force-pushed
the
feat/app_registration_v3
branch
from
September 14, 2026 04:01
40560c8 to
f21996c
Compare
kiraWangRuilong
added
domain/auth
github-actions
Bot
added
size/XL
kiraWangRuilong
force-pushed
the
feat/app_registration_v3
branch
5 times, most recently
from
September 22, 2026 08:06
3044a3b to
6515047
Compare
kiraWangRuilong
force-pushed
the
feat/app_registration_v3
branch
from
September 22, 2026 09:07
6515047 to
84ececf
Compare
kiraWangRuilong
force-pushed
the
feat/app_registration_v3
branch
2 times, most recently
from
September 24, 2026 05:14
d0aff4e to
bb0d0d9
Compare
…ows, keychain_signer darwin)
Replace the cgo Security.framework bindings with runtime FFI (ebitengine/purego) so the keychain_signer builds with CGO_ENABLED=0 and cross-compiles for darwin from any host. Same non-extractable-key security model (SecKeyCreateSignature on an OS-held key). Release goes back to a single ubuntu runner; a macos-latest job validates the FFI round-trip on real hardware as a release gate.
…build Drop the keychain_signer build tag now that the signer is cgo-free (purego runtime FFI). darwin builds always include it, so release and PR-preview binaries are signed without a tag. Adjust go vet to -unsafeptr=false for the FFI data-symbol dereference (golangci-lint still runs full govet honoring the inline //nolint:govet).
- init_interactive.go: use errors.Is(err, huh.ErrUserAborted) instead of == (the new auth-method picker path; comparison fails on wrapped errors) - app_registration.go: wrap read-body error with %w instead of %v
sks's Windows COM dependency go-ole v1.2.5 has no arm64 VARIANT, so building windows/arm64 with -tags sks_signer fails (undefined: VARIANT). Mirror .goreleaser.yml's windows-arm64 build: ship arm64 without the TPM signer (client_secret only). Other targets keep sks_signer.
The keychain signer lacked a HardwareProber, so probeHardware() returned ok=false and doctor printed "no TEE signer in this build" on macOS — a false negative, since the signer is registered and private_key_jwt works. Implement ProbeHardware on keychainSigner (reports backend=keychain, available when /usr/bin/security is present; no key access, no prompt) so doctor shows 'keychain TEE available'.
…ault Drop the sks_signer build tag, mirroring the darwin keychain signer: the TPM signer now compiles into every linux and windows/amd64 build via constraint //go:build linux || (windows && amd64) — no -tags needed. windows/arm64 is arch-excluded (go-ole has no arm64 VARIANT) and falls back to client_secret only. - goreleaser: drop -tags=sks_signer; merge windows-arm64 into the windows build (amd64+arm64) since no tag is needed and arm64 is arch-excluded. - build-pkg-pr-new.sh: remove tag logic. - doctor: update the no-signer hint (signer ships by default on macOS, Linux, Windows/amd64). - Switching from a custom tag to GOOS/GOARCH constraints also lets go mod tidy track sks/go-tpm/go-ole correctly.
kiraWangRuilong
force-pushed
the
feat/app_registration_v3
branch
from
September 28, 2026 07:48
bb0d0d9 to
f8f29fc
Compare
This branch has not been deployed
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
4 participants
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 app-secret-free client authentication for Lark/Feishu applications alongside the existing
client_secretflow:private_key_jwtuses a CLI-managed signing key.private_key_jwt_local_keypairreferences a user-owned PEM whose public key is already registered for the app.Both modes prove possession with RFC 7523 client assertions. They do not store or transmit an AppSecret. The actual private-key storage depends on the selected signer backend: a platform key store, an encrypted software key file, a verified external provider, or a referenced PEM file.
client_secretremains the default authentication method.Changes
Signer architecture (
internal/keysigner,internal/keylesshelper)puregoSecurity.framework bindings.go-tpm.software-filefallback and verifiedlarksuite.keylessexternal-provider routing.Key storage and references
authMethodplus a typedkeyRef(source,provider,id) instead of an AppSecret.software-filestores an AES-GCM-encrypted PKCS#8 private key protected by a random unlock secret held in the credential store.--private-key-filereferences an existing user-owned PEM; the CLI does not copy or delete that file.JWT signing (
internal/auth/jwt)kid.kid, allowing the server to select the registered public key.Registration and CLI
lark-cli config init --new --private-key-jwtregisters an app with a CLI-managed key.lark-cli config init --app-id <app-id> --private-key-file <path>configuresprivate_key_jwt_local_keypairfor an already-registered public key.Token endpoints
client_assertion.client_secret.client_secretremains supported and is still the default, while sharing some registration and token infrastructure with the new modes.Config and diagnostics
authMethod, key source, provider, and key identity before use.auth statusrecognizes AppSecret-free applications.doctorchecks the configured signer and reports its backend andkid.Build and dependencies
CGO_ENABLED=0; signer inclusion is selected by GOOS/GOARCH constraints, not custom build tags.github.com/ebitengine/purego,github.com/google/go-tpm, andgolang.org/x/crypto.Compatibility and limitations
client_secretremains the default for existing configurations.--private-key-filemust have its public key registered for the target app before running the command.lark-cli eventWebSocket connections are not supported by private-key JWT profiles.Test Plan
make buildmake fmt-checkkidmatching, uniquejti, expected claims/TTL, and absence ofclient_secretcmd/doctor/doctor_test.go; fixed in6515047f. GitHub checks for the latest commit are authoritative.Related Issues