[pbs-devel] [PATCH proxmox-backup/proxmox 0/6] ACL removal on user/token deletion + token regeneration

[pbs-devel] [PATCH proxmox-backup/proxmox 0/6] ACL removal on user/token deletion + token regeneration

[Hannes Laimer]

If a user is deleted all its permissions and tokens
will now be deleted with it. If a token is deleted
all its permissions will now be deleted.
Until now neither of those two happened[1].
The last two commits add the possibility to regenerate
tokens, basically revoking the old and generating a
new secret while keeping all the set permissions.

This is all in the same series since just adding the
removal of permissions would kill the currently only
way to keep the permissions but change the secret of
a token(deleting it and creating it again with the
same name[2]).

-> pbs-api-types dep has to be bumped since we need the schema added in #4
for #5.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4382
[2] https://bugzilla.proxmox.com/show_bug.cgi?id=3887


* proxmox
Hannes Laimer (1):
  pbs-api-types: add REGENERATE_TOKEN_SCHEMA

 pbs-api-types/src/user.rs | 6 ++++++
 1 file changed, 6 insertions(+)

* proxmox-backup
Hannes Laimer (5):
  pbs-config: move secret generation into token_shadow
  fix #4382: api: access: remove permissions of token on deletion
  fix #4382: api: remove permissions and tokens of user on deletion
  fix #3887: api: access: allow secret regeneration
  fix #3887: ui: add regenerate token button

 pbs-config/Cargo.toml          |  1 +
 pbs-config/src/token_shadow.rs | 10 ++++-
 src/api2/access/user.rs        | 80 +++++++++++++++++++++++++++-------
 www/config/TokenView.js        | 29 ++++++++++++
 4 files changed, 104 insertions(+), 16 deletions(-)

-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 1/6] pbs-config: move secret generation into token_shadow

[Hannes Laimer]

so we have only one place where we generate secrets.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-config/Cargo.toml          |  1 +
 pbs-config/src/token_shadow.rs | 10 +++++++++-
 src/api2/access/user.rs        |  3 +--
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/pbs-config/Cargo.toml b/pbs-config/Cargo.toml
index 12d0eb3d..28414965 100644
--- a/pbs-config/Cargo.toml
+++ b/pbs-config/Cargo.toml
@@ -24,6 +24,7 @@ proxmox-section-config.workspace = true
 proxmox-shared-memory.workspace = true
 proxmox-sys = { workspace = true, features = [ "acl", "crypt", "timer" ] }
 proxmox-time.workspace = true
+proxmox-uuid.workspace = true
 
 pbs-api-types.workspace = true
 pbs-buildcfg.workspace = true
diff --git a/pbs-config/src/token_shadow.rs b/pbs-config/src/token_shadow.rs
index 2efb187e..640fabbf 100644
--- a/pbs-config/src/token_shadow.rs
+++ b/pbs-config/src/token_shadow.rs
@@ -61,8 +61,16 @@ pub fn verify_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
     }
 }
 
+/// Generates a new secret for the given tokenid / API token, sets it then returns it.
+/// The secret is stored as salted hash.
+pub fn generate_and_set_secret(tokenid: &Authid) -> Result<String, Error> {
+    let secret = format!("{:x}", proxmox_uuid::Uuid::generate());
+    set_secret(tokenid, &secret)?;
+    Ok(secret)
+}
+
 /// Adds a new entry for the given tokenid / API token secret. The secret is stored as salted hash.
-pub fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
+fn set_secret(tokenid: &Authid, secret: &str) -> Result<(), Error> {
     if !tokenid.is_token() {
         bail!("not an API token ID");
     }
diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index 4821551c..f3c3644a 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -495,8 +495,7 @@ pub fn generate_token(
         );
     }
 
-    let secret = format!("{:x}", proxmox_uuid::Uuid::generate());
-    token_shadow::set_secret(&tokenid, &secret)?;
+    let secret = token_shadow::generate_and_set_secret(&tokenid)?;
 
     let token = ApiToken {
         tokenid,
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/6] fix #4382: api: access: remove permissions of token on deletion

[Hannes Laimer]

... and move token deletion into new `do_delete_token` function.
Since it'll be resued later on user deletion.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/access/user.rs | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index f3c3644a..39cffdba 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -8,6 +8,7 @@ use std::collections::HashMap;
 
 use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment, SubdirMap};
 use proxmox_schema::api;
+use proxmox_section_config::SectionConfigData;
 use proxmox_tfa::api::TfaConfig;
 
 use pbs_api_types::{
@@ -15,9 +16,7 @@ use pbs_api_types::{
     EXPIRE_USER_SCHEMA, PASSWORD_FORMAT, PBS_PASSWORD_SCHEMA, PRIV_PERMISSIONS_MODIFY,
     PRIV_SYS_AUDIT, PROXMOX_CONFIG_DIGEST_SCHEMA, SINGLE_LINE_COMMENT_SCHEMA,
 };
-use pbs_config::token_shadow;
-
-use pbs_config::CachedUserInfo;
+use pbs_config::{acl::AclTree, token_shadow, CachedUserInfo};
 
 fn new_user_with_tokens(user: User, tfa: &TfaConfig) -> UserWithTokens {
     UserWithTokens {
@@ -625,29 +624,41 @@ pub fn delete_token(
     token_name: Tokenname,
     digest: Option<String>,
 ) -> Result<(), Error> {
-    let _lock = pbs_config::user::lock_config()?;
+    let _acl_lock = pbs_config::acl::lock_config()?;
+    let _user_lock = pbs_config::user::lock_config()?;
 
-    let (mut config, expected_digest) = pbs_config::user::config()?;
+    let (mut user_config, expected_digest) = pbs_config::user::config()?;
 
     if let Some(ref digest) = digest {
         let digest = <[u8; 32]>::from_hex(digest)?;
         crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
     }
 
+    let (mut acl_config, _digest) = pbs_config::acl::config()?;
+    do_delete_token(token_name, &userid, &mut user_config, &mut acl_config)?;
+
+    pbs_config::user::save_config(&user_config)?;
+    pbs_config::acl::save_config(&acl_config)?;
+    Ok(())
+}
+
+fn do_delete_token(
+    token_name: Tokenname,
+    userid: &Userid,
+    user_config: &mut SectionConfigData,
+    acl_config: &mut AclTree,
+) -> Result<(), Error> {
     let tokenid = Authid::from((userid.clone(), Some(token_name.clone())));
     let tokenid_string = tokenid.to_string();
-
-    if config.sections.remove(&tokenid_string).is_none() {
+    if user_config.sections.remove(&tokenid_string).is_none() {
         bail!(
             "token '{}' of user '{}' does not exist.",
             token_name.as_str(),
             userid
         );
     }
-
     token_shadow::delete_secret(&tokenid)?;
-
-    pbs_config::user::save_config(&config)?;
+    acl_config.delete_authid(&tokenid);
 
     Ok(())
 }
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 3/6] fix #4382: api: remove permissions and tokens of user on deletion

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/access/user.rs | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index 39cffdba..9bed14a4 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -353,6 +353,7 @@ pub async fn update_user(
 pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error> {
     let _lock = pbs_config::user::lock_config()?;
     let _tfa_lock = crate::config::tfa::write_lock()?;
+    let _acl_lock = pbs_config::acl::lock_config()?;
 
     let (mut config, expected_digest) = pbs_config::user::config()?;
 
@@ -380,6 +381,19 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
         eprintln!("error updating TFA config after deleting user {userid:?} {err}",);
     }
 
+    let user_tokens: Vec<ApiToken> = config
+        .convert_to_typed_array::<ApiToken>("token")?
+        .into_iter()
+        .filter(|token| token.tokenid.user().eq(&userid))
+        .collect();
+
+    let (mut acl_tree, _digest) = pbs_config::acl::config()?;
+    for token in user_tokens {
+        if let Some(name) = token.tokenid.tokenname() {
+            do_delete_token(name.to_owned(), &userid, &mut config, &mut acl_tree)?;
+        }
+    }
+
     Ok(())
 }
 
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 3/6] fix #4382: api: remove permissions and tokens of user on deletion

[Christian Ebner]

one comment inline

On 3/20/25 14:57, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/api2/access/user.rs | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index 39cffdba..9bed14a4 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -353,6 +353,7 @@ pub async fn update_user(
>   pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error> {
>       let _lock = pbs_config::user::lock_config()?;
>       let _tfa_lock = crate::config::tfa::write_lock()?;
> +    let _acl_lock = pbs_config::acl::lock_config()?;
>   
>       let (mut config, expected_digest) = pbs_config::user::config()?;
>   
> @@ -380,6 +381,19 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
>           eprintln!("error updating TFA config after deleting user {userid:?} {err}",);
>       }
>   
> +    let user_tokens: Vec<ApiToken> = config
> +        .convert_to_typed_array::<ApiToken>("token")?
> +        .into_iter()
> +        .filter(|token| token.tokenid.user().eq(&userid))
> +        .collect();
> +
> +    let (mut acl_tree, _digest) = pbs_config::acl::config()?;
> +    for token in user_tokens {
> +        if let Some(name) = token.tokenid.tokenname() {
> +            do_delete_token(name.to_owned(), &userid, &mut config, &mut acl_tree)?;

This removes the tokenid from the user_config and acl_config, but that 
change is never written to the config afterwards? There should be calls 
to the respective save_config implementations just like for 
delete_token. Or am I overlooking something?

The user config and tfa config are already written at this point, so the 
pbs_config::user::save_config(&config) must be moved below this and the 
pbs_config::acl::save_config(&acl_tree) added as well.

> +        }
> +    }
> +
>       Ok(())
>   }
>   



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 3/6] fix #4382: api: remove permissions and tokens of user on deletion

[Hannes Laimer]

On 4/4/25 17:06, Christian Ebner wrote:
> one comment inline
> 
> On 3/20/25 14:57, Hannes Laimer wrote:
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   src/api2/access/user.rs | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
>> index 39cffdba..9bed14a4 100644
>> --- a/src/api2/access/user.rs
>> +++ b/src/api2/access/user.rs
>> @@ -353,6 +353,7 @@ pub async fn update_user(
>>   pub fn delete_user(userid: Userid, digest: Option<String>) -> 
>> Result<(), Error> {
>>       let _lock = pbs_config::user::lock_config()?;
>>       let _tfa_lock = crate::config::tfa::write_lock()?;
>> +    let _acl_lock = pbs_config::acl::lock_config()?;
>>       let (mut config, expected_digest) = pbs_config::user::config()?;
>> @@ -380,6 +381,19 @@ pub fn delete_user(userid: Userid, digest: 
>> Option<String>) -> Result<(), Error>
>>           eprintln!("error updating TFA config after deleting user 
>> {userid:?} {err}",);
>>       }
>> +    let user_tokens: Vec<ApiToken> = config
>> +        .convert_to_typed_array::<ApiToken>("token")?
>> +        .into_iter()
>> +        .filter(|token| token.tokenid.user().eq(&userid))
>> +        .collect();
>> +
>> +    let (mut acl_tree, _digest) = pbs_config::acl::config()?;
>> +    for token in user_tokens {
>> +        if let Some(name) = token.tokenid.tokenname() {
>> +            do_delete_token(name.to_owned(), &userid, &mut config, 
>> &mut acl_tree)?;
> 
> This removes the tokenid from the user_config and acl_config, but that 
> change is never written to the config afterwards? There should be calls 
> to the respective save_config implementations just like for 
> delete_token. Or am I overlooking something?
> 
> The user config and tfa config are already written at this point, so the 
> pbs_config::user::save_config(&config) must be moved below this and the 
> pbs_config::acl::save_config(&acl_tree) added as well.

actually true, I missed that. Not necessarily moving though, having it
in two places is fine, we hold the lock. And given there are some `?`s
I'd rather just add an additional
`pbs_config::user::save_config(&config)`
at the end.

A bit of unlucky timing with my v2, I'll send a v3 fixing this.
Thanks for taking a look!

> 
>> +        }
>> +    }
>> +
>>       Ok(())
>>   }
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox 4/6] pbs-api-types: add REGENERATE_TOKEN_SCHEMA

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/user.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/pbs-api-types/src/user.rs b/pbs-api-types/src/user.rs
index 42f41266..8b63d6c8 100644
--- a/pbs-api-types/src/user.rs
+++ b/pbs-api-types/src/user.rs
@@ -18,6 +18,12 @@ pub const EXPIRE_USER_SCHEMA: Schema = IntegerSchema::new(
 .minimum(0)
 .schema();
 
+pub const REGENERATE_TOKEN_SCHEMA: Schema = BooleanSchema::new(
+    "Regenerate token secret while keeping permissions.",
+)
+.default(false)
+.schema();
+
 pub const FIRST_NAME_SCHEMA: Schema = StringSchema::new("First name.")
     .format(&SINGLE_LINE_COMMENT_FORMAT)
     .min_length(2)
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH proxmox 4/6] pbs-api-types: add REGENERATE_TOKEN_SCHEMA

[Thomas Lamprecht]

Am 20.03.25 um 14:57 schrieb Hannes Laimer:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-api-types/src/user.rs | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
>

Questioning a bit if it's worth to keep every schema for such trivial
flags here or if they should be rather inlined, or alternatively have
the full API params struct as type here instead.

But as we currently already have quite a few super trivial types here
and as it doesn't really hurt and at least provides the description
and the default value available for clients (UIs) I'm fine with this
for now, so:

applied, thanks!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 5/6] fix #3887: api: access: allow secret regeneration

[Hannes Laimer]

... through the token PUT endpoint by adding a new `regenerate` bool
parameter.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/access/user.rs | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index 9bed14a4..1e71bf67 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -14,7 +14,8 @@ use proxmox_tfa::api::TfaConfig;
 use pbs_api_types::{
     ApiToken, Authid, Tokenname, User, UserUpdater, UserWithTokens, Userid, ENABLE_USER_SCHEMA,
     EXPIRE_USER_SCHEMA, PASSWORD_FORMAT, PBS_PASSWORD_SCHEMA, PRIV_PERMISSIONS_MODIFY,
-    PRIV_SYS_AUDIT, PROXMOX_CONFIG_DIGEST_SCHEMA, SINGLE_LINE_COMMENT_SCHEMA,
+    PRIV_SYS_AUDIT, PROXMOX_CONFIG_DIGEST_SCHEMA, REGENERATE_TOKEN_SCHEMA,
+    SINGLE_LINE_COMMENT_SCHEMA,
 };
 use pbs_config::{acl::AclTree, token_shadow, CachedUserInfo};
 
@@ -549,12 +550,26 @@ pub fn generate_token(
                 schema: EXPIRE_USER_SCHEMA,
                 optional: true,
             },
+            regenerate: {
+                schema: REGENERATE_TOKEN_SCHEMA,
+                optional: true,
+            },
             digest: {
                 optional: true,
                 schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
             },
         },
     },
+    returns: {
+        description: "Regenerated secret, if regenerate is set.",
+        properties: {
+            secret: {
+                type: String,
+                optional: true,
+                description: "The new API token secret",
+            },
+        },
+    },
     access: {
         permission: &Permission::Or(&[
             &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
@@ -569,8 +584,9 @@ pub fn update_token(
     comment: Option<String>,
     enable: Option<bool>,
     expire: Option<i64>,
+    regenerate: Option<bool>,
     digest: Option<String>,
-) -> Result<(), Error> {
+) -> Result<Value, Error> {
     let _lock = pbs_config::user::lock_config()?;
 
     let (mut config, expected_digest) = pbs_config::user::config()?;
@@ -602,11 +618,21 @@ pub fn update_token(
         data.expire = if expire > 0 { Some(expire) } else { None };
     }
 
+    let new_secret = if regenerate.unwrap_or_default() {
+        Some(token_shadow::generate_and_set_secret(&tokenid)?)
+    } else {
+        None
+    };
+
     config.set_data(&tokenid_string, "token", &data)?;
 
     pbs_config::user::save_config(&config)?;
 
-    Ok(())
+    if let Some(secret) = new_secret {
+        Ok(json!({"secret": secret}))
+    } else {
+        Ok(Value::Null)
+    }
 }
 
 #[api(
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 6/6] fix #3887: ui: add regenerate token button

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/config/TokenView.js | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/www/config/TokenView.js b/www/config/TokenView.js
index 6282a4d4..340698ad 100644
--- a/www/config/TokenView.js
+++ b/www/config/TokenView.js
@@ -100,6 +100,25 @@ Ext.define('PBS.config.TokenView', {
 	    }).show();
 	},
 
+	regenerateToken: function(_button, _event, record) {
+	    let tokenid = record.data.tokenid;
+	    let user = PBS.Utils.extractTokenUser(tokenid);
+	    let tokenname = PBS.Utils.extractTokenName(tokenid);
+	    Proxmox.Utils.API2Request({
+	        method: 'PUT',
+	        url: `/access/users/${user}/token/${tokenname}`,
+	        params: { 'regenerate': true },
+	        success: function(res, opt) {
+		    Ext.create("PBS.window.TokenShow", {
+			autoShow: true,
+			tokenid: tokenid,
+			secret: res.result.data.secret,
+		    });
+	        },
+	        failure: res => Ext.Msg.alert(gettext("Error"), res.htmlStatus),
+	    });
+	},
+
 	showPermissions: function() {
 	    let me = this;
 	    let view = me.getView();
@@ -174,6 +193,16 @@ Ext.define('PBS.config.TokenView', {
 	    handler: 'showPermissions',
 	    disabled: true,
 	},
+	{
+	    xtype: 'proxmoxButton',
+	    text: gettext('Regenerate'),
+	    handler: 'regenerateToken',
+	    dangerous: true,
+	    confirmMsg: rec => Ext.String.format(
+		gettext("Regenerate the secret of the API token '{0}'? All current use-sites will loose access!"),
+		rec.data.tokenid,
+	    ),
+	},
     ],
 
     viewConfig: {
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel