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.

Sign up for GitHub

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

noise: specify proto2 #456

Merged
merged 3 commits into from
Sep 20, 2022
Merged

noise: specify proto2 #456

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bedf06
noise: specify proto2
marten-seemann Sep 19, 2022
df395cf
noise: specify optionality of fields in protobuf
marten-seemann Sep 20, 2022
96ba2fe
noise: bump version
marten-seemann Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions noise/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ When decrypted, the payload contains a serialized [protobuf][protobuf]
`NoiseHandshakePayload` message with the following schema:

``` protobuf
syntax = "proto2";

message NoiseHandshakePayload {
bytes identity_key = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bytes identity_key = 1;
required bytes identity_key = 1;

If I am not mistaken proto2 requires either required, optional or repeated.

https://developers.google.com/protocol-buffers/docs/proto#specifying-rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any objections against making them all optional? If we ever want to send early data with the first handshake message, we can then use this Protobuf.

Copy link
Member

Choose a reason for hiding this comment

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

For the record, a proto3 encoder will simply provide the default value to the user in case any of these fields are optional and are not set before encoding with a proto2 encoder.

I am fine with these fields being optional. Though keep in mind that that would allow the above incompatibility at the schema level. I.e. a proto2 implementation could send a message to an old proto3 implementation that is incompatible.

How about making them optional and documenting that some implementations (using proto3) expect these fields to be set on the second and third Noise handshake message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about making them optional and documenting that some implementations (using proto3) expect these fields to be set on the second and third Noise handshake message?

I think our handshake description specifies exactly that. Not sure if we need to add any text, mind suggesting something if you feel strongly?
Sending this Protobuf in the first handshake is forbidden by this spec anyway (although we don't enforce it), so using this Protobuf for that purpose would require a spec change anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I think our handshake description specifies exactly that.

Yes, you are right. Sorry for that.

bytes identity_sig = 2;
Expand Down