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

[WIP] Generate Space proofs on the fly, on access/claim #1555

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Peeja
Copy link
Contributor

@Peeja Peeja commented Sep 26, 2024

The bulk of the work for storacha/project-tracking#125

This together with w3ui calling access/claim on page load will make new spaces appear on the page without logging out.

The Strategy

  • On access/confirm, where we previously created a */ucan:* delegation with proofs for each space, we now create one with no proofs. This delegation technically provides no capability, but serves as a signal in access/claim.
  • During access/claim, we look for the Agent's */ucan:* delegations in the store, and translate them on the fly into ones containing proofs for the relevant spaces. This is legitimate, because */ucan:* means the Agent has been granted access to anything the Account can do, it just doesn't have the proofs that Account can do anything yet. We're just putting 2 and 2 together.

Remaining

  • We don't validate the */ucan:* delegation before recreating it. If access/confirm doesn't create an attestation, access/claim will still happily make one for its new delegation. That's a (minor?) security issue. If the delegation is in our store, we can probably trust it, but better to care about the attestation.
  • Tests for all of this.

@Peeja Peeja requested a review from alanshaw September 26, 2024 16:30
// This collection also includes the attestation delegations which validate
// prove those delegations.
const sessionProofsResult = await createSessionProofsForLogins(
loginDelegations,
Copy link
Member

Choose a reason for hiding this comment

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

If there is none, what happens?

// delegations for them. But they won't have any proofs on them. They're proof
// of login, but don't serve to give the Agent any actual capabilities. That's
// our job.
const loginDelegations = storedDelegationsResult.ok.filter((delegation) => {
Copy link
Member

Choose a reason for hiding this comment

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

So, it's my understanding that access/delegate and access/claim are general purpose. I think it's ok to add functionality from access/confirm here but it would be nice (slash not a breaking change) to retain existing ability to stash/retrieve any delegation. I think with the current implementation you'd not receive anything unless ucan:* delegations are found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, that's true. We should just translate those and leave the rest in the output.

@Peeja
Copy link
Contributor Author

Peeja commented Sep 26, 2024

@alanshaw Hmm. I'm worried that Ucanto doesn't like this trick. It tries to resolve the ucan:* when I ask it questions, and because there are no proofs in the delegation, it acts as if the ucan:* doesn't exist. Maybe I need to give up on using Ucanto to validate the delegation and keep poking at it by hand? I suppose this is basically a hack to hold us over until 1.0.

// These delegations will actually have proofs granting access to the spaces.
// This collection also includes the attestation delegations which validate
// prove those delegations.
const sessionProofsResult = await createSessionProofsForLogins(
Copy link
Contributor

Choose a reason for hiding this comment

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

could we put this (and probably some of the logic above) behind a feature flag of some sort? (we don't have a formal feature flagging system, but I think we might have some floating around in env vars which is probably the way to go for now?)

I'm just a little concerned about the impact this might have on the performance of this invocation handler and would like to be able to switch it on and off - refreshing the space list is nice but I want to make sure we can disable it for now if it starts causing trouble...

Choose a reason for hiding this comment

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

are we that concerned? I feel like "access/claim" is hardly a big hot path.

Choose a reason for hiding this comment

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

and are we really talking about a lot of work? Making session proofs is not a huge endeavor I would imagine. but maybe I'm missing historical context.

Copy link
Contributor

@travis travis Oct 1, 2024

Choose a reason for hiding this comment

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

ah so what worries me is that

a) we poll access/claim pretty aggressively from the client during login and
b) unless I'm mistaken there are at least one but possibly many network requests/dynamo queries inside createSessionProofsForLogins

totally possible this doesn't amount to enough work to be a concern, but I'd at least like to have some sort of profiling in place to keep an eye on it

@alanshaw
Copy link
Member

@alanshaw Hmm. I'm worried that Ucanto doesn't like this trick. It tries to resolve the ucan:* when I ask it questions, and because there are no proofs in the delegation, it acts as if the ucan:* doesn't exist. Maybe I need to give up on using Ucanto to validate the delegation and keep poking at it by hand? I suppose this is basically a hack to hold us over until 1.0.

Can you elaborate? When do you call ucanto & what is the error?

@Peeja
Copy link
Contributor Author

Peeja commented Sep 27, 2024

Never mind the below! I see now that the outer delegation is self-signed, so it never even looks at the inner one. Just a poor example on my part.


Maybe the point is moot—I'm using Ucanto's claim() and I can't get it to make sense, or I'm misunderstanding what it does:

import * as Server from '@ucanto/server'
import { capability, URI, Schema, ok } from '@ucanto/validator'
import { ed25519, Verifier } from '@ucanto/principal'

const authority = await ed25519.Signer.generate()

export const drive = capability({
  can: 'car/drive',
  with: URI.match({ protocol: 'did:' }),
  derives: ok,
})

const alice = await ed25519.Signer.generate()
const bob = await ed25519.Signer.generate()
const alicesCar = await ed25519.Signer.generate()

const delegation = await drive.delegate({
  issuer: alice,
  audience: bob,
  // !!! Wrong `with`!
  with: alice.did(),
  expiration: Infinity,
  proofs: [
    await drive.delegate({
      issuer: alicesCar,
      audience: alice,
      with: alicesCar.did(),
      expiration: Infinity,
      proofs: [],
      facts: [],
    }),
  ],
  facts: [],
})

const claimResult = await Server.claim(
  drive,
  [delegation /* , attestation */],
  {
    authority,
    principal: Verifier,
    validateAuthorization: () => ({ ok: {} }),
  }
)

console.log(claimResult)
{
  ok: Authorization {
    match: Match {
      source: [Array],
      value: [CapabilityView],
      descriptor: [Object]
    },
    proofs: []
  }
}

That doesn't even have the same with as its proof!

I've been trying to use claim() to look for a ucan:*, and I couldn't get it to do what I wanted, but I've reduced it to this weirdness and now I think I'm building on a misunderstanding.

Copy link

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

Overall this all seems quite sound to me. And, bonus, generating the proofs in the moment of access/claim is actually way more UCAN 1.0 like :)

// These delegations will actually have proofs granting access to the spaces.
// This collection also includes the attestation delegations which validate
// prove those delegations.
const sessionProofsResult = await createSessionProofsForLogins(

Choose a reason for hiding this comment

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

are we that concerned? I feel like "access/claim" is hardly a big hot path.

// These delegations will actually have proofs granting access to the spaces.
// This collection also includes the attestation delegations which validate
// prove those delegations.
const sessionProofsResult = await createSessionProofsForLogins(

Choose a reason for hiding this comment

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

and are we really talking about a lot of work? Making session proofs is not a huge endeavor I would imagine. but maybe I'm missing historical context.

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