feat: rcan v2 - #27
Conversation
we got rcan two times, so just the name is ambiguous
matheus23
left a comment
matheus23
left a comment
There was a problem hiding this comment.
Authorizeris mostly unchanged, but it works with chains ofDelegationorTypedDelegation<C>that can each contain v1 or v2. It has fns for both opaque and typed delegation chains, that use the same underlying code.
I think this is more complicated than it needs to be: We never used Authorizer so far - it's fine if it only handles the current version.
TypedDelegation<C>is the above but you know that the bytes are compatible withC.
I'd like to scope the PR down a bit. Do we strictly need this? I don't think we need to introduce this type. It just means users (and agents) will more often have to choose whether to use TypedDelegation or Delegation in their code.
It's totally fine if users just deserialize a Delegation over the wire and then check it using Authorizer later.
(Compatibility code that never uses Authorizer is a different story but needs to be handled differently anyways.)
Q: Is the
V1Compat<C>pulling its weight? What it does is allowing us to read v1Rcan<C>via serde, but we could accomplish the same by just importing rcan 0.4.1 in addition to rcan 0.5 and having some glue. I guess it is nice to not need 2 versions of the same crate, also we don't need a dep to ctserde. But really I don't know...
I'm not sure. If we can skip it, that's great! I guess that would need a careful look at svc to see if we can just introduce new backhaul versions etc. without breaking things.
Q: Should we keep calling a delegation a "RCan"?
We should never actually use this capitalization... It's RCAN, matching UCAN.
That said, no I think calling the crate "rcan" and calling delegations "Delegation" seems totally fine to me.
Q: Currently Delegation serialize uses textual format for human readable serializers. This looks nice, but requires dealing with the quirks of the various serializers. Alternatives would be 1. just declare that only encode/decode and postcard is stable. 2. use base64(postcard) as string for the human readable formats.
Or hex(postcard). Yeah not sure.
There are cases like query parameters where encoding a delegation into "human-readable" strings is desirable. But tbh, for those cases you want to encode a whole delegation chain anyways, and probably rather encode as base64url(deflate(postcard(delegation_chain))).
So yeah - not super necessary IMO.
Skipping it means we have to have a dep on both rcan 0.4.1 or 0.5.0 and whatever the version with the new format is. That is a bit weird, but also kind of honest. We import the last published version of the old format to read the old format. It also means that conversion to and from |
This is a good reason to have a DelegationChain like in the other PR after all... Delegations chains contain redundancy with the keys, so deflating it by default would help with size. But then do we really care about some small multiple of 32 bytes? |
My suspicion is we won't need these conversions. And then updating the other systems (net-diagnostics, etc.) can work similarly I think. (And I probably wouldn't want to update the web token auth stuff, that one doesn't benefit from delegations at all, so we probably don't want to do that yet) |
not worth it. We just verify the signature again.
matheus23
left a comment
matheus23
left a comment
There was a problem hiding this comment.
I'm sorry I'm having a hard time formulating actionable feedback on this PR.
All I can say is I feel like this introduces so much stuff... There are now 6 manual implementations of serde traits in lib.rs and 5 more in v1.rs, there are a bunch of variants of check_invocation_from for both with and without time and typed or untyped.
The whole thing has now become quite big and I'm not sure if everything in here is pulling its weight...
Perhaps I'd much rather be hand-held and introduce things one-by-one and understand why we need each thing really well. Maybe I'm just getting lapped here not sure.
FWIW it just means my review will take more time at the end of the day.
The manual implementations are needed I think to replicate v1 behaviour, and are a bit more verbose than necessary so we avoid the dependency on serdect. We can also just use serdect everywhere to save a bit of verbosity. I just checked - it also has slice I am not sure if a line by line review is the best thing to do right now. This is still draft state, so maybe just look at the API using cargo doc and see what works and what doesn't? E.g. I think we could save quite a lot of surface area if we didn't have the OpaqueDelegation. But then I think we are going to need it, so I'd rather not remove it. |
Fix comment Co-authored-by: Philipp Krüger <philipp.krueger1@gmail.com>
Serialize as Vec<u8> in binary serializers and as String in textual serializers
Eliminate all v1 compat
Required some more serde plumbing to avoid the serde-bytes dep.
matheus23
left a comment
matheus23
left a comment
There was a problem hiding this comment.
I looked into this a bit which gave me some ideas for how we might make this easier, which I had to try out.
What do you think of this version: https://github.com/n0-computer/rcan/tree/matheus23/reduced ?
It's way more minimal though, but it's enough for all intents and purposes in smartpipe.
| struct Signed { | ||
| payload: Payload, | ||
| signature: Signature, | ||
| } |
There was a problem hiding this comment.
Do OpaqueDelegation need to be a wrapper around Signed for a good reason right now?
I find the name Signed to be fairly bad. But also - I don't think we need it. We could just remove the indirection between OpaqueDelegation and Signed, no?
| /// | ||
| /// Make sure to verify that the `invoker` signed and authenticated the | ||
| /// message containing the `capability`. | ||
| pub fn check_opaque_invocation_from<C: Capability>( |
There was a problem hiding this comment.
Have you had a need to check opaque delegation chains?
My experiments in smartpipe have shown that I don't really need that. I just parse into Delegation<SmartpipeCapability> on requests directly. And I don't really see why we wouldn't parse into the correct type immediately on requests?
| /// The issuer of the delegation. | ||
| pub fn issuer(&self) -> &VerifyingKey { | ||
| self.opaque.issuer() | ||
| } |
There was a problem hiding this comment.
There's a ton of repetition here.
We have Delegation::issuer, Payload::issuer and OpaqueDelegation::issuer.
If we ever add a ::nonce() we need to make sure we don't forget about one of these.
What do you think of making Payload private and instead just expose stuff on the delegation? And then at least Delegation and OpaqueDelegation can just reach directly into &self.payload.issuer, but that's fine because it's an internal struct in the same module.
The reduced 'rework' commit had reverted Delegation serde to Vec::<u8>::serialize(), which cbor/ciborium encodes as an integer array rather than a byte string. Restore serializer.serialize_bytes so the CBOR form matches the byte-string wire format. Drop the incorrect cbor_array assertion (both array and byte-string forms round-trip) and remove the stale is_err() check that only held before serialize_bytes.
Reduce the number of structs and indirection in RCAN v2
2 participants
Description
Adds rcan v2.
Delegationis an opaque delegation that can contain both v1 and v2. IfDelegationcontains a V1 delegation, encode will produce bytes that are readable by rcan v1. reading a v1 delegation viadecode_any<C>requiresC. Once read, a v1Delegationcan be serialized and deserialized without needingC, but the serialization format is not compatible with v1.TypedDelegation<C>is the above but you know that the bytes are compatible withC.V1Compat<C>serializes and deserializes exactly the same as 0.4rcan::RCan<C>for all serializers including weird ones (ron).Authorizeris mostly unchanged, but it works with chains ofDelegationorTypedDelegation<C>that can each contain v1 or v2. It has fns for both opaque and typed delegation chains, that use the same underlying code.Breaking Changes
Basically everything, but the concepts of the API remain roughly the same. We could have less breakage by renaming
TypedDelegation<C>toRcan<C>, but it needs to be a version increase because serialization via serde changed.Notes & open questions
Q: Is the typed or the opaque variant more frequently used? Depending on which we could name things, e.g.
DelegationandTypedDelegation<C>orDelegation<C>andOpaqueDelegationor something?Q: Is the
V1Compat<C>pulling its weight? What it does is allowing us to read v1Rcan<C>via serde, but we could accomplish the same by just importing rcan 0.4.1 in addition to rcan 0.5 and having some glue. I guess it is nice to not need 2 versions of the same crate, also we don't need a dep to ctserde. But really I don't know...Q: Should we keep calling a delegation a "RCan"?
Q:
TypedDelegation<C>can in principle keep both the bytes and the deserialized C, to stop us from deserializing multiple times. The question is: is this worth it?Q: Currently Delegation serialize uses textual format for human readable serializers. This looks nice, but requires dealing with the quirks of the various serializers. Alternatives would be 1. just declare that only encode/decode and postcard is stable. 2. use base64(postcard) as string for the human readable formats.
Todo