chore: address comments

Author: kc1212

core/grpc/proto/kms-service-insecure.v1.proto

@@ -3,79 +3,52 @@ package kms_service.v1;
 
 import "kms.v1.proto";
 
+// WARNING: This service is insecure and should not be used in production.
+//
+// Since this is the insecure endpoint, we do not duplicate
+// the documentation. Please see the documentation of the secure endpoint.
+// All insecure RPCs (i.e., the ones that have `Insecure` in the prefix)
+// have the same semantics as the secure ones.
 service CoreServiceEndpoint {
-  // Perform the threshold KMS initialization.
-  // This call returns an error on the centralized KMS.
   rpc Init(kms.v1.InitRequest) returns (kms.v1.Empty);
 
-  // Start generating preprocessing materials for key generation asynchronously.
-  // This call returns an error on the centralized KMS.
   rpc KeyGenPreproc(kms.v1.KeyGenPreprocRequest) returns (kms.v1.Empty);
 
-  // This call returns an error on the centralized KMS.
   rpc GetKeyGenPreprocResult(kms.v1.RequestId) returns (kms.v1.KeyGenPreprocResult);
 
-  // Generate new keys asynchronously.
   rpc KeyGen(kms.v1.KeyGenRequest) returns (kms.v1.Empty);
 
-  // Return a URI where they can be accessed.
-  // The keys at the URI contains signature and authentication information.
-  // NOTE: Unprivileged and insecure call
   rpc GetKeyGenResult(kms.v1.RequestId) returns (kms.v1.KeyGenResult);
 
-  // Generate new keys asynchronously.
   rpc InsecureKeyGen(kms.v1.KeyGenRequest) returns (kms.v1.Empty);
 
-  // Return a URI where they can be accessed.
-  // The keys at the URI contains signature and authentication information.
-  // NOTE: Unprivileged and insecure call
   rpc GetInsecureKeyGenResult(kms.v1.RequestId) returns (kms.v1.KeyGenResult);
 
-  // Perform public decryption of a ciphertext and return the signed plaintext.
   rpc PublicDecrypt(kms.v1.PublicDecryptionRequest) returns (kms.v1.Empty);
 
-  // Get the public decryption result.
-  // This query fails if the result is not available yet.
   rpc GetPublicDecryptionResult(kms.v1.RequestId) returns (kms.v1.PublicDecryptionResponse);
 
-  // Perform user decryption of a ciphertext under a user-specified key and return a signcrypted
-  // share of the decrypted plaintext.
   rpc UserDecrypt(kms.v1.UserDecryptionRequest) returns (kms.v1.Empty);
 
-  // Get the user decryption result.
-  // This query fails if the result is not available yet.
   rpc GetUserDecryptionResult(kms.v1.RequestId) returns (kms.v1.UserDecryptionResponse);
 
-  // Start the CRS generation protocol asynchronously.
   rpc CrsGen(kms.v1.CrsGenRequest) returns (kms.v1.Empty);
 
-  // Get a reference to the CRS.
-  // This query fails if the result is not available yet.
   rpc GetCrsGenResult(kms.v1.RequestId) returns (kms.v1.CrsGenResult);
 
-  // Start the insecure CRS generation protocol asynchronously.
   rpc InsecureCrsGen(kms.v1.CrsGenRequest) returns (kms.v1.Empty);
 
-  // Get a reference to the insecure CRS.
-  // This query fails if the result is not available yet.
   rpc GetInsecureCrsGenResult(kms.v1.RequestId) returns (kms.v1.CrsGenResult);
 
-  // Create a new KMS context, it may be created without an existing one.
   rpc NewKmsContext(kms.v1.NewKmsContextRequest) returns (kms.v1.Empty);
 
-  // Destroy an existing KMS context.
   rpc DestroyKmsContext(kms.v1.DestroyKmsContextRequest) returns (kms.v1.Empty);
 
-  // Create a new custodian context, it may be created without an existing one.
   rpc NewCustodianContext(kms.v1.NewCustodianContextRequest) returns (kms.v1.Empty);
 
-  // Destroy an existing custodian context.
   rpc DestroyCustodianContext(kms.v1.DestroyCustodianContextRequest) returns (kms.v1.Empty);
 
-  // Get the public key that a custodian can use to encrypt a backup share
-  // before sending it to the KMS.
   rpc GetOperatorPublicKey(kms.v1.Empty) returns (kms.v1.OperatorPublicKey);
 
-  // Restore the contents of the KMS private storage from custodian backup shares.
   rpc CustodianBackupRestore(kms.v1.Empty) returns (kms.v1.Empty);
 }

core/grpc/proto/kms-service.v1.proto

@@ -24,6 +24,9 @@ pub enum IdentifierError {
 
     #[error("Invalid hex format: {0}")]
     InvalidHexFormat(#[from] hex::FromHexError),
+
+    #[error("Validation failure")]
+    ValidationFailure,
 }
 
 /// KeyId represents a unique identifier for a key in the system
@@ -349,29 +352,23 @@ macro_rules! impl_identifiers {
                     type Error = IdentifierError;
 
                     fn try_from(proto: v1::RequestId) -> Result<Self, Self::Error> {
-                        Self::from_str(&proto.request_id)
+                        let out = Self::from_str(&proto.request_id)?;
+                        if !out.is_valid() {
+                            return Err(Self::Error::ValidationFailure);
+                        }
+                        Ok(out)
                     }
                 }
 
                 impl<'a> TryFrom<&'a v1::RequestId> for $type {
                     type Error = IdentifierError;
 
                     fn try_from(proto: &'a v1::RequestId) -> Result<Self, Self::Error> {
-                        Self::from_str(&proto.request_id)
-                    }
-                }
-
-                impl TryFrom<Option<v1::RequestId>> for $type {
-                    type Error = IdentifierError;
-
-                    fn try_from(opt: Option<v1::RequestId>) -> Result<Self, Self::Error> {
-                        match opt {
-                            Some(proto) => $type::try_from(proto),
-                            None => Err(IdentifierError::InvalidLength {
-                                expected: ID_LENGTH,
-                                actual: 0,
-                            }),
+                        let out = Self::from_str(&proto.request_id)?;
+                        if !out.is_valid() {
+                            return Err(Self::Error::ValidationFailure);
                         }
+                        Ok(out)
                     }
                 }
             };

core/grpc/src/identifiers.rs

@@ -928,7 +928,7 @@ impl Client {
 
         let request_id = parse_optional_proto_request_id(
             &key_gen_result.request_id,
-            RequestIdParsingErr::General("invalid ID while retrieving public key".to_string()),
+            RequestIdParsingErr::Other("invalid ID while retrieving public key".to_string()),
         )
         .map_err(|e| anyhow::anyhow!(e.to_string()))?;
         tracing::debug!(
@@ -993,7 +993,7 @@ impl Client {
         )?;
         let request_id = parse_optional_proto_request_id(
             &key_gen_result.request_id,
-            RequestIdParsingErr::General("invalid request ID while retrieving key".to_string()),
+            RequestIdParsingErr::Other("invalid request ID while retrieving key".to_string()),
         )
         .map_err(|e| anyhow::anyhow!(e.to_string()))?;
         let key: S = self.get_key(&request_id, key_type, storage).await?;
@@ -1052,7 +1052,7 @@ impl Client {
         )?;
         let request_id = parse_optional_proto_request_id(
             &crs_gen_result.request_id,
-            RequestIdParsingErr::General("invalid request ID while processing CRS".to_string()),
+            RequestIdParsingErr::Other("invalid request ID while processing CRS".to_string()),
         )
         .map_err(|e| anyhow::anyhow!(e.to_string()))?;
         let pp = self.get_crs(&request_id, storage).await?;
@@ -6897,7 +6897,7 @@ pub(crate) mod tests {
         }
 
         wait_for_keygen_result(
-            req_keygen.request_id.clone().try_into().unwrap(),
+            req_keygen.request_id.clone().unwrap().try_into().unwrap(),
             preproc_req_id,
             kms_clients,
             internal_client,