Skip to content

Commit ac08fd3

Browse files
Use JSON payload for request signing to prevent signature confusion
Address PR review feedback from aram356: - Replace colon-delimited build_payload with JSON serialization to prevent signature confusion via crafted Host headers - Include signing protocol version in the signed payload - Remove trivial test_signing_version_constant test
1 parent 84c7ae0 commit ac08fd3

1 file changed

Lines changed: 50 additions & 17 deletions

File tree

crates/common/src/request_signing/signing.rs

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
use base64::{engine::general_purpose, Engine};
77
use ed25519_dalek::{Signature, Signer as Ed25519Signer, SigningKey, Verifier, VerifyingKey};
88
use error_stack::{Report, ResultExt};
9+
use serde::Serialize;
910

1011
use crate::error::TrustedServerError;
1112
use crate::fastly_storage::{FastlyConfigStore, FastlySecretStore};
@@ -48,6 +49,20 @@ pub struct RequestSigner {
4849
/// Current version of the signing protocol
4950
pub const SIGNING_VERSION: &str = "1.1";
5051

52+
/// Canonical payload structure for request signing.
53+
///
54+
/// Serialized as JSON to prevent signature confusion attacks that could
55+
/// exploit delimiter-based formats.
56+
#[derive(Serialize)]
57+
struct SigningPayload<'a> {
58+
version: &'a str,
59+
kid: &'a str,
60+
host: &'a str,
61+
scheme: &'a str,
62+
id: &'a str,
63+
ts: u64,
64+
}
65+
5166
/// Parameters for enhanced request signing
5267
#[derive(Debug, Clone)]
5368
pub struct SigningParams {
@@ -74,13 +89,26 @@ impl SigningParams {
7489

7590
/// Builds the canonical payload string for signing.
7691
///
77-
/// Format: `kid:request_host:request_scheme:id:ts`
78-
#[must_use]
79-
pub fn build_payload(&self, kid: &str) -> String {
80-
format!(
81-
"{}:{}:{}:{}:{}",
82-
kid, self.request_host, self.request_scheme, self.request_id, self.timestamp
83-
)
92+
/// The payload is a JSON-serialized [`SigningPayload`] to prevent signature
93+
/// confusion attacks that could exploit delimiter-based formats.
94+
///
95+
/// # Errors
96+
///
97+
/// Returns an error if the payload cannot be serialized to JSON.
98+
pub fn build_payload(&self, kid: &str) -> Result<String, Report<TrustedServerError>> {
99+
let payload = SigningPayload {
100+
version: SIGNING_VERSION,
101+
kid,
102+
host: &self.request_host,
103+
scheme: &self.request_scheme,
104+
id: &self.request_id,
105+
ts: self.timestamp,
106+
};
107+
serde_json::to_string(&payload).map_err(|e| {
108+
Report::new(TrustedServerError::Configuration {
109+
message: format!("Failed to serialize signing payload: {}", e),
110+
})
111+
})
84112
}
85113
}
86114

@@ -124,7 +152,8 @@ impl RequestSigner {
124152

125153
/// Signs a request using the enhanced v1.1 signing protocol.
126154
///
127-
/// The signed payload format is: `kid:request_host:request_scheme:id:ts`
155+
/// The signed payload is a JSON object containing version, kid, host,
156+
/// scheme, id, and ts fields.
128157
///
129158
/// # Errors
130159
///
@@ -133,7 +162,7 @@ impl RequestSigner {
133162
&self,
134163
params: &SigningParams,
135164
) -> Result<String, Report<TrustedServerError>> {
136-
let payload = params.build_payload(&self.kid);
165+
let payload = params.build_payload(&self.kid)?;
137166
self.sign(payload.as_bytes())
138167
}
139168
}
@@ -298,8 +327,17 @@ mod tests {
298327
timestamp: 1706900000,
299328
};
300329

301-
let payload = params.build_payload("kid-abc");
302-
assert_eq!(payload, "kid-abc:example.com:https:req-123:1706900000");
330+
let payload = params
331+
.build_payload("kid-abc")
332+
.expect("should build payload");
333+
let parsed: serde_json::Value =
334+
serde_json::from_str(&payload).expect("should be valid JSON");
335+
assert_eq!(parsed["version"], SIGNING_VERSION);
336+
assert_eq!(parsed["kid"], "kid-abc");
337+
assert_eq!(parsed["host"], "example.com");
338+
assert_eq!(parsed["scheme"], "https");
339+
assert_eq!(parsed["id"], "req-123");
340+
assert_eq!(parsed["ts"], 1706900000);
303341
}
304342

305343
#[test]
@@ -335,7 +373,7 @@ mod tests {
335373
assert!(!signature.is_empty());
336374

337375
// Verify the signature is valid by reconstructing the payload
338-
let payload = params.build_payload(&signer.kid);
376+
let payload = params.build_payload(&signer.kid).unwrap();
339377
let result = verify_signature(payload.as_bytes(), &signature, &signer.kid).unwrap();
340378
assert!(result, "Enhanced signature should be valid");
341379
}
@@ -366,9 +404,4 @@ mod tests {
366404
"Different hosts should produce different signatures"
367405
);
368406
}
369-
370-
#[test]
371-
fn test_signing_version_constant() {
372-
assert_eq!(SIGNING_VERSION, "1.1");
373-
}
374407
}

0 commit comments

Comments
 (0)