Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ implement more of the Azure DevOps client #4456

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JamieMagee
Copy link
Contributor

@JamieMagee JamieMagee commented Dec 13, 2024

What kind of change does this PR introduce?

Includes:

  • GetBranch
  • GetSuccessfulWorkflowRuns
  • ListCheckRunsForRef
  • ListStatuses
  • ListWebhooks
  • SearchCommits

Also, includes comments about methods which can never be implemented:

  • GetOrgRepoClient
    • Org repository, AKA the <org>/.github repository, is a GitHub-specific feature
  • ListLicenses
    • Azure DevOps doesn't have a license detection feature. Thankfully, the License check falls back to file-based detection.

Still need to implement:

  • ListReleases
    • Needs a little more investigation to line up the Azure DevOps implementation with what Scorecard expects

(Is it a bug fix, feature, docs update, something else?)

What is the current behavior?

Missing implementations for the methods listed above

What is the new behavior (if this is a feature change)?**

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Next step of #4177

Special notes for your reviewer

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

implement more of the Azure DevOps client

@JamieMagee JamieMagee requested a review from a team as a code owner December 13, 2024 05:31
@JamieMagee JamieMagee requested review from justaugustus and spencerschrock and removed request for a team December 13, 2024 05:31
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 54.13223% with 222 lines in your changes missing coverage. Please review.

Project coverage is 68.47%. Comparing base (353ed60) to head (a58e4f5).
Report is 85 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4456      +/-   ##
==========================================
+ Coverage   66.80%   68.47%   +1.67%     
==========================================
  Files         230      246      +16     
  Lines       16602    18427    +1825     
==========================================
+ Hits        11091    12618    +1527     
- Misses       4808     4983     +175     
- Partials      703      826     +123     

clients/azuredevopsrepo/branches.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/builds.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/builds.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/builds.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/const.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/commits.go Outdated Show resolved Hide resolved
clients/azuredevopsrepo/commits_test.go Outdated Show resolved Hide resolved
@spencerschrock
Copy link
Member

How do Branch-Protection and SAST look now?

@JamieMagee
Copy link
Contributor Author

SAST works perfectly, but Branch-Protection is still broken until ListReleases is implemented:

SAST
$ env SCORECARD_EXPERIMENTAL=1 go run ./. --repo https://dev.azure.com/jamiemagee/jamiemagee/_git/scorecard --checks SAST
Starting [SAST]
Finished [SAST]

RESULTS
-------
Aggregate score: 10.0 / 10

Check scores:
|---------|------|----------------------------|-----------------------------------------------------------------|
|  SCORE  | NAME |           REASON           |                    DOCUMENTATION/REMEDIATION                    |
|---------|------|----------------------------|-----------------------------------------------------------------|
| 10 / 10 | SAST | SAST tool detected: CodeQL | https://github.com/ossf/scorecard/blob/main/docs/checks.md#sast |
|---------|------|----------------------------|-----------------------------------------------------------------|
Branch-Protection
$ env SCORECARD_EXPERIMENTAL=1 go run ./. --repo https://dev.azure.com/jamiemagee/jamiemagee/_git/scorecard --checks Branch-Protection
Starting [Branch-Protection]
Finished [Branch-Protection]

RESULTS
-------
Aggregate score: ?

Check scores:
|-------|-------------------|--------------------------------|------------------------------------------------------------------------------|
| SCORE |       NAME        |             REASON             |                          DOCUMENTATION/REMEDIATION                           |
|-------|-------------------|--------------------------------|------------------------------------------------------------------------------|
| ?     | Branch-Protection | internal error: unsupported    | https://github.com/ossf/scorecard/blob/main/docs/checks.md#branch-protection |
|       |                   | feature                        |                                                                              |
|-------|-------------------|--------------------------------|------------------------------------------------------------------------------|
Error: check runtime error: Branch-Protection: internal error: unsupported feature
2024/12/18 10:58:02 error during command execution: check runtime error: Branch-Protection: internal error: unsupported feature
exit status 1

The issue with ListReleases on Azure DevOps is it doesn't have a concept of a release in the same way that GitHub and GitLab do. What is called a release on Azure DevOps is a type of build pipeline. I'm still trying to figure out the best way to map the Scorecard concept to Azure DevOps, but I didn't want to block these changes until I have figured it out.

The approach I'm working on at the moment is listing releases, finding the build that triggered the release, and using the artifacts produced by the build.

@JamieMagee JamieMagee force-pushed the jamiemagee/azure-devops-rest-of-the-owl branch 2 times, most recently from d5da1e2 to 12af663 Compare December 18, 2024 19:14
Includes:
  - `GetBranch`
  - `GetSuccessfulWorkflowRuns`
  - `ListCheckRunsForRef`
  - `ListStatuses`
  - `ListWebhooks`
  - `SearchCommits`

Also, includes comments about methods which can never be implemented:
  - `GetOrgRepoClient`
    - Org repository, AKA the `<org>/.github` repository, is a GitHub-specific feature
  - `ListLicenses`
    - Azure DevOps doesn't have a license detection feature. Thankfully, the License check falls back to file-based detection.

Still need to implement:
  - `ListReleases`
    - Needs a little more investigation to line up the Azure DevOps implementation with what Scorecard expects

Signed-off-by: Jamie Magee <[email protected]>
Signed-off-by: Jamie Magee <[email protected]>
@JamieMagee JamieMagee force-pushed the jamiemagee/azure-devops-rest-of-the-owl branch from 12af663 to 1d3cd0b Compare December 18, 2024 19:14
@spencerschrock
Copy link
Member

SAST works perfectly

I'll note this is falling back to the github detection of the .github/workflows, and not through your added vstfs:///build/build string (which will be more likely for AZDO projects I assume?)

The issue with ListReleases on Azure DevOps is it doesn't have a concept of a release in the same way that GitHub and GitLab do

I'm curious if we should just continue on the ListReleases ErrUnsupportedCheck so we get branch protection data for the default branch at least

@JamieMagee
Copy link
Contributor Author

I'll note this is falling back to the github detection of the .github/workflows, and not through your added vstfs:///build/build string (which will be more likely for AZDO projects I assume?)

That's an issue with the check itself than with the Azure DevOps client. There are definitely a couple of checks like that.

The SAST check specifically would need to be updated to look for all YAML files in the repository, because Azure Pipelines files can be in any arbitrary location in the repository, and look for the AdvancedSecurity-Codeql-Analyze task. See Code scanning for GitHub Advanced Security for Azure DevOps for more information.

I'm curious if we should just continue on the ListReleases ErrUnsupportedCheck so we get branch protection data for the default branch at least

Yes, that would be one approach. But I don't want to expand the scope of this PR too much more.

@spencerschrock
Copy link
Member

The SAST check specifically would need to be updated to look for all YAML files in the repository, because Azure Pipelines files can be in any arbitrary location in the repository, and look for the AdvancedSecurity-Codeql-Analyze task

Whoops, I had meant CI-Tests, as that's what you added the vstfs:///build/build string for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants