feat(config): bind OpenClaw keyless credentials#1999
Open
evandance wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR adds private-key JWT authentication with hardware-backed and external keyless signers, updates registration, device flow, config init, binding, token minting, diagnostics, and platform-specific release validation. ChangesAuthentication and signing
Configuration and operations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigInit
participant AppRegistration
participant KeySigner
participant OAuth
ConfigInit->>KeySigner: Probe hardware signer
ConfigInit->>AppRegistration: Request nonce and supported auth methods
AppRegistration-->>ConfigInit: Return nonce and auth methods
ConfigInit->>KeySigner: Sign registration attestation
ConfigInit->>AppRegistration: Begin registration with attestation
AppRegistration-->>ConfigInit: Return client ID and auth method
ConfigInit->>OAuth: Mint token with client assertion
OAuth-->>ConfigInit: Return access token
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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@a81e5d17c544f8b4ec7b65d47b16a96f82bdf84a🧩 Skill updatenpx skills add larksuite/cli#feat/keyless-integration -y -g |
![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
🧹 Nitpick comments (4)
internal/keylesshelper/helper.go (1)
195-198: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winApply the restricted environment independently of
cwd.
cmd.Env = envis set only inside thecwd != ""branch. The productionexecutepath always passes a non-emptyproviderCWD, so the minimal environment is applied today. But coupling the two means any caller that supplies a restrictedenvwith an emptycwd(e.g.runCommand) silently drops it and the child inherits the full parent environment — the opposite of the intended isolation. Decouple them so the restricted env is always honored when provided.🔒 Proposed change
cmd := exec.CommandContext(helperCtx, argv[0], argv[1:]...) - if cwd != "" { - cmd.Dir = cwd - cmd.Env = env - } + if cwd != "" { + cmd.Dir = cwd + } + if env != nil { + cmd.Env = env + }🤖 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/keylesshelper/helper.go` around lines 195 - 198, Decouple environment assignment from the working-directory condition in the command setup: keep cmd.Dir = cwd conditional on a non-empty cwd, but apply the provided env independently whenever it is supplied. Update the surrounding command-execution logic near runCommand so an empty cwd cannot cause a restricted environment to be omitted.cmd/config/binder.go (2)
204-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a shared constant for the
"app_secret"literal.
AuthMethodPrivateKeyJWThas a package constant ininternal/binding/types.go, but the sibling"app_secret"value used here is a bare string literal. Definingbinding.AuthMethodAppSecret = "app_secret"would prevent drift between this check and any future openclaw-schema consumer.🤖 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 `@cmd/config/binder.go` around lines 204 - 207, Define a shared binding.AuthMethodAppSecret constant with the value "app_secret" alongside AuthMethodPrivateKeyJWT in internal/binding/types.go, then update the AuthMethod validation in the binder to compare against that constant instead of the bare literal while preserving the existing supported-value behavior.
44-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale interface doc: no "signer command" is ever returned.
SourceBinder.Build's comment says it returns "the app plus any signer command needed by the workspace," butBindResultonly carriesAppConfig, and its own comment clarifies signer info is just the logical provider onAppConfig.KeyRef— no command/path is ever returned. Worth tightening the wording to avoid a future implementer expecting a command field.✏️ Suggested wording
- // Build resolves credentials and returns the app plus any signer command - // needed by the workspace. Must be called after ListCandidates succeeds. + // Build resolves credentials into the app to persist. External signer + // configuration, if any, is embedded as a logical provider reference on + // AppConfig.KeyRef. Must be called after ListCandidates succeeds. Build(ctx context.Context, candidate Candidate) (*BindResult, error)🤖 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 `@cmd/config/binder.go` around lines 44 - 46, Update the SourceBinder.Build interface comment to remove the claim that it returns a signer command, and describe it as resolving credentials and returning the app configuration while signer information is represented through AppConfig.KeyRef. Keep the requirement that Build follows ListCandidates succeeds.internal/core/config.go (1)
286-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
core.SecretSourceTEEinstead of the literal"tee".
internal/core/config.go#L286: replaceapp.KeyRef.Source != "tee"withapp.KeyRef.Source != SecretSourceTEE.cmd/config/init.go#L194: replaceSource: "tee"withSource: core.SecretSourceTEE.🤖 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.go` at line 286, Replace the literal TEE source value with the shared SecretSourceTEE constant in both sites: update the source comparison in internal/core/config.go at lines 286-286, and set Source to core.SecretSourceTEE in cmd/config/init.go at lines 194-194.
🤖 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 @.github/workflows/release.yml:
- Around line 44-55: Harden the signer-test-macos job by configuring the
actions/checkout step to disable persisted credentials and configuring
actions/setup-go to disable its default dependency caching. Update only the
checkout and setup-go steps, preserving their existing versions and Go version.
In `@cmd/config/keyless_bind_test.go`:
- Around line 109-126: The error-path test
TestConfigBindRun_OpenClawKeylessProbeFailureDoesNotWrite only checks that
configBindRun returns an error. Strengthen its assertions by extracting the
typed problem with errs.ProblemOf and validating the expected category, subtype,
and parameter, while also verifying that the original errs.NewConfigError cause
is preserved; retain the existing assertion that the config file is not written.
In `@cmd/doctor/doctor_test.go`:
- Around line 182-220: Add an adjacent test covering the external-keyless-signer
path in teeSignerCheck by configuring an app with a non-empty KeyProvider and
exercising keylessprovider.Resolve. Directly assert the resulting check status
and message for both the successful/warn and failure cases as applicable,
ensuring regressions in the cfg.KeyProvider branch and its fail/warn behavior
cause the test to fail; retain TestDoctorRun_TeeSignerWired for the
client_secret path.
In `@cmd/doctor/doctor.go`:
- Around line 164-187: Update teeSignerCheck to enter the external signer flow
whenever cfg.KeyProvider is non-empty, regardless of usesPKJWT, so client_secret
configurations produce the intended warning while private_key_jwt configurations
still fail. After keylessprovider.Resolve, validate that helper is non-nil
before calling helper.Probe, and return the existing unavailable/misconfigured
result with a repair hint when resolution yields no helper.
---
Nitpick comments:
In `@cmd/config/binder.go`:
- Around line 204-207: Define a shared binding.AuthMethodAppSecret constant with
the value "app_secret" alongside AuthMethodPrivateKeyJWT in
internal/binding/types.go, then update the AuthMethod validation in the binder
to compare against that constant instead of the bare literal while preserving
the existing supported-value behavior.
- Around line 44-46: Update the SourceBinder.Build interface comment to remove
the claim that it returns a signer command, and describe it as resolving
credentials and returning the app configuration while signer information is
represented through AppConfig.KeyRef. Keep the requirement that Build follows
ListCandidates succeeds.
In `@internal/core/config.go`:
- Line 286: Replace the literal TEE source value with the shared SecretSourceTEE
constant in both sites: update the source comparison in internal/core/config.go
at lines 286-286, and set Source to core.SecretSourceTEE in cmd/config/init.go
at lines 194-194.
In `@internal/keylesshelper/helper.go`:
- Around line 195-198: Decouple environment assignment from the
working-directory condition in the command setup: keep cmd.Dir = cwd conditional
on a non-empty cwd, but apply the provided env independently whenever it is
supplied. Update the surrounding command-execution logic near runCommand so an
empty cwd cannot cause a restricted environment to be omitted.
🪄 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: 47615d2a-4434-42e7-a879-ae9765fcb856
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (62)
.github/workflows/release.yml.goreleaser.ymlcmd/auth/login.gocmd/auth/login_test.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_auth_method_test.gocmd/config/init_interactive.gocmd/config/init_probe.gocmd/config/init_probe_test.gocmd/config/init_test.gocmd/config/keyless_bind.gocmd/config/keyless_bind_test.gocmd/doctor/doctor.gocmd/doctor/doctor_test.gogo.modinternal/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/binding/keyless_types_test.gointernal/binding/types.gointernal/core/config.gointernal/core/config_test.gointernal/core/secret.gointernal/core/types.gointernal/core/types_test.gointernal/credential/default_provider.gointernal/credential/tat_fetch.gointernal/credential/tat_fetch_test.gointernal/credential/types.gointernal/credential/types_test.gointernal/identitydiag/diagnostics.gointernal/keylesshelper/helper.gointernal/keylesshelper/helper_test.gointernal/keylessprovider/inspect_command_unix.gointernal/keylessprovider/inspect_command_windows.gointernal/keylessprovider/inspect_command_windows_test.gointernal/keylessprovider/provider.gointernal/keylessprovider/provider_test.gointernal/keylessprovider/security_unix.gointernal/keylessprovider/security_windows.gointernal/keysigner/keysigner.gointernal/keysigner/keysigner_test.gointernal/keysigner/registry.gointernal/keysigner/signer_keychain_darwin.gointernal/keysigner/signer_keychain_darwin_test.gointernal/keysigner/signer_sks.gointernal/keysigner/signer_sks_test.gorelease_config_test.goscripts/build-pkg-pr-new.shsidecar/server-multi-tenant-demo/auth_bridge.go
Comment on lines
+44
to
+55
| signer-test-macos: | ||
| runs-on: macos-latest | ||
| permissions: | ||
| contents: read | ||
| steps: | ||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | ||
| - uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5 | ||
| with: | ||
| go-version: '1.23' | ||
| - name: Keychain signer round-trip (CGO-free purego FFI) | ||
| run: LARK_KEYCHAIN_IT=1 CGO_ENABLED=0 go test -tags keychain_signer -run Keychain -v ./internal/keysigner/ | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Harden the new signer-test-macos job per zizmor findings.
Two supply-chain hardening gaps flagged by static analysis on the new job:
checkout@v4(Line 49) doesn't setpersist-credentials: false, leaving theGITHUB_TOKENcredential on disk for the duration of the job (which then runs arbitrarygo testcode).setup-go@v5(Line 50) enables its default caching, which is susceptible to cache-poisoning if the cache is later reused by another workflow run.
🔒️ Proposed hardening
- uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
+ with:
+ persist-credentials: false
- uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5
with:
go-version: '1.23'
+ cache: false📝 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.
Suggested change
| signer-test-macos: | |
| runs-on: macos-latest | |
| permissions: | |
| contents: read | |
| steps: | |
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | |
| - uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5 | |
| with: | |
| go-version: '1.23' | |
| - name: Keychain signer round-trip (CGO-free purego FFI) | |
| run: LARK_KEYCHAIN_IT=1 CGO_ENABLED=0 go test -tags keychain_signer -run Keychain -v ./internal/keysigner/ | |
| signer-test-macos: | |
| runs-on: macos-latest | |
| permissions: | |
| contents: read | |
| steps: | |
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | |
| with: | |
| persist-credentials: false | |
| - uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff # v5 | |
| with: | |
| go-version: '1.23' | |
| cache: false | |
| - name: Keychain signer round-trip (CGO-free purego FFI) | |
| run: LARK_KEYCHAIN_IT=1 CGO_ENABLED=0 go test -tags keychain_signer -run Keychain -v ./internal/keysigner/ |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 49-49: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 50-50: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): enables caching by default
(cache-poisoning)
🤖 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 @.github/workflows/release.yml around lines 44 - 55, Harden the
signer-test-macos job by configuring the actions/checkout step to disable
persisted credentials and configuring actions/setup-go to disable its default
dependency caching. Update only the checkout and setup-go steps, preserving
their existing versions and Go version.
Source: Linters/SAST tools
Comment on lines
+109
to
+126
| func TestConfigBindRun_OpenClawKeylessProbeFailureDoesNotWrite(t *testing.T) { | ||
| saveWorkspace(t) | ||
| clearAgentEnv(t) | ||
| base := t.TempDir() | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", base) | ||
| writeOpenClawKeylessConfig(t, "cli_wrong_key", "openclaw-lark") | ||
| replaceBindProbe(t, func(context.Context, *http.Client, core.LarkBrand, string, keysigner.Signer, string, string) (string, error) { | ||
| return "", errs.NewConfigError(errs.SubtypeInvalidClient, "public key is not bound") | ||
| }) | ||
|
|
||
| f, _, _, _ := cmdutil.TestFactory(t, nil) | ||
| if err := configBindRun(&BindOptions{Factory: f, Source: "openclaw", Identity: "bot-only"}); err == nil { | ||
| t.Fatal("expected probe error") | ||
| } | ||
| if _, err := os.Stat(filepath.Join(base, "openclaw", "config.json")); !os.IsNotExist(err) { | ||
| t.Fatalf("config must not be written; stat error = %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert typed error metadata instead of a bare err == nil check.
This error-path test for configBindRun (a cmd/**/*.go command) only checks err == nil; it never verifies the typed category/subtype/param via errs.ProblemOf, nor that the cause (errs.NewConfigError(...)) is preserved.
✅ Suggested strengthening
f, _, _, _ := cmdutil.TestFactory(t, nil)
- if err := configBindRun(&BindOptions{Factory: f, Source: "openclaw", Identity: "bot-only"}); err == nil {
- t.Fatal("expected probe error")
+ err := configBindRun(&BindOptions{Factory: f, Source: "openclaw", Identity: "bot-only"})
+ if err == nil {
+ t.Fatal("expected probe error")
+ }
+ problem, ok := errs.ProblemOf(err)
+ if !ok || problem.Subtype != errs.SubtypeInvalidClient {
+ t.Fatalf("problem = %#v, ok = %v, want SubtypeInvalidClient", problem, ok)
}As per coding guidelines: "Command error-path tests must assert typed metadata through errs.ProblemOf, including category, subtype, and param, and must verify cause preservation rather than relying only on message substrings."
🤖 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 `@cmd/config/keyless_bind_test.go` around lines 109 - 126, The error-path test
TestConfigBindRun_OpenClawKeylessProbeFailureDoesNotWrite only checks that
configBindRun returns an error. Strengthen its assertions by extracting the
typed problem with errs.ProblemOf and validating the expected category, subtype,
and parameter, while also verifying that the original errs.NewConfigError cause
is preserved; retain the existing assertion that the config file is not written.
Source: Coding guidelines
Comment on lines
+182
to
+220
| // TestDoctorRun_TeeSignerWired proves the tee_signer check is part of doctorRun. | ||
| // It asserts the build-independent invariant (a client_secret app must never | ||
| // FAIL on TEE) so the test passes whether or not a signer is compiled in. | ||
| func TestDoctorRun_TeeSignerWired(t *testing.T) { | ||
| t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) | ||
| if err := core.SaveMultiAppConfig(&core.MultiAppConfig{ | ||
| CurrentApp: "default", | ||
| Apps: []core.AppConfig{{ | ||
| Name: "default", AppId: "test-app", | ||
| AppSecret: core.PlainSecret("secret"), Brand: core.BrandFeishu, | ||
| }}, | ||
| }); err != nil { | ||
| t.Fatalf("SaveMultiAppConfig() error = %v", err) | ||
| } | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, &core.CliConfig{ | ||
| AppID: "test-app", AppSecret: "secret", Brand: core.BrandFeishu, | ||
| }) | ||
| if err := doctorRun(&DoctorOptions{Factory: f, Ctx: context.Background(), Offline: true}); err != nil { | ||
| t.Fatalf("doctorRun() error = %v", err) | ||
| } | ||
| var got struct { | ||
| Checks []checkResult `json:"checks"` | ||
| } | ||
| if err := json.Unmarshal(stdout.Bytes(), &got); err != nil { | ||
| t.Fatalf("json.Unmarshal() error = %v", err) | ||
| } | ||
| var c *checkResult | ||
| for i := range got.Checks { | ||
| if got.Checks[i].Name == "tee_signer" { | ||
| c = &got.Checks[i] | ||
| } | ||
| } | ||
| if c == nil { | ||
| t.Fatalf("tee_signer check not present in doctor output: %#v", got.Checks) | ||
| } | ||
| if c.Status == "fail" { | ||
| t.Errorf("tee_signer = fail for a client_secret app; want skip/warn/pass (msg=%q)", c.Message) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
No test exercises the external-keyless-signer branch of teeSignerCheck.
TestDoctorRun_TeeSignerWired only covers a client_secret app with no KeyProvider, which never enters the cfg.KeyProvider != "" branch in teeSignerCheck (cmd/doctor/doctor.go, Lines 166-183). That branch — including the fail/warn split and the keylessprovider.Resolve call — has zero test coverage, which is how the dead-code/nil-pointer issue flagged on doctor.go went unnoticed. Per path instructions, behavior changes should have an adjacent test that would fail if the implementation regresses.
As per path instructions, "Every behavior change must include an adjacent test, and the test must directly assert the changed field or behavior so reverting the implementation causes the test to fail."
🤖 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 `@cmd/doctor/doctor_test.go` around lines 182 - 220, Add an adjacent test
covering the external-keyless-signer path in teeSignerCheck by configuring an
app with a non-empty KeyProvider and exercising keylessprovider.Resolve.
Directly assert the resulting check status and message for both the
successful/warn and failure cases as applicable, ensuring regressions in the
cfg.KeyProvider branch and its fail/warn behavior cause the test to fail; retain
TestDoctorRun_TeeSignerWired for the client_secret path.
Source: Path instructions
evandance
force-pushed
the
feat/keyless-integration
branch
from
July 22, 2026 11:35
d8fb531 to
e1bd673
Compare
evandance
force-pushed
the
feat/keyless-integration
branch
from
July 22, 2026 11:46
e1bd673 to
a81e5d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/core/config.go (1)
285-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
SecretSourceTEEconstant instead of the"tee"literal.
SecretSourceTEE = "tee"is defined insecret.goandcmd/config/binder.goalready writesSource: core.SecretSourceTEE. Comparing against the literal here risks silent drift if the constant value ever changes.♻️ Suggested tweak
- if app.KeyRef == nil || app.KeyRef.Source != "tee" || app.KeyRef.ID == "" { + if app.KeyRef == nil || app.KeyRef.Source != SecretSourceTEE || app.KeyRef.ID == "" {🤖 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.go` around lines 285 - 289, Update the validation condition in the AuthMethodPrivateKeyJWT configuration path to compare app.KeyRef.Source with the existing SecretSourceTEE constant instead of the literal "tee"; preserve the current nil, ID, and error-handling checks.
🤖 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/binding/types.go`:
- Around line 315-321: Gate the CandidateApp append in the account-candidate
path with bindableCredential(appSecret, authMethod, keyRef), so candidates
lacking both an effective secret and a private_key_jwt key reference are
skipped. Preserve the existing CandidateApp field construction and match the
credential filtering used by the implicit-default path.
In `@internal/credential/default_provider.go`:
- Around line 182-188: The tests should directly exercise DefaultTokenProvider
with AuthMethodPrivateKeyJWT and assert it selects
FetchTATWithAssertionForProvider, forwarding acct.KeyProvider and acct.KeyLabel
rather than using app-secret minting. Add an adjacent provider-selection test
that verifies the returned token and the forwarded key-selection fields.
In `@release_config_test.go`:
- Around line 37-39: Update the Darwin release matrix test around the existing
builds["darwin"] assertion to first verify that the darwin build entry exists
and is non-nil, then retain the unsupported riscv64 check. Ensure the test
directly fails when the Darwin release target is removed.
---
Nitpick comments:
In `@internal/core/config.go`:
- Around line 285-289: Update the validation condition in the
AuthMethodPrivateKeyJWT configuration path to compare app.KeyRef.Source with the
existing SecretSourceTEE constant instead of the literal "tee"; preserve the
current nil, ID, and error-handling checks.
🪄 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: bafe46ed-a992-48ca-a7d0-f435afc0f764
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (65)
.github/workflows/release.yml.goreleaser.ymlcmd/auth/login.gocmd/auth/login_test.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_auth_method_test.gocmd/config/init_interactive.gocmd/config/init_probe.gocmd/config/init_probe_test.gocmd/config/init_test.gocmd/config/keyless_bind.gocmd/config/keyless_bind_test.gocmd/doctor/doctor.gocmd/doctor/doctor_test.gogo.modinternal/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/auth/uat_client_test.gointernal/binding/keyless_types_test.gointernal/binding/types.gointernal/core/config.gointernal/core/config_test.gointernal/core/secret.gointernal/core/types.gointernal/core/types_test.gointernal/credential/default_provider.gointernal/credential/tat_fetch.gointernal/credential/tat_fetch_test.gointernal/credential/types.gointernal/credential/types_test.gointernal/identitydiag/diagnostics.gointernal/keylesshelper/helper.gointernal/keylesshelper/helper_test.gointernal/keylessprovider/inspect_command_unix.gointernal/keylessprovider/inspect_command_windows.gointernal/keylessprovider/inspect_command_windows_test.gointernal/keylessprovider/manifest.gointernal/keylessprovider/manifest_test.gointernal/keylessprovider/provider.gointernal/keylessprovider/provider_test.gointernal/keylessprovider/security_unix.gointernal/keylessprovider/security_windows.gointernal/keysigner/keysigner.gointernal/keysigner/keysigner_test.gointernal/keysigner/registry.gointernal/keysigner/signer_keychain_darwin.gointernal/keysigner/signer_keychain_darwin_test.gointernal/keysigner/signer_sks.gointernal/keysigner/signer_sks_test.gorelease_config_test.goscripts/build-pkg-pr-new.shsidecar/server-multi-tenant-demo/auth_bridge.go
🚧 Files skipped from review as they are similar to previous changes (50)
- internal/keylessprovider/inspect_command_windows.go
- internal/auth/uat_client_options_test.go
- internal/identitydiag/diagnostics.go
- internal/core/types_test.go
- .goreleaser.yml
- sidecar/server-multi-tenant-demo/auth_bridge.go
- internal/core/types.go
- internal/credential/types_test.go
- scripts/build-pkg-pr-new.sh
- internal/auth/jwt/jwt.go
- internal/keysigner/registry.go
- internal/keysigner/signer_sks_test.go
- cmd/config/init_probe.go
- internal/keylesshelper/helper_test.go
- internal/keylessprovider/inspect_command_windows_test.go
- internal/credential/types.go
- internal/core/config_test.go
- internal/auth/uat_client_test.go
- internal/auth/device_flow_test.go
- internal/auth/uat_client.go
- cmd/auth/login_test.go
- internal/keylessprovider/security_unix.go
- internal/keylessprovider/inspect_command_unix.go
- cmd/auth/login.go
- internal/keysigner/keysigner.go
- cmd/config/init_auth_method_test.go
- internal/auth/device_flow.go
- internal/auth/app_registration.go
- cmd/config/bind_test.go
- cmd/config/keyless_bind.go
- internal/auth/client_auth_test.go
- cmd/doctor/doctor_test.go
- cmd/config/binder_test.go
- internal/keysigner/signer_sks.go
- cmd/config/init_probe_test.go
- internal/auth/app_registration_test.go
- cmd/config/config_test.go
- internal/auth/client_auth.go
- cmd/config/init_test.go
- cmd/config/init.go
- internal/keylesshelper/helper.go
- internal/credential/tat_fetch.go
- internal/auth/jwt/jwt_test.go
- cmd/doctor/doctor.go
- cmd/config/binder.go
- internal/keysigner/signer_keychain_darwin_test.go
- cmd/config/keyless_bind_test.go
- cmd/config/init_interactive.go
- internal/keylessprovider/provider.go
- cmd/config/bind.go
Comment on lines
315
to
+321
| apps = append(apps, CandidateApp{ | ||
| Label: label, | ||
| AppID: appID, | ||
| AppSecret: appSecret, | ||
| Brand: brand, | ||
| Label: label, | ||
| AppID: appID, | ||
| AppSecret: appSecret, | ||
| Brand: brand, | ||
| AuthMethod: authMethod, | ||
| KeyRef: keyRef, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip account candidates without an effective credential.
Line 315 appends an account even when its inherited secret is empty and its private_key_jwt key reference is blank. That makes an unusable candidate selectable. Gate the append with bindableCredential(appSecret, authMethod, keyRef), matching the implicit-default path.
Proposed fix
+ if !bindableCredential(appSecret, authMethod, keyRef) {
+ continue
+ }
apps = append(apps, CandidateApp{📝 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.
Suggested change
| apps = append(apps, CandidateApp{ | |
| Label: label, | |
| AppID: appID, | |
| AppSecret: appSecret, | |
| Brand: brand, | |
| Label: label, | |
| AppID: appID, | |
| AppSecret: appSecret, | |
| Brand: brand, | |
| AuthMethod: authMethod, | |
| KeyRef: keyRef, | |
| if !bindableCredential(appSecret, authMethod, keyRef) { | |
| continue | |
| } | |
| apps = append(apps, CandidateApp{ | |
| Label: label, | |
| AppID: appID, | |
| AppSecret: appSecret, | |
| Brand: brand, | |
| AuthMethod: authMethod, | |
| KeyRef: keyRef, |
🤖 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/binding/types.go` around lines 315 - 321, Gate the CandidateApp
append in the account-candidate path with bindableCredential(appSecret,
authMethod, keyRef), so candidates lacking both an effective secret and a
private_key_jwt key reference are skipped. Preserve the existing CandidateApp
field construction and match the credential filtering used by the
implicit-default path.
Comment on lines
+182
to
+188
| if acct.AuthMethod == core.AuthMethodPrivateKeyJWT { | ||
| signer := keysigner.Active() | ||
| token, err := FetchTATWithAssertionForProvider(ctx, httpClient, acct.Brand, acct.AppID, signer, acct.KeyProvider, acct.KeyLabel) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return &TokenResult{Token: token}, nil |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a direct provider-selection test.
The supplied tests cover assertion minting but not that DefaultTokenProvider selects this branch and forwards KeyProvider/KeyLabel instead of using app-secret minting. A regression in this selector could remain undetected.
As per coding guidelines, “Every behavior change must include an adjacent test, and the test must directly assert the changed field or behavior.”
🤖 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/credential/default_provider.go` around lines 182 - 188, The tests
should directly exercise DefaultTokenProvider with AuthMethodPrivateKeyJWT and
assert it selects FetchTATWithAssertionForProvider, forwarding acct.KeyProvider
and acct.KeyLabel rather than using app-secret minting. Add an adjacent
provider-selection test that verifies the returned token and the forwarded
key-selection fields.
Source: Coding guidelines
Comment on lines
+37
to
+39
| if contains(builds["darwin"], "riscv64") { | ||
| t.Errorf("darwin release matrix must not include unsupported riscv64; got %v", builds["darwin"]) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the Darwin build exists.
A missing darwin build makes contains(nil, "riscv64") false, so removing that release target still passes this test.
Proposed fix
+ darwin, ok := builds["darwin"]
+ if !ok {
+ t.Fatal("darwin release build is missing")
+ }
- if contains(builds["darwin"], "riscv64") {
- t.Errorf("darwin release matrix must not include unsupported riscv64; got %v", builds["darwin"])
+ if contains(darwin, "riscv64") {
+ t.Errorf("darwin release matrix must not include unsupported riscv64; got %v", darwin)
}As per coding guidelines, “Every behavior change must include an adjacent test, and the test must directly assert the changed field or behavior so reverting the implementation causes the test to fail.”
📝 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.
Suggested change
| if contains(builds["darwin"], "riscv64") { | |
| t.Errorf("darwin release matrix must not include unsupported riscv64; got %v", builds["darwin"]) | |
| } | |
| darwin, ok := builds["darwin"] | |
| if !ok { | |
| t.Fatal("darwin release build is missing") | |
| } | |
| if contains(darwin, "riscv64") { | |
| t.Errorf("darwin release matrix must not include unsupported riscv64; got %v", darwin) | |
| } |
🤖 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 `@release_config_test.go` around lines 37 - 39, Update the Darwin release
matrix test around the existing builds["darwin"] assertion to first verify that
the darwin build entry exists and is non-nil, then retain the unsupported
riscv64 check. Ensure the test directly fails when the Darwin release target is
removed.
Source: Coding guidelines
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
Add keyless
private_key_jwtauthentication and OpenClaw binding so the CLI can reuse an OpenClaw-managed signing identity without requiring or persisting an app secret.Changes
private_key_jwtacross app registration, tenant-token, and user authorization flows.config bind --source openclaw, persisting only the logical provider and key reference; discovered signer executable paths are never written to CLI config.Test Plan
make unit-testgo vet ./...CGO_ENABLED=0Related Issues
Summary by CodeRabbit
New Features
private_key_jwtauthentication support using secure hardware or approved keyless signing providers.config init --private-key-jwtandconfig init --restorewith auth-method aware registration.doctorwith a TEE/signer diagnostic (tee_signer) and improved human-friendly output while keeping JSON output behavior.Bug Fixes
--app-idmatches.Chores