Skip to content

chore(ci): finalize Artifact Signing setup - #24

Merged
RomneyDa merged 5 commits into
mainfrom
chore/document-artifact-signing-setup
Sep 12, 2026
Merged

RomneyDa merged 5 commits into
mainfrom
chore/document-artifact-signing-setup

Conversation

@RomneyDa

@RomneyDa RomneyDa commented Sep 10, 2026

Copy link
Copy Markdown
Member

Summary

  • read the Azure OIDC identifiers from release-signing environment variables instead of secrets
  • document the exact GitHub environment, immutable OIDC subject, dedicated Entra application, Azure signing resource, and RBAC requirement
  • recurse into the downloaded architecture directories so Artifact Signing actually discovers both MSIX inputs
  • add a workflow contract test that preserves OIDC authentication and forbids a client secret

The packaging workflow already contained most of the MSIX signing implementation. This PR completes and documents its repository and Entra contract.

Validation

  • git diff --check
  • Ruby YAML parse of .github/workflows/gateway-msix.yml
  • verified the repository OIDC settings report use_immutable_subject: true and the documented owner/repository IDs match GitHub's API
  • verified the dedicated Entra application, federated credential, certificate-profile role assignment, and GitHub environment variables
  • PowerShell, .NET, NativeAOT, package, and CodeQL validation run in GitHub Actions

Worked on by

@RomneyDa

Copy link
Copy Markdown
Member Author

I will set up required environment etc BEFORE merging this pr

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. 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. labels Sep 11, 2026
@clawsweeper

clawsweeper Bot commented Sep 11, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed September 11, 2026, 10:16 PM ET / September 12, 2026, 02:16 UTC (Revision 6).

ClawSweeper review

What this changes

The PR reads Azure signing identifiers from environment variables, enables signing of packages in architecture subdirectories, documents setup, and adds a workflow configuration check.

Merge readiness

Needs changes before merge - 2 items remain

The signing fixes remain necessary on main, and the maintainer confirmed the external configuration is ready. One small repository test-policy violation remains; no introduced runtime or security defect was found.

Priority: P2
Reviewed head: c4f56ba483bde91e93067562a8f213df87096726

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused signing correction with confirmed setup and one small validation-policy repair.
Proof confidence 🌊 off-meta tidepool Not applicable: The MEMBER-authored PR is exempt from ordinary contributor runtime proof. The maintainer confirmed external configuration; official Azure signing was skipped in PR checks and first-install validation remains scheduled before release announcement. No material expansion of authorization was found that requires additional authority-chain proof.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The MEMBER-authored PR is exempt from ordinary contributor runtime proof. The maintainer confirmed external configuration; official Azure signing was skipped in PR checks and first-install validation remains scheduled before release announcement. No material expansion of authorization was found that requires additional authority-chain proof.
Evidence reviewed 9 items Current main still needs both workflow corrections: Main reads the three Azure identifiers from secrets and sets directory depth without enabling recursion. The introduced delta changes those inputs without changing package identity, version calculation, authorization conditions, or metadata format.
Dependency contract confirms recursion is required: The workflow directly executes azure/artifact-signing-action@v2. Its action metadata defines recursive discovery as false by default and describes depth as a limit on recursive searching, supporting the added recurse input.
Maintainer resolved external configuration uncertainty: The September 12 comment confirms environment variables, main-only deployment policy, the dedicated Entra identity, immutable OIDC subject, absence of password/key credentials, and certificate-profile-scoped signing permission: #24 (comment). It schedules fresh-install verification before announcing the first official release.
Findings 1 actionable finding [P3] Remove the source-marker test from required validation
Security None None.

How this fits together

The packaging workflow builds Windows packages for x64 and ARM64, validates their provenance, and sends approved packages to Azure Artifact Signing. Signed packages are verified before being uploaded as workflow artifacts.

flowchart LR
  A[Windows package builds] --> B[Unsigned architecture packages]
  B --> C[Main branch and package authorization]
  C --> D[Protected signing environment]
  D --> E[Azure authentication and recursive signing]
  E --> F[Signature verification]
  F --> G[Signed workflow artifacts]
Loading

Before merge

  • Remove the source-marker test from required validation (P3) - This searches the entire workflow for text rather than validating signing behavior: commented-out settings or settings in another job still satisfy the check. CONTRIBUTING.md:190–192 explicitly disallows source-marker assertions. Remove this script and its CI invocation, or replace it with observable-behavior coverage. This is a late finding: the script is unchanged from the previously reviewed head, and I missed the policy conflict then.
  • Complete next step (P2) - Remove the source-marker test and its CI invocation, or replace them with observable-behavior coverage consistent with CONTRIBUTING.md.

Findings

  • [P3] Remove the source-marker test from required validation — scripts/Test-WorkflowSigningConfiguration.ps1:27-30
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Change footprint Workflow +9/-3, tests +37, documentation +29 The production change is limited to signing configuration and one validation step; no application runtime code grows.

Technical review

Best possible solution:

Keep the focused signing corrections and existing authorization checks, with validation that follows the repository’s observable-behavior testing policy.

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

Yes, from source: main places packages in architecture subdirectories but omits the Azure action’s required recursion switch. No official signing run was executed during this read-only review.

Is this the best way to solve the issue?

The workflow corrections are appropriate and narrow; the added source-marker test should be removed or replaced because it contradicts CONTRIBUTING.md and cannot establish the claimed signing contract.

Full review comments:

  • [P3] Remove the source-marker test from required validation — scripts/Test-WorkflowSigningConfiguration.ps1:27-30
    This searches the entire workflow for text rather than validating signing behavior: commented-out settings or settings in another job still satisfy the check. CONTRIBUTING.md:190–192 explicitly disallows source-marker assertions. Remove this script and its CI invocation, or replace it with observable-behavior coverage. This is a late finding: the script is unchanged from the previously reviewed head, and I missed the policy conflict then.
    Confidence: 0.96
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is correct
Overall confidence: 0.9

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 6f89905a5349.

Labels

Label justifications:

  • P2: This is a bounded correction to official Windows package signing with confirmed external setup.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The MEMBER-authored PR is exempt from ordinary contributor runtime proof. The maintainer confirmed external configuration; official Azure signing was skipped in PR checks and first-install validation remains scheduled before release announcement. No material expansion of authorization was found that requires additional authority-chain proof.

Evidence

Acceptance criteria:

  • [P1] git diff --check.
  • [P1] pwsh -NoProfile -File ./scripts/Test-SigningInputs.Tests.ps1.
  • [P1] pwsh -NoProfile -File ./scripts/Test-WorkflowPackageVersion.Tests.ps1.

What I checked:

  • Current main still needs both workflow corrections: Main reads the three Azure identifiers from secrets and sets directory depth without enabling recursion. The introduced delta changes those inputs without changing package identity, version calculation, authorization conditions, or metadata format. (.github/workflows/gateway-msix.yml:404, 6f89905a5349)
  • Dependency contract confirms recursion is required: The workflow directly executes azure/artifact-signing-action@v2. Its action metadata defines recursive discovery as false by default and describes depth as a limit on recursive searching, supporting the added recurse input. (action.yml:57, c7ab2a863ab5)
  • Maintainer resolved external configuration uncertainty: The September 12 comment confirms environment variables, main-only deployment policy, the dedicated Entra identity, immutable OIDC subject, absence of password/key credentials, and certificate-profile-scoped signing permission: chore(ci): finalize Artifact Signing setup #24 (comment). It schedules fresh-install verification before announcing the first official release. (c4f56ba483bd)
  • Authorization boundary remains intact: Official signing still requires successful main-only authorization before the protected environment requests Azure credentials. The validator checks both architectures, approved commits, package hashes, manifests, and embedded inventories; uploads contain explicitly named MSIX and metadata files. (scripts/Test-SigningInputs.ps1:126, c4f56ba483bd)
  • Repository test guidance: CONTRIBUTING.md requires observable-behavior assertions and explicitly prohibits reading source files to assert implementation string markers. The new test does precisely that. (CONTRIBUTING.md:190, c4f56ba483bd)
  • Re-review continuity: The previous completed review recorded this exact head with no findings. Comparing that revision to HEAD produces no changes to the configuration test, so the test-policy finding is a late discovery. (scripts/Test-WorkflowSigningConfiguration.ps1:27, c4f56ba483bd)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)
  • Linus Huang: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Remove the source-marker test and its workflow invocation, retaining the existing behavioral signing-policy tests.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (5 earlier review cycles)
  • reviewed 2026-09-11T07:50:27.187Z sha 8311723 :: blocked before merge. :: none
  • reviewed 2026-09-12T01:04:40.484Z sha 421f38d :: blocked before merge. :: none
  • reviewed 2026-09-12T01:13:37.559Z sha 442def2 :: blocked before merge. :: none
  • reviewed 2026-09-12T01:40:34.205Z sha 442def2 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-12T01:48:21.932Z sha c4f56ba :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Sep 12, 2026
@RomneyDa

Copy link
Copy Markdown
Member Author

External signing setup is confirmed on the current PR head:

  • GitHub environment release-signing contains AZURE_CLIENT_ID, AZURE_TENANT_ID, and AZURE_SUBSCRIPTION_ID as environment variables, with no environment secrets.
  • Its only deployment branch policy is main.
  • Entra application and service principal: openclaw-windows-msix-signing.
  • It has the exact immutable repository/environment OIDC subject and no password or key credentials.
  • Its only Azure role assignment is Artifact Signing Certificate Profile Signer on the existing OpenClaw certificate profile.

There are no existing GitHub Releases or previously official-signed MSIX packages in this repository, so an official-to-official upgrade test is not possible before the first release. The package identity and publisher are unchanged; the first official package moves from workflow-derived 0.1.x.y versions to the greater release-derived 2026.x.y.z range. Fresh-install verification will be performed on the first official main run before announcing the release.

@RomneyDa

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@clawsweeper clawsweeper Bot removed merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Sep 12, 2026
@RomneyDa
RomneyDa force-pushed the chore/document-artifact-signing-setup branch from 442def2 to c4f56ba Compare September 12, 2026 01:44
@RomneyDa

Copy link
Copy Markdown
Member Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@RomneyDa
RomneyDa merged commit cc1d833 into main Sep 12, 2026
14 checks passed
@RomneyDa
RomneyDa deleted the chore/document-artifact-signing-setup branch September 12, 2026 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. 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