Distressing proliferation of ambient authority

[erights]

A simple search

shows 26 modules (!!) in agoric-sdk importing from 'fs'; after filtering out tests and demo code. What other sources of ambient authority are we using? How should we detect/prevent/warn on these by default? How should we override that default for the (should be) rare resource module that should directly access ambient authority?

Attn @dckc

[dckc]

I'd like to see "do not export powerful objects" on a code review checklist. It's one thing to import fs in a rollup config; that's more or less unavoidable. But it's quite another for packages / modules that we develop to export objects that close over fs.

Do we have a code review checklist? By way of example: https://informatics.kumc.edu/work/wiki/CodeReviewNotes

For cleaning up existing code, lavamoat would let us detect use of ambient authority in many / most cases, I suppose.

Strictly speaking, I don't think this constitutes a bug, since I don't see any incorrect observable behavior identified. In KUMC Informatics, we would label this as technical-debt and review the list of such issues every few weeks in a QA meeting. Making time to pay off technical debt in the schedule when it competes with observable bugs and features is a challenge, but one that healthy teams eventually deal with.

[katelynsills]

For what it is worth, the deploy scripts are taking local files and bundling and deploying them, so they must have the ability to read parts of the filesystem.

[dckc]

Yes, but shouldn't that access to the filesystem be passed in explicitly? I don't see any particular reason why deployApi, which is exported, should close over ambient access to fs.promises.writeFile. It seems straightforward to pass writeFile (or writeDefaultFile) in, just like { bundleSource, pathResolve } are passed in.

[erights]

Hi @dckc so far you've been our most effective conscience getting us to clean up our act on this. Ok if I assign it to you? Not that I'm asking you to do this but rather to get people to do this for the code they're responsible for.

[dckc]

If you'll clarify the "finish line" a bit, I'm fine if you assign it to me.

Above I proposed "do not export powerful objects"; is that a good target / finish line?

[erights]

If someone is just using an ambient fs power to do something narrow, then they aren't exporting anything powerful. But the least authority they seem to require is immensely more that the least authority they actually require. Also, they should generally accept authority by parameters, not by ambient use. You know all this and have already been reacting on all these dimensions. Please keep doing so.

The finish line is when you've sufficiently habituated everyone else that we are all sensitive to it and reliably complain about it in code reviews. I don't know how to fit that into the goal of closing an issue, but it is what we must do. Adding @rowgraus to the assignees for that process issue. Suggestion: you coordinate with @rowgraus . You get all this started until he feels like he can take the process forward from there.

@dckc @rowgraus reasonable?

[dckc]

The target you're after is reasonably clear, @erights . I'm ok to aim for it.

@rowgraus you're welcome to help, but it seems a bit of an odd match for your role, to me.

[erights]

@rowgraus you're welcome to help, but it seems a bit of an odd match for your role, to me.

I leave that to the two of you.

[erights]

We really need to do an inventory of all the uses of ambient authority scattered through our code. Since this bug was filed, it seems the issue has only been getting worse, not better. For one, just redoing the query that started this thread, we've gone from 33 scattered uses of from 'fs'; to 39.

[erights]

While I've been worrying about and asking for feedback on how to thread configuration options to lockdown endojs/endo#578 , it seems we've been pervasively ignoring the underlying hazard and just passing environment variables around everywhere. In fact, it is worse. The following screenshot is of uses of process.env. All of these use process as a global variable. None of these have anything like a /* global process */ declaration at the top of the file. Despite this absence, none of their uses of process is shown in vscode with a red squiggle underline and none generates a warning. In fact, the only such declaration I could find was in a test file, which was therefore excluded from that search. test-node-version.js begins with the peculiar text

// eslint-disable-next-line no-redeclare
/* global process */

indicating that eslint considers this declaration a redeclaration. Where is the base declaration and how do we turn it off?

[erights]

One lesson I learned at Google is that an organization is much more likely to make incremental progress to a goal if it can turn it into a metric. We've been planning for a closer integration with LavaMoat anyway. It seems we need to do this sooner rather than later so that we can track all of these proliferating uses of ambient authority, understand where they all are, take inventory, identify the worst offenders to fix quickly, and then start making incremental but rapid progress on reducing the remainder.

[erights]

Hi @kumavis we need to start integrating LavaMoat into our own development process. In its absence, we've let uses of ambient authority proliferate.

[erights]

This concern is already on our https://github.com/Agoric/agoric-sdk/wiki/Code-Review-Checklist . We need to start applying it.

[erights]

Btw, I have made this mistake too. fatal-assert.js uses process as an ambient global, and a source of much more dangerous authority than process.env --- process.abort and process.exit. And it has no /* global comment or anything else to flag WARNING WARNING DANGER DANGER THERE BE DRAGONS HERE. Without tool support or any best practices around this issue, we'll all be too sloppy.

[erights]

[michaelfig]

I recognize the danger of ambient authority. IIUC, many uses of it just need to be moved up to the script main entrypoint and propagated explicitly to other modules.

We should have conventions for that entrypoint's name, such as rollup.config.js, packages/*/test/**/test-*.js, packages/*/scripts/*, and packages/*/bin/*. If you ignore those patterns, how many uses are left?

[dckc]

In endojs/endo#715 (comment) @erights writes re import fs:

We should adopt some convention that we can check, for making sources of static authority stand out in our source files.

I was thinking this is infeasible to check, but that's pretty much exactly what LavaMoat does, isn't it?

Anybody got a pointer to how? @kriskowal @kumavis ?

[dckc]

@warner writes in #3207 (comment) :

I don't know where to draw the line between "this module expects to run in the start compartment and can thus import or globalThis.foo anything it needs", and "this module is pure and the caller must do import/etc and give it the following N things".

A "main" that should execute in a scope with lots of authority is indicated by #!/usr/bin/env node at the top (and +x file permissions). A test script is named test-*.js. No modules should export powerful objects (for example, objects that close over fs). Note that tests don't export anything.

I'm not sure to what extent the team has adopted this rule, but @erights writes Mar 7 in #2160 (comment) :

This concern is already on our https://github.com/Agoric/agoric-sdk/wiki/Code-Review-Checklist . We need to start applying it.

He also notes lack of tool support in #2160 (comment) and utility of metrics to track progress in #2160 (comment)

I hope to try out lavamoat to establish a metric.

[dckc]

An interesting tool to look for global mutable state is Moddable's mutabilities:
https://github.com/agoric-labs/xsnap-pub/blob/master/xsnap/documentation/lockdown.md#mutabilitieso
https://github.com/agoric-labs/xsnap-pub/tree/master/xsnap/examples/lockdown 3fc543d
on Mar 22, 2022

It's currently commented out, but that's easy to address if you want to play around with it a bit:

diff --git a/xsnap/sources/xsnap.c b/xsnap/sources/xsnap.c
index 6389db2..4589083 100644
--- a/xsnap/sources/xsnap.c
+++ b/xsnap/sources/xsnap.c
@@ -375,14 +375,14 @@ void xsBuildAgent(xsMachine* machine)
        modInstallTextEncoder(the);
        modInstallBase64(the);
 //     
-//     xsResult = xsNewHostFunction(fx_harden, 1);
-//     xsDefine(xsGlobal, xsID("harden"), xsResult, xsDontEnum);
-//     xsResult = xsNewHostFunction(xs_lockdown, 0);
-//     xsDefine(xsGlobal, xsID("lockdown"), xsResult, xsDontEnum);
-//     xsResult = xsNewHostFunction(fx_petrify, 1);
-//     xsDefine(xsGlobal, xsID("petrify"), xsResult, xsDontEnum);
-//     xsResult = xsNewHostFunction(fx_mutabilities, 1);
-//     xsDefine(xsGlobal, xsID("mutabilities"), xsResult, xsDontEnum);
+       xsResult = xsNewHostFunction(fx_harden, 1);
+       xsDefine(xsGlobal, xsID("harden"), xsResult, xsDontEnum);
+       xsResult = xsNewHostFunction(xs_lockdown, 0);
+       xsDefine(xsGlobal, xsID("lockdown"), xsResult, xsDontEnum);
+       xsResult = xsNewHostFunction(fx_petrify, 1);
+       xsDefine(xsGlobal, xsID("petrify"), xsResult, xsDontEnum);
+       xsResult = xsNewHostFunction(fx_mutabilities, 1);
+       xsDefine(xsGlobal, xsID("mutabilities"), xsResult, xsDontEnum);
 
        xsEndHost(machine);
 }

[kumavis]

could make an eslint rule that gets angry when you import builtin modules

[turadg]

could make an eslint rule that gets angry when you import builtin modules

+:100: to automating whatever the policy is. We don't even need a new rule if we're content with listing the most problematic: https://eslint.org/docs/latest/rules/no-restricted-imports

Then we can use the tooling to partition paths in which is really is a problem to import these from those were it's not. E.g. CLI tools.

[dckc]

LavaMoat-like techniques are starting to look feasible: LavaMoat/LavaMoat#228 (comment)

here's hoping for time to dig in more...