BuildTransitive targets and BinSkim issues #7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheFieryScythe
changed the title
|
Pinging last committer for visibility. @skardile-perforce |
|
Thanks @TheFieryScythe. Noted this. Will take a look at this for the next release. |
|
The BinSkim security issues have not been resolved in the latest package. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BuildTransitive
The targets that add p4bridge.dll to the output of the consuming assembly are not present within a buildTransitive folder in the current NuGet packages. I believe this means that if p4api is a nested, transitive dependency then the targets might not get applied, resulting in the top level assembly not having p4bridge in it's output. This change adds the targets to the buildTransitive folder.
BinSkim
When Microsoft BinSkim is executed over the p4api package contents the following issues are raised:
BA2022 - SignSecurely
p4api.net and p4bridge.dll both get flagged with this issue, as a result of being signed using a SHA1 digest algorithm. SHA256 is the required minimum to be compliant. This is not a change that can fixed by GitHub PR.
BA2008 - EnableControlFlowGuard
This check requires p4bridge.dll to be compiled with the /guard:cf flags and linked with the /guard:cf and /DYNAMICBASE flags when built with MSVC. This is a mitigation against malicious code injection into switch statements. I have attempted to add these flags to the CMake config, though due to issues linking with OpenSSL locally (also without this change) I have been unable to get p4bridge to build completely.
Architecture Packages
An additional observation, the nuspecs for all packages are largely identical meaning assemblies are being included in architecture packages that will never be used. For example the x86 package includes x64 assemblies, the net.core package references and targets net framework assemblies, etc. I have not changed these nuspecs under the assumption these are for backwards compatibility. Only a single multi targeting package would be necessary otherwise. The targets for x86 also mirrors this inconsistency by having conditions for x64 and AnyCPU, and adding x64 libraries to the build output for Linux and OSX.