Skip to content

Commit 7fe64b2

Browse files
gregorydemaygregorydemay
committed
Merge branch 'gdemay/CRP-2020-transient-internal-errors' into 'master'
refactor(crypto): CRP-2020 all CSP vault client operations return a `TransientInternalError` in case of an RPC error Operations on the CSP vault client may always return a [`tarpc::client::RpcError`](https://github.com/google/tarpc/blob/bed85e282710a5d21444fd25a5c0f55796ffc889/tarpc/src/client.rs#L177) in case for example an RPC connection error occurs. For each operation, this low-level error from the [Tarpc framework](https://docs.rs/tarpc/latest/tarpc/) is mapped to some higher level error. This MR consolidates the error handling in such a case by always returning some variant named `TransientInternalError`, such that `ErrorReproduciblity::is_reproducibility` is always `false` on this variant whenever `ErrorReproduciblity` is implemented by the high level error. This affects the following operations: 1. `BasicSignatureCspVault::sign` 2. `MultiSignatureCspVault::multi_sign` 3. `ThresholdSignatureCspVault::threshold_sign`. This also prevents `ThresholdSigner::sign_threshold` from panicking in case of transient error (solves [CRP-1291](https://dfinity.atlassian.net/browse/CRP-1291)). 4. `SecretKeyStoreCspVault::sks_contains` 5. `NiDkgCspVault::create_dealing` 6. `TlsHandshakeCspVault::tls_sign` 7. `IDkgProtocolCspVault::idkg_create_dealing` 8. `IDkgProtocolCspVault::idkg_verify_dealing_private` 9. `IDkgProtocolCspVault::idkg_load_transcript` 10. `IDkgProtocolCspVault::idkg_load_transcript_with_openings` 11. `IDkgProtocolCspVault::idkg_retain_active_keys` 12. `IDkgProtocolCspVault::idkg_open_dealing` 13. `ThresholdEcdsaSignerCspVault::ecdsa_sign_share` See merge request dfinity-lab/public/ic!12574
2 parents dfec7ee + 8e59cc9 commit 7fe64b2

File tree

18 files changed

+133
-80
lines changed

18 files changed

+133
-80
lines changed

‎rs/crypto/internal/crypto_lib/threshold_sig/bls12_381/src/api/dkg_errors/imported_conversions.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,8 @@ mod create_dealing_error_conversions_v2 {
8181
panic_prefix, error
8282
);
8383
}
84-
CspDkgCreateReshareDealingError::InternalError(error) => {
85-
DkgCreateDealingError::InternalError(InternalError {
84+
CspDkgCreateReshareDealingError::TransientInternalError(error) => {
85+
DkgCreateDealingError::TransientInternalError(InternalError {
8686
internal_error: error.internal_error,
8787
})
8888
}

‎rs/crypto/internal/crypto_lib/threshold_sig/bls12_381/src/api/ni_dkg_errors.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,8 @@ pub enum CspDkgCreateDealingError {
134134
/// Hardware error: This machine cannot handle this request because some
135135
/// parameter was too large.
136136
SizeError(SizeError),
137-
// An internal error, e.g. an RPC error.
138-
InternalError(InternalError),
137+
/// Transient internal error, e.g. an RPC error.
138+
TransientInternalError(InternalError),
139139
}
140140

141141
impl From<EncryptAndZKProveError> for CspDkgCreateDealingError {
@@ -183,8 +183,8 @@ pub enum CspDkgCreateReshareDealingError {
183183
/// Hardware error: This machine cannot handle this request because some
184184
/// parameter was too large.
185185
SizeError(SizeError),
186-
// An internal error, e.g. an RPC error.
187-
InternalError(InternalError),
186+
/// Transient internal error, e.g. an RPC error.
187+
TransientInternalError(InternalError),
188188
}
189189

190190
impl From<EncryptAndZKProveError> for CspDkgCreateReshareDealingError {
@@ -230,8 +230,8 @@ impl From<CspDkgCreateDealingError> for CspDkgCreateReshareDealingError {
230230
CspDkgCreateDealingError::SizeError(error) => {
231231
CspDkgCreateReshareDealingError::SizeError(error)
232232
}
233-
CspDkgCreateDealingError::InternalError(error) => {
234-
CspDkgCreateReshareDealingError::InternalError(error)
233+
CspDkgCreateDealingError::TransientInternalError(error) => {
234+
CspDkgCreateReshareDealingError::TransientInternalError(error)
235235
}
236236
}
237237
}
@@ -269,8 +269,8 @@ impl From<CspDkgCreateReshareDealingError> for CspDkgCreateDealingError {
269269
CspDkgCreateReshareDealingError::SizeError(error) => {
270270
CspDkgCreateDealingError::SizeError(error)
271271
}
272-
CspDkgCreateReshareDealingError::InternalError(error) => {
273-
CspDkgCreateDealingError::InternalError(error)
272+
CspDkgCreateReshareDealingError::TransientInternalError(error) => {
273+
CspDkgCreateDealingError::TransientInternalError(error)
274274
}
275275
}
276276
}

‎rs/crypto/internal/crypto_service_provider/csp_proptest_utils/src/lib.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ mod csp_basic_signature_error {
135135
UnsupportedAlgorithm => {algorithm in arb_algorithm_id()},
136136
WrongSecretKeyType => {algorithm in arb_algorithm_id(), secret_key_variant in ".*"},
137137
MalformedSecretKey => {algorithm in arb_algorithm_id()},
138-
InternalError => {internal_error in ".*"}
138+
TransientInternalError => {internal_error in ".*"}
139139
);
140140
}
141141

@@ -245,7 +245,7 @@ mod csp_multi_signature_error {
245245
SecretKeyNotFound => {algorithm in arb_algorithm_id(), key_id in arb_key_id()},
246246
UnsupportedAlgorithm => {algorithm in arb_algorithm_id()},
247247
WrongSecretKeyType => {algorithm in arb_algorithm_id(), secret_key_variant in ".*"},
248-
InternalError => {internal_error in ".*"}
248+
TransientInternalError => {internal_error in ".*"}
249249
);
250250
}
251251

@@ -270,7 +270,7 @@ mod csp_threshold_sign_error {
270270
UnsupportedAlgorithm => {algorithm in arb_algorithm_id()},
271271
WrongSecretKeyType => {},
272272
MalformedSecretKey => {algorithm in arb_algorithm_id()},
273-
InternalError => {internal_error in ".*"}
273+
TransientInternalError => {internal_error in ".*"}
274274
);
275275
}
276276

@@ -279,6 +279,6 @@ mod csp_secret_key_store_contains_error {
279279
use ic_crypto_internal_csp::vault::api::CspSecretKeyStoreContainsError;
280280

281281
proptest_strategy_for_enum!(CspSecretKeyStoreContainsError;
282-
InternalError => {internal_error in ".*"}
282+
TransientInternalError => {internal_error in ".*"}
283283
);
284284
}

‎rs/crypto/internal/crypto_service_provider/csp_proptest_utils/src/tests.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,14 @@ macro_rules! should_have_a_strategy_for_each_variant {
3434
use ic_crypto_internal_csp::vault::api::CspBasicSignatureError;
3535
should_have_a_strategy_for_each_variant!(
3636
CspBasicSignatureError,
37-
CspBasicSignatureError::InternalError {
37+
CspBasicSignatureError::TransientInternalError {
3838
internal_error: "dummy error to match upon".to_string(),
3939
},
4040
SecretKeyNotFound { .. },
4141
UnsupportedAlgorithm { .. },
4242
WrongSecretKeyType { .. },
4343
MalformedSecretKey { .. },
44-
InternalError { .. }
44+
TransientInternalError { .. }
4545
);
4646

4747
use ic_crypto_internal_csp::types::CspSignature;
@@ -109,13 +109,13 @@ should_have_a_strategy_for_each_variant!(
109109
use ic_crypto_internal_csp::vault::api::CspMultiSignatureError;
110110
should_have_a_strategy_for_each_variant!(
111111
CspMultiSignatureError,
112-
CspMultiSignatureError::InternalError {
112+
CspMultiSignatureError::TransientInternalError {
113113
internal_error: "dummy error to match upon".to_string(),
114114
},
115115
SecretKeyNotFound { .. },
116116
UnsupportedAlgorithm { .. },
117117
WrongSecretKeyType { .. },
118-
InternalError { .. }
118+
TransientInternalError { .. }
119119
);
120120

121121
use ic_crypto_internal_csp::vault::api::CspMultiSignatureKeygenError;
@@ -133,21 +133,21 @@ should_have_a_strategy_for_each_variant!(
133133
use ic_crypto_internal_csp::api::CspThresholdSignError;
134134
should_have_a_strategy_for_each_variant!(
135135
CspThresholdSignError,
136-
CspThresholdSignError::InternalError {
136+
CspThresholdSignError::TransientInternalError {
137137
internal_error: "dummy error to match upon".to_string(),
138138
},
139139
SecretKeyNotFound { .. },
140140
UnsupportedAlgorithm { .. },
141141
WrongSecretKeyType { .. },
142142
MalformedSecretKey { .. },
143-
InternalError { .. }
143+
TransientInternalError { .. }
144144
);
145145

146146
use ic_crypto_internal_csp::vault::api::CspSecretKeyStoreContainsError;
147147
should_have_a_strategy_for_each_variant!(
148148
CspSecretKeyStoreContainsError,
149-
CspSecretKeyStoreContainsError::InternalError {
149+
CspSecretKeyStoreContainsError::TransientInternalError {
150150
internal_error: "dummy error to match upon".to_string(),
151151
},
152-
InternalError { .. }
152+
TransientInternalError { .. }
153153
);

‎rs/crypto/internal/crypto_service_provider/src/api/threshold/threshold_sign_error.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ pub enum CspThresholdSignError {
1616
MalformedSecretKey {
1717
algorithm: AlgorithmId,
1818
},
19-
InternalError {
19+
TransientInternalError {
2020
internal_error: String,
2121
},
2222
}
@@ -64,8 +64,8 @@ impl fmt::Display for CspThresholdSignError {
6464
"Unable to parse the secret key with algorithm id {:?}",
6565
algorithm
6666
),
67-
CspThresholdSignError::InternalError { internal_error } => {
68-
write!(f, "Internal error: {}", internal_error)
67+
CspThresholdSignError::TransientInternalError { internal_error } => {
68+
write!(f, "Transient internal error: {}", internal_error)
6969
}
7070
}
7171
}

‎rs/crypto/internal/crypto_service_provider/src/vault/api.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ pub enum CspBasicSignatureError {
4747
MalformedSecretKey {
4848
algorithm: AlgorithmId,
4949
},
50-
InternalError {
50+
TransientInternalError {
5151
internal_error: String,
5252
},
5353
}
@@ -72,7 +72,7 @@ pub enum CspMultiSignatureError {
7272
algorithm: AlgorithmId,
7373
secret_key_variant: String,
7474
},
75-
InternalError {
75+
TransientInternalError {
7676
internal_error: String,
7777
},
7878
}
@@ -104,7 +104,7 @@ pub enum CspThresholdSignatureKeygenError {
104104

105105
#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
106106
pub enum CspSecretKeyStoreContainsError {
107-
InternalError { internal_error: String },
107+
TransientInternalError { internal_error: String },
108108
}
109109

110110
#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
@@ -157,7 +157,7 @@ pub enum CspTlsSignError {
157157
SigningFailed {
158158
error: String,
159159
},
160-
InternalError {
160+
TransientInternalError {
161161
internal_error: String,
162162
},
163163
}

‎rs/crypto/internal/crypto_service_provider/src/vault/mod.rs

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -40,11 +40,8 @@ impl From<CspBasicSignatureError> for CryptoError {
4040
internal_error: "Malformed secret key".to_string(),
4141
}
4242
}
43-
// TODO(CRP-1262): using InvalidArgument here is not ideal.
44-
CspBasicSignatureError::InternalError { internal_error } => {
45-
CryptoError::InvalidArgument {
46-
message: format!("Internal error: {}", internal_error),
47-
}
43+
CspBasicSignatureError::TransientInternalError { internal_error } => {
44+
CryptoError::TransientInternalError { internal_error }
4845
}
4946
}
5047
}
@@ -73,10 +70,8 @@ impl From<CspMultiSignatureError> for CryptoError {
7370
"Wrong secret key type: expected {algorithm:?} but found {secret_key_variant}"
7471
),
7572
},
76-
CspMultiSignatureError::InternalError { internal_error } => {
77-
CryptoError::InvalidArgument {
78-
message: internal_error,
79-
}
73+
CspMultiSignatureError::TransientInternalError { internal_error } => {
74+
CryptoError::TransientInternalError { internal_error }
8075
}
8176
}
8277
}
@@ -85,8 +80,8 @@ impl From<CspMultiSignatureError> for CryptoError {
8580
impl From<CspSecretKeyStoreContainsError> for CryptoError {
8681
fn from(e: CspSecretKeyStoreContainsError) -> Self {
8782
match e {
88-
CspSecretKeyStoreContainsError::InternalError { internal_error } => {
89-
CryptoError::InternalError { internal_error }
83+
CspSecretKeyStoreContainsError::TransientInternalError { internal_error } => {
84+
CryptoError::TransientInternalError { internal_error }
9085
}
9186
}
9287
}

‎rs/crypto/internal/crypto_service_provider/src/vault/remote_csp_vault/tarpc_csp_vault_client.rs

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ impl BasicSignatureCspVault for RemoteCspVault {
255255
key_id,
256256
))
257257
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
258-
Err(CspBasicSignatureError::InternalError {
258+
Err(CspBasicSignatureError::TransientInternalError {
259259
internal_error: rpc_error.to_string(),
260260
})
261261
})
@@ -288,7 +288,7 @@ impl MultiSignatureCspVault for RemoteCspVault {
288288
key_id,
289289
))
290290
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
291-
Err(CspMultiSignatureError::InternalError {
291+
Err(CspMultiSignatureError::TransientInternalError {
292292
internal_error: rpc_error.to_string(),
293293
})
294294
})
@@ -323,7 +323,7 @@ impl ThresholdSignatureCspVault for RemoteCspVault {
323323
key_id,
324324
))
325325
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
326-
Err(CspThresholdSignError::InternalError {
326+
Err(CspThresholdSignError::TransientInternalError {
327327
internal_error: rpc_error.to_string(),
328328
})
329329
})
@@ -337,7 +337,7 @@ impl SecretKeyStoreCspVault for RemoteCspVault {
337337
.sks_contains(context_with_timeout(self.rpc_timeout), *key_id),
338338
)
339339
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
340-
Err(CspSecretKeyStoreContainsError::InternalError {
340+
Err(CspSecretKeyStoreContainsError::TransientInternalError {
341341
internal_error: rpc_error.to_string(),
342342
})
343343
})
@@ -469,7 +469,7 @@ impl NiDkgCspVault for RemoteCspVault {
469469
maybe_resharing_secret,
470470
))
471471
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
472-
Err(CspDkgCreateReshareDealingError::InternalError(
472+
Err(CspDkgCreateReshareDealingError::TransientInternalError(
473473
InternalError {
474474
internal_error: rpc_error.to_string(),
475475
},
@@ -553,7 +553,7 @@ impl TlsHandshakeCspVault for RemoteCspVault {
553553
*key_id,
554554
))
555555
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
556-
Err(CspTlsSignError::InternalError {
556+
Err(CspTlsSignError::TransientInternalError {
557557
internal_error: rpc_error.to_string(),
558558
})
559559
})
@@ -581,7 +581,7 @@ impl IDkgProtocolCspVault for RemoteCspVault {
581581
transcript_operation.clone(),
582582
))
583583
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
584-
Err(IDkgCreateDealingError::InternalError {
584+
Err(IDkgCreateDealingError::TransientInternalError {
585585
internal_error: rpc_error.to_string(),
586586
})
587587
})
@@ -606,9 +606,9 @@ impl IDkgProtocolCspVault for RemoteCspVault {
606606
context_data.to_vec(),
607607
))
608608
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
609-
Err(IDkgVerifyDealingPrivateError::CspVaultRpcError(
610-
rpc_error.to_string(),
611-
))
609+
Err(IDkgVerifyDealingPrivateError::TransientInternalError {
610+
internal_error: rpc_error.to_string(),
611+
})
612612
})
613613
}
614614

@@ -629,7 +629,7 @@ impl IDkgProtocolCspVault for RemoteCspVault {
629629
transcript.clone(),
630630
))
631631
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
632-
Err(IDkgLoadTranscriptError::InternalError {
632+
Err(IDkgLoadTranscriptError::TransientInternalError {
633633
internal_error: rpc_error.to_string(),
634634
})
635635
})
@@ -654,7 +654,7 @@ impl IDkgProtocolCspVault for RemoteCspVault {
654654
transcript.clone(),
655655
))
656656
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
657-
Err(IDkgLoadTranscriptError::InternalError {
657+
Err(IDkgLoadTranscriptError::TransientInternalError {
658658
internal_error: rpc_error.to_string(),
659659
})
660660
})
@@ -671,7 +671,7 @@ impl IDkgProtocolCspVault for RemoteCspVault {
671671
oldest_public_key,
672672
))
673673
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
674-
Err(IDkgRetainKeysError::InternalError {
674+
Err(IDkgRetainKeysError::TransientInternalError {
675675
internal_error: rpc_error.to_string(),
676676
})
677677
})
@@ -706,7 +706,7 @@ impl IDkgProtocolCspVault for RemoteCspVault {
706706
*opener_key_id,
707707
))
708708
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
709-
Err(IDkgOpenTranscriptError::InternalError {
709+
Err(IDkgOpenTranscriptError::TransientInternalError {
710710
internal_error: rpc_error.to_string(),
711711
})
712712
})
@@ -739,7 +739,7 @@ impl ThresholdEcdsaSignerCspVault for RemoteCspVault {
739739
algorithm_id,
740740
))
741741
.unwrap_or_else(|rpc_error: tarpc::client::RpcError| {
742-
Err(ThresholdEcdsaSignShareError::InternalError {
742+
Err(ThresholdEcdsaSignShareError::TransientInternalError {
743743
internal_error: rpc_error.to_string(),
744744
})
745745
})

‎rs/crypto/internal/crypto_service_provider/tests/remote_csp_vault.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ mod rpc_connection {
2424
use ic_config::logger::Config as LoggerConfig;
2525
use ic_crypto_internal_csp::api::CspCreateMEGaKeyError;
2626
use ic_crypto_internal_csp::types::CspSignature;
27-
use ic_crypto_internal_csp::vault::api::CspBasicSignatureError::InternalError;
27+
use ic_crypto_internal_csp::vault::api::CspBasicSignatureError::TransientInternalError;
2828
use ic_crypto_internal_csp::vault::api::{
2929
BasicSignatureCspVault, CspBasicSignatureError, CspPublicKeyStoreError,
3030
IDkgProtocolCspVault, PublicKeyStoreCspVault,
@@ -60,7 +60,7 @@ mod rpc_connection {
6060
assert_matches!(signature, Ok(_));
6161

6262
let signature = sign_message(TooLarge, key_id, &client_cannot_send_large_request);
63-
assert_matches!(signature, Err(InternalError {internal_error}) if internal_error.contains("the client failed to send the request"));
63+
assert_matches!(signature, Err(TransientInternalError {internal_error}) if internal_error.contains("the client failed to send the request"));
6464

6565
let signature = sign_message(Small, key_id, &client_cannot_send_large_request);
6666
assert_matches!(signature, Ok(_));
@@ -87,7 +87,7 @@ mod rpc_connection {
8787
assert_matches!(signature_before_error, Ok(_));
8888

8989
let signature = sign_message(TooLarge, key_id, &client);
90-
assert_matches!(signature, Err(InternalError {internal_error}) if internal_error.contains("the request exceeded its deadline"));
90+
assert_matches!(signature, Err(TransientInternalError {internal_error}) if internal_error.contains("the request exceeded its deadline"));
9191

9292
let signature_after_error = sign_message(Small, key_id, &client);
9393
assert_eq!(signature_before_error, signature_after_error);

0 commit comments

Comments
 (0)