fix: address Phase 2 code review feedback · str4d.xyz/atproto-devtool@70ef662

+1

Cargo.lock

··· 123 123 "miette", 124 124 "multibase", 125 125 "p256", 126 + "percent-encoding", 126 127 "reqwest", 127 128 "serde", 128 129 "serde_json",

+1

Cargo.toml

··· 15 15 miette = { version = "7.6", features = ["fancy"] } 16 16 multibase = "0.9" 17 17 p256 = { version = "0.13", features = ["ecdsa"] } 18 + percent-encoding = "2.3" 18 19 reqwest = { version = "0.13", default-features = false, features = ["rustls", "json", "gzip"] } 19 20 serde = { version = "1.0", features = ["derive"] } 20 21 serde_json = "1.0"

+234 -105

src/common/identity.rs

··· 4 4 //! allowing callers to swap real network I/O with recorded fixtures in tests. 5 5 6 6 use async_trait::async_trait; 7 + use k256::ecdsa::signature::hazmat::PrehashVerifier; 8 + use percent_encoding; 7 9 use serde::{Deserialize, Serialize}; 10 + use std::fmt; 8 11 use std::sync::Arc; 9 12 use thiserror::Error; 10 13 use url::Url; ··· 40 43 } 41 44 } 42 45 43 - impl std::fmt::Display for Did { 44 - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { 46 + impl fmt::Display for Did { 47 + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { 45 48 write!(f, "{}", self.0) 46 49 } 47 50 } ··· 126 129 P256(p256::ecdsa::VerifyingKey), 127 130 } 128 131 132 + impl AnyVerifyingKey { 133 + /// Verifies a prehashed signature against this key. 134 + /// 135 + /// The prehash must be a 32-byte SHA-256 digest. 136 + pub fn verify_prehash( 137 + &self, 138 + prehash: &[u8; 32], 139 + sig: &AnySignature, 140 + ) -> Result<(), AnySignatureError> { 141 + match (self, sig) { 142 + (AnyVerifyingKey::K256(key), AnySignature::K256(sig)) => key 143 + .verify_prehash(prehash, sig) 144 + .map_err(AnySignatureError::K256), 145 + (AnyVerifyingKey::P256(key), AnySignature::P256(sig)) => key 146 + .verify_prehash(prehash, sig) 147 + .map_err(AnySignatureError::P256), 148 + _ => Err(AnySignatureError::CurveMismatch), 149 + } 150 + } 151 + } 152 + 153 + /// A signature that may be one of several supported curves. 154 + #[derive(Debug, Clone)] 155 + pub enum AnySignature { 156 + /// secp256k1 signature. 157 + K256(k256::ecdsa::Signature), 158 + /// P-256 signature. 159 + P256(p256::ecdsa::Signature), 160 + } 161 + 162 + /// Error from signature verification across multiple curves. 163 + #[derive(Debug)] 164 + pub enum AnySignatureError { 165 + /// secp256k1 signature error. 166 + K256(k256::ecdsa::Error), 167 + /// P-256 signature error. 168 + P256(p256::ecdsa::Error), 169 + /// Signature and key use mismatched curves. 170 + CurveMismatch, 171 + } 172 + 129 173 /// A multikey public key, parsed and ready for signature verification. 130 174 #[derive(Debug, Clone)] 131 175 pub struct ParsedMultikey { ··· 158 202 #[source] 159 203 source: Box<IdentityError>, 160 204 }, 205 + 206 + /// DNS backend resolver error. 207 + #[error("DNS backend error")] 208 + DnsBackend(#[from] hickory_resolver::ResolveError), 161 209 162 210 /// HTTPS fallback for handle resolution failed. 163 211 #[error("HTTP fallback for handle resolution failed")] ··· 212 260 #[error("Invalid multikey length")] 213 261 MultikeyLengthInvalid, 214 262 215 - /// HTTP transport error. 263 + /// HTTP transport error from reqwest. 216 264 #[error("HTTP transport error")] 217 - HttpTransport { 218 - /// The underlying error. 219 - #[source] 220 - source: Box<dyn std::error::Error + Send + Sync>, 221 - }, 265 + HttpTransport(#[from] reqwest::Error), 222 266 } 223 267 224 268 /// Trait for HTTP clients used by identity resolution. ··· 246 290 247 291 impl RealHttpClient { 248 292 /// Creates a new HTTP client with a conservative 10-second timeout. 293 + /// 294 + /// Uses rustls for TLS (per Cargo.toml `rustls-tls` feature) rather than native-tls 295 + /// to avoid linking against OpenSSL. Sets a User-Agent header for HTTPS requests. 249 296 pub fn new() -> Result<Self, IdentityError> { 250 297 let client = reqwest::Client::builder() 298 + .use_rustls_tls() 299 + .user_agent("atproto-devtool/0.0.0") 251 300 .timeout(std::time::Duration::from_secs(10)) 252 - .build() 253 - .map_err(|e| IdentityError::HttpTransport { 254 - source: Box::new(e), 255 - })?; 301 + .build()?; 256 302 Ok(Self { inner: client }) 257 303 } 258 304 } ··· 260 306 #[async_trait] 261 307 impl HttpClient for RealHttpClient { 262 308 async fn get_bytes(&self, url: &Url) -> Result<(u16, Vec<u8>), IdentityError> { 263 - let response = 264 - self.inner 265 - .get(url.clone()) 266 - .send() 267 - .await 268 - .map_err(|e| IdentityError::HttpTransport { 269 - source: Box::new(e), 270 - })?; 309 + let response = self.inner.get(url.clone()).send().await?; 271 310 let status = response.status().as_u16(); 272 - let bytes = response 273 - .bytes() 274 - .await 275 - .map_err(|e| IdentityError::HttpTransport { 276 - source: Box::new(e), 277 - })?; 311 + let bytes = response.bytes().await?; 278 312 Ok((status, bytes.to_vec())) 279 313 } 280 314 } ··· 303 337 #[async_trait] 304 338 impl DnsResolver for RealDnsResolver { 305 339 async fn txt_lookup(&self, name: &str) -> Result<Vec<String>, IdentityError> { 306 - self.inner 307 - .txt_lookup(name) 308 - .await 309 - .map_err(|e| IdentityError::DnsLookupFailed { 310 - source: Box::new(IdentityError::HttpTransport { 311 - source: Box::new(e), 312 - }), 313 - })? 340 + let lookup = self.inner.txt_lookup(name).await?; 341 + lookup 314 342 .iter() 315 343 .map(|record| { 316 - Ok(record 344 + let text = record 317 345 .iter() 318 - .map(|data| String::from_utf8(data.to_vec()).unwrap_or_default()) 346 + .map(|data| { 347 + String::from_utf8(data.to_vec()).unwrap_or_else(|_| { 348 + tracing::debug!( 349 + target = "atproto_devtool::identity", 350 + "dropping non-UTF-8 TXT record data" 351 + ); 352 + String::new() 353 + }) 354 + }) 319 355 .collect::<Vec<_>>() 320 - .join("")) 356 + .join(""); 357 + Ok(text) 321 358 }) 322 359 .collect() 323 360 } ··· 350 387 351 388 // DNS path: look up _atproto.<handle> for did= records. 352 389 let dns_name = format!("_atproto.{}", handle); 353 - let mut dns_error_opt: Option<Box<IdentityError>> = None; 354 390 355 - if let Err(e) = dns.txt_lookup(&dns_name).await { 356 - dns_error_opt = Some(Box::new(e)); 357 - } else if let Ok(records) = dns.txt_lookup(&dns_name).await { 358 - for record in records { 359 - let trimmed = record.trim(); 360 - if let Some(did_str) = trimmed.strip_prefix("did=") { 361 - let did = Did(did_str.to_string()); 362 - tracing::debug!( 363 - target = "atproto_devtool::identity", 364 - did = %did, 365 - "resolved handle via DNS" 366 - ); 367 - return Ok(did); 391 + let dns_error_opt = match dns.txt_lookup(&dns_name).await { 392 + Ok(records) => { 393 + // Check if any record contains a did= entry. 394 + for record in records { 395 + let trimmed = record.trim(); 396 + if let Some(did_str) = trimmed.strip_prefix("did=") { 397 + let did = Did(did_str.to_string()); 398 + tracing::debug!( 399 + target = "atproto_devtool::identity", 400 + did = %did, 401 + "resolved handle via DNS" 402 + ); 403 + return Ok(did); 404 + } 368 405 } 406 + // 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(), 410 + })) 369 411 } 370 - } 412 + Err(e) => Some(Box::new(e)), 413 + }; 371 414 372 415 // HTTPS fallback: GET https://<handle>/.well-known/atproto-did. 373 416 let url = format!("https://{}/.well-known/atproto-did", handle); ··· 375 418 .parse::<Url>() 376 419 .map_err(|_| IdentityError::InvalidHandle)?; 377 420 378 - let mut http_error_opt: Option<Box<IdentityError>> = None; 379 - 380 - match http.get_bytes(&url).await { 421 + let http_error_opt = match http.get_bytes(&url).await { 381 422 Ok((200, bytes)) => { 382 423 let did_str = String::from_utf8_lossy(&bytes).trim().to_string(); 383 - if !did_str.is_empty() { 424 + if !did_str.is_empty() && did_str.starts_with("did:") { 384 425 let did = Did(did_str); 385 426 tracing::debug!( 386 427 target = "atproto_devtool::identity", ··· 388 429 "resolved handle via HTTPS" 389 430 ); 390 431 return Ok(did); 432 + } else { 433 + 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 + }), 438 + })) 391 439 } 392 440 } 393 - Err(e) => { 394 - http_error_opt = Some(Box::new(e)); 395 - } 396 - _ => {} 397 - } 441 + Ok((status, bytes)) => Some(Box::new(IdentityError::HandleHttpFallbackFailed { 442 + source: Box::new(IdentityError::DidResolutionFailed { 443 + status, 444 + body: String::from_utf8_lossy(&bytes).to_string(), 445 + }), 446 + })), 447 + Err(e) => Some(Box::new(IdentityError::HandleHttpFallbackFailed { 448 + source: Box::new(e), 449 + })), 450 + }; 398 451 399 452 // Both paths failed. 400 453 Err(IdentityError::HandleUnresolvable { ··· 418 471 419 472 let (url, source_name) = match did.method() { 420 473 DidMethod::Plc => { 474 + // did:plc identifiers are base32-like and contain only URL-safe characters. 475 + // No additional percent-encoding is needed. 421 476 let url_str = format!("https://plc.directory/{}", did.0); 422 477 let url = url_str 423 478 .parse::<Url>() ··· 509 564 let services = doc.service.as_ref()?; 510 565 511 566 for service in services { 512 - // Match both "#fragment" and "did:...#fragment" forms. 513 - let matches_id = service.id.ends_with(&format!("#{}", id_fragment)) 514 - || service 515 - .id 516 - .ends_with(&format!("#{}:{}", id_fragment, id_fragment)); 517 - 518 - if matches_id && service.type_ == expected_type { 567 + // Extract the fragment after '#', matching both forms: 568 + // - "#fragment" (short form) 569 + // - "did:...#fragment" (full form) 570 + let frag = service.id.rsplit_once('#').map(|(_, f)| f); 571 + if frag == Some(id_fragment) && service.type_ == expected_type { 519 572 return Some(service); 520 573 } 521 574 } ··· 598 651 599 652 /// Helper to percent-decode a string. 600 653 fn percent_decode_str(s: &str) -> Result<String, IdentityError> { 601 - let mut result = String::new(); 602 - let mut chars = s.chars(); 603 - 604 - while let Some(ch) = chars.next() { 605 - if ch == '%' { 606 - let hex: String = chars.by_ref().take(2).collect(); 607 - if let Ok(byte) = u8::from_str_radix(&hex, 16) { 608 - result.push(byte as char); 609 - } else { 610 - return Err(IdentityError::DidResolutionFailed { 611 - status: 400, 612 - body: "Invalid percent encoding".to_string(), 613 - }); 614 - } 615 - } else { 616 - result.push(ch); 617 - } 618 - } 619 - 620 - Ok(result) 654 + let decoded = percent_encoding::percent_decode_str(s) 655 + .decode_utf8() 656 + .map_err(|_| IdentityError::DidResolutionFailed { 657 + status: 400, 658 + body: "Invalid UTF-8 in percent-encoded path".to_string(), 659 + })?; 660 + Ok(decoded.to_string()) 621 661 } 622 662 623 663 #[cfg(test)] 624 664 mod tests { 625 665 use super::*; 666 + use k256::ecdsa::signature::hazmat::PrehashSigner; 626 667 use std::collections::HashMap; 627 668 628 669 /// Fake HTTP client for testing, built from a map of URL → (status, bytes). ··· 814 855 type_: "AtprotoPersonalDataServer".to_string(), 815 856 service_endpoint: "https://example.com/pds".to_string(), 816 857 }, 858 + // Service with a name that contains the search fragment as a substring. 859 + Service { 860 + id: "#xatproto_labeler".to_string(), 861 + type_: "OtherType".to_string(), 862 + service_endpoint: "https://example.com/other".to_string(), 863 + }, 817 864 ]), 818 865 }; 819 866 ··· 826 873 assert!(pds.is_some()); 827 874 let pds = pds.unwrap(); 828 875 assert_eq!(pds.id, "#atproto_pds"); 876 + 877 + // Ensure false-positive on substring is not matched. 878 + let wrong = find_service(&doc, "atproto_labeler", "OtherType"); 879 + assert!(wrong.is_none()); 829 880 } 830 881 831 882 #[test] ··· 857 908 let parsed = result.unwrap(); 858 909 assert_eq!(parsed.curve, Curve::Secp256k1); 859 910 860 - // Verify the key can be extracted and is the correct type. 911 + // Verify the key can be extracted and its bytes match the expected value. 861 912 match &parsed.verifying_key { 862 913 AnyVerifyingKey::K256(key) => { 863 914 let sec1_bytes = key.to_sec1_bytes(); 864 915 assert_eq!(sec1_bytes.len(), 33); // Compressed form is 33 bytes. 916 + // Expected bytes derived from the multikey fixture. 917 + // zQ3shVc2UkAfJCdc1TR8E66J85h48P43r93q8jGPkPpjF9Ef9 decodes to: 918 + // [0xe7, 0x01] (k256 prefix) + 33-byte SEC1 point. 919 + let expected_hex = 920 + "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798"; 921 + let actual_hex: String = sec1_bytes.iter().map(|b| format!("{:02x}", b)).collect(); 922 + assert_eq!(actual_hex, expected_hex); 865 923 } 866 924 _ => panic!("Expected K256 verifying key"), 867 925 } ··· 881 939 882 940 // Verify the key can be extracted and is the correct type. 883 941 match &parsed.verifying_key { 884 - AnyVerifyingKey::P256(_key) => { 885 - // Key was successfully parsed. Phase 6 will test signature verification. 942 + 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 + } 886 956 } 887 957 _ => panic!("Expected P256 verifying key"), 888 958 } ··· 893 963 // Build a multikey with an unsupported curve prefix (0x01 0x00). 894 964 let mut unsupported_bytes = vec![0x01, 0x00]; 895 965 unsupported_bytes.extend_from_slice(&[0; 33]); // Fake 33-byte key. 896 - let multikey = format!( 897 - "z{}", 898 - multibase::encode(multibase::Base::Base58Btc, unsupported_bytes) 899 - ); 966 + // multibase::encode already returns the z-prefixed string for Base58Btc. 967 + let multikey = multibase::encode(multibase::Base::Base58Btc, unsupported_bytes); 900 968 901 969 let result = parse_multikey(&multikey); 902 970 assert!(result.is_err()); ··· 928 996 #[test] 929 997 fn parse_multikey_wrong_length() { 930 998 // Build a multikey with a 10-byte body instead of 33. 931 - // Use a simple test vector that won't cause round-trip encoding issues. 932 - // Create a valid base58btc encoding manually: just take a valid p256 multikey 933 - // and modify it to have 10 bytes instead of 33. 934 999 let mut wrong_len_bytes = vec![0x80, 0x24]; // p256 prefix 935 1000 wrong_len_bytes.extend_from_slice(&[0; 10]); // Only 10 bytes instead of 33 936 1001 937 - // Encode to base58btc properly 938 - let encoded = multibase::encode(multibase::Base::Base58Btc, &wrong_len_bytes); 939 - let multikey = format!("z{}", encoded); 1002 + // multibase::encode already returns the z-prefixed string for Base58Btc. 1003 + let multikey = multibase::encode(multibase::Base::Base58Btc, &wrong_len_bytes); 940 1004 941 1005 let result = parse_multikey(&multikey); 942 1006 assert!(result.is_err()); 943 1007 944 - // Should get either MultikeyLengthInvalid (if we parse varint correctly) 945 - // or UnsupportedCurve (if the length mismatch causes other issues) 1008 + // Must strictly assert MultikeyLengthInvalid. 946 1009 match result.unwrap_err() { 947 - IdentityError::MultikeyLengthInvalid => {} 948 - _ => {} // Acceptable; we're testing that invalid lengths are rejected 1010 + IdentityError::MultikeyLengthInvalid => { 1011 + // Correct error variant. 1012 + } 1013 + e => panic!("Expected MultikeyLengthInvalid, got {:?}", e), 949 1014 } 1015 + } 1016 + 1017 + #[test] 1018 + fn verify_prehash_k256_valid() { 1019 + use k256::ecdsa::{Signature, SigningKey}; 1020 + 1021 + // Create an ephemeral k256 keypair for testing. 1022 + let signing_key = SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); 1023 + let verifying_key = signing_key.verifying_key(); 1024 + 1025 + // Create a test prehash (32 bytes). 1026 + let prehash = *b"01234567890123456789012345678901"; 1027 + 1028 + // Sign the prehash. 1029 + let signature = signing_key.sign_prehash(&prehash).expect("signing failed"); 1030 + 1031 + // Wrap in our generic types. 1032 + let any_key = AnyVerifyingKey::K256(*verifying_key); 1033 + let any_sig = AnySignature::K256(signature); 1034 + 1035 + // Verify should succeed. 1036 + assert!(any_key.verify_prehash(&prehash, &any_sig).is_ok()); 1037 + } 1038 + 1039 + #[test] 1040 + fn verify_prehash_p256_valid() { 1041 + use p256::ecdsa::{Signature, SigningKey}; 1042 + 1043 + // Create an ephemeral p256 keypair for testing. 1044 + let signing_key = SigningKey::random(&mut p256::elliptic_curve::rand_core::OsRng); 1045 + let verifying_key = signing_key.verifying_key(); 1046 + 1047 + // Create a test prehash (32 bytes). 1048 + let prehash = *b"01234567890123456789012345678901"; 1049 + 1050 + // Sign the prehash. 1051 + let signature = signing_key.sign_prehash(&prehash).expect("signing failed"); 1052 + 1053 + // Wrap in our generic types. 1054 + let any_key = AnyVerifyingKey::P256(*verifying_key); 1055 + let any_sig = AnySignature::P256(signature); 1056 + 1057 + // Verify should succeed. 1058 + assert!(any_key.verify_prehash(&prehash, &any_sig).is_ok()); 1059 + } 1060 + 1061 + #[test] 1062 + 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. 1067 + let k256_signing_key = K256SigningKey::random(&mut k256::elliptic_curve::rand_core::OsRng); 1068 + let k256_key = AnyVerifyingKey::K256(*k256_signing_key.verifying_key()); 1069 + 1070 + let p256_signing_key = P256SigningKey::random(&mut p256::elliptic_curve::rand_core::OsRng); 1071 + let prehash = *b"01234567890123456789012345678901"; 1072 + let p256_sig = p256_signing_key 1073 + .sign_prehash(&prehash) 1074 + .expect("signing failed"); 1075 + let p256_any_sig = AnySignature::P256(p256_sig); 1076 + 1077 + // Verify should fail due to curve mismatch. 1078 + assert!(k256_key.verify_prehash(&prehash, &p256_any_sig).is_err()); 950 1079 } 951 1080 }

Configure Feed

Configure Feed