Restructure Protobuf-based message implementation #148
Conversation
Regenerate the outdated Protobuf code with protoc v3.11.2. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
The package's documentation is cluttered with auto-generated stuff which is not a part of packages' public API. Separate auto-generated code into a sub-package. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
messages/protobuf/impl.go
Outdated
| @@ -32,7 +31,7 @@ func NewImpl() messages.MessageImpl { | |||
| func (*impl) NewFromBinary(data []byte) (messages.Message, error) { | |||
| msg := &pb.Message{} | |||
| if err := proto.Unmarshal(data, msg); err != nil { | |||
| return nil, fmt.Errorf("failed to unmarshal message wrapper: %s", err) | |||
| return nil, errors.Wrap(err, "failed to unmarshal message wrapper") | |||
There was a problem hiding this comment.
As you may know, Go 1.13 introduces new build-in error handling features and there is a compatibility package, golang.org/x/xerrors for earlier versions. How about using the new feature?
To use the feature, just replace %s to %w:
fmt.Errorf("failed to unmarshal message wrapper: %w", err)
There was a problem hiding this comment.
Thank you for pointing this out; I actually overlooked this new feature. The new feature looks almost the same as what pkg/errors provide. One notable difference is that pkg/errors automatically annotates errors with stack trace, which might be useful when analyzing problems and debugging. What do you think?
There was a problem hiding this comment.
I checked more carefully and found that xerrors package in fact annotates errors with stack trace, as well. Then I think we should rather use it in favor of pkg/error package.
The only question if we should give up on supporting Go 1.11 or use the compatibility package? Given that this is a component to be used by other projects, I tend to think of preserving better compatibility, even though it is less convenient.
There was a problem hiding this comment.
golang.org/x/xerrors supports Go 1.11 or later:
https://github.com/golang/xerrors/blob/master/go.mod
So we don't need to update our compatibility policy.
There was a problem hiding this comment.
Should we also include such a go directive in our go.mod?
There was a problem hiding this comment.
Yes, I will do. Thanks for creating the issue.
According to issue hyperledger-labs#106, the code base should eventually switch to use `golang.org/x/xerrors` package if favor of built-in `fmt` and `errors` packages. Switch this package to use `xerrors` before doing further changes. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
There is no benefit in promoting methods from generated Protobuf structures. As opposed to embedding Protobuf structures, declaring explicitly named fields allows to avoid method/field name conflicts. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
It is better to avoid copying and duplicating whole Protobuf structures over. Redefine structures implementing message interfaces to store pointers to Protobuf structures. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
There is no use of uninitialized message structures and the code is simpler when message structures are always initialized with dedicated constructor functions. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Commit message virtually includes the corresponding Prepare message. As opposed to decomposing the embedded Prepare into individual bits, represent it as a single field. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Liberate individual message type implementation from marshaling the generic message wrapper. This also allows to bring both marshaling and unmarshaling message type selectors close to each other. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
This will simplify further improvements to unit tests. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Check that individual bits of the embedded message are covered in certified message payload. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Protobuf does not guarantee deterministic serialization, thus it is not suitable to compute messages' signed/certified payload. Introduce helper functions to obtain authenticated content from Protobuf messages, and use those to implement SignedPayload/CertifiedPayload methods. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Message type should be covered in authenticated data to guarantee that it cannot be misinterpreted. This resolves hyperledger-labs#6. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Now inner definitions of Protobuf messages are not used to compute authenticated payload, the definitions can be simplified. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Normally, different implementations of the message interfaces will not be used together. Assuming that, try to avoid recreating Protobuf messages from abstract interfaces. Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>
sergefdrv
force-pushed
the
restructure-protobuf-messages
branch
from
97045db to
e9754f0
Compare
|
Summary of changes:
|
There was a problem hiding this comment.
Sorry for late response, the suggested change looks nice.
The non-determinism of protobuf's serialization was new to me. As described in godoc deterministic flag doesn't guarantee stable serialization over time, so it's recommended to "define their own canonicalization specification and implement their own serializer rather than relying on this API." That's what authen.go does.
This pull request restructures Protobuf-based message implementation in order to solve its two main problems: non-determinism and possible misinterpretation of messages' authenticated payload. Check individual commit messages for more details.
Resolves #6