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

Update grpc version when upstream security issue is fixed #1350

Closed
lukehoban opened this issue May 9, 2018 · 6 comments
Closed

Update grpc version when upstream security issue is fixed #1350

lukehoban opened this issue May 9, 2018 · 6 comments
Assignees
Milestone

Comments

@lukehoban
Copy link
Member

When installing 0.12.1 NPM packages on Node 8.10 on Linux, I see this:

Installing dependencies...
npm WARN notice [SECURITY] protobufjs has 1 moderate vulnerability. Go here for more details: https://nodesecurity.io/advisories?search=protobufjs&version=5.0.2 - Run `npm i npm@latest -g` to upgrade your npm version, and then `npm audit` to get more info.

> grpc@1.11.3 install /home/ec2-user/test/node_modules/grpc
> node-pre-gyp install --fallback-to-build --library=static_library

[grpc] Success: "/home/ec2-user/test/node_modules/grpc/src/node/extension_binary/node-v57-linux-x64-glibc/grpc_node.node" is installed via remote

> @pulumi/aws@0.12.1 install /home/ec2-user/test/node_modules/@pulumi/aws
> pulumi plugin install resource aws v0.12.1
@lukehoban lukehoban added this to the 0.14 milestone May 9, 2018
@swgillespie
Copy link
Contributor

This comes from our gRPC dependency, I'll see if I can bump the version as part of #1320.

@ericrudder
Copy link
Member

ericrudder commented May 10, 2018 via email

@joeduffy
Copy link
Member

I love the idea of running the new audit command in CI.

Would doing so have caught this specific issue?

@joeduffy
Copy link
Member

TL;DR, I looked into this after seeing the error pop up in @mehzer's otherwise immaculate video. Sadly, the latest grpc package, 1.11.3, which we depend on directly, has a dependency on ^5.0.0 of protobufjs. So, nothing we do can force it to accept 6.8.6, which has the patch.

I coded up the rejection of make ensures that contain npm audit failures, so I know we can trivially do this if we want. The problem that the above situation points out is that there will be cases outside of our control that inevitably mean we're stuck with failures that we can't do anything with.

I wish there was a way to only audit immediate dependencies, but that does not appear to be the case.

@ellismg and @justinvp in the meantime, should we suppress this for our installer/templates, using npm install ... --no-audit, to avoid scaring people away with the frightening red bangs?

@ellismg
Copy link
Contributor

ellismg commented May 18, 2018

It looks like grpc/grpc-node#277 tracks taking the fix into upstream.

ellismg added a commit that referenced this issue May 18, 2018
While we wait for upstream to fix the security issue, we'll just pass
`--no-audit` to `npm install` when creating a new project. Since the
warning is against an indirect dependency of @pulumi/pulumi, we can't
actually address the issue ourselves.

Mitigates #1350
ellismg added a commit that referenced this issue May 18, 2018
While we wait for upstream to fix the security issue, we'll just pass
`--no-audit` to `npm install` when creating a new project. Since the
warning is against an indirect dependency of @pulumi/pulumi, we can't
actually address the issue ourselves.

Mitigates #1350
@ellismg ellismg modified the milestones: 0.14, 0.15 May 18, 2018
@ellismg ellismg changed the title npm WARN notice [SECURITY] protobufjs has 1 moderate vulnerability. Update grpc version when upstream security issue is fixed May 18, 2018
@ellismg
Copy link
Contributor

ellismg commented May 18, 2018

I've checked in the mitigation such that pulumi new no longer causes an audit to be done. I've moved this issue to M15, and changed the title to reflect that we need to update when upstream makes a release.

We'll probably also want to remove the --no-audit flag at the same time.

ellismg added a commit that referenced this issue May 24, 2018
This version has an updated protobufs dependency, which will remove
the warnings from `npm audit`.

Fixes #1350
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants