Repository navigation
hosting-signin-app: keep, verify and add the sign-in app's client secret — never rotate, never show (Plugins#1719) - #4109
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical vault handling and CI tracking issues remain, along with credential-ordering and invalid recovery-command problems.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an AKS operator command to preserve, verify, and append Entra sign-in app secrets without rotating existing credentials.
Changes:
- Adds
hosting-signin-applifecycle logic. - Adds Azure CLI stubs and behavioral tests.
- Registers the command in CI and lifecycle documentation.
File summaries
| File | Summary |
|---|---|
deploy/aks/operator/test/stubs/signin-app/az |
Azure CLI stub for sign-in app and secret scenarios. |
deploy/aks/operator/test/run-tests.sh |
Behavioral and security test coverage. |
deploy/aks/operator/bin/hosting-signin-app |
Secret handling and redirect verification; unresolved vault error handling, mutation ordering, and invalid recovery commands remain. |
deploy/aks/INSTANCE-LIFECYCLE.md |
Documents the operator lifecycle step. |
.github/workflows/hosting-operator.yml |
CI registration and tracking guard; the tracked-file floor and diagnostic count are incorrect. |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ret — never rotate, never show (MeshWeaver.Plugins#1719)
Runbook step 1 registered the multi-tenant Entra app, minted its secret and wrote
<prefix>Authentication-Microsoft-ClientSecret by hand. What this step automates and
what it cannot is decided by the identity's Graph rights and stated:
present object → KEPT (never minted over, never rotated). With --client-id the
app is verified to list https://<host>/signin-microsoft; an
identity without Application.Read.All reports
signin_app_verify=unknown with the hand command (not red); a
readable app missing the URI is RED naming az ad app update.
absent + app id → a credential is ADDED (az ad app credential reset --append:
existing credentials stay valid), stderr discarded (az echoes
the secret there), stored through a mode-600 --file, read back;
signin_secret=created + expiry. Needs Application.ReadWrite.OwnedBy
on an app the identity owns — else RED with the exact commands.
absent, no id → RED: registering the app needs the Graph right AND the client id
written back to the git-owned record; the runbook's commands are
named. Not automated, deliberately.
Tests: 34 cases against an az stub that, like the real az, prints the minted secret
on stderr — proving it is discarded. Workflow: plan list, floor 24, stub list.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cfd73e6 to
d6d4ef8
Compare
|
Corrected the tracked-file floor in 🤖 Generated with Claude Code |
#4100 (hosting-kv-ensure --db-connection) landed first. Both conflicts were "both sides added": the INSTANCE-LIFECYCLE guarantee list keeps main's extended kv-ensure bullet AND the signin-app one, and the behaviour suite keeps both blocks (352 passed, 0 failed = 325 + main's 27). The workflow's tracked-stub list is the union. The tracked-file floor is CORRECTED rather than carried: main declared 23 while already tracking 24 (its comment said "20 commands" — stale, so one lost script would have passed). With hosting-signin-app the real count is 25, and the floor and its comment now say 25. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Merge resolved by content; floor settled at 25 — measured, not assumed.
The floor, and how it was counted. Guards run on the pushed head: The follow-up is still needed and is now precisely scoped. Each of #4103/#4105/#4109/#4110 sets the floor true for its own merge, so as the siblings land the real count rises 25 → 26 → 27 → 28 while the floor stays 25 — up to 3 files of slack. After all four have merged, a one-line PR sets 🤖 Generated with Claude Code |
Operator half of MeshWeaver.Plugins#1719 (the plan half — an "Ensure sign-in app secret" step after the vault step — is the paired Plugins PR;
Pairs-with: none — this PR removes no public surface).The hand step it replaces (partly — read on)
Memex
docs/new-deployment.mdstep 1, "The sign-in app →<prefix>Authentication-Microsoft-ClientSecret":What is automated, what is verification-only, and why
New command
hosting-signin-app --name <id> --host <host> --vault … --object … [--client-id <app id>] [--years 1]:--client-id, the app is verified to listhttps://<host>/signin-microsoft— a readable app missing it is RED namingaz ad app update; an identity that cannot read the app reportssignin_app_verify=unknownwith the hand command and does not fail the provisionApplication.Read.Allfor the verify — not granted tohosting-operatortoday, so on the fleet this reportsunknownuntil it isaz ad app credential reset --append— existing credentials stay valid; this adds, it never rotates), az's stderr echo of the secret discarded, the value stored through a mode-600--file, read back,signin_secret=created+ expiryApplication.ReadWrite.OwnedByon an app the identity owns (orApplication.ReadWrite.All) — not granted; without it the step is RED with the exact hand commandssignIn.microsoftClientIdSo today, on the fleet, the step is the verification half for the two absent cases (RED with the manual command) and a no-op keep for the present case. Granting
hosting-operatorApplication.ReadWrite.OwnedBy+ making it an owner of each instance's app turns the middle row on with no code change.Gates run
hosting-operator.yml: plan list, tracked-file floor 23 → 24 (will needmainmerged and the floor re-counted once the sibling PRs land), stub list.Ships how
Hop (C):
hosting-operator:<sha>on merge tomain; reaches a Provision whenoperator.imageonDeployments/memexmoves. Merging is not shipping.🤖 Generated with Claude Code