fix(crypto): address PR review feedback for verify_plc_operation

+80 -7

1 changed file

expand all

crates

crypto

src

plc.rs

+80 -7

crates/crypto/src/plc.rs

··· 161 161 /// attribute ensures that direct construction is not possible outside this 162 162 /// module. 163 163 #[non_exhaustive] 164 + #[derive(Debug)] 164 165 pub struct VerifiedGenesisOp { 165 166 /// The derived DID, e.g. `"did:plc:abcdefghijklmnopqrstuvwx"`. 166 167 pub did: String, ··· 418 419 /// signed op; the caller uses them for semantic validation and DID document 419 420 /// construction. 420 421 #[non_exhaustive] 422 + #[derive(Debug)] 421 423 pub struct VerifiedPlcOp { 422 424 /// The derived DID. `Some` for genesis ops (derived from signed CBOR), 423 425 /// `None` for rotation ops (caller provides the DID from context). ··· 442 444 /// canonical field ordering, and verifies the ECDSA-SHA256 signature against 443 445 /// each key in `authorized_rotation_keys` until one succeeds. 444 446 /// 447 + /// # Caller obligation 448 + /// The caller is responsible for providing the correct set of authorized 449 + /// rotation keys. For genesis ops, these come from the op itself; for rotation 450 + /// ops, they come from the **previous** operation's `rotationKeys` array. 451 + /// This function only checks that the signature was made by one of the 452 + /// provided keys — it does not verify that those keys are the right ones 453 + /// for this DID's current state. 454 + /// 445 455 /// # Parameters 446 456 /// - `signed_op_json`: JSON-encoded signed PLC operation. 447 457 /// - `authorized_rotation_keys`: The set of `did:key:` URIs authorized to sign 448 - /// this operation. For genesis ops, these come from the op itself; for rotation 449 - /// ops, they come from the previous operation's state. 458 + /// this operation. 450 459 /// 451 460 /// # Errors 452 461 /// Returns `CryptoError::PlcOperation` if no authorized key verifies the ··· 461 470 )); 462 471 } 463 472 473 + // Fail fast on encoding setup before doing any crypto work. 474 + let b32 = base32_lowercase()?; 475 + 464 476 // Parse the signed op, rejecting unknown fields. 465 477 let signed_op: SignedPlcOp = serde_json::from_str(signed_op_json) 466 478 .map_err(|e| CryptoError::PlcOperation(format!("invalid signed op JSON: {e}")))?; ··· 494 506 .map_err(|e| CryptoError::PlcOperation(format!("cbor encode unsigned op: {e}")))?; 495 507 496 508 // Try each authorized rotation key until one verifies the signature. 497 - let mut last_error = String::new(); 498 - for key in authorized_rotation_keys { 509 + // Accumulate all per-key errors so the caller can diagnose multi-key failures. 510 + let mut key_errors: Vec<String> = Vec::new(); 511 + for (i, key) in authorized_rotation_keys.iter().enumerate() { 499 512 match verify_signature_with_key(key, &unsigned_cbor, &signature) { 500 513 Ok(()) => { 501 514 // Signature verified — compute DID and CID. ··· 509 522 // DID is only derivable from genesis ops (prev == None). 510 523 let did = if signed_op.prev.is_none() { 511 524 let hash = Sha256::digest(&signed_cbor); 512 - let encoded = base32_lowercase()?.encode(hash.as_ref()); 525 + let encoded = b32.encode(hash.as_ref()); 513 526 Some(format!("did:plc:{}", &encoded[..24])) 514 527 } else { 515 528 None ··· 526 539 }); 527 540 } 528 541 Err(e) => { 529 - last_error = e; 542 + key_errors.push(format!("key[{i}]: {e}")); 530 543 } 531 544 } 532 545 } 533 546 534 547 Err(CryptoError::PlcOperation(format!( 535 - "no authorized rotation key verified the signature: {last_error}" 548 + "no authorized rotation key verified the signature: {}", 549 + key_errors.join("; ") 536 550 ))) 537 551 } 538 552 ··· 1289 1303 assert!( 1290 1304 matches!(result, Err(CryptoError::PlcOperation(ref msg)) if msg.contains("must not be empty")), 1291 1305 "empty key list must fail" 1306 + ); 1307 + } 1308 + 1309 + /// Tampered rotationKeys in signed JSON must be rejected (signature covers the unsigned op). 1310 + #[test] 1311 + fn verify_plc_operation_rejects_tampered_rotation_keys() { 1312 + let (signing_key, op) = make_op_for_verify(); 1313 + 1314 + // Parse JSON, swap rotationKeys[0] for a different key, re-serialize 1315 + let mut v: serde_json::Value = 1316 + serde_json::from_str(&op.signed_op_json).expect("valid JSON"); 1317 + let wrong_kp = generate_p256_keypair().expect("wrong keypair"); 1318 + v["rotationKeys"][0] = serde_json::json!(wrong_kp.key_id.0); 1319 + let tampered_json = serde_json::to_string(&v).expect("re-serialize"); 1320 + 1321 + let result = verify_plc_operation(&tampered_json, &[signing_key]); 1322 + assert!( 1323 + matches!(result, Err(CryptoError::PlcOperation(_))), 1324 + "tampered rotationKeys must fail verification" 1325 + ); 1326 + } 1327 + 1328 + /// Non-plc_operation type is rejected by verify_plc_operation. 1329 + #[test] 1330 + fn verify_plc_operation_rejects_non_plc_operation_type() { 1331 + let (signing_key, op) = make_op_for_verify(); 1332 + 1333 + let mut v: serde_json::Value = 1334 + serde_json::from_str(&op.signed_op_json).expect("valid JSON"); 1335 + v["type"] = serde_json::json!("plc_tombstone"); 1336 + let modified_json = serde_json::to_string(&v).expect("re-serialize"); 1337 + 1338 + let result = verify_plc_operation(&modified_json, &[signing_key]); 1339 + assert!( 1340 + matches!(result, Err(CryptoError::PlcOperation(ref msg)) if msg.contains("plc_tombstone")), 1341 + "non-plc_operation type must be rejected" 1342 + ); 1343 + } 1344 + 1345 + /// Wrong-length signature from rotation op callback hits the length guard. 1346 + #[test] 1347 + fn rotation_op_wrong_length_signature_returns_error() { 1348 + let (signing_key, _, _, prev_cid) = make_genesis_for_rotation(); 1349 + 1350 + let mut verification_methods = BTreeMap::new(); 1351 + verification_methods.insert("atproto".to_string(), signing_key.0.clone()); 1352 + 1353 + let result = build_did_plc_rotation_op( 1354 + &prev_cid, 1355 + vec![signing_key.0.clone()], 1356 + verification_methods, 1357 + vec![], 1358 + BTreeMap::new(), 1359 + |_| Ok(vec![0u8; 32]), // 32 bytes instead of expected 64 1360 + ); 1361 + 1362 + assert!( 1363 + matches!(result, Err(CryptoError::PlcOperation(ref msg)) if msg.contains("expected 64")), 1364 + "wrong-length signature must be rejected, got: {result:?}" 1292 1365 ); 1293 1366 } 1294 1367

Configure Feed

Configure Feed