feat: update introduction bundle encoding#43
Conversation
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
Technical Content
Looks good. I think Protobufs are a smart direction for now. I see versioning, I see everything I would expect.
Written Spec
The Top half is good. Really like the depth on the Overview, and reasoning for choices. Format specification is 👌 .
Areas that need work
- Duplicate content in different sections. Examples and Expected size repeat much of the same information. Process and examples are repeated. Would like to see specs treated more like Code in that they are "DRY"
- Duplication with other Specs. Think this specification should focus on solely the exchanging of payload types over human based channels. Signature mechanics, crypto protocols seem out of scope.
- This is not the correct location for this. We'll need to move this to another location
| # Introduction Bundle Encoding Specification | ||
|
|
||
| **Status:** Draft | ||
|
|
||
| ## Conventions |
There was a problem hiding this comment.
[Sand] This doesn't follow the suggested format for Logos specs https://github.com/logos-messaging/specs/blob/master/template.md. Not critical but will eventually need to be in that form
There was a problem hiding this comment.
Adapted to Logos specs format.
| **Algorithm:** [XEdDSA][xeddsa] — Ed25519-compatible signatures produced from | ||
| X25519 key pairs. This allows the same X25519 installation key to be used for | ||
| both Diffie-Hellman key agreement and signing, without maintaining a separate | ||
| Ed25519 identity key. |
There was a problem hiding this comment.
I don't think this belongs in the Spec for encoding. This is chat protocol specific, best to not repeat information in multiple places. Recommend removing
There was a problem hiding this comment.
I agree, removed it altogether.
| ``` | ||
| signed_message = domain_separator || ephemeral_pubkey | ||
| ``` |
There was a problem hiding this comment.
While this is a healthy approach. Message signing has integrity implications for the communication channel - It will be defined elsewhere.
The focus is solely on documenting the format for this interchange data, and the reasons for those choices
There was a problem hiding this comment.
I agree, removed it altogether.
| | `installation_pubkey` protobuf field | 34 (1 tag + 1 length + 32 value) | | ||
| | `ephemeral_pubkey` protobuf field | 34 (1 tag + 1 length + 32 value) | | ||
| | `signature` protobuf field | 66 (1 tag + 1 length + 64 value) | | ||
| | **Total binary payload** | **134 bytes** | | ||
| | Base64url encoded (no padding) | 179 characters | | ||
| | Prefix `logos_chatintro_1_` | 18 characters | | ||
| | **Total encoded string** | **~197 characters** | |
There was a problem hiding this comment.
While helpful this information seems like it would be hard to manage. And repeats with other sections.
[Dust] I'd cut this down to what is necessary, collapse this into different sections.
- Focus on good examples section. Input, output, statistics is much more helpful in my opinion.
- The spec says that key encoding is deferred to protobuf, but then references protobuf internals. I'd choose one path - either the encoding details matter or they do not.
There was a problem hiding this comment.
I agree, removed it altogether.
Focus on good examples section. Input, output, statistics is much more helpful in my opinion.
This is yet to come, after implementation is done.
| Encoding steps: | ||
|
|
||
| 1. Construct the `IntroBundle` protobuf message with the three fields above. | ||
| 2. Serialize using proto3 encoding (134 bytes). | ||
| 3. Encode as base64url without padding (179 characters). | ||
| 4. Prepend the version prefix: |
There was a problem hiding this comment.
[Sand] The process ought to be listed clearly enough. If a reader cannot easily follow along, then the instructions ought to be made clearer.
| 4. Prepend the version prefix: | ||
|
|
||
| ``` | ||
| logos_chatintro_1_<179-character base64url payload> |
There was a problem hiding this comment.
[Pebble] Examples are very helpful for implementors to at least smoke test their implementations. I'd convert this section to a TestVector approach.
Clear explicit bytes, so that implementers can verify.
There was a problem hiding this comment.
Removed it altogether, will add examples once implementation is ready.
| A V1 Introduction Bundle carries the cryptographic material needed for an | ||
| [X3DH][x3dh] key agreement: |
There was a problem hiding this comment.
[Sand] This seems hyper specific. The spec should theoretically work for another protocol stack that matches the same structure.
Since this protocol treats the internal bytes as blobs then they theoretically could be anything.
High Level I see this spec being referred to by a spec like Inbox. Inbox defines the keys and types, it then references this spec for its encoding.
| The `_` character is present in the alphabets of common binary-to-text | ||
| encodings (hex, base32, base64url) and may therefore appear inside | ||
| `<payload>`. Parsers MUST split the encoded string on the **first three** `_` | ||
| characters; everything after the third `_` is the payload verbatim. |
There was a problem hiding this comment.
everything after the third
_is the payload verbatim.
After some further thought, you may want to consider:
[Dust] Using two delimiters would make the implementations less prone to parsing issues.
Intro = [Preamble]<delim_1>[Bytes]
Preamble = [Prefix]<delim_2>[Namespace]<delim_2>[Version]
That way the parsing rules become:
- Introduction MUST contains one and only one <delimiter_1>
There was a problem hiding this comment.
What you meant is something like logos_chatintro_1~<payload>, where _ separates preamble fields and ~ separates preamble from payload. Then parsing becomes: split on ~ once -> left side is preamble, right side is payload. No "first three" counting. Did I get that right?
That's a good suggestion. However, imo, it adds complexity for marginal benefit. The "split on first 3 underscores" rule is simple and unambiguous, typically one line of code in any language. Two delimiters means two characters to document, two things to validate, and slightly more visual noise.
I'd lean toward keeping the single delimiter if you're fine with it.
There was a problem hiding this comment.
I'd lean toward keeping the single delimiter if you're fine with it.
Sounds good. Its a [Dust] do with it what you will.
And yes, you got that right. It's namely a style choice between being prescriptive, or being flexible.
I like the direction you are going.
|
I'd lets work on the "spec writing" and the implementation is parallel. I don't see any blockers on the content, and the written aspect can be iterated on separately. |
osmaczko
force-pushed
the
chore/add-introduction-encoding-specs
branch
from
a5ca4cd to
2f9963a
Compare
Moved specs to logos-messaging/specs#101. |
osmaczko
force-pushed
the
chore/add-introduction-encoding-specs
branch
3 times, most recently
from
0ca963e to
025b5d6
Compare
osmaczko
changed the title
|
@jazzz please re-review 🙏. The specs have been moved out, and this PR is now focused on the implementation. |
osmaczko
force-pushed
the
chore/add-introduction-encoding-specs
branch
from
025b5d6 to
9d55933
Compare
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
This Looks good.
I see intentional decisions around
- safe signature practices
- Type conversions and
- Testing
Theres some ways to make this more rusty but I don't see that as a blocker.
The main limitations of this work are caused by the existing code framework that needs a refactor, and is not in scope for this change-set.
| const BUNDLE_PREFIX: &str = "logos_chatintro_1_"; | ||
|
|
||
| const INTRO_DOMAIN_SEPARATOR: &[u8] = b"logos_chatintro_v1_bind"; |
There was a problem hiding this comment.
So close but so different. Any reason to not use the same prefix?
There was a problem hiding this comment.
Used BUNDLE_PREFIX for both 👍
| fn intro_binding_message(identity: &PublicKey, ephemeral: &PublicKey) -> Vec<u8> { | ||
| let mut message = Vec::with_capacity(INTRO_DOMAIN_SEPARATOR.len() + 64); | ||
| message.extend_from_slice(INTRO_DOMAIN_SEPARATOR); | ||
| message.extend_from_slice(identity.as_bytes()); |
There was a problem hiding this comment.
[Pebble] Adding the Identity key is redundant. Authenticity is provided by the signature it self.
There was a problem hiding this comment.
True, removed identity key from the message.
conversations/Cargo.toml
Outdated
| crate-type = ["staticlib","dylib"] | ||
|
|
||
| [dependencies] | ||
| base64ct = { version = "1.6", features = ["alloc"] } |
There was a problem hiding this comment.
I think the alloc feature here makes sense - the clarity and simplicity is an asset.
This means that the library cannot run in wasm or other environments so we may have to revisit in the future.
| @@ -1,64 +1,180 @@ | |||
| use base64ct::{Base64UrlUnpadded, Encoding}; | |||
There was a problem hiding this comment.
This line set off alarm bells. Is key material being encoded in base64?
Is a constant time operation required here or being cautious?
There was a problem hiding this comment.
The encoded data has no secret material, using regular base64 would be fine as well as there's nothing to leak via timing. I used constant-time as a reasonable default, as far as I know, there is no meaningful penalty.
There was a problem hiding this comment.
Not an issue now, but definitely in the realm of "crypto-smell" we will want to cleanup before release. We want our libraries and primitive decisions to be thoughtful, and intentional.
A reasonable default implies uncertainty in the approach taken. Either we need a constant time operation, or we do not.
Choosing it implies that its needed, which causes red flags in external reviewers.
There was a problem hiding this comment.
That's fair, I'll follow principle of least surprise and move to non-constat-time version.
| .signature | ||
| .as_ref() | ||
| .try_into() | ||
| .map_err(|_| ChatError::BadBundleValue("invalid signature length".into()))?; |
There was a problem hiding this comment.
[Dust] From a defensive code perspective. I'd consider validating the signature here.
Its possible for this type to create an invariant that Introduction always contains a valid ephemeral key.
This would make reasoning about this object easier if it was never possible to create an invalid Introduction.
That would contain the validation logic within this file and make it harder for future contributors to make mistakes.
There was a problem hiding this comment.
Good point, adapted the code 👍
conversations/src/inbox/handler.rs
Outdated
| PrekeyBundle { | ||
| identity_key: self.ident.public_key(), | ||
| signed_prekey: signed_prekey, | ||
| signature: [0u8; 64], | ||
| signed_prekey, | ||
| signature, | ||
| onetime_prekey: None, | ||
| } |
There was a problem hiding this comment.
While not strictly related to this work, I think it would make sense to change this to return an Introduction, rather than using the PrekeyBundle Type. The dependence on PrekeyBundle ought to be contained as tight as possible.
Not required for this PR mostly flagging follow up pieces of work
There was a problem hiding this comment.
I've reworked it to return Introduction.
crypto/src/xeddsa_sign.rs
Outdated
|
|
||
| use rand_core::{CryptoRng, RngCore}; | ||
| use x25519_dalek::{PublicKey, StaticSecret}; | ||
| use xeddsa::{xed25519, Sign, Verify}; |
There was a problem hiding this comment.
Great work solving the key-conversion issue.
Though from a protocol POV I don't love using XEdDSA here.
Out-of-scope for now, but a cleaner approach would be to update Identity to use an Ed25519 SigningKey and derive the X25519 StaticSecret as needed.
| hex::encode(self.installation_key.as_bytes()), | ||
| hex::encode(self.ephemeral_key.as_bytes()), | ||
| ); | ||
| impl From<Introduction> for Vec<u8> { |
| secret: &StaticSecret, | ||
| ephemeral: &PublicKey, | ||
| rng: R, | ||
| ) -> [u8; 64] { |
There was a problem hiding this comment.
[Pebble] I'd create a Newtype using struct Ed25519Signature { sig: [u8; 64] }
That way the context is bound with the datatype as well as allowing convenience functions to be added in the future.
osmaczko
force-pushed
the
chore/add-introduction-encoding-specs
branch
from
9d55933 to
4f44f3d
Compare
osmaczko
force-pushed
the
fix/ci-checks
branch
from
f20f9f2 to
0082967
Compare
osmaczko
force-pushed
the
chore/add-introduction-encoding-specs
branch
2 times, most recently
from
2ed31d1 to
25d5cd8
Compare
osmaczko
force-pushed
the
chore/add-introduction-encoding-specs
branch
4 times, most recently
from
f7313e1 to
3d6ac7d
Compare
|
@jazzz thanks for review! I've addressed your comments, please re-check 🙏 |
jazzz
left a comment
jazzz
left a comment
There was a problem hiding this comment.
A few comments, but looks its much improved. Love it.
osmaczko
force-pushed
the
chore/add-introduction-encoding-specs
branch
from
3d6ac7d to
c1902ed
Compare
2 participants
closes: #27