Skip to content

Less scary warning in README.md?#403

Merged
kumavis merged 3 commits into
LavaMoat:mainfrom
jjlee:less-scary-warning
Nov 21, 2022
Merged

Less scary warning in README.md?#403
kumavis merged 3 commits into
LavaMoat:mainfrom
jjlee:less-scary-warning

Conversation

@jjlee

@jjlee jjlee commented Nov 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

I don't know if my change below reflects reality, but if it does, I think it would be helpful to people like me to apply some change like this to this README :-)

I'd love to use LavaMoat, and I don't think it would be a big problem for me if the situation is that it has a decent chance of foiling some attacks despite the possibility of being ineffective at isolating code against attackers able to find issues that a security audit might turn up.

Would I be correct that the situation is that LavaMoat node:

  • Is stable enough for use in production
  • Has not been security audited -- so maybe there are lurking issues that could leak authority
  • Adds some runtime performance penalty, but nothing dramatic
  • Adds a testing requirement: Code running under LavaMoat needs to be tested under it, so that any usage incompatible with SES shows up before getting to production

If so would it be helpful if I added a brief explicit statement similar to that to this PR?

@naugtur

naugtur commented Nov 14, 2022

Copy link
Copy Markdown
Member

We could explain the warning better. I don't think using the word "production" is necessary or delivering the message we intend to deliver here.
Let's iterate on the wording. I like some of what you wrote in the PR description more than the actual edit ;)

LavaMoat applies SES(which has been audited) in a framing that was not audited. It does provide protections that are very useful in defending against potential malicious dependencies. It does cause a ~2x performance penalty on globals lookup and there's a specific case of using typed arrays where the performance hit may be significant.
Compatibility is not part of this warning though.

So we'd need to say this is experimental, not audited, but works correctly and is useful already.

@jjlee

jjlee commented Nov 14, 2022

Copy link
Copy Markdown
Contributor Author

Performance/auditing: I think the content of the "LavaMoat applies" paragraph in your comment is great for potential users skimming to see if they should spend more time evaluating.

So we'd need to say this is experimental

This is a word that, by itself, is confusing to me. Nothing wrong with it being there in the warning ("this is experimental in senses x, y, z"), but your using it again just now like that makes me again think: what does he mean by "experimental?" It almost sounds like you have a specific meaning in mind for that word alone all by itself, could you clarify?

"Production": I don't mind if it doesn't go in this warning, but I do think potential users are rightly interested in what you as the developers think would be reasonable use cases right now.

Re compatibility: again regardless where it goes, I think potential users trying to rapidly assess a project are after this sort of information (and from what I remember from reading about SES as it was years ago, I suspect it's a good story given the payoff!). I personally would hope to find some info about this somewhere in the README, ideally near the top, and suspect I'm not unusual there.

@kumavis

kumavis commented Nov 16, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Lavamoat itself has actually been externally audited, but not recently

@kumavis

kumavis commented Nov 21, 2022

Copy link
Copy Markdown
Member

lavamoat has been audited (though due for another round) and is used in production at metamask. api is stable enough and follows semver. opting to remove the warning.

@kumavis kumavis merged commit 536e663 into LavaMoat:main Nov 21, 2022
@kumavis

kumavis commented Nov 21, 2022

Copy link
Copy Markdown
Member

thanks for bringing our attention to it!

legobeat added a commit that referenced this pull request May 30, 2023
@legobeat
node/version - 7.0.0
cbf12c3 
c4857f3 browserify,node,tofu,viz: bump @babel packages to latest
d557312 devDeps: eslint@^7.32.0->^8.40.0; plugins; lint:fix (#568)
93e1d2a devDeps: minor bump eslint packages (#568)
85a6ae2 core/version - 14.1.0
ccdd3d6 node/deps: yargs@16->17
73e54f6 deps: node-gyp-build@4.2.3->4.6.0
849f288 node/deps: json-stable-stringify@^1.0.1->^1.0.2
e0bf366 node/deps: remove object.fromentries (#555)
82beffa restrict engines.node to < 19.0.0
ced99e5 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/examples/express
d5de5d5 drop uneeded additionalOpts arg and migrate back into using scenario object (#471)
dc027eb core/version - 14.0.0
9a4759e node: yarn test:prep: force NODE_ENV=development for test projects setup
1de0a17 core/version - 13.0.0
23a9b06 Bump decode-uri-component in /packages/node/examples/express
ec1c9b7 Bump decode-uri-component in /packages/node/test/projects/2
d8ea54f Bump express from 4.17.1 to 4.17.3 in /packages/node/examples/express
73dec5d Bump versions for core/lavapack/browserify (#431)
28e98d4 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/test/projects/2
e12aedd lint:fix
190391a fix: remove redundant engine entries from package.json (#411)
536e663 Less scary warning in README.md? (#403)
084fbb6 Improve eslint (#409)
cee9caf Bump minimum nodejs version to 14.0.0
e90070f Fix for PR #382 [scuttling] (#391)
legobeat added a commit that referenced this pull request May 30, 2023
@legobeat
node/version - 7.0.0
d0f635f 
c4857f3 browserify,node,tofu,viz: bump @babel packages to latest
d557312 devDeps: eslint@^7.32.0->^8.40.0; plugins; lint:fix (#568)
93e1d2a devDeps: minor bump eslint packages (#568)
85a6ae2 core/version - 14.1.0
ccdd3d6 node/deps: yargs@16->17
73e54f6 deps: node-gyp-build@4.2.3->4.6.0
849f288 node/deps: json-stable-stringify@^1.0.1->^1.0.2
e0bf366 node/deps: remove object.fromentries (#555)
82beffa restrict engines.node to < 19.0.0
ced99e5 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/examples/express
d5de5d5 drop uneeded additionalOpts arg and migrate back into using scenario object (#471)
dc027eb core/version - 14.0.0
9a4759e node: yarn test:prep: force NODE_ENV=development for test projects setup
1de0a17 core/version - 13.0.0
23a9b06 Bump decode-uri-component in /packages/node/examples/express
ec1c9b7 Bump decode-uri-component in /packages/node/test/projects/2
d8ea54f Bump express from 4.17.1 to 4.17.3 in /packages/node/examples/express
73dec5d Bump versions for core/lavapack/browserify (#431)
28e98d4 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/test/projects/2
e12aedd lint:fix
190391a fix: remove redundant engine entries from package.json (#411)
536e663 Less scary warning in README.md? (#403)
084fbb6 Improve eslint (#409)
cee9caf Bump minimum nodejs version to 14.0.0
e90070f Fix for PR #382 [scuttling] (#391)
legobeat added a commit that referenced this pull request Jun 2, 2023
@legobeat
node/version - 7.0.0
5631d34 
c4857f3 browserify,node,tofu,viz: bump @babel packages to latest
d557312 devDeps: eslint@^7.32.0->^8.40.0; plugins; lint:fix (#568)
93e1d2a devDeps: minor bump eslint packages (#568)
85a6ae2 core/version - 14.1.0
ccdd3d6 node/deps: yargs@16->17
73e54f6 deps: node-gyp-build@4.2.3->4.6.0
849f288 node/deps: json-stable-stringify@^1.0.1->^1.0.2
e0bf366 node/deps: remove object.fromentries (#555)
82beffa restrict engines.node to < 19.0.0
ced99e5 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/examples/express
d5de5d5 drop uneeded additionalOpts arg and migrate back into using scenario object (#471)
dc027eb core/version - 14.0.0
9a4759e node: yarn test:prep: force NODE_ENV=development for test projects setup
1de0a17 core/version - 13.0.0
23a9b06 Bump decode-uri-component in /packages/node/examples/express
ec1c9b7 Bump decode-uri-component in /packages/node/test/projects/2
d8ea54f Bump express from 4.17.1 to 4.17.3 in /packages/node/examples/express
73dec5d Bump versions for core/lavapack/browserify (#431)
28e98d4 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/test/projects/2
e12aedd lint:fix
190391a fix: remove redundant engine entries from package.json (#411)
536e663 Less scary warning in README.md? (#403)
084fbb6 Improve eslint (#409)
cee9caf Bump minimum nodejs version to 14.0.0
e90070f Fix for PR #382 [scuttling] (#391)
legobeat added a commit that referenced this pull request Jun 2, 2023
@legobeat
node/version - 7.0.0
422f842 
c4857f3 browserify,node,tofu,viz: bump @babel packages to latest
d557312 devDeps: eslint@^7.32.0->^8.40.0; plugins; lint:fix (#568)
93e1d2a devDeps: minor bump eslint packages (#568)
85a6ae2 core/version - 14.1.0
ccdd3d6 node/deps: yargs@16->17
73e54f6 deps: node-gyp-build@4.2.3->4.6.0
849f288 node/deps: json-stable-stringify@^1.0.1->^1.0.2
e0bf366 node/deps: remove object.fromentries (#555)
82beffa restrict engines.node to < 19.0.0
ced99e5 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/examples/express
d5de5d5 drop uneeded additionalOpts arg and migrate back into using scenario object (#471)
dc027eb core/version - 14.0.0
9a4759e node: yarn test:prep: force NODE_ENV=development for test projects setup
1de0a17 core/version - 13.0.0
23a9b06 Bump decode-uri-component in /packages/node/examples/express
ec1c9b7 Bump decode-uri-component in /packages/node/test/projects/2
d8ea54f Bump express from 4.17.1 to 4.17.3 in /packages/node/examples/express
73dec5d Bump versions for core/lavapack/browserify (#431)
28e98d4 Bump minimatch from 3.0.4 to 3.1.2 in /packages/node/test/projects/2
e12aedd lint:fix
190391a fix: remove redundant engine entries from package.json (#411)
536e663 Less scary warning in README.md? (#403)
084fbb6 Improve eslint (#409)
cee9caf Bump minimum nodejs version to 14.0.0
e90070f Fix for PR #382 [scuttling] (#391)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jjlee @naugtur @kumavis