From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 53F906D115 for ; Wed, 31 Mar 2021 14:56:04 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 49F1CFDE2 for ; Wed, 31 Mar 2021 14:56:04 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id BF05FFDD6 for ; Wed, 31 Mar 2021 14:56:00 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id BF6B84296F for ; Wed, 31 Mar 2021 14:55:59 +0200 (CEST) Date: Wed, 31 Mar 2021 14:55:57 +0200 From: Wolfgang Bumiller To: Stefan Reiter Cc: pbs-devel@lists.proxmox.com Message-ID: <20210331125557.xcrymxfbcedqjegj@wobu-vie.proxmox.com> References: <20210331102202.14767-1-s.reiter@proxmox.com> <20210331102202.14767-10-s.reiter@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210331102202.14767-10-s.reiter@proxmox.com> User-Agent: NeoMutt/20180716 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.028 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox-backup-proxy.rs, proxmox-backup-api.rs, rest.rs, config.rs, auth.rs] Subject: Re: [pbs-devel] [PATCH v3 proxmox-backup 09/20] server/rest: add ApiAuth trait to make user auth generic X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 31 Mar 2021 12:56:04 -0000 LGTM but I'll suggest some quality-of-life improvements: On Wed, Mar 31, 2021 at 12:21:51PM +0200, Stefan Reiter wrote: > This allows switching the base user identification/authentication method > in the rest server. Will initially be used for single file restore VMs, > where authentication is based on a ticket file, not the PBS user > backend (PAM/local). > > To avoid putting generic types into the RestServer type for this, we > merge the two calls "extract_auth_data" and "check_auth" into a single > one, which can use whatever type it wants internally. > > Signed-off-by: Stefan Reiter > --- > > v3: > * merge both calls into one trait, that way it doesn't have to be generic > > src/bin/proxmox-backup-api.rs | 13 ++- > src/bin/proxmox-backup-proxy.rs | 7 +- > src/server/auth.rs | 192 +++++++++++++++++++------------- > src/server/config.rs | 13 ++- > src/server/rest.rs | 36 +++--- > 5 files changed, 159 insertions(+), 102 deletions(-) > > diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs > index 7d800259..e514a801 100644 > --- a/src/bin/proxmox-backup-api.rs > +++ b/src/bin/proxmox-backup-api.rs > @@ -6,8 +6,11 @@ use proxmox::api::RpcEnvironmentType; > > //use proxmox_backup::tools; > //use proxmox_backup::api_schema::config::*; > -use proxmox_backup::server::rest::*; > -use proxmox_backup::server; > +use proxmox_backup::server::{ > + self, > + auth::default_api_auth, > + rest::*, > +}; > use proxmox_backup::tools::daemon; > use proxmox_backup::auth_helpers::*; > use proxmox_backup::config; > @@ -53,7 +56,11 @@ async fn run() -> Result<(), Error> { > let _ = csrf_secret(); // load with lazy_static > > let mut config = server::ApiConfig::new( > - buildcfg::JS_DIR, &proxmox_backup::api2::ROUTER, RpcEnvironmentType::PRIVILEGED)?; > + buildcfg::JS_DIR, > + &proxmox_backup::api2::ROUTER, > + RpcEnvironmentType::PRIVILEGED, > + default_api_auth(), > + )?; > > let mut commando_sock = server::CommandoSocket::new(server::our_ctrl_sock()); > > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > index 541d34b5..7e026455 100644 > --- a/src/bin/proxmox-backup-proxy.rs > +++ b/src/bin/proxmox-backup-proxy.rs > @@ -14,6 +14,7 @@ use proxmox::api::RpcEnvironmentType; > use proxmox_backup::{ > backup::DataStore, > server::{ > + auth::default_api_auth, > WorkerTask, > ApiConfig, > rest::*, > @@ -84,7 +85,11 @@ async fn run() -> Result<(), Error> { > let _ = csrf_secret(); // load with lazy_static > > let mut config = ApiConfig::new( > - buildcfg::JS_DIR, &proxmox_backup::api2::ROUTER, RpcEnvironmentType::PUBLIC)?; > + buildcfg::JS_DIR, > + &proxmox_backup::api2::ROUTER, > + RpcEnvironmentType::PUBLIC, > + default_api_auth(), > + )?; > > // Enable experimental tape UI if tape.cfg exists > if Path::new("/etc/proxmox-backup/tape.cfg").exists() { > diff --git a/src/server/auth.rs b/src/server/auth.rs > index 24151886..0a9a740c 100644 > --- a/src/server/auth.rs > +++ b/src/server/auth.rs > @@ -1,102 +1,140 @@ > //! Provides authentication primitives for the HTTP server > -use anyhow::{bail, format_err, Error}; > +use anyhow::{format_err, Error}; > + > +use std::sync::Arc; > > -use crate::tools::ticket::Ticket; > -use crate::auth_helpers::*; > -use crate::tools; > -use crate::config::cached_user_info::CachedUserInfo; > use crate::api2::types::{Authid, Userid}; > +use crate::auth_helpers::*; > +use crate::config::cached_user_info::CachedUserInfo; > +use crate::tools; > +use crate::tools::ticket::Ticket; > > use hyper::header; > use percent_encoding::percent_decode_str; > > -pub struct UserAuthData { > +pub enum AuthError { > + Generic(Error), > + NoData, > +} > + > +impl From for AuthError { > + fn from(err: Error) -> Self { > + AuthError::Generic(err) > + } > +} ^ When you define an Error type you should immediately also derive Debug and implement `std::fmt::Display`. While you only "display" it once, I don't think the error message should be left up to the caller (see further down below). In order to make this shorter and easier, I'd propose the inclusion of the `thiserror` helper crate, then we'd need only as little code as: #[derive(thiserror::Error, Debug)] pub enum AuthError { #[error(transparent)] Generic(#[from] Error), #[error("no authentication credentials provided")] NoData, } And the manual `From` impl can simply be dropped ;-) That is *unless* you explicitly want this to *not* be convertible to `anyhow::Error` automagically. Then you don't want this to impl `std::error::Error`, but then you should add a comment for this ;-) Which would be a valid way to go given that you're already wrapping `anyhow::Error` in there... OTOH all the token parsing errors could be combined into a single `AuthError::BadToken` instead of multiple "slightly different" `format_err!` messages where the minor difference in the error message doesn't really provide much value anyway (IMO). Anyway, this can easily be left as is and updated up in a later series. > + > +pub trait ApiAuth { > + fn check_auth( > + &self, > + headers: &http::HeaderMap, > + method: &hyper::Method, > + user_info: &CachedUserInfo, > + ) -> Result; > +} > + > +struct UserAuthData { > ticket: String, > csrf_token: Option, > } > > -pub enum AuthData { > +enum AuthData { > User(UserAuthData), > ApiToken(String), > } > > -pub fn extract_auth_data(headers: &http::HeaderMap) -> Option { > - if let Some(raw_cookie) = headers.get(header::COOKIE) { > - if let Ok(cookie) = raw_cookie.to_str() { > - if let Some(ticket) = tools::extract_cookie(cookie, "PBSAuthCookie") { > - let csrf_token = match headers.get("CSRFPreventionToken").map(|v| v.to_str()) { > - Some(Ok(v)) => Some(v.to_owned()), > - _ => None, > - }; > - return Some(AuthData::User(UserAuthData { > - ticket, > - csrf_token, > - })); > - } > - } > - } > - > - match headers.get(header::AUTHORIZATION).map(|v| v.to_str()) { > - Some(Ok(v)) => { > - if v.starts_with("PBSAPIToken ") || v.starts_with("PBSAPIToken=") { > - Some(AuthData::ApiToken(v["PBSAPIToken ".len()..].to_owned())) > - } else { > - None > - } > - }, > - _ => None, > - } > +pub struct UserApiAuth {} > +pub fn default_api_auth() -> Arc { > + Arc::new(UserApiAuth {}) > } > > -pub fn check_auth( > - method: &hyper::Method, > - auth_data: &AuthData, > - user_info: &CachedUserInfo, > -) -> Result { > - match auth_data { > - AuthData::User(user_auth_data) => { > - let ticket = user_auth_data.ticket.clone(); > - let ticket_lifetime = tools::ticket::TICKET_LIFETIME; > - > - let userid: Userid = Ticket::::parse(&ticket)? > - .verify_with_time_frame(public_auth_key(), "PBS", None, -300..ticket_lifetime)? > - .require_full()?; > - > - let auth_id = Authid::from(userid.clone()); > - if !user_info.is_active_auth_id(&auth_id) { > - bail!("user account disabled or expired."); > - } > - > - if method != hyper::Method::GET { > - if let Some(csrf_token) = &user_auth_data.csrf_token { > - verify_csrf_prevention_token(csrf_secret(), &userid, &csrf_token, -300, ticket_lifetime)?; > - } else { > - bail!("missing CSRF prevention token"); > +impl UserApiAuth { > + fn extract_auth_data(headers: &http::HeaderMap) -> Option { > + if let Some(raw_cookie) = headers.get(header::COOKIE) { > + if let Ok(cookie) = raw_cookie.to_str() { > + if let Some(ticket) = tools::extract_cookie(cookie, "PBSAuthCookie") { > + let csrf_token = match headers.get("CSRFPreventionToken").map(|v| v.to_str()) { > + Some(Ok(v)) => Some(v.to_owned()), > + _ => None, > + }; > + return Some(AuthData::User(UserAuthData { ticket, csrf_token })); > } > } > + } > > - Ok(auth_id) > - }, > - AuthData::ApiToken(api_token) => { > - let mut parts = api_token.splitn(2, ':'); > - let tokenid = parts.next() > - .ok_or_else(|| format_err!("failed to split API token header"))?; > - let tokenid: Authid = tokenid.parse()?; > - > - if !user_info.is_active_auth_id(&tokenid) { > - bail!("user account or token disabled or expired."); > + match headers.get(header::AUTHORIZATION).map(|v| v.to_str()) { > + Some(Ok(v)) => { > + if v.starts_with("PBSAPIToken ") || v.starts_with("PBSAPIToken=") { > + Some(AuthData::ApiToken(v["PBSAPIToken ".len()..].to_owned())) > + } else { > + None > + } > } > - > - let tokensecret = parts.next() > - .ok_or_else(|| format_err!("failed to split API token header"))?; > - let tokensecret = percent_decode_str(tokensecret) > - .decode_utf8() > - .map_err(|_| format_err!("failed to decode API token header"))?; > - > - crate::config::token_shadow::verify_secret(&tokenid, &tokensecret)?; > - > - Ok(tokenid) > + _ => None, > } > } > } > > +impl ApiAuth for UserApiAuth { > + fn check_auth( > + &self, > + headers: &http::HeaderMap, > + method: &hyper::Method, > + user_info: &CachedUserInfo, > + ) -> Result { > + let auth_data = Self::extract_auth_data(headers); > + match auth_data { > + Some(AuthData::User(user_auth_data)) => { > + let ticket = user_auth_data.ticket.clone(); > + let ticket_lifetime = tools::ticket::TICKET_LIFETIME; > + > + let userid: Userid = Ticket::::parse(&ticket)? > + .verify_with_time_frame(public_auth_key(), "PBS", None, -300..ticket_lifetime)? > + .require_full()?; > + > + let auth_id = Authid::from(userid.clone()); > + if !user_info.is_active_auth_id(&auth_id) { > + return Err(format_err!("user account disabled or expired.").into()); > + } > + > + if method != hyper::Method::GET { > + if let Some(csrf_token) = &user_auth_data.csrf_token { > + verify_csrf_prevention_token( > + csrf_secret(), > + &userid, > + &csrf_token, > + -300, > + ticket_lifetime, > + )?; > + } else { > + return Err(format_err!("missing CSRF prevention token").into()); > + } > + } > + > + Ok(auth_id) > + } > + Some(AuthData::ApiToken(api_token)) => { > + let mut parts = api_token.splitn(2, ':'); > + let tokenid = parts > + .next() > + .ok_or_else(|| format_err!("failed to split API token header"))?; > + let tokenid: Authid = tokenid.parse()?; > + > + if !user_info.is_active_auth_id(&tokenid) { > + return Err(format_err!("user account or token disabled or expired.").into()); > + } > + > + let tokensecret = parts > + .next() > + .ok_or_else(|| format_err!("failed to split API token header"))?; > + let tokensecret = percent_decode_str(tokensecret) > + .decode_utf8() > + .map_err(|_| format_err!("failed to decode API token header"))?; > + > + crate::config::token_shadow::verify_secret(&tokenid, &tokensecret)?; > + > + Ok(tokenid) > + } > + None => Err(AuthError::NoData), > + } > + } > +} > diff --git a/src/server/config.rs b/src/server/config.rs > index 9094fa80..ad378b0a 100644 > --- a/src/server/config.rs > +++ b/src/server/config.rs > @@ -13,6 +13,7 @@ use proxmox::api::{ApiMethod, Router, RpcEnvironmentType}; > use proxmox::tools::fs::{create_path, CreateOptions}; > > use crate::tools::{FileLogger, FileLogOptions}; > +use super::auth::ApiAuth; > > pub struct ApiConfig { > basedir: PathBuf, > @@ -23,11 +24,16 @@ pub struct ApiConfig { > template_files: RwLock>, > request_log: Option>>, > pub enable_tape_ui: bool, > + pub api_auth: Arc, > } > > impl ApiConfig { > - > - pub fn new>(basedir: B, router: &'static Router, env_type: RpcEnvironmentType) -> Result { > + pub fn new>( > + basedir: B, > + router: &'static Router, > + env_type: RpcEnvironmentType, > + api_auth: Arc, > + ) -> Result { > Ok(Self { > basedir: basedir.into(), > router, > @@ -37,7 +43,8 @@ impl ApiConfig { > template_files: RwLock::new(HashMap::new()), > request_log: None, > enable_tape_ui: false, > - }) > + api_auth, > + }) > } > > pub fn find_method( > diff --git a/src/server/rest.rs b/src/server/rest.rs > index 9a971890..2d033510 100644 > --- a/src/server/rest.rs > +++ b/src/server/rest.rs > @@ -14,7 +14,6 @@ use hyper::header::{self, HeaderMap}; > use hyper::http::request::Parts; > use hyper::{Body, Request, Response, StatusCode}; > use lazy_static::lazy_static; > -use percent_encoding::percent_decode_str; > use regex::Regex; > use serde_json::{json, Value}; > use tokio::fs::File; > @@ -31,16 +30,15 @@ use proxmox::api::{ > }; > use proxmox::http_err; > > +use super::auth::AuthError; > use super::environment::RestEnvironment; > use super::formatter::*; > use super::ApiConfig; > -use super::auth::{check_auth, extract_auth_data}; > > use crate::api2::types::{Authid, Userid}; > use crate::auth_helpers::*; > use crate::config::cached_user_info::CachedUserInfo; > use crate::tools; > -use crate::tools::ticket::Ticket; > use crate::tools::FileLogger; > > extern "C" { > @@ -614,6 +612,7 @@ async fn handle_request( > rpcenv.set_client_ip(Some(*peer)); > > let user_info = CachedUserInfo::new()?; > + let auth = &api.api_auth; > > let delay_unauth_time = std::time::Instant::now() + std::time::Duration::from_millis(3000); > let access_forbidden_time = std::time::Instant::now() + std::time::Duration::from_millis(500); > @@ -639,13 +638,15 @@ async fn handle_request( > } > > if auth_required { > - let auth_result = match extract_auth_data(&parts.headers) { > - Some(auth_data) => check_auth(&method, &auth_data, &user_info), > - None => Err(format_err!("no authentication credentials provided.")), > - }; > - match auth_result { > + match auth.check_auth(&parts.headers, &method, &user_info) { > Ok(authid) => rpcenv.set_auth_id(Some(authid.to_string())), > - Err(err) => { > + Err(auth_err) => { > + let err = match auth_err { > + AuthError::Generic(err) => err, > + AuthError::NoData => { > + format_err!("no authentication credentials provided.") > + } > + }; With the `fmt::Display` impl for `AuthError` the above match on `auth_err` can be dropped as well and the `Err(auth_err)` case can stay `Err(err)` with the code below still working fine as is. > let peer = peer.ip(); > auth_logger()?.log(format!( > "authentication failure; rhost={} msg={}", > @@ -708,9 +709,9 @@ async fn handle_request( > > if comp_len == 0 { > let language = extract_lang_header(&parts.headers); > - if let Some(auth_data) = extract_auth_data(&parts.headers) { > - match check_auth(&method, &auth_data, &user_info) { > - Ok(auth_id) if !auth_id.is_token() => { > + match auth.check_auth(&parts.headers, &method, &user_info) { > + Ok(auth_id) => { > + if !auth_id.is_token() { > let userid = auth_id.user(); > let new_csrf_token = assemble_csrf_prevention_token(csrf_secret(), userid); > return Ok(get_index( > @@ -721,14 +722,13 @@ async fn handle_request( > parts, > )); > } > - _ => { > - tokio::time::sleep_until(Instant::from_std(delay_unauth_time)).await; > - return Ok(get_index(None, None, language, &api, parts)); > - } > } > - } else { > - return Ok(get_index(None, None, language, &api, parts)); > + Err(AuthError::Generic(_)) => { > + tokio::time::sleep_until(Instant::from_std(delay_unauth_time)).await; > + } > + Err(AuthError::NoData) => {} > } > + return Ok(get_index(None, None, language, &api, parts)); > } else { > let filename = api.find_alias(&components); > return handle_static_file_download(filename).await; > -- > 2.20.1