[pbs-devel] [PATCH proxmox-backup v3 0/4] close #3612: allow config of SSL cipher-suites for proxy

[pbs-devel] [PATCH proxmox-backup v3 0/4] close #3612: allow config of SSL cipher-suites for proxy

[Hannes Laimer]

Cannot be configured in the WebUI, only through proxmox-backup-manager,
api or in the config file directly(not recommended). For changes to take
effect the proxy has to be restarted.

Since the string can be rather long and I assume most of the time the
defaults are used, it is not in the WebUI.

v2:
  - allow setting for TLSv1.3 and TLS <= 1.2 individually

v3:
  - add proper regex

Note: if the 2nd patch should be applied, it should probably be squashed
with the 1st one.

Hannes Laimer (4):
  config: add tls ciphers to NodeConfig
  pbs-api-types: make regex for ciphers more strict
  proxy: use ciphers from config if set
  api2: make tls ciphers updatable

 pbs-api-types/src/lib.rs        | 41 +++++++++++++++++++++++++++++++++
 src/api2/node/config.rs         |  8 +++++++
 src/bin/proxmox-backup-proxy.rs | 10 ++++++++
 src/config/node.rs              | 26 ++++++++++++++++++++-
 4 files changed, 84 insertions(+), 1 deletion(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 1/4] config: add tls ciphers to NodeConfig

[Hannes Laimer]

for TLS 1.3 and for TLS <= 1.2

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/lib.rs | 15 +++++++++++++++
 src/config/node.rs       | 26 +++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 0a0dd33d..b4882064 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -124,6 +124,10 @@ const_regex! {
 
     pub FINGERPRINT_SHA256_REGEX = r"^(?:[0-9a-fA-F][0-9a-fA-F])(?::[0-9a-fA-F][0-9a-fA-F]){31}$";
 
+    pub OPENSSL_CIPHER_LIST_REGEX = r"^[A-Za-z0-9!\-+=@, :]+$";
+    
+    pub TLS_CIPHERSUITE_LIST_REGEX = r"^[A-Za-z0-9_:]+$";
+
     /// Regex for safe identifiers.
     ///
     /// This
@@ -160,6 +164,9 @@ pub const BLOCKDEVICE_NAME_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&B
 pub const SUBSCRIPTION_KEY_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&SUBSCRIPTION_KEY_REGEX);
 pub const SYSTEMD_DATETIME_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&SYSTEMD_DATETIME_REGEX);
 pub const HOSTNAME_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&HOSTNAME_REGEX);
+pub const OPENSSL_CIPHER_LIST_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&OPENSSL_CIPHER_LIST_REGEX);
+pub const TLS_CIPHERSUITE_LIST_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&TLS_CIPHERSUITE_LIST_REGEX);
+
 
 pub const DNS_ALIAS_FORMAT: ApiStringFormat =
     ApiStringFormat::Pattern(&DNS_ALIAS_REGEX);
@@ -189,6 +196,14 @@ pub const HOSTNAME_SCHEMA: Schema = StringSchema::new("Hostname (as defined in R
     .format(&HOSTNAME_FORMAT)
     .schema();
 
+pub const OPENSSL_CIPHER_LIST_SCHEMA: Schema = StringSchema::new("OpenSSL cipher string list used by the proxy for TLS <= 1.2")
+    .format(&OPENSSL_CIPHER_LIST_FORMAT)
+    .schema();
+
+pub const TLS_CIPHERSUITE_LIST_SCHEMA: Schema = StringSchema::new("TLS ciphersuites list used by the proxy for TLSv1.3")
+    .format(&TLS_CIPHERSUITE_LIST_FORMAT)
+    .schema();
+
 pub const DNS_NAME_FORMAT: ApiStringFormat =
     ApiStringFormat::Pattern(&DNS_NAME_REGEX);
 
diff --git a/src/config/node.rs b/src/config/node.rs
index 4f2ab029..74db8fe3 100644
--- a/src/config/node.rs
+++ b/src/config/node.rs
@@ -1,5 +1,6 @@
 use std::collections::HashSet;
 
+use openssl::ssl::{SslAcceptor, SslMethod};
 use anyhow::{bail, Error};
 use serde::{Deserialize, Serialize};
 
@@ -7,7 +8,7 @@ use proxmox_schema::{api, ApiStringFormat, ApiType, Updater};
 
 use proxmox_http::ProxyConfig;
 
-use pbs_api_types::EMAIL_SCHEMA;
+use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHER_LIST_SCHEMA, TLS_CIPHERSUITE_LIST_SCHEMA};
 use pbs_buildcfg::configdir;
 use pbs_config::{open_backup_lockfile, BackupLockGuard};
 
@@ -91,6 +92,14 @@ pub struct AcmeConfig {
             schema: EMAIL_SCHEMA,
             optional: true,
         },
+        "ciphers-tls3": {
+            schema: TLS_CIPHERSUITE_LIST_SCHEMA,
+            optional: true,
+        },
+        "ciphers-tls2": {
+            schema: OPENSSL_CIPHER_LIST_SCHEMA,
+            optional: true,
+        },
     },
 )]
 #[derive(Deserialize, Serialize, Updater)]
@@ -121,6 +130,14 @@ pub struct NodeConfig {
     
     #[serde(skip_serializing_if = "Option::is_none")]
     pub email_from: Option<String>,
+
+    /// List of SSL ciphers for tls 1.3 that will be used by the proxy. (Proxy has to be restarted for changes to take effect)
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub ciphers_tls3: Option<String>,
+    
+    /// List of SSL ciphers for tls <= 1.2 that will be used by the proxy. (Proxy has to be restarted for changes to take effect)
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub ciphers_tls2: Option<String>,
 }
 
 impl NodeConfig {
@@ -172,6 +189,13 @@ impl NodeConfig {
                 bail!("duplicate domain '{}' in ACME config", domain.domain);
             }
         }
+        let mut dummy_acceptor = SslAcceptor::mozilla_intermediate_v5(SslMethod::tls()).unwrap();
+        if let Some(ciphers) = self.ciphers_tls3.as_deref() {
+            dummy_acceptor.set_ciphersuites(ciphers)?;
+        }
+        if let Some(ciphers) = self.ciphers_tls2.as_deref() {
+            dummy_acceptor.set_cipher_list(ciphers)?;
+        }
 
         Ok(())
     }
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup v3 1/4] config: add tls ciphers to NodeConfig

[Dietmar Maurer]

Why do you use a different naming scheme here?

OPENSSL_CIPHER_LIST_REGEX vs. TLS_CIPHERSUITE_LIST_REGEX

What about the following:

TLS1_2_CIPHERSUITE_LIST_REGEX TLS1_3_CIPHERSUITE_LIST_REGEX

And why do the have different syntax at all??

Also, AFAIK there is no TLS version 2 or version 3 (its 1.2 and 1.3). So 
"ciphers-tls2" and "ciphers-tls3" are a bit misleading.

Apache only has a single config called "SSLCipherSuite". Why do we need 
two different configs?

nginx also use a single config "ssl_ciphers"


On 1/8/22 08:08, Hannes Laimer wrote:
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 0a0dd33d..b4882064 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -124,6 +124,10 @@ const_regex! {
>   
>       pub FINGERPRINT_SHA256_REGEX = r"^(?:[0-9a-fA-F][0-9a-fA-F])(?::[0-9a-fA-F][0-9a-fA-F]){31}$";
>   
> +    pub OPENSSL_CIPHER_LIST_REGEX = r"^[A-Za-z0-9!\-+=@, :]+$";
> +
> +    pub TLS_CIPHERSUITE_LIST_REGEX = r"^[A-Za-z0-9_:]+$";
> +

Re: [pbs-devel] [PATCH proxmox-backup v3 1/4] config: add tls ciphers to NodeConfig

[Fabian Grünbichler]

On January 10, 2022 6:40 am, Dietmar Maurer wrote:
> Why do you use a different naming scheme here?
> 
> OPENSSL_CIPHER_LIST_REGEX vs. TLS_CIPHERSUITE_LIST_REGEX
> 
> What about the following:
> 
> TLS1_2_CIPHERSUITE_LIST_REGEX TLS1_3_CIPHERSUITE_LIST_REGEX
> 
> And why do the have different syntax at all??
> 
> Also, AFAIK there is no TLS version 2 or version 3 (its 1.2 and 1.3). So 
> "ciphers-tls2" and "ciphers-tls3" are a bit misleading.
> 
> Apache only has a single config called "SSLCipherSuite". Why do we need 
> two different configs?

we could do it like Apache[0] and have an optional prefix before the 
cipher spec indicating whether its for <= TLS 1.2 or for TLS 1.3, and 
allow specifying it multiple times - but that is not really less 
confusing than two parameters?

the contents are incompatible, openssl has different methods for setting 
them (with different semantics!), so having two parameters doesn't seem 
too bad..

I'll follow whatever the result of this discussion here is for my perl 
(PVE/PMG) series in any case ;)

> nginx also use a single config "ssl_ciphers"

but doesn't support TLS 1.3 cipher suites AFAICT (requiring a workaround 
of injecting them via a 'set arbitrary openssl.cnf parameters' nginx 
config parameter![1]).

0: https://httpd.apache.org/docs/trunk/mod/mod_ssl.html#sslciphersuite
1: https://trac.nginx.org/nginx/ticket/1529

> On 1/8/22 08:08, Hannes Laimer wrote:
>> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
>> index 0a0dd33d..b4882064 100644
>> --- a/pbs-api-types/src/lib.rs
>> +++ b/pbs-api-types/src/lib.rs
>> @@ -124,6 +124,10 @@ const_regex! {
>>   
>>       pub FINGERPRINT_SHA256_REGEX = r"^(?:[0-9a-fA-F][0-9a-fA-F])(?::[0-9a-fA-F][0-9a-fA-F]){31}$";
>>   
>> +    pub OPENSSL_CIPHER_LIST_REGEX = r"^[A-Za-z0-9!\-+=@, :]+$";
>> +
>> +    pub TLS_CIPHERSUITE_LIST_REGEX = r"^[A-Za-z0-9_:]+$";
>> +
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

[pbs-devel] [PATCH proxmox-backup v3 2/4][optional] pbs-api-types: make regex for ciphers more strict

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
if applied should be squashed with patch 0001

 pbs-api-types/src/lib.rs | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index b4882064..8ad9f23a 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -100,6 +100,20 @@ mod local_macros {
     macro_rules! DNS_ALIAS_NAME {
         () => (concat!(r"(?:(?:", DNS_ALIAS_LABEL!() , r"\.)*", DNS_ALIAS_LABEL!(), ")"))
     }
+    macro_rules! TLS_CIPHERSUITE_RE { 
+        () => (
+            r"TLS_AES_256_GCM_SHA384|TLS_CHACHA20_POLY1305_SHA256|TLS_AES_128_GCM_SHA256|TLS_AES_128_CCM_8_SHA256|TLS_AES_128_CCM_SHA256"
+        ) 
+    }
+    macro_rules! OPENSSL_CIPHER_STRING_RE { 
+        () => (concat!(
+            r"([!\-+]?(COMPLEMENTOFDEFAULT|ALL|COMPLEMENTOFALL|HIGH|MEDIUM|LOW|[ae]?NULL|[ka]?RSA|",
+            "kDH[rdE]?|kEDH|DHE?|EDH|ADH|kEECDH|kECDHE|ECDH|ECDHE|EECDH|AECDH|a?DSS|aDH|a?ECDSA|",
+            "SSLv3|AES(128|256)?|GCM|AESGCM|AESCCM|AESCCM8|ARIA(128|256)?|CAMELLIA(128|256)?|",
+            "CHACHA20|3?DES|RC[24]|IDEA|SEED|MD5|SHA(1|256|384)?|aGOST(01)?|kGOST|GOST94|GOST89MAC|",
+            "[ak]?PSK|kECDHEPSK|kDHEPSK|kRSAPSK|SUITEB(128|128ONLY|192)?|CBC3?|POLY1305))+"
+        )) 
+    }
 }
 
 const_regex! {
@@ -124,9 +138,21 @@ const_regex! {
 
     pub FINGERPRINT_SHA256_REGEX = r"^(?:[0-9a-fA-F][0-9a-fA-F])(?::[0-9a-fA-F][0-9a-fA-F]){31}$";
 
-    pub OPENSSL_CIPHER_LIST_REGEX = r"^[A-Za-z0-9!\-+=@, :]+$";
+    pub OPENSSL_CIPHER_LIST_REGEX = concat!(
+        r"^((",
+        OPENSSL_CIPHER_STRING_RE!(),
+        ")([: ,](",
+        OPENSSL_CIPHER_STRING_RE!(),
+        "))*)$"
+    );
     
-    pub TLS_CIPHERSUITE_LIST_REGEX = r"^[A-Za-z0-9_:]+$";
+    pub TLS_CIPHERSUITE_LIST_REGEX = concat!(
+        r"^((",
+        TLS_CIPHERSUITE_RE!(),
+        ")(:(",
+        TLS_CIPHERSUITE_RE!(),
+        "))*)$"
+    );
 
     /// Regex for safe identifiers.
     ///
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 3/4] proxy: use ciphers from config if set

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/bin/proxmox-backup-proxy.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 287cff0a..2c6505a6 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -343,7 +343,17 @@ fn make_tls_acceptor() -> Result<SslAcceptor, Error> {
     let key_path = configdir!("/proxy.key");
     let cert_path = configdir!("/proxy.pem");
 
+    let (config, _) = proxmox_backup::config::node::config()?;
+    let ciphers_tls3 = config.ciphers_tls3;
+    let ciphers_tls2 = config.ciphers_tls2;
+
     let mut acceptor = SslAcceptor::mozilla_intermediate_v5(SslMethod::tls()).unwrap();
+    if let Some(ciphers) = ciphers_tls3.as_deref() {
+        acceptor.set_ciphersuites(ciphers)?;
+    }
+    if let Some(ciphers) = ciphers_tls2.as_deref() {
+        acceptor.set_cipher_list(ciphers)?;
+    }
     acceptor.set_private_key_file(key_path, SslFiletype::PEM)
         .map_err(|err| format_err!("unable to read proxy key {} - {}", key_path, err))?;
     acceptor.set_certificate_chain_file(cert_path)
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 4/4] api2: make tls ciphers updatable

[Hannes Laimer]

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

diff --git a/src/api2/node/config.rs b/src/api2/node/config.rs
index 3f060981..f5ac9eae 100644
--- a/src/api2/node/config.rs
+++ b/src/api2/node/config.rs
@@ -56,6 +56,10 @@ pub enum DeletableProperty {
     http_proxy,
     /// Delete the email-from property.
     email_from,
+    /// Delete the ciphers-tls3 property.
+    ciphers_tls3,
+    /// Delete the ciphers-tls2 property.
+    ciphers_tls2,
 }
 
 #[api(
@@ -113,6 +117,8 @@ pub fn update_node_config(
                 DeletableProperty::acmedomain4 => { config.acmedomain4 = None; },
                 DeletableProperty::http_proxy => { config.http_proxy = None; },
                 DeletableProperty::email_from => { config.email_from = None; },
+                DeletableProperty::ciphers_tls3 => { config.ciphers_tls3 = None; },
+                DeletableProperty::ciphers_tls2 => { config.ciphers_tls2 = None; },
             }
         }
     }
@@ -125,6 +131,8 @@ pub fn update_node_config(
     if update.acmedomain4.is_some() { config.acmedomain4 = update.acmedomain4; }
     if update.http_proxy.is_some() { config.http_proxy = update.http_proxy; }
     if update.email_from.is_some() { config.email_from = update.email_from; }
+    if update.ciphers_tls3.is_some() { config.ciphers_tls3 = update.ciphers_tls3; }
+    if update.ciphers_tls2.is_some() { config.ciphers_tls2 = update.ciphers_tls2; }
 
     crate::config::node::save_config(&config)?;
 
-- 
2.30.2