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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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?

> +    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?

> +            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?

> +
> +    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