claimRequest() and other powerbox-related updates

[dwrensha]

This pulls together the work from #2016 and #2019.

The ip-networking integration tests are working after applying https://github.com/zarvox/sandstorm-test-python/pull/4.

I have not yet updated the other powerbox integration tests, which are based on https://github.com/jparyani/sandstorm-test-app. So this will not pass Jenkins.

I've split out a new file powerbox.capnp. Doing so isn't strictly necessary for this change, but if we want to do that eventually, now seems like an opportune time.

I've not yet addressed all of the comments in #2019, but I wanted to get this pull request up for discussion.

[dwrensha]

@kentonv this is the code that I was talking about earlier today. It sets the owner.user.title field based on the owner.grain.saveLabel field, which feels wrong to me. In my understanding, saveLabel is used by the app to explain to the user why it saved the capability. I can imagine it being weird if such an explanation ended up in a user's title for a grain.

[kentonv]

@dwrensha I agree, that does look wrong. It seems like the code meant to use tokenInfo.owner.user.title.

[dwrensha]

Hm... it also occurs to me that ApiTokenOwner.grain.introducerIdentity is obsolete after this change. It's role gets fulfilled instead by ApiTokenOwner.clientPowerboxRequest.introducerIdentity.

[zarvox]

Hm... it also occurs to me that ApiTokenOwner.grain.introducerIdentity is obsolete after this change. It's role gets fulfilled instead by ApiTokenOwner.clientPowerboxRequest.introducerIdentity.

I'm not so sure.

Would making that change require that other users who want to review/audit the capabilities owned by a grain also be able to see ApiTokens with owner clientPowerboxRequest? I'm thinking about what information you'd need to build a compelling "audit connections" UI.

Does this still make sense if the powerbox request is initiated server-side, or do we intend to drop that flow entirely? Seems we'd need a place for the introducer in that workflow too, but you wouldn't necessarily want to chain to a clientPowerboxRequest there.

[dwrensha]

Would making that change require that other users who want to review/audit the capabilities owned by a grain also be able to see ApiTokens with owner clientPowerboxRequest? I'm thinking about what information you'd need to build a compelling "audit connections" UI.

No, clientPowerboxRequest tokens are short-lived and would not need to show up in an audit UI.

The introducerIdentity field is only needed to determine which user to apply requiredPermissions to.

The clientside flow goes like this:

1. Grain makes postMessage powerbox request on Alice's session, gets back a requestToken that has introducerIdentity set to Alice's identity ID.
2. Grain calls SandstormApi.claimRequest(requestToken, requiredPermissions) on the server side. The returned capability has a permissionsHeld requirement so that it will stop working when Alice no longer holds requiredPermissions.
3. The live capability gets saved. Note that here grain, webkey and user are all possibilities for the owner.

In a hypothetical server-side-only flow, the first two steps are merged into a single SessionContext.request(query, requiredPermissions) call.

It's the sturdyref created in step 3 that I think you're talking about showing up in an audit UI. The requirements field will hold the needed information. It's true that a capability can accumulate a large number of membrane requirements if it is passed through many powerbox requests. However, the way we represent that is through a chain of parent tokens; each link along the way will contain only the requirements picked up during the powerbox interaction which minted that token.

[zarvox]

It is not obvious to me if or why I should assume that some (the last?) item in the requirements array is, in fact, the introducer's identity, should I wish to show that to the user.

If we do make that guarantee, we should probably document it and assert about it in the code. I have always assumed requiredPermissions/requirements are an implementation detail of the permissions computation algorithm, rather than something with a guaranteed layout suitable for presentation to a user. Happy to have that clarified, though!

This suggests that we will need some pretty complicated join logic to pull profiles for identities referenced by any number of ApiTokens, but perhaps that was already true.

[zarvox]

One issue that I'm not sure this addresses is "how do I associate an identity (not just an account) with an IpNetwork I've granted to a grain, so I can show a sane thing in the UI?" There, the Supervisor.MembraneRequirement is just userIsAdmin: accountId.

Maybe userIsAdmin needs to be a struct of (accountId, identityId), rather than just a Text?

[dwrensha]

There, the Supervisor.MembraneRequirement is just userIsAdmin: accountId.

To pass the IpNetwork to a grain, you need to go through a powerbox flow, which must be associated to a SessionContext, which will have a particular identity associated with it, and should attach a permissionsHeld requirement.

[zarvox]

Ahhh, so for a hypothetical IpNetwork in the grain-specific audit interface, I'll look only for tokens with owner.grain with the matching grainId, and can always infer the introducer identity from the requirements.

Okay, so it sounds like the guarantee is:

• An ApiToken with owner grain is guaranteed to have as the last entry in its requirements a permissionsHeld struct which represents the account/identity responsible for creating the connection.

Does that sound right?

(Also, thanks for helping me understand all the intent here!)

[dwrensha]

An ApiToken with owner grain is guaranteed to have as the last entry in its requirements a permissionsHeld struct which represents the account/identity responsible for creating the connection.

Note quite. I would say that the entire requirements list is relevant, not just the final element in the list.

Consider the following interaction.

1. Admin Alice grants an IpNetwork to grain G, which saves the cap as a sturdyref T with a requirements list including "Alice's account has admin privileges" and "Alice can access G".
2. Alice shares G to Bob.
3. Bob opens grain H, which makes a powerbox request for an IpNetwork. Bob chooses to allow G to fulfill the request, and to do that G uses the sturdyref T from step 1.
4. H saves its new IpNetwork the resulting sturdyref has a requirements list consisting only of the permissionsHeld requirement "Bob can access H". The new sturdyref U points to T as its parent token.

The IpNetwork cap has passed through several powerbox interactions. At each step along the chain the local requirements list consists only of those requirements relevant to the most recent powerbox interaction. But to restore the cap, all of the requirements along the chain of parent tokens must be fulfilled.

The "requirements relevant to the most recent powerbox interaction" seems to me to be exactly the information needed for a an audit UI. I suppose it is plausible that in future revisions of the powerbox this may no longer be the case (e.g. if we support capabilities that don't autorevoke when the sharer loses access to the intermediary grain). Then we will need to add some additional way to capture the data needed in the audit UI. But I say let's not worry about that until it becomes a problem.

[dwrensha]

After some updates to the copy/paste powerbox flow and these changes to the powerbox test app, the integration tests are now passing.

[dwrensha]

Note that I'm not quite happy with ViewInfo.getIconUrls(). As discussed in #2016 (comment), maybe this should be ViewInfo.getIconAssetIds() and we should add a staticHost field to WebSession.Params.

[dwrensha]

In the latest commit, ViewInfo gets a metadata: DenormalizedGrainMetadata field and WebSession.Params gets a staticAssetPath: Text field. This allows a collections app to read icon asset IDs and then turn them into URLs suitable for including in an <img> tag. Each appearance of an asset ID in a Grains.cachedViewInfo increments that asset's reference count.

This approach still does not seem quite satisfactory, because a grain has no way either to prevent an asset from being deleted or even to get notified when an asset is deleted, short of repeatedly calling getViewInfo(). This suggests to me that we should try something like the following:

interface StaticAsset {
    getUrl @0 () -> (url :Text);
}

interface SandstormApi {
    // ....
    createStaticAsset @8 (mimeType :Text, encoding :Text, content :Data)
        -> (asset :StaticAsset);
}

struct GrainMetadata {
    appTitle @0 :LocalizedText;
    union {
         icon :group {
             format @1 :Text;
             asset @2 :StaticAsset;
             asset2xDpi @3 :StaticAsset;
             # If present, an equivalent asset at twice-resolution
         }
         appId @4 :Text;
    }
}

struct ViewInfo {
    /// ...
    metadata @5 :GrainMetadata;
} 

That is, we would put the actual StaticAsset capabilities in the ViewInfo. Then a grain would be able to prevent an asset from being deleted, simply by holding on to a reference to it; when a StaticAsset is restored, it increments the refcount in the database, and when it is dropped, it decrements the refcount.

I think we would also want to prevent grains from providing their own implementations of StaticAsset, to avoid possible cross-site scripting attacks. I think such prevention should be possible via a capnp::MembranePolicy.

@kentonv: I'm interested in your thoughts on all of this.

[dwrensha]

Another idea for keeping assets valid: we could add a method like

interface SandstormApi {
    // ....
    holdStaticAsset @9 (assetId :Text) -> (handle :Util.Handle);
}

where the asset's refcount is incremented for as long as handle is held. This would allow us to pass around asset IDs like in my proposed ViewInfo.metadata. A grain that wants to use an asset calls holdStaticAsset() to check that it's still valid and to ensure that it stays valid.

Then maybe createStaticAsset() would update to look like:

interface SandstormApi {
    // ....
    createStaticAsset @8 (mimeType :Text, encoding :Text, content :Data)
        -> (asset :StaticAsset, handle :Util.Handle);
}

[kentonv]

This TODO wouldn't be caught by our release script, FWIW. You need to do TODO(now) TODO(security), or just say that TODO(now) takes precedence.

[kentonv]

OK, reviewed everything.

I'm worried that referencing assets by string ID rather than by capability is going to create obstacles somewhere down the road, particularly when we start having federation between Sandstorm servers where the same asset probably has different IDs on different servers.

I think we will want to be aware when asset references move between servers, so that data can automatically be synced in the background, etc. We obviously only have that ability with capabilities.

Let me think about this more tomorrow.

[dwrensha]

Ready for re-review.

In this version, the StaticAsset capability can represent either some data in the StaticAssets collection or an identicon that will be served from an "identicon" host, which acts similarly to the "static" host, but does not require any backing data. This idea came up in a separate discussion: #1866 (comment).

[kentonv]

This is XSS-prone: If the StaticAsset capability is implemented by another app or other malicious source, it can return a javascript: URL or similar, which is likely to be evaluated in the context of the calling app. You can say it is the caller's responsibility to filter, but most won't.

We can prevent this problem by changing the return to something like:

(https: Bool, hostPath :Text)

The URL is then formed as:

(https ? "https://" : "http://") + hostPath

[dwrensha]

I've split the return value of StaticAsset.getUrl() into a protocol and a hostPath field, and I've moved the identicon server to our existing "static" host, under the "/identicon/" path.

Ready for re-review.