Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly include port in host header #23

Merged
merged 3 commits into from
Nov 12, 2024
Merged

Correctly include port in host header #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4fe52cb
Correctly include port in host header
robalar Oct 30, 2024
30c4b66
Remove use of deprecated chrono functions
robalar Oct 30, 2024
066381a
Bump version to 0.6
robalar Oct 31, 2024
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
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]
name = "http-sig"
description = "Implementation of the IETF draft 'Signing HTTP Messages'"
version = "0.5.0"
version = "0.6.0"
authors = ["Jack Cargill <[email protected]>", "Diggory Blake"]
edition = "2018"
readme = "README.md"
Expand Down
2 changes: 0 additions & 2 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,6 @@ pub mod mock_request;

#[cfg(feature = "reqwest")]
mod reqwest_impls;
#[cfg(feature = "reqwest")]
pub use reqwest_impls::*;

#[cfg(feature = "rouille")]
mod rouille_impls;
Expand Down
101 changes: 96 additions & 5 deletions src/reqwest_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,14 @@ use http::header::{HeaderName, HeaderValue};

use super::*;

/// Returns the correct `Host` header value for a given URL, in the form `<host>:<port>`.
fn host_from_url(url: &url::Url) -> Option<String> {
url.host_str().map(|host| match url.port() {
Some(port) => format!("{}:{}", host, port),
None => host.into(),
})
}

impl RequestLike for reqwest::Request {
fn header(&self, header: &Header) -> Option<HeaderValue> {
match header {
Expand All @@ -20,7 +28,7 @@ impl RequestLike for reqwest::Request {

impl ClientRequestLike for reqwest::Request {
fn host(&self) -> Option<String> {
robalar marked this conversation as resolved.
Show resolved Hide resolved
self.url().host_str().map(Into::into)
host_from_url(self.url())
}
fn compute_digest(&mut self, digest: &dyn HttpDigest) -> Option<String> {
self.body()?.as_bytes().map(|b| digest.http_digest(b))
Expand Down Expand Up @@ -52,7 +60,7 @@ impl RequestLike for reqwest::blocking::Request {

impl ClientRequestLike for reqwest::blocking::Request {
fn host(&self) -> Option<String> {
self.url().host_str().map(Into::into)
host_from_url(self.url())
}
fn compute_digest(&mut self, digest: &dyn HttpDigest) -> Option<String> {
let bytes_to_digest = self.body_mut().as_mut()?.buffer().ok()?;
Expand All @@ -66,7 +74,7 @@ impl ClientRequestLike for reqwest::blocking::Request {
#[cfg(test)]
mod tests {
use chrono::{offset::TimeZone, Utc};
use http::header::{AUTHORIZATION, CONTENT_TYPE, DATE};
use http::header::{AUTHORIZATION, CONTENT_TYPE, DATE, HOST};

use super::*;

Expand All @@ -81,8 +89,9 @@ mod tests {
.header(CONTENT_TYPE, "application/json")
.header(
DATE,
Utc.ymd(2014, 7, 8)
.and_hms(9, 10, 11)
Utc.with_ymd_and_hms(2014, 7, 8, 9, 10, 11)
.single()
.expect("valid date")
.format("%a, %d %b %Y %T GMT")
.to_string(),
)
Expand All @@ -100,8 +109,90 @@ mod tests {
.unwrap(),
"SHA-256=2vgEVkfe4d6VW+tSWAziO7BUx7uT/rA9hn1EoxUJi2o="
);
assert_eq!(with_sig.headers().get(HOST).unwrap(), "test.com");
}

#[test]
fn it_works_blocking() {
let config = SigningConfig::new_default("test_key", "abcdefgh".as_bytes());

let client = reqwest::blocking::Client::new();
Copy link
Collaborator

@thomassa thomassa Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It took me a few minutes playing spot-the-difference to see that this line is the only difference between this test and the it_works test: I resorted to pasting the two of them into kdiff3 to display the difference.

Is it worth factoring out the duplicated code?
Something like

    #[test]
    fn it_works_blocking() {
        it_works(reqwest::blocking::Client::new())
    }

    #[test]
    fn it_works_async() {
        it_works(reqwest::Client::new())
    }

or using a parameterisation library such as test-case.

It's the same for the other pair of tests: the ones specifying a non-standard port.

Copy link
Member Author

Choose a reason for hiding this comment

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

they are completely seperate types, and don't implement a shared trait for the builder (as far as i can tell) so I dont think its possible to construct a function that takes both as an argument?


let without_sig = client
.post("http://test.com/foo/bar")
.header(CONTENT_TYPE, "application/json")
.header(
DATE,
Utc.with_ymd_and_hms(2014, 7, 8, 9, 10, 11)
.single()
.expect("valid date")
.format("%a, %d %b %Y %T GMT")
.to_string(),
)
.body(&br#"{ "x": 1, "y": 2}"#[..])
.build()
.unwrap();

let with_sig = without_sig.signed(&config).unwrap();

assert_eq!(with_sig.headers().get(AUTHORIZATION).unwrap(), "Signature keyId=\"test_key\",algorithm=\"hs2019\",signature=\"F8gZiriO7dtKFiP5eSZ+Oh1h61JIrAR6D5Mdh98DjqA=\",headers=\"(request-target) host date digest\"");
assert_eq!(
with_sig
.headers()
.get(HeaderName::from_static("digest"))
.unwrap(),
"SHA-256=2vgEVkfe4d6VW+tSWAziO7BUx7uT/rA9hn1EoxUJi2o="
);
assert_eq!(with_sig.headers().get(HOST).unwrap(), "test.com");
}

#[test]
fn sets_host_header_with_port_correctly() {
let config = SigningConfig::new_default("test_key", "abcdefgh".as_bytes());
let client = reqwest::Client::new();

let without_sig = client
.post("http://localhost:8080/foo/bar")
.header(CONTENT_TYPE, "application/json")
.header(
DATE,
Utc.with_ymd_and_hms(2014, 7, 8, 9, 10, 11)
.single()
.expect("valid date")
.format("%a, %d %b %Y %T GMT")
.to_string(),
)
.body(&br#"{ "x": 1, "y": 2}"#[..])
.build()
.unwrap();

let with_sig = without_sig.signed(&config).unwrap();
assert_eq!(with_sig.headers().get(HOST).unwrap(), "localhost:8080");
}
robalar marked this conversation as resolved.
Show resolved Hide resolved

#[test]
fn sets_host_header_with_port_correctly_blocking() {
let config = SigningConfig::new_default("test_key", "abcdefgh".as_bytes());
let client = reqwest::blocking::Client::new();

let without_sig = client
.post("http://localhost:8080/foo/bar")
.header(CONTENT_TYPE, "application/json")
.header(
DATE,
Utc.with_ymd_and_hms(2014, 7, 8, 9, 10, 11)
.single()
.expect("valid date")
.format("%a, %d %b %Y %T GMT")
.to_string(),
)
.body(&br#"{ "x": 1, "y": 2}"#[..])
.build()
.unwrap();

let with_sig = without_sig.signed(&config).unwrap();
assert_eq!(with_sig.headers().get(HOST).unwrap(), "localhost:8080");
}
#[test]
#[ignore]
fn it_can_talk_to_reference_integration() {
Expand Down
11 changes: 5 additions & 6 deletions src/rouille_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,14 +103,13 @@ mod tests {
vec![
("Host".into(), "test.com".into()),
("ContentType".into(), "application/json".into()),
("Date".into(), Utc.ymd(2014, 7, 8)
.and_hms(9, 10, 11)
("Date".into(), Utc.with_ymd_and_hms(2014, 7, 8, 9, 10, 11).single().expect("valid date")
.format("%a, %d %b %Y %T GMT")
.to_string()),
("Digest".into(), "SHA-256=2vgEVkfe4d6VW+tSWAziO7BUx7uT/rA9hn1EoxUJi2o=".into()),
("Authorization".into(), "Signature keyId=\"test_key\",algorithm=\"hmac-sha256\",signature=\"uH2I9FSuCGUrIEygs7hR29oz0Afkz0bZyHpz6cW/mLQ=\",headers=\"(request-target) date digest host".into()),
],
br#"{ "x": 1, "y": 2}"#[..].into()
br#"{ "x": 1, "y": 2}"#[..].into(),
);

request.verify(&config).unwrap();
Expand All @@ -129,13 +128,13 @@ mod tests {
"GET",
"/foo/bar",
vec![
("Date".into(), Utc.ymd(2014, 7, 8)
.and_hms(9, 10, 11)
("Date".into(), Utc.with_ymd_and_hms(2014, 7, 8,
9, 10, 11).single().expect("valid date")
.format("%a, %d %b %Y %T GMT")
.to_string()),
("Authorization".into(), "Signature keyId=\"test_key\",algorithm=\"hmac-sha256\",signature=\"sGQ3hA9KB40CU1pHbRLXLvLdUWYn+c3fcfL+Sw8kIZE=\",headers=\"(request-target) date".into()),
],
Vec::new()
Vec::new(),
);

request.verify(&config).unwrap();
Expand Down
5 changes: 3 additions & 2 deletions src/signing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ use crate::{DefaultDigestAlgorithm, DefaultSignatureAlgorithm, DATE_FORMAT};
/// HTTP request. The HTTP signing extension methods are available on
/// any type implementing this trait.
pub trait ClientRequestLike: RequestLike {
/// Returns the host for the request (eg. "example.com") in case the Host header has
/// not been set explicitly.
/// Returns the host for the request (eg. "example.com", "127.0.0.1:8080") in case the Host header has
/// not been set explicitly. Note, the correct form of the `Host` header is `<host>:<port>` if
/// the port is non-standard for the protocol used (e.g., 443 for an HTTPS URL, and 80 for an HTTP URL).
/// When implementing this trait, do not just read the `Host` header from the request -
/// this method will only be called when the `Host` header is not set.
fn host(&self) -> Option<String> {
Expand Down
2 changes: 1 addition & 1 deletion src/verifying.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ fn verify_except_digest<T: ServerRequestLike>(
})?;

// Then parse into a datetime
let provided_date = DateTime::<Utc>::from_utc(
let provided_date = DateTime::<Utc>::from_naive_utc_and_offset(
NaiveDateTime::parse_from_str(date_value, DATE_FORMAT)
.ok()
.or_else(|| {
Expand Down
Loading