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 4331562C7A for ; Wed, 28 Oct 2020 12:36:52 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3FEB41EB19 for ; Wed, 28 Oct 2020 12:36:52 +0100 (CET) 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 0DC081E9DC for ; Wed, 28 Oct 2020 12:36:49 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id CB0DC45F3A for ; Wed, 28 Oct 2020 12:36:48 +0100 (CET) From: =?UTF-8?q?Fabian=20Gr=C3=BCnbichler?= To: pbs-devel@lists.proxmox.com Date: Wed, 28 Oct 2020 12:36:32 +0100 Message-Id: <20201028113632.814586-12-f.gruenbichler@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20201028113632.814586-1-f.gruenbichler@proxmox.com> References: <20201028113632.814586-1-f.gruenbichler@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 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. [datastore.rs, backup.rs, reader.rs] Subject: [pbs-devel] [PATCH proxmox-backup 09/16] owner checks: handle backups owned by API tokens 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, 28 Oct 2020 11:36:52 -0000 a user should be allowed to read/list/overwrite backups owned by their own tokens, but a token should not be able to read/list/overwrite backups owned by their owning user. when changing ownership of a backup group, a user should be able to transfer ownership to/from their own tokens if the backup is owned by them (or one of their tokens). Signed-off-by: Fabian Grünbichler --- src/api2/admin/datastore.rs | 128 ++++++++++++++++++++++-------------- src/api2/backup.rs | 5 +- src/api2/reader.rs | 5 +- 3 files changed, 88 insertions(+), 50 deletions(-) diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs index 8862637d..9d00ed8d 100644 --- a/src/api2/admin/datastore.rs +++ b/src/api2/admin/datastore.rs @@ -44,13 +44,29 @@ use crate::config::acl::{ PRIV_DATASTORE_BACKUP, }; -fn check_backup_owner( +fn check_priv_or_backup_owner( store: &DataStore, group: &BackupGroup, auth_id: &Authid, + required_privs: u64, +) -> Result<(), Error> { + let user_info = CachedUserInfo::new()?; + let privs = user_info.lookup_privs(&auth_id, &["datastore", store.name()]); + + if privs & required_privs == 0 { + let owner = store.get_owner(group)?; + check_backup_owner(&owner, auth_id)?; + } + Ok(()) +} + +fn check_backup_owner( + owner: &Authid, + auth_id: &Authid, ) -> Result<(), Error> { - let owner = store.get_owner(group)?; - if &owner != auth_id { + let correct_owner = owner == auth_id + || (owner.is_token() && &Authid::from(owner.user().clone()) == auth_id); + if !correct_owner { bail!("backup owner check failed ({} != {})", auth_id, owner); } Ok(()) @@ -171,7 +187,7 @@ fn list_groups( let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0; let owner = datastore.get_owner(group)?; - if !list_all && owner != auth_id { + if !list_all && check_backup_owner(&owner, &auth_id).is_err() { continue; } @@ -231,15 +247,11 @@ pub fn list_snapshot_files( ) -> Result, Error> { let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); - let datastore = DataStore::lookup_datastore(&store)?; let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?; - let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_READ)) != 0; - if !allowed { check_backup_owner(&datastore, snapshot.group(), &auth_id)?; } + check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_READ)?; let info = BackupInfo::new(&datastore.base_path(), snapshot)?; @@ -283,15 +295,11 @@ fn delete_snapshot( ) -> Result { let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?; - let datastore = DataStore::lookup_datastore(&store)?; - let allowed = (user_privs & PRIV_DATASTORE_MODIFY) != 0; - if !allowed { check_backup_owner(&datastore, snapshot.group(), &auth_id)?; } + check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_MODIFY)?; datastore.remove_backup_dir(&snapshot, false)?; @@ -362,7 +370,7 @@ pub fn list_snapshots ( let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0; let owner = datastore.get_owner(group)?; - if !list_all && owner != auth_id { + if !list_all && check_backup_owner(&owner, &auth_id).is_err() { continue; } @@ -702,8 +710,6 @@ fn prune( let backup_id = tools::required_string_param(¶m, "backup-id")?; let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); let dry_run = param["dry-run"].as_bool().unwrap_or(false); @@ -711,8 +717,7 @@ fn prune( let datastore = DataStore::lookup_datastore(&store)?; - let allowed = (user_privs & PRIV_DATASTORE_MODIFY) != 0; - if !allowed { check_backup_owner(&datastore, &group, &auth_id)?; } + check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?; let prune_options = PruneOptions { keep_last: param["keep-last"].as_u64(), @@ -960,8 +965,6 @@ fn download_file( let datastore = DataStore::lookup_datastore(store)?; let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); let file_name = tools::required_string_param(¶m, "file-name")?.to_owned(); @@ -971,8 +974,7 @@ fn download_file( let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; - let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; - if !allowed { check_backup_owner(&datastore, backup_dir.group(), &auth_id)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_READ)?; println!("Download {} from {} ({}/{})", file_name, store, backup_dir, file_name); @@ -1033,8 +1035,6 @@ fn download_file_decoded( let datastore = DataStore::lookup_datastore(store)?; let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); let file_name = tools::required_string_param(¶m, "file-name")?.to_owned(); @@ -1044,8 +1044,7 @@ fn download_file_decoded( let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; - let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; - if !allowed { check_backup_owner(&datastore, backup_dir.group(), &auth_id)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_READ)?; let (manifest, files) = read_backup_index(&datastore, &backup_dir)?; for file in files { @@ -1158,7 +1157,8 @@ fn upload_backup_log( let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - check_backup_owner(&datastore, backup_dir.group(), &auth_id)?; + let owner = datastore.get_owner(backup_dir.group())?; + check_backup_owner(&owner, &auth_id)?; let mut path = datastore.base_path(); path.push(backup_dir.relative_path()); @@ -1228,13 +1228,10 @@ fn catalog( let datastore = DataStore::lookup_datastore(&store)?; let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; - let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; - if !allowed { check_backup_owner(&datastore, backup_dir.group(), &auth_id)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_READ)?; let file_name = CATALOG_NAME; @@ -1399,8 +1396,6 @@ fn pxar_file_download( let datastore = DataStore::lookup_datastore(&store)?; let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); let filepath = tools::required_string_param(¶m, "filepath")?.to_owned(); @@ -1410,8 +1405,7 @@ fn pxar_file_download( let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; - let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; - if !allowed { check_backup_owner(&datastore, backup_dir.group(), &auth_id)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_READ)?; let mut components = base64::decode(&filepath)?; if components.len() > 0 && components[0] == '/' as u8 { @@ -1578,13 +1572,9 @@ fn get_notes( let datastore = DataStore::lookup_datastore(&store)?; let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; - let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; - if !allowed { check_backup_owner(&datastore, backup_dir.group(), &auth_id)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_READ)?; let (manifest, _) = datastore.load_manifest(&backup_dir)?; @@ -1631,13 +1621,9 @@ fn set_notes( let datastore = DataStore::lookup_datastore(&store)?; let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); - let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; - let allowed = (user_privs & PRIV_DATASTORE_READ) != 0; - if !allowed { check_backup_owner(&datastore, backup_dir.group(), &auth_id)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_READ)?; datastore.update_manifest(&backup_dir,|manifest| { manifest.unprotected["notes"] = notes.into(); @@ -1664,7 +1650,8 @@ fn set_notes( }, }, access: { - permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true), + permission: &Permission::Anybody, + description: "Datastore.Modify on whole datastore, or changing ownership between user and a user's token for owned backups with Datastore.Backup" }, )] /// Change owner of a backup group @@ -1673,15 +1660,60 @@ fn set_backup_owner( backup_type: String, backup_id: String, new_owner: Authid, - _rpcenv: &mut dyn RpcEnvironment, + rpcenv: &mut dyn RpcEnvironment, ) -> Result<(), Error> { let datastore = DataStore::lookup_datastore(&store)?; let backup_group = BackupGroup::new(backup_type, backup_id); + let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; + let user_info = CachedUserInfo::new()?; + let privs = user_info.lookup_privs(&auth_id, &["datastore", &store]); + + let allowed = if (privs & PRIV_DATASTORE_MODIFY) != 0 { + // High-privilege user/token + true + } else if (privs & PRIV_DATASTORE_BACKUP) != 0 { + let owner = datastore.get_owner(&backup_group)?; + + match (owner.is_token(), new_owner.is_token()) { + (true, true) => { + // API token to API token, owned by same user + let owner = owner.user(); + let new_owner = new_owner.user(); + owner == new_owner && Authid::from(owner.clone()) == auth_id + }, + (true, false) => { + // API token to API token owner + Authid::from(owner.user().clone()) == auth_id + && new_owner == auth_id + }, + (false, true) => { + // API token owner to API token + owner == auth_id + && Authid::from(new_owner.user().clone()) == auth_id + }, + (false, false) => { + // User to User, not allowed for unprivileged users + false + }, + } + } else { + false + }; + + if !allowed { + return Err(http_err!(UNAUTHORIZED, + "{} does not have permission to change owner of backup group '{}' to {}", + auth_id, + backup_group, + new_owner, + )); + } + if !user_info.is_active_auth_id(&new_owner) { bail!("{} '{}' is inactive or non-existent", if new_owner.is_token() { diff --git a/src/api2/backup.rs b/src/api2/backup.rs index e030d60d..ce9a34ae 100644 --- a/src/api2/backup.rs +++ b/src/api2/backup.rs @@ -108,7 +108,10 @@ async move { let (owner, _group_guard) = datastore.create_locked_backup_group(&backup_group, &auth_id)?; // permission check - if owner != auth_id && worker_type != "benchmark" { + let correct_owner = owner == auth_id + || (owner.is_token() + && Authid::from(owner.user().clone()) == auth_id); + if !correct_owner && worker_type != "benchmark" { // only the owner is allowed to create additional snapshots bail!("backup owner check failed ({} != {})", auth_id, owner); } diff --git a/src/api2/reader.rs b/src/api2/reader.rs index 3eeece52..010d73e3 100644 --- a/src/api2/reader.rs +++ b/src/api2/reader.rs @@ -94,7 +94,10 @@ fn upgrade_to_backup_reader_protocol( let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; if !priv_read { let owner = datastore.get_owner(backup_dir.group())?; - if owner != auth_id { + let correct_owner = owner == auth_id + || (owner.is_token() + && Authid::from(owner.user().clone()) == auth_id); + if !correct_owner { bail!("backup owner check failed!"); } } -- 2.20.1