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

add TSA config #4860

Merged
merged 5 commits into from
Jul 12, 2024
Merged

add TSA config #4860

merged 5 commits into from
Jul 12, 2024

Conversation

danmoseley
Copy link
Member

@danmoseley danmoseley commented Jul 11, 2024

  • Add tsa config which is necessary to get some SDL tools running. Based on aspnetcore and runtime ones.
  • Include empty policheck exclusion file so it's easy to add any exclusions needed later.
  • Add other kind of tsa config.

@mmitche could you confirm this looks right, and whether we have any other actions to onboard Aspire and see compliance alerts flowing?

Next I will add to dotnet/extensions and dotnet/aspire-samples -- I see they already exist in sdk, winforms,wpf, runtime, aspnetcore and all seem comparable. @wtgodbe where else might we need these?

@mmitche
Copy link
Member

mmitche commented Jul 11, 2024

I think that's arcade's support for SDL in post-build. The 1ES equivalent is https://github.com/dotnet/arcade/blob/35ae51c95ebb0baca22475b0f5f39d5270a2f2aa/Documentation/CodeQLGuidance.md?plain=1#L18

@mmitche
Copy link
Member

mmitche commented Jul 11, 2024

Actually, you may want both. Let me look further

@danmoseley
Copy link
Member Author

If there's any doubt at all, I suggest every repo have both, it's harmless?

Clearly we need the 1ES ones for non Arcade repos.

@danmoseley
Copy link
Member Author

I see that dotnet/sdk and dotnet/wpf do not have .config\tsaoptions.config. cc @marcpopMSFT and @pchaurasia14 to see whatever guidance you provide us...

@danmoseley
Copy link
Member Author

OK added both to all these PR's.

eng/sdl-tsa-vars.config Outdated Show resolved Hide resolved
@mmitche
Copy link
Member

mmitche commented Jul 11, 2024

Here's my recommendation. Can you create a branch with tsaoptions.json with relevant info, add in the change i made for enabling policheck, and let's use that. That will cover all of the tools in the Arcade list and we can verify the tsa filing capabilities.

eng/sdl-tsa-vars.config Outdated Show resolved Hide resolved
@danmoseley
Copy link
Member Author

Here's my recommendation. Can you create a branch with tsaoptions.json with relevant info, add in the change i made for enabling policheck, and let's use that. That will cover all of the tools in the Arcade list and we can verify the tsa filing capabilities.

Well, I pushed this branch to https://github.com/dotnet/aspire/commits/tsaconfig/ -- is that what you want? (This PR is using a branch in my fork)

add in the change i made for enabling policheck

I don't know what this is?

@mmitche
Copy link
Member

mmitche commented Jul 11, 2024

Here's my recommendation. Can you create a branch with tsaoptions.json with relevant info, add in the change i made for enabling policheck, and let's use that. That will cover all of the tools in the Arcade list and we can verify the tsa filing capabilities.

Well, I pushed this branch to https://github.com/dotnet/aspire/commits/tsaconfig/ -- is that what you want? (This PR is using a branch in my fork)

add in the change i made for enabling policheck

I don't know what this is?

https://dev.azure.com/dnceng/internal/_git/dotnet-aspire/commit/5243220775049681b78d936d0aa8f43c2c5fba63/

@danmoseley
Copy link
Member Author

ah! OK, added.

@danmoseley
Copy link
Member Author

Why is policheck special in the template -- what about other compliance tooling, do we enable other stuff there? (I guess my answer is probably in the 1ES docs.)

@danmoseley
Copy link
Member Author

@mmitche do you want to sign off.

@marcpopMSFT
Copy link
Member

I see that dotnet/sdk and dotnet/wpf do not have .config\tsaoptions.config. cc @marcpopMSFT and @pchaurasia14 to see whatever guidance you provide us...
We have a different tsa config file that we haven't touched in quite a while. Is there a new way we should be doing this now?
https://github.com/dotnet/sdk/blob/main/eng/sdl-tsa-vars.config

@danmoseley
Copy link
Member Author

Is there a new way we should be doing this now?

@marcpopMSFT I think we're using this PR to test the theory that since moving from custom/Arcade templates to 1ES templates, we don't use that file any more, but instead use tsaoptions.config. We're going to merge this and see what happens then follow in other repos.

@danmoseley
Copy link
Member Author

@mmitche if you can sign off we can see what happens overnight?

@mmitche mmitche merged commit a133411 into dotnet:main Jul 12, 2024
9 checks passed
@harshit7962
Copy link
Member

@danmoseley similar to dotnet/sdk we too have a sdl-tsa-vars.config file but as you mentioned there, it is not being consumed by any of the pipelines here either. Are we expected to make changes to this and create tsaoption.json file and add it to the pipeline?

@mmitche
Copy link
Member

mmitche commented Jul 12, 2024

for SDK or WPF, it actually does get consumed because the staging pipeline runs SDL (currently) for all repos in the drop. Repos like aspire don't participate in that pipeline by design.

My sense here is that tsaoptions is the right way to do things going forward, but let's watch the output of this overnight to be sure.

@danmoseley danmoseley deleted the tsaconfig branch July 12, 2024 16:55
@danmoseley
Copy link
Member Author

seems to all have run?
https://dev.azure.com/dnceng/internal/_build/results?buildId=2493583&view=logs&j=bc38e8b8-e027-53cb-48e7-2adbd1070eca&t=b700cb98-6adf-5a7d-1ac8-1351904e01bc

not sure what this is

fail: Error: Failed to assign build '231591' to channel ''.
      System.InvalidOperationException: Unable to activate type 'Microsoft.DotNet.DarcLib.AzureDevOpsClient'. The following constructors are ambiguous:
      Void .ctor(Maestro.Common.AzureDevOpsTokens.IAzureDevOpsTokenProvider, Microsoft.DotNet.DarcLib.Helpers.IProcessManager, Microsoft.Extensions.Logging.ILogger`1[Microsoft.DotNet.DarcLib.AzureDevOpsClient])
      Void .ctor(Maestro.Common.AzureDevOpsTokens.IAzureDevOpsTokenProvider, Microsoft.DotNet.DarcLib.Helpers.IProcessManager, Microsoft.Extensions.Logging.ILogger)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateConstructorCallSite(ResultCache lifetime, ServiceIdentifier serviceIdentifier, Type implementationType, CallSiteChain callSiteChain)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact(ServiceDescriptor descriptor, ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain, Int32 slot)
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain)

@mmitche
Copy link
Member

mmitche commented Jul 12, 2024

Rerunning. We just rolled out a fix and it should work now

@danmoseley
Copy link
Member Author

I see credscan, binskim, policheck there -- good. How do I see those results, short of them showing up on my S360 eventually?

Also I don't see codeql run - is that expected? And I don't see run results for it either: https://codeql.microsoft.com/codeql/jobs?Uri=https%3A%2F%2Fdev.azure.com%2Fdnceng%2Finternal%2F_git%2Fdotnet-aspire

@danmoseley
Copy link
Member Author

That issue above is now gone.

@mmitche thoughts about my 2 questions above

@danmoseley
Copy link
Member Author

I downloaded drop_sdl_sources from the artifacts and see policheck and credscan results there (and "armory"). There's two credscan hits I'll suppress. I don't see binskim or codeql.

@mmitche
Copy link
Member

mmitche commented Jul 12, 2024

@mmitche
Copy link
Member

mmitche commented Jul 12, 2024

@danmoseley
Copy link
Member Author

Binskim kept getting skipped because it didn't find any binaries to scan: https://dev.azure.com/dnceng/internal/_build/results?buildId=2493583&view=logs&j=0bc77094-9fcd-5c38-f6e4-27d2ae131589&t=61b312a4-28dc-5d04-2147-0002389671a5

several of those steps are looking in a 'codecoverage' sub directory of the output, which seems fishy/wrong. eg

2024-07-12T15:48:49.9733142Z [command]C:\Windows\System32\WindowsPowerShell\v1.0\\powershell.exe -NoLogo -NoProfile -NonInteractive -ExecutionPolicy Unrestricted -File D:\a\_work\_tasks\1ESGPTRunTask_99645293-d90d-4e16-b7b6-ca10044e8ce6\3.0.179\scripts\binSkimCollectFiles.ps1
2024-07-12T15:48:51.4202762Z Received output directory: 'D:\a\_work\1\s/artifacts//CodeCoverage' and sources directory: 'D:\a\_work\1\s'

@RussKie any idea why it thinks that's the output directory? Is the dotnet-coverage collect step somehow setting it? Full log:

https://dev.azure.com/dnceng/7ea9116e-9fac-403d-b258-b31fcf1bb293/_apis/build/builds/2493583/logs/270

radical added a commit to radical/aspire that referenced this pull request Jul 12, 2024
commit 9071c84
Author: Ankit Jain <radical@gmail.com>
Date:   Fri Jul 12 19:04:26 2024 -0400

    fix e2e

commit 68a7ae0
Merge: 09acf2d 681f2e7
Author: Ankit Jain <radical@gmail.com>
Date:   Fri Jul 12 18:57:33 2024 -0400

    Merge remote-tracking branch 'origin/main' into tests-out-of-repo

commit 681f2e7
Author: Dan Moseley <danmose@microsoft.com>
Date:   Fri Jul 12 14:04:57 2024 -0600

    Make credscan run clean (dotnet#4876)

    * suppressions

    * more

    * typo

commit a133411
Author: Dan Moseley <danmose@microsoft.com>
Date:   Fri Jul 12 08:42:40 2024 -0600

    add TSA config (dotnet#4860)

    * tsaconfig

    * more

    * remove the config per Matt

    * add policheck in 1ES template

    * policheck exclusions

commit cf61a58
Author: James Newton-King <james@newtonking.com>
Date:   Fri Jul 12 13:00:02 2024 +0800

    Load plotly as a module on metrics page (dotnet#4857)

commit 09acf2d
Author: Ankit Jain <radical@gmail.com>
Date:   Fri Jul 12 00:34:04 2024 -0400

    fix archive path for e2e

commit da382ff
Merge: 18b92c3 de35684
Author: Ankit Jain <radical@gmail.com>
Date:   Fri Jul 12 00:12:02 2024 -0400

    Merge remote-tracking branch 'origin/main' into tests-out-of-repo

commit 18b92c3
Author: Ankit Jain <radical@gmail.com>
Date:   Fri Jul 12 00:07:52 2024 -0400

    merge from playground branch

commit de35684
Author: James Newton-King <james@newtonking.com>
Date:   Fri Jul 12 09:43:58 2024 +0800

    Add support for exemplars in metrics UI (dotnet#4629)

commit e23cd4c
Merge: ef3158b 9421fc8
Author: Ankit Jain <radical@gmail.com>
Date:   Thu Jul 11 18:00:20 2024 -0400

    Merge remote-tracking branch 'origin/main' into tests-out-of-repo

commit ef3158b
Author: Ankit Jain <radical@gmail.com>
Date:   Thu Jul 11 03:27:52 2024 -0400

    wip

commit 7c61df1
Author: Ankit Jain <radical@gmail.com>
Date:   Thu Jul 11 02:48:20 2024 -0400

    fixies mores

commit d6e21ab
Author: Ankit Jain <radical@gmail.com>
Date:   Thu Jul 11 03:10:21 2024 -0400

    share targets

commit 01e419d
Merge: 0741157 421f8b1
Author: Ankit Jain <radical@gmail.com>
Date:   Thu Jul 11 03:00:16 2024 -0400

    Merge remote-tracking branch 'origin/main' into tests-out-of-repo

    # Conflicts:
    #	tests/helix/send-to-helix-inner.proj

commit 0741157
Merge: 3406514 f647a66
Author: Ankit Jain <radical@gmail.com>
Date:   Tue Jul 9 18:43:36 2024 -0400

    Merge remote-tracking branch 'origin/main' into tests-out-of-repo

commit 3406514
Author: Ankit Jain <radical@gmail.com>
Date:   Mon Jul 8 21:18:23 2024 -0400

    fix build

commit 3489ff3
Author: Ankit Jain <radical@gmail.com>
Date:   Mon Jul 8 20:23:10 2024 -0400

    fix props generation

commit 6a6315f
Author: Ankit Jain <radical@gmail.com>
Date:   Mon Jul 8 20:14:53 2024 -0400

    fix

commit 92642ea
Author: Ankit Jain <radical@gmail.com>
Date:   Mon Jul 8 19:36:17 2024 -0400

    rename

commit 4bea9f4
Author: Ankit Jain <radical@gmail.com>
Date:   Mon Jul 8 19:21:36 2024 -0400

    cleanup

# Conflicts:
#	tests/Aspire.EndToEnd.Tests/Aspire.EndToEnd.Tests.csproj
#	tests/Shared/RepoTesting/Aspire.Testing.Repo.targets
@RussKie
Copy link
Member

RussKie commented Jul 15, 2024

@RussKie any idea why it thinks that's the output directory? Is the dotnet-coverage collect step somehow setting it? Full log:

https://dev.azure.com/dnceng/7ea9116e-9fac-403d-b258-b31fcf1bb293/_apis/build/builds/2493583/logs/270

I'm pretty sure because we use 1ES.PublishPipelineArtifact@1 to explicitly publish artifacts instead of using "context":

- ${{ if ne(parameters.runAsPublic, 'true') }}:
- task: 1ES.PublishPipelineArtifact@1
displayName: Publish code coverage results
inputs:
targetPath: '${{ parameters.repoArtifactsPath }}/CodeCoverage'
artifactName: '$(Agent.JobName)_CodeCoverageResults'

@mmitche
Copy link
Member

mmitche commented Jul 15, 2024

Yeah, it's scanning any published artifacts. Part of the issue used to be (maybe still is?) that binskim didn't properly look for binaries to scan. It didn't unpack nupkgs, or deal with binaries that had embedded PDBs, etc. I think this all got fixed though.

@danmoseley
Copy link
Member Author

I'm not sure where this leaves us -- @RussKie is that something you think we should change / do you plan to? ie., how do we fix the binskim errors here.

@danmoseley
Copy link
Member Author

@joperezr, it looks like the "binskim collect" step runs 4 times, once for each publish of some kind, and code coverage is just the one I happened to see, it's not directly relevant. We don't expect it to find any dll's in the code covearge outputs of course, but it doesn't find them in anything else either. Does this mean we need to explicitly publish dlls, not just nupkg, xml etc?

Yeah, it's scanning any published artifacts. Part of the issue used to be (maybe still is?) that binskim didn't properly look for binaries to scan. It didn't unpack nupkgs, or deal with binaries that had embedded PDBs, etc. I think this all got fixed though.

I think (?) it is getting the nupkg, but still says 0 to scan.

@RussKie and I don't know how this fits together, or is normally expected to work.

Aside -- when I picked a random newer build from this pipeline, I don't see Binskim at all. What is special about this build that it has binskim collect at all?

@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants