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

Add draft CapTP spec #42

Merged
merged 13 commits into from
May 25, 2023
Merged

Add draft CapTP spec #42

merged 13 commits into from
May 25, 2023

Conversation

tsyesika
Copy link
Contributor

No description provided.

@tsyesika tsyesika changed the title Add first draft CapTP spec Add draft CapTP spec Apr 20, 2023
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
@tsyesika
Copy link
Contributor Author

Thanks for the review @davexunit, I've addressed your feedback.

@zenhack
Copy link
Collaborator

zenhack commented Apr 24, 2023

Some general high-level/structural comments:

  • I'm not sure who the target audience of this document is supposed to be; there's a lot of stuff in here that I feel like would be incomprehensible to someone who hadn't already grokked what what using Goblins or something like it was like, and needlessly verbose to someone who has.

    My intuition is that probably the spec should be written for an audience that already has an understanding of what CapTP does for them, but may not know how that works at the protocol level at all.

  • The document floats back and forth between describing things that are protocol-level considerations and things that are really descriptions of a typical API that a library might provide. This is confusing, and more clearly marking and organizing these would go a long way to making this easier to digest.

Some of the lower-level descriptions are useful, but I think this needs a lot of organizational work.

I have an obvious bias, but if I were going to take a whack at it, I'd probably start off by stealing much of the language and structure from rpc.capnp; bits will need to be tweaked for the differences between the protocols, but the general shape of the protocol is similar enough that I think drawing heavily on that would probably save a lot of work. Feedback on that documentation has generally been extremely positive, and multiple implementations have been written when referencing it.

@tsyesika tsyesika mentioned this pull request Apr 24, 2023
Netlayers specification. Once a secure channel is established, we MUST to
perform the following in this order:

1. Create a per-session cryptography key pair (see
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why a session key pair is in this layer.

The Agoric CapTP API doesn't use a session key pair.

cc @erights @kriskowal @michaelfig

Copy link
Collaborator

Choose a reason for hiding this comment

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

The 3PH presentation from a couple meetings ago goes over how they're used, but I do think it might be good to add some abstraction boundaries; probably most of the crypto should be netlayer specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. The session key pair is only used for third party handoffs. It's used in the certificates that the gifter and the receiver make, see the 3PH section, desc:handoff-give and desc:handoff-receive sections for details in how they work.

There certainly could be more moved into netlayers that I've currently put in CapTP, however the interface between the two specifications would have to be higher, which isn't necessarily a bad thing, but just something to keep in mind. The netlayer currently is only responsible for opening up channel between two sessions and delivering messages through this channel, it's fairly limited in scope.

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 reworking this can wait for separate PRs after merging, but I think having connection semantics cleanly separated from the netlayer is important. Note, in particular, capnp often gets used over local unix sockets, where layering crypto on top would be silly...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I promoted this thread to an issue: #53.

Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Thanks for the very thorough explanation!

draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved

```text
<op:bootstrap [answer-position ; positive integer or 0
resolve-me-desc]> ; desc:import-object | desc:import-promise
Copy link

@zarutian zarutian Apr 25, 2023

Choose a reason for hiding this comment

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

resolve-me-desc here and in op:deliver seem to be redundant functionality of what op:listen does. See #15 for why.

<op:deliver [to-desc ; desc:export
args ; sequence
answer-pos ; positive integer | false
resolve-me-desc]> ; desc:import-object | desc:import-promise
Copy link

@zarutian zarutian Apr 25, 2023

Choose a reason for hiding this comment

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

resolve-me-desc here and in op:bootstrap seem to be redundant functionality of what op:listen does. See #15 for why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually hadn't seen #15 before so thank you for linking it. I think the concept of dropping the resolve-me-desc and then simplifying the op:deliver and op:deliver-only messages into a single op:deliver message and then relying on op:listenmore is really interesting. I discussed it briefly with Christine and she thinks it's an interesting idea too.

I think this for now, this would be better addressed as a follow up PR if the group generally think this is a good idea (I think my initial impression is that I agree, this might be a nicer solution) we go ahead and make the change later.

1. A gift ID that is positive integer.
2. A `desc:import-object` or `desc:import-promise` which has been exported
within the given CapTP session.

Choose a reason for hiding this comment

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

missing an argument:
3. VatId of the recipiant of the gift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current spec and the Goblins' implementation of CapTP don't currently have the concept of vats or VatIds so this isn't part of the handoff process. What would the VatId correspond to and how would this affect the third party handoff process?

Choose a reason for hiding this comment

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

In Spritely Goblins case here it would refer to the publiv key (or the cryptographics hash there of) of the endpont that is to get the gift.

@markm has the nifty phrase "My name is how you know it is me." which captures the essence of the idea of self authenticated designators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is needed or adds anything to the handoff process.

The desc:handoff-give and the desc:handoff-receive already contain enough information that ensures that only the exporter can pick up the gift left for them.


This should have been sent with the `op:deliver` operation, the response the
bootstrap object should give is the gift which was (or will be) deposited.

Copy link

@zarutian zarutian Apr 26, 2023

Choose a reason for hiding this comment

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

This actually needs two arguments:

  1. gift id of the gift
  2. VatId of the giver

(Related to this comment )

It's important to have a warning which explains what the status of the
CapTP spec is. Prospective implementors should be aware that the spec
is initially based on a single implementation, and that it is going to
change as we develop the implementation further.
Copy link
Collaborator

@zenhack zenhack left a comment

Choose a reason for hiding this comment

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

Sorry this took me so long to get through. I've tried to restrict myself to commenting on things that are about the spec, rather than critiquing the protocol itself; I certainly came across a number of substantive things that seem worth improving, but we'll get those as part of our ongoing discussions.

In addition to my in-line comments, I will also say I think the doc could use a description up front about how the doc is organized. I spent the first chunk of my time reading this saying "I wish there was just a reference for all the message types somewhere" only to eventually get to that at the end. The nature of a doc like this is that you tend to skip around, so when one section references a particular structure, it's good to know how to go look up what that is; the beginning of the doc should orient the reader so they can do this.

A session is between two entities which are communicating via CapTP through a
OCapN Netlayer's channel. A CapTP life cycle consists of the following stages:

1. A session is established pairwise between machines.
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are a couple things that are weird about this presentation:

  1. At this level of abstraction, there aren't actually any interesting state transitions; it's just (1) open connection (2) do stuff, (3) close connection. So describing this as a "lifecycle" seems needlessly baroque; you could just say CapTP operates over a reliable, in-order stream (i.e. something like TCP) and then just say "these are some things that can happen."
  2. Buried in one of the bullets below is 3PH, which can necessitate creating more sessions. Some of them may even outlive the current session, so framing this as an [open, do stuff, close] workflow is at best confusing and at worst actually wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've re-written this paragraph a little and moved away from life cycle closer to what you're suggesting here, let me know if this is what you had in mind.

Netlayers specification. Once a secure channel is established, we MUST to
perform the following in this order:

1. Create a per-session cryptography key pair (see
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 reworking this can wait for separate PRs after merging, but I think having connection semantics cleanly separated from the netlayer is important. Note, in particular, capnp often gets used over local unix sockets, where layering crypto on top would be silly...

draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved

When an object sends a message to another object across a CapTP boundary, it
either expects a reply or not. If the object expects a reply a
[promise](#promises) is created to represent the reply. To do this, we send a
Copy link
Collaborator

Choose a reason for hiding this comment

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

This talk of "creating a promise" feels like it's treading into implementation details. I don't necessarily think that's unreasonable, but it's a bit weird to do it this way while making a point of avoiding talking about connection tables.

Messages can be sent to a promise as if it were the resolved object. These sent
messages will be relayed to the eventual object if it is `fulfill`ed to one.
There is also a "promise resolver" object which is used to provide the promise a
value or break on an error. The resolver object has two methods:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The resolver half of this seems like it's describing a library API, not a protocol level concern, unless I'm missing something? For inspiration on how to express what I think this is trying to get at in more declarative terms, I would look at the comments for the Resolve message type in rpc.capnp, as well as the senderPromise variant of CapDescriptor.

...which, come to think of it: does goblins have an equivalent of resolve and if so what is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Goblins promises are built upon regular actors/objects and use op:deliver/op:deliver-only to function. This means that a promise is created via op:deliver (or in Goblins, you can also spawn them as you can with any other object, and share them within a message over the CapTP boundary). The promise object can have messages sent to it, and these will be forwarded to the object (or promise) it eventually resolves to, unless it breaks.

Goblins doesn't have a CapTP level op:resolve or anything similar; it simply has this resolver object which is spawned at the same time as the promise. When it's sent a message, it expects the first symbol to be 'fulfill' or 'break', followed by one or more values.

This is how promises work, both over CapTP and locally, and why this "promise API" documentation ends up in the specification. There are two such object APIs in the CapTP spec from Goblins' perspective: the bootstrap object with its three "methods", and the promise resolver with its two "methods".

draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Show resolved Hide resolved
draft-specifications/CapTP Specification.md Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
…ation for now

These may want to be removed or changed in the future, but this is to better
reflect how Goblins currently works and the features it provides.
@tsyesika
Copy link
Contributor Author

tsyesika commented May 18, 2023

Thanks for everyone's feedback so far. I've hopefully addressed everything. I'd like to propose we could merge this as a preliminary draft so we can begin opening up issues where we have disagreement and begin working on followup PRs as we come to consensus.

Just replying to one of Ian's comments here as Github for some reason didn't let me inline:

I think reworking this can wait for separate PRs after merging, but I think having connection semantics cleanly separated from the netlayer is important. Note, in particular, capnp often gets used over local unix sockets, where layering crypto on top would be silly...

This not crypto which is used to encrypt all messages, it's used in just two places:

  • Once when setting up the session in the the op:start-session operation.
  • On each handoff as part of the handoff certificates (desc:handoff-give and desc:handoff-receive).

I'm suspecting this overhead is relatively minor and it is integral to the design. I think if you still think this is an issue, maybe this is better as a follow up issue as it would involve changing how handoffs work in a substantial way that would be better as it's own change set. I think two user space programs on the same machine speaking over unix sockets may still wish to have this security though.

@zenhack
Copy link
Collaborator

zenhack commented May 18, 2023

I'm fine with merging this and addressing outstanding issues separately.

@tsyesika
Copy link
Contributor Author

I'm thinking it'd be good if we could take a vote on merging this in tomorrow's meeting (@jar398) It'd be good if folks could take a look if you've not already got to it and let me know if you have blocking feedback.

@jar398
Copy link
Contributor

jar398 commented May 22, 2023

I must be missing something - what is the risk of merging? I don't see that having something in the repo implies any kind of endorsement; traditionally endorsements would be spelled out in the document itself. All we're doing is making the document easier to find, and that seems like a good thing.

@dckc
Copy link
Collaborator

dckc commented May 22, 2023

Agoric hasn't completed its review; we'd like more time.

Copy link
Collaborator

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

More comments—a mixture of grammar/Markdown suggestions, requests for clarification (in some cases including section reorganization, in some cases consistent use of vocabulary such as session vs. party, and in others just miscellany), and bigger questions such as where the line is drawn between convention and requirement (particularly with respect to method identification and multi-valued promise resolution).

draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
draft-specifications/CapTP Specification.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@dckc dckc left a comment

Choose a reason for hiding this comment

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

Agoric has completed its review now. Modulo the issues we've raised and any editorial stuff from @gibson042 , this looks OK to merge.

cc @erights

@tsyesika tsyesika merged commit d3c38a1 into main May 25, 2023
@tsyesika
Copy link
Contributor Author

I've merged this as per the discussion we had in our last meeting.

@dckc
Copy link
Collaborator

dckc commented May 25, 2023

It's not clear what you decided to do with editorial input from @gibson042 ... github makes it look like some of it is not resolved. Did you consider those and prefer to not make the suggested edits? If so, very well. But if you have not yet considered them, please do.

@tsyesika
Copy link
Contributor Author

Oh I think github had "helpfully" hidden a lot of his feedback, to me it looked like I had addressed it. I'll follow up with another PR which addresses the rest of his feedback, sorry for missing it!

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.

7 participants