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 44C25609AA for ; Mon, 19 Oct 2020 09:40:37 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3AE1D29913 for ; Mon, 19 Oct 2020 09:40:07 +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 E861C298E7 for ; Mon, 19 Oct 2020 09:40:05 +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 B061B45DFD for ; Mon, 19 Oct 2020 09:40:05 +0200 (CEST) From: =?UTF-8?q?Fabian=20Gr=C3=BCnbichler?= To: pbs-devel@lists.proxmox.com Date: Mon, 19 Oct 2020 09:39:16 +0200 Message-Id: <20201019073919.588521-13-f.gruenbichler@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20201019073919.588521-1-f.gruenbichler@proxmox.com> References: <20201019073919.588521-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.031 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 Subject: [pbs-devel] [RFC proxmox-backup 12/15] 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: Mon, 19 Oct 2020 07:40:37 -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 | 124 +++++++++++++++++++++++------------- src/api2/backup.rs | 3 +- src/api2/reader.rs | 3 +- 3 files changed, 82 insertions(+), 48 deletions(-) diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs index 5c9902e1..a279d382 100644 --- a/src/api2/admin/datastore.rs +++ b/src/api2/admin/datastore.rs @@ -37,13 +37,29 @@ use crate::config::acl::{ PRIV_DATASTORE_BACKUP, }; -fn check_backup_owner( +fn check_priv_or_backup_owner( store: &DataStore, group: &BackupGroup, userid: &Userid, + required_privs: u64, ) -> Result<(), Error> { - let owner = store.get_owner(group)?; - if &owner != userid { + let user_info = CachedUserInfo::new()?; + let user_privs = user_info.lookup_privs(&userid, &["datastore", store.name()]); + + if user_privs & required_privs == 0 { + let owner = store.get_owner(group)?; + check_backup_owner(&owner, userid)?; + } + + Ok(()) +} + +fn check_backup_owner( + owner: &Userid, + userid: &Userid, +) -> Result<(), Error> { + let correct_owner = owner == userid || (owner.is_tokenid() && &owner.owner()? == userid); + if !correct_owner { bail!("backup owner check failed ({} != {})", userid, owner); } Ok(()) @@ -164,7 +180,7 @@ fn list_groups( let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0; let owner = datastore.get_owner(group)?; - if !list_all && owner != userid { + if !list_all && check_backup_owner(&owner, &userid).is_err() { continue; } @@ -224,15 +240,11 @@ pub fn list_snapshot_files( ) -> Result, Error> { let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["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(), &userid)?; } + check_priv_or_backup_owner(&datastore, snapshot.group(), &userid, PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_READ)?; let info = BackupInfo::new(&datastore.base_path(), snapshot)?; @@ -276,15 +288,11 @@ fn delete_snapshot( ) -> Result { let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["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(), &userid)?; } + check_priv_or_backup_owner(&datastore, snapshot.group(), &userid, PRIV_DATASTORE_MODIFY)?; datastore.remove_backup_dir(&snapshot, false)?; @@ -355,7 +363,7 @@ pub fn list_snapshots ( let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0; let owner = datastore.get_owner(group)?; - if !list_all && owner != userid { + if !list_all && check_backup_owner(&owner, &userid).is_err() { continue; } @@ -637,8 +645,6 @@ fn prune( let backup_id = tools::required_string_param(¶m, "backup-id")?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); let dry_run = param["dry-run"].as_bool().unwrap_or(false); @@ -646,8 +652,7 @@ fn prune( let datastore = DataStore::lookup_datastore(&store)?; - let allowed = (user_privs & PRIV_DATASTORE_MODIFY) != 0; - if !allowed { check_backup_owner(&datastore, &group, &userid)?; } + check_priv_or_backup_owner(&datastore, &group, &userid, PRIV_DATASTORE_MODIFY)?; let prune_options = PruneOptions { keep_last: param["keep-last"].as_u64(), @@ -894,8 +899,6 @@ fn download_file( let datastore = DataStore::lookup_datastore(store)?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); let file_name = tools::required_string_param(¶m, "file-name")?.to_owned(); @@ -905,8 +908,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(), &userid)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &userid, PRIV_DATASTORE_READ)?; println!("Download {} from {} ({}/{})", file_name, store, backup_dir, file_name); @@ -967,8 +969,6 @@ fn download_file_decoded( let datastore = DataStore::lookup_datastore(store)?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); let file_name = tools::required_string_param(¶m, "file-name")?.to_owned(); @@ -978,8 +978,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(), &userid)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &userid, PRIV_DATASTORE_READ)?; let (manifest, files) = read_backup_index(&datastore, &backup_dir)?; for file in files { @@ -1092,7 +1091,8 @@ fn upload_backup_log( let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; - check_backup_owner(&datastore, backup_dir.group(), &userid)?; + let owner = datastore.get_owner(backup_dir.group())?; + check_backup_owner(&owner, &userid)?; let mut path = datastore.base_path(); path.push(backup_dir.relative_path()); @@ -1162,13 +1162,10 @@ fn catalog( let datastore = DataStore::lookup_datastore(&store)?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["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(), &userid)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &userid, PRIV_DATASTORE_READ)?; let file_name = CATALOG_NAME; @@ -1273,8 +1270,6 @@ fn pxar_file_download( let datastore = DataStore::lookup_datastore(&store)?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); let filepath = tools::required_string_param(¶m, "filepath")?.to_owned(); @@ -1284,8 +1279,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(), &userid)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &userid, PRIV_DATASTORE_READ)?; let mut components = base64::decode(&filepath)?; if components.len() > 0 && components[0] == '/' as u8 { @@ -1420,13 +1414,10 @@ fn get_notes( let datastore = DataStore::lookup_datastore(&store)?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["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(), &userid)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &userid, PRIV_DATASTORE_READ)?; let (manifest, _) = datastore.load_manifest(&backup_dir)?; @@ -1473,13 +1464,10 @@ fn set_notes( let datastore = DataStore::lookup_datastore(&store)?; let userid: Userid = rpcenv.get_user().unwrap().parse()?; - let user_info = CachedUserInfo::new()?; - let user_privs = user_info.lookup_privs(&userid, &["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(), &userid)?; } + check_priv_or_backup_owner(&datastore, backup_dir.group(), &userid, PRIV_DATASTORE_READ)?; datastore.update_manifest(&backup_dir,|manifest| { manifest.unprotected["notes"] = notes.into(); @@ -1506,7 +1494,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 @@ -1515,15 +1504,58 @@ fn set_backup_owner( backup_type: String, backup_id: String, new_owner: Userid, - _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 userid:Userid = rpcenv.get_user().unwrap().parse()?; + let user_info = CachedUserInfo::new()?; + let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]); + + let allowed = if (user_privs & PRIV_DATASTORE_MODIFY) != 0 { + // High-privilege user/token + true + } else if (user_privs & PRIV_DATASTORE_BACKUP) != 0 { + let owner = datastore.get_owner(&backup_group)?; + + match (owner.is_tokenid(), new_owner.is_tokenid()) { + (true, true) => { + // API token to API token, owned by same user + let owner = owner.owner().unwrap(); + let new_owner = new_owner.owner().unwrap(); + owner == new_owner && owner == userid + }, + (true, false) => { + // API token to API token owner + owner.owner().unwrap() == userid && new_owner == userid + }, + (false, true) => { + // API token owner to API token + owner == userid && new_owner.owner().unwrap() == userid + }, + (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 {}", + userid, + backup_group, + new_owner, + )); + } + if !user_info.is_active_user(&new_owner) { bail!("user '{}' is inactive or non-existent", new_owner); } diff --git a/src/api2/backup.rs b/src/api2/backup.rs index 8b2b0e1a..17da0558 100644 --- a/src/api2/backup.rs +++ b/src/api2/backup.rs @@ -108,7 +108,8 @@ async move { let (owner, _group_guard) = datastore.create_locked_backup_group(&backup_group, &userid)?; // permission check - if owner != userid && worker_type != "benchmark" { + let correct_owner = owner == userid || (owner.is_tokenid() && owner.owner()? == userid); + if !correct_owner && worker_type != "benchmark" { // only the owner is allowed to create additional snapshots bail!("backup owner check failed ({} != {})", userid, owner); } diff --git a/src/api2/reader.rs b/src/api2/reader.rs index 8f47047e..7ea066c4 100644 --- a/src/api2/reader.rs +++ b/src/api2/reader.rs @@ -94,7 +94,8 @@ 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 != userid { + let correct_owner = owner == userid || (owner.is_tokenid() && owner.owner()? == userid); + if !correct_owner { bail!("backup owner check failed!"); } } -- 2.20.1