Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion httpsig/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@ rust-version.workspace = true
[features]
default = []
rsa-signature = ["rsa"]
tracing = ["dep:tracing"]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log output of existing users should not silently disappear, so the current behavior must be preserved.

[features]
default = ["tracing"]
tracing = ["dep:tracing"]


[dependencies]
thiserror = { version = "2.0.18" }
tracing = { version = "0.1.44" }
tracing = { version = "0.1.44", optional = true }
rustc-hash = { version = "2.1.1" }
indexmap = { version = "2.11.4" }
rand = { version = "0.10.0" }
Expand Down
20 changes: 16 additions & 4 deletions httpsig/src/crypto/asymmetric.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
use super::AlgorithmName;
use crate::{
error::{HttpSigError, HttpSigResult},
trace::*,
};
use crate::error::{HttpSigError, HttpSigResult};
use ecdsa::{
elliptic_curve::{PublicKey as EcPublicKey, SecretKey as EcSecretKey, sec1::ToEncodedPoint},
signature::{DigestSigner, DigestVerifier},
Expand All @@ -13,6 +10,8 @@ use p384::NistP384;
use pkcs8::{Document, PrivateKeyInfo, der::Decode};
use sha2::{Digest, Sha256, Sha384};
use spki::SubjectPublicKeyInfoRef;
#[cfg(feature = "tracing")]
use tracing::debug;

#[cfg(feature = "rsa-signature")]
use rsa::{
Expand Down Expand Up @@ -65,16 +64,19 @@ impl SecretKey {
pub fn from_bytes(alg: &AlgorithmName, bytes: &[u8]) -> HttpSigResult<Self> {
match alg {
AlgorithmName::EcdsaP256Sha256 => {
#[cfg(feature = "tracing")]
debug!("Read P256 private key");
let sk = EcSecretKey::from_bytes(bytes.into()).map_err(|e| HttpSigError::ParsePrivateKeyError(e.to_string()))?;
Ok(Self::EcdsaP256Sha256(sk))
}
AlgorithmName::EcdsaP384Sha384 => {
#[cfg(feature = "tracing")]
debug!("Read P384 private key");
let sk = EcSecretKey::from_bytes(bytes.into()).map_err(|e| HttpSigError::ParsePrivateKeyError(e.to_string()))?;
Ok(Self::EcdsaP384Sha384(sk))
}
AlgorithmName::Ed25519 => {
#[cfg(feature = "tracing")]
debug!("Read Ed25519 private key");
let mut seed = [0u8; 32];
seed.copy_from_slice(bytes);
Comment on lines 78 to 82
Expand Down Expand Up @@ -176,6 +178,7 @@ impl super::SigningKey for SecretKey {
fn sign(&self, data: &[u8]) -> HttpSigResult<Vec<u8>> {
match &self {
Self::EcdsaP256Sha256(sk) => {
#[cfg(feature = "tracing")]
debug!("Sign EcdsaP256Sha256");
let sk = ecdsa::SigningKey::from(sk);
let mut digest = <Sha256 as Digest>::new();
Expand All @@ -184,6 +187,7 @@ impl super::SigningKey for SecretKey {
Ok(sig.to_bytes().to_vec())
}
Self::EcdsaP384Sha384(sk) => {
#[cfg(feature = "tracing")]
debug!("Sign EcdsaP384Sha384");
let sk = ecdsa::SigningKey::from(sk);
let mut digest = <Sha384 as Digest>::new();
Expand All @@ -192,6 +196,7 @@ impl super::SigningKey for SecretKey {
Ok(sig.to_bytes().to_vec())
}
Self::Ed25519(sk) => {
#[cfg(feature = "tracing")]
debug!("Sign Ed25519");
let sig = sk.sign(data, Some(ed25519_compact::Noise::default()));
Ok(sig.as_ref().to_vec())
Comment on lines 198 to 202
Expand Down Expand Up @@ -260,16 +265,19 @@ impl PublicKey {
pub fn from_bytes(alg: &AlgorithmName, bytes: &[u8]) -> HttpSigResult<Self> {
match alg {
AlgorithmName::EcdsaP256Sha256 => {
#[cfg(feature = "tracing")]
debug!("Read P256 public key");
let pk = EcPublicKey::from_sec1_bytes(bytes).map_err(|e| HttpSigError::ParsePublicKeyError(e.to_string()))?;
Ok(Self::EcdsaP256Sha256(pk))
}
AlgorithmName::EcdsaP384Sha384 => {
#[cfg(feature = "tracing")]
debug!("Read P384 public key");
let pk = EcPublicKey::from_sec1_bytes(bytes).map_err(|e| HttpSigError::ParsePublicKeyError(e.to_string()))?;
Ok(Self::EcdsaP384Sha384(pk))
}
AlgorithmName::Ed25519 => {
#[cfg(feature = "tracing")]
debug!("Read Ed25519 public key");
let pk = ed25519_compact::PublicKey::from_slice(bytes).map_err(|e| HttpSigError::ParsePublicKeyError(e.to_string()))?;
Ok(Self::Ed25519(pk))
Comment on lines 279 to 283
Expand Down Expand Up @@ -358,6 +366,7 @@ impl super::VerifyingKey for PublicKey {
fn verify(&self, data: &[u8], signature: &[u8]) -> HttpSigResult<()> {
match self {
Self::EcdsaP256Sha256(pk) => {
#[cfg(feature = "tracing")]
debug!("Verify EcdsaP256Sha256");
let signature = ecdsa::Signature::<NistP256>::from_bytes(signature.into())
.map_err(|e| HttpSigError::ParseSignatureError(e.to_string()))?;
Expand All @@ -368,6 +377,7 @@ impl super::VerifyingKey for PublicKey {
.map_err(|e| HttpSigError::InvalidSignature(e.to_string()))
}
Self::EcdsaP384Sha384(pk) => {
#[cfg(feature = "tracing")]
debug!("Verify EcdsaP384Sha384");
let signature = ecdsa::Signature::<NistP384>::from_bytes(signature.into())
.map_err(|e| HttpSigError::ParseSignatureError(e.to_string()))?;
Expand All @@ -378,6 +388,7 @@ impl super::VerifyingKey for PublicKey {
.map_err(|e| HttpSigError::InvalidSignature(e.to_string()))
}
Self::Ed25519(pk) => {
#[cfg(feature = "tracing")]
debug!("Verify Ed25519");
let sig =
ed25519_compact::Signature::from_slice(signature).map_err(|e| HttpSigError::ParseSignatureError(e.to_string()))?;
Expand All @@ -386,6 +397,7 @@ impl super::VerifyingKey for PublicKey {
}
#[cfg(feature = "rsa-signature")]
Self::RsaV1_5Sha256(pk) => {
#[cfg(feature = "tracing")]
debug!("Verify RsaV1_5Sha256");
let sig = pkcs1v15::Signature::try_from(signature).map_err(|e| HttpSigError::ParseSignatureError(e.to_string()))?;
pk.verify(data, &sig)
Comment on lines 398 to 403

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg. it must be unacceptable.

Expand Down
10 changes: 6 additions & 4 deletions httpsig/src/crypto/symmetric.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
use super::AlgorithmName;
use crate::{
error::{HttpSigError, HttpSigResult},
trace::*,
};
use crate::error::{HttpSigError, HttpSigResult};
use base64::{Engine as _, engine::general_purpose};
use hmac::{Hmac, Mac};
use sha2::{Digest, Sha256};
#[cfg(feature = "tracing")]
use tracing::debug;

type HmacSha256 = Hmac<sha2::Sha256>;

Expand All @@ -21,6 +20,7 @@ pub enum SharedKey {
impl SharedKey {
/// Create a new shared key from base64 encoded string
pub fn from_base64(alg: &AlgorithmName, key: &str) -> HttpSigResult<Self> {
#[cfg(feature = "tracing")]
debug!("Create SharedKey from base64 string");
let key = general_purpose::STANDARD.decode(key)?;
match alg {
Expand All @@ -38,6 +38,7 @@ impl super::SigningKey for SharedKey {
fn sign(&self, data: &[u8]) -> HttpSigResult<Vec<u8>> {
match self {
SharedKey::HmacSha256(key) => {
#[cfg(feature = "tracing")]
debug!("Sign HmacSha256");
let mut mac = HmacSha256::new_from_slice(key).unwrap();
mac.update(data);
Expand All @@ -61,6 +62,7 @@ impl super::VerifyingKey for SharedKey {
fn verify(&self, data: &[u8], expected_mac: &[u8]) -> HttpSigResult<()> {
match self {
SharedKey::HmacSha256(key) => {
#[cfg(feature = "tracing")]
debug!("Verify HmacSha256");
let mut mac = HmacSha256::new_from_slice(key).unwrap();
mac.update(data);
Expand Down
1 change: 0 additions & 1 deletion httpsig/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ mod error;
mod message_component;
mod signature_base;
mod signature_params;
mod trace;
mod util;

pub mod prelude {
Expand Down
8 changes: 4 additions & 4 deletions httpsig/src/message_component/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,10 @@ use super::{
component_param::{HttpMessageComponentParam, handle_params_key_into, handle_params_sf},
component_value::HttpMessageComponentValue,
};
use crate::{
error::{HttpSigError, HttpSigResult},
trace::*,
};
use crate::error::{HttpSigError, HttpSigResult};

#[cfg(feature = "tracing")]
use tracing::debug;
/* ---------------------------------------------------------------- */
#[derive(Debug, Clone)]
/// Http message component
Expand Down Expand Up @@ -160,6 +159,7 @@ pub(super) fn build_http_field_component(
return Err(HttpSigError::NotYetImplemented("`bs` is not supported yet".to_string()));
}
HttpMessageComponentParam::Req => {
#[cfg(feature = "tracing")]
debug!("`req` is given for http field component");
}
HttpMessageComponentParam::Tr => return Err(HttpSigError::NotYetImplemented("`tr` is not supported yet".to_string())),
Expand Down
5 changes: 4 additions & 1 deletion httpsig/src/signature_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,16 @@ use crate::{
crypto::{AlgorithmName, SigningKey},
error::{HttpSigError, HttpSigResult},
message_component::HttpMessageComponentId,
trace::*,
util::has_unique_elements,
};
use base64::{Engine as _, engine::general_purpose};
use rand::RngExt;
use sfv::{FieldType, ListEntry, Parser};
use std::time::{SystemTime, UNIX_EPOCH};

#[cfg(feature = "tracing")]
use tracing::error;

const DEFAULT_DURATION: u64 = 300;

/* ---------------------------------------- */
Expand Down Expand Up @@ -197,6 +199,7 @@ impl TryFrom<&ListEntry> for HttpSignatureParams {
"keyid" => params.keyid = bare_item.as_string().map(|v| v.to_string()),
"tag" => params.tag = bare_item.as_string().map(|v| v.to_string()),
_ => {
#[cfg(feature = "tracing")]
error!("Ignore invalid signature parameter: {}", key)
}
});
Expand Down
2 changes: 0 additions & 2 deletions httpsig/src/trace.rs

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding #[cfg(feature = "tracing")] to ~20 individual call sites is hard to maintain: every future log line would need to remember the guard, and a single forgotten one breaks the --no-default-features build. Please keep src/trace.rs and make it provide no-op macros when the feature is disabled, so that all call sites stay untouched:

#[cfg(feature = "tracing")]
#[allow(unused)]
pub(crate) use tracing::{debug, error, info, trace, warn};

/// No-op macros used when the `tracing` feature is disabled.
/// They expand to `()` so that they are usable both in statement and
/// expression positions, like the original `tracing` macros.
/// `warn` is defined as `warn_` and re-exported, since a macro named
/// `warn` conflicts with the built-in `warn` attribute (E0659).
#[cfg(not(feature = "tracing"))]
mod noop {
  #![allow(unused_macros)]
  macro_rules! debug { ($($arg:tt)*) => { () }; }
  macro_rules! error { ($($arg:tt)*) => { () }; }
  macro_rules! info { ($($arg:tt)*) => { () }; }
  macro_rules! trace { ($($arg:tt)*) => { () }; }
  macro_rules! warn_ { ($($arg:tt)*) => { () }; }
  #[allow(unused_imports)]
  pub(crate) use {debug, error, info, trace, warn_ as warn};
}

#[cfg(not(feature = "tracing"))]
#[allow(unused)]
pub(crate) use noop::*;

I have verified locally that this compiles with the feature both on and off. Two gotchas encoded in the snippet:

  • a macro_rules! macro named warn cannot be re-exported because it collides with the built-in warn attribute (E0659), hence the warn_ definition re-exported under the name warn;
  • the no-op macros must expand to () rather than to nothing, because some call sites use them in expression position (e.g. the _ => { error!(...) } arm in signature_params.rs).

With this approach the diff shrinks to Cargo.toml and trace.rs only. Please revert the changes to the other five files.

This file was deleted.

Loading