Skip to content

fix(shared): remove libsodium from device identity#746

Draft
vincentkoc wants to merge 1 commit into
mainfrom
fix/libsodium-arm64-tests
Draft

fix(shared): remove libsodium from device identity#746
vincentkoc wants to merge 1 commit into
mainfrom
fix/libsodium-arm64-tests

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

Summary

  • replace NSec.Cryptography/libsodium device identity signing with managed BouncyCastle Ed25519 using the same raw 32-byte private/public key and 64-byte signature shapes
  • remove test-host libsodium.dll copying and retarget release native dependency checks to the remaining native speech/ONNX runtime payload
  • update release docs, CI comments, and release workflow tests so the native runtime policy no longer requires a deleted libsodium payload

Context

This was exposed while validating #744 on a Windows 11 ARM64 VM: DeviceIdentity failed during static initialization because NSec could not load libsodium.dll or its ARM64 VC runtime dependency. The cleaner fix is to remove that native dependency from identity signing entirely rather than add another fragile ARM64 test-host copy path.

Verification

  • Windows 11 ARM64 VM: dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj → 2251 passed
  • Windows 11 ARM64 VM: dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore → 2251 passed
  • Windows 11 ARM64 VM: dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj → 1040 passed
  • Windows 11 ARM64 VM: dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore → 1040 passed
  • Windows 11 ARM64 VM: built output check found no libsodium.dll under the shared/tray test outputs
  • Windows 11 ARM64 VM: dotnet list .\src\OpenClaw.Shared\OpenClaw.Shared.csproj package --include-transitive | findstr /i "NSec libsodium BouncyCastle" → only BouncyCastle.Cryptography 2.6.2
  • local: git diff --check
  • local: autoreview clean with /Users/vincentkoc/GIT/_Perso/openclaw/.agents/skills/autoreview/scripts/autoreview --mode local --base origin/main

./build.ps1 was attempted on the same Windows ARM64 VM but is environment-blocked there: missing Git, Node.js, and Windows 10 SDK. The required shared/tray test projects do build and pass on that VM.

@clawsweeper

clawsweeper Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 11, 2026, 4:59 AM ET / 08:59 UTC.

Summary
The PR replaces NSec/libsodium-backed device identity generation and signing with managed BouncyCastle Ed25519, removes libsodium test-host copying, and retargets release native-dependency validation and documentation to the remaining speech/ONNX payload.

Reproducibility: yes. at source level: current main loads NSec/libsodium during device identity use, and the related merged ARM64 work records broad test failures from that native dependency; this review did not independently execute a Windows VM.

Review metrics: 3 noteworthy metrics.

  • Patch surface: 9 files, 72 additions, 69 deletions. The bounded patch spans authentication identity code, dependency selection, tests, and native release policy.
  • Reported ARM64 tests: 3,291 tests passed. The PR provides real Windows ARM64 after-change execution across both required test projects.
  • Native crypto dependencies: 1 removed. Removing libsodium eliminates the reported ARM64 native-load path from device identity initialization.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Obtain a successful ./build.ps1 result from a complete Windows toolchain or the required Windows CI gate.

Risk before merge

  • [P1] The repository-required full ./build.ps1 validation did not complete on the contributor’s Windows ARM64 VM because Git, Node.js, and the Windows 10 SDK were absent; publish and installer integration therefore still depend on a complete Windows CI or maintainer environment.
  • [P1] This deliberately changes the cryptographic implementation behind persisted gateway identities, so maintainers should treat the existing raw-key and wire-format contract as an upgrade invariant even though the inspected code preserves it.

Maintainer options:

  1. Complete the Windows gate (recommended)
    Run the required full build and both test projects on a complete Windows toolchain, then merge if the existing ARM64 identity and package-output evidence remains green.
  2. Accept CI as the full-build proof
    Maintain the patch as written if required Windows CI covers the full build, publish, and native dependency checks that the contributor VM could not run.
  3. Keep the draft open
    Pause landing if neither maintainer validation nor required CI can demonstrate the complete publish and installer path.

Next step before merge

  • [P2] A maintainer should confirm the complete Windows build/publish gate and then handle this member-authored draft; no concrete patch defect requires an automated repair.

Security
Cleared: The patch removes native cryptographic code execution and adds no new secret access, workflow permission, artifact download, lifecycle hook, or unpinned third-party execution path.

Review details

Best possible solution:

Merge the managed BouncyCastle implementation after a complete Windows environment passes ./build.ps1 and the required shared and tray suites, retaining the current persisted key and signature formats without a migration.

Do we have a high-confidence way to reproduce the issue?

Yes at source level: current main loads NSec/libsodium during device identity use, and the related merged ARM64 work records broad test failures from that native dependency; this review did not independently execute a Windows VM.

Is this the best way to solve the issue?

Yes. Removing the native cryptography dependency while retaining the established raw Ed25519 formats is narrower and more maintainable than adding another architecture-specific DLL-copy workaround.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 913ba4e8f504.

Label changes

Label changes:

  • add P2: This fixes a real Windows ARM64 identity initialization failure with limited platform scope and no evidence of data loss or security bypass.
  • add merge-risk: 🚨 compatibility: Upgraded installations must retain their existing raw Ed25519 identity, public key, and device ID across the provider replacement.
  • add merge-risk: 🚨 auth-provider: The changed key derivation and signing implementation directly supplies gateway device authentication credentials.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body contains credible after-fix Windows 11 ARM64 terminal results for both full test projects, repeat runs, output inspection, and resolved dependency inspection; future posted proof should continue to redact keys, tokens, private endpoints, host details, and other private information.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body contains credible after-fix Windows 11 ARM64 terminal results for both full test projects, repeat runs, output inspection, and resolved dependency inspection; future posted proof should continue to redact keys, tokens, private endpoints, host details, and other private information.

Label justifications:

  • P2: This fixes a real Windows ARM64 identity initialization failure with limited platform scope and no evidence of data loss or security bypass.
  • merge-risk: 🚨 compatibility: Upgraded installations must retain their existing raw Ed25519 identity, public key, and device ID across the provider replacement.
  • merge-risk: 🚨 auth-provider: The changed key derivation and signing implementation directly supplies gateway device authentication credentials.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body contains credible after-fix Windows 11 ARM64 terminal results for both full test projects, repeat runs, output inspection, and resolved dependency inspection; future posted proof should continue to redact keys, tokens, private endpoints, host details, and other private information.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body contains credible after-fix Windows 11 ARM64 terminal results for both full test projects, repeat runs, output inspection, and resolved dependency inspection; future posted proof should continue to redact keys, tokens, private endpoints, host details, and other private information.
Evidence reviewed

What I checked:

Likely related people:

  • christineyan4: Christine Yan authored commit 85445c7, which introduced the current raw Ed25519 key storage, public-key-derived device ID, and signing behavior being migrated. (role: introduced behavior; confidence: high; commits: 85445c78066b; files: src/OpenClaw.Shared/DeviceIdentity.cs, tests/OpenClaw.Shared.Tests/DeviceIdentityTests.cs)
  • Copilot: Commit 8bf605c introduced the native release dependency verifier and VC runtime publish policy that this patch retargets after removing libsodium. (role: recent area contributor; confidence: high; commits: 8bf605cdbd32; files: scripts/Test-ReleaseNativeDependencies.ps1, src/Directory.Build.targets, docs/RELEASING.md)
  • vincentkoc: Vincent Koc authored the immediately preceding merged ARM64 validation work whose full test failures exposed the libsodium load problem, providing relevant current-main platform context beyond merely opening this PR. (role: recent adjacent contributor; confidence: medium; commits: 913ba4e8f504; files: tests/OpenClaw.Shared.Tests/DeviceIdentityTests.cs, tests/OpenClaw.Tray.Tests/ReleaseSigningWorkflowTests.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant