endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
829 stars 72 forks source link

feat(exo): lightweight inter-facet rights amplification #1902

Closed erights closed 10 months ago

erights commented 10 months ago

towards: https://github.com/Agoric/agoric-sdk/issues/8664 refs: #1666

Description

Adds a receiveAmplifier option to definedExoClassKit to parallel the existing receiveRevoker. The amplifier function you receive is a two argument function of aFacet and a facetName. If aFacet is an unrevoked facet instance of a cohort of that exo class kit, and facetName is the name of a facet column of that exo class kit, then the named element of aFacet's cohort is returned.

See https://github.com/Agoric/agoric-sdk/issues/8664 for more explanation.

This PR does not by itself finish https://github.com/Agoric/agoric-sdk/issues/8664 . Rather, we need to wait for an endo release incorporating this PR followed by an agoric-sdk-endo sync before we can get started implementing this same feature for virtual and durable class kits.

Security Considerations

As explained at https://github.com/Agoric/agoric-sdk/issues/8664 , this makes one particular pattern of rights amplification easier to express, and more auditable when expressed in this manner.

This is in contrast to systems like JS's or Joe-E class-private instance variables, where inter-instance intra-class amplification is implicit and easy to overlook in a review.

Scaling Considerations

As explained at https://github.com/Agoric/agoric-sdk/issues/8664 , this avoids the need for the additional weak store that would otherwise be needed to express such rights amplification, without losing the explicitness of the separate weak store.

Documentation Considerations

Yet another thing to document. But should probably wait until https://github.com/Agoric/agoric-sdk/issues/8664 is closed, i.e., until the feature is available for virtual and durable exo kits. Probably should wait until revocability is as well, and then both should be documented together.

Testing Considerations

Should enable more secure "jig" testing, where an exo class kit also defines a facet to be used for privileged testing/debugging, and the testing framework is, somehow, endowed with the needed amplifier. We would still need to figure out how to wire that up so the amplifier cannot otherwise be obtained or used.

Upgrade Considerations

Assuming that implementing this feature for virtual and durable exos is similar and as straightforward, this PR should not cause any change to virtual or durable state, enabling exo class kit instance state defined without amplifiers (likewise revokers) to be upgraded into an exo class kit which does.

erights commented 10 months ago

We generally assume the number of facet names for an exo class kit is very small. This is proportional only to that. Does this seem right?

Cheers, --MarkM

On Tue, Dec 19, 2023 at 6:13 PM Dan Connolly @.***> wrote:

@.**** commented on this pull request.

In packages/exo/src/exo-makers.js https://github.com/endojs/endo/pull/1902#discussion_r1432174393:

@@ -228,6 +254,21 @@ export const defineExoClassKit = ( receiveRevoker(revoke); }

  • if (receiveAmplifier) {
  • const amplify = (aFacet, facetName) => {
  • for (const contextMap of values(contextMapKit)) {

Can this amplify function be considered approximately O(1) for the purpose of limiting work by message size https://github.com/Agoric/agoric-sdk/wiki/Limit-work-by-message-size?

If not, please document that LOUDLY. Is contextMapKit typically high-cardinality? What causes it to grow?

In the crabble review https://github.com/Agoric/agoric-sdk/issues/8465, we looked for loops and .map() and such, but we didn't consider the possibility that functions in the Agoric SDK (including endo) might have running time that grows much faster than their arguments.

— Reply to this email directly, view it on GitHub https://github.com/endojs/endo/pull/1902#pullrequestreview-1790059992, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACC3TGYTK4I2QB2DHQPD3DYKJCWLAVCNFSM6AAAAABA2GVNRGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJQGA2TSOJZGI . You are receiving this because you authored the thread.Message ID: @.***>

mhofman commented 10 months ago

In the kernel meeting a couple weeks ago, we discussed having the shape of this rights amplification function be a simple function that accepts a facet, and returns the cohort. I think I preferred that shape vs taking a facet name, but I do realize I didn't capture that feedback in this pull request.

erights commented 10 months ago

In the kernel meeting a couple weeks ago, we discussed having the shape of this rights amplification function be a simple function that accepts a facet, and returns the cohort. I think I preferred that shape vs taking a facet name, but I do realize I didn't capture that feedback in this pull request.

Quite right. See https://github.com/endojs/endo/pull/1924