Skip to content

Commit fb63807

Browse files
AkshatMAkshatM
authored
Drop requiring alg to be present on the message + enhancements (#39)
`alg` is discouraged owing to the possibility of key downgrade attacks in [RFC 9421](https://www.rfc-editor.org/rfc/rfc9421.html#name-key-and-algorithm-specifica). Previous incarnations of this crate assumed that `alg` would either be present in the message or supplied externally. This change instead requires the *keyring* to remember which algorithm a key corresponds to during verification, meaning we no longer trust the message's use of `alg`. The change involves a few places: 1. `KeyRing` is now a hashmap from key IDs to algorithms and public keys, rather than just public keys. In the long term, we should move these fields to a dedicated `KeyAndMetadata` struct. 2. `Algorithm` has been extended to support all known algorithms per the IANA list. However, we will continue to only support importing ed25519 keys in the keyring. If the community needs this, people can add support. Similarly, we will not attempt to verify or sign a message for non-ed25519 keys just now. I've additionally dropped `alg` being required on the HTTP signature directory. I've also lumped in a feature request I received internally: the ability to inspect the message components in the signature base. I did this by making `ParsedLabel` public, as well as `SignatureBase`. They were not public previously since I surmised people would find more value in these details being opaque - but there's no serious reason they need to be private. It also means we don't need to needlessly clone `ParameterDetails`.
1 parent deb9075 commit fb63807

File tree

9 files changed

+148
-118
lines changed

9 files changed

+148
-118
lines changed

‎Cargo.lock‎

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ members = [
66
resolver = "2"
77

88
[workspace.package]
9-
version = "0.2.1"
9+
version = "0.3.0"
1010
authors = [
1111
"Akshat Mahajan <akshat@cloudflare.com>",
1212
"Gauri Baraskar <gbaraskar@cloudflare.com>",
@@ -32,4 +32,4 @@ serde_json = "1.0.140"
3232
data-url = "0.3.1"
3333

3434
# workspace dependencies
35-
web-bot-auth = { version = "0.2.1", path = "./crates/web-bot-auth" }
35+
web-bot-auth = { version = "0.3.0", path = "./crates/web-bot-auth" }

‎crates/http-signature-directory/src/main.rs‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use reqwest::{
1212
use web_bot_auth::{
1313
components::{CoveredComponent, DerivedComponent},
1414
keyring::{JSONWebKeySet, KeyRing, Thumbprintable},
15-
message_signatures::{Algorithm, MessageVerifier, SignedMessage},
15+
message_signatures::{MessageVerifier, SignedMessage},
1616
};
1717

1818
const MIME_TYPE: &str = "application/http-message-signatures-directory+json";
@@ -148,7 +148,6 @@ fn main() -> Result<(), String> {
148148

149149
let verifier = MessageVerifier::parse(
150150
&directory,
151-
Some(Algorithm::Ed25519),
152151
|(_, innerlist)| {
153152
innerlist.params.contains_key("expires")
154153
&& innerlist.params.contains_key("created")
@@ -159,13 +158,6 @@ fn main() -> Result<(), String> {
159158
.is_some_and(|tag| {
160159
tag.as_str() == "http-message-signatures-directory"
161160
})
162-
&& innerlist
163-
.params
164-
.get("alg")
165-
.and_then(|tag| tag.as_string())
166-
.is_some_and(|tag| {
167-
tag.as_str() == "ed25519"
168-
})
169161
&& innerlist
170162
.params
171163
.get("keyid")

‎crates/web-bot-auth/src/keyring.rs‎

Lines changed: 72 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ use std::collections::HashMap;
77
/// Errors that may be thrown by this module
88
/// when importing a JWK key.
99
#[derive(Debug)]
10-
pub enum ImportError {
10+
pub enum KeyringError {
1111
/// JWK key specified an unsupported algorithm
1212
UnsupportedAlgorithm,
1313
/// The contained parameters could not be
@@ -23,6 +23,37 @@ pub enum ImportError {
2323
/// Represents a public key to be consumed during the verification.
2424
pub type PublicKey = Vec<u8>;
2525

26+
/// Subset of [HTTP signature algorithm](https://www.iana.org/assignments/http-message-signature/http-message-signature.xhtml)
27+
/// implemented in this module. In the future, we may support more.
28+
#[derive(Clone, Debug, PartialEq, Eq)]
29+
pub enum Algorithm {
30+
/// [The `ed25519` algorithm](https://www.rfc-editor.org/rfc/rfc9421#name-eddsa-using-curve-edwards25)
31+
Ed25519,
32+
/// [The `rsa-pss-sha512` algorithm](https://www.rfc-editor.org/rfc/rfc9421.html#name-rsassa-pss-using-sha-512)
33+
RsaPssSha512,
34+
/// [The `rsa-v1_5-sha256` algorithm](https://www.rfc-editor.org/rfc/rfc9421.html#name-rsassa-pkcs1-v1_5-using-sha)
35+
RsaV1_5Sha256,
36+
/// [The `hmac-sha256` algorithm](https://www.rfc-editor.org/rfc/rfc9421.html#name-hmac-using-sha-256)
37+
HmacSha256,
38+
/// [The `ecdsa-p256-sha256` algorithm](https://www.rfc-editor.org/rfc/rfc9421.html#name-ecdsa-using-curve-p-256-dss)
39+
EcdsaP256Sha256,
40+
/// [The `ecdsa-p384-sha384` algorithm](https://www.rfc-editor.org/rfc/rfc9421.html#name-ecdsa-using-curve-p-384-dss)
41+
EcdsaP384Sha384,
42+
}
43+
44+
impl std::fmt::Display for Algorithm {
45+
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
46+
match self {
47+
Algorithm::Ed25519 => write!(f, "ed25519"),
48+
Algorithm::RsaPssSha512 => write!(f, "rsa-pss-sha512"),
49+
Algorithm::RsaV1_5Sha256 => write!(f, "rsa-pss-sha512"),
50+
Algorithm::HmacSha256 => write!(f, "hmac-sha256"),
51+
Algorithm::EcdsaP256Sha256 => write!(f, "ecdsa-p256-sha256"),
52+
Algorithm::EcdsaP384Sha384 => write!(f, "ecdsa-p384-sha384"),
53+
}
54+
}
55+
}
56+
2657
/// Represents a JSON Web Key containing the bare minimum that
2758
/// can be thumbprinted per [RFC 7638](https://www.rfc-editor.org/rfc/rfc7638.html)
2859
#[derive(Eq, PartialEq, Debug, Clone, Serialize, Deserialize)]
@@ -90,20 +121,35 @@ impl Thumbprintable {
90121
/// Today we only support importing ed25519 keys. Errors may
91122
/// be thrown when decoding or converting the JSON web key
92123
/// into an ed25519 public key.
93-
pub fn public_key(&self) -> Result<Vec<u8>, ImportError> {
124+
pub fn public_key(&self) -> Result<Vec<u8>, KeyringError> {
94125
match self {
95126
Thumbprintable::OKP { crv, x } => match crv.as_str() {
96127
"Ed25519" => {
97128
let decoded = general_purpose::URL_SAFE_NO_PAD
98129
.decode(x)
99-
.map_err(ImportError::ParsingError)?;
130+
.map_err(KeyringError::ParsingError)?;
100131
VerifyingKey::try_from(decoded.as_slice())
101132
.map(|key| key.to_bytes().to_vec())
102-
.map_err(ImportError::ConversionError)
133+
.map_err(KeyringError::ConversionError)
103134
}
104-
_ => Err(ImportError::UnsupportedAlgorithm),
135+
_ => Err(KeyringError::UnsupportedAlgorithm),
136+
},
137+
_ => Err(KeyringError::UnsupportedAlgorithm),
138+
}
139+
}
140+
141+
/// Attempt to extract algorithm.
142+
///
143+
/// # Errors
144+
///
145+
/// Today we only support extracting the algorithm of an ed25519 key.
146+
pub fn algorithm(&self) -> Result<Algorithm, KeyringError> {
147+
match self {
148+
Thumbprintable::OKP { crv, .. } => match crv.as_str() {
149+
"Ed25519" => Ok(Algorithm::Ed25519),
150+
_ => Err(KeyringError::UnsupportedAlgorithm),
105151
},
106-
_ => Err(ImportError::UnsupportedAlgorithm),
152+
_ => Err(KeyringError::UnsupportedAlgorithm),
107153
}
108154
}
109155
}
@@ -112,11 +158,11 @@ impl Thumbprintable {
112158
/// verifying keys for verificiation.
113159
#[derive(Default, Debug, Clone)]
114160
pub struct KeyRing {
115-
ring: HashMap<String, PublicKey>,
161+
ring: HashMap<String, (Algorithm, PublicKey)>,
116162
}
117163

118-
impl FromIterator<(String, PublicKey)> for KeyRing {
119-
fn from_iter<T: IntoIterator<Item = (String, PublicKey)>>(iter: T) -> KeyRing {
164+
impl FromIterator<(String, (Algorithm, PublicKey))> for KeyRing {
165+
fn from_iter<T: IntoIterator<Item = (String, (Algorithm, PublicKey))>>(iter: T) -> KeyRing {
120166
KeyRing {
121167
ring: HashMap::from_iter(iter),
122168
}
@@ -126,8 +172,17 @@ impl FromIterator<(String, PublicKey)> for KeyRing {
126172
impl KeyRing {
127173
/// Insert a raw public key under a known identifier. If an identifier is already
128174
/// known, it will *not* be updated and this method will return false.
129-
pub fn import_raw(&mut self, identifier: String, public_key: Vec<u8>) -> bool {
130-
!self.ring.contains_key(&identifier) && self.ring.insert(identifier, public_key).is_none()
175+
pub fn import_raw(
176+
&mut self,
177+
identifier: String,
178+
algorithm: Algorithm,
179+
public_key: Vec<u8>,
180+
) -> bool {
181+
!self.ring.contains_key(&identifier)
182+
&& self
183+
.ring
184+
.insert(identifier, (algorithm, public_key))
185+
.is_none()
131186
}
132187

133188
/// Rename a public key from `old_identifier` to `new_identifier`. Returns `false` if the old
@@ -140,7 +195,7 @@ impl KeyRing {
140195
}
141196

142197
/// Retrieve a key. Semantics are identical to `HashMap::get`.
143-
pub fn get(&self, identifier: &String) -> Option<&Vec<u8>> {
198+
pub fn get(&self, identifier: &String) -> Option<&(Algorithm, Vec<u8>)> {
144199
self.ring.get(identifier)
145200
}
146201

@@ -150,18 +205,19 @@ impl KeyRing {
150205
///
151206
/// Unsupported keys will not be imported, as will keys that failed to
152207
/// be inserted
153-
pub fn try_import_jwk(&mut self, jwk: &Thumbprintable) -> Result<(), ImportError> {
208+
pub fn try_import_jwk(&mut self, jwk: &Thumbprintable) -> Result<(), KeyringError> {
154209
let thumbprint = jwk.b64_thumbprint();
155210
let public_key = jwk.public_key()?;
156-
if !self.import_raw(thumbprint, public_key) {
157-
return Err(ImportError::KeyAlreadyExists);
211+
let algorithm = jwk.algorithm()?;
212+
if !self.import_raw(thumbprint, algorithm, public_key) {
213+
return Err(KeyringError::KeyAlreadyExists);
158214
}
159215
Ok(())
160216
}
161217

162218
/// Import a JSON Web Key Set on a best-effort basis. This method returns a vector indicating
163219
/// whether or not the corresponding key in the key set could be imported.
164-
pub fn import_jwks(&mut self, jwks: JSONWebKeySet) -> Vec<Option<ImportError>> {
220+
pub fn import_jwks(&mut self, jwks: JSONWebKeySet) -> Vec<Option<KeyringError>> {
165221
jwks.keys
166222
.iter()
167223
.map(|jwk| self.try_import_jwk(jwk).err())

‎crates/web-bot-auth/src/lib.rs‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,10 @@ pub mod keyring;
2626
pub mod message_signatures;
2727

2828
use components::CoveredComponent;
29-
use message_signatures::{
30-
Algorithm, MessageVerifier, ParameterDetails, SignatureTiming, SignedMessage,
31-
};
29+
use message_signatures::{MessageVerifier, ParameterDetails, SignatureTiming, SignedMessage};
3230

3331
use data_url::DataUrl;
34-
use keyring::{JSONWebKeySet, KeyRing};
32+
use keyring::{Algorithm, JSONWebKeySet, KeyRing};
3533
use std::time::SystemTimeError;
3634

3735
/// Errors that may be thrown by this module.
@@ -54,7 +52,7 @@ pub enum ImplementationError {
5452
/// that isn't currently supported by this implementation. The subset
5553
/// of [registered IANA signature algorithms](https://www.iana.org/assignments/http-message-signature/http-message-signature.xhtml)
5654
/// implemented here is provided by `Algorithms` struct.
57-
UnsupportedAlgorithm,
55+
UnsupportedAlgorithm(Algorithm),
5856
/// An attempt to resolve key identifier to a valid public key failed.
5957
/// This prevents message verification.
6058
NoSuchKey,
@@ -129,13 +127,10 @@ impl WebBotAuthVerifier {
129127
/// # Errors
130128
///
131129
/// Returns `ImplementationErrors` relevant to verifying and parsing.
132-
pub fn parse(
133-
message: &impl WebBotAuthSignedMessage,
134-
algorithm: Option<Algorithm>,
135-
) -> Result<Self, ImplementationError> {
130+
pub fn parse(message: &impl WebBotAuthSignedMessage) -> Result<Self, ImplementationError> {
136131
let signature_agents = message.fetch_all_signature_agents();
137132
let web_bot_auth_verifier = Self {
138-
message_verifier: MessageVerifier::parse(message, algorithm, |(_, innerlist)| {
133+
message_verifier: MessageVerifier::parse(message, |(_, innerlist)| {
139134
innerlist.params.contains_key("keyid")
140135
&& innerlist.params.contains_key("tag")
141136
&& innerlist.params.contains_key("expires")
@@ -219,7 +214,7 @@ impl WebBotAuthVerifier {
219214

220215
/// Retrieve the parsed `ParameterDetails` from the message. Useful for logging
221216
/// information about the message.
222-
pub fn get_details(&self) -> ParameterDetails {
217+
pub fn get_details(&self) -> &ParameterDetails {
223218
self.message_verifier.get_details()
224219
}
225220
}
@@ -268,9 +263,10 @@ mod tests {
268263
let mut keyring = KeyRing::default();
269264
keyring.import_raw(
270265
"poqkLGiymh_W0uP6PZFw-dvez3QJT5SolqXBCW38r0U".to_string(),
266+
Algorithm::Ed25519,
271267
public_key.to_vec(),
272268
);
273-
let verifier = WebBotAuthVerifier::parse(&test, None).unwrap();
269+
let verifier = WebBotAuthVerifier::parse(&test).unwrap();
274270
let advisory = verifier.get_details().possibly_insecure(|_| false);
275271
// Since the expiry date is in the past.
276272
assert!(advisory.is_expired.unwrap_or(true));
@@ -343,11 +339,11 @@ mod tests {
343339
let mut keyring = KeyRing::default();
344340
keyring.import_raw(
345341
"poqkLGiymh_W0uP6PZFw-dvez3QJT5SolqXBCW38r0U".to_string(),
342+
Algorithm::Ed25519,
346343
public_key.to_vec(),
347344
);
348345

349346
let signer = message_signatures::MessageSigner {
350-
algorithm: Algorithm::Ed25519,
351347
keyid: "poqkLGiymh_W0uP6PZFw-dvez3QJT5SolqXBCW38r0U".into(),
352348
nonce: "end-to-end-test".into(),
353349
tag: "web-bot-auth".into(),
@@ -362,11 +358,12 @@ mod tests {
362358
.generate_signature_headers_content(
363359
&mut mytest,
364360
std::time::Duration::from_secs(10),
361+
Algorithm::Ed25519,
365362
&private_key.to_vec(),
366363
)
367364
.unwrap();
368365

369-
let verifier = WebBotAuthVerifier::parse(&mytest, None).unwrap();
366+
let verifier = WebBotAuthVerifier::parse(&mytest).unwrap();
370367
let advisory = verifier.get_details().possibly_insecure(|_| false);
371368
assert!(!advisory.is_expired.unwrap_or(true));
372369
assert!(!advisory.nonce_is_invalid.unwrap_or(true));
@@ -406,7 +403,7 @@ mod tests {
406403
}
407404

408405
let test = MissingParametersTestVector {};
409-
WebBotAuthVerifier::parse(&test, None).expect_err("This should not have parsed");
406+
WebBotAuthVerifier::parse(&test).expect_err("This should not have parsed");
410407
}
411408

412409
#[test]
@@ -437,7 +434,7 @@ mod tests {
437434
}
438435

439436
let test = MissingParametersTestVector {};
440-
WebBotAuthVerifier::parse(&test, None).expect_err("This should not have parsed");
437+
WebBotAuthVerifier::parse(&test).expect_err("This should not have parsed");
441438
}
442439

443440
#[test]
@@ -481,11 +478,12 @@ mod tests {
481478
let mut keyring = KeyRing::default();
482479
keyring.import_raw(
483480
"poqkLGiymh_W0uP6PZFw-dvez3QJT5SolqXBCW38r0U".to_string(),
481+
Algorithm::Ed25519,
484482
public_key.to_vec(),
485483
);
486484

487485
let test = StandardTestVector {};
488-
let verifier = WebBotAuthVerifier::parse(&test, None).unwrap();
486+
let verifier = WebBotAuthVerifier::parse(&test).unwrap();
489487
let timing = verifier.verify(&keyring, None).unwrap();
490488
assert!(timing.generation.as_nanos() > 0);
491489
assert!(timing.verification.as_nanos() > 0);

0 commit comments

Comments
 (0)