Support configurable hooking into LavaMoat scuttling

[weizman]

Motivation

⚠️ EXPERIMENTAL - API might change in future versions!

We'd like to be able to optionally hook into the scuttling process at runtime.
It will be useful to us because it'll allow us to use Snow to perform scuttling on all new realms instead of just the main one.

This continues the work of #360 where scuttling was originally introduced

Implementation

before:

scuttleGlobalThis: <boolean>
scuttleGlobalThisExceptions: []<string>

after:

scuttleGlobalThis: {
  enabled: <boolean>,
  exceptions: []<string>,
  scuttler: <string>, // e.g. 'SCUTTLER'
}

The new option scuttler will be a string pointing to a reference on the global object, which should refer to a function:

Object.defineProperty(window, 'SCUTTLER', {
        value: (globalRef, scuttle) => {
          console.log('scuttler (start)');
          scuttle(globalRef);
          console.log('scuttler (end)');
        }
})

When scuttler is left untouched, LavaMoat runtime won't try using an external scuttler.

Visit scuttle docs for further info

[weizman]

Intro

Thinking out loud on how configuration should look like.

There are a few things to be considered when deciding on the right configuration for the motivation this PR pushes forward:

• It needs to be easy for the user to understand.
• The idea is to be able to configure whether we want Snow to automatically ship scuttling security feature to all potential child realms in the browser LavaMoat build or not.
• In the future, Snow might be used for more stuff than just scuttling.
• In the future, Snow will still only serve the browser-based LavaMoat builds.

Option 1

We could turn scuttleGlobalThis into a multi-value option to represent the 3 possible states:

• 'none'[:default] - do not activate scuttling.
• 'top-realm' - activate scuttling for the top main realm only.
• 'all-realms' - activate scuttling through Snow so it will apply scuttling to all potential child realms automatically.

Few thoughts:

• Could use some help with the naming of the fields.
• This doesn't answer our future potential need where we'll likely be using Snow for other stuff than just scuttling.
• A bit messy?
• 'all-realms' is irrelevant in non-browser-based envs.

Option 2

We could just implement the activation of Snow as a separate config field, such as useSnow:

• false[:default] - don't use Snow for anything.
• true - whenever Snow can be helpful for shipping security to child realms (such as - but not limited to - scuttling), use it.
  • Unless such certain security feature is also configurable and is turned off.

Few thoughts:

• Could use some help with the naming of the field.
• This introduces yet another config field, although boolean is less messy.

Option 3

Any other suggestions? Would love to hear them.

[naugtur]

In the future, Snow might be used for more stuff than just scuttling.
It will still be used as a mechanism to deliver changes to other realms transparently and the user should not need exposure to the word "snow" in the config. So no option2

In the future, Snow will still only serve the browser-based LavaMoat builds.
This means we want to make it less likely a Node.js user would attempt to use a feature that's irrelevant for them

For option1:

scuttleGlobalThis: [ 'no', 'yes', 'all-browser-realms' ]

But to address the browser being separate, we could do something like this:

scuttleGlobalThis: boolean,
protectBrowserRealms: [ 'scuttle', 'ban' ]

with 'scuttle' being the only option for now, but open for future expansion

[legobeat]

protectBrowserRealms: [ 'scuttle', 'ban' ]

@naugtur why ban?

@weizman In general I agree with the point raised by @naugtur to not go for 2.

If I understand it right, I'm thinking of this as if/how recursion should be done.
I also think it can make sense to make the first-level value into a dict (for extensibility without polluting the options dict).
Putting that together along the line of Option 1:

// 3.1
{
   scuttleGlobalThis: {  // you could optionally recognize a bool here, with `true` being equal to whatever defaults make sense
    enabled: bool,
    exceptions: string[], // both this and existing field would initially be recognized, dropping the old one in a future major release
    recursion: 'full' | 'none', // don't know if it would be useful to have a `number` for "max recursion depth"?
  }
}

if we want to be even more general and not fully close the door on globalThis behavior:

// 3.2
{
   scuttleGlobals: {
    enabled: string[], // default ['globalThis']
    exceptions: string[], // both this and existing field would initially be recognized, dropping the old one in a future major release
    recursion: 'full' | 'none', // don't know if it would be useful to have a `number` for "max recursion depth"?
  }
}

or even

// 3.3
{
   scuttling: {
    globalThis: { // default; keys correspond to globals
      exceptions: string[], // fall back to globalThisExceptions
      recursion: 'full' | 'none', // don't know if it would be useful to have a `number` for "max recursion depth"?
    },
    // , window: {...} etc
  }
}

Going further: Inverting it (configuring options - in this case only scuttling - per global). domain-driven rather than feature-driven configuration:

// 3.4
{
  globals: {
    globalThis: {
      scuttling: {
        enabled: bool,
        exceptions: string[], // fall back to globalThisExceptions
        recursion: 'full' | 'none', // don't know if it would be useful to have a `number` for "max recursion depth"?
    },
    // , window: {...} etc
  }
}

Some variation of any of these seems sensible to me. It's possible that 3.3-3.4 are "overkill" and solving for unneeded granularity (opening for treating different globals differently)

[weizman]

once #471 is merged, the diff in this file can be fixed back to make more sense

[legobeat]

The removal of opts.scuttleGlobalThisExceptions seems to be the only breaking part in this changeset - if it would fall back to the old field in absence of the new one, we should be able to make this non-breaking?

Looks great otherwise!

[weizman]

you mean on core or in all packages affected in this PR?

[legobeat]

All, given that the latest published release of each package exposes the option in the public API.

(In principle of course we can still consider each indepently if there would be particularities to any individual package)

[weizman]

Aren't those going to be breaking changes anyway? considering the scuttleGlobalThis option has changed

[legobeat]

The semantics don't seem to have changed? What I mean is that it would still recognize the old config, making that non-breaking (to be actually broken in a future major). Something like:

    if (opts.scuttleGlobalThisExceptions) {
      // log deprecation warning
    }
    const scuttleGlobalThis = opts.scuttleGlobalThis
    const scuttleGlobalThisExceptions = scuttleGlobalThis?.exceptions || opts.scuttleGlobalThisExceptions || []

[naugtur]

We can avoid a breaking change here, yes. It's a little ugly in code. It'd be best to not have to do that and not have to bump major. That's what marking a feature as experimental initially is great - we can make breaking changes to it and get away with it.

This feature was never documented. That means, if we document it as experimental, we could consider breaking it knowing that there was no usage beyond what we control in MetaMask. Is this a good idea? Remains to be decided.

In general, we need to set some rules. I suggest:

• Features or changes cannot be merged without complete documentation.
• A new feature, distinct from other features, should be marked as experimental initially unless a good reason not to do that is provided.
• Experimental feature must be listed as experimental in documentation but can remain undocumented in detail.

[legobeat]

In general +1 @naugtur but it might be helpful to put the emphasis on "new API or behavior" rather than "new feature" since the former is less open to interpretation (though I'm sure it's arguable that one follows the other)

[legobeat]

Can we include (vendor) snow.js inside this directory (or at appropriate place in the tree) to avoid loading from CDN here?

[weizman]

Including it will mean replacing it every time Snow updates. Or, bake it into the HTML, but that will introduce a step in the build process of the example.
We can also just demonstrate the example without Snow (after all - it is optional): 0f7a43d

[legobeat]

Suggested change

	const scuttleGlobalThis = opts.scuttleGlobalThis
	if (opts.scuttleGlobalThisExceptions) {
	console.warn('Lavamoat - "scuttleGlobalThisExceptions" is deprecated. Use "scuttleGlobalThis.exceptions" instead.')
	}
	const exceptions = scuttleGlobalThis?.exceptions || opts.scuttleGlobalThisExceptions
	scuttleGlobalThis.exceptions = exceptions
	if (opts.scuttleGlobalThisExceptions) {
	console.warn('Lavamoat - "scuttleGlobalThisExceptions" is deprecated. Use "scuttleGlobalThis.exceptions" instead.')
	}
	const scuttleGlobalThis = {
	...opts.scuttleGlobalThis,
	exceptions: opts.scuttleGlobalThis?.exceptions || opts.scuttleGlobalThisExceptions,
	}
	const exceptions = scuttleGlobalThis.exceptions

This way we leave the opts object unmodified.
I also personally find it more readable to assign all the options fields ni one statement like this, but that might be subjective.

[weizman]

706199a

[legobeat]

We should avoid modifying the input arguments (in this case scuttleGlobalThis.

[weizman]

706199a

[legobeat]

This is modifying the input (it's desirable to be pure wrt input arguments)

[weizman]

706199a

[legobeat]

See https://github.com/LavaMoat/LavaMoat/pull/462/files#r1149232521

[weizman]

706199a

[legobeat]

Follow-up: #597

[weilun0510]

Object.defineProperty(window, 'SCUTTLER', {
        value: (globalRef, scuttle) => {
          console.log('scuttler (start)');
          scuttle(globalRef);
          console.log('scuttler (end)');
        }
})

I don't understand too much, is this the solution?

Where should the following passage be executed?

Object.defineProperty(window, 'SCUTTLER', {
        value: (globalRef, scuttle) => {
          console.log('scuttler (start)');
          scuttle(globalRef);
          console.log('scuttler (end)');
        }
})

[legobeat]

@weizman What do you say about rebasing this one on main (there are conflicts to be resolved) and then merging #475 into here as-is? As you're the author of both and #475 is in the desired direction in any case.

[weizman]

@legobeat i would love to, but let's first merge the easier ones #482 and #477 into #475 to handle less PRs

[weizman]

you got it right @weilun0510 - it should be executed at any point before the execution of LavaMoat, so when LavaMoat loads it'll pick up this scuttler option from the global object.

[naugtur]

We took a bit too long on this one. Can we ship this in the next release cycle and finish #597 for the release after that?
We've got people waiting for this.

Are there any parts of #597 that are stable and finished that could be moved into this one? We still have time until the current release cycle is done. It'd be best to release node 18 support fully before this.

[weizman]

#597 is not ready to be merged, and picking specifics from it will delay us further, I am against.
These types of things should be reflected after this PR is merged.
I'd appreciate an approve on this PR @naugtur.

[naugtur]

After in-person conversations I see consensus that this should have been merged a month ago and while we're good at coming up with what to improve, we need to get better at deciding when.