Repository navigation
Conversation
7910e59 to
2172eb9
Compare
|
|
1 similar comment
|
|
96b921f to
e5a9270
Compare
|
|
1 similar comment
|
|
grondo
left a comment
grondo
left a comment
There was a problem hiding this comment.
This looks good to me!
Some high level comments after trying to digest most of this RFC. Note that you may have already considered and rejected some of this while drafting this first version, which appears very well designed! If so, then feel free to ignore comments.
| The signature part SHALL be an RFC 39 signature of the *message* class whose | ||
| payload is the 32-byte message digest. The signing mechanism SHALL be | ||
| selected by the flux-security configuration, as for other RFC 39 | ||
| signatures. | ||
|
|
||
| This specification relies on the following properties of the RFC 39 | ||
| message class: | ||
|
|
||
| - A message-class signature has a limited lifetime, on the order of | ||
| minutes, after which verification fails. | ||
|
|
||
| - A message-class signature cannot be verified as a signature of another | ||
| class, nor a signature of another class as a message-class signature. |
There was a problem hiding this comment.
It wasn't 100% clear what is meant here by message "class" and message-class signature. Is the definition of "message class" coming from another RFC?
There was a problem hiding this comment.
My bad! I meant to add to the PR description that this PR assumes that we'd enhance RFC 39 to have another type of signatures for messages with a tighter TTL and perhaps optional replay detection. Having those relaxed for jobspec is necessary but it would weaken message signatures unnecessarily.
| The *message digest* SHALL be the SHA-256 digest of the concatenation of | ||
| the following items, in order. Each item SHALL be encoded as a 4-byte | ||
| length in network byte order followed by the item's bytes: | ||
|
|
||
| #. The topic part, including its terminating NUL, if *cflag-topic* is set. | ||
|
|
||
| #. The payload, if *cflag-payload* is set. | ||
|
|
||
| #. The attachment, if *cflag-attachment* is set. | ||
|
|
||
| #. The content header, including any extension fields. |
There was a problem hiding this comment.
To avoid the ability for a forwarder to substitute a response from another request or stream to the sender, it may be useful to encode something extra in the message digest (to be included in the signature) for responses. Claude suggests a random nonce that the sender then verifies. For streaming RPCs a sequence number could be included to ensure message order isn't tampered with either, though I'm not 100% sure how this would be implemented in the actual code. Just leaving the comment here for consideration.
There was a problem hiding this comment.
Oh interesting. Good thought, I will ponder that one.
| A mechanism that rejects replays, such as MUNGE, would reject a | ||
| second verification of the same signature on the same host. Requests and | ||
| responses each have exactly one receiver, so verifying only at that | ||
| receiver, and only once, avoids rejecting legitimate messages. |
There was a problem hiding this comment.
Note: This note contradicts RFC 39 which states that EMUNGE_CRED_REPLAYED and EMUNGE_CRED_EXPIRED SHALL be treated as success.
| A receiver that requires authentication of a message SHALL: | ||
|
|
||
| #. Reject the message with EPERM if *cflag-signed* is clear. | ||
|
|
There was a problem hiding this comment.
This could get a bit confusing because "A receiver" here probably means a service receiving a response (reject with EPERM doesn't make sense otherwise), and a receiver of a response actually MUST accept unsigned error requests as noted below (for ENOSYS, EHOSTUNREACH type errors)
Claude had a good suggestion to add a Terminology section with the following definitions to more clearly separate the roles of general sender, receiver vs client, server.
- sender: the component that creates a message.
- forwarder: a broker, connector, or other component that relays a message without being its final recipient.
- receiver: the final recipient of a message, request or response.
- service: the receiver of a request.
- client: the receiver of a response.
|
|
||
| A response rejected by verification SHALL be discarded and MAY be logged. | ||
| A client that discards a response in a streaming RPC MAY terminate the RPC. | ||
|
|
There was a problem hiding this comment.
Suggestion: Specifically require that an unsigned error response terminates a streaming RPC with an error. A client MUST NOT treat an unsigned ENODATA as the normal end of a stream, so a responder MUST sign an ENODATA response.
| The content header contains only fields chosen by the sender. Forwarders | ||
| SHALL NOT modify it. | ||
|
|
||
| The *cflags* bit positions differ from those of the version 1 flags. The | ||
| correspondence between them is given in `Correspondence with Version 1`_. | ||
| Route presence is a transport flag in version 2. | ||
|
|
||
| Undefined *cflags* bits and the *cpad* field SHALL be zero. A receiver | ||
| SHALL reject a message with an undefined *cflags* bit set, or a nonzero | ||
| *cpad*, with EPROTO, because a content field may change the meaning of a | ||
| message. |
There was a problem hiding this comment.
Should it also be specified that forwarders ignore unknown cflags, so an updated protocol with an added cflag can pass through an older instance (e.g. when launching a newer flux version within an older)
| The *tpad* field and the undefined *tflags* bits are reserved. Senders SHALL | ||
| set them to zero, forwarders SHALL pass them through unchanged, and | ||
| receivers SHALL ignore them. | ||
|
|
||
| .. note:: | ||
|
|
||
| These rules let future features be added without a new message version. | ||
| A new content flag makes older receivers reject messages that use it, | ||
| which is the safe outcome for anything that changes a message's meaning. | ||
| A new transport flag or transport extension field is carried through | ||
| older forwarders and ignored by older receivers. |
There was a problem hiding this comment.
Not sure if this is the right place for this comment, but Claude had this observation about the transport header extensions:
Transport header extensions need a layout rule:
The spec reserves the bytes after the fixed transport fields for future revisions, but it does not say how a receiver finds a field there. I suggest stating the rule now, before any extension is defined:
- An extension field SHALL be present only if its tflags bit is set, unless its definition makes it always present.
- Extension fields SHALL appear in the order in which they are defined.
- The definition of each field SHALL fix its size, or SHALL define a length prefix for it.
- A receiver SHALL ignore bytes after the last field that it knows.
This uses the same approach as the content header. It is compatible with v1 and with current v2 implementations, because senders cannot include extension bytes until a revision defines them.
There was a problem hiding this comment.
Excellent refinement, thanks!
|
Tick the box to add this pull request to the merge queue (same as
|
|
That was some really excellent feedback, all of it. I'm preparing a new draft and will force push shorlty. |
e5a9270 to
e9c5f7f
Compare
|
|
Problem: message protocol v1 (RFC 3) does not support message signatures, multiple payloads, and is not extensible. Introduce message format v2 to address these needs. Assisted-by: Claude:opus-5.5
e9c5f7f to
89a19a0
Compare
|
|
|
Just force pushed some edits. I went down a rabbit hole with signed responses. Responses can be generated by the transport (like ENOSYS or EHOSTUNREACH), live RPCs are tracked, a client can't safely recycle a matchtag until it knows the server will no longer use it, etc. Ultimately, since we don't have an immediate need for signed requests, I just dropped it and left it as a future enhancement. I think we can add it without a protocol revision later when the need arises, and the issues are easily resolved - it just seemed like investing too much energy in that right now was misplaced. LMK if you are OK with that. I think I addressed your suggestions that were not response related. |
2 participants
Problem: message protocol v1 (RFC 3) does not support message signatures, multiple payloads, and is not extensible.
Introduce message format v2 to address these needs.
Assited-by: Claude:opus-5.5