Azure cloud discovery onboarding: consent, Reader, and a certificate credential - #53
Open
amansingh39 wants to merge 22 commits into
Open
amansingh39 wants to merge 22 commits into
amansingh39 wants to merge 22 commits into
Conversation
Onboarding only -- consent an Entra tenant and prove whether that consent
bought ARM read access. No identity scan, no permission extraction, no access
graph; those cannot start until a tenant reaches arm_reader_ok.
THE FACT THE DESIGN FOLLOWS FROM. Consent and authorisation are separate grants
in Azure, made by different people at different times. Admin consent creates the
service principal in the customer's directory and grants Graph application
permissions; an Azure RBAC role assignment is what lets that principal read
anything. An application can be fully consented and still read nothing at all.
Recording consent as if it were access is the mistake the split exists to
prevent, which is why arm_reader_ok is a separate column that consent never
sets.
Layering follows the AWS connector: internal/azureonboard holds provider logic
with no database, no gin and no workspace concept, reaching Microsoft through an
interface so the service layer can be driven by a fake.
Notable decisions:
- One multi-tenant App Registration, never one per tenant. The per-tenant
service principal is created by consent, and its object id is read from the
oid claim of an app-only token rather than from Graph /servicePrincipals --
which keeps the promise that this flow never touches those endpoints.
- azure_oauth_state is a one-shot row redeemed with DELETE ... RETURNING, so
reading a state and spending it are the same statement. A literal state
string cannot be validated; the row is what makes a callback authorised, and
it is the only place the workspace and consented tenant are read from.
- arm_reader_ok requires ARM to return at least one subscription, not merely
to answer. ARM replies 200 with an empty list when the token is valid but no
role assignment exists anywhere, and that was observed reporting a tenant as
fully onboarded while the application could read nothing.
- The delegated token goes to the secrets store, never to a cookie and never
to a process map: a cookie would put a live ARM bearer token in a browser,
and a map would drop every sign-in on restart.
Verified against a real Entra tenant: sign-in, admin consent, app-only token
acquisition and the ARM probe all exercised end to end. Migration 015 applies
cleanly and 001_bootstrap.sql carries the same end state.
…ator's behalf
Six routes: login, callback, tenants, consent, validate-arm, connectors, plus
reader-setup and assign-reader.
WHY /api/azure AND NOT /authsec/discovery/azure. The callback path is fixed by
the redirect URI registered on the Entra application; Microsoft will not
redirect anywhere else. The rest of the flow is grouped with it so it reads as
one thing.
WHY TWO ROUTES ARE UNAUTHENTICATED. login and callback are top-level browser
navigations, and a redirect arriving from Microsoft carries no bearer token, so
AuthMiddleware would reject every callback. They are rate limited and authorised
by the one-shot state row instead -- the same reasoning the GitHub webhook route
already uses. The remaining routes are ordinary fetch calls and authenticate and
RBAC check exactly like the AWS discovery routes.
REMOVING THE MANUAL AZURE WORK. Assigning ARM Reader is the one step no
application can perform for itself; Azure offers no mechanism, deliberately.
What it can do is ask ARM on behalf of a human who already holds Owner or User
Access Administrator, which is exactly what that person would be doing through
the portal, minus the clicking. assign-reader does that with the operator's own
delegated token, and falls back to ready-made az / PowerShell / ARM template /
portal instructions when they lack the privilege -- nothing is left half done.
The assignment name is a deterministic UUIDv5 over (scope, principal, role) so a
retry addresses the same object rather than creating a duplicate;
RoleAssignmentExists counts as success because the grant is present either way;
principalType is set so ARM waits for directory replication instead of refusing
a freshly consented principal.
Two smaller pieces earn their place:
- POST /consent answers JSON when the client asks for it. A browser console
cannot use the 302: the endpoint needs a bearer token so it cannot be a
plain navigation, and fetch follows the redirect itself, sending an XHR to
login.microsoftonline.com where the absence of CORS headers fails the
request before the consent page is ever shown.
- AZURE_SIGNIN_TENANT pins sign-in to one tenant's authority. The default,
organizations, rejects personal Microsoft accounts, which is usually right
-- but an account that signed up for Azure with a personal address
administers its directory as an external guest and is turned away despite
genuinely administering a tenant.
Sovereign clouds are reachable through AZURE_AUTHORITY_HOST and
AZURE_ARM_ENDPOINT; both default to the public cloud.
Still open, and documented: Graph authorisation is never verified, so a tenant
whose application permissions were never declared is recorded as consented while
holding none. Subscriptions are listed and discarded rather than persisted, so
the tenant-to-subscription relationship a later access graph needs does not
exist yet.
Closes the two places onboarding claimed something it had never checked, plus
the audit gap and the cookie flag found alongside them.
GRAPH AUTHORISATION WAS NEVER VERIFIED. Admin consent wrote consented_at and the
flow moved on; nothing asked Microsoft whether a single permission had been
granted. Against a real tenant that produced a connector recorded as consented
whose app-only token carried no roles claim at all, with every Graph endpoint
answering Authorization_RequestDenied. The cause is that .default consent grants
only what the application declares, and an application declaring nothing is
consented successfully and holds nothing.
validate-graph is the counterpart to validate-arm, and reads the roles claim of
an app-only token rather than probing an endpoint. That works even when nothing
was granted, needs no permission of its own, costs no extra call, and names the
individual permissions that are missing -- where a probe only shows that one
endpoint happened to answer. Against the real tenant it now reports graph_ok
false, lists all four required permissions as missing, and explains that the
registration declares none and that the Type must be Application rather than
Delegated, which is the mistake that produces this state.
SUBSCRIPTIONS ARE NOW ROWS. The ARM probe listed them and threw them away,
keeping one boolean for the whole tenant, so Reader on two of five subscriptions
reported true -- an all-clear that was not earned, the same class of error as
the empty-list false positive fixed earlier. Reader is assigned per scope, so
coverage is a per-subscription fact and is now stored as one.
The composite foreign key to the connector is as much the point as the rows: a
subscription cannot exist without its tenant, so a subscription id can never be
read without the tenant context that says whose it is and which credential may
reach it. It is also what the eventual access graph attaches resources, managed
identities and role assignments to.
Two smaller fixes found while auditing:
- assign-reader is the only state-changing action this feature performs inside
another organisation's tenant and left no audit record. Both outcomes are
audited now; the refusal is the more interesting one.
- The session cookie took its Secure flag from Request.TLS, which is nil
behind a terminating proxy -- the normal deployment -- so a cookie
addressing a live delegated Azure token shipped without it. Now uses the
same X-Forwarded-Proto test middlewares/security.go already applies to HSTS.
Schema goes in 016 rather than amending 015, which has already been pushed;
001_bootstrap.sql carries the same end state. Verified end to end against a real
Entra tenant.
Consent settles whether a permission is granted. It does not settle whether
the data is reachable: Microsoft gates some of it on the tenant's LICENCE.
Found against the real tenant immediately after the four application
permissions were granted and validate-graph went green. Ten of the eleven
endpoints discovery needs answered 200. /auditLogs/signIns answered 403
Authentication_RequestFromNonPremiumTenantOrB2CTenant -- "tenant is not a B2C
tenant and doesn't have premium license" -- while holding AuditLog.Read.All.
Sign-in logs need Entra ID P1 or P2. directoryAudits, the other half of the same
permission, works on the free tier.
Discovery has to know this before it runs. Without it the sign-in-activity
reader fails on every free-tier tenant in a way that reads as a code bug and is
not, and "last used" becomes silently unknown with nothing recording why.
validate-graph now probes the capabilities consent alone cannot settle and
reports them separately from graph_ok, flagging the licence gate as such:
granted_roles : all four
graph_ok : true
capabilities : signin_activity available=false license_gated=true
"the tenant has no Entra ID P1/P2 licence, so sign-in logs
are withheld even though AuditLog.Read.All is granted"
Kept out of graph_ok deliberately. A licence gate is a customer purchasing
decision, not a missing consent and not an AuthSec fault, and collapsing the
two would send an operator to re-run consent that already succeeded.
Probed only when something was granted -- with nothing granted every capability
fails for the obvious reason and the probe says nothing useful.
GET /api/azure/config, so a setup screen can show a live checklist instead of an operator inferring configuration from a 500. Deliberately does NOT construct the onboarding service: that constructor fails when configuration is incomplete, which is precisely the state this endpoint exists to describe. It reads the environment directly. Secret VALUES are never returned -- only whether each one is set. The client id, redirect uri and endpoints are public by design, since they appear in every authorize URL, and echoing them lets a setup screen catch the single most common misconfiguration: a redirect uri that does not match the one registered on the Entra application, which fails with AADSTS50011 before a password is typed. What it cannot answer, and says so: whether the Graph application permissions are declared on the registration or granted in any tenant. Only validate-graph can, by reading the roles claim of a real app-only token.
…onboarding # Conflicts: # .env.example
GET /api/azure/app/check. Asserts, by machine, every field the runbook
currently asks a human to verify by eye in the portal.
WHY. One of those fields -- undeclared application permissions -- produced a
consent that completed successfully while granting nothing, and read as a code
bug for an hour before the token's roles claim gave it away. Admin consent uses
scope=.default, which grants only what the application DECLARES, so an
application declaring nothing is consented and holds nothing. That is checkable
in one call and now is.
Five assertions:
signInAudience is AzureADMultipleOrgs else no customer can ever consent
AZURE_REDIRECT_URI is registered else AADSTS50011 before a password
the four Graph permissions are declared as APPLICATION, compared by role id
because an application object stores
ids and never names -- a check on
names would have nothing to compare
a credential exists at all
the soonest credential expiry warns inside 30 days, errors past it
The expiry check earns its place on its own: this credential is global, so when
it lapses every consented tenant stops working at once, and nothing else in the
product would have said so beforehand. Against the live registration it reports
2027-03-05.
NEEDS NO NEW PERMISSION. Application.Read.All is already required for identity
discovery, and reading one application is within it. Deliberately no write
access: a check that can only report and never repair cannot itself become a way
to repoint the product at a different application.
This is also what makes two setup paths coherent. Whether the registration was
created by hand in the portal or by an automated bootstrap, this is the single
assertion that says the end state is correct -- so the manual and automated
routes converge on one verification rather than each needing its own.
Reading /applications is scoped to our own app by appId filter. The promise this
package keeps is not "never call this endpoint" but "never enumerate a
customer's directory objects with it".
Assigning Reader one subscription at a time is the step that makes Azure
onboarding feel manual, and it goes stale on its own: a subscription created
next month is not covered by an assignment made today.
One assignment at the tenant root management group covers all of them,
including ones that do not exist yet. The reason onboarding could not simply
do that is privilege -- writing there needs User Access Administrator at the
root, which nobody holds by default, because Entra roles and Azure RBAC are
separate systems and administering a directory grants nothing over its
resources. The runbook's answer was a portal toggle, a sign-out, a sign-in,
and a hunt through the IAM blade.
Microsoft exposes the toggle as an ARM call, and this flow already holds the
delegated token it needs. So POST /api/azure/assign-reader takes tenantWide,
and the ordering is the whole safety argument:
1. Try with the privilege the operator already has. Anyone who has done
this before is never elevated at all.
2. On refusal, look for an EXISTING root elevation. One that is already
there reflects somebody's earlier decision, and removing it afterwards
would revoke standing access AuthSec never granted -- so report the
failure rather than touch it.
3. Otherwise elevate, retry past RBAC propagation, and give it back.
Removal runs on a context of its own, deliberately not derived from the
request: a cancelled browser or an expired deadline must not be the reason a
human is left holding root User Access Administrator, which does not expire.
When removal does fail, the response and the audit row say so and name the
manual fix.
Elevation is reported in full on success too -- attempted, elevated, removed.
An operator should not have to read an audit log to learn that AuthSec briefly
held the widest role in their tenant.
Tests assert the call SEQUENCE, not just the outcome, because a run that
elevated when it should not have looks identical in the result to one that
did the right thing. First tests for this feature; the fake exercises the
WithClient seam that has been unused since it was added.
Also corrects four claims in the flow doc that the code stopped honouring:
Reader is no longer assigned by hand, Graph is no longer called zero times,
GET /applications is called once for our own app object, and
RoleManagement.Read.Directory was missing from the permission table.
Neither showed up in unit tests or in a real end-to-end run against Azure.
Both needed the flow driven against a fake Microsoft that models
authorization and starts from an empty database.
FIRST CONSENT FOR ANY TENANT DIED ON A NOT-NULL CONSTRAINT.
azure_connectors.graph_granted_roles is text[] NOT NULL DEFAULT '{}', but a
nil pq.StringArray is not a zero value GORM omits unless the field declares a
default -- so every INSERT sent an explicit NULL. It survived a real
end-to-end run because that tenant's row already existed and the upsert path
only touches the display columns. Against an empty table, no tenant could be
onboarded at all.
ELEVATION REPORTED removed:true WHILE STILL ASSIGNED.
removeElevation re-derived the assignment id from a principal filter at
cleanup time. When that filter matched nothing the code read it as "already
gone" and set Removed:true -- while root User Access Administrator was still
held. The simulation printed the response saying removed:true next to the
role assignment that was still there.
The id is now captured immediately after elevateAccess returns, while we know
for certain a grant exists, and cleanup deletes that known object. A lookup
miss is no longer success: it says the role may still be assigned and names
the manual fix. Removed:true now means exactly one thing -- a DELETE was
issued and ARM accepted it.
That is the same false-success shape as arm_reader_ok going true on an empty
subscription list. A check that cannot fail is not a check.
Verified end to end in three scenarios. Operator is a Global Administrator:
PUT refused, no existing elevation found, elevate, capture, retry, succeed,
DELETE -- and the tenant is left holding Reader and nothing else. Operator
already elevated: the first PUT succeeds, and no elevate or delete is
attempted at all, so an elevation AuthSec did not create is never touched.
Operator is not a Global Administrator: elevateAccess 403s, the response
carries the az/PowerShell/ARM-template/portal fallback, and nothing is left
behind.
Without prompt=select_account, Microsoft silently reuses whatever account the browser is already signed in with. When that happens to be a personal account -- outlook.com, live.com, an Xbox login -- the operator gets "You can't sign in here with a personal account" and no way to choose a different one, because nothing ever asked them which account to use. The refusal itself is correct and not something the app registration should accommodate. A personal Microsoft account has no Entra directory behind it: no tenant, no directory objects, nothing for GET /tenants to return. Widening signInAudience to AzureADandPersonalMicrosoftAccount would let the sign-in succeed and then make every call afterwards come back empty, which is worse than a clear error. So the fix is not to accept more accounts, it is to let the operator pick. Only on the ordinary sign-in path. prompt=admin_consent already forces its own account selection and is mutually exclusive with this.
… spot Onboarding had no first step. AZURE_CLIENT_ID, AZURE_CLIENT_SECRET and AZURE_REDIRECT_URI were read once at process start and were deployment-global, so the only way to hand the product an App Registration was to edit .env and restart. That is not a setup flow, it makes two workspaces on one deployment share one application, and it rules out a customer bringing their own. POST /api/azure/config takes the four details from the portal, stores them, and in the SAME call asks Microsoft whether that application actually has what onboarding needs. Missing permissions come back named, with the exact portal path and the warning that they must be APPLICATION permissions rather than delegated -- the mistake that produces a consent which succeeds and grants nothing. It answers 200 even when the application is wrong, because "saved, and here is what to fix" is a different outcome from "rejected" and the operator needs the list, not an error. The secret goes to Vault. Postgres gets a path, and AuthRef carries json:"-" so it cannot leak through a handler that serialises the row wholesale. No endpoint returns it. Resolution order matches connector_provider_apps, deliberately: this workspace's row first, else the deployment env vars. A deployment configuring neither is unchanged. Three consequences worth naming, because they are easy to get wrong: The constructor no longer fails when the environment is empty. It could not: refusing to construct would make the endpoint that FIXES the missing configuration unreachable. Ready() answers that question instead, after the workspace is known. Every handler is now workspace-bound through serviceFor(). ForWorkspace returns a COPY -- the service is shared across requests, so mutating it would let one workspace's credentials serve another's. The callback binds before it redeems. Microsoft exchanges an authorization code only for the application that issued it, so a callback authenticating as a different workspace's application fails with an opaque invalid_client. Which application that is lives in the state row, hence PeekStateWorkspace -- a read that grants nothing, with ConsumeState still redeeming exactly once. Also here: the sign-in authority becomes per-request rather than an env var. /organizations has to resolve a typed address across every directory AND the consumer namespace, and when the same address exists in both it can land on the personal one -- "You can't sign in here with a personal account", with a picker that keeps offering the same wrong identity. Naming the directory removes the ambiguity. Per-request because pinning it in the environment would make the product single-tenant, which is the opposite of the point. And app/check now accepts AzureADandPersonalMicrosoftAccount as well as AzureADMultipleOrgs -- both let a customer tenant consent -- reporting the first as a warning, because Microsoft caps that audience at two client secrets and refuses it in the national clouds.
Nine call sites reach Microsoft and any of them can be answered 429. The retry lives in the transport so no call site has to remember it. Retries 429, 502, 503 and 504. Not 500: the write may have applied, and re-sending it is worse than reporting the failure. Not 4xx: re-sending a rejected request does not make it valid. Client.Timeout stays unset. It would bound the whole request including retries and cannot vary per call; the caller's context already carries the bound the caller wants. Request bodies are re-read through GetBody rather than reused. Reusing the reader panicked on the first retried POST, caught by a test before it shipped.
A client secret travels to Microsoft on every token request and has to reach the deployment through a clipboard, a browser and a request body. A certificate signs a short-lived assertion instead, and the private key never moves. AuthSec generates the key pair. There is no field for submitting one: an operator pasting a private key sends it down exactly the path this exists to avoid. If a customer's policy ever requires the key be born in their own HSM, that case gets its own handling then. The assertion carries x5t in base64url, as the protocol requires. The thumbprint reported to an operator is uppercase hex, which is what the portal lists and what AADSTS700027 quotes. A test asserts the two agree. Also validates the ARM assignment scope before it is concatenated onto ARMBase. "https://management.azure.com" + "@10.0.0.7" parses with host 10.0.0.7 and the real host demoted to userinfo, which sent a PUT carrying the operator's bearer token to any address the deployment could route to. AssignRole re-checks the parsed host before sending.
prompt=admin_consent is a v1.0 parameter. The v2.0 endpoint rejects it with AADSTS901001 and the sign-in fails before Microsoft shows anything. prompt=consent is not a substitute: it consents the scopes in that request, and the request asks for ARM's delegated scope, not Graph's application permissions. One /authorize call is one resource. So: sign in first, then send the administrator to /v2.0/adminconsent for the tenant the returned token names. The state carries the mode across both legs so the callback knows which one answered. The ARM scopes also stop hardcoding management.azure.com. AuthorityBase, ARMBase and GraphBase are env-configurable for sovereign clouds and .env.example says to set all three, but the scope constants named the public cloud anyway -- so a configured Gov or China deployment asked its own authority for a public-cloud resource.
"azure returned 403 (Authorization_RequestDenied): Insufficient privileges to
complete the operation" is true, useless, and easy to read as "the credential
is wrong" -- which sends whoever is debugging to regenerate a certificate that
was working. The two failures are fixed in different blades of the portal:
401 invalid_client wrong credential Certificates & secrets
403 Authorization_Request right credential, API permissions
nothing granted to it
Diagnose names nine of them, each with a title, the fix and whose problem it
is. The permission list comes from RequiredGraphRoleIDs rather than the
message text, so it cannot drift from what the check actually requires.
An unrecognised error returns nil. The raw message always stays; guessing
means telling someone confidently to change the wrong thing.
Admin consent with .default replaces a tenant's existing grants with exactly what the registration declares. The application never declared ARM's user_impersonation, so consenting deleted the incremental grant that had been working, and the next Reader assignment failed with AADSTS65001 on its first redemption. Verified against the tenant's own oauth2PermissionGrants. The missing ARM scope is now reported separately from the Graph ones. Merged, user_impersonation rendered under "Microsoft Graph -> Application permissions", where it does not exist and cannot be found. Refresh tokens rotate: Entra returns a new one on every redemption and supersedes the presented one, so a chain that redeems N+1 times invalidates its own earlier tokens -- five subscriptions, one granted, four refused. The auto-setup path now makes one AssignReader call and keeps the rotated token. Two ways a privilege could be left live: An ElevateAccess error is not the same as a failed elevation. A timeout, a reset or a 5xx after the write leaves the operator holding root User Access Administrator, and this returned before the removal defer existed. It now looks, gives it back, and says so. ErrNotGlobalAdmin is exempt: it provably applied nothing. A workspace that submitted its own application must act as that application or none. The bind-failure path kept the deployment's clientID, redirectURI and Microsoft client, and Ready() only mentioned the problem when something else was also missing -- so with AZURE_* set, a customer's administrator could be sent to grant admin consent to the wrong application in their own directory, recorded as though it were right.
Consent, find the tenant, assign Reader across it, verify both planes. The administrator signs in once and the rest runs behind a status endpoint. Not a replacement for the step-by-step routes. This path assumes one person holds both privileges the flow needs -- Global Administrator to consent, Owner or User Access Administrator to assign Reader -- in the tenant they are signing in to. When those are different people, or the operator is onboarding a tenant they do not administer, the separate endpoints are the only thing that works. One AssignReader call for the whole tenant, not one per subscription: each redemption rotates the refresh token and supersedes the last, so a loop invalidates its own credential partway through. Tenant-wide grants settle asynchronously, so ARM is validated twice with a wait between. A single check right after the assignment reports failure on a grant that is about to work.
Every save demanded a credential, and the only certificate this accepts is one it generates. So correcting a redirect URI, or re-running the check after granting consent, minted a new key pair and overwrote the stored one. The certificate already uploaded stopped matching, and the failure was AADSTS700027 quoting a thumbprint the operator had never seen. Saving again to fix it produced a third. Three certificates were generated this way before the cause was found, and none of it was the operator's doing. keepCredential saves everything else and leaves the credential alone. A credential is required only when there is not one already. The certificate is also handed back on request. Offering it once, in the moment it was made, meant closing the page had no way back except generating again -- the same trap from the other side. It is the public half: it verifies a signature and cannot make one. Generating no longer runs the Microsoft check. A certificate created a second ago cannot be on the registration, so the check could only fail with the exact error the upload is about to fix, reported next to the new key pair. Also: the thumbprint is matched against Graph's hex as well as the documented base64 (Graph returns hex, and an uploaded certificate read as missing), and the expiry reported is the credential in use rather than whichever one the blob happened to hold.
Every Azure refusal now comes back with a problem object naming the failure, the fix and whose problem it is, beside the raw message rather than instead of it. An unclassified error used to be 400 -- a Postgres outage or an unreachable Vault reported to the caller as a bad request, sending whoever is on call to look at the client. Deployment failures answer 500 with fault "authsec": 4xx means fix the request, 5xx means the deployment is broken and retrying is correct. /login and /callback are unauthenticated by necessity, and the configuration errors name the vault path and carry the secrets-store error verbatim. That detail makes a broken workspace debuggable from the console and does not belong in a response anyone on the network can ask for. It goes to the log; the caller gets the fact. ConfigStatus no longer discards the error from reading its own row, which made an unreachable database indistinguishable from a workspace that never configured anything -- answered with a checklist of environment variables to go and set. Two response keys renamed for shape: an error body's "problem" is a Diagnosis object, so the config endpoint's string is credential_problem. One key holding two shapes is how a client ends up rendering [object Object]. Adds POST /auto-setup, GET /setup-status and DELETE /config.
A database built from the bootstrap had no azure_app_config table, so a new deployment could not store its own application and POST /api/azure/config -- the first step of onboarding -- failed on a fresh install while working everywhere it had been migrated. The incremental migration existed and the bootstrap had never been updated to match. Verified by running the bootstrap against an empty database. Additive: 38 lines, IF NOT EXISTS, no deletions. .env.example also drifted in three ways. AZURE_SIGNIN_TENANT documented a default of /organizations where the code reads "common", AZURE_GRAPH_ENDPOINT was missing from a block that tells sovereign deployments to set all three endpoints together, and AZURE_ALLOW_ROOT_ELEVATION was undocumented despite gating a privilege raise.
The block still said the registration is created by hand in the AuthSec home tenant and marked multi-tenant. That was true before POST /api/azure/config existed. AuthSec is self-hosted, so one baked-in application would mean every customer trusting a registration they do not control. The application is submitted through the API and stored per workspace: the public fields in azure_app_config, the credential in Vault. The three AZURE_* variables are a fallback for a single-tenant deployment that would rather bake one in, and are commented out now because that is what they are. They can also only carry a client secret, where the API path can generate a certificate whose private half never leaves the secrets store.
SESSION_SECRET was a variable this feature introduced, that nothing else in the codebase reads, and that an operator was only told about at the end of an otherwise complete setup -- ConfigStatus listed it under missing, so a tenant could be fully onboarded and still stop on a secret nobody had asked for. The cookie key is now HMAC(JWT_SECRET, "authsec/azure-session-cookie/v1"), which is independent of the JWT key rather than the same bytes doing two jobs: a signature valid for one is meaningless to the other, so the cookie key cannot mint a JWT and the JWT key cannot forge a cookie. SESSION_SECRET still wins when set and 32+ characters, for deployments that already configured one and for anyone who wants the two blast radii separate. Shorter than that it is ignored in favour of the derivation. With neither there is no key at all, and the caller refuses to issue a cookie rather than signing one anybody could forge. Verified with the backend running and SESSION_SECRET unset: ready, nothing missing.
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.
Changes
Azure cloud-discovery onboarding: everything needed to get a customer's Entra tenant
to the point where AuthSec holds a working application-only token and both Azure
authorization planes have been verified. 18 endpoints under
/api/azure, 40 files,13,760 insertions, no deletions.
AuthSec is self-hosted, so the customer creates the app registration in their own tenant
and submits it through the API. Nothing here assumes AuthSec runs on Azure, and nothing
assumes one global application.
The idea the whole feature rests on
Azure has two separate authorization systems, and onboarding is complete only when both
are satisfied:
Consent is not access. A tenant can grant every Graph permission and the application
still sees zero subscriptions. Most of the complexity below is keeping these two apart in
the code and, more importantly, in what an operator is told.
What is in the PR
Per-workspace application (
services/azure_app_config.go,models/,repository/,migrations/017) — a workspace submits its own client id, tenant and credential viaPOST /api/azure/config; the public fields land inazure_app_config, the credential inVault.
AZURE_CLIENT_ID/AZURE_CLIENT_SECRET/AZURE_REDIRECT_URIremain as anoptional fallback for single-tenant deployments.
Certificate credentials (
internal/azureonboard/credential.go) — AuthSec generatesthe RSA key pair, keeps the private half in Vault and hands back only the certificate to
upload. A client secret travels to Microsoft on every token request; a certificate signs a
short-lived assertion and the key never moves. There is deliberately no field for
submitting your own key: pasting one sends it down exactly the path the certificate
exists to avoid.
Two-leg consent chain (
internal/azureonboard/oauth.go) — sign in, then send theadministrator to
/v2.0/adminconsentfor the tenant the token names.prompt=admin_consentis a v1.0 parameter and v2.0 rejects it withAADSTS901001;prompt=consentis not a substitute, because one/authorizecall consents one resourceand this one asks for ARM.
Reader assignment (
services/azure_onboarding.go) — per subscription, or once at thetenant root management group so subscriptions created later are covered. Sent as the
signed-in operator, so it succeeds exactly when that person could have done it by hand;
when it cannot, the response carries ready-made
az/ PowerShell / ARM-template / portalfallbacks rather than leaving the flow half-done. Root scope can require briefly raising
the operator's own privilege — opt-in behind
AZURE_ALLOW_ROOT_ELEVATION, and theprivilege is always given back.
One-sign-in setup (
services/azure_auto_setup.go) — consent, find the tenant, assignReader, verify both planes, behind a status endpoint. Assumes ONE person holds both
privileges, which is the on-prem case; the step-by-step endpoints remain for when they are
different people.
Retry transport (
internal/azureonboard/retry.go) — 429/502/503/504 in one placerather than at nine call sites. Not 500: a write may have applied.
Named failures (
internal/azureonboard/diagnose.go) —Diagnoseturns Microsoft'srefusal into a title, a fix and whose problem it is, beside the raw message rather than
instead of it. Two failures look almost identical and are fixed in different blades of the
portal:
An unrecognised error returns
nil. Guessing means telling someone confidently to changethe wrong thing.
No new deployment secret — the sign-in cookie key is derived as
HMAC(JWT_SECRET, "authsec/azure-session-cookie/v1"). An earlier revision introducedSESSION_SECRET, which nothing else in the codebase read and which an operator was onlytold about at the very end of an otherwise complete setup. It remains an optional override
for anyone who wants the two blast radii separate.
Docs —
docs/flows/azure-onboarding.md.Bugs found and fixed in review
An audit of this code (8 dimensions, adversarially verified) produced 55 confirmed
findings. The nine that were security- or correctness-critical are fixed here:
ARMBase + scopewas concatenated with novalidation, and
"https://management.azure.com" + "@10.0.0.7"parses with host10.0.0.7and the real ARM host demoted to userinfo — so a caller could send a PUT withthe operator's bearer token to any address the deployment can route to. Now validated at
the service boundary, and
AssignRolere-checks the parsed host before sending.service copy that still held the deployment's
clientID,redirectURIand Microsoftclient, and
Ready()only surfaced the problem when something else was also missing.With
AZURE_*set,Ready()returnednil, and a customer's administrator could besent to grant admin consent to a different application in their own directory,
recorded as correct.
ElevateAccessfailure(timeout, reset, 5xx after the write) returned before the removal defer was registered,
leaving root User Access Administrator live while the status said
elevated: false./loginand/callbackreturned the Vaultpath and the raw secrets-store error. The detail now goes to the log; the caller gets
the fact.
request. Deployment failures now answer 500 with
fault: "authsec".certificate this accepts is one it generates, so correcting a redirect URI minted a new
key pair and orphaned the certificate already uploaded —
AADSTS700027quoting athumbprint nobody had seen.
keepCredentialfixes it.the value shown after generating matched nothing in the portal. Both are hex now.
ARMBaseis env-configurable but the ARM scopeshardcoded
management.azure.com, so a Gov/China deployment asked its own authority fora public-cloud resource.
The remaining 46 findings are documentation drift, dead code and test-coverage gaps. None
is a security issue. The largest is that
AssignRole— the only write this codebase makesinto a customer's Azure — has no test coverage; worth a follow-up PR.
Testing
go test -short ./tests/unit/)go vet ./...cleanrun-integrationto this PR to trigger integration tests in CI (requires a live DB)17 test files, 123 test functions, 4,682 lines. All green: 45 in
internal/azureonboard, 64 inservices/azure_*, 7 incontrollers/platform.These caught real defects rather than confirming intent: a panic on the first retried POST
(nil
GetBody), a bare subscription GUID reaching ARM as a scope, and — during thisreview — a bug in the new scope validator itself, which trimmed whitespace before matching
and so accepted a trailing newline.
Verified against a real Azure tenant
Not a simulator. The full flow was run end to end against a live tenant with the
certificate credential, and the discovery surface was then probed directly:
29 of 30 reads succeed. The one failure is
sign-in logs -> 403 Authentication_RequestFromNonPremiumTenantOrB2CTenant—AuditLog.Read.Allis in thetoken, but the endpoint requires an Entra ID P1/P2 licence. A tenant licensing limit,
not a permission gap, and reported as such rather than as a missing consent.
Tenant-root Reader was verified to cover all five subscriptions, and the Graph managed
identities join correctly to their ARM role assignments (13 assignments held by managed
identities) — the join the product exists to surface.
Also verified live with
SESSION_SECRETunset and noAZURE_*variables at all: theworkspace reports
ready: true,missing: [], running entirely off the database andVault.
Not in this PR
Azure discovery does not exist yet. This PR is onboarding only: it gets a working
token and proves both planes are satisfied. Nothing reads identities or resources out of
Azure into
cloud_identity/cloud_resource, and nothing writes acloud_connectorrowwith provider
azure— an onboarded tenant lands only inazure_connectors, confirmedagainst the live database. The probe above shows the data is all reachable; the reader is
the next PR.
Checklist
value, in the column, the response and the logs.
CurrentCertificatePEMreturnsonly
CERTIFICATEblocks and a test asserts the private key can never come back.docs/flows/azure-onboarding.md, and.env.examplecorrected: the Azure block described the pre-API model (oneapplication baked into the deployment),
AZURE_SIGNIN_TENANTdocumented a defaultof
/organizationswhere the code readscommon,AZURE_GRAPH_ENDPOINTwasmissing, and
AZURE_ALLOW_ROOT_ELEVATIONwas undocumented despite gating aprivilege raise.
Migration note
migrations/master/001_bootstrap.sqlgainsazure_app_config(38 lines, additive,IF NOT EXISTS, zero deletions). It existed in017_azure_app_config.sqlbut had neverbeen mirrored into the bootstrap, so a database built from the bootstrap had no table and
POST /api/azure/config— the first step of onboarding — failed on a fresh install whileworking everywhere it had been migrated. Verified by running the bootstrap against an
empty database.
Reviewer notes
Worth a careful look, in order:
internal/azureonboard/credential.go— the client assertion and what is deliberatelynot offered
services/azure_onboarding.go,assignReaderTenantWide— the elevation is raised andreturned; every exit path must give it back
internal/azureonboard/oauth.go,ValidateARMScope— the SSRF fixservices/azure_app_config.go,ForWorkspace— the wrong-application fixFacts established live against a real tenant, in case they save someone the same
afternoon: admin consent with
.defaultreplaces a tenant's existing delegated grantsrather than adding to them; Entra rotates the refresh token on every redemption, so a loop
that redeems N+1 times invalidates its own earlier tokens; Graph returns an X509
credential's
customKeyIdentifieras uppercase hex, not the base64 the documentationclaims; and
Policy.Read.ConditionalAccessdoes not work for/identity/conditionalAccess/policies— it needsPolicy.Read.All.