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

CACAOv3 #196

Merged
merged 5 commits into from
Jun 1, 2023
Merged

Conversation

oed
Copy link
Collaborator

@oed oed commented Dec 17, 2022

This PR introduces a big upgrade to the CACAO IPLD data structure to make it more concise and compatible with ReCap, UCAN and possibly other object-capability formats in the future.

This work builds upon ucan-ipld by @Gozala and introduces the use of varsig and multidid.

Note that both ReCap and UCAN are currently undergoing some changes in their data structures. This work (I think) represents the latest consensus.

📄 Preview

@oed oed requested a review from ukstv December 17, 2022 13:50
@zachferland zachferland mentioned this pull request Dec 19, 2022
CAIPs/caip-74.md Outdated
Comment on lines 136 to 137
* `iss` - convert the `iss` string of the UCAN to a multidid
* `aud` - convert the `aud` string of the UCAN to a multidid
Copy link

Choose a reason for hiding this comment

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

When multidid lands, we'll update ucan-ipld to use it, too 💯

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need ucan-ipld if it maps 1-to-1 with the CACAO ipld schema? My intention here was to dedpelicate this work.

CAIPs/caip-74.md Outdated
type Ability String // e.g. crud/create

type NB { String : Any }
type Abilities { Ability : [NB] }
Copy link

@expede expede Dec 19, 2022

Choose a reason for hiding this comment

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

I think we need to specify the semantics of this array: it can be interpreted as a logical OR (i.e. "any"), or AND (i.e. "all"). Here's a contrived example:

{
  "https://example.com/blog/posts": {
    "crud/create": [
      {"day": "friday"},
      {"status": "draft"}
    ]
  }
}

One can interpret this as either

  1. "You can post drafts to the blog on Fridays"
  2. "You can post drafts to the blog, or post any status on Fridays"

In UCAN <=0.9, these look something like...

// Option 1
[
  {
    "with": "https://example.com/blog/posts",
    "can": "crud/create",
    "nb": {"day": "friday", "status": "draft"}
  }
]

// Option 2
[
  {
    "with": "https://example.com/blog/posts",
    "can": "crud/create",
    "nb": {"day": "friday"}
  },
  {
    "with": "https://example.com/blog/posts",
    "can": "crud/create",
    "nb": {"status": "draft"}
  }
]

...which has a very clear intention that the outer array is an OR (or, well, it's using constructive logic, but same thing in this case). Using this logic, to make it an AND, you need to put all keys in a single entry:

{
  "https://example.com/blog/posts": {
    "crud/create": [
      {
        "day": "friday",
        "status": "draft"
      }   
    ]
  }
}

I'm fairly certain that without specifying this, we open ourselves to confused deputies based on misinterpreting the semantics from the source capability format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Will address this!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an example. Let me know if this fully addresses your concern!

Copy link

@chunningham chunningham left a comment

Choose a reason for hiding this comment

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

this ties things together quite nicely

CAIPs/caip-74.md Outdated

```
uOqJlcm9vdHOB2CpYJQABcRIgEbxa4r0lKwE4Oj8ZUbYCpULmPfgw2g_r12IcKX1CxNlndmVyc2lvbgHdBAFxEiARvFrivSUrATg6PxlRtgKlQuY9-DDaD-vXYhwpfULE2aNhaKFhdGdlaXA0MzYxYXCrY2F1ZHgbaHR0cDovL2xvY2FsaG9zdDozMDAwL2xvZ2luY2V4cHgdMjAyMi0wMy0xMFQxODowOToyMS40ODErMDM6MDBjaWF0eB0yMDIyLTAzLTEwVDE3OjA5OjIxLjQ4MSswMzowMGNpc3N4O2RpZDpwa2g6ZWlwMTU1OjE6MHhCQWM2NzVDMzEwNzIxNzE3Q2Q0QTM3RjZjYmVBMUYwODFiMUMyYTA3Y25iZngdMjAyMi0wMy0xMFQxNzowOToyMS40ODErMDM6MDBlbm9uY2VmMzI4OTE3ZmRvbWFpbm5sb2NhbGhvc3Q6MzAwMGd2ZXJzaW9uAWlyZXF1ZXN0SWRxcmVxdWVzdC1pZC1yYW5kb21pcmVzb3VyY2VzgnhCaXBmczovL2JhZnliZWllbXhmNWFiandqYmlrb3o0bWMzYTNkbGE2dWFsM2pzZ3BkcjRjanIzb3ozZXZmeWF2aHdxeCZodHRwczovL2V4YW1wbGUuY29tL215LXdlYjItY2xhaW0uanNvbmlzdGF0ZW1lbnR4QUkgYWNjZXB0IHRoZSBTZXJ2aWNlT3JnIFRlcm1zIG9mIFNlcnZpY2U6IGh0dHBzOi8vc2VydmljZS5vcmcvdG9zYXOiYXNYQVzLE0rT2HTLtAoys5lUnNMslT3F3IfcZGJKPj3AaE19SDMEPdfp9KaJSFP43FVfl7x-PH3T_MZkCeuYK_86RGcbYXRmZWlwMTkx
```

### <a name="appendix-a"></a>Appendix A: Timestamp converstion algorithm

The values in SIWx are encoded as [RFC3339](https://datatracker.ietf.org/doc/html/rfc3339#section-5.6) strings, while CACAO requires unix timestamps (in seconds). The algorithm used to convert between the two is outlined below.

Choose a reason for hiding this comment

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

I assume these timestamps can be fractional? or should they be micro/nanoseconds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They can't, I think we would need to put ms information in the timezone fct properties.

CAIPs/caip-74.md Outdated
Resources:
- {.p.resources[0]}
- {.p.resources[1]}
- {recap-uri}

Choose a reason for hiding this comment

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

perhaps I missed it, but should we explicitly constrain this to one recap-uri per siwx? additionally, if we expect .fct.resources to be readable-ish, should we move the recap uri to the end? users may see a large b64 recap blob and tune out, ignoring the resources after

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think constraining ReCap spec to only include one recap-uri per siwx would make a lot of sense yes!
I don't feel strongly about ordering, but your logic makes sense 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think chunny is onto something with the footgun-factor of big blobs being anywhere except at the end :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the recap-uri to last.

CAIPs/caip-74.md Outdated
* `iat` - is based on `issued-at`
* `nbf` - is based on `not-before`
* `exp` - is based on `expiration-time`
* `fct.z-iat` - timezone info from `issued-at`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Timezones mandatory? Default to 0/UTC if not present for backwards compat simplicity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think if there is no timezone in the SIWx and no ms then they can be left out. This will be specified in Appendix A.

CAIPs/caip-74.md Outdated

## Backwards Compatibility

In the [previous version of this specification](https://github.com/ChainAgnostic/CAIPs/blob/91aaaff73038c2629ff11b88c2209f61521d8ece/CAIPs/caip-74.md), the header type was restricted to `eip4361` as it was designed to work only with Sign-in with Ethereum. Newer implementations should support both header types - `eip4361` and `caip122`.

## Example

**TODO - update these examples**

Below you could find a CACAO, along with its serialized presentation in CAR file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

should "version" be "2" ? Separate CAIP?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"version" is not a property of CACAO, but rather SIWe(x). Realized that there is a "version" section below which I will merge with the backwards compatibility section.

CAIPs/caip-74.md Outdated

According to the [solana namespace](https://namespaces.chainagnostic.org/solana/caip122),

* `content_multicodec` - set to *caip122*, `0xd510`
Copy link
Collaborator

@bumblefudge bumblefudge Jan 17, 2023

Choose a reason for hiding this comment

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

but is it caip122 or is it caip122-solana:ed25519 ?

in Haardik's solana/caip122, it proposes the string solana:ed25519 as the signatureMeta, equivalent to eip155/caip122 using caip122-eip191 and caip122-eip1271 😬

solana/caip122 is still in draft so we could change that signatureMeta value... and update CAIP-122 while we're at it before 10 more namespaces get written without a naming convention!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but is it caip122 or is it caip122-solana:ed25519 ?

It doesn't matter in this context I think. ed25519 is defined by the key_multicodec.

It's a good point about cleaning up namespaces after this PR get's merged!

@bumblefudge
Copy link
Collaborator

Just went to see if the w3c/vc-jwt spec had some language or algorithms that could be cut-and-pasted or referenced for the timestamp mechanics appendices... and found a TODO there as well. well, it was worth a try! I watch that repo so if those sections get filled out before these ones do, I'll drop a link here.

@oed
Copy link
Collaborator Author

oed commented Jan 31, 2023

Moved to CAIP-196 and marked as replacing: 74.

@oed oed changed the title Modify CACAO for interoperability CACAOv3 Mar 13, 2023
@expede expede mentioned this pull request Apr 3, 2023
1 task
@obstropolos
Copy link
Contributor

Bumping on any additional action items here.

Additionally noting recent merge of ucan v0.10 ucan-wg/spec#132

@oed
Copy link
Collaborator Author

oed commented May 19, 2023

@bumblefudge I changed this to "Draft", do you think we can merge it so that it's more easily refereneable on chainagnostic.org by implementors?
It would also allow other people than me to follow up with PRs to address the outstanding issues. Both @chunningham and @zachferland are working on implementations right now.

@obstropolos
Copy link
Contributor

Seconding - merging the draft is likely a good idea, given implementations and the majority being covered here. cc @bumblefudge

@obstropolos obstropolos merged commit d76ba55 into ChainAgnostic:master Jun 1, 2023
@obstropolos
Copy link
Contributor

Since no objection, merging as a draft for now.

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.

6 participants