Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds a new CipherWrapper gRPC plugin with protobuf definitions and generated code. Three issues were found: a stale "KeyWrapper" service comment that contradicts the actual service name, missing protovalidate constraints on REQUIRED fields (plaintext, key_uri, ciphertext) that cause Validate() to always pass silently, and an inconsistent receiver type on CipherWrapperPluginClient.Type() (value receiver) versus the other methods (pointer receivers), which can cause interface satisfaction surprises.
PR Bot Information
Version: 1.18 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
5d32a140-1d7e-11f1-98e5-fde3886e1e37 - Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet
| // REQUIRED: The raw cryptographic material to be wrapped. | ||
| bytes plaintext = 1; |
There was a problem hiding this comment.
Bug: plaintext field uses wrong wire type for bytes
Field 1 (plaintext) is declared bytes plaintext = 1; but the raw descriptor in the generated .pb.go encodes it with wire type \f (field type 12), which corresponds to proto type bytes in the newer edition encoding. However, looking at the .pb.go struct tag: protobuf:"bytes,1,opt,name=plaintext,proto3" — this is consistent. The real concern is that plaintext contains raw cryptographic key material and is transmitted without any protection at the proto layer. There are no (validate.rules).bytes.min_len constraints on the REQUIRED fields (plaintext, key_uri, ciphertext), so a caller can pass an empty plaintext or empty key_uri and the server-side Validate() / ValidateAll() calls will always return nil — silently accepting invalid input.
Consider adding protovalidate rules (e.g. (buf.validate.field).bytes.min_len = 1 for plaintext/ciphertext and (buf.validate.field).string.min_len = 1 for key_uri) so that the generated validate() methods actually enforce the REQUIRED semantics documented in the comments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| CipherWrapperClient | ||
| } | ||
|
|
||
| func (s CipherWrapperPluginClient) Type() string { |
There was a problem hiding this comment.
Bug: Type() method on value receiver is inconsistent with pointer-receiver methods on CipherWrapperPluginClient
Type() is defined on a value receiver (CipherWrapperPluginClient), while IsInitialized(), GRPCServiceName(), and InitClient() are defined on pointer receivers (*CipherWrapperPluginClient). This means CipherWrapperPluginClient (value) satisfies Type() but only *CipherWrapperPluginClient satisfies the full api.PluginClient interface. Callers storing the struct by value will find only Type() available, leading to a compile-time interface mismatch if they try to use it as an api.PluginClient directly. This is consistent with the pattern in certificate_issuer_ext_plugin.pb.go, so it appears to be a generator issue — but it should still be flagged since it creates an asymmetric method set.
Consider changing the Type() method to a pointer receiver to make the full interface consistent:
| func (s CipherWrapperPluginClient) Type() string { | |
| func (s *CipherWrapperPluginClient) Type() string { | |
| return Type | |
| } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
1 participant
feat: Add CipherWrapper Plugin for Krypton
New Features
✨ Introduces a new
CipherWrapperplugin as part of the Krypton plugin system. This plugin provides a universal envelope encryption interface enabling the core engine to delegate cryptographic key wrapping and unwrapping to external KMS providers (AWS KMS, Google Cloud KMS, Azure Key Vault, HashiCorp Vault, or hardware HSMs via PKCS#11) over a local gRPC socket.Changes
proto/plugin/cipher_wrapper/v1/cipher_wrapper.proto: Defines theCipherWrappergRPC service withWrapandUnwrapRPCs, along with message types forWrapRequest,WrapResponse,UnwrapRequest, andUnwrapResponse. Supports optional fields for AAD, encryption context, IV, mechanism, and dynamic options to accommodate multiple provider types.proto/plugin/cipher_wrapper/v1/cipher_wrapper.pb.go: Auto-generated Go protobuf code for theCipherWrappermessage types, including field accessors and reflection support.proto/plugin/cipher_wrapper/v1/cipher_wrapper_grpc.pb.go: Auto-generated gRPC client/server stubs for theCipherWrapperservice, includingRegisterCipherWrapperServer,UnimplementedCipherWrapperServer, and handler implementations.proto/plugin/cipher_wrapper/v1/cipher_wrapper_ext_plugin.pb.go: Auto-generated plugin extension code that bridges theCipherWrappergRPC service with the plugin SDK'sapi.PluginServer/api.PluginClientinterfaces, enabling dynamic plugin registration and client initialization.proto/plugin/cipher_wrapper/v1/cipher_wrapper.pb.validate.go: Auto-generated validation code for allCipherWrappermessage types usingprotoc-gen-validate.📬 Subscribe to the Hyperspace PR Bot DL to get the latest announcements and pilot features!
PR Bot Information
Version:
1.18| 📖 Documentation | 🚨 Create Incident | 💬 Feedbackpull_request.opened5d32a140-1d7e-11f1-98e5-fde3886e1e37anthropic--claude-4.6-sonnet