Skip to content

ci: run CredScan and break the build on findings - #1269

Open
Maxim Semenov (maximsemenov80) wants to merge 2 commits into
mainfrom
maximsemenov/add-credscan-post-analysis
Open

Maxim Semenov (maximsemenov80) wants to merge 2 commits into
mainfrom
maximsemenov/add-credscan-post-analysis

Conversation

@maximsemenov80

Copy link
Copy Markdown

Why

The SDL static analysis requirement asks every team to run CredScan and to either file bugs or break the build on findings. This repo's CodeQL already runs in vsts/build.yaml, but CredScan has not run since the classic azure-iot-sdk-python-sdl pipeline was disabled in 2023.

What

Adds a CredScan job to vsts/build.yaml: CredScan@3 with the existing credscan_suppression.json, PublishSecurityAnalysisLogs@3, and PostAnalysis@2 (CredScan only) so unsuppressed findings fail the build. TSA bug filing is not used; the requirement accepts a build break instead. The job runs on windows-2022 (CredScan is a Windows task) and in parallel with the other jobs, so it does not lengthen the pipeline.

Verification

Queued Azure.azure-iot-sdk-python (def 330) on this branch, build 166125: all jobs succeeded. CredScan scanned 390 files, found 19 credential-like records, all covered by credscan_suppression.json, 0 unsuppressed; Post Analysis passed. I did not test a failing case.

Notes

  • This job also runs on pull requests, so a new secret in a PR will fail it.
  • Not added here: PoliCheck and Bandit (lightweight checks, not required by the requirement), and TSA upload.
  • Related work item: SDL task "Run SDL code analysis tools and automatically file bugs for identified security issues" (One #40462909).

Add a CredScan job to the build pipeline, with Post Analysis so unsuppressed
credential findings fail the build. CredScan is required of every team by the
SDL static analysis requirement, and the old classic SDL pipeline that ran it
for this repo has been disabled since 2023. TSA bug filing is not used; the
requirement accepts a build break instead.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@maximsemenov80

Copy link
Copy Markdown
Author

CI status: the two failing e2e checks on this PR are not caused by this change.

  • The new CredScan job passes (build 166127: CredScan, Post Analysis, Component Governance and Publish all succeeded). It scanned 390 files and found 19 credential-like records, all covered by credscan_suppression.json; 0 unsuppressed. CodeQL, the Python 3.10-3.14 unit tests, Ruff and the CLA check also pass.
  • Python E2E (def 426, build 166129) fails in 5 jobs ("E2E Device Client MQTT Connection String"). The scheduled run on main this morning (build 166098, 11-10 13:00, before this PR) failed the same 5 jobs and the same tests; main passed 11-06 to 11-09. So this broke today independently of this PR.
  • DPS e2e (def 331, build 166128) fails with an HttpOperationError inside a test that creates an X.509 individual enrollment (a service call made by the test). There is no main run today to compare against (last runs 10-27 and 11-03 passed). I have not found the cause; I will re-run it.
  • This PR only touches vsts/build.yaml, which only the Azure.azure-iot-sdk-python pipeline (def 330) uses. The e2e pipelines use their own YAML.

Not tested: a failing CredScan case (a new unsuppressed secret). Adding this job is part of the SDL static analysis requirement (CredScan, with Post Analysis as the build break instead of TSA bug filing).

@maximsemenov80

Copy link
Copy Markdown
Author

Update on the DPS e2e failure: it is not caused by this PR.

I re-ran Azure.azure-iot-sdk-python-dps-e2e (def 331) twice with the same code as the original failing run:

Build Source Result
166136 this PR (merge ref) 20 failed, 4 skipped
166137 main (3c77949, no PR changes) 20 failed, 4 skipped, same tests

In both, the device registers successfully through DPS (result_from_register returns), and the test then fails inside assert_device_provisioned, where the test helper calls the IoT Hub service API get_device (iothubservice20180630) and gets 400 Bad Request. So the failure is in the test's service-side verification step, not in the device SDK code path or in vsts/build.yaml. I have not identified the cause. The last passing runs were 10-27 and 11-03; the "Resolve latest dependencies" step pulls the newest versions of many packages, so a dependency or test-hub change since then is possible.

The Python E2E failure on main today (build 166098) is a different set of tests. Neither is related to the CredScan job added here, which passes.

@maximsemenov80

Copy link
Copy Markdown
Author

Update: root cause of the e2e failures found. Fix PR: #1270 (draft).

The azure-iot CLI extension is installed on demand in these pipelines, and its 1.0.0 release (2026-11-05) made az iot hub connection-string show and az iot dps linked-hub create default to automatic hostname selection. New connection strings carry <hub>.device.azure-devices.net, and registry (service) calls sent to that device endpoint get HTTP 400. That is what broke both the DPS e2e and Python E2E pipelines on main (same commit passed on 10-27). It is not caused by this PR or by the SDK.

With the fix from #1270 both pipelines pass on that branch (DPS e2e: 20 passed; Python E2E: all jobs green). Once #1270 merges, the e2e checks on this PR should pass after a re-run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The PostAnalysis@2 step uses the older CredScan input instead of GdnBreakGdnToolCredScan, leaving the intended CredScan-specific build-break policy unconfigured.

1 open issue
What changed in this PR

Adds credential scanning to the existing Azure Pipelines build to meet the SDL requirement, with build failure intended to replace automatic bug filing.

Changes:

  • Adds a parallel CredScan job on windows-2022, using the existing suppression file.
  • Publishes security analysis logs and adds Post Analysis for intended CredScan enforcement.
File Description
vsts/​build.yaml Adds the CredScan job, log publishing, and build-break configuration.

Review effort: Balanced

Comment thread vsts/build.yaml Resolved
PostAnalysis@2 does not have a "CredScan" input, so it was silently ignored and
the step included every tool in its break filter. Set GdnBreakAllTools: false
and GdnBreakGdnToolCredScan: true so only CredScan results break the build, as
intended.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The CredScan pipeline change has no active review issues.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants