-
Notifications
You must be signed in to change notification settings - Fork 131
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
Security Fixes #226
Security Fixes #226
Conversation
Removing NPM from the image since it's not necessary and it becomes vulnerable very often. Updating node alpine image, so it will have newer yarn version (not vulnerable). List of Vulnerabilities getting fixed: - https://nvd.nist.gov/vuln/detail/CVE-2020-8131 - https://nvd.nist.gov/vuln/detail/CVE-2020-8116 - https://nvd.nist.gov/vuln/detail/CVE-2019-10773
@rafael-ladislau ah nice idea to run Have you inspected other images from other Jupyter repos? I'm asking as I find this security work very valuable and would like to learn about it in general. |
FWIW, removing npm from the image doesn't offer any security improvements, because npm is not used when the container is running. |
Reading in https://nvd.nist.gov/vuln/detail/CVE-2020-8131's referenced https://hackerone.com/reports/730239:
I can't get this fully as I'm unsure about the coupling of npm / yarn for example, but I speculate that the key issue may be the act of having used npm to install, rather than having it installed then. Having it installed may been what made us able to detect the vulnerability by the use of some tool? @minrk do you suggest any action point for this repo with regards to having/not having npm and/or using/not using npm to install chp? |
Removing it from the image doesn't change anything. We should definitely use npm to install CHP in the image. If there's anything to do, it's probably to make sure that npm itself is up to date, as we generally do with pip. I would expect that keeping the node image itself up-to-date is enough to accomplish this. The main task I believe here is to:
|
@rafael-ladislau is there a specific reason that having unused files in an image still violates security rules? I've reverted the removal of npm in #228 since it does nothing to the vulnerability of the image, but we can keep it if there really is a reason for it. |
Hello @minrk, as you can see here: npm/cli#782, It's not an easy solution in the NPM side. But we have the concept of Keeping the attack surface as small as possible. (https://en.wikipedia.org/wiki/Attack_surface), that we should apply to the docker images. Of course, NPM alone is not a significant threat. Still, together with other risks that you can even not know about, it can be a problem, so if it's not necessary, and you want to follow this concept of keeping the attack surface as small as possible, you should remove it. But I think I was wrong about the npm audit fix. It changes the dependencies, and it should be part of your development cycle, it should not be a command in the Dockerfile. As we are following the FedRamp procedures here in our environment, we need to scan our environment very often, and we have a deadline to solve the vulnerabilities we find. If you open the vulnerabilities, you will see they are marked as critical, and it means I need to address them in 7 days., I can try to explain it's not a significant threat, but as I said, someone can always argue that together with other vulnerabilities, it can become a considerable threat. |
Hello, it's me again. As you know, I'm running a FedRamp environment. And we've detected some vulnerabilities in the Configurable-Http-Proxy image. We've fixed the issues, and we are requesting the merge of the patch.
We are removing NPM from the image since it's not necessary, and it becomes vulnerable very often. We are also updating the Node Alpine image so that it will have the newer yarn version (not vulnerable).
List of Vulnerabilities getting fixed:
If you accept the image, could you also build a new image and publish it in your public image repository?