feat(auth): support private_key_jwt registration and authentication#1379
Open
albertnusouo wants to merge 34 commits into
Open
feat(auth): support private_key_jwt registration and authentication#1379albertnusouo wants to merge 34 commits into
albertnusouo wants to merge 34 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 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@71036917e51cb335e70d528edce219cdac106df8🧩 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 74.45% 74.43% -0.02%
==========================================
Files 860 866 +6
Lines 89906 90774 +868
==========================================
+ Hits 66940 67569 +629
- Misses 17780 17969 +189
- Partials 5186 5236 +50 ☔ 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
…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
private_key_jwt shipped with only a macOS Keychain signer, so keysigner.Active() was nil on Linux/Windows and the secretless auth was unusable there (registration failed with "requires a TEE key signer"). Add a TPM 2.0 signer backed by github.com/facebookincubator/sks — the backend named in the keysigner docstring — behind the `sks_signer` build tag, mirroring the macOS `keychain_signer` gating. Signer (extension/keysigner/signer_sks.go, (linux||windows) && sks_signer): - Non-exportable ECDSA P-256 key in the TPM (/dev/tpmrm0 on Linux, CNG on Windows); ES256. - sks emits ASN.1 DER but JWS requires fixed-width r||s (RFC 7518 §3.4); add ecdsaDERToJOSE in the core and convert. Both sks backends emit DER. - EnsureKey creates-or-loads, PublicKey reads without creating, Sign hashes+signs+converts. - Silence sks's verbose flog (glog-fork) TPM logging in init() via flog.SetOutput(io.Discard); the CLI does not use flog and real failures are returned as errors. TEE diagnostics: - HardwareProber capability + ProbeActiveHardware in the core; sksSigner implements it via sks.GetSecureHardwareVendorData (prefix-collapsed error text). - `lark-cli doctor` gains a tee_signer check: a hard requirement for private_key_jwt apps, informational for client_secret. - doctor renders a human-readable report on a TTY and keeps JSON for pipes/scripts; add IOStreams.StdoutIsTerminal (stdout-based, unlike the stdin-based IsTerminal) so `doctor | jq` still emits JSON. Dependency: pin sks to its last go-1.20 commit (6823f23, before sks bumped its own go directive to 1.24) so the CLI module stays on go 1.23 and the golang.org/x/* packages are not force-upgraded. sks pulls a pure-Go TPM stack, compiled only under -tags sks_signer, so the default build stays free of it (client_secret only). Verified on linux/amd64 against a real TPM 2.0: key creation, ES256 signing with r||s verification, and the full private_key_jwt registration + tenant-token mint via TPM-signed client_assertion.
albertnusouo
force-pushed
the
feat/app_registration_v3
branch
from
June 13, 2026 08:52
75910e8 to
e6c8fd5
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).
# Conflicts: # cmd/config/init.go # cmd/doctor/doctor.go # internal/core/config.go # internal/core/config_test.go # internal/credential/tat_fetch.go
- 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.
albertnusouo
force-pushed
the
feat/app_registration_v3
branch
from
July 11, 2026 10:19
71be742 to
8c11b27
Compare
PR Quality SummaryCI did not complete successfully. Use the failed check links below to decide whether this PR needs a code change or a rerun. Failed checksdeterministic-gate
|
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
2 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 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