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

feat(compartment-mapper): Host module exits #2422

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

kriskowal
Copy link
Member

@kriskowal kriskowal commented Aug 20, 2024

Description

This change closes the remaining gap between Endo as a Zip archive bundler and support for “exits” to host modules from those bundles at import-time.

This begins with a test to demonstrate the latent support for conditional exits to host modules through importLocation. The test fixture has a library that exports the host implementation of itself if the bundler specifies the exit (endo:lib) as a condition. This causes the module to be omitted from bundles and relies on the importer to provide the implementation. There remained a gap for a round-trip through a bundle.

The subsequent changes fix a bottleneck for exit modules in the compartment-mapper. The LavaMoat policy-enforcement runtime is limited to virtual module sources, which constrained support for other kinds of module descriptor. This change opens that up so arbitrary module descriptors pass-through the attenuating adapter if no policy is in effect for that edge. We can return to explore attenuation of other kinds of module-descriptor.

At this point, all exits have to be explicitly marked with an importHook that returns a module descriptor for the named exit module specifier. We then add a feature to the bundler that implicitly recognizes any module specifier that starts with a URI-scheme prefix is an exit, for convenience on the bundler side. This will obviate the need for an additional command-line flag in bundle-source in the common case.

Then, we trivially thread the importHook through importBundle options.

Security Considerations

Host provided modules must be hardened and pure, to avoid being useful as a side-channel or mitm attack surface between guests.

Scaling Considerations

This should allow the creation of smaller bundles.

Documentation Considerations

Any module that implements this feature should document the condition that enables it for bundling and importing.

Testing Considerations

Just a test.

Compatibility Considerations

None.

Upgrade Considerations

None.

Copy link
Contributor

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I don't think I get it yet.

@@ -0,0 +1 @@
export { feature } from 'lib';
Copy link
Contributor

Choose a reason for hiding this comment

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

why is app under node_modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this would mostly work at the directory containing node_modules, but the compartment mapper issues a warning if the package.json name does not match the parent directory name.

Comment on lines +32 to +33
const { namespace } = await importLocation(readPowers, fixture, {
conditions: new Set(['endo:lib']),
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes the module to be omitted from bundles ...

I'm trying to make such a bundle but losing:

~/projects/endo/packages/compartment-mapper$ ../../node_modules/.bin/bundle-source -C endo:lib  ./test/fixtures-conditional-host-exports/node_modules/app/index.js 
(TypeError#1)
TypeError#1: Failed to load module "./index.js" in package "file:///home/connolly/projects/endo/packages/compartment-mapper/test/fixtures-conditional-host-exports/node_modules/app/" (1 underlying failures: Cannot find external module "endo:lib" in package file:///home/connolly/projects/endo/packages/compartment-mapper/test/fixtures-conditional-host-exports/node_modules/lib/

  at throwAggregateError (packages/ses/src/module-load.js:546:11)
  at load (packages/ses/src/module-load.js:594:3)
  at async digestFromMap (packages/compartment-mapper/src/archive-lite.js:364:3)
  at async makeAndHashArchiveFromMap (packages/compartment-mapper/src/archive-lite.js:415:52)
  at async bundleZipBase64 (packages/bundle-source/src/zip-base64.js:83:29)
  at async main (packages/bundle-source/src/main.js:107:20)

Copy link
Member Author

Choose a reason for hiding this comment

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

I’ve got some more wrangling to do to make this work through the makeBundle to parseBundle, and a tiny amount of work to thread through from bundleSource to importBundle after that. Retreating to draft.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should work now. bundle-source -C endo:lib -T produced a bundle with a transparent exit import for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

confirmed:

$ cd packages/compartment-mapper
$ mkdir -p exit
$ mkdir -p no-exit

$ ../../node_modules/.bin/bundle-source  -T ./test/fixtures-conditional-host-exports/node_modules/app/index.js | jq -r .endoZipBase64| base64 -d | bsdtar -C no-exit -xvf-
x compartment-map.json
x app/index.js
x lib/index.js

$ ../../node_modules/.bin/bundle-source -C endo:lib -T ./test/fixtures-conditional-host-exports/node_modules/app/index.js | jq -r .endoZipBase64| base64 -d | bsdtar -C exit -xvf-
x compartment-map.json
x app/index.js
x lib/endo.js

$ cat exit/lib/endo.js 
export { feature } from 'endo:lib';

$ cat no-exit/lib/index.js 
export const feature = 'ponyfill';

Copy link
Contributor

Choose a reason for hiding this comment

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

POC: Eliding @endo/patterns etc. from a contract bundle

ooh! works on orca.contract.js from dapp-orchestration-basics too!

Contract bundle in its original form is around 2.1M:

$ cd ~/projects/dapp-orchestration-basics/contract

$ ~/projects/endo/node_modules/.bin/bundle-source -T src/orca.contract.js | jq -r .endoZipBase64| base64 -d >no-exit.zip && ls -sh no-exit.zip 
2.1M no-exit.zip

Then I tweaked @endo/patterns, @endo/far, and @endo/eventual-send something like:

$ git diff .
diff --git a/packages/patterns/package.json b/packages/patterns/package.json
index b2b9866ea..1a149a52f 100644
--- a/packages/patterns/package.json
+++ b/packages/patterns/package.json
@@ -18,7 +18,10 @@
   "main": "./index.js",
   "module": "./index.js",
   "exports": {
-    ".": "./index.js",
+    ".": {
+      "endo:1": "./endo-eventual-send-exit.js",
+      "default": "./index.js"
+    },
     "./package.json": "./package.json"
   },

And tweaked the dapp dependencies:

$ ls -l ../node_modules/@endo/{far,patterns,eventual-send}
lrwxrwxrwx 1 connolly connolly 51 Aug 26 21:32 ../node_modules/@endo/eventual-send -> /home/connolly/projects/endo/packages/eventual-send
lrwxrwxrwx 1 connolly connolly 41 Aug 26 21:25 ../node_modules/@endo/far -> /home/connolly/projects/endo/packages/far
lrwxrwxrwx 1 connolly connolly 47 Aug 26 21:37 ../node_modules/@endo/patterns -> /home/connolly/projects/endo/packages/patterns/

Contract bundle shrinks to 17.M:

$ ~/projects/endo/node_modules/.bin/bundle-source -C endo:1 -T src/orca.contract.js | jq -r .endoZipBase64| base64 -d >exit.zip &&
 ls -sh exit.zip 
1.7M exit.zip

@kriskowal kriskowal marked this pull request as draft August 21, 2024 19:56
@kriskowal kriskowal force-pushed the kriskowal-ponyfill-host-module branch from 5262c7d to 03c08a3 Compare August 26, 2024 23:59
@kriskowal kriskowal changed the title test(compartment-mapper): Demonstration of host module ponyfill condi… test(compartment-mapper): Host module exits Aug 27, 2024
@kriskowal kriskowal force-pushed the kriskowal-ponyfill-host-module branch from 03c08a3 to 9d1811a Compare August 27, 2024 00:12
@kriskowal kriskowal marked this pull request as ready for review August 27, 2024 00:22
@kriskowal kriskowal requested a review from dckc August 27, 2024 00:22
@kriskowal kriskowal force-pushed the kriskowal-ponyfill-host-module branch from 9d1811a to 754f1b3 Compare August 27, 2024 00:23
Comment on lines 5 to 7
- Archives will now automatically treat any module specifier that appears
to start with a URL scheme (like `node:fs`) and these "exits" will no
longer need to be marked in the `"modules"` option.
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar? I'm having a hard time parsing this sentence.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be more clear now.

Copy link
Contributor

@dckc dckc left a comment

Choose a reason for hiding this comment

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

Yay! This definitely establishes that we can shrink bundles!

It's more than just a test, though. I'm not confident about approving some of the non-test material.

),
specifier: moduleSpecifier,
};
return attenuateModuleHook(
Copy link
Contributor

Choose a reason for hiding this comment

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

The scope of this PR has expanded beyond a test: now, right? Seems like a feat: or a chore: in preparation for a feat:.

@@ -60,6 +60,8 @@ const has = (object, key) => apply(hasOwnProperty, object, [key]);
*/
const extensionImpliesLanguage = extension => extension !== 'js';

const urlish = /^[a-z][a-z0-9+\-.]*:/;
Copy link
Contributor

Choose a reason for hiding this comment

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

This syntax is subject to an external design constraint; let's please cite it:

Suggested change
const urlish = /^[a-z][a-z0-9+\-.]*:/;
const urlish = /^[a-z][a-z0-9+\-.]*:/; // cf. section 4.3 of RFC 3986 Scheme URI Generic Syntax

optionally include https://www.rfc-editor.org/rfc/rfc3986#section-4.3

Comment on lines -462 to 513
* @param {import('ses').ThirdPartyStaticModuleInterface} originalModuleRecord - reference to the exit module
* @param {import('ses').VirtualModuleSource | import('ses').StrictModuleDescriptor} moduleDescriptor - reference to the exit module
* @param {import('./types.js').PackagePolicy} policy - local compartment policy
* @param {import('./types.js').DeferredAttenuatorsProvider} attenuators - a key-value where attenuations can be found
* @returns {Promise<import('ses').ThirdPartyStaticModuleInterface>} - the attenuated module
* @returns {Promise<import('ses').ModuleDescriptor>} - the attenuated module descriptor
*/
export const attenuateModuleHook = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

To what extent is this an externally visible API change?

I don't see any corresponding changes in tests, which suggests it's not an external API change.
But that could be just a lack of test coverage. Or maybe it's only tested indirectly.

Copy link
Member Author

Choose a reason for hiding this comment

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

As part of convergence with XS, VirtualModuleSource is an alias for ThirdPartyStaticModuleInterface and ModuleDescriptor is a new type that captures all of the new and old types importHooks may return (and the range of types that an importHook adapter is expected to handle). The change here allows the full range of module descriptors to pass through this adapter when a LavaMoat policy is absent, which allows us to surface the whole module descriptor range in an exit module importHook. This was a bottleneck before.

So, the input range expands from VirtualModuleSource to also accept StrictModuleDescriptor, the forward-compatible subset of ModuleDescriptor. I did not feel the need to create a new obligation for backward compatibility past the next SES major version and wasn’t able to get TypeScript to play along with reliably differentiating a ModuleExportsNamespace from a VirtualModuleSource. In SES, we use a brand check for that.

This catches us up with what I expected to be working behavior, so is more like a bugfix than a feature, so I didn’t capture this in the notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this API in any previous release?

Comment on lines +5 to +7
- Exports a `StrictModuleDescriptor` type that consists only of the
`NamespaceModuleDescriptor` and `SourceModuleDescriptor` that are mutually
supported by SES and XS.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. PR is clearly more than just test:.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. The left of the PR description has now caught up with the right of the PR description. The tags on individual commits also capture the conventions.

@kriskowal kriskowal force-pushed the kriskowal-ponyfill-host-module branch from 754f1b3 to c7481af Compare August 27, 2024 17:15
@kriskowal kriskowal changed the title test(compartment-mapper): Host module exits feat(compartment-mapper): Host module exits Aug 27, 2024
@kriskowal kriskowal force-pushed the kriskowal-ponyfill-host-module branch from c7481af to 1bf012f Compare August 27, 2024 17:44
@kriskowal kriskowal requested a review from erights August 27, 2024 17:44
Copy link
Contributor

@dckc dckc left a comment

Choose a reason for hiding this comment

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

looks good.

I'm not confident that I know all of the invariants that should be maintained. This review presumes they're covered by tests. If you're not confident about that, please get review from someone who is more familiar with the detailed design of this code.

@kriskowal kriskowal requested a review from naugtur August 27, 2024 19:38
@kriskowal
Copy link
Member Author

Summoning @naugtur since this touches policy enforcement code with delicate invariants.

@naugtur
Copy link
Member

naugtur commented Aug 27, 2024

I'll need some time to dig through this. I'll see if I can go through it before the meeting tomorrow.

@boneskull boneskull self-requested a review August 28, 2024 18:45
}

/**
* Attenuates a module
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to have descriptions differ between the two functions

exports: [],
execute() {
throw new Error(
'Cannot import an application loaded strictly for analysis',
Copy link
Member

Choose a reason for hiding this comment

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

I'll give it another chance later, but I don't understand this error message.

}) {
await null;
if ('source' in moduleDescriptor) {
const { source: moduleSource } = moduleDescriptor;
Copy link
Member

Choose a reason for hiding this comment

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

const moduleSource = normalizeModuleDescriptorOrSource(moduleDescriptor);

and we don't need all the duplication

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.

4 participants