flightcheck: add INFRA-011 connector secret storage check (verify sec…

[daeunJe0ng]

Summary

Adds a new FlightCheck pre-deployment check, INFRA-011 (connector secret storage safety), to the ESS Maker Kit. It verifies connector credentials are stored safely (in a Key Vault-backed Power Platform environment variable) rather than pasted inline as plaintext, and surfaces connection-stored secrets the platform never exposes for manual verification.

The check is a detect-first gate over Dataverse environment variables and connection references, with these outcomes:

• A. No secret-bearing auth present -> informational PASSED.
• B1. Secret-type (Key Vault-backed) env var, populated -> PASSED for storage, plus a MANUAL vault-hardening directive.
• B2. Secret pasted into a plain Text env var -> FAILED (high-signal keyword) or WARNING (broad keyword tier).
• C. Secret inside a connection (Workday legacy ISU 0786a/d6081, or any ServiceNow connection) -> MANUAL.

Design rationale

• MANUAL for connection-stored secrets (approved by Senthil): the platform never returns connection secrets via any API, so auto-validation is impossible. MANUAL never fails readiness, matching the kit's existing pattern for non-readable config.
• Scoped to supported connectors (approved by Senthil). Grounded finding: every real ESS connector secret lives inside the connection (Workday legacy ISU Basic, ServiceNow OAuth2/Basic); every Entra path is secretless. No supported connector ships a Key Vault-backed env var today, so for the shipped surface this check runs as a defensive plaintext scanner plus MANUAL directives.
• Documented Dataverse tier only. Reads environmentvariabledefinitions / environmentvariablevalues / connectionreferences via the existing auth.query_all. No new external API is called. Read-only and idempotent (GETs only).
• Follows AGENTS.md conventions: status-bucketing (principle 7), result/remediation contract (principle 8), hardening-WARNING framing (principle 9), roles on every result, no fabricated doc URLs.

Effective-value evaluation

The classification loop iterates environment variable definitions and evaluates the effective value: the current-value override when present and non-empty, otherwise the definition's defaultvalue. This closes the blind spot where a secret pasted into a definition's default value (with no override record, common for solution-imported vars) would go unscanned. A Secret-type definition is only reported as safely stored when it actually holds a value; an unbound one is not asserted safe.

Testing

• New: tests/flightcheck/checks/test_infrastructure_secret_storage.py (18 tests) covering all outcomes, multi-resource bucketing, the KV-reference-in-Text-var non-flag case, the token keyword path, both SKIPPED (missing token / missing env URL) paths, plus the effective-value cases: plaintext secret in defaultvalue-only -> FAILED, Key Vault reference in defaultvalue-only -> not flagged, and unbound Secret-type var -> not reported as stored. Tests assert on result AND remediation phrases per the repo test rules.
• Full FlightCheck suite green: 882 passed.
• Run: python -m pytest tests/flightcheck/checks/test_infrastructure_secret_storage.py -q

Manual end-to-end validation (live environment)

Exercised against a live Power Platform environment via a probe that calls the check with a cached Dataverse token.

Validated:

• PASS (Outcome A) - no secret-bearing auth present: one INFRA-011 row, PASSED, "No inline connector secrets detected".
• FAIL (Outcome B2, high-signal keyword) - secret pasted into a plain Text env var: FAILED / Critical, remediation shows --scope full.
• WARNING (Outcome B2, broad keyword tier) - e.g. an apikey/token Text var: WARNING / Medium, remediation shows --scope full.

Pending (blocked on live Azure Key Vault access, in progress):

• PASS + MANUAL (Outcome B1) - populated Secret-type (Key Vault-backed) env var: PASSED for storage plus the MANUAL vault-hardening directive.
• MANUAL (Outcome C) - secret inside a ServiceNow or Workday legacy ISU connection.
• SKIPPED - --scope infrastructure (no Dataverse auth) and missing environment URL.

Risks / assumptions / known scope limits

• Vault hardening (AC1/AC5) is MANUAL in v1, not probed. Emitting the AC5 WARNING for a provably disabled soft-delete / purge / RBAC requires an Azure Key Vault ARM call, which needs a new registered API and an Azure rights grant most makers lack. Tracked as a documented fast-follow (see the FAST-FOLLOW comment in infrastructure.py).
• Plaintext detection (AC2) is env-var-name-based, not value-pattern-based. The effective value (current or default) is evaluated, but classification keys off the variable name keyword tiers, not a pattern match on the value. A secret in a blandly named Text var is not flagged. Deliberate, to keep the readiness-failing FAIL bucket free of false positives. Noted in code and the validation matrix.
• JSON-type env vars are not value-scanned. Only Text and Secret types participate; a secret embedded in a JSON-type var value is out of scope for v1.
• ServiceNow auth type is not readable from Dataverse, so any ServiceNow connection is surfaced as MANUAL (safe default, never fails readiness).

Files

• solutions/ess-maker-skills/scripts/flightcheck/checks/infrastructure.py - the check + helpers, registered in _INFRA_CHECKS.
• solutions/ess-maker-skills/src/reference/ess-docs/flightcheck/validation-matrix.md - INFRA-011 row.
• tests/flightcheck/checks/test_infrastructure_secret_storage.py - tests.
• tests/mocks/dataverse.py - env_var_def mock gains default_value.

[daeunJe0ng]

Reviewer Context

PR #186 - INFRA-011 Connector Secret Storage Safety - ADO task

What this PR adds
INFRA-011 is a new FlightCheck check that flags connector secrets that are stored unsafely.
It reads Dataverse environment variable definitions, environment variable values, and connection references, then classifies each into a readiness outcome.
It is read-only and idempotent.

How it decides (detect-first gate)
FAIL (Critical): a Text-type env var whose name contains a high-confidence secret keyword (secret, password, pwd, clientsecret) and whose Current Value is a non-empty literal that is not a Key Vault reference.
WARN (Medium): a Text-type env var whose name contains a broad keyword (token, apikey, api_key, credential) with an inline value, since the name alone is not conclusive.
PASS + MANUAL: a Key Vault-backed Secret-type env var passes for storage, and vault hardening (soft-delete, purge protection, RBAC) is surfaced as MANUAL because FlightCheck does not hold vault-plane rights.
MANUAL: a secret-bearing connection (Workday legacy ISU, ServiceNow) where the credential lives inside the connection and no API exposes it.
PASS (informational): none of the above present.

Manual end-to-end validation done against a live environment (test-dawn)
PASS (clean): no secret vars or connections, reported Passed.
FAIL: a Text var with high keywords and an inline Current Value, reported Failed / Critical listing the var.
WARN: a Text var named with a broad keyword (apikey) and an inline value, reported Warning / Medium listing the var.
Each outcome was verified by driving real Dataverse state in the maker portal and re-running the check.

Changes made during review
Fixed the remediation text in the FAIL and WARN branches: it told users to re-run /flightcheck --scope infrastructure, but that scope skips Dataverse auth so INFRA-011 would come back SKIPPED.
It now says --scope full, which is the only scope that authenticates Dataverse and actually re-runs the check.
This was live-verified in both FAIL and WARN output, and the INFRA-011 unit suite passes 15/15.

Known coverage gaps worth a reviewer decision (not blockers)
Default Value is not scanned. INFRA-011 only reads the environment variable values table, which is the Current Value.
A secret pasted into the definition's Default Value field is invisible to the check and returns PASS. This was reproduced.
Makers commonly set only the Default Value, so this is a real gap.

JSON-type env vars are not scanned by value. Detection is name-based on Text vars only.
Number, Boolean, and Data source types physically cannot hold a pasted secret, and Secret type is the safe case, but JSON can hold one and is skipped.

AC2 interpretation. The task AC2 says scan flow definitions for inline plaintext secrets via pattern match.
The PR instead implements env-var-name matching. The Default Value gap and the JSON gap share this root cause.
Recommend confirming the intended AC2 interpretation before merge.

Suggested reviewer focus
Confirm the AC2 interpretation (name-based detection vs flow-def pattern match).
Decide whether the Default Value gap should be closed now or tracked as a fast-follow.
Confirm the WARN vs FAIL keyword tiers match policy expectations.

[apurvabanka]

Overall

Solid, well-grounded PR. The three-outcome detect-first design is sound, statuses map correctly to readiness buckets (only high-confidence FAILED blocks; WARNING/MANUAL/PASSED/SKIPPED don't), the Workday ISU suffixes (0786a/d6081) and ServiceNow connector id (shared_service-now) match the fingerprints documented in tests/fixtures/cassettes/INDEX.md, and it follows the existing query_all/registry conventions. No blocking gaps.

Gaps worth addressing

1. defaultvalue is never scanned (real coverage blind spot). The check only reads environmentvariablevalues (current-value override records). A plaintext secret pasted into an environment-variable definition's defaultvalue — with no override value record — is never evaluated, so both the B2a FAILED and B2b WARNING paths silently miss it. This is a common shape: solution-imported env vars frequently carry only a defaultvalue. It mirrors the existing workday.py pattern, so it's a consistent limitation, but for a check whose whole purpose is catching inline plaintext secrets it's the most meaningful blind spot. Consider selecting defaultvalue on the definitions query and evaluating definitions that have no matching value record.

2. Secret-type env var reported PASSED even when unbound. In the value loop, a _ENV_VAR_TYPE_SECRET definition is appended to secret_env_vars regardless of whether raw_value is empty. An unbound/unset Secret-type var would still emit "Secret stored as Key Vault-backed… PASSED." Edge case, but it asserts safe storage for something that isn't actually populated.

3. statuscode (and connectionid) selected but unused. The connectionreferences query selects statuscode/connectionid, but _classify_secret_connections ignores them — so stale/disconnected connection references are still surfaced as MANUAL. Low impact (MANUAL never fails readiness), but it's either dead select fields to drop or an intended active-only filter that's missing.

4. No API-tier / INDEX.md entry (minor doc gap). INFRA-006 added a row to tests/fixtures/cassettes/INDEX.md; INFRA-011 didn't. The endpoints are already documented-tier (and connectionreferences is registered), and tests stub query_all, so it's arguably optional — but the new security reuse of the Workday ISU suffix fingerprint and the ServiceNow marker aren't cross-referenced there.

Nits

• The self-reported test count is off (PR body says 12; the file has 15). Not a problem, just stale.

CC: @daeunJe0ng