Skip to content

Commit

Permalink
Merge pull request #142 from sebadob/dpop-fixes
Browse files Browse the repository at this point in the history 
DPoP fixes and improvements
  • Loading branch information
@sebadob
sebadob authored Nov 3, 2023
2 parents 5965d9a + 4736400 commit 4329303
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 23 deletions.
8 changes: 6 additions & 2 deletions rauthy-common/src/constants.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,14 @@ lazy_static! {
pub static ref DATABASE_URL: String = env::var("DATABASE_URL").expect("DATABASE_URL is not set");
pub static ref DB_TYPE: DbType = DbType::from_str(&DATABASE_URL).unwrap();
pub static ref ROLE_ADMIN: String = "rauthy_admin".to_string();
// pub static ref ROLE_ADMIN: String = "ROLE_rauthy_admin".to_string();
pub static ref DEV_MODE: bool = env::var("DEV_MODE")
.unwrap_or_else(|_| String::from("false"))
.parse::<bool>()
.expect("DEV_MODE cannot be parsed to bool - bad format");
pub static ref DEV_DPOP_HTTP: bool = env::var("DEV_DPOP_HTTP")
.unwrap_or_else(|_| String::from("false"))
.parse::<bool>()
.expect("DEV_DPOP_HTTP cannot be parsed to bool - bad format");
pub static ref HA_MODE: bool =
env::var("HA_MODE").map(|s| s.to_lowercase() == "true").unwrap_or(false);

Expand Down Expand Up @@ -92,7 +95,8 @@ lazy_static! {

pub static ref DPOP_TOKEN_ENDPOINT: Uri = {
let pub_url = env::var("PUB_URL").expect("PUB_URL env var is not set");
let uri = format!("https://{}/auth/v1/oidc/token", pub_url);
let scheme = if *DEV_MODE && *DEV_DPOP_HTTP { "http" } else { "https" };
let uri = format!("{}://{}/auth/v1/oidc/token", scheme, pub_url);
Uri::from_str(&uri).unwrap()
};

Expand Down
25 changes: 22 additions & 3 deletions rauthy-common/src/error_response.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::constants::{APPLICATION_JSON, HEADER_HTML, HEADER_RETRY_NOT_BEFORE};
use crate::utils::build_csp_header;
use actix_multipart::MultipartError;
use actix_web::error::BlockingError;
use actix_web::http::header::ToStrError;
use actix_web::http::header::{ToStrError, WWW_AUTHENTICATE};
use actix_web::http::StatusCode;
use actix_web::{HttpResponse, HttpResponseBuilder, ResponseError};
use css_color::ParseColorError;
Expand All @@ -23,6 +23,7 @@ pub enum ErrorResponseType {
Database,
DatabaseIo,
Disabled,
DPoP,
Forbidden,
Internal,
JoseError,
Expand Down Expand Up @@ -75,6 +76,7 @@ impl ResponseError for ErrorResponse {
ErrorResponseType::NotFound => StatusCode::NOT_FOUND,
ErrorResponseType::Disabled
| ErrorResponseType::CSRFTokenError
| ErrorResponseType::DPoP
| ErrorResponseType::PasswordExpired
| ErrorResponseType::SessionExpired
| ErrorResponseType::SessionTimeout
Expand All @@ -87,18 +89,35 @@ impl ResponseError for ErrorResponse {
}

fn error_response(&self) -> HttpResponse {
let status = self.status_code();

match self.error {
ErrorResponseType::TooManyRequests(not_before_timestamp) => {
HttpResponseBuilder::new(self.status_code())
.append_header((HEADER_RETRY_NOT_BEFORE, not_before_timestamp))
.append_header(HEADER_HTML)
// TODO we could possibly to a small `unsafe` call here to just take
// TODO we could possibly do a small `unsafe` call here to just take
// the content without cloning it -> more efficient, especially for blocked IPs
.body(self.message.clone())
}
_ => HttpResponseBuilder::new(self.status_code())

ErrorResponseType::DPoP => HttpResponseBuilder::new(self.status_code())
.append_header((WWW_AUTHENTICATE, "DPoP error=invalid_dpop_proof"))
.content_type(APPLICATION_JSON)
.body(serde_json::to_string(&self).unwrap()),

_ => {
if status == StatusCode::UNAUTHORIZED {
HttpResponseBuilder::new(self.status_code())
.append_header((WWW_AUTHENTICATE, "OAuth"))
.content_type(APPLICATION_JSON)
.body(serde_json::to_string(&self).unwrap())
} else {
HttpResponseBuilder::new(self.status_code())
.content_type(APPLICATION_JSON)
.body(serde_json::to_string(&self).unwrap())
}
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion rauthy-handlers/src/oidc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ pub async fn get_session_xsrf(
post,
path = "/oidc/token",
tag = "oidc",
request_body = TokenRequest,
request_body(content = TokenRequest, content_type = "application/x-www-form-urlencoded"),
responses(
(status = 200, description = "Ok", body = TokenSet),
(status = 400, description = "BadRequest", body = ErrorResponse),
Expand Down
34 changes: 17 additions & 17 deletions rauthy-models/src/entity/dpop_proof.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,15 @@ pub struct DPoPClaims {
pub htu: Uri,
/// Creation timestamp of the JWT (Section 4.1.6 of [RFC7519]).
pub iat: i64,

// // TODO does Rauthy even need 'ath'? maybe when refreshing a token?
// /// Hash of the access token. The value MUST be the result of a
// /// base64url encoding (as defined in Section 2 of [RFC7515])
// /// the SHA-256 [SHS] hash of the ASCII encoding of the associated
// /// access token's value.
// ///
// /// MUST be valid when used in conjunction with an access token
// The 'ath' claim does not apply to Rauthy, only used by resource servers.
// It is just here for completeness.
//
// Hash of the access token. The value MUST be the result of a
// base64url encoding (as defined in Section 2 of [RFC7515])
// the SHA-256 [SHS] hash of the ASCII encoding of the associated
// access token's value.
//
// MUST be valid when used in conjunction with an access token
// #[serde(skip_serializing_if = "Option::is_none")]
// pub ath: Option<String>,

Expand All @@ -78,7 +79,7 @@ impl TryFrom<&str> for DPoPProof {
fn try_from(value: &str) -> Result<Self, Self::Error> {
match value.split_once('.') {
None => Err(ErrorResponse::new(
ErrorResponseType::BadRequest,
ErrorResponseType::DPoP,
"Invalid DPoP header format".to_string(),
)),

Expand All @@ -97,7 +98,7 @@ impl TryFrom<&str> for DPoPProof {

match rest.split_once('.') {
None => Err(ErrorResponse::new(
ErrorResponseType::BadRequest,
ErrorResponseType::DPoP,
"Invalid DPoP claims format".to_string(),
)),
Some((claims, signature)) => {
Expand Down Expand Up @@ -134,7 +135,7 @@ impl DPoPProof {
let b64 = v.to_str()?;
if !RE_TOKEN_68.is_match(b64) {
Err(ErrorResponse::new(
ErrorResponseType::BadRequest,
ErrorResponseType::DPoP,
"DPoP header must be in Token68 format".to_string(),
))
} else {
Expand Down Expand Up @@ -187,7 +188,7 @@ impl DPoPProof {
// 4. The typ JOSE Header Parameter has the value dpop+jwt.The alg JOSE Header
if &self.header.typ != "dpop+jwt" {
return Err(ErrorResponse::new(
ErrorResponseType::BadRequest,
ErrorResponseType::DPoP,
"Expected header typ of 'dpop+jwt'".to_string(),
));
}
Expand All @@ -209,7 +210,7 @@ impl DPoPProof {
// 8. The htm claim matches the HTTP method of the current request.
if self.claims.htm != http::Method::POST {
return Err(ErrorResponse::new(
ErrorResponseType::BadRequest,
ErrorResponseType::DPoP,
"The 'htm' claim from the DPoP header != POST".to_string(),
));
}
Expand All @@ -218,7 +219,7 @@ impl DPoPProof {
// which the JWT was received, ignoring any query and fragment parts.
if self.claims.htu != *DPOP_TOKEN_ENDPOINT {
return Err(ErrorResponse::new(
ErrorResponseType::BadRequest,
ErrorResponseType::DPoP,
"Invalid 'htu' claim".to_string(),
));
}
Expand All @@ -236,7 +237,7 @@ impl DPoPProof {
let now_minus_1 = now - 60;
if self.claims.iat < now_minus_1 || self.claims.iat > now {
return Err(ErrorResponse::new(
ErrorResponseType::BadRequest,
ErrorResponseType::DPoP,
"DPoP 'iat' claim is out of range".to_string(),
));
}
Expand All @@ -263,9 +264,9 @@ mod tests {
// use rsa::sha2::{Digest, Sha256};
// use rsa::signature::SignatureEncoding;
// use rsa::traits::{PublicKeyParts};
use std::fmt::Write;
use ed25519_compact::Noise;
use rauthy_common::constants::DPOP_TOKEN_ENDPOINT;
use std::fmt::Write;

// Note: this test does not work anymore with the way more strict deserialization after
// the initial testing with just Strings.
Expand Down Expand Up @@ -333,7 +334,6 @@ mod tests {
htm: http::Method::POST,
htu: DPOP_TOKEN_ENDPOINT.clone(),
iat: Utc::now().timestamp(),
// ath: None,
// nonce: None,
};

Expand Down
5 changes: 5 additions & 0 deletions rauthy.cfg
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
# !!! DO NOT USE IN PRODUCTION !!!
DEV_MODE=true

# Can be set to 'true' during local development to allow an HTTP scheme for the DPoP 'htu' claim
# Will only be applied if `DEV_MODE == true` as well.
# !!! DO NOT USE IN PRODUCTION !!!
DEV_DPOP_HTTP=true

#####################################
############## ACCESS ###############
#####################################
Expand Down

0 comments on commit 4329303

Please sign in to comment.