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 osx-x64 release #576

Merged
merged 23 commits into from
May 19, 2022
Merged

Add osx-x64 release #576

merged 23 commits into from
May 19, 2022

Conversation

marmegh
Copy link
Contributor

@marmegh marmegh commented Feb 16, 2022

Addressing issue #554
@jeffersonking, recommended adding osx-x64 specific release binaries per the dotnet documentation on runtime identifier (RID).

@eddynaka and @michaelcfanning, still need to get the result tested on mac to see if this resolves the issue. Will update accordingly and publish for formal review once this step is complete. Feel free to add any feedback before this.

image

Results of test run (verify code signing and codesign validation steps, disabled publish steps):
image

@eddynaka
Copy link
Contributor

@marmegh ,

The change looks ok but we should test it.
My suggestion is: update the yaml pipeline to test on a mac. using the command line, you would be able to verify how to run it and analyze binskim itself.

@michaelcfanning
Copy link
Member

michaelcfanning commented Apr 25, 2022

@eddynaka, can you help make your proposed pipeline change, it's an excellent suggestion.


In reply to: 1108956840


In reply to: 1108956840

@eddynaka
Copy link
Contributor

Sorry for the delay.
I was able to produce a few changes in some files to test during the build pipeline.


In reply to: 1108956840

ado-build.yml Outdated
@@ -32,3 +27,23 @@ jobs:
script: 'BuildAndTest.cmd'

- task: ComponentGovernanceComponentDetection@0
inputs:
ignoreDirectories: '..\src\sarif-sdk'
Copy link
Contributor

@eddynaka eddynaka Apr 29, 2022

Choose a reason for hiding this comment

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

we already have component governance in sarif-sdk. We don't need to analyze here as well.

So, let's ignore the entire submodule. #Closed

displayName: 'Run BinSkim'
inputs:
targetType: 'inline'
script: 'dotnet bld/bin/x64_Release/netcoreapp3.1/binskim.dll analyze src/Test.FunctionalTests.BinSkim.Driver/BaselineTestData/macho.*'
Copy link
Contributor

@eddynaka eddynaka Apr 29, 2022

Choose a reason for hiding this comment

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

dotnet bld/bin/x64_Release/netcoreapp3.1/binskim.dll

we should add this in some doc.
the user would need to execute:
dotnet binskim.dll analyze .... #Resolved

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

I see one candidate place,
at the bottom of the readme.md
we currently have:

Example: binskim.exe analyze c:\bld\*.dll --recurse --output MyRun.sarif

I think we can change it to 3 OS version example here.

we can also consider if want to include the path in the example to make it more clear, because the previous step is unzip.

Copy link
Collaborator

Choose a reason for hiding this comment

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

more info fyi
I found that we have this file "BinSkim" in the package
./BinSkim analyze
I just tried works in Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shaopeng-gh, could you clarify where you found this file?

Copy link
Collaborator

Choose a reason for hiding this comment

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

download package from
https://www.nuget.org/packages/Microsoft.CodeAnalysis.BinSkim/
(which is the result of our build and package)
unzip
go to folder \tools\netcoreapp3.1\linux-x64
file "BinSkim" without ext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies. I was confused since this change is specific to osx. You were being helpful, I was concerned you were telling me we had a binary for linux in the wrong location.

Copy link
Collaborator

Choose a reason for hiding this comment

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

macOS also have a same file in same location, after your change:
\tools\netcoreapp3.1\osx-x64\BinSkim

I point out in the case we want to add doc for user
see if we want to say use
./BinSkim analyze
or
dotnet binskim.dll analyze

Copy link
Contributor

Choose a reason for hiding this comment

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

When I created this, I didn't want to change the current directory that we are right now. So, it is why we have the full path.

But, in the end, it is the same as dotnet binskim.dll analyze.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mary also updated the readme showing where the files will be. so, I think, Shaopeng's suggestion is to add the dotnet binskim.dll analyze as a new step in the readme

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, Shaopeng's suggestion is to add the dotnet binskim.dll analyze as a new step in the readme
------ yes, I think it is not necessary, it should be clear enough, just throwing ideals, let me close this comment.

@@ -65,7 +65,11 @@ public void AnalyzeCommand_ReadSarifLog_ShouldBeAbleToReadCurrent()
[Obsolete]
public void AnalyzeCommand_Hashes_ShouldUpdateDataToInsert()
Copy link
Contributor

@eddynaka eddynaka Apr 29, 2022

Choose a reason for hiding this comment

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

AnalyzeCommand_Hashes_ShouldUpdateDataToInsert

This test were failing in the unix pipelines. With the changes below, i was able to make it pass. #Closed

@eddynaka
Copy link
Contributor

FYI, the windows pipeline isn't working properly.
I opened an issue in the ComponentGovernance team since its throwing an exception.


In reply to: 1113754507

@eddynaka
Copy link
Contributor

For the point above, i was able to fix it already. :)


In reply to: 1113758229

@marmegh marmegh marked this pull request as ready for review May 10, 2022 17:52
@marmegh marmegh requested a review from michaelcfanning as a code owner May 10, 2022 17:52
README.md Outdated
@@ -22,9 +22,9 @@ This repository contains the source code for BinSkim, a Portable Executable (PE)
### How to extract the exe file from the nuget package
If you only want to run the Binskim tool without installing anything, then you can
1. Download BinSkim from **[NuGet](https://www.nuget.org/packages/Microsoft.CodeAnalysis.BinSkim/)**
2. Rename the file extension from .nupkg to .zip
2. Rename the file extension from .nupkg to .zip (ie. via commandline: `rename microsoft.codeanalysis.binskim.x.y.z.nupkg microsoft.codeanalysis.binskim.1.9.4.zip`)
Copy link
Contributor

@eddynaka eddynaka May 10, 2022

Choose a reason for hiding this comment

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

1.9.4

update this to x.y.z #Resolved

@@ -5,6 +5,7 @@
* Bump ELFSharp from 2.13.2 to 2.14.0. [#628](https://github.com/microsoft/binskim/pull/628)
* Bump System.Reflection.Metadata from 5.0.0 to 6.0.1 and System.Collections.Immutable from 5.0.0 to 6.0.0. [#605](https://github.com/microsoft/binskim/pull/605)
* Bump ELFSharp from 2.14.0 to 2.15.0. [#631](https://github.com/microsoft/binskim/pull/631)
* BUGFIX: Unblock running binskim on MacOS. [#576](https://github.com/microsoft/binskim/pull/576)
Copy link
Contributor

@eddynaka eddynaka May 10, 2022

Choose a reason for hiding this comment

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

BUGFIX

not sure if this is considerable a bugfix.
maybe feature: enable BinSkim for MacOS. or something like that. #Resolved

Copy link
Contributor

@eddynaka eddynaka left a comment

Choose a reason for hiding this comment

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

:shipit:

@@ -5,6 +5,7 @@
* Bump ELFSharp from 2.13.2 to 2.14.0. [#628](https://github.com/microsoft/binskim/pull/628)
* Bump System.Reflection.Metadata from 5.0.0 to 6.0.1 and System.Collections.Immutable from 5.0.0 to 6.0.0. [#605](https://github.com/microsoft/binskim/pull/605)
* Bump ELFSharp from 2.14.0 to 2.15.0. [#631](https://github.com/microsoft/binskim/pull/631)
* FEATURE: Enable BinSkim for MacOS. [#576](https://github.com/microsoft/binskim/pull/576)
- Bump Sarif.Sdk by updating submodule from [4e9f606 to fc9a9df](https://github.com/microsoft/sarif-sdk/compare/4e9f606bb0e88428866e253352cdc70dc68f98cb...fc9a9dfb865096b5aaa9fa3651854670940f7459). [#638](https://github.com/microsoft/binskim/pull/638)
Copy link
Collaborator

@shaopeng-gh shaopeng-gh May 19, 2022

Choose a reason for hiding this comment

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

looks like a typo should be * ? #Resolved

@@ -1,11 +1,12 @@
#!/bin/bash
Copy link
Collaborator

@shaopeng-gh shaopeng-gh May 19, 2022

Choose a reason for hiding this comment

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

there is also a file "UpdateBaselines.sh" which was for Linux only, not sure if we want to update that. #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the proposed change? I don't see any reference in that file to BinSkimLinux.sln or the uname variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh because I see this file is running under folder /netcoreapp3.1/linux-x64/
I don't think we need another one running for macOS.

Copy link
Collaborator

@shaopeng-gh shaopeng-gh left a comment

Choose a reason for hiding this comment

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

:shipit:

@marmegh marmegh enabled auto-merge (squash) May 19, 2022 23:26
@marmegh marmegh merged commit d107b63 into main May 19, 2022
@marmegh marmegh deleted the users/marmegh/addMacRelease branch May 19, 2022 23:37
@marmegh marmegh mentioned this pull request Jun 21, 2022
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.

4 participants