diff --git a/client/tests/common/mod.rs b/client/tests/common/mod.rs index b433de45..0bc96147 100644 --- a/client/tests/common/mod.rs +++ b/client/tests/common/mod.rs @@ -206,9 +206,13 @@ pub async fn dev_discovery() -> Option { // ─── In-process provider node (for StorageUserClient tests) ────────────────── /// Spawn an in-process provider node on a random port and return its URL. +/// +/// Uses `//Alice` as the signing key so endpoints that sign commitments +/// (`/commit`, `/commitment`, `/checkpoint/sign`, `/delete`) work end-to-end. pub async fn start_test_provider() -> String { let storage = Arc::new(Storage::new()); - let state = Arc::new(ProviderState::new(storage, "0xtest_provider".to_string())); + let state = + Arc::new(ProviderState::with_seed(storage, "//Alice").expect("//Alice is a valid SURI")); let app = create_router(state); let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); diff --git a/provider-node/src/api.rs b/provider-node/src/api.rs index 04b84bec..3fe57edb 100644 --- a/provider-node/src/api.rs +++ b/provider-node/src/api.rs @@ -17,7 +17,7 @@ use axum::{ }; use base64::{engine::general_purpose::STANDARD as BASE64, Engine}; use codec::Encode; -use sp_core::{Pair, H256}; +use sp_core::H256; use std::sync::Arc; use storage_primitives::{CheckpointProposal, CommitmentPayload}; use tower_http::cors::CorsLayer; @@ -273,7 +273,7 @@ async fn commit( // Create commitment payload and sign it // Note: leaf_count is set to 0 to match pallet's challenge_offchain verification let payload = CommitmentPayload::new(request.bucket_id, mmr_root, start_seq, 0); - let signature = state.sign(&payload.encode()); + let signature = state.sign(&payload.encode())?; Ok(Json(CommitResponse { mmr_root: format!("0x{}", hex_encode(mmr_root.as_bytes())), @@ -338,7 +338,7 @@ async fn get_commitment( // Create commitment payload and sign it // Note: leaf_count is set to 0 to match pallet's challenge_offchain verification let payload = CommitmentPayload::new(query.bucket_id, bucket.mmr_root, bucket.start_seq, 0); - let signature = state.sign(&payload.encode()); + let signature = state.sign(&payload.encode())?; Ok(Json(CommitmentResponse { bucket_id: query.bucket_id, @@ -372,7 +372,7 @@ async fn get_checkpoint_signature( bucket.start_seq, leaf_count, ); - let signature = state.sign(&payload.encode()); + let signature = state.sign(&payload.encode())?; Ok(Json(CheckpointSignatureResponse { bucket_id: query.bucket_id, @@ -467,7 +467,7 @@ async fn delete_data( // Create commitment payload and sign it // Note: leaf_count is set to 0 to match pallet's challenge_offchain verification let payload = CommitmentPayload::new(request.bucket_id, mmr_root, start_seq, 0); - let signature = state.sign(&payload.encode()); + let signature = state.sign(&payload.encode())?; Ok(Json(DeleteResponse { mmr_root: format!("0x{}", hex_encode(mmr_root.as_bytes())), @@ -596,16 +596,7 @@ async fn sign_checkpoint_proposal( ); let encoded = proposal.encode(); - let signature = match &state.keypair { - Some(kp) => { - let sig = kp.sign(&encoded); - format!("0x{}", hex::encode(sig.0)) - } - None => { - // No keypair configured - return placeholder - format!("0x{}", hex::encode([0u8; 64])) - } - }; + let signature = state.sign(&encoded)?; Ok(Json(SignProposalResponse { signer: state.provider_id.clone(), diff --git a/provider-node/src/error.rs b/provider-node/src/error.rs index 9a038885..8dde76e2 100644 --- a/provider-node/src/error.rs +++ b/provider-node/src/error.rs @@ -66,6 +66,9 @@ pub enum Error { #[error("Insufficient role for this operation")] InsufficientRole, + + #[error("Signing unavailable: provider has no keypair configured")] + SigningUnavailable, } #[derive(Serialize)] @@ -200,6 +203,16 @@ impl IntoResponse for Error { details: None, }, ), + Error::SigningUnavailable => ( + StatusCode::SERVICE_UNAVAILABLE, + ErrorResponse { + error: "signing_unavailable".to_string(), + details: Some(serde_json::json!({ + "message": "provider node has no signing key configured; \ + start with --keyfile to enable signing-bound endpoints" + })), + }, + ), }; (status, Json(error_response)).into_response() diff --git a/provider-node/src/lib.rs b/provider-node/src/lib.rs index 59f6fb8b..7817c17d 100644 --- a/provider-node/src/lib.rs +++ b/provider-node/src/lib.rs @@ -121,17 +121,112 @@ impl ProviderState { } } - /// Sign a message and return the signature as hex. - pub fn sign(&self, message: &[u8]) -> String { - match &self.keypair { - Some(keypair) => { - let signature = keypair.sign(message); - format!("0x{}", hex::encode(signature.0)) - } - None => { - // Return placeholder if no keypair configured - format!("0x{}", hex::encode([0u8; 64])) - } + /// Sign a message and return the signature as `0x`-prefixed hex. + /// + /// Returns `Err(Error::SigningUnavailable)` if no keypair is configured. + /// Callers must propagate this so the HTTP layer returns 503 rather than + /// silently emitting a 64-zero-byte placeholder signature, which would + /// be a cryptographically invalid commitment masquerading as a real one. + pub fn sign(&self, message: &[u8]) -> Result { + let keypair = self.keypair.as_ref().ok_or(Error::SigningUnavailable)?; + let signature = keypair.sign(message); + Ok(format!("0x{}", hex::encode(signature.0))) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use sp_core::ByteArray; + + fn test_storage() -> Arc { + Arc::new(storage::Storage::new()) + } + + #[test] + fn sign_without_keypair_refuses_with_signing_unavailable() { + // The pre-fix behaviour silently returned 64 zero bytes. The new + // contract is that `sign()` MUST return `Err(SigningUnavailable)` + // when no keypair is configured, so the HTTP layer can map it to a + // 503 instead of emitting a cryptographically invalid placeholder. + let state = ProviderState::new(test_storage(), "no-key-provider".to_string()); + let err = state + .sign(b"any message") + .expect_err("must refuse to sign without a keypair"); + assert!(matches!(err, Error::SigningUnavailable)); + } + + #[test] + fn sign_with_keypair_returns_real_sr25519_signature() { + // Round-trip: sign with //Alice, decode the hex, verify against + // Alice's public key. This catches any regression where sign() ever + // returns the 0x00..00 placeholder again, and also catches the more + // subtle case where the bytes look random but aren't valid sr25519. + let state = ProviderState::with_seed(test_storage(), "//Alice").unwrap(); + let message = b"commitment-payload-bytes"; + + let sig_hex = state.sign(message).expect("signing succeeds with keypair"); + let sig_bytes = hex::decode(sig_hex.strip_prefix("0x").unwrap()).unwrap(); + assert_eq!(sig_bytes.len(), 64, "sr25519 signatures are 64 bytes"); + assert_ne!( + sig_bytes, + vec![0u8; 64], + "must not return the zero-byte placeholder" + ); + + let sig = sr25519::Signature::from_slice(&sig_bytes).unwrap(); + let alice = sr25519::Pair::from_string("//Alice", None).unwrap(); + assert!( + sr25519::Pair::verify(&sig, message, &alice.public()), + "signature did not verify under //Alice's public key" + ); + } + + #[test] + fn sign_produces_distinct_signatures_each_call_but_all_verify() { + // sr25519 (schnorrkel) is randomised — two calls over the same + // message produce different signatures, but both must verify. This + // test guards against accidentally swapping to a backend that + // returns a constant value (e.g. zero bytes). + let state = ProviderState::with_seed(test_storage(), "//Alice").unwrap(); + let alice_pub = sr25519::Pair::from_string("//Alice", None) + .unwrap() + .public(); + let msg = b"commitment-payload"; + + let sig_a = state.sign(msg).unwrap(); + let sig_b = state.sign(msg).unwrap(); + + for sig_hex in [&sig_a, &sig_b] { + let bytes = hex::decode(sig_hex.strip_prefix("0x").unwrap()).unwrap(); + assert_ne!(bytes, vec![0u8; 64]); + let sig = sr25519::Signature::from_slice(&bytes).unwrap(); + assert!(sr25519::Pair::verify(&sig, msg, &alice_pub)); } } + + #[test] + fn signatures_from_different_keys_do_not_cross_verify() { + // Negative control: //Bob's signature must NOT verify under //Alice. + // Cheap protection against a future refactor that accidentally + // stops checking the message or the key. + let alice = ProviderState::with_seed(test_storage(), "//Alice").unwrap(); + let bob = ProviderState::with_seed(test_storage(), "//Bob").unwrap(); + let alice_pub = sr25519::Pair::from_string("//Alice", None) + .unwrap() + .public(); + let msg = b"checkpoint payload"; + + let bob_sig_hex = bob.sign(msg).unwrap(); + let bob_sig_bytes = hex::decode(bob_sig_hex.strip_prefix("0x").unwrap()).unwrap(); + let bob_sig = sr25519::Signature::from_slice(&bob_sig_bytes).unwrap(); + + assert!(!sr25519::Pair::verify(&bob_sig, msg, &alice_pub)); + + // Sanity: //Alice's own signature still verifies under her own key. + let alice_sig_hex = alice.sign(msg).unwrap(); + let alice_sig_bytes = hex::decode(alice_sig_hex.strip_prefix("0x").unwrap()).unwrap(); + let alice_sig = sr25519::Signature::from_slice(&alice_sig_bytes).unwrap(); + assert!(sr25519::Pair::verify(&alice_sig, msg, &alice_pub)); + } } diff --git a/provider-node/tests/api_integration.rs b/provider-node/tests/api_integration.rs index bf6ee62f..c72c97be 100644 --- a/provider-node/tests/api_integration.rs +++ b/provider-node/tests/api_integration.rs @@ -4,10 +4,13 @@ use axum::http::StatusCode; use base64::{engine::general_purpose::STANDARD as BASE64, Engine}; +use codec::Encode; use reqwest::Client; use serde_json::{json, Value}; +use sp_core::{sr25519, ByteArray, Pair, H256}; use std::net::SocketAddr; use std::sync::Arc; +use storage_primitives::CommitmentPayload; use storage_provider_node::{create_router, ProviderState, Storage}; use tokio::net::TcpListener; @@ -18,10 +21,31 @@ struct TestServer { } impl TestServer { + /// Spin up a server with `//Alice` as the signing key. + /// + /// Endpoints that sign commitments (`/commit`, `/commitment`, ...) work + /// because a real sr25519 keypair is available. async fn new() -> Self { - let storage = Arc::new(Storage::new()); - let state = Arc::new(ProviderState::new(storage, "0xtest_provider".to_string())); + Self::with_state(Arc::new( + ProviderState::with_seed(Arc::new(Storage::new()), "//Alice") + .expect("//Alice is a valid SURI"), + )) + .await + } + + /// Spin up a server with no signing key. + /// + /// Used to verify that signing-bound endpoints return 503 rather than + /// silently emitting zero-byte placeholder signatures. + async fn new_unsigned() -> Self { + Self::with_state(Arc::new(ProviderState::new( + Arc::new(Storage::new()), + "0xtest_provider".to_string(), + ))) + .await + } + async fn with_state(state: Arc) -> Self { let app = create_router(state); // Bind to port 0 to get a random available port @@ -386,6 +410,345 @@ async fn test_full_upload_commit_read_flow() { assert!(read_body["chunks"].is_array()); } +// ───────────────────────────────────────────────────────────────────────────── +// Signing security: commit/commitment endpoints must never emit a placeholder +// 64-zero-byte "signature" when the provider has no keypair configured. +// They must refuse with 503 Service Unavailable instead. +// ───────────────────────────────────────────────────────────────────────────── + +/// Upload a single chunk and commit it. Returns the chunk hash hex and +/// the parsed commit-response body. +async fn upload_and_commit(server: &TestServer, bucket_id: u64) -> (String, Value) { + let data = b"chunk-for-signature-tests"; + let hash = storage_primitives::blake2_256(data); + let hash_hex = format!("0x{}", hex_encode(hash.as_bytes())); + + let resp = server + .client + .put(server.url("/node")) + .json(&json!({ + "bucket_id": bucket_id, + "hash": hash_hex, + "data": BASE64.encode(data), + "children": null, + })) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + + let commit_resp = server + .client + .post(server.url("/commit")) + .json(&json!({ + "bucket_id": bucket_id, + "data_roots": [hash_hex], + })) + .send() + .await + .unwrap(); + + let status = commit_resp.status(); + let body: Value = commit_resp.json().await.unwrap(); + assert_eq!(status, StatusCode::OK, "commit failed: {body:?}"); + (hash_hex, body) +} + +/// `//Alice`'s sr25519 public key, used to verify signatures produced by the +/// server. Derived once via `sr25519::Pair::from_string("//Alice", None)`. +fn alice_public() -> sr25519::Public { + sr25519::Pair::from_string("//Alice", None) + .expect("//Alice is a valid SURI") + .public() +} + +#[tokio::test] +async fn commit_returns_valid_sr25519_signature_over_commitment_payload() { + // The whole point of the zero-byte-signing fix: when a key IS configured, + // the server must produce a real sr25519 signature that verifies under the + // provider's public key against the exact bytes the pallet will hash. + let server = TestServer::new().await; + let bucket_id = 7; + + let (_chunk_hash, body) = upload_and_commit(&server, bucket_id).await; + + let mmr_root_hex = body["mmr_root"].as_str().expect("mmr_root present"); + let start_seq = body["start_seq"].as_u64().expect("start_seq present"); + let sig_hex = body["provider_signature"] + .as_str() + .expect("provider_signature present"); + + // Defensive: the zero-byte placeholder this PR removes was exactly 64 + // hex-encoded zero bytes. If it ever returns, this catches it. + let sig_bytes = hex_decode(sig_hex).expect("signature hex parses"); + assert_eq!(sig_bytes.len(), 64, "sr25519 signatures are 64 bytes"); + assert_ne!( + sig_bytes, + vec![0u8; 64], + "server returned zero-byte placeholder instead of a real signature" + ); + + // Reconstruct exactly what the handler signed: CommitmentPayload with + // leaf_count = 0 (matches /commit's encoding). + let mmr_root_bytes = hex_decode(mmr_root_hex).unwrap(); + let mmr_root = H256::from_slice(&mmr_root_bytes); + let payload = CommitmentPayload::new(bucket_id, mmr_root, start_seq, 0); + let encoded = payload.encode(); + + let sig = sr25519::Signature::from_slice(&sig_bytes).expect("64-byte signature"); + assert!( + sr25519::Pair::verify(&sig, &encoded, &alice_public()), + "signature did not verify against //Alice over the expected commitment payload" + ); +} + +#[tokio::test] +async fn checkpoint_signature_verifies_with_real_leaf_count() { + // `/checkpoint-signature` signs the payload with the *real* leaf_count + // (unlike /commit which uses 0). Verify both that a real signature comes + // back and that it round-trips against the public key. + let server = TestServer::new().await; + let bucket_id = 8; + + upload_and_commit(&server, bucket_id).await; + + let resp = server + .client + .get(server.url(&format!("/checkpoint-signature?bucket_id={bucket_id}"))) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + let body: Value = resp.json().await.unwrap(); + + let mmr_root = H256::from_slice(&hex_decode(body["mmr_root"].as_str().unwrap()).unwrap()); + let start_seq = body["start_seq"].as_u64().unwrap(); + let leaf_count = body["leaf_count"].as_u64().unwrap(); + assert!(leaf_count > 0, "leaf_count must be the real on-disk value"); + + let sig_bytes = hex_decode(body["provider_signature"].as_str().unwrap()).unwrap(); + assert_ne!(sig_bytes, vec![0u8; 64]); + + let payload = CommitmentPayload::new(bucket_id, mmr_root, start_seq, leaf_count); + let sig = sr25519::Signature::from_slice(&sig_bytes).unwrap(); + assert!(sr25519::Pair::verify( + &sig, + payload.encode(), + &alice_public() + )); +} + +#[tokio::test] +async fn commitment_signature_does_not_verify_under_a_different_key() { + // Negative control: //Bob must not be able to take credit for //Alice's + // signature. Catches accidental no-op verifiers. + let server = TestServer::new().await; + let bucket_id = 9; + + let (_h, body) = upload_and_commit(&server, bucket_id).await; + let mmr_root = H256::from_slice(&hex_decode(body["mmr_root"].as_str().unwrap()).unwrap()); + let start_seq = body["start_seq"].as_u64().unwrap(); + let sig = sr25519::Signature::from_slice( + &hex_decode(body["provider_signature"].as_str().unwrap()).unwrap(), + ) + .unwrap(); + let encoded = CommitmentPayload::new(bucket_id, mmr_root, start_seq, 0).encode(); + + let bob = sr25519::Pair::from_string("//Bob", None).unwrap().public(); + assert!( + !sr25519::Pair::verify(&sig, &encoded, &bob), + "Alice's signature wrongly verifies under Bob's key" + ); +} + +#[tokio::test] +async fn commit_returns_503_when_no_signing_key_configured() { + // The pre-fix behaviour silently returned 64 zero bytes here. Post-fix, + // the handler must refuse with 503 Service Unavailable so callers do not + // mistake a placeholder for a real provider commitment. + let server = TestServer::new_unsigned().await; + + let data = b"chunk-without-key"; + let hash = storage_primitives::blake2_256(data); + let hash_hex = format!("0x{}", hex_encode(hash.as_bytes())); + + server + .client + .put(server.url("/node")) + .json(&json!({ + "bucket_id": 1, + "hash": hash_hex, + "data": BASE64.encode(data), + "children": null, + })) + .send() + .await + .unwrap(); + + let resp = server + .client + .post(server.url("/commit")) + .json(&json!({ "bucket_id": 1, "data_roots": [hash_hex] })) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); + let body: Value = resp.json().await.unwrap(); + assert_eq!(body["error"], "signing_unavailable"); +} + +#[tokio::test] +async fn commitment_endpoint_returns_503_when_no_signing_key() { + // GET /commitment also signs the response — it must also refuse rather + // than emit zero bytes. + let server = TestServer::new_unsigned().await; + + // Seed the bucket via storage so the bucket-lookup gate passes and the + // handler actually reaches the signing step. Calling /commit on the + // unsigned server returns 503 but still mutates storage, which is enough + // to make /commitment reach state.sign(...). + let data = b"chunk-for-commitment-503"; + let hash = storage_primitives::blake2_256(data); + let hash_hex = format!("0x{}", hex_encode(hash.as_bytes())); + server + .client + .put(server.url("/node")) + .json(&json!({ + "bucket_id": 1, + "hash": hash_hex, + "data": BASE64.encode(data), + "children": null, + })) + .send() + .await + .unwrap(); + server + .client + .post(server.url("/commit")) + .json(&json!({ "bucket_id": 1, "data_roots": [hash_hex] })) + .send() + .await + .unwrap(); + + let resp = server + .client + .get(server.url("/commitment?bucket_id=1")) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); + let body: Value = resp.json().await.unwrap(); + assert_eq!(body["error"], "signing_unavailable"); +} + +#[tokio::test] +async fn checkpoint_sign_endpoint_returns_503_when_no_signing_key() { + // POST /checkpoint/sign is the second of the two duplicated zero-byte + // sites the audit flagged (provider-node/src/api.rs:604-608). It must + // also refuse with 503 — and crucially, never emit a 0x00..00 signature. + let unsigned = TestServer::new_unsigned().await; + let keyed = TestServer::new().await; + + // Seed identical state on both servers so the unsigned server's local + // MMR matches the proposal and the handler reaches the signing step + // (it short-circuits with `agreed: false` if the local root differs). + let data = b"chunk-for-checkpoint-sign"; + let hash = storage_primitives::blake2_256(data); + let hash_hex = format!("0x{}", hex_encode(hash.as_bytes())); + for s in [&unsigned, &keyed] { + s.client + .put(s.url("/node")) + .json(&json!({ + "bucket_id": 1, + "hash": hash_hex, + "data": BASE64.encode(data), + "children": null, + })) + .send() + .await + .unwrap(); + s.client + .post(s.url("/commit")) + .json(&json!({ "bucket_id": 1, "data_roots": [hash_hex] })) + .send() + .await + .unwrap(); + } + // Take the proposal parameters from the keyed server (its /commit + // succeeded). The unsigned server's storage mutated on its own 503'd + // /commit, so both servers' MMR roots are identical. + let proposal = keyed + .client + .get(keyed.url("/commitment?bucket_id=1")) + .send() + .await + .unwrap() + .json::() + .await + .unwrap(); + + let resp = unsigned + .client + .post(unsigned.url("/checkpoint/sign")) + .json(&json!({ + "bucket_id": 1, + "mmr_root": proposal["mmr_root"], + "start_seq": proposal["start_seq"], + "leaf_count": proposal["leaf_count"], + "window": 0, + })) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); + let body: Value = resp.json().await.unwrap(); + assert_eq!(body["error"], "signing_unavailable"); +} + +#[tokio::test] +async fn delete_endpoint_returns_503_when_no_signing_key() { + let server = TestServer::new_unsigned().await; + + // Seed and commit so the delete handler can reach its sign() call. + let data = b"chunk-for-delete-503"; + let hash = storage_primitives::blake2_256(data); + let hash_hex = format!("0x{}", hex_encode(hash.as_bytes())); + server + .client + .put(server.url("/node")) + .json(&json!({ + "bucket_id": 1, + "hash": hash_hex, + "data": BASE64.encode(data), + "children": null, + })) + .send() + .await + .unwrap(); + server + .client + .post(server.url("/commit")) + .json(&json!({ "bucket_id": 1, "data_roots": [hash_hex] })) + .send() + .await + .unwrap(); + + let resp = server + .client + .post(server.url("/delete")) + .json(&json!({ + "bucket_id": 1, + "new_start_seq": 1, + "admin_signature": "0x00", + })) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); + let body: Value = resp.json().await.unwrap(); + assert_eq!(body["error"], "signing_unavailable"); +} + // Helper functions fn hex_encode(bytes: &[u8]) -> String {