Re: [pdm-devel] [PATCH proxmox 3/3] access-control: add api endpoints for handling tokens

["Shannon Sterz" <s.sterz@proxmox.com>]

On Fri Sep 26, 2025 at 11:14 AM CEST, Fabian Grünbichler wrote:
> some smaller nits/questions below
>
> On September 24, 2025 4:51 pm, Shannon Sterz wrote:
>> by adding most of the logic of token creation, updating and deleting
>> here, users can more easily add api tokens to their api. this requires
>> the api feature.
>>
>> users of these api endpoints are expected to set the `access`
>> parameters according to their needs. by default only super users can
>> access any of them.
>>
>> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
>> ---
>>  proxmox-access-control/Cargo.toml        |   2 +
>>  proxmox-access-control/src/api/tokens.rs | 306 +++++++++++++++++++++++
>>  proxmox-access-control/src/types.rs      |  17 ++
>>  3 files changed, 325 insertions(+)
>>  create mode 100644 proxmox-access-control/src/api/tokens.rs
>>
>> diff --git a/proxmox-access-control/Cargo.toml b/proxmox-access-control/Cargo.toml
>> index dbcbeced..274c08a7 100644
>> --- a/proxmox-access-control/Cargo.toml
>> +++ b/proxmox-access-control/Cargo.toml
>> @@ -31,12 +31,14 @@ proxmox-section-config = { workspace = true, optional = true }
>>  proxmox-shared-memory = { workspace = true, optional = true }
>>  proxmox-sys = { workspace = true, features = [ "crypt" ], optional = true }
>>  proxmox-time = { workspace = true }
>> +proxmox-uuid = { workspace = true, optional = true }
>>
>>  [features]
>>  default = []
>>  api = [
>>      "impl",
>>      "dep:hex",
>> +    "dep:proxmox-uuid"
>>  ]
>>  impl = [
>>      "dep:nix",
>> diff --git a/proxmox-access-control/src/api/tokens.rs b/proxmox-access-control/src/api/tokens.rs
>> new file mode 100644
>> index 00000000..1bde36c8
>> --- /dev/null
>> +++ b/proxmox-access-control/src/api/tokens.rs
>> @@ -0,0 +1,306 @@
>> +//! User Management
>> +
>> +use anyhow::{bail, Error};
>> +use proxmox_config_digest::ConfigDigest;
>> +use proxmox_schema::api_types::COMMENT_SCHEMA;
>> +
>> +use proxmox_auth_api::types::{Authid, Tokenname, Userid};
>> +use proxmox_router::{ApiMethod, RpcEnvironment};
>> +use proxmox_schema::api;
>> +
>> +use crate::token_shadow::{self};
>> +use crate::types::{
>> +    ApiToken, ApiTokenSecret, TokenApiEntry, ENABLE_USER_SCHEMA, EXPIRE_USER_SCHEMA,
>> +};
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            userid: {
>> +                type: Userid,
>> +            },
>> +            "token-name": {
>> +                type: Tokenname,
>> +            },
>> +        },
>> +    },
>> +    returns: { type: ApiToken },
>> +)]
>> +/// Read user's API token metadata
>> +pub fn read_token(
>> +    userid: Userid,
>> +    token_name: Tokenname,
>> +    _info: &ApiMethod,
>> +    rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<ApiToken, Error> {
>> +    let (config, digest) = crate::user::config()?;
>> +
>> +    let tokenid = Authid::from((userid, Some(token_name)));
>> +
>> +    rpcenv["digest"] = hex::encode(digest).into();
>> +    config.lookup("token", &tokenid.to_string())
>> +}
>> +
>> +#[api(
>> +    protected: true,
>> +    input: {
>> +        properties: {
>> +            userid: {
>> +                type: Userid,
>> +            },
>> +            "token-name": {
>> +                type: Tokenname,
>> +            },
>> +            comment: {
>> +                optional: true,
>> +                schema: COMMENT_SCHEMA,
>> +            },
>> +            enable: {
>> +                schema: ENABLE_USER_SCHEMA,
>> +                optional: true,
>> +            },
>> +            expire: {
>> +                schema: EXPIRE_USER_SCHEMA,
>> +                optional: true,
>> +            },
>> +            digest: {
>> +                optional: true,
>> +                type: ConfigDigest,
>> +            },
>> +        },
>> +    },
>> +    returns: { type: ApiTokenSecret },
>> +)]
>> +/// Generate a new API token with given metadata
>> +pub fn generate_token(
>> +    userid: Userid,
>> +    token_name: Tokenname,
>> +    comment: Option<String>,
>> +    enable: Option<bool>,
>> +    expire: Option<i64>,
>> +    digest: Option<ConfigDigest>,
>> +) -> Result<ApiTokenSecret, Error> {
>> +    let _lock = crate::user::lock_config()?;
>> +    let (mut config, config_digest) = crate::user::config()?;
>> +
>> +    config_digest.detect_modification(digest.as_ref())?;
>> +
>> +    let tokenid = Authid::from((userid.clone(), Some(token_name.clone())));
>> +    let tokenid_string = tokenid.to_string();
>> +
>> +    if config.sections.contains_key(&tokenid_string) {
>> +        bail!(
>> +            "token '{}' for user '{userid}' already exists.",
>> +            token_name.as_str(),
>> +        );
>> +    }
>> +
>> +    let secret = format!("{:x}", proxmox_uuid::Uuid::generate());
>
> in PBS, we leave the generation part to token_shadow and don't expose
> setting arbitrary secret, which seems like the "safer" interface..
>
> any reason to not do the same here?

no, i took most of this code from pdm's version (as i wanted it to be
the first user of this implementation. so i wanted to keep that as
similar as possible). i'll adapt this in a v2.

>> +    token_shadow::set_secret(&tokenid, &secret)?;
>> +
>> +    let token = ApiToken {
>> +        tokenid: tokenid.clone(),
>> +        comment,
>> +        enable,
>> +        expire,
>> +    };
>> +
>> +    config.set_data(&tokenid_string, "token", &token)?;
>> +
>> +    crate::user::save_config(&config)?;
>> +
>> +    Ok(ApiTokenSecret { tokenid, secret })
>> +}
>> +
>> +#[api(
>> +    protected: true,
>> +    input: {
>> +        properties: {
>> +            userid: {
>> +                type: Userid,
>> +            },
>> +            "token-name": {
>> +                type: Tokenname,
>> +            },
>> +            comment: {
>> +                optional: true,
>> +                schema: COMMENT_SCHEMA,
>> +            },
>> +            enable: {
>> +                schema: ENABLE_USER_SCHEMA,
>> +                optional: true,
>> +            },
>> +            expire: {
>> +                schema: EXPIRE_USER_SCHEMA,
>> +                optional: true,
>> +            },
>> +            regenerate: {
>> +                description: "Whether the token should be regenerated or not.",
>> +                optional: true,
>> +                type: bool,
>> +                default: false,
>
> this has a schema in PBS as well
>
>> +            },
>
> and we allow deleting comments via `delete` in PBS, should we allow it
> here as well?

yep, makes sense and happened the same way as above. added this in v2.

>> +            digest: {
>> +                optional: true,
>> +                type: ConfigDigest,
>> +            },
>> +        },
>> +    },
>> +    returns: {
>> +        type: ApiTokenSecret,
>> +        optional: true
>> +    }
>> +)]
>> +/// Update user's API token metadata. If regenerate is set to true, the token and it's new secret
>> +/// will be returned.
>> +pub fn update_token(
>> +    userid: Userid,
>> +    token_name: Tokenname,
>> +    comment: Option<String>,
>> +    enable: Option<bool>,
>> +    expire: Option<i64>,
>> +    regenerate: bool,
>> +    digest: Option<ConfigDigest>,
>> +) -> Result<Option<ApiTokenSecret>, Error> {
>> +    let _lock = crate::user::lock_config()?;
>> +
>> +    let (mut config, config_digest) = crate::user::config()?;
>> +    config_digest.detect_modification(digest.as_ref())?;
>> +
>> +    let tokenid = Authid::from((userid, Some(token_name)));
>> +    let tokenid_string = tokenid.to_string();
>> +
>> +    let mut data: ApiToken = config.lookup("token", &tokenid_string)?;
>> +
>> +    if let Some(comment) = comment {
>> +        let comment = comment.trim().to_string();
>> +        if comment.is_empty() {
>> +            data.comment = None;
>> +        } else {
>> +            data.comment = Some(comment);
>> +        }
>> +    }
>> +
>> +    if let Some(enable) = enable {
>> +        data.enable = if enable { None } else { Some(false) };
>> +    }
>> +
>> +    if let Some(expire) = expire {
>> +        data.expire = if expire > 0 { Some(expire) } else { None };
>> +    }
>> +
>> +    let new_secret = if regenerate {
>> +        let secret = format!("{:x}", proxmox_uuid::Uuid::generate());
>> +        crate::token_shadow::set_secret(&tokenid, &secret)?;
>
> see comment above
>
>> +
>> +        Some(ApiTokenSecret { tokenid, secret })
>> +    } else {
>> +        None
>> +    };
>> +
>> +    config.set_data(&tokenid_string, "token", &data)?;
>> +
>> +    crate::user::save_config(&config)?;
>> +
>> +    Ok(new_secret)
>> +}
>> +
>> +#[api(
>> +    protected: true,
>> +    input: {
>> +        properties: {
>> +            userid: {
>> +                type: Userid,
>> +            },
>> +            "token-name": {
>> +                type: Tokenname,
>> +            },
>> +            digest: {
>> +                optional: true,
>> +                type: ConfigDigest,
>> +            },
>> +        },
>> +    },
>> +)]
>> +/// Delete a user's API token
>> +pub fn delete_token(
>> +    userid: Userid,
>> +    token_name: Tokenname,
>> +    digest: Option<ConfigDigest>,
>> +) -> Result<(), Error> {
>> +    let _lock = crate::user::lock_config()?;
>> +
>> +    let (mut config, config_digest) = crate::user::config()?;
>> +    config_digest.detect_modification(digest.as_ref())?;
>> +
>> +    let tokenid = Authid::from((userid.clone(), Some(token_name.clone())));
>> +    let tokenid_string = tokenid.to_string();
>> +
>> +    match config.sections.get(&tokenid_string) {
>> +        Some(_) => {
>> +            config.sections.remove(&tokenid_string);
>> +        }
>> +        None => bail!(
>> +            "token '{}' of user '{userid}' does not exist.",
>> +            token_name.as_str(),
>> +        ),
>> +    }
>> +
>> +    token_shadow::delete_secret(&tokenid)?;
>> +
>> +    crate::user::save_config(&config)?;
>
> should we clean up ACL entries for that token here as well?

yep, makes sense, will be in the v2

>> +
>> +    Ok(())
>> +}
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            userid: {
>> +                type: Userid,
>> +            },
>> +        },
>> +    },
>> +    returns: {
>> +        description: "List user's API tokens (with config digest).",
>> +        type: Array,
>> +        items: { type: TokenApiEntry },
>> +    },
>> +)]
>> +/// List user's API tokens
>> +pub fn list_tokens(
>> +    userid: Userid,
>> +    _info: &ApiMethod,
>> +    rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<Vec<TokenApiEntry>, Error> {
>> +    let (config, digest) = crate::user::config()?;
>> +
>> +    let list: Vec<ApiToken> = config.convert_to_typed_array("token")?;
>> +
>> +    rpcenv["digest"] = hex::encode(digest).into();
>> +
>> +    let filter_by_owner = |token: ApiToken| {
>> +        if token.tokenid.is_token() && token.tokenid.user() == &userid {
>> +            let token_name = token.tokenid.tokenname().unwrap().to_owned();
>> +            Some(TokenApiEntry { token_name, token })
>> +        } else {
>> +            None
>> +        }
>> +    };
>> +
>> +    let res = list.into_iter().filter_map(filter_by_owner).collect();
>> +
>> +    Ok(res)
>> +}
>> +
>> +/*const TOKEN_ITEM_ROUTER: Router = Router::new()
>> +    .get(&API_METHOD_READ_TOKEN)
>> +    .put(&API_METHOD_UPDATE_TOKEN)
>> +    .post(&API_METHOD_GENERATE_TOKEN)
>> +    .delete(&API_METHOD_DELETE_TOKEN);
>> +
>> +const TOKEN_ROUTER: Router = Router::new()
>> +    .get(&API_METHOD_LIST_TOKENS)
>> +    .match_all("token-name", &TOKEN_ITEM_ROUTER);
>> +
>> +const USER_SUBDIRS: SubdirMap = &[("token", &TOKEN_ROUTER)];*/
>> diff --git a/proxmox-access-control/src/types.rs b/proxmox-access-control/src/types.rs
>> index a146700d..2771f3b8 100644
>> --- a/proxmox-access-control/src/types.rs
>> +++ b/proxmox-access-control/src/types.rs
>> @@ -154,9 +154,26 @@ impl ApiToken {
>>  pub struct ApiTokenSecret {
>>      pub tokenid: Authid,
>>      /// The secret associated with the token.
>> +    // rename to `value` as that is what it is called in the api
>> +    #[serde(rename = "value")]
>>      pub secret: String,
>>  }
>>
>> +#[api(
>> +    properties: {
>> +        token: { type: ApiToken },
>> +    }
>> +)]
>> +#[derive(Serialize, Deserialize)]
>> +#[serde(rename_all = "kebab-case")]
>> +/// A Token Entry that contains the token-name
>> +pub struct TokenApiEntry {
>> +    /// The Token name
>> +    pub token_name: Tokenname,
>> +    #[serde(flatten)]
>> +    pub token: ApiToken,
>> +}
>> +
>>  #[api(
>>      properties: {
>>          userid: {
>> --
>> 2.47.3
>>
>>
>>
>> _______________________________________________
>> pdm-devel mailing list
>> pdm-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
>>
>>
>>
>
>
> _______________________________________________
> pdm-devel mailing list
> pdm-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel



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