[pdm-devel] [PATCH proxmox v3 07/21] auth-api: add endpoint for issuing tickets as HttpOnly tickets

[Shannon Sterz <s.sterz@proxmox.com>]

this adds a new endpoint for requesting tickets. instead of returning
the ticket in the responses body, the ticket is set as a HttpOnly
cookie. this has a couple of advantages:

- the cookie cannot be stolen if an attacker downgrades the connection
  to http and injects malicious javascript (`HttpOnly`)
- we don't need to rely on the client to make sure that the cookie is
  only send in the appropriate context and only over https
  connections (`Secure`, `SameSite`).
- the cookie cannot be overwritten by other subdomains, insecure
  connections etc. (the default is to prefix them with `__Host-`)

this follows the best practice guide for secure cookies from MDN
[1]. we also set the cookies to expire when the ticket would so that
the browser removes the cookie once the ticket isn't valid anymore.

the endpoint still returns a ticket that only contains the
informational portions of the ticket but not a valid signature. this
is helpful to let clients know when to refresh the ticket by querying
this endpoint again. it still protects the cookie, though, as it
isn't a valid ticket by itself.

[1]: https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/Cookies

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 proxmox-auth-api/Cargo.toml        |   4 +
 proxmox-auth-api/src/api/access.rs | 168 ++++++++++++++++++++++++++++-
 proxmox-auth-api/src/api/mod.rs    |   5 +-
 proxmox-auth-api/src/ticket.rs     |   5 +
 4 files changed, 177 insertions(+), 5 deletions(-)

diff --git a/proxmox-auth-api/Cargo.toml b/proxmox-auth-api/Cargo.toml
index 49398775..848c947c 100644
--- a/proxmox-auth-api/Cargo.toml
+++ b/proxmox-auth-api/Cargo.toml
@@ -22,6 +22,7 @@ base64 = { workspace = true, optional = true }
 libc = { workspace = true, optional = true }
 log = { workspace = true, optional = true }
 http = { workspace = true, optional = true }
+hyper = { workspace = true, optional = true }
 nix = { workspace = true, optional = true }
 openssl = { workspace = true, optional = true }
 pam-sys = { workspace = true, optional = true }
@@ -37,6 +38,7 @@ proxmox-router = { workspace = true, optional = true }
 proxmox-schema = { workspace = true, optional = true, features = [ "api-macro", "api-types" ] }
 proxmox-sys = { workspace = true, optional = true }
 proxmox-tfa = { workspace = true, optional = true, features = [ "api" ] }
+proxmox-time = { workspace = true, optional = true }
 
 [features]
 default = []
@@ -48,11 +50,13 @@ api = [
     "ticket",
 
     "dep:http",
+    "dep:hyper",
     "dep:serde_json",
 
     "dep:proxmox-rest-server",
     "dep:proxmox-router",
     "dep:proxmox-tfa",
+    "dep:proxmox-time",
 ]
 pam-authenticator = [ "api", "dep:libc", "dep:log", "dep:pam-sys" ]
 password-authenticator = [
diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
index f7d52e95..273d7701 100644
--- a/proxmox-auth-api/src/api/access.rs
+++ b/proxmox-auth-api/src/api/access.rs
@@ -1,18 +1,25 @@
 //! Provides the "/access/ticket" API call.
 
 use anyhow::{bail, format_err, Error};
+use http::request::Parts;
+use http::Response;
+use hyper::Body;
 use openssl::hash::MessageDigest;
 use serde_json::{json, Value};
 
-use proxmox_rest_server::RestEnvironment;
-use proxmox_router::{http_err, Permission, RpcEnvironment};
-use proxmox_schema::{api, api_types::PASSWORD_SCHEMA};
+use proxmox_rest_server::{extract_cookie, RestEnvironment};
+use proxmox_router::{
+    http_err, ApiHandler, ApiMethod, ApiResponseFuture, Permission, RpcEnvironment,
+};
+use proxmox_schema::{
+    api, api_types::PASSWORD_SCHEMA, AllOfSchema, ApiType, ParameterSchema, ReturnType,
+};
 use proxmox_tfa::api::TfaChallenge;
 
 use super::ApiTicket;
 use super::{auth_context, HMACKey};
 use crate::ticket::Ticket;
-use crate::types::{Authid, Userid};
+use crate::types::{Authid, CreateTicket, CreateTicketResponse, Userid};
 
 #[allow(clippy::large_enum_variant)]
 enum AuthResult {
@@ -132,6 +139,159 @@ pub async fn create_ticket(
     }
 }
 
+
+pub const API_METHOD_CREATE_TICKET_HTTP_ONLY: ApiMethod = ApiMethod::new_full(
+    &ApiHandler::AsyncHttpBodyParameters(&create_ticket_http_only),
+    ParameterSchema::AllOf(&AllOfSchema::new(
+        "Get a new ticket as an HttpOnly cookie. Supports tickets via cookies.",
+        &[&CreateTicket::API_SCHEMA],
+    )),
+)
+.returns(ReturnType::new(false, &CreateTicketResponse::API_SCHEMA))
+.protected(true)
+.access(None, &Permission::World);
+
+fn create_ticket_http_only(
+    parts: Parts,
+    param: Value,
+    _info: &ApiMethod,
+    rpcenv: Box<dyn RpcEnvironment>,
+) -> ApiResponseFuture {
+    Box::pin(async move {
+        let auth_context = auth_context()?;
+        let host_cookie = auth_context.prefixed_auth_cookie_name();
+        let mut create_params: CreateTicket = serde_json::from_value(param)?;
+
+        // previously to refresh a ticket, the old ticket was provided as a password via this
+        // endpoint's parameters. however, once the ticket is set as an HttpOnly cookie, some
+        // clients won't have access to it anymore. so we need to check whether the ticket is set
+        // in a cookie here too.
+        //
+        // only check the newer `__Host-` prefixed cookies here as older tickets should be
+        // provided via the password parameter anyway.
+        create_params.password = parts
+            .headers
+            // there is a `cookie_from_header` function we could use, but it seems to fail when
+            // multiple cookie headers are set
+            .get_all(http::header::COOKIE)
+            .iter()
+            .filter_map(|c| c.to_str().ok())
+            // after this only `__Host-{Cookie Name}` cookies are in the iterator
+            .filter_map(|c| extract_cookie(c, host_cookie))
+            // so this should just give us the first one if it exists
+            .next()
+            // if not use the parameter
+            .or(create_params.password);
+
+        let env: &RestEnvironment = rpcenv
+            .as_any()
+            .downcast_ref::<RestEnvironment>()
+            .ok_or(format_err!("detected wrong RpcEnvironment type"))?;
+
+        let mut ticket_response = handle_ticket_creation(create_params, env).await?;
+
+        // if `ticket_info` is set, we want to return the ticket in a `SET_COOKIE` header and not
+        // the response body
+        if ticket_response.ticket_info.is_some() {
+            // parse the ticket here, so we can use the correct timestamp of the `Expire` parameter
+            if let Some(ref ticket) = ticket_response.ticket.take() {
+                let ticket = Ticket::<ApiTicket>::parse(ticket)?;
+
+                // see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#expiresdate
+                // see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date
+                // see: https://developer.mozilla.org/en-US/docs/Web/Security/Practical_implementation_guides/Cookies#expires
+                let expire =
+                    proxmox_time::epoch_to_http_date(ticket.time() + crate::TICKET_LIFETIME)?;
+
+                // this makes sure that ticket cookies:
+                // - Typically `__Host-`-prefixed: are only send to the specific domain that set
+                //   them and that scripts served via http cannot overwrite the cookie.
+                // - `Expires`: expire at the same time as the encoded timestamp in the ticket.
+                // - `Secure`: are only sent via https.
+                // - `SameSite=Lax`: are only sent on cross-site requests when the user is
+                //   navigating to the origin site from an external site.
+                // - `HttpOnly`: cookies are not readable to client-side javascript code.
+                let cookie = format!(
+                    "{host_cookie}={}; Expires={}; Secure; SameSite=Lax; HttpOnly; Path=/;",
+                    // the unwrap here is safe, as we already parse the ticket above, it must be set
+                    ticket_response.ticket.unwrap(),
+                    expire,
+                );
+
+                return Ok(Response::builder()
+                    .header(hyper::header::SET_COOKIE, cookie)
+                    // unset the ticket here explicitly, so that JavaScript clients can't access it
+                    .body(Body::from(
+                        json!({"data": CreateTicketResponse { ticket: None, .. ticket_response} })
+                            .to_string(),
+                    ))?);
+            }
+        }
+
+        Ok(Response::builder().body(Body::from(json!({"data": ticket_response }).to_string()))?)
+    })
+}
+
+async fn handle_ticket_creation(
+    create_params: CreateTicket,
+    env: &RestEnvironment,
+) -> Result<CreateTicketResponse, Error> {
+    let username = create_params.username;
+    let password = create_params
+        .password
+        .ok_or(format_err!("no password provided"))?;
+
+    match authenticate_user(
+        &username,
+        &password,
+        create_params.path,
+        create_params.privs,
+        create_params.port,
+        create_params.tfa_challenge,
+        env,
+    )
+    .await
+    {
+        Ok(AuthResult::Success) => Ok(CreateTicketResponse {
+            username,
+            ..Default::default()
+        }),
+        Ok(AuthResult::CreateTicket) => {
+            let auth_context = auth_context()?;
+            let api_ticket = ApiTicket::Full(username.clone());
+            let mut ticket = Ticket::new(auth_context.auth_prefix(), &api_ticket)?;
+            let csrfprevention_token =
+                assemble_csrf_prevention_token(auth_context.csrf_secret(), &username);
+
+            env.log_auth(username.as_str());
+
+            Ok(CreateTicketResponse {
+                username,
+                ticket: Some(ticket.sign(auth_context.keyring(), None)?),
+                ticket_info: Some(ticket.ticket_info()),
+                csrfprevention_token: Some(csrfprevention_token),
+            })
+        }
+        Ok(AuthResult::Partial(challenge)) => {
+            let auth_context = auth_context()?;
+            let api_ticket = ApiTicket::Partial(challenge);
+            let ticket = Ticket::new(auth_context.auth_prefix(), &api_ticket)?
+                .sign(auth_context.keyring(), Some(username.as_str()))?;
+
+            Ok(CreateTicketResponse {
+                username,
+                ticket: Some(ticket),
+                csrfprevention_token: Some("invalid".to_string()),
+                ..Default::default()
+            })
+        }
+        Err(err) => {
+            env.log_failed_auth(Some(username.to_string()), &err.to_string());
+            Err(http_err!(UNAUTHORIZED, "permission check failed."))
+        }
+    }
+}
+
 async fn authenticate_user(
     userid: &Userid,
     password: &str,
diff --git a/proxmox-auth-api/src/api/mod.rs b/proxmox-auth-api/src/api/mod.rs
index 32d18299..3ee2d0e1 100644
--- a/proxmox-auth-api/src/api/mod.rs
+++ b/proxmox-auth-api/src/api/mod.rs
@@ -18,7 +18,10 @@ mod ticket;
 use crate::ticket::Ticket;
 use access::verify_csrf_prevention_token;
 
-pub use access::{assemble_csrf_prevention_token, create_ticket, API_METHOD_CREATE_TICKET};
+pub use access::{
+    assemble_csrf_prevention_token, create_ticket, API_METHOD_CREATE_TICKET,
+    API_METHOD_CREATE_TICKET_HTTP_ONLY,
+};
 pub use ticket::{ApiTicket, PartialTicket};
 
 /// Authentication realms are used to manage users: authenticate, change password or remove.
diff --git a/proxmox-auth-api/src/ticket.rs b/proxmox-auth-api/src/ticket.rs
index 498e9385..59293492 100644
--- a/proxmox-auth-api/src/ticket.rs
+++ b/proxmox-auth-api/src/ticket.rs
@@ -227,6 +227,11 @@ where
             _type_marker: PhantomData,
         })
     }
+
+    pub fn ticket_info(&self) -> String {
+        // append a `::ticketinfo` signature to distinguish the ticket info from proper tickets
+        format!("{}::ticketinfo", self.ticket_data())
+    }
 }
 
 #[cfg(test)]
-- 
2.39.5



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel