[pbs-devel] [PATCH v3 proxmox-backup 09/20] server/rest: add ApiAuth trait to make user auth generic
Wolfgang Bumiller
w.bumiller at proxmox.com
Wed Mar 31 14:55:57 CEST 2021
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 <s.reiter at proxmox.com>
> ---
>
> 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<Error> 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<anyhow::Error>` 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<Authid, AuthError>;
> +}
> +
> +struct UserAuthData {
> ticket: String,
> csrf_token: Option<String>,
> }
>
> -pub enum AuthData {
> +enum AuthData {
> User(UserAuthData),
> ApiToken(String),
> }
>
> -pub fn extract_auth_data(headers: &http::HeaderMap) -> Option<AuthData> {
> - 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<UserApiAuth> {
> + Arc::new(UserApiAuth {})
> }
>
> -pub fn check_auth(
> - method: &hyper::Method,
> - auth_data: &AuthData,
> - user_info: &CachedUserInfo,
> -) -> Result<Authid, Error> {
> - 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::<super::ticket::ApiTicket>::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<AuthData> {
> + 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<Authid, AuthError> {
> + 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::<super::ticket::ApiTicket>::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<HashMap<String, (SystemTime, PathBuf)>>,
> request_log: Option<Arc<Mutex<FileLogger>>>,
> pub enable_tape_ui: bool,
> + pub api_auth: Arc<dyn ApiAuth + Send + Sync>,
> }
>
> impl ApiConfig {
> -
> - pub fn new<B: Into<PathBuf>>(basedir: B, router: &'static Router, env_type: RpcEnvironmentType) -> Result<Self, Error> {
> + pub fn new<B: Into<PathBuf>>(
> + basedir: B,
> + router: &'static Router,
> + env_type: RpcEnvironmentType,
> + api_auth: Arc<dyn ApiAuth + Send + Sync>,
> + ) -> Result<Self, Error> {
> 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
More information about the pbs-devel
mailing list