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

BUG - Pinned-Dependencies should not look into vendor directories #1095

Open
naveensrinivasan opened this issue Oct 1, 2021 · 14 comments
Open
Labels

Comments

@naveensrinivasan
Copy link
Member

naveensrinivasan commented Oct 1, 2021

Describe the bug
go run . --repo=github.com/hyperledger/fabric --checks=Pinned-Dependencies --show-details

vendor/github.com/nxadm/tail/Dockerfile:
vendor/github.com/nxadm/tail/Dockerfile:                                                                                        
'go get -v github.com/nxadm/tail'
@naveensrinivasan naveensrinivasan added the kind/bug Something isn't working label Oct 1, 2021
@naveensrinivasan
Copy link
Member Author

@laurentsimon FYI...

@laurentsimon
Copy link
Contributor

laurentsimon commented Oct 1, 2021

is there value in alerting repo owners that some code in their repo downloads software unpinned, even if it's in vendor/ directory? Wdut?

Instead of disabling the check for vendor directories, an alternative would be to provide an option to configure which directories to be discarded. The main issue with this is that the default run we make (and made public via BQ table) would not have have the option turned on based on each repo's preference.

An alternative solution could be to add this in the policy files. This way, a result would contain a universal/canonical/ground truth result, and anyone could apply a policy/view to the result (e.g., discard certain directories based on the policy they apply).

Maybe worth further discussion in the meeting?

@naveensrinivasan
Copy link
Member Author

Yes, I like the policy idea. It is up to the repository owner to decide what is critical for them. Let's discuss this in our meeting.

@pjbgf
Copy link

pjbgf commented Dec 3, 2021

Completely agree with the benefits of splitting the analysis between the current repository and its (vendored) dependencies, as at the moment this results in "false positives" - considering the target repository as scope of the pinned dependencies scoring.

To some extent, anything related to a project's dependencies could be better handled on a category of its own around supply chain scoring.

@laurentsimon @naveensrinivasan did this discussion happen? Was there any agreement on the way forwards?

@naveensrinivasan
Copy link
Member Author

Completely agree with the benefits of splitting the analysis between the current repository and its (vendored) dependencies, as at the moment this results in "false positives" - considering the target repository as scope of the pinned dependencies scoring.

To some extent, anything related to a project's dependencies could be better handled on a category of its own around supply chain scoring.

@laurentsimon @naveensrinivasan did this discussion happen? Was there any agreement on the way forwards?

We didn't get a chance to discuss about this.

Do you have any suggestions?

@pjbgf
Copy link

pjbgf commented Dec 3, 2021

@naveensrinivasan thank you for the quick reply.

My suggestion would be:

  1. The vendor folder to be removed from the Pinned-Dependencies checks for golang projects.
  2. Create a new check that would aggregate Dependency Violations from dependencies that are found at the BQ table.

This way the information is still available, and in both cases actionable. The latter requiring upstream contribution or the replacement of a given dependency.

@laurentsimon
Copy link
Contributor

Thanks for the suggestion. If we can easily map vendored dependencies to their original GitHub project - which I think is doable for golang projects - I think that would work. For which other languages would it work or not work?

  • C/C++ won't work, unless the vendored dependences use GitHub link?
  • ?

@pjbgf
Copy link

pjbgf commented Dec 5, 2021

I am not aware of any major language that supports vendoring quite the same way as golang.

  • .Net (C#/VB/etc): won't work as source code vendoring is not a practice generally followed/supported.
  • Java: same reason as above.

This may work for NodeJS, but I am not sure there is a specific structure (or folder name) that is used across the community.

@laurentsimon
Copy link
Contributor

thanks. So essentially we need a list of folder names to exclude (vendor, vendors, vendored, third_party, thirdparty, thirdparties, third_parties, external, externals, etc).

Is this the best heuristics we can use? Or are there other ways to do this?

cc @peterhsshin

@peterhsshin
Copy link

Yes, that's a good heuristics. Set up an environmental variable with the list of folder names or paths to exclude. The folder names vary too much to come up with an absolute list. It's best to leave it up to the users to edit/modify.

On the other hand, make sure these pinned vendor lists are not seen or checked into the public repository. It can potentially lead to security or data breaches.

@laurentsimon
Copy link
Contributor

On the other hand, make sure these pinned vendor lists are not seen or checked into the public repository. It can potentially lead to security or data breaches.

what do you mean?

@AdamKorcz
Copy link
Contributor

AdamKorcz commented Nov 16, 2023

Dockerfiles fixed by #3675 (comment)

@spencerschrock
Copy link
Member

I'm not sure this is entirely fixed. Dockerfiles are just one aspect of the Pinned-Dependencies check

@spencerschrock
Copy link
Member

The other alternative is just letting this be part of maintainer annotation feature.

cc @gabibguti (Maybe vendored is another qualifier that can be added later, similar to test data but different name)

@afmarcum afmarcum moved this to Backlog - Bugs in Scorecard - NEW Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog - Bugs
Development

No branches or pull requests

6 participants