From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id E1C251FF2AB for ; Wed, 17 Jul 2024 17:35:03 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D59FB3E88E; Wed, 17 Jul 2024 17:35:33 +0200 (CEST) Date: Wed, 17 Jul 2024 17:35:28 +0200 Message-Id: To: "Proxmox VE development discussion" , From: "Max Carrara" Mime-Version: 1.0 X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240712112755.123630-1-l.wagner@proxmox.com> <20240712112755.123630-3-l.wagner@proxmox.com> In-Reply-To: <20240712112755.123630-3-l.wagner@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 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: [pve-devel] [PATCH proxmox v2 02/12] notify: add api for webhook targets X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Fri Jul 12, 2024 at 1:27 PM CEST, Lukas Wagner wrote: > All in all pretty similar to other endpoint APIs. > One thing worth noting is how secrets are handled. We never ever > return the values of previously stored secrets in get_endpoint(s) > calls, but only a list of the names of all secrets. This is needed > to build the UI, where we display all secrets that were set before in > a table. > > For update calls, one is supposed to send all secrets that should be > kept and updated. If the value should be updated, the name and value > is expected, and if the current value should preseved, only the name > is sent. If a secret's name is not present in the updater, it will be > dropped. If 'secret' is present in the 'delete' array, all secrets > will be dropped, apart from those which are also set/preserved in the > same update call. > > Signed-off-by: Lukas Wagner > --- > proxmox-notify/src/api/mod.rs | 20 ++ > proxmox-notify/src/api/webhook.rs | 406 ++++++++++++++++++++++++++++++ > 2 files changed, 426 insertions(+) > create mode 100644 proxmox-notify/src/api/webhook.rs > > diff --git a/proxmox-notify/src/api/mod.rs b/proxmox-notify/src/api/mod.rs > index a7f6261c..7f823bc7 100644 > --- a/proxmox-notify/src/api/mod.rs > +++ b/proxmox-notify/src/api/mod.rs > @@ -15,6 +15,8 @@ pub mod matcher; > pub mod sendmail; > #[cfg(feature = "smtp")] > pub mod smtp; > +#[cfg(feature = "webhook")] > +pub mod webhook; > > // We have our own, local versions of http_err and http_bail, because > // we don't want to wrap the error in anyhow::Error. If we were to do that, > @@ -54,6 +56,9 @@ pub enum EndpointType { > /// Gotify endpoint > #[cfg(feature = "gotify")] > Gotify, > + /// Webhook endpoint > + #[cfg(feature = "webhook")] > + Webhook, > } > > #[api] > @@ -113,6 +118,17 @@ pub fn get_targets(config: &Config) -> Result, HttpError> { > }) > } > > + #[cfg(feature = "webhook")] > + for endpoint in webhook::get_endpoints(config)? { > + targets.push(Target { > + name: endpoint.name, > + origin: endpoint.origin.unwrap_or(Origin::UserCreated), > + endpoint_type: EndpointType::Webhook, > + disable: endpoint.disable, > + comment: endpoint.comment, > + }) > + } > + > Ok(targets) > } > > @@ -145,6 +161,10 @@ fn ensure_endpoint_exists(#[allow(unused)] config: &Config, name: &str) -> Resul > { > exists = exists || smtp::get_endpoint(config, name).is_ok(); > } > + #[cfg(feature = "webhook")] > + { > + exists = exists || webhook::get_endpoint(config, name).is_ok(); > + } > > if !exists { > http_bail!(NOT_FOUND, "endpoint '{name}' does not exist") > diff --git a/proxmox-notify/src/api/webhook.rs b/proxmox-notify/src/api/webhook.rs > new file mode 100644 > index 00000000..b7f17c55 > --- /dev/null > +++ b/proxmox-notify/src/api/webhook.rs > @@ -0,0 +1,406 @@ > +use proxmox_http_error::HttpError; > +use proxmox_schema::property_string::PropertyString; > + > +use crate::api::http_err; > +use crate::endpoints::webhook::{ > + DeleteableWebhookProperty, KeyAndBase64Val, WebhookConfig, WebhookConfigUpdater, > + WebhookPrivateConfig, WEBHOOK_TYPENAME, > +}; > +use crate::{http_bail, Config}; > + > +use super::remove_private_config_entry; > +use super::set_private_config_entry; > + > +/// Get a list of all webhook endpoints. > +/// > +/// The caller is responsible for any needed permission checks. > +/// Returns a list of all webhook endpoints or a `HttpError` if the config is > +/// erroneous (`500 Internal server error`). > +pub fn get_endpoints(config: &Config) -> Result, HttpError> { > + let mut endpoints: Vec = config > + .config > + .convert_to_typed_array(WEBHOOK_TYPENAME) > + .map_err(|e| http_err!(NOT_FOUND, "Could not fetch endpoints: {e}"))?; > + > + for endpoint in &mut endpoints { > + let priv_config: WebhookPrivateConfig = config > + .private_config > + .lookup(WEBHOOK_TYPENAME, &endpoint.name) > + .unwrap_or_default(); > + > + let mut secret_names = Vec::new(); > + for secret in priv_config.secret { > + secret_names.push( > + KeyAndBase64Val { > + name: secret.name.clone(), > + value: None, > + } > + .into(), > + ) > + } > + > + endpoint.secret = secret_names; > + } > + > + Ok(endpoints) > +} > + > +/// Get webhook endpoint with given `name` > +/// > +/// The caller is responsible for any needed permission checks. > +/// Returns the endpoint or a `HttpError` if the endpoint was not found (`404 Not found`). > +pub fn get_endpoint(config: &Config, name: &str) -> Result { > + let mut endpoint: WebhookConfig = config > + .config > + .lookup(WEBHOOK_TYPENAME, name) > + .map_err(|_| http_err!(NOT_FOUND, "endpoint '{name}' not found"))?; > + > + let priv_config: Option = config > + .private_config > + .lookup(WEBHOOK_TYPENAME, &endpoint.name) > + .ok(); > + > + let mut secret_names = Vec::new(); > + if let Some(priv_config) = priv_config { > + for secret in &priv_config.secret { > + secret_names.push( > + KeyAndBase64Val { > + name: secret.name.clone(), > + value: None, > + } > + .into(), > + ); > + } > + } > + > + endpoint.secret = secret_names; > + > + Ok(endpoint) > +} > + > +/// Add a new webhook endpoint. > +/// > +/// The caller is responsible for any needed permission checks. > +/// The caller also responsible for locking the configuration files. > +/// Returns a `HttpError` if: > +/// - an entity with the same name already exists (`400 Bad request`) > +/// - the configuration could not be saved (`500 Internal server error`) > +pub fn add_endpoint( > + config: &mut Config, > + mut endpoint_config: WebhookConfig, > +) -> Result<(), HttpError> { > + super::ensure_unique(config, &endpoint_config.name)?; > + > + let secrets = std::mem::take(&mut endpoint_config.secret); > + > + set_private_config_entry( > + config, > + &WebhookPrivateConfig { > + name: endpoint_config.name.clone(), > + secret: secrets, > + }, > + WEBHOOK_TYPENAME, > + &endpoint_config.name, > + )?; > + > + config > + .config > + .set_data(&endpoint_config.name, WEBHOOK_TYPENAME, &endpoint_config) > + .map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not save endpoint '{}': {e}", > + endpoint_config.name > + ) > + }) > +} > + > +/// Update existing webhook endpoint > +/// > +/// The caller is responsible for any needed permission checks. > +/// The caller also responsible for locking the configuration files. > +/// Returns a `HttpError` if: > +/// - an entity with the same name already exists (`400 Bad request`) > +/// - the configuration could not be saved (`500 Internal server error`) > +pub fn update_endpoint( > + config: &mut Config, > + name: &str, > + config_updater: WebhookConfigUpdater, > + delete: Option<&[DeleteableWebhookProperty]>, > + digest: Option<&[u8]>, > +) -> Result<(), HttpError> { > + super::verify_digest(config, digest)?; > + > + let mut endpoint = get_endpoint(config, name)?; > + endpoint.secret.clear(); > + > + let old_secrets = config > + .private_config > + .lookup::(WEBHOOK_TYPENAME, name) > + .map_err(|err| http_err!(INTERNAL_SERVER_ERROR, "could not read secret config: {err}"))? > + .secret; > + > + if let Some(delete) = delete { > + for deleteable_property in delete { > + match deleteable_property { > + DeleteableWebhookProperty::Comment => endpoint.comment = None, > + DeleteableWebhookProperty::Disable => endpoint.disable = None, > + DeleteableWebhookProperty::Header => endpoint.header = Vec::new(), > + DeleteableWebhookProperty::Body => endpoint.body = None, > + DeleteableWebhookProperty::Secret => { > + set_private_config_entry( > + config, > + &WebhookPrivateConfig { > + name: name.into(), > + secret: Vec::new(), > + }, > + WEBHOOK_TYPENAME, > + name, > + )?; > + } > + } > + } > + } > + > + // Destructuring makes sure we don't forget any members > + let WebhookConfigUpdater { > + url, > + body, > + header, > + method, > + disable, > + comment, > + secret, > + } = config_updater; I didn't know that that kind of destructuring was a thing. Neat! > + > + if let Some(url) = url { > + endpoint.url = url; > + } > + > + if let Some(body) = body { > + endpoint.body = Some(body); > + } > + > + if let Some(header) = header { > + endpoint.header = header; > + } > + > + if let Some(method) = method { > + endpoint.method = method; > + } > + > + if let Some(disable) = disable { > + endpoint.disable = Some(disable); > + } > + > + if let Some(comment) = comment { > + endpoint.comment = Some(comment); > + } > + > + if let Some(secret) = secret { > + let mut new_secrets: Vec> = Vec::new(); > + > + for new_secret in &secret { > + let a = if new_secret.value.is_some() { > + // Make sure it is valid base64 encoded data > + let _ = new_secret.decode_value().map_err(|_| { > + http_err!( > + BAD_REQUEST, > + "secret '{}' does not have valid base64 encoded data", > + new_secret.name > + ) > + })?; > + new_secret.clone() > + } else if let Some(old_secret) = old_secrets.iter().find(|v| v.name == new_secret.name) > + { > + old_secret.clone() > + } else { > + http_bail!(BAD_REQUEST, "secret '{}' not known", new_secret.name); > + }; > + > + if new_secrets.iter().any(|s| s.name == a.name) { > + http_bail!(BAD_REQUEST, "secret '{}' defined multiple times", a.name) > + } > + > + new_secrets.push(a); > + } > + > + set_private_config_entry( > + config, > + &WebhookPrivateConfig { > + name: name.into(), > + secret: new_secrets, > + }, > + WEBHOOK_TYPENAME, > + name, > + )?; > + } > + > + config > + .config > + .set_data(name, WEBHOOK_TYPENAME, &endpoint) > + .map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not save endpoint '{name}': {e}" > + ) > + }) > +} > + > +/// Delete existing webhook endpoint > +/// > +/// The caller is responsible for any needed permission checks. > +/// The caller also responsible for locking the configuration files. > +/// Returns a `HttpError` if: > +/// - the entity does not exist (`404 Not found`) > +/// - the endpoint is still referenced by another entity (`400 Bad request`) > +pub fn delete_endpoint(config: &mut Config, name: &str) -> Result<(), HttpError> { > + // Check if the endpoint exists > + let _ = get_endpoint(config, name)?; > + super::ensure_safe_to_delete(config, name)?; > + > + remove_private_config_entry(config, name)?; > + config.config.sections.remove(name); > + > + Ok(()) > +} > + > +#[cfg(test)] > +mod tests { > + use super::*; > + use crate::{api::test_helpers::empty_config, endpoints::webhook::HttpMethod}; > + > + use base64::encode; > + > + pub fn add_default_webhook_endpoint(config: &mut Config) -> Result<(), HttpError> { > + add_endpoint( > + config, > + WebhookConfig { > + name: "webhook-endpoint".into(), > + method: HttpMethod::Post, > + url: "http://example.com/webhook".into(), > + header: vec![KeyAndBase64Val::new_with_plain_value( > + "Content-Type", > + "application/json", > + ) > + .into()], > + body: Some(encode("this is the body")), > + comment: Some("comment".into()), > + disable: Some(false), > + secret: vec![KeyAndBase64Val::new_with_plain_value("token", "secret").into()], > + ..Default::default() > + }, > + )?; > + > + assert!(get_endpoint(config, "webhook-endpoint").is_ok()); > + Ok(()) > + } > + > + #[test] > + fn test_update_not_existing_returns_error() -> Result<(), HttpError> { > + let mut config = empty_config(); > + > + assert!(update_endpoint(&mut config, "test", Default::default(), None, None).is_err()); > + > + Ok(()) > + } > + > + #[test] > + fn test_update_invalid_digest_returns_error() -> Result<(), HttpError> { > + let mut config = empty_config(); > + add_default_webhook_endpoint(&mut config)?; > + > + assert!(update_endpoint( > + &mut config, > + "webhook-endpoint", > + Default::default(), > + None, > + Some(&[0; 32]) > + ) > + .is_err()); > + > + Ok(()) > + } > + > + #[test] > + fn test_update() -> Result<(), HttpError> { > + let mut config = empty_config(); > + add_default_webhook_endpoint(&mut config)?; > + > + let digest = config.digest; > + > + update_endpoint( > + &mut config, > + "webhook-endpoint", > + WebhookConfigUpdater { > + url: Some("http://new.example.com/webhook".into()), > + comment: Some("newcomment".into()), > + method: Some(HttpMethod::Put), > + // Keep the old token and set a new one > + secret: Some(vec![ > + KeyAndBase64Val::new_with_plain_value("token2", "newsecret").into(), > + KeyAndBase64Val { > + name: "token".into(), > + value: None, > + } > + .into(), > + ]), > + ..Default::default() > + }, > + None, > + Some(&digest), > + )?; > + > + let endpoint = get_endpoint(&config, "webhook-endpoint")?; > + > + assert_eq!(endpoint.url, "http://new.example.com/webhook".to_string()); > + assert_eq!(endpoint.comment, Some("newcomment".to_string())); > + assert!(matches!(endpoint.method, HttpMethod::Put)); > + > + let secrets = config > + .private_config > + .lookup::(WEBHOOK_TYPENAME, "webhook-endpoint") > + .unwrap() > + .secret; > + > + assert_eq!(secrets[1].name, "token".to_string()); > + assert_eq!(secrets[1].value, Some(encode("secret"))); > + assert_eq!(secrets[0].name, "token2".to_string()); > + assert_eq!(secrets[0].value, Some(encode("newsecret"))); > + > + // Test property deletion > + update_endpoint( > + &mut config, > + "webhook-endpoint", > + Default::default(), > + Some(&[DeleteableWebhookProperty::Comment, DeleteableWebhookProperty::Secret]), You missed a `cargo fmt` here ;) > + None, > + )?; > + > + let endpoint = get_endpoint(&config, "webhook-endpoint")?; > + assert_eq!(endpoint.comment, None); > + > + let secrets = config > + .private_config > + .lookup::(WEBHOOK_TYPENAME, "webhook-endpoint") > + .unwrap() > + .secret; > + > + assert!(secrets.is_empty()); > + > + > + Ok(()) > + } > + > + #[test] > + fn test_gotify_endpoint_delete() -> Result<(), HttpError> { > + let mut config = empty_config(); > + add_default_webhook_endpoint(&mut config)?; > + > + delete_endpoint(&mut config, "webhook-endpoint")?; > + assert!(delete_endpoint(&mut config, "webhook-endpoint").is_err()); > + assert_eq!(get_endpoints(&config)?.len(), 0); > + > + Ok(()) > + } > +} _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel