-
Notifications
You must be signed in to change notification settings - Fork 1
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
v1.0.0-rc.1 #2
base: main
Are you sure you want to change the base?
v1.0.0-rc.1 #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @expede for capturing all of the discussions that have occured in side channels in this spec, I am super excited to see it in current state.
I made couple of suggestions and notes inline but here is the list of things that I'd like to align on before we call this done.
- Support for powerlines
- I think we were an agreement that
like
was a better operator name thanmatch
, but spec uses later. I have some reservations, but mostly want to make sure that there is a good rational for switching names. - Restricting negation to single cause is unnecessarily restricting and leaves undefined behavior when multiple clause may be present. I suggest we do what datomic does here and allow multiple clauses. Or alternatively we define what behaviour should be if more than one clause is present.
- I was under assumption that we had an agreement on selector behavior captured by the table here selector syntax #5 specifically in regards to errors and
?
operator but some of the spec seems to be in contradiction. - Looks like slice selection has being omitted, unless this is deliberate I'd like to include it.
- Iteration over unary relations are defined as
false
conditions, which I find unnecessarily restrictive and making a case a case that they should be treated as collections of single item instead (details inline). - Clarification of selectors over
bytes
would be really appreciated.
README.md
Outdated
|
||
# Capability | ||
|
||
Capabilities are the semantically-relevant claims of a delegation. They MUST be presented as a map under the `cap` field as a map. This map is REQUIRED but MAY be empty. This MUST take the following form: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems out of sync with the rest of the spec perhaps something like this ?
Capabilities are the semantically-relevant claims of a delegation. They MUST be presented as a map under the `cap` field as a map. This map is REQUIRED but MAY be empty. This MUST take the following form: | |
Capability is the semantically-relevant claim of a delegation. It is represented by subset of the fields of the delegation map. Capability MUST take the following form: |
| `and` | `[Statement]` | `["and", [[">", ".a", 1], [">", ".b", 2]]` | | ||
| `or` | `[Statement]` | `["or", [[">", ".a", 1], [">", ".b", 2]]` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imbalanced parens and I think you meant following
| `and` | `[Statement]` | `["and", [[">", ".a", 1], [">", ".b", 2]]` | | |
| `or` | `[Statement]` | `["or", [[">", ".a", 1], [">", ".b", 2]]` | | |
| `and` | `[Statement]` | `["and", [">", ".a", 1], [">", ".b", 2]]` | | |
| `or` | `[Statement]` | `["or", [">", ".a", 1], [">", ".b", 2]]` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gozala LOL yeah, I think I blended the the two options here 😵💫 The other way is to always force an array, but I'm happy either way. Will update per your comment.
// Alternative
["and", [[">", ".a", 1], [">", ".b", 2]]] // I like this less
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Will merge, but want to check the prose to make sure it's also clear about this case first)
README.md
Outdated
|
||
| Operator | Argument(s) | Example | | ||
|----------|---------------|--------------------------------------------| | ||
| `not` | `Statement` | `["not", [">", ".a", 1]]` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not
be able to enclose multiple statements ? Unless you have objections I propose that it should, for what it's worth in datomic not clause expresses that all of the nested predicates MUST be false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be able to enclose multiple statements
That may be confusing, no? Does it add an implicit and
? Datomic likely does an implicit or
due to how DNF tuple selection works (the policy language is not a query language)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restricting negation to single cause is unnecessarily restricting and leaves undefined behavior when multiple clause may be present.
Also I disagree about undefined behaviour when multiple clauses are present: that case is disallowed since multiple clauses after a not
matches no forms for the rationale given earlier.
Despite the parens and use of operators in prefix, I really think we should stay away from Scheme idioms since they're very foreign to normie devs. Arguably we should switch the operators to words for this reason, but it's not a hill that I'm going to die on.
README.md
Outdated
|
||
Validation involves substituting the values from the `args` field into the Policy, and evaluating the predicate. Since Policies are tree structured, selector substitution and predicate evaluation MAY proceed in any order. | ||
|
||
If a selector cannot be resolved (there is no value at that path), the associated statement MUST return false, and MUST NOT throw an exception. Note that for consistent semantics, selecting a missing keys on a map MUST return `null` (but nested selectors without a try MUST then fail the predicate). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This contradicts some of the prior agreements that made here #5 specifically I made case that:
- reference to missing key
- out of bound index reference
- Iteration over non collection
All must be errors which could be trapped using try ?
suffix to opt-in into return null
behavior. My rational had been that optionality could be handled explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think is a case of me working from the Rust version where we had diverged while iterating quickly. You're right and the rationale makes sense to me; will fix 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it's also how jq
behaves:
> echo '{"a": 1}' | jq '.b.c.d.e.f'
null
> echo '[1]' | jq '.b'
jq: error (at <stdin>:1): Cannot index array with string "b"
When writing this, I had jq
running on the side to check edge cases. Anywhere that we diverge from jq
will need to be highlighted in big neon lights (or we need to do whatever they do since it's easier to manually test against / principle of least surprise)
|
||
## Quantification | ||
|
||
When a selector resolves to a collection (an array or map), quantifiers provide a way to extend `and` and `or` to their contents. Attempting to quantify over a non-collection MUST return false and MUST NOT throw an exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempting to quantify over a non-collection MUST return false and MUST NOT throw an exception.
What is the rational for this decision ? I have being advocating that quantifying over non collection should behave as quantifying over single element collection because it is helpful in schema evolution when things 1:1 relations can change to 1:n relations.
In my experience with schema-on-read systems like datomic are a lot more flexible because they follow this approach.
I should clarify that my position implies that iteration over null
should be equivalent to iteration over []
as opposed to [null]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not believe iteration over map had being covered and if it is supported it needs to be clarified what .
gets bound to for the maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rationale is that it breaks the policy assertion. Quantification implies that there is some collection, so IMO the other option is just throwing an error. Much like using ?
elsewhere, you can always recover by wrapping in an or
to say "or if it's not a collection, do this instead".
I should clarify that my position implies that iteration over null should be equivalent to iteration over [] as opposed to [null].
And I guess that iteration over 1
should be [1]
and over false
would be [false]
? If so, that seems fiddly to me, like how truthy values lead to weird edge cases. But that could be the strongly typed person in me talking... but this is how jq treats this situation:
$ echo '[1,2,3]' | jq "all < 5"
true
$ echo '1' | jq "all < 5"
jq: error (at <stdin>:1): Cannot iterate over number (1)
$ echo '[null]' | jq "all == 5"
false
$ echo 'null' | jq "all == 5"
jq: error (at <stdin>:1): Cannot iterate over null (null)
and somewhat infuriatingly, null
is the smallest number
$ echo '[null]' | jq "all < -999999"
true
$ echo '[null]' | jq "all > -999999"
false
Also, that they have any
and all
that behaves like this makes me want to change every
and some
back to all
and any
for consistency
In my experience with schema-on-read systems like datomic are a lot more flexible because they follow this approach.
Datalogs are query languages. This is a policy language. I'm increasingly convinced that they're duals of each other: querying is finding "some" matching data in a larger set, and a policy language enforces that all of the data you have conforms to expectation.
Selector analysis/implementationI'm doing preliminary work on a Go implementation of UCAN v1.0.0-rc1 MethodologyThe following methodology was employed to develop the
Result
Recommendations
Notes
|
@smoyer64 thanks for taking a stab at it and sharing the feedback. It may be worth pointing out that selector syntax while was mostly inspired by jq, compatibility with it was not a goal. Bunch of jq features aren't supported and there were set of cases where diverging from jq seemed like a better choice in our context, you can view some of the details here #5 I'm not sure about the rust implementation but I had a draft of JS implementation that can be viewed here https://github.com/storacha-network/ucanto/pull/344/files |
Co-authored-by: Irakli Gozalishvili <[email protected]> Signed-off-by: Brooklyn Zelenka <[email protected]>
Thanks for the feedback @smoyer64 ! Responses inline: Quickly jumping to the end first
Thank you for the deep engagement with the material! I think other than the question about how strictly we adhere to jq, I'd like to implement the rest of your comments! 🙏
PR merged! Thanks :)
Yes, as Irakli responded earlier, we do have some divergences from jq. We think that they're well motivated ( #5 ) , but they have already bitten both you and me, so perhaps we should rethink that.
Agreed ✅ Will fix (DONE)
This is one of the examples that we chose to diverge on. In short: jq is (largely) a parser that processes streams, and UCAN policies are a policy language that operate on static data. The security implications of permissive behaviour may be significant, and we can always get the There's also a few cases of expressivity, e.g. distinguishing between a
Interesting! Can you expand on this?
🤔 Perhaps. jq operates on streams, and is more permissive in how it treats things like assertion violation (e.g. when a field is not present), which may not be appropriate for an access control context.
Could you expand? I'm not certain that ours is filtering in the same sense as jq filters on streams (or in the sense that you
Sure, let's switch it to "Optional" ✅ (DONE)
If helpful, we could also switch to IPLD Schema if that's better for this purpose than ANBF. I was trying to avoid IPFS Schema since it has its own set of design issues.
Yes, it does depend on the other design decisions for certain. I'm not as confident in the strategy of generating a parser directly since you'll need to reserialise the CBOR to JSON for this to work, right? That could be expensive on every request.
I'll flip the switch on the Rust implementation momentarily (DONE). It needs a few tweaks to come in line with your and Irakli's comments, but a version of it was being used right at the end of Fission.
We have two parallel implementations: JS and Rust. |
Indeed, that's how the Rust code works.
Will update the spec ✅ |
@smoyer64 I pushed the branch directly to the |
I'm happy to concede that the deviations from strict compatibility with
This example from the
Yes and I'm happy to see that @alanshaw has written a tokenizer/parser/matcher that works on the IPLD nodes.
Awesome! This will make it easier to decipher the spec - should I feed anything I think are ambiguities back into PRs? |
| `iss` | `DID` | Yes | Issuer DID (sender) | | ||
| `aud` | `DID` | Yes | Audience DID (receiver) | | ||
| `sub` | `DID \| null` | Yes | Principal that the chain is about (the [Subject]) | | ||
| `cmd` | `String` | Yes | The [Command] to eventually invoke | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance to make that cmd
a [String]
, so that multiple capability can be delegated with the same payload?
Unless I missed something, if a (larger) service wants to delegate a set of capabilities to a user, it might not be possible to express that with a nice wildcard. Instead, you'd have a list of command: /foo/*, /bar/baz, /bar/boz, ...
.
As I understand, with cmd
being a String
(and not allowing comma-separated value I assume), this has the following consequences:
- a separate payload need to be issued and sign for each of the capabilities
- the receiver (audience) need to handle multiple delegation: much more complex UX/DX and logic necessary on the client side
- much larger token overall, especially as the signatures are duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe better, have it be a [Capability]
, so that one policy remains attached to one command neatly.
I may get pilloried for this version. WIP, obviouslyPreview 📚
Okay, this version switches to IPLD. This makes it much easier to not need a IPLD version off to the side, and many teams that have adopted UCAN already use IPLD somewhere in their stack. There is a contingent of people that feel strongly in favour of JWT for a variety of reasons. I also defended the JWT strategy for a long time, beacuse I had many first-hand converstaions of "I need to sell this to management, please tell me it's a JWT and not some inscrutable binary format".
A few things have changed:
I believe that this proposal makes writing both UCAN Delegations and libraries much easier and more comprehensible. It also lowers our maintenance burden between multiple formats.
Changelog
Metadata
1.0.0-rc.1
Structure
ucan-wg/revocation
ucan-wg/invocation
ucan/*
(moving toucan-wg/ucan-uri
)ucan-wg/ucan-uri
prf
field toucan-wg/invocation
+ add to top-levelucan-wg/spec
Time
exp
nullableProse
6[now] 5, that you only invalidate single capabilities; not everything