Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
761774e to
cb168e8
Compare
There was a problem hiding this comment.
Thanks solid structure overall, the N/A-CI-* convention is right, tests are thorough, and the NIST mapping fix (PR.DS-5 → PR.AC-4) looks good. All CI is green. However there is one blocking integration bug plus a few correctness gaps that need fixing before merge:
Blocking
1. The four rules are loaded by the scan engine but can never run.
engine.py glob-loads every az_*.py in scanner/rules/ and calls rule.scan(self.client, self.subscription_id) with an AzureClient (engine.py:71, engine.py:114). The AZ-CI rules declare scan(github_client, owner, repo) three required params so every production scan raises TypeError for all four rules, incrementing RULE_ERRORS_TOTAL and logging 4 errors per scan, while the rules silently never execute. Even if the wiring lands in PR 3/3, this PR must either exclude az_ci_* from the engine loader or provide an engine-compatible adapter, otherwise every scan between the two merges is polluted.
Should fix
2. github_client.py get_workflows() does not paginate.
/repos/{owner}/{repo}/actions/workflows returns 30 items per page by default. Repos with >30 workflows are silently truncated → false negatives on exactly the large repos this feature targets. Follow the Link header or pass per_page=100 + loop.
3. _workflow_common.py collect_uses() misses job-level uses: and crashes on empty jobs.
Reusable workflow calls (jobs.<id>.uses) are never collected, so AZ-CI-003 can't flag an unpinned reusable workflow. Also jobs: {build: null} → None.get("steps") → AttributeError.
4. az_ci_001.py OIDC anywhere in the file suppresses the long-lived-credential finding.
A workflow using azure/login with OIDC and separately referencing ARM_ACCESS_KEY passes clean. The suppression should require the credential to be absent, not merely OIDC to be present. Related: uses_workload_identity() matches the literal string "id-token: write", which breaks on extra spacing or quoted YAML — check the parsed permissions dict instead.
Nits
**5. github_client.py get_workflow_content():** for files >1 MB the contents API returns an empty contentfield; the code decodes""and caches it as valid content, so an oversized workflow scans as compliant instead of UNKNOWN. Treat emptycontentwith a presentdownload_url`/size as a failure.
6. tests/test_rules_ci_workflow.py test_no_token_returns_none_from_get_token: assert token is None or isinstance(token, str) is always true; assert token is None under the cleared env instead.
612156a to
81936b9
Compare
|
Hi @TFT444 @Vishnu2707 @H-Sihan , all 6 review items addressed:
46 tests passing, ruff clean, single signed commit. Ready for re-review. Thank you! |
TFT444
left a comment
TFT444
left a comment
There was a problem hiding this comment.
@emon22-ts good progress: engine exclusion, collect_uses hardening, the 1 MB guard, and the token test are all properly fixed. One blocker remains:
AZ-CI-001 OIDC suppression is still broken, and the new comment claims otherwise. az_ci_001.py says "A workflow using OIDC but also referencing ARM_ACCESS_KEY still fails", but uses_workload_identity() checks only _LONGTERM_CRED_PATTERNS[0] (AZURE_CLIENT_SECRET). OIDC + ARM_ACCESS_KEY/ARM_CLIENT_SECRET/AZURE_STORAGE_KEY/AZURE_PASSWORD still passes clean. Make the suppression require no credential match at all (e.g. gate on not creds in the scan, or check every pattern), and add a regression test for OIDC + ARM_ACCESS_KEY, since the absence of that test is exactly why this slipped.
Smaller items, same push: get_workflows() sends per_page=100 but still doesn't follow the Link header (docstring says "Paginates"; >100 workflows still truncate), uses_workload_identity() ignores job-level permissions so job-scoped id-token: write false-positives, and the four framework JSONs lost their trailing newlines.
Fix the AZ-CI-001 suppression and this is mergeable from my side.
81936b9 to
2f9482e
Compare
|
Hi @TFT444 @Vishnu2707 , all remaining items addressed:
47 tests passing, ruff clean, single signed commit. Ready for final review. Thank you! |
parthrohit22
left a comment
parthrohit22
left a comment
There was a problem hiding this comment.
Architecturally this is sound: github_client.py matches the codebase's existing try/except -> logger.error -> None-means-UNKNOWN convention exactly, no RULE_ID collisions against the ~90 existing rules, all 4 playbooks exist and pass bash -n, compliance framework JSON additions match the existing schema, and the YAML-1.1 on: parsing as boolean True is handled correctly (workflow.get("on") or workflow.get(True)) - an easy, common mistake in GitHub Actions tooling that this PR avoids.
But for a compliance scanner, misclassifying a secure config as a violation is a correctness bug, not a nit, and I confirmed 4 real false-positive paths by feeding realistic workflow YAML directly into each rule's scan() rather than just reading the code - all 4 are in code this PR adds. Left inline comments on each with the specific repro.
One more, documented as intentional rather than an oversight (az_ci_003.py's REMEDIATION explicitly calls Docker actions exempt) but worth a decision either way: is_action_pinned unconditionally treats every docker:// reference as pinned, so uses: docker://alpine:latest (a mutable tag) is never flagged - confirmed with a direct repro (0 findings). This is a real gap against the rule's own stated threat model, and OpenSSF Scorecard's Pinned-Dependencies check (cited as this PR's own compliance evidence source) does flag it. Worth confirming whether this is deliberate scope-narrowing for 1/3 or should require @sha256:... digests for Docker actions too.
Also flagging for visibility, not blocking: scanner/engine.py explicitly excludes az_ci_* from the automatic rule loader with a comment claiming a separate CI/CD scan entry point invokes them - I grepped the whole repo (api/, scanner/worker.py, everything) and no such entry point exists anywhere yet, on this branch or dev. Not a bug (nothing crashes), but since this is titled "1/3," worth confirming reviewers know this ships tested logic with no way to actually run it in the product until a follow-up wires it up. Separately, TestGitHubClient only covers trivial success/failure - the Link-header pagination in get_workflows, the GitHub App JWT-auth flow in _get_app_token, and the >1MB download_url branch in get_workflow_content have no coverage, worth closing before 2/3 builds on top of this client.
Full suite (after installing this environment's missing azure-*/prometheus-client/python-dotenv packages, an environment gap unrelated to the PR): 758 passed, 3 skipped. tests/test_rules_ci_workflow.py: 47/47 passed - the existing tests don't catch any of the 4 false positives below because none of them construct a workflow that's secure-by-a-more-specific-mechanism-than-the-naive-check (safe pull_request_target via base-ref checkout, job-level OIDC alongside an unrelated job's real secret, a credential name only in a comment, or per-job explicit least-privilege with no top-level default).
|
|
||
| def _checks_out_pr_code(content: str) -> bool: | ||
| """Return True if the workflow appears to check out PR branch code.""" | ||
| return any(p in content for p in _CHECKOUT_PATTERNS) and ( |
There was a problem hiding this comment.
This only checks whether actions/checkout and github.event.pull_request.head/head_ref co-occur anywhere in the raw file text - it never checks whether the checkout step's ref: is actually set to the untrusted value. Repro: a pull_request_target workflow that checks out the base ref (safe - no ref: override) and only interpolates github.event.pull_request.head.sha into a PR comment (exactly GitHub's own documented-safe mitigation) still gets flagged HIGH. test_safe_prt_without_checkout_no_finding only covers the trivial no-checkout-at-all case, so this gap isn't caught.
Failure scenario: any label/triage/comment workflow on pull_request_target that references head.sha for display purposes gets a false HIGH "pwn-request" finding.
| if parsed is not None: | ||
| perms = parsed.get("permissions") or {} | ||
| if isinstance(perms, dict): | ||
| has_oidc_permission = perms.get("id-token", "").lower() == "write" |
There was a problem hiding this comment.
This only reads the top-level permissions block. Job-level permissions: (GitHub's own recommended least-privilege pattern) is invisible here. Repro: a deploy job with permissions: id-token: write set at the job level, using azure/login with no client-secret (genuine OIDC), alongside an unrelated tf-state job that legitimately uses ARM_ACCESS_KEY for a Terraform remote-state backend - az_ci_001.py still emits a false HIGH "long-lived credential" finding on the deploy job because uses_workload_identity can't see the job-level grant that makes it real OIDC.
| has_azure_login = "azure/login" in yaml_content | ||
| # Check ALL long-lived credential patterns — OIDC suppression requires | ||
| # every credential to be absent, not just AZURE_CLIENT_SECRET. | ||
| has_any_long_lived_cred = any(p.search(yaml_content) for p in _LONGTERM_CRED_PATTERNS) |
There was a problem hiding this comment.
This scans the raw YAML text, including comments. Repro: a workflow with proper top-level id-token: write + azure/login (no secret) and no long-lived credential actually used, but with a migration comment like # Old workflow used ARM_CLIENT_SECRET, replaced by federated identity below. - the credential-name regex matches inside the comment, has_any_long_lived_cred becomes true, and az_ci_001 flags it HIGH even though the workflow is genuinely OIDC-only. Any team that leaves a changelog-style comment when migrating to OIDC (a natural thing to do) hits this.
|
|
||
| def is_permissions_broad(permissions: Optional[Dict[str, str]]) -> bool: | ||
| """Return True if permissions grant write-all or multiple broad scopes.""" | ||
| if permissions is None: |
There was a problem hiding this comment.
An absent top-level permissions: block is unconditionally treated as broad here, even when every individual job explicitly restricts its own permissions - which is itself a real, commonly-recommended secure pattern (explicit per-job least privilege instead of a workflow-level default). Repro: a workflow with no top-level permissions block but where every job declares permissions: contents: read still gets flagged HIGH by az_ci_002, which directly contradicts that rule's own REMEDIATION text ("Override per-job only where broader access is needed") - the rule's own guidance describes a pattern its own detection logic flags as a violation.
eccdc5a to
51ad175
Compare
|
Hi @TFT444 @parthrohit22 @Vishnu2707 , all 4 false positives fixed with regression tests: 1.FP1 (AZ-CI-004) - _checks_out_pr_code now uses the parsed workflow dict and checks whether the checkout step's ref: value actually contains a PR head reference. A pull_request_target workflow that checks out the base ref and only references head.sha in a run step (GitHub's documented-safe pattern) no longer flags. 2.FP2 (AZ-CI-001)- uses_workload_identity() now checks both top-level and job-level permissions blocks for id-token: write, so a deploy job with per-job OIDC permissions alongside other jobs is correctly recognised as OIDC. 3.FP3 (AZ-CI-001) - has_long_lived_credentials() now strips YAML comment lines before scanning, so a migration comment like # Old workflow used ARM_CLIENT_SECRET no longer triggers a finding. 4.FP4 (AZ-CI-002) - is_permissions_broad() now returns False when the top-level permissions block is absent but every job declares explicit permissions - per-job least privilege is correctly recognised as secure. Each fix has a dedicated regression test. 51 tests passing, ruff clean, single signed commit. On the docker:// pinning question: treating docker:// as always-pinned is intentional scope-narrowing for PR 1/3 - the REMEDIATION text already notes this. Will track docker @sha256: digest checking as a follow-up in PR 2/3 or 3/3. Ready for final review. Thank you! |
|
@emon22-ts, thanks for the false-positive fixes. Before I re-review them, please rebase onto current |
51ad175 to
1a7db10
Compare
|
@m-khan-97 Hi sir , Rebased onto current dev and CI is running. Head SHA: 1a7db10 Merge conflicts in all 4 compliance framework JSONs were resolved — upstream controls preserved and AZ-CI-001..004 mappings retained (110 controls each). @parthrohit22 @TFT444 ready for re-review |
m-khan-97
left a comment
m-khan-97
left a comment
There was a problem hiding this comment.
Emon, the four reported false-positive fixes survived the rebase: PR-head detection is scoped to checkout ref, job-level OIDC is recognized, comment-only credential names are ignored, and fully job-scoped permissions no longer fail merely because the top-level block is absent. CI is green.
I found one remaining permission-model false negative in AZ-CI-002. is_permissions_broad() treats an absent top-level block as safe whenever every job merely has a non-null permissions field; it never evaluates what those job-level values grant. Therefore a workflow where every job declares permissions: write-all, or three sensitive *: write scopes, returns compliant. Likewise, a harmless top-level contents: read plus a job-level permissions: write-all is never examined because only the top-level block is passed into the check. Those are exactly the over-broad token permissions this rule claims to detect.
Please evaluate the effective permissions at both levels: flag write-all/write and the broad-scope threshold in any job override, while preserving the valid per-job least-privilege case. Add regression tests for (1) no top-level block with every job declaring write-all, (2) safe top-level permissions overridden by one broad job, and (3) multiple jobs with narrow explicit permissions remaining clean. Then rerun the suite and request rereview.
|
Hi @m-khan-97 Sir , is_permissions_broad() now evaluates effective permissions at both levels:
The fix uses a nested _block_is_broad() helper that checks both write-all strings and 3+ sensitive write scopes, applied to both the top-level block and every job-level override independently. 53 tests passing. Head SHA: 8146a96 Ready for re-review. Thank you. |
8146a96 to
86bfd75
Compare
parthrohit22
left a comment
parthrohit22
left a comment
There was a problem hiding this comment.
Requesting changes before approval.
The earlier false-positive fixes are present, but three issues still prevent this PR from delivering reliable CI/CD security coverage:
-
AZ-CI-004 misses a privileged
workflow_runpath. It detects PR-head references but not checkouts usinggithub.event.workflow_run.head_sha. Aworkflow_runjob can access secrets and write tokens; checking out code from the triggering untrusted run is therefore a pwn-request risk that this rule currently reports as compliant. -
AZ-CI-002 does not calculate effective token permissions. It treats an absent workflow-level
permissionsblock as broad without considering enterprise, organization, or repository defaults. A centrally enforced restricted default can make the workflow read-only, so this produces false HIGH findings. Resolve the configured default permissions, or reportUNKNOWNwhen they cannot be read. -
The rules are not executable through OpenShield.
ScanEngineskips allaz_ci_*rules and this PR contains no alternative CI/CD scan entry point, target configuration, persistence flow, orUNKNOWNevaluation. As submitted, normal OpenShield scans cannot run these controls.
Please add regression tests for the workflow_run checkout case and restricted-default permissions case, then wire the rules into a CI/CD scanning path that records PASS/FAIL/UNKNOWN before requesting re-review.
|
Hi @parthrohit22 @m-khan-97 @TFT444 , all 3 blocking items addressed:
63 tests passing, ruff clean. Ready for re-review. Thank you. |
parthrohit22
left a comment
parthrohit22
left a comment
There was a problem hiding this comment.
Re-reviewed a8bf1d6 against my three items from the Sep 7 review.
1. AZ-CI-004 workflow_run checkout path — resolved
_DANGEROUS_TRIGGERS includes workflow_run and _PR_HEAD_REF_PATTERNS now covers github.event.workflow_run.head_sha / head_branch, so a workflow_run workflow that checks out the triggering run's untrusted head is flagged. Regression test present. Good.
2. AZ-CI-002 effective permissions — the resolve path is reading the wrong endpoint
scan() calls github_client.get_repo_info(), which is GET /repos/{owner}/{repo}. That response body does not carry default_workflow_permissions — that field is only returned by GET /repos/{owner}/{repo}/actions/permissions/workflow (which needs administration: read). So in production repo_default_permissions is always None:
- the
repo_default_permissions == "read"suppression branch is dead code, and - every workflow with no
permissions:block and non-explicit jobs emits theMEDIUM/effective_permissions: UNKNOWNfinding regardless of the real org/repo default.
test_..._restricted_default_permissions only passes because it mocks client.get_repo_info = lambda: {"default_workflow_permissions": "read"} — a field the real method never returns, so the test is asserting behaviour that can't happen against GitHub.
Fix: add a dedicated client method that calls /repos/{owner}/{repo}/actions/permissions/workflow, return None when it 403s (no admin scope), and have the tests mock that method. UNKNOWN-on-unavailable is the right fallback; it just needs to actually be reachable via the correct call, and the "default is read → suppress" claim in the PR description needs a code path that can execute.
3. Rules still aren't runnable in an OpenShield scan
scanner/ci_engine.py defines CIScanEngine, but nothing calls it — grep for CIScanEngine / ci_engine outside the file and its own tests returns nothing. There is:
- no API route and no hook into
POST /api/scans, - no CLI entry point,
- no persistence:
run_scan()returnsList[RuleEvaluation]and no caller writes those torule_evaluations/ throughsave_scan(), so nothingget_compliance_score()reads is ever populated, - no target resolution:
owner/repoare bare constructor args with no config/env wiring.
So "normal OpenShield scans cannot run these controls" is still true — this is a library class with a usage snippet in its docstring, not an entry point. If the wiring is deliberately deferred to a later PR in the 1/5 series, please say so in the PR/issue scope and I'll track item 3 separately instead of blocking on it; as written the PR text ("CI/CD scan entry point — scanner/ci_engine.py adds CIScanEngine") reads as if it closes this, and it doesn't.
Non-blocking
ci_engine.run_scan()calls eachrule.scan()(which already iterates every workflow) and then iterates the workflows again per rule — every workflow YAML is parsed twice per rule. The client cache keeps the HTTP cost down, but consider having the engine consume the rule findings it already computed rather than re-walking._checks_out_pr_codeonly inspectsactions/checkoutsteps'with.ref. Arun:step doinggit checkout ${{ github.event.workflow_run.head_sha }}orgh pr checkoutis missed. The same gap applies to thepull_requestpatterns so it isn't a regression, but worth a follow-up.
63 CI-rule tests pass locally. Happy to re-review once 2 is reading the right endpoint and 3 is either wired up or explicitly scoped out.
TFT444
left a comment
TFT444
left a comment
There was a problem hiding this comment.
@emon22-ts three issues remain:
-
Double workflow content fetch (raised in prior review):
get_workflow_content(path)is called once per rule per workflow file (4 rules × N files = 4N API calls for the same content). Cache content keyed by path before the rule loop. -
finding_pathspath mismatch:finding_pathsis built fromf.get("resource_name")but compared againstwf.get("path")(full path like.github/workflows/ci.yml). If any rule setsresource_nameto just the filename, the lookup misses it and emits a false PASS. -
Bare
except Exceptionin_get_app_token(): swallowsInvalidKeyErrorand other diagnostics. Logexcso operators can debug auth failures.
OWASP#259 PR 1/3) Implements controls 1-4 from issue OWASP#259: - AZ-CI-001: Long-lived Azure credentials instead of workload identity federation - AZ-CI-002: Unnecessarily broad workflow token permissions - AZ-CI-003: Third-party action not pinned to immutable commit SHA - AZ-CI-004: Untrusted PR input reaches privileged workflow context False positive fixes: - FP1 az_ci_004: _checks_out_pr_code uses parsed dict, checks checkout ref: value - FP2 az_ci_001: uses_workload_identity checks job-level permissions blocks - FP3 az_ci_001: credential scan excludes YAML comment lines - FP4 az_ci_002: absent top-level permissions not broad when all jobs declare explicit perms New files: - scanner/github_client.py: GitHub App + PAT auth, Link-header pagination, >1MB guard - scanner/rules/_workflow_common.py: shared YAML helpers - scanner/rules/az_ci_001..004.py: four scanner rule modules - playbooks/cli/fix_az_ci_001..004.sh: remediation playbooks - tests/test_rules_ci_workflow.py: 51 tests including 4 FP regression tests ci.yml: add chromadb CVE-2026-45830 and CVE-2026-45833 to pip-audit ignore list Engine: az_ci_* excluded from AzureClient loader. Compliance: CIS N/A-CI-*, NIST, ISO 27001, SOC2 with trailing newlines. Refs OWASP#259 Signed-off-by: Mahfuzur Rahman Emon <mahfuzur.emon01@gmail.com>
…egression tests - is_permissions_broad() now checks both top-level and job-level permission blocks - No top-level block with every job declaring write-all now correctly flags - Safe top-level permissions overridden by a broad job-level block now correctly flags - Per-job least-privilege (narrow explicit permissions) remains clean - Added regression tests for all 3 scenarios (53 tests total) Refs OWASP#259 Signed-off-by: Mahfuzur Rahman Emon <mahfuzur.emon01@gmail.com>
…issions handling - scanner/ci_engine.py: new CIScanEngine returning RuleEvaluation PASS/FAIL/UNKNOWN - az_ci_004: add workflow_run head_sha/head_branch to dangerous checkout patterns - az_ci_002: return UNKNOWN finding when repo default permissions unavailable, skip when repo default is restricted (read-only) - tests: add regression tests for workflow_run checkout, restricted repo default, unavailable repo default, and 7 CIScanEngine evaluation contract tests - 63 tests total Refs OWASP#259 Signed-off-by: Mahfuzur Rahman Emon <mahfuzur.emon01@gmail.com>
… exception - ci_engine: pre-fetch and cache workflow content once (was 4N API calls, now N) - ci_engine: match findings on metadata.workflow_path not resource_name to avoid false PASS - ci_engine: unknown_finding branch now also requires effective_permissions==UNKNOWN so FAIL findings are not misclassified - github_client: log exception type in _get_app_token for auth debugging Refs OWASP#259 Signed-off-by: Mahfuzur Rahman Emon <mahfuzur.emon01@gmail.com>
a8bf1d6 to
20d32ed
Compare
Signed-off-by: Mahfuzur Rahman Emon <mahfuzur.emon01@gmail.com>
| logger.error( # nosemgrep: python-logger-credential-disclosure | ||
| "GitHub App token generation failed (check GITHUB_APP_* env vars): %s", | ||
| type(exc).__name__, | ||
| ) |
|
Hi, all 3 review items addressed and CI is green:
63 tests passing. Head SHA: 43e5c88 @parthrohit22 @Vishnu2707 @TFT444 ready for re-review |
TFT444
left a comment
TFT444
left a comment
There was a problem hiding this comment.
Re-reviewed. The double-fetch is resolved (content cache keyed by path) and the finding_paths key mismatch is fixed. One issue remains: get_repo_info() calls GET /repos/{owner}/{repo}, but default_workflow_permissions is not returned by that endpoint. It is only available from GET /repos/{owner}/{repo}/actions/permissions/workflow (requires administration: read). In production this means repo_default_permissions is always None and AZ-CI-002 silently produces only UNKNOWN findings. Please add a separate get_workflow_permissions() method calling the correct endpoint.
5 participants
What does this PR do?
Implements controls 1–4 from issue #259 (DevSecOps and supply-chain security). Adds a GitHub API client, shared workflow YAML helpers, four scanner rules, remediation playbooks, and 46 tests.
Type of change
Rules added
New files
scanner/github_client.py— GitHub App installation auth with GITHUB_TOKEN PAT fallback, returns None on missing permissions (UNKNOWN handling)scanner/rules/_workflow_common.py— shared YAML parsing, action pinning detection, permission analysis, dangerous trigger detectionscanner/rules/az_ci_001.py— long-lived credential detectionscanner/rules/az_ci_002.py— broad token permission detectionscanner/rules/az_ci_003.py— unpinned action detectionscanner/rules/az_ci_004.py— pwn-request pattern detectionplaybooks/cli/fix_az_ci_001..004.sh— remediation playbookstests/test_rules_ci_workflow.py— 46 testsCompliance mappings
Testing
Related issue
Refs #259 (PR 1 of 3 - controls 1 - 4)