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

Enhance import-ses to sniff environment variables. #2757

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

erights
Copy link
Member

@erights erights commented Mar 30, 2021

Fixes endojs/endo#578
by surrendering to environment variables.

No automates tests, but I did hand check it. If there is a straightforward was to
add an automated test, please let me know. Otherwise let's do that in another
PR.

From the new NEWS.md:

The @agoric/import-ses package exists so the "main" of production code can
start with the following import or its equivalent.

import '@agoric/install-ses';

But production code must also be tested. Normal ocap discipline of passing
explicit arguments into the lockdown
call would require an awkward structuring of start modules, since
the install-ses module calls lockdown during its initialization,
before any explicit code in the start module gets to run. Even if other code
does get to run first, the lockdown call in this module happens during
module initialization, before it can legitimately receive parameters by
explicit parameter passing.

Instead, for now, install-ses violates normal ocap discipline by feature
testing global state for a passed "parameter". This is something that a
module can but normally should not do, during initialization or otherwise.
Initialization is often awkward.

The install-ses module tests, first,
for a JavaScript global named LOCKDOWN_OPTIONS, and second, for an environment
variable named LOCKDOWN_OPTIONS. If either is present, its value should be
a JSON encoding of the options bag to pass to the lockdown call. If so,
then install-ses calls lockdown with those options. If there is no such
feature, install-ses calls lockdown with appropriate settings for
production use.

@erights erights self-assigned this Mar 30, 2021
@erights
Copy link
Member Author

erights commented Mar 30, 2021

Note to reviewers: Be sure to check "Hide whitespace changes"

@warner
Copy link
Member

warner commented Mar 30, 2021

I suspect the environment variable is the most ergonomic approach, but I'll think out loud about the other options might be.

I see two use cases. The first is unit test programs, when I've seen a test fail, but the normal lockdown options are interfering with the output and I'm happy to turn off security for the sake of debugging. I want to temporarily change the options. It's not a big deal for me to edit a line of code, as long as I can find it, and I've already got the test program open in my editor. So changing a top-level import from one ses thing to some ses-debug thing is easy, and probably easier than an AVA argv would be (even if we could add such an argument to AVA without patching it, and even if ava --help made it obvious what to add, I'd probably forget that yarn test --newargs is how you invoke ava --newargs).

The second is production tools like ag-solo and ag-chain-cosmos. These are begging for an argv switch, because then their --help can tell you what it is. But that presumes the human deciding to change modes is also the one typing ag-solo into a shell, whereas a lot of our tooling for using those applications buries the invocation under a few layers of Makefiles and Docker images and devops deployment scripts. For the general case of building apps on top of swingset, a command-line flag is the most correct control, but for many of our current instantiations, it's not necessarily the most usable.

Plus, it's annoying to write an application that can sense an argv flag early enough to influence the modules it imports. We don't want to use the dynamic import expression, which doesn't leave a lot of ways to implement the branch in javascript. We could make the entry point (ag-solo) be a shell script, have it parse the arguments enough to figure out whether you want this --debug mode or not, and then invoke one of two different JS programs, which differ by one line (import normal-or-debug SES, then import+execute the real program). But that's super messy, doesn't let you use normal JS arg-parsing libraries, and it's hard to coordinate the shell script interpretation of the arguments and the real JS-side interpretation. You could establish a convention like "if any argv is --debug then use debug-SES", and have the JS-side library recognize-and-ignore that value, maybe strip it out of process.argv before calling such a library, but that feels fragile.

Having the imported library silently examine (and, eek, mutate) process.argv during import would remove that shell script frontend, and the two variant entry point JS files. But it would feel even more surprising.

Having the imported library silently examine process.env to sense an environment variable is easier to implement and less surprising. OTOH it doesn't provide a satisfying level of control: programs using libraries would like to control what those libraries do, instead of having the libraries go behind the caller's back for configuration data. OT3H since the humans who may find themselves wanting to flip this switch aren't necessarily running ag-chain-cosmos or ag-solo themselves (rather they're going through Makefiles and such), an environment variable will be easier for them to pass all the way through to the application. OT4H since these SES-debug settings are effectively visible to the confined JS code (by virtue of sensing how Errors behave), they're consensus-critical, and we don't want ag-chain-cosmos to be at all casual about consensus-critical configuration knobs.

On the whole, I can't think of a better way to implement this than the environment variable. The next runner up would be for a different library (ses-debug-argv-sensing or something) to read+strip a --ses-debug argument from process.argv, but it's a distant second: I don't like "mutation of argv" as a side-effect of importing a library, and for our specific use case, it's not actually very usable.

So yeah, sorry, I can't think of a good reason to talk you down from this :).

Copy link
Member

@warner warner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't think of anything better.

The only improvement I can think of, and it might be less usable so don't take it too seriously, would be to have two distinct imports. One ignores process.env, and has a short default-ish name (like install-ses). The other pays attention, and reminds the caller of that in the name (like install-ses-get-options-from-envvars or something). That might serve to remind the reader that something non-obvious is going on during the import. I can't decide if that's worth the extra length (and bikeshedding over the specific name).

@erights
Copy link
Member Author

erights commented Mar 30, 2021

Verbal discussion with @warner clarified

This change only relates to those that import '@agoric/install-ses'. It does not affect importers of install-ses-debug or of any prepare-test-env*, since those already provide lockdown options appropriate for for testing. Only production code should be importing @agoric/install-ses rather than those. The conundrum this PR addresses is only how to run production code in a way that sacrifices safety for diagnostics useful for testing and debugging. Only that conundrum justifies sniffing environment variables or global state.

Also, @warner and I agreed to go ahead with this approach but with an additional logging that such sniffing happened.

@erights erights enabled auto-merge (squash) March 30, 2021 20:14
@erights erights merged commit 6e779bf into master Mar 30, 2021
@erights erights deleted the surrender-to-env-variables branch March 30, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to thread configuration data through to lockdown
2 participants