search

klarna / kco_rest_dotnet

[DEPRECATED] Official .NET.Core SDK library for Klarna Services
https://developers.klarna.com/api/
Apache License 2.0
20 stars 37 forks source link

Add a security.md file to repo, highlighting existing vulnerabilities #52

Closed freethan closed 4 years ago

freethan commented 5 years ago

Is your feature request related to a problem? Please describe. Currently Snyk is used to scan the repo for vulnerabilities. If these are not resolved, then we should inform the community of their existence and the reasons behind not mitigating them.

Describe the solution you'd like A security.md file describing the current vulnerabilities and the expected mitigation or reasons for lack thereof

Describe alternatives you've considered A section on the README.md

igeligel commented 5 years ago

Instead of this could we just use https://dependabot.com/?

This tool would even create automatic PRs if there are dependency upgrades. You can see it in action here: https://github.com/gothinkster/vue-realworld-example-app/pulls?q=is%3Apr+is%3Aclosed+label%3Adependencies

freethan commented 5 years ago

Hi @igeligel , the repo as well as the whole org has been setup to be scanned by Snyk, hence the warnings we get about the code and its dependencies when we log into the Snyk console.

In some cases, it's just a lo-fi way of acknowledging / clarifying why we, at the moment, have vulnerabilities and why we choose not to address them. For example, this repo has HTTP vulnerabilities introduced by the testing framework but as of the time of writing this we do not wish to force our users to upgrade to the .NET version that fixes this.

edit: Having said that, dependabot could work in a complimentary fashion, doing the chore work of offering PRs that we need to approve.

igeligel commented 5 years ago

Having said that, dependabot could work in a complimentary fashion

I think so too.

at the moment, have vulnerabilities and why we choose not to address them

To not address them sounds really wrong in my opinion. I mean the basic reason is that clients do not want to upgrade their .NET version. I know this cost time and money but can we really live with delivering security vulnerabilities? If people want to have the newest features, they should also be able to upgrade their framework. If they want to stay on an old version they are free to fork the repo at the point where the release is created.

Also, I think there is some misunderstanding here. The Core library is targeting netstandard2.0 here which means it can run in those environments:

See compatibility table

The testing library is still running on .NET standard and upgrading versions there would not break anything. Anyway, considering Snyk is testing everything in this project including the test project we should be able to tell Snyk to ignore the test project actually. I could not find a way to tell snyk to ignore the test, sample or example projects but I will reach out to the person who set those scans up and will ask there.

So right now we do not have any security vulnerability in the created package but in the development environment which is alright.

freethan commented 5 years ago

Thank you for the feedback. Allow me to firmly state the following:

In some cases, it's just a lo-fi way of acknowledging / clarifying why we, at the moment, have vulnerabilities and why we choose not to address them.

No, that does not mean that we enforce a blanket rule for not addressing vulnerabilities. In our case the vulnerability exists in the testing framework and not on the actual library we offer. I think that allows us for some room to maneuver as we chart the next steps. Having a file indicating this vulnerability and why it's not addressed yet while working out strategies to improve the library, works towards transparency.

I don't believe this PR is about "not dealing with vulnerabilities" but rather if we have to explain ourselves, we can do it with such a file and provide other users with a "heads up".

igeligel commented 5 years ago

Why should we create noise if it is not important for our customer?

We should really think about this because the security vulnerability does not affect the artifact which is the product for the customers in the end.

The normal workflow of someone wanting to integrate Klarna into their current .NET systems would be to google `klarna .net or searching on nuget directly. If there is any message of security vulnerability which just affects developing, we should not make a public statement about it and rather create an issue and leave it at that. I think this is enough transparency for every developer who wants to contribute to this repository.

Public vulnerabilities should be listed in the README.md. This will also encourage users to upgrade their versions. Nuget has the possibility to update a README.md file for a better description of packages: https://github.com/NuGet/Home/wiki/Package-README.md-support Unfortunately there is no way to bundle the markdown file currently but updating it manually would be fine enough.