mart-w/kanidm
1
0
Fork 0
mirror of https://github.com/kanidm/kanidm.git synced 2025-02-23 20:47:01 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity

cookies don't clear unless you set domain (#3332)

Browse source 
* make everything cookie consistent
* Stricter on expiry
* Relearn a painful lesson about needing domains in removal cookies
* fix: DRY cookie creation code and reduce the sins
This commit is contained in:
Firstyear 2025-01-04 10:33:01 +10:00 committed by William Brown
parent a7fabdedef
commit 095df1b216
4 changed files with 93 additions and 97 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

50
server/core/src/https/views/cookies.rs
View file

@ -6,11 +6,29 @@ use compact_jwt::{Jws, JwsSigner};
use serde::de::DeserializeOwned; use serde::de::DeserializeOwned;
use serde::Serialize; use serde::Serialize;
#[instrument(name = "views::cookies::destroy", level = "debug", skip(jar))] fn new_cookie<'a>(state: &'_ ServerState, ck_id: &'a str, value: String) -> Cookie<'a> {
pub fn destroy(jar: CookieJar, ck_id: &str) -> CookieJar { let mut token_cookie = Cookie::new(ck_id, value);
token_cookie.set_secure(state.secure_cookies);
token_cookie.set_same_site(SameSite::Lax);
// Prevent Document.cookie accessing this. Still works with fetch.
token_cookie.set_http_only(true);
// We set a domain here because it allows subdomains
// of the idm to share the cookie. If domain was incorrect
// then webauthn won't work anyway!
token_cookie.set_domain(state.domain.clone());
token_cookie.set_path("/");
token_cookie
}
#[instrument(name = "views::cookies::destroy", level = "debug", skip(jar, state))]
pub fn destroy(jar: CookieJar, ck_id: &str, state: &ServerState) -> CookieJar {
if let Some(ck) = jar.get(ck_id) { if let Some(ck) = jar.get(ck_id) {
let mut removal_cookie = ck.clone(); let mut removal_cookie = ck.clone();
removal_cookie.make_removal(); removal_cookie.make_removal();
// Need to be set to domain else the cookie isn't removed!
removal_cookie.set_domain(state.domain.clone());
// Need to be set to / to remove on all parent paths. // Need to be set to / to remove on all parent paths.
// If you don't set a path, NOTHING IS REMOVED!!! // If you don't set a path, NOTHING IS REMOVED!!!
removal_cookie.set_path("/"); removal_cookie.set_path("/");
@ -21,30 +39,14 @@ pub fn destroy(jar: CookieJar, ck_id: &str) -> CookieJar {
} }
} }
pub fn make_unsigned<'a>( pub fn make_unsigned<'a>(state: &'_ ServerState, ck_id: &'a str, value: String) -> Cookie<'a> {
state: &'_ ServerState, new_cookie(state, ck_id, value)
ck_id: &'a str,
value: String,
path: &'a str,
) -> Cookie<'a> {
let mut token_cookie = Cookie::new(ck_id, value);
token_cookie.set_secure(state.secure_cookies);
token_cookie.set_same_site(SameSite::Lax);
// Prevent Document.cookie accessing this. Still works with fetch.
token_cookie.set_http_only(true);
// We set a domain here because it allows subdomains
// of the idm to share the cookie. If domain was incorrect
// then webauthn won't work anyway!
token_cookie.set_domain(state.domain.clone());
token_cookie.set_path(path);
token_cookie
} }
pub fn make_signed<'a, T: Serialize>( pub fn make_signed<'a, T: Serialize>(
state: &'_ ServerState, state: &'_ ServerState,
ck_id: &'a str, ck_id: &'a str,
value: &'_ T, value: &'_ T,
path: &'a str,
) -> Option<Cookie<'a>> { ) -> Option<Cookie<'a>> {
let kref = &state.jws_signer; let kref = &state.jws_signer;
@ -63,13 +65,7 @@ pub fn make_signed<'a, T: Serialize>(
}) })
.ok()?; .ok()?;
let mut token_cookie = Cookie::new(ck_id, token); Some(new_cookie(state, ck_id, token))
token_cookie.set_secure(state.secure_cookies);
token_cookie.set_same_site(SameSite::Lax);
token_cookie.set_http_only(true);
token_cookie.set_path(path);
token_cookie.set_domain(state.domain.clone());
Some(token_cookie)
} }
pub fn get_signed<T: DeserializeOwned>( pub fn get_signed<T: DeserializeOwned>(

42
server/core/src/https/views/login.rs
View file

@ -177,10 +177,10 @@ pub async fn view_logout_get(
}; };
// Always clear cookies even on an error. // Always clear cookies even on an error.
jar = cookies::destroy(jar, COOKIE_BEARER_TOKEN); jar = cookies::destroy(jar, COOKIE_BEARER_TOKEN, &state);
jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &state);
jar = cookies::destroy(jar, COOKIE_AUTH_SESSION_ID); jar = cookies::destroy(jar, COOKIE_AUTH_SESSION_ID, &state);
jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN); jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN, &state);
(jar, response).into_response() (jar, response).into_response()
} }
@ -195,7 +195,7 @@ pub async fn view_reauth_get(
) -> Response { ) -> Response {
// No matter what, we always clear the stored oauth2 cookie to prevent // No matter what, we always clear the stored oauth2 cookie to prevent
// ui loops // ui loops
let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &state);
let session_valid_result = state let session_valid_result = state
.qe_r_ref .qe_r_ref
@ -322,7 +322,7 @@ pub async fn view_index_get(
// No matter what, we always clear the stored oauth2 cookie to prevent // No matter what, we always clear the stored oauth2 cookie to prevent
// ui loops // ui loops
let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &state);
match session_valid_result { match session_valid_result {
Ok(()) => { Ok(()) => {
@ -915,14 +915,10 @@ async fn view_login_step(
// Update jar // Update jar
let token_str = token.to_string(); let token_str = token.to_string();
// Important - this can be make unsigned as token_str has it's own // Important - this can be make unsigned as token_str has its own
// signatures. // signatures.
let mut bearer_cookie = cookies::make_unsigned( let mut bearer_cookie =
&state, cookies::make_unsigned(&state, COOKIE_BEARER_TOKEN, token_str.clone());
COOKIE_BEARER_TOKEN,
token_str.clone(),
"/",
);
// Important - can be permanent as the token has its own expiration time internally // Important - can be permanent as the token has its own expiration time internally
bearer_cookie.make_permanent(); bearer_cookie.make_permanent();
@ -933,7 +929,6 @@ async fn view_login_step(
&state, &state,
COOKIE_USERNAME, COOKIE_USERNAME,
session_context.username.clone(), session_context.username.clone(),
Urls::Login.as_ref(),
); );
username_cookie.make_permanent(); username_cookie.make_permanent();
jar.add(username_cookie) jar.add(username_cookie)
@ -980,16 +975,11 @@ fn add_session_cookie(
jar: CookieJar, jar: CookieJar,
session_context: &SessionContext, session_context: &SessionContext,
) -> Result<CookieJar, OperationError> { ) -> Result<CookieJar, OperationError> {
cookies::make_signed( cookies::make_signed(state, COOKIE_AUTH_SESSION_ID, session_context)
state, .map(|mut cookie| {
COOKIE_AUTH_SESSION_ID, // Not needed when redirecting into this site
session_context, cookie.set_same_site(SameSite::Strict);
Urls::Login.as_ref(), jar.add(cookie)
) })
.map(|mut cookie| { .ok_or(OperationError::InvalidSessionState)
// Not needed when redirecting into this site
cookie.set_same_site(SameSite::Strict);
jar.add(cookie)
})
.ok_or(OperationError::InvalidSessionState)
} }

90
server/core/src/https/views/oauth2.rs
View file

@ -26,7 +26,6 @@ use axum_extra::extract::cookie::{CookieJar, SameSite};
use axum_htmx::HX_REDIRECT; use axum_htmx::HX_REDIRECT;
use serde::Deserialize; use serde::Deserialize;
use super::constants::Urls;
use super::login::{LoginDisplayCtx, Oauth2Ctx}; use super::login::{LoginDisplayCtx, Oauth2Ctx};
use super::{cookies, UnrecoverableErrorView}; use super::{cookies, UnrecoverableErrorView};
@ -96,7 +95,7 @@ async fn oauth2_auth_req(
) -> Response { ) -> Response {
// No matter what, we always clear the stored oauth2 cookie to prevent // No matter what, we always clear the stored oauth2 cookie to prevent
// ui loops // ui loops
let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &state);
// If the auth_req was cross-signed, old, or just bad, error. But we have *cleared* it // If the auth_req was cross-signed, old, or just bad, error. But we have *cleared* it
// from the cookie which means we won't see it again. // from the cookie which means we won't see it again.
@ -149,14 +148,17 @@ async fn oauth2_auth_req(
consent_token, consent_token,
}) => { }) => {
// We can just render the form now, the consent token has everything we need. // We can just render the form now, the consent token has everything we need.
ConsentRequestView { (
client_name, jar,
// scopes, ConsentRequestView {
pii_scopes, client_name,
consent_token, // scopes,
redirect: None, pii_scopes,
} consent_token,
.into_response() redirect: None,
},
)
.into_response()
} }
Ok(AuthoriseResponse::AuthenticationRequired { Ok(AuthoriseResponse::AuthenticationRequired {
@ -165,20 +167,19 @@ async fn oauth2_auth_req(
}) => { }) => {
// Sign the auth req and hide it in our cookie - we'll come back for // Sign the auth req and hide it in our cookie - we'll come back for
// you later. // you later.
let maybe_jar = let maybe_jar = cookies::make_signed(&state, COOKIE_OAUTH2_REQ, &auth_req)
cookies::make_signed(&state, COOKIE_OAUTH2_REQ, &auth_req, Urls::Ui.as_ref()) .map(|mut cookie| {
.map(|mut cookie| { cookie.set_same_site(SameSite::Strict);
cookie.set_same_site(SameSite::Strict); // Expire at the end of the session.
// Expire at the end of the session. cookie.set_expires(None);
cookie.set_expires(None); // Could experiment with this to a shorter value, but session should be enough.
// Could experiment with this to a shorter value, but session should be enough. cookie.set_max_age(time::Duration::minutes(15));
cookie.set_max_age(None); jar.clone().add(cookie)
jar.add(cookie) })
}) .ok_or(OperationError::InvalidSessionState);
.ok_or(OperationError::InvalidSessionState);
match maybe_jar { match maybe_jar {
Ok(jar) => { Ok(new_jar) => {
let display_ctx = LoginDisplayCtx { let display_ctx = LoginDisplayCtx {
domain_info, domain_info,
oauth2: Some(Oauth2Ctx { client_name }), oauth2: Some(Oauth2Ctx { client_name }),
@ -186,21 +187,27 @@ async fn oauth2_auth_req(
error: None, error: None,
}; };
super::login::view_oauth2_get(jar, display_ctx, login_hint) super::login::view_oauth2_get(new_jar, display_ctx, login_hint)
} }
Err(err_code) => UnrecoverableErrorView { Err(err_code) => (
err_code, jar,
operation_id: kopid.eventid, UnrecoverableErrorView {
} err_code,
.into_response(), operation_id: kopid.eventid,
},
)
.into_response(),
} }
} }
Err(Oauth2Error::AccessDenied) => { Err(Oauth2Error::AccessDenied) => {
// If scopes are not available for this account. // If scopes are not available for this account.
AccessDeniedView { (
operation_id: kopid.eventid, jar,
} AccessDeniedView {
.into_response() operation_id: kopid.eventid,
},
)
.into_response()
} }
/* /*
RFC - If the request fails due to a missing, invalid, or mismatching RFC - If the request fails due to a missing, invalid, or mismatching
@ -219,11 +226,14 @@ async fn oauth2_auth_req(
&err_code.to_string() &err_code.to_string()
); );
UnrecoverableErrorView { (
err_code: OperationError::InvalidState, jar,
operation_id: kopid.eventid, UnrecoverableErrorView {
} err_code: OperationError::InvalidState,
.into_response() operation_id: kopid.eventid,
},
)
.into_response()
} }
} }
} }
@ -237,13 +247,13 @@ pub struct ConsentForm {
} }
pub async fn view_consent_post( pub async fn view_consent_post(
State(state): State<ServerState>, State(server_state): State<ServerState>,
Extension(kopid): Extension<KOpId>, Extension(kopid): Extension<KOpId>,
VerifiedClientInformation(client_auth_info): VerifiedClientInformation, VerifiedClientInformation(client_auth_info): VerifiedClientInformation,
jar: CookieJar, jar: CookieJar,
Form(consent_form): Form<ConsentForm>, Form(consent_form): Form<ConsentForm>,
) -> Result<Response, UnrecoverableErrorView> { ) -> Result<Response, UnrecoverableErrorView> {
let res = state let res = server_state
.qe_w_ref .qe_w_ref
.handle_oauth2_authorise_permit(client_auth_info, consent_form.consent_token, kopid.eventid) .handle_oauth2_authorise_permit(client_auth_info, consent_form.consent_token, kopid.eventid)
.await; .await;
@ -254,7 +264,7 @@ pub async fn view_consent_post(
state, state,
code, code,
}) => { }) => {
let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ); let jar = cookies::destroy(jar, COOKIE_OAUTH2_REQ, &server_state);
if let Some(redirect) = consent_form.redirect { if let Some(redirect) = consent_form.redirect {
Ok(( Ok((

8
server/core/src/https/views/reset.rs
View file

@ -220,7 +220,7 @@ pub(crate) async fn commit(
.await?; .await?;
// No longer need the cookie jar. // No longer need the cookie jar.
let jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN); let jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN, &state);
Ok((jar, HxLocation::from(Uri::from_static("/ui")), "").into_response()) Ok((jar, HxLocation::from(Uri::from_static("/ui")), "").into_response())
} }
@ -241,7 +241,7 @@ pub(crate) async fn cancel_cred_update(
.await?; .await?;
// No longer need the cookie jar. // No longer need the cookie jar.
let jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN); let jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN, &state);
Ok(( Ok((
jar, jar,
@ -688,7 +688,7 @@ fn add_cu_cookie(
cu_session_token: CUSessionToken, cu_session_token: CUSessionToken,
) -> CookieJar { ) -> CookieJar {
let mut token_cookie = let mut token_cookie =
cookies::make_unsigned(state, COOKIE_CU_SESSION_TOKEN, cu_session_token.token, "/"); cookies::make_unsigned(state, COOKIE_CU_SESSION_TOKEN, cu_session_token.token);
token_cookie.set_same_site(SameSite::Strict); token_cookie.set_same_site(SameSite::Strict);
jar.add(token_cookie) jar.add(token_cookie)
} }
@ -788,7 +788,7 @@ pub(crate) async fn view_reset_get(
| OperationError::InvalidState, | OperationError::InvalidState,
) => { ) => {
// If our previous credential update session expired we want to see the reset form again. // If our previous credential update session expired we want to see the reset form again.
jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN); jar = cookies::destroy(jar, COOKIE_CU_SESSION_TOKEN, &state);
if let Some(token) = params.token { if let Some(token) = params.token {
let token_uri_string = format!("{}?token={}", Urls::CredReset, token); let token_uri_string = format!("{}?token={}", Urls::CredReset, token);