Skip to content

Conversation

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jan 26, 2022

Guardian now looks for this file at the repo root instead of in .config

CC @safern @carlossanlop dotnet/runtime might need to move this file too to get CredScan to pick up your suppressions again

@wtgodbe wtgodbe requested review from a team and riarenas January 26, 2022 17:54
@wtgodbe wtgodbe requested a review from Pilchie as a code owner January 26, 2022 17:54
@riarenas
Copy link
Contributor

I would definitely run a test internal build to make sure this actually gets picked up as you expect. Guardian is... finicky at best.

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 26, 2022

I would definitely run a test internal build to make sure this actually gets picked up as you expect. Guardian is... finicky at best.

How can I tell if it gets picked up by the aspnetcore pipeline? I thought SDL/CredScan only ran in Validate-DotNet

@riarenas
Copy link
Contributor

You should be able to enable SDL validation in post-build.yml with code like this https://github.com/dotnet/arcade/blob/fd38a2e8191a46ddd6a559dbb52c9624989b2cf4/azure-pipelines.yml#L324-L338

@riarenas
Copy link
Contributor

@adiaaida might also have suggestions on whether you can just try directly on the validation pipeline?

@michellemcdaniel
Copy link
Contributor

You won't be able to validate this until the build is published to BAR. However, you could run an official build on your branch, run the promotion pipeline on that build (thus publishing to BAR) and then manually Validate-DotNet with that bar id (I normally find a recent run of the repo of interest, click "run new" from that run, and then update the bar id from the variables section)

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 26, 2022

Got a test build going here that should run SDL, if it doesn't I'll try what you suggested @adiaaida. Thanks!

https://dev.azure.com/dnceng/internal/_build/results?buildId=1573989&view=results

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 26, 2022

Didn't work, following up w/ the Guardian team over email

@wtgodbe wtgodbe added the blocked The work on this issue is blocked due to some dependency label Jan 26, 2022
@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 26, 2022
@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 31, 2022

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 31, 2022

@michellemcdaniel
Copy link
Contributor

https://dev.azure.com/dnceng/internal/_build/results?buildId=1582912&view=results

This will probably also fail, because I think you need to take the /s out (/s is needed for the sdl-tsa-vars.config file because of where Validate-DotNet clones repos, but will be wrong here, I think)

@wtgodbe
Copy link
Member Author

wtgodbe commented Jan 31, 2022

This will probably also fail, because I think you need to take the /s out (/s is needed for the sdl-tsa-vars.config file because of where Validate-DotNet clones repos, but will be wrong here, I think)

Thanks - https://dev.azure.com/dnceng/internal/_build/results?buildId=1582945&view=results

-TsaOnboard $True
-TsaPublish $True
-PoliCheckAdditionalRunConfigParams @("UserExclusionPath < $(Build.SourcesDirectory)/s/eng/PoliCheckExclusions.xml")
-CrScanAdditionalRunConfigParams @("SuppressionsFile < $(Build.SourcesDirectory)/s/CredScanSuppressions.json")
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to include this in dotnet/runtime as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, and you don't need to move the file - just point this towards wherever the file lives (assuming this change is validated by https://dev.azure.com/dnceng/internal/_build/results?buildId=1582945&view=results)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that is why I was wondering, if it is needed to be specified in this config file we do we need to move it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wtgodbe it's a nit but could we try to limit our root directory files to those that contributors normally care about❔ I'd rather we left the suppressions file in .config/ or eng/. (eng/ is probably better since .config/ means something special to Arcade's Tools.proj.)

@wtgodbe
Copy link
Member Author

wtgodbe commented Feb 1, 2022

@wtgodbe wtgodbe removed the blocked The work on this issue is blocked due to some dependency label Feb 9, 2022
@wtgodbe
Copy link
Member Author

wtgodbe commented Feb 9, 2022

Followed up with credcscan folks offline, we think this should work as-is.

@wtgodbe wtgodbe merged commit cc3f45a into dotnet:main Feb 9, 2022
@ghost ghost added this to the 7.0-preview2 milestone Feb 9, 2022
dougbu added a commit that referenced this pull request Apr 7, 2022
)

* Revert "Move CredScanSuppressions.json to root of repo (#39780)"
- root location does not work w/ `CredScan` check when pushing to AzDO mirror

This reverts commit cc3f45a.

* Inform Guardian of CredScanSuppressions.json path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants