From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 101FA1FF16B for ; Fri, 26 Sep 2025 11:14:01 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 06E87BBE8; Fri, 26 Sep 2025 11:14:34 +0200 (CEST) Date: Fri, 26 Sep 2025 11:14:27 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Datacenter Manager development discussion References: <20250924145137.407070-1-s.sterz@proxmox.com> <20250924145137.407070-4-s.sterz@proxmox.com> In-Reply-To: <20250924145137.407070-4-s.sterz@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1758877128.ur2auc56hh.astroid@yuna.none> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1758878055267 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pdm-devel] [PATCH proxmox 3/3] access-control: add api endpoints for handling tokens X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" 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 > --- > 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 { > + 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, > + enable: Option, > + expire: Option, > + digest: Option, > +) -> Result { > + 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, > + enable: Option, > + expire: Option, > + regenerate: bool, > + digest: Option, > +) -> 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, 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, > +) -> 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, Error> { > + let (config, digest) = crate::user::config()?; > + > + let list: Vec = 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