CLI app for developers prototyping atproto functionality
fix: address Phase 2 review cycle 2 issues
Resolves all critical, important, and minor issues identified in the second
review cycle for identity.rs:
Critical (N1): Remove unused `Signature` imports in verify_prehash tests and
relocate per-function `use` statements to module level.
Important (N2): Replace no-op `use percent_encoding;` with direct use of the
percent_decode_str helper function (which already exists and wraps the
percent_encoding crate).
Important (N3): Hoist SigningKey type imports to test module level as
K256SigningKey and P256SigningKey aliases to comply with module organization
rules.
Important (N4): Improve parse_multikey_p256 test by forcing compressed form
unconditionally with to_encoded_point(true) and asserting the hex literal
without soft-gating.
Minor (N5): Add dedicated DnsNoDidRecord error variant for DNS records that
exist but lack did= entries, replacing generic DidResolutionFailed.
Minor (N6): Add dedicated InvalidDidBody error variant for invalid DID format
in HTTPS fallback response, replacing generic DidResolutionFailed with
contradictory status: 200.
Minor (N7): Add #[derive(thiserror::Error)] and #[error(...)] attributes to
AnySignatureError enum so it can chain into miette diagnostics.
Minor (N8): Add symmetric p256-key+k256-sig test case to verify_prehash_curve_mismatch
for complete coverage of curve mismatch scenarios.
All tests pass; cargo fmt and cargo clippy --all-targets pass cleanly.
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
+49
-32
+49
-32
src/common/identity.rs
+49
-32
src/common/identity.rs
···
5
5
6
6
use async_trait::async_trait;
7
7
use k256::ecdsa::signature::hazmat::PrehashVerifier;
8
-
use percent_encoding;
9
8
use serde::{Deserialize, Serialize};
10
9
use std::fmt;
11
10
use std::sync::Arc;
···
160
159
}
161
160
162
161
/// Error from signature verification across multiple curves.
163
-
#[derive(Debug)]
162
+
#[derive(Debug, thiserror::Error)]
164
163
pub enum AnySignatureError {
165
164
/// secp256k1 signature error.
165
+
#[error("secp256k1 signature verification failed")]
166
166
K256(k256::ecdsa::Error),
167
167
/// P-256 signature error.
168
+
#[error("P-256 signature verification failed")]
168
169
P256(p256::ecdsa::Error),
169
170
/// Signature and key use mismatched curves.
171
+
#[error("Signature and key use mismatched curves")]
170
172
CurveMismatch,
171
173
}
172
174
···
228
230
body: String,
229
231
},
230
232
233
+
/// DNS record exists but contains no DID entry.
234
+
#[error("DNS record for {handle} has no did= entry")]
235
+
DnsNoDidRecord {
236
+
/// The handle that was queried.
237
+
handle: String,
238
+
},
239
+
240
+
/// DID body is invalid or malformed.
241
+
#[error("Invalid DID body: {body}")]
242
+
InvalidDidBody {
243
+
/// The invalid body content.
244
+
body: String,
245
+
},
246
+
231
247
/// DID document could not be decoded.
232
248
#[error("DID document decode failed")]
233
249
DidDocumentDecodeFailed {
···
404
420
}
405
421
}
406
422
// Records exist but no did= entry found; populate error for fallback.
407
-
Some(Box::new(IdentityError::DidResolutionFailed {
408
-
status: 400,
409
-
body: "No did= entry in DNS records".to_string(),
423
+
Some(Box::new(IdentityError::DnsNoDidRecord {
424
+
handle: handle.to_string(),
410
425
}))
411
426
}
412
427
Err(e) => Some(Box::new(e)),
···
431
446
return Ok(did);
432
447
} else {
433
448
Some(Box::new(IdentityError::HandleHttpFallbackFailed {
434
-
source: Box::new(IdentityError::DidResolutionFailed {
435
-
status: 200,
436
-
body: "Invalid DID format in response body".to_string(),
437
-
}),
449
+
source: Box::new(IdentityError::InvalidDidBody { body: did_str }),
438
450
}))
439
451
}
440
452
}
···
663
675
#[cfg(test)]
664
676
mod tests {
665
677
use super::*;
678
+
use k256::ecdsa::SigningKey as K256SigningKey;
666
679
use k256::ecdsa::signature::hazmat::PrehashSigner;
680
+
use p256::ecdsa::SigningKey as P256SigningKey;
667
681
use std::collections::HashMap;
668
682
669
683
/// Fake HTTP client for testing, built from a map of URL → (status, bytes).
···
940
954
// Verify the key can be extracted and is the correct type.
941
955
match &parsed.verifying_key {
942
956
AnyVerifyingKey::P256(key) => {
943
-
let sec1_bytes = key.to_sec1_bytes();
944
-
// The fixture contains a compressed SEC1 key (33 bytes starting with 02 or 03).
945
-
// The verifying key implementation may return either compressed or uncompressed form.
946
-
// Verify it's a valid encoding (either 33 bytes compressed, or 65 bytes uncompressed).
947
-
assert!(sec1_bytes.len() == 33 || sec1_bytes.len() == 65);
948
-
// If compressed, verify the expected form.
949
-
if sec1_bytes.len() == 33 {
950
-
let hex: String = sec1_bytes.iter().map(|b| format!("{:02x}", b)).collect();
951
-
assert_eq!(
952
-
hex,
953
-
"026b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c296"
954
-
);
955
-
}
957
+
// Force compressed form for consistent testing.
958
+
let sec1_bytes = key.to_encoded_point(true).as_bytes().to_vec();
959
+
assert_eq!(sec1_bytes.len(), 33);
960
+
// Expected bytes derived from the multikey fixture.
961
+
let expected_hex =
962
+
"026b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c296";
963
+
let actual_hex: String = sec1_bytes.iter().map(|b| format!("{:02x}", b)).collect();
964
+
assert_eq!(actual_hex, expected_hex);
956
965
}
957
966
_ => panic!("Expected P256 verifying key"),
958
967
}
···
1016
1025
1017
1026
#[test]
1018
1027
fn verify_prehash_k256_valid() {
1019
-
use k256::ecdsa::{Signature, SigningKey};
1020
-
1021
1028
// Create an ephemeral k256 keypair for testing.
1022
-
let signing_key = SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng);
1029
+
let signing_key = K256SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng);
1023
1030
let verifying_key = signing_key.verifying_key();
1024
1031
1025
1032
// Create a test prehash (32 bytes).
···
1038
1045
1039
1046
#[test]
1040
1047
fn verify_prehash_p256_valid() {
1041
-
use p256::ecdsa::{Signature, SigningKey};
1042
-
1043
1048
// Create an ephemeral p256 keypair for testing.
1044
-
let signing_key = SigningKey::random(&mut p256::elliptic_curve::rand_core::OsRng);
1049
+
let signing_key = P256SigningKey::random(&mut p256::elliptic_curve::rand_core::OsRng);
1045
1050
let verifying_key = signing_key.verifying_key();
1046
1051
1047
1052
// Create a test prehash (32 bytes).
···
1060
1065
1061
1066
#[test]
1062
1067
fn verify_prehash_curve_mismatch() {
1063
-
use k256::ecdsa::SigningKey as K256SigningKey;
1064
-
use p256::ecdsa::SigningKey as P256SigningKey;
1065
-
1066
-
// Create an ephemeral k256 keypair and a p256 signature.
1068
+
// Test k256 key with p256 signature (should fail).
1067
1069
let k256_signing_key = K256SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng);
1068
1070
let k256_key = AnyVerifyingKey::K256(*k256_signing_key.verifying_key());
1069
1071
···
1076
1078
1077
1079
// Verify should fail due to curve mismatch.
1078
1080
assert!(k256_key.verify_prehash(&prehash, &p256_any_sig).is_err());
1081
+
1082
+
// Test p256 key with k256 signature (symmetric case).
1083
+
let p256_signing_key_2 =
1084
+
P256SigningKey::random(&mut p256::elliptic_curve::rand_core::OsRng);
1085
+
let p256_key = AnyVerifyingKey::P256(*p256_signing_key_2.verifying_key());
1086
+
1087
+
let k256_signing_key_2 =
1088
+
K256SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng);
1089
+
let k256_sig = k256_signing_key_2
1090
+
.sign_prehash(&prehash)
1091
+
.expect("signing failed");
1092
+
let k256_any_sig = AnySignature::K256(k256_sig);
1093
+
1094
+
// Verify should fail due to curve mismatch.
1095
+
assert!(p256_key.verify_prehash(&prehash, &k256_any_sig).is_err());
1079
1096
}
1080
1097
}