Skip to content

Remove cloaked binaries #18563

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

Merged
merged 16 commits into from
Mar 22, 2024
Merged

Conversation

ellahathaway
Copy link
Member

@ellahathaway ellahathaway commented Feb 7, 2024

Related to dotnet/source-build#4087

This Pull Request implements several changes to handle cloaked binaries as part of dotnet/source-build#4088. The changes in this PR are as follows:

  • Uncloaks all binaries that are not part of non-OSS licenses. This is needed to accomplish building product from a single branch.
  • Updates allowed-binaries.txt to be more exclusive. This is needed because otherwise broad exclusion patterns may detect non-allowed SB binaries as "allowable". For example, when **/Test/**/* was present in the allowed list, src/cecil/rocks/Test/Resources/assemblies/decsec-att.dll was considered "allowed" in the VMR for source build.
  • Includes disallowed-sb-binaries.txt and integrates disallowed-sb-binaries.txt into prep. The list of detected disallowed source-build binaries by pattern can be found here.

License scan run with all cloaks removed (internal link).

Run of binary tooling with current changes (internal link)

@ellahathaway ellahathaway marked this pull request as ready for review February 8, 2024 19:26
@ellahathaway ellahathaway requested a review from a team as a code owner February 8, 2024 19:26
@ellahathaway ellahathaway requested a review from a team February 8, 2024 21:25
@jkotas
Copy link
Member

jkotas commented Feb 9, 2024

I think we should be excluding the entire directories with binary test assets instead of individual files to make things simpler. I have not commented on all instances.

@jkotas
Copy link
Member

jkotas commented Feb 9, 2024

In dotnet/source-build#4088, we have said that we will allow binaries in VMR repo and provide a script to delete them before source build. Is the idea that these exclusions are eventually going to drive the script to delete the binaries and not what is actually included in the VMR repo?

@ellahathaway
Copy link
Member Author

I think we should be excluding the entire directories with binary test assets instead of individual files to make things simpler. I have not commented on all instances.

The initial course of action was to start narrow and expand the includes if the feedback suggested so. Given what's been suggested, I've expanded the scope in 67c4170

@MichaelSimons
Copy link
Member

@ellahathaway - If you haven't already, please run the License Scan tests against these changes prior to merging.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

LGTM but I'm not up-to-date on the requirements / goals. Is it still a goal for repos to eventually be able to allow new binaries directly within their source, i.e. via a allowed-binaries.txt file in their eng folder?

@ellahathaway
Copy link
Member Author

Is it still a goal for repos to eventually be able to allow new binaries directly within their source, i.e. via a allowed-binaries.txt file in their eng folder?

It's a possibility. The tooling as is currently prevents new binaries at the product level, but based on some changes described in dotnet/source-build#4219, specifically the change specifying the ability to "import" a file, I foresee it being possible to add this tooling at the individual repo level if the VMR file can "import" the repo files.

@ellahathaway ellahathaway marked this pull request as draft March 12, 2024 22:35
@ellahathaway
Copy link
Member Author

Converting to a draft in favor of completing dotnet/source-build#4219 first.

@MichaelSimons
Copy link
Member

Is it still a goal for repos to eventually be able to allow new binaries directly within their source, i.e. via a allowed-binaries.txt file in their eng folder?

One problem I see is that the repo's don't know about the cloaks. To do binary detection, you would have to move the allowed-binaries and cloaking definitions to the repo-level and then the vmr syncing/binary tooling would have to aggregate them.

cc @premun

@premun
Copy link
Member

premun commented Mar 13, 2024

Is it still a goal for repos to eventually be able to allow new binaries directly within their source, i.e. via a allowed-binaries.txt file in their eng folder?

One problem I see is that the repo's don't know about the cloaks. To do binary detection, you would have to move the allowed-binaries and cloaking definitions to the repo-level and then the vmr syncing/binary tooling would have to aggregate them.

cc @premun

@MichaelSimons can we discuss in one of the meetings? I am unclear on some things and the plan for the future.

@ellahathaway ellahathaway marked this pull request as ready for review March 20, 2024 22:19
Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

One small request. Rest LGTM

@ellahathaway
Copy link
Member Author

Waiting to merge based on license scanning results running on the main-ub branch.

@MichaelSimons
Copy link
Member

Waiting to merge based on license scanning results running on the main-ub branch.

Why do we care about it running on the main-ub branch? I would expect it should be queued on your PR branch.

Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

I approve pending a clean license scan w/these changes.

@ellahathaway
Copy link
Member Author

ellahathaway commented Mar 21, 2024

Waiting to merge based on license scanning results running on the main-ub branch.

Why do we care about it running on the main-ub branch? I would expect it should be queued on your PR branch.

I had originally assumed that the cloaked files would flow into the VMR automatically when the source-mappings was updated. I've since discovered that this is not the case and that the files have to be added back manually. As a result, I've rerun the license scanner with the cloaks from this PR added back into my local branch: https://dev.azure.com/dnceng/internal/_build/results?buildId=2411055&view=results

Edit: scan results match those currently at the tip of main, so I'm going to merge.

@ellahathaway ellahathaway merged commit 3a61950 into dotnet:main Mar 22, 2024
@ellahathaway ellahathaway deleted the remove-binary-cloaks branch March 22, 2024 00:07
MichaelSimons pushed a commit to dotnet/dotnet that referenced this pull request Mar 22, 2024
Adds back cloaked binaries and other cloaked files removed in
dotnet/installer#18563
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.

7 participants