An easy-to-host PDS on the ATProtocol, MacOS. Grandma-approved.
fix(crypto): address MM-90 code review issues in plc.rs and keys.rs
Critical:
- [C2] Validate prev=null and op_type="plc_operation" in verify_genesis_op
immediately after parsing, rejecting rotation ops and non-genesis types.
Prevents clients from submitting rotation ops that bypass plc.directory
validation, which would leave accounts stuck.
Important:
- [I5] Document rotation_key caller obligation in verify_genesis_op docstring:
"The caller is responsible for verifying that the provided key appears in
the op's rotationKeys array; this function only checks that the signature
was made by that key."
Type Design:
- [T1] Apply #[non_exhaustive] to VerifiedGenesisOp to prevent struct-literal
construction outside plc.rs module. Ensures construction only via
verify_genesis_op.
- [T2] Apply #[non_exhaustive] to PlcGenesisOp to prevent struct-literal
construction outside plc.rs module. Ensures construction only via
build_did_plc_genesis_op.
- [T3] Promote P256_MULTICODEC_PREFIX to pub(crate) in keys.rs and import
in plc.rs. Eliminates silent divergence risk between two copies of the
same constant.
- [T4] Improve rotation_key parameter docstring in verify_genesis_op to
clarify its purpose: "The key that must have signed the unsigned operation
— the caller determines which of the op's rotation keys to verify against."
Tests:
- [G1] Add verify_rotation_op_with_non_null_prev_returns_error test
covering prev != null rejection.
- [G1] Add verify_non_genesis_op_type_returns_error test covering non-
"plc_operation" type rejection.
- [G1] Add verify_rotation_key_can_verify_own_op test for canonical usage
pattern where the same keypair both signs and appears at rotationKeys[0].
This was missing coverage — prior tests verified with signing_key only.
authored by
Tangled
911ca8e5
85b7219f
+120
-22
+1
-1
crates/crypto/src/keys.rs
+1
-1
crates/crypto/src/keys.rs
···
12
12
13
13
/// P-256 multicodec varint prefix for did:key URIs.
14
14
/// 0x1200 encoded as LEB128 varint = [0x80, 0x24].
15
-
const P256_MULTICODEC_PREFIX: &[u8] = &[0x80, 0x24];
15
+
pub(crate) const P256_MULTICODEC_PREFIX: &[u8] = &[0x80, 0x24];
16
16
17
17
/// A `did:key:z...` URI — the canonical identifier for a P-256 keypair.
18
18
///
+119
-21
crates/crypto/src/plc.rs
+119
-21
crates/crypto/src/plc.rs
···
32
32
use serde::{Deserialize, Serialize};
33
33
use sha2::{Digest, Sha256};
34
34
35
-
use crate::{CryptoError, DidKeyUri};
35
+
use crate::{keys::P256_MULTICODEC_PREFIX, CryptoError, DidKeyUri};
36
36
37
37
/// The result of building a did:plc genesis operation.
38
+
///
39
+
/// Only valid if constructed via [`build_did_plc_genesis_op`] — the
40
+
/// `#[non_exhaustive]` attribute ensures that direct construction is not
41
+
/// possible outside this module.
42
+
#[non_exhaustive]
38
43
#[derive(Debug)]
39
44
pub struct PlcGenesisOp {
40
45
/// The derived DID, e.g. `"did:plc:abcdefghijklmnopqrstuvwx"`.
···
44
49
/// Ready to POST to plc.directory.
45
50
pub signed_op_json: String,
46
51
}
47
-
48
-
/// P-256 multicodec varint prefix for did:key URIs.
49
-
/// 0x1200 encoded as LEB128 varint = [0x80, 0x24].
50
-
///
51
-
/// This constant is redefined here rather than promoted to `pub(crate)` in
52
-
/// `keys.rs` to avoid cross-module coupling between two sibling functional
53
-
/// modules. Each module owns its own copy; if the value ever needs to change,
54
-
/// both sites are easy to find via the shared literal `[0x80, 0x24]`.
55
-
const P256_MULTICODEC_PREFIX: &[u8] = &[0x80, 0x24];
56
52
57
53
// ── Internal serialization types ────────────────────────────────────────────
58
54
//
···
122
118
/// Returned by [`verify_genesis_op`]. Fields are extracted directly from the
123
119
/// verified signed op; the relay uses them for semantic validation and DID
124
120
/// document construction.
121
+
///
122
+
/// Only valid if constructed via [`verify_genesis_op`] — the `#[non_exhaustive]`
123
+
/// attribute ensures that direct construction is not possible outside this
124
+
/// module.
125
+
#[non_exhaustive]
125
126
pub struct VerifiedGenesisOp {
126
127
/// The derived DID, e.g. `"did:plc:abcdefghijklmnopqrstuvwx"`.
127
128
pub did: String,
···
237
238
/// Verify a client-submitted did:plc signed genesis operation.
238
239
///
239
240
/// Parses `signed_op_json` into a [`SignedPlcOp`] (rejecting unknown fields),
240
-
/// reconstructs the unsigned operation with the same DAG-CBOR field ordering
241
-
/// as [`build_did_plc_genesis_op`], verifies the ECDSA-SHA256 signature against
241
+
/// validates that this is a genesis operation (not a rotation), reconstructs
242
+
/// the unsigned operation with the same DAG-CBOR field ordering as
243
+
/// [`build_did_plc_genesis_op`], verifies the ECDSA-SHA256 signature against
242
244
/// `rotation_key`, derives the DID (SHA-256 of signed CBOR → base32-lowercase
243
245
/// first 24 chars), and returns the extracted operation fields.
244
246
///
247
+
/// # Important: rotation_key caller obligation
248
+
/// The caller is responsible for verifying that the provided `rotation_key`
249
+
/// appears in the op's `rotationKeys` array; this function only checks that
250
+
/// the signature was made by that key.
251
+
///
252
+
/// # Parameters
253
+
/// - `signed_op_json`: JSON-encoded signed genesis operation from a client.
254
+
/// - `rotation_key`: The key that must have signed the unsigned operation — the
255
+
/// caller determines which of the op's rotation keys to verify against.
256
+
/// Must be a valid `did:key:` URI with P-256 multicodec prefix.
257
+
///
245
258
/// # Errors
246
-
/// Returns `CryptoError::PlcOperation` for any parse, format, or cryptographic failure.
259
+
/// Returns `CryptoError::PlcOperation` for any parse, format, cryptographic
260
+
/// failure, or if the operation is not a genesis operation (prev != null or
261
+
/// type != "plc_operation").
247
262
pub fn verify_genesis_op(
248
263
signed_op_json: &str,
249
264
rotation_key: &DidKeyUri,
···
252
267
let signed_op: SignedPlcOp = serde_json::from_str(signed_op_json)
253
268
.map_err(|e| CryptoError::PlcOperation(format!("invalid signed op JSON: {e}")))?;
254
269
255
-
// Step 2: Base64url-decode the signature field.
270
+
// Step 2: Validate this is a genesis operation, not a rotation (C2).
271
+
if signed_op.prev.is_some() {
272
+
return Err(CryptoError::PlcOperation(
273
+
"genesis op must have prev = null".to_string(),
274
+
));
275
+
}
276
+
if signed_op.op_type != "plc_operation" {
277
+
return Err(CryptoError::PlcOperation(format!(
278
+
"expected type 'plc_operation', got '{}'",
279
+
signed_op.op_type
280
+
)));
281
+
}
282
+
283
+
// Step 3: Base64url-decode the signature field.
256
284
let sig_bytes = URL_SAFE_NO_PAD
257
285
.decode(&signed_op.sig)
258
286
.map_err(|e| CryptoError::PlcOperation(format!("invalid sig base64url: {e}")))?;
259
287
260
-
// Step 3: Parse the 64-byte r‖s fixed-size ECDSA signature.
288
+
// Step 4: Parse the 64-byte r‖s fixed-size ECDSA signature.
261
289
let signature = Signature::try_from(sig_bytes.as_slice())
262
290
.map_err(|e| CryptoError::PlcOperation(format!("invalid ECDSA signature bytes: {e}")))?;
263
291
264
-
// Step 4: Reconstruct the unsigned operation from signed op fields.
292
+
// Step 5: Reconstruct the unsigned operation from signed op fields.
265
293
// Field order must match UnsignedPlcOp's DAG-CBOR canonical ordering.
266
294
let unsigned_op = UnsignedPlcOp {
267
295
prev: signed_op.prev.clone(),
···
272
300
verification_methods: signed_op.verification_methods.clone(),
273
301
};
274
302
275
-
// Step 5: CBOR-encode the unsigned op — byte-exact match to what was signed.
303
+
// Step 6: CBOR-encode the unsigned op — byte-exact match to what was signed.
276
304
let mut unsigned_cbor = Vec::new();
277
305
into_writer(&unsigned_op, &mut unsigned_cbor)
278
306
.map_err(|e| CryptoError::PlcOperation(format!("cbor encode unsigned op: {e}")))?;
279
307
280
-
// Step 6: Parse rotation key URI → P-256 VerifyingKey.
308
+
// Step 7: Parse rotation key URI → P-256 VerifyingKey.
281
309
let key_str = rotation_key.0.strip_prefix("did:key:").ok_or_else(|| {
282
310
CryptoError::PlcOperation("rotation key missing did:key: prefix".to_string())
283
311
})?;
···
291
319
let verifying_key = VerifyingKey::from_sec1_bytes(&multikey_bytes[2..])
292
320
.map_err(|e| CryptoError::PlcOperation(format!("invalid P-256 public key: {e}")))?;
293
321
294
-
// Step 7: Verify the ECDSA-SHA256 signature (SHA-256 applied internally by p256).
322
+
// Step 8: Verify the ECDSA-SHA256 signature (SHA-256 applied internally by p256).
295
323
verifying_key
296
324
.verify(&unsigned_cbor, &signature)
297
325
.map_err(|e| CryptoError::PlcOperation(format!("signature verification failed: {e}")))?;
298
326
299
-
// Step 8: CBOR-encode the signed op and derive the DID.
327
+
// Step 9: CBOR-encode the signed op and derive the DID.
300
328
let mut signed_cbor = Vec::new();
301
329
into_writer(&signed_op, &mut signed_cbor)
302
330
.map_err(|e| CryptoError::PlcOperation(format!("cbor encode signed op: {e}")))?;
···
311
339
let encoded = base32_encoding.encode(hash.as_ref());
312
340
let did = format!("did:plc:{}", &encoded[..24]);
313
341
314
-
// Step 9: Extract atproto_pds endpoint from services map.
342
+
// Step 10: Extract atproto_pds endpoint from services map.
315
343
let atproto_pds_endpoint = signed_op
316
344
.services
317
345
.get("atproto_pds")
···
641
669
assert!(
642
670
matches!(result, Err(CryptoError::PlcOperation(_))),
643
671
"Verify with unknown fields should return CryptoError::PlcOperation"
672
+
);
673
+
}
674
+
675
+
/// MM-90.AC2.1: Rotation op (prev != null) is rejected
676
+
#[test]
677
+
fn verify_rotation_op_with_non_null_prev_returns_error() {
678
+
let (signing_key, op) = make_op_for_verify();
679
+
680
+
// Parse JSON, set prev to a non-null value, re-serialize
681
+
let mut v: serde_json::Value =
682
+
serde_json::from_str(&op.signed_op_json).expect("valid JSON");
683
+
v["prev"] = serde_json::json!("some-hash-value");
684
+
let modified_json = serde_json::to_string(&v).expect("re-serialize JSON");
685
+
686
+
let result = verify_genesis_op(&modified_json, &signing_key);
687
+
688
+
assert!(
689
+
matches!(result, Err(CryptoError::PlcOperation(_))),
690
+
"Verify with non-null prev should return CryptoError::PlcOperation"
691
+
);
692
+
}
693
+
694
+
/// MM-90.AC2.2: Non-genesis op_type is rejected
695
+
#[test]
696
+
fn verify_non_genesis_op_type_returns_error() {
697
+
let (signing_key, op) = make_op_for_verify();
698
+
699
+
// Parse JSON, change type to a rotation type, re-serialize
700
+
let mut v: serde_json::Value =
701
+
serde_json::from_str(&op.signed_op_json).expect("valid JSON");
702
+
v["type"] = serde_json::json!("plc_tombstone");
703
+
let modified_json = serde_json::to_string(&v).expect("re-serialize JSON");
704
+
705
+
let result = verify_genesis_op(&modified_json, &signing_key);
706
+
707
+
assert!(
708
+
matches!(result, Err(CryptoError::PlcOperation(_))),
709
+
"Verify with non-genesis op_type should return CryptoError::PlcOperation"
710
+
);
711
+
}
712
+
713
+
/// MM-90.AC3: Canonical usage pattern — rotation key signs and appears at rotationKeys[0].
714
+
/// The same keypair is both the rotation key and the signing key.
715
+
#[test]
716
+
fn verify_rotation_key_can_verify_own_op() {
717
+
let kp = generate_p256_keypair().expect("keypair");
718
+
let private_key_bytes = *kp.private_key_bytes;
719
+
720
+
// Use the SAME keypair as both rotation and signing key.
721
+
// This is the canonical usage: kp appears at rotationKeys[0] AND signs the op.
722
+
let op = build_did_plc_genesis_op(
723
+
&kp.key_id,
724
+
&kp.key_id,
725
+
&private_key_bytes,
726
+
"alice.example.com",
727
+
"https://relay.example.com",
728
+
)
729
+
.expect("genesis op should succeed");
730
+
731
+
// Verify using kp.key_id (which appears in rotationKeys[0] AND made the signature).
732
+
let result = verify_genesis_op(&op.signed_op_json, &kp.key_id);
733
+
734
+
assert!(
735
+
result.is_ok(),
736
+
"Verify with rotation key that signed and appears at rotationKeys[0] should succeed"
737
+
);
738
+
let verified = result.unwrap();
739
+
assert_eq!(
740
+
verified.did, op.did,
741
+
"DID should match the original op's DID"
644
742
);
645
743
}
646
744
}