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

Configuration to exclude packages being sent to OSS Index #35

Closed
rikkiprince opened this issue Sep 16, 2020 · 19 comments
Closed

Configuration to exclude packages being sent to OSS Index #35

rikkiprince opened this issue Sep 16, 2020 · 19 comments
Assignees
Labels
enhancement New feature or request

Comments

@rikkiprince
Copy link

What is the Feature? Please describe.
I would like a configuration option (maybe scoped to OssIndex would make sense) that allows me to list names of packages that should not be transmitted to a third party, and have that configuration acted upon. Essentially, if I provided a list of packages that we consider internal or sensitive, then NuGet Defense would not send them in the request to OSS Index.

Use Case
As a sec ops, I would like to be able to exclude certain packages being sent to OSS Index, so that commercially-sensitive package names are not sent to a third party.

We have internal packages used in the projects that we are scanning with NuGet Defense, but our legal department would like the names of those internal packages not to be sent to third party services (such as OSS Index), in case the names are commercially sensitive (e.g. "SecretNewFeaturePackage").

Describe alternatives you've considered
We have considered just disabling the call to OSS Index, and just using NVD, but we're concerned that will provide a less fulsome vulnerability scan.

We have also considered excluding the internal packages manually, by removing them from the project before scanning. However, we would like to integrate this into our CI/CD pipeline, so the security scan will run automatically on new commits.

Additional context
We very much appreciate your work on NuGet Defense. We were using SafeNuGet but had similar concerns to you about it being out of date. We were very excited to see you had created NuGet Defense!

@rikkiprince rikkiprince added the enhancement New feature or request label Sep 16, 2020
@digitalcoyote
Copy link
Owner

digitalcoyote commented Sep 16, 2020

I've actually got something planned for this, but I didn't want to put it in unless someone actually requested it to avoid complicating configuration even more.
OSS Index is currently the only RemoteVulnerabilitySource that has been made. I intend to add others eventually (considering vulndb). All of these will depend on a list of packages that are sensitive.

I've got a couple issues that came up today ahead of this one, but I'll try to get to this by this weekend.

Do you have a suggested name for this list of packages that won't be sent it RemoteVulnerabilitySources?

Additionally, do you have any issues with the dependencies getting sent to a RemoteVulnerabilitySource?

@rikkiprince
Copy link
Author

Hi @digitalcoyote, thanks for the quick response!

We very much appreciate you taking a look at this request.

Naming is hard, and there's never an answer that satisfies everyone! I would lean towards something like SensitivePackageNames, ExcludedPackages, ExcludeFromScan or DoNotSendList. I think it's up to you to pick something that's consistent with your other naming, and as long as it's documented clearly, it'll be fine.

@rikkiprince
Copy link
Author

Regarding sending dependencies to the RemoveVulnerabilitySource, I don't quite understand how they are calculated? From a single package name, how do you know its dependencies? If you could explain that to me, then I think I can answer your question.

@digitalcoyote
Copy link
Owner

digitalcoyote commented Sep 16, 2020

This is actually something introduced recently. For newer sdk style projects (.net core or .net 5+ use this by default) it uses dotnet list packages --include-transitives to get the dependencies for the project that are not directly referenced based on the target framework. For the legacy .Net Framework (4.8 and below would have started with this) I believe these dependencies are directly referenced in the packages.config file (dotnet list doesn't work for legacy style projects anyway).

There is an undocumented option that allow disabling this check (I'm working on documentation in the evenings right now, but I don't have it updated for 1.0.8 yet).

https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-list-package

And if you are interested, the code used for this is in NuGetDefense.Core.NuGetFile. It's currently a bit buggy since I didn't consider non-english languages being used by the sdk (will be fixed in the next release).

@digitalcoyote
Copy link
Owner

Starting on this tonight, but I wouldn't expect a prerelease build until tomorrow as it's fairly late here.

@digitalcoyote
Copy link
Owner

NuGetDefense.1.0.9-pre0004.nupkg.zip

I'd advise you to test this with something like fidder (and a non-sensitive package) prior to using it. I'll try to look up a vulnerable package for the tests tomorrow, but I walked through the code to verify it was correctly excluding a package on my setup.

@digitalcoyote digitalcoyote self-assigned this Sep 19, 2020
@rikkiprince
Copy link
Author

Thanks @digitalcoyote for the rapid turn around. I will make sure I test this tomorrow morning and get back to you ASAP!

@digitalcoyote
Copy link
Owner

I left to out of 1.0.9 in case you needed any changes to configuration. If you need it pushed to NuGet.com first, I can release it as 1.0.10 as soon as you are ready.

@rikkiprince
Copy link
Author

Ok, I've just been testing this, but I can't get it to work. I'm sure I've just got the configuration or something?

I have intentionally added bootstrap@3.0.0 to a test .NET Core WebAPI project because I know it has vulnerabilities:
image

Based on what I saw in the test in your commit, I added bootstrap to the configuration file in the array SensitivePackages:
image

But when I run a clean and a build, Fiddler tells me that it's still sending the boostrap package to ossindex.sonatype.org:
image
image

I also tried putting:

  • bootstrap@3.0.0
  • nuget/bootstrap@3.0.0
  • pkg:nuget/bootstrap@3.0.0
    in SensitivePackages but those didn't work either.

Your code looks sensible, but I think what I'm missing from my mental model is what format is the name of the package in?

What should the format of the bootstrap package name be to get it to exclude it?

Or am I not running the package properly? The csproj seems to have the right package in it ("NuGetDefense" Version="1.0.9-pre0004"). Is that change definitely compiled into that package you shared?

@digitalcoyote
Copy link
Owner

digitalcoyote commented Sep 23, 2020

I'm going to do a deeper dive into his myself now that I have some time. I need to double check OSSIndex authentication in fiddler anyway. Hopefully I'll have a better answer by morning.

It was intended that it finds any package whose ID is in the list and removes it before sending it up.

@digitalcoyote
Copy link
Owner

digitalcoyote commented Sep 23, 2020

I can only imagine I gave you a bad build of that to test.

In the I blocked 3 packages on the second run in the image below and they were successfully removed from the OSSIndex request.
image

Heres a newer build (Includes optional authentication with OSSIndex that I'm still testing as well).
NuGetDefense.1.0.10-pre0002.nupkg.zip

@digitalcoyote
Copy link
Owner

digitalcoyote commented Sep 23, 2020

Since it seems to be working in my tests (Windows 10 and Linux Mint), I'm going to go ahead and push this out in 1.0.10 with OSS Index Authentication(#32) .

@rikkiprince I'll be leaving this open in case you have any problems or need any changes to fit your use-case.

Until this issue is closed, this functionality should be treated as a pre-release of this feature as it may change in the next version based on any discussion that follows. If anyone has any preferences, now is the time to state them.

@rikkiprince
Copy link
Author

Ok, that seems to work great, thanks @digitalcoyote. On reflection, I might have been installing the package wrong, so I'm sorry about that.

Now I've seen it in action, the only other element of this feature I would suggest adding might be to support wildcards. That way I could specify MyCompany.* and capture all internal packages (as long as I named my package that way). I would be happy for you to close this request and release it, and I could open a new feature request for the wildcarding, if you'd prefer?

@digitalcoyote
Copy link
Owner

As much as I'm tempted to make it Regex, I think your suggestion will serve the purpose without requiring a reference to configure Sensitive Packages. I'll start on this when I get a chance.

@rikkiprince
Copy link
Author

Yeah, I feel your temptation, but regex is probably overkill 🤣

@rikkiprince
Copy link
Author

Do you want me to create a separate issue for that request?

@digitalcoyote
Copy link
Owner

digitalcoyote commented Sep 24, 2020

I've pushed up a commit I believe should do the job. I'm too tired to test it properly, so I'll work on that either tomorrow morning or after work.

Do you want me to create a separate issue for that request?

I think this is a reasonable scope increase. If all internal/sensitive packages start with the same name (or at least follow a pattern that a wildcard can handle), it could help to keep one from slipping through by accident

@digitalcoyote
Copy link
Owner

@rikkiprince Wildcard functionality has been released in v1.0.11, Let me know if it gives you any issues or needs changes.

@rikkiprince
Copy link
Author

This is awesome! Thank you so much @digitalcoyote. I've tested with v1.0.11 and it works great. The wildcard is lovely. I've verified by watching the traffic in Fiddler. This all seems good to me, and you can definitely close the issue when you're ready.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

2 participants