[pve-devel] [PATCH proxmox-offline-mirror 0/2] improve GPG verification

[pve-devel] [PATCH proxmox-offline-mirror 0/2] improve GPG verification

[Fabian Grünbichler]

this patch series improves the error messages when encountering an issue with
GPG verification, and offers an (opt-in) escape hatch for allowing weak
cryptographic algorithms if a repository still needs those.

Fabian Grünbichler (2):
  improve GPG error messages
  fix #4632: allow escape hatches for legacy repositories

 src/bin/proxmox-offline-mirror.rs             |  2 +
 src/bin/proxmox_offline_mirror_cmds/config.rs |  4 ++
 src/config.rs                                 | 42 +++++++++++++-
 src/helpers/verifier.rs                       | 55 +++++++++++++++++--
 src/mirror.rs                                 | 17 +++++-
 5 files changed, 111 insertions(+), 9 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH proxmox-offline-mirror 1/2] improve GPG error messages

[Fabian Grünbichler]

e.g., when encoutering a key that is self-signed with SHA-1 (which is not that
uncommon for non-distro repositories that have an old key), instead of the
following:

----8<----
Fetching Release/Release.gpg files
-> GET 'https://download.ceph.com/debian-quincy//dists/bullseye/Release.gpg'..
-> GET 'https://download.ceph.com/debian-quincy//dists/bullseye/Release'..
Verifying 'Release(.gpg)' signature using provided repository key..
	Subkey of 08B73419AC32B4E966C1A330E84AC2C0460F3994 not bound: No binding signature at time 2022-10-17T22:41:10Z
Error: encountered 1 error(s)
---->8----

which only gives us a rought idea that something is wrong with a key signature,
we now get the following:

----8<----
Fetching Release/Release.gpg files
-> GET 'https://download.ceph.com/debian-quincy//dists/bullseye/Release.gpg'..
-> GET 'https://download.ceph.com/debian-quincy//dists/bullseye/Release'..
Verifying 'Release(.gpg)' signature using provided repository key..

Subkey of 08B73419AC32B4E966C1A330E84AC2C0460F3994 not bound: No binding signature at time 2022-10-17T22:41:10Z
Caused by:
	0: Policy rejected non-revocation signature (PositiveCertification) requiring second pre-image resistance
	1: SHA1 is not considered secure since 2023-02-01T00:00:00Z

Error: No valid signature found.
---->8----

which shows us that the key signature was rejected because it's SHA-1, and the
(default and currently only) policy doesn't allow that (anymore).

the output is also improved in case the Release file is signed multiple times
and none of the signatures are accepted.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/helpers/verifier.rs | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/src/helpers/verifier.rs b/src/helpers/verifier.rs
index 57bfd1b..131bccd 100644
--- a/src/helpers/verifier.rs
+++ b/src/helpers/verifier.rs
@@ -3,8 +3,8 @@ use anyhow::{bail, Error};
 use sequoia_openpgp::{
     parse::{
         stream::{
-            DetachedVerifierBuilder, MessageLayer, MessageStructure, VerificationHelper,
-            VerifierBuilder,
+            DetachedVerifierBuilder, MessageLayer, MessageStructure, VerificationError,
+            VerificationHelper, VerifierBuilder,
         },
         Parse,
     },
@@ -53,10 +53,35 @@ impl<'a> VerificationHelper for Helper<'a> {
         if good {
             Ok(()) // Good signature.
         } else {
-            for err in &errors {
-                eprintln!("\t{err}");
+            if errors.len() > 1 {
+                eprintln!("\nEncountered {} errors:", errors.len());
             }
-            Err(anyhow::anyhow!("encountered {} error(s)", errors.len()))
+
+            for (n, err) in errors.iter().enumerate() {
+                if errors.len() > 1 {
+                    eprintln!("\nSignature #{n}: {err}");
+                } else {
+                    eprintln!("\n{err}");
+                }
+                match err {
+                    VerificationError::MalformedSignature { error, .. }
+                    | VerificationError::UnboundKey { error, .. }
+                    | VerificationError::BadKey { error, .. }
+                    | VerificationError::BadSignature { error, .. } => {
+                        let mut cause = error.chain();
+                        if cause.len() > 1 {
+                            cause.next(); // already included in `err` above
+                            eprintln!("Caused by:");
+                            for (n, e) in cause.enumerate() {
+                                eprintln!("\t{n}: {e}");
+                            }
+                        }
+                    }
+                    VerificationError::MissingKey { .. } => {} // doesn't contain a cause
+                };
+            }
+            eprintln!();
+            Err(anyhow::anyhow!("No valid signature found."))
         }
     }
 }
-- 
2.30.2

[pve-devel] [PATCH proxmox-offline-mirror 2/2] fix #4632: allow escape hatches for legacy repositories

[Fabian Grünbichler]

there are still repositories out there that are using things like DSA/RSA-1024
and SHA1, so let's allow POM users to opt into accepting those insecure
cryptographic parameters, but keep the default settings secure.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/bin/proxmox-offline-mirror.rs             |  2 +
 src/bin/proxmox_offline_mirror_cmds/config.rs |  4 ++
 src/config.rs                                 | 42 ++++++++++++++++++-
 src/helpers/verifier.rs                       | 20 ++++++++-
 src/mirror.rs                                 | 17 +++++++-
 5 files changed, 81 insertions(+), 4 deletions(-)

diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
index 3af33bb..bec366a 100644
--- a/src/bin/proxmox-offline-mirror.rs
+++ b/src/bin/proxmox-offline-mirror.rs
@@ -423,6 +423,7 @@ fn action_add_mirror(config: &SectionConfigData) -> Result<Vec<MirrorConfig>, Er
                 use_subscription: None,
                 ignore_errors: false,
                 skip,
+                weak_crypto: None,
             });
         }
     }
@@ -438,6 +439,7 @@ fn action_add_mirror(config: &SectionConfigData) -> Result<Vec<MirrorConfig>, Er
         use_subscription,
         ignore_errors: false,
         skip,
+        weak_crypto: None,
     };
 
     configs.push(main_config);
diff --git a/src/bin/proxmox_offline_mirror_cmds/config.rs b/src/bin/proxmox_offline_mirror_cmds/config.rs
index 3ebf4ad..696da11 100644
--- a/src/bin/proxmox_offline_mirror_cmds/config.rs
+++ b/src/bin/proxmox_offline_mirror_cmds/config.rs
@@ -274,6 +274,10 @@ pub fn update_mirror(
         data.skip.skip_sections = Some(skip_sections);
     }
 
+    if let Some(weak_crypto) = update.weak_crypto {
+        data.weak_crypto = Some(weak_crypto);
+    }
+
     config.set_data(&id, "mirror", &data)?;
     proxmox_offline_mirror::config::save_config(&config_file, &config)?;
 
diff --git a/src/config.rs b/src/config.rs
index 39b1193..0e19c77 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -5,7 +5,7 @@ use lazy_static::lazy_static;
 use proxmox_subscription::{sign::ServerBlob, SubscriptionInfo};
 use serde::{Deserialize, Serialize};
 
-use proxmox_schema::{api, ApiType, Schema, Updater};
+use proxmox_schema::{api, ApiStringFormat, ApiType, Schema, Updater};
 use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
 use proxmox_sys::fs::{replace_file, CreateOptions};
 
@@ -46,6 +46,38 @@ pub struct SkipConfig {
     pub skip_packages: Option<Vec<String>>,
 }
 
+#[api(
+    properties: {
+        "allow-sha1": {
+            type: bool,
+            default: false,
+            optional: true,
+        },
+        "min-dsa-key-size": {
+            type: u64,
+            optional: true,
+        },
+        "min-rsa-key-size": {
+            type: u64,
+            optional: true,
+        },
+    },
+)]
+#[derive(Default, Serialize, Deserialize, Updater, Clone, Debug)]
+#[serde(rename_all = "kebab-case")]
+/// Weak Cryptography Configuration
+pub struct WeakCryptoConfig {
+    /// Whether to allow SHA-1 based signatures
+    #[serde(default)]
+    pub allow_sha1: bool,
+    /// Whether to lower the key size cutoff for DSA-based signatures
+    #[serde(default)]
+    pub min_dsa_key_size: Option<u64>,
+    /// Whether to lower the key size cutoff for RSA-based signatures
+    #[serde(default)]
+    pub min_rsa_key_size: Option<u64>,
+}
+
 #[api(
     properties: {
         id: {
@@ -81,6 +113,11 @@ pub struct SkipConfig {
         "skip": {
             type: SkipConfig,
         },
+        "weak-crypto": {
+            type: String,
+            optional: true,
+            format: &ApiStringFormat::PropertyString(&WeakCryptoConfig::API_SCHEMA),
+        },
     }
 )]
 #[derive(Clone, Debug, Serialize, Deserialize, Updater)]
@@ -111,6 +148,9 @@ pub struct MirrorConfig {
     /// Skip package files using these criteria
     #[serde(default, flatten)]
     pub skip: SkipConfig,
+    /// Whether to allow using weak cryptography algorithms or parameters, deviating from the default policy.
+    #[serde(default)]
+    pub weak_crypto: Option<String>,
 }
 
 #[api(
diff --git a/src/helpers/verifier.rs b/src/helpers/verifier.rs
index 131bccd..e38ef40 100644
--- a/src/helpers/verifier.rs
+++ b/src/helpers/verifier.rs
@@ -9,10 +9,13 @@ use sequoia_openpgp::{
         Parse,
     },
     policy::StandardPolicy,
+    types::HashAlgorithm,
     Cert, KeyHandle,
 };
 use std::io;
 
+use crate::config::WeakCryptoConfig;
+
 struct Helper<'a> {
     cert: &'a Cert,
 }
@@ -91,10 +94,25 @@ pub(crate) fn verify_signature<'msg>(
     msg: &'msg [u8],
     key: &[u8],
     detached_sig: Option<&[u8]>,
+    weak_crypto: &WeakCryptoConfig,
 ) -> Result<Vec<u8>, Error> {
     let cert = Cert::from_bytes(key)?;
 
-    let policy = StandardPolicy::new();
+    let mut policy = StandardPolicy::new();
+    if weak_crypto.allow_sha1 {
+        policy.accept_hash(HashAlgorithm::SHA1);
+    }
+    if let Some(min_dsa) = weak_crypto.min_dsa_key_size {
+        if min_dsa <= 1024 {
+            policy.accept_asymmetric_algo(sequoia_openpgp::policy::AsymmetricAlgorithm::DSA1024);
+        }
+    }
+    if let Some(min_rsa) = weak_crypto.min_dsa_key_size {
+        if min_rsa <= 1024 {
+            policy.accept_asymmetric_algo(sequoia_openpgp::policy::AsymmetricAlgorithm::RSA1024);
+        }
+    }
+
     let helper = Helper { cert: &cert };
 
     let verified = if let Some(sig) = detached_sig {
diff --git a/src/mirror.rs b/src/mirror.rs
index 0dc0751..3766f23 100644
--- a/src/mirror.rs
+++ b/src/mirror.rs
@@ -10,10 +10,11 @@ use flate2::bufread::GzDecoder;
 use globset::{Glob, GlobSet, GlobSetBuilder};
 use nix::libc;
 use proxmox_http::{client::sync::Client, HttpClient, HttpOptions, ProxyConfig};
+use proxmox_schema::{ApiType, Schema};
 use proxmox_sys::fs::file_get_contents;
 
 use crate::{
-    config::{MirrorConfig, SkipConfig, SubscriptionKey},
+    config::{MirrorConfig, SkipConfig, SubscriptionKey, WeakCryptoConfig},
     convert_repo_line,
     pool::Pool,
     types::{Diff, Snapshot, SNAPSHOT_REGEX},
@@ -50,6 +51,7 @@ struct ParsedMirrorConfig {
     pub client: Client,
     pub ignore_errors: bool,
     pub skip: SkipConfig,
+    pub weak_crypto: WeakCryptoConfig,
 }
 
 impl TryInto<ParsedMirrorConfig> for MirrorConfig {
@@ -72,6 +74,15 @@ impl TryInto<ParsedMirrorConfig> for MirrorConfig {
 
         let client = Client::new(options);
 
+        let weak_crypto = match self.weak_crypto {
+            Some(property_string) => {
+                let value = (WeakCryptoConfig::API_SCHEMA as Schema)
+                    .parse_property_string(&property_string)?;
+                serde_json::from_value(value)?
+            }
+            None => WeakCryptoConfig::default(),
+        };
+
         Ok(ParsedMirrorConfig {
             repository,
             architectures: self.architectures,
@@ -83,6 +94,7 @@ impl TryInto<ParsedMirrorConfig> for MirrorConfig {
             client,
             ignore_errors: self.ignore_errors,
             skip: self.skip,
+            weak_crypto,
         })
     }
 }
@@ -208,7 +220,8 @@ fn fetch_release(
 
     println!("Verifying '{name}' signature using provided repository key..");
     let content = fetched.data_ref();
-    let verified = helpers::verify_signature(content, &config.key, sig.as_deref())?;
+    let verified =
+        helpers::verify_signature(content, &config.key, sig.as_deref(), &config.weak_crypto)?;
     println!("Success");
 
     let sha512 = Some(openssl::sha::sha512(content));
-- 
2.30.2

[pve-devel] applied: [PATCH proxmox-offline-mirror 0/2] improve GPG verification

[Thomas Lamprecht]

Am 04/04/2023 um 09:48 schrieb Fabian Grünbichler:
> this patch series improves the error messages when encountering an issue with
> GPG verification, and offers an (opt-in) escape hatch for allowing weak
> cryptographic algorithms if a repository still needs those.
> 
> Fabian Grünbichler (2):
>   improve GPG error messages
>   fix #4632: allow escape hatches for legacy repositories
> 
>  src/bin/proxmox-offline-mirror.rs             |  2 +
>  src/bin/proxmox_offline_mirror_cmds/config.rs |  4 ++
>  src/config.rs                                 | 42 +++++++++++++-
>  src/helpers/verifier.rs                       | 55 +++++++++++++++++--
>  src/mirror.rs                                 | 17 +++++-
>  5 files changed, 111 insertions(+), 9 deletions(-)
> 


applied both patches, but inline comment for a possible small follow-up, thanks!

Re: [pve-devel] [PATCH proxmox-offline-mirror 2/2] fix #4632: allow escape hatches for legacy repositories

[Thomas Lamprecht]

Am 04/04/2023 um 09:48 schrieb Fabian Grünbichler:
> there are still repositories out there that are using things like DSA/RSA-1024
> and SHA1, so let's allow POM users to opt into accepting those insecure
> cryptographic parameters, but keep the default settings secure.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  src/bin/proxmox-offline-mirror.rs             |  2 +
>  src/bin/proxmox_offline_mirror_cmds/config.rs |  4 ++
>  src/config.rs                                 | 42 ++++++++++++++++++-
>  src/helpers/verifier.rs                       | 20 ++++++++-
>  src/mirror.rs                                 | 17 +++++++-
>  5 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
> index 3af33bb..bec366a 100644
> --- a/src/bin/proxmox-offline-mirror.rs
> +++ b/src/bin/proxmox-offline-mirror.rs
> @@ -423,6 +423,7 @@ fn action_add_mirror(config: &SectionConfigData) -> Result<Vec<MirrorConfig>, Er
>                  use_subscription: None,
>                  ignore_errors: false,
>                  skip,
> +                weak_crypto: None,
>              });
>          }
>      }
> @@ -438,6 +439,7 @@ fn action_add_mirror(config: &SectionConfigData) -> Result<Vec<MirrorConfig>, Er
>          use_subscription,
>          ignore_errors: false,
>          skip,
> +        weak_crypto: None,
>      };
>  
>      configs.push(main_config);
> diff --git a/src/bin/proxmox_offline_mirror_cmds/config.rs b/src/bin/proxmox_offline_mirror_cmds/config.rs
> index 3ebf4ad..696da11 100644
> --- a/src/bin/proxmox_offline_mirror_cmds/config.rs
> +++ b/src/bin/proxmox_offline_mirror_cmds/config.rs
> @@ -274,6 +274,10 @@ pub fn update_mirror(
>          data.skip.skip_sections = Some(skip_sections);
>      }
>  
> +    if let Some(weak_crypto) = update.weak_crypto {
> +        data.weak_crypto = Some(weak_crypto);
> +    }
> +
>      config.set_data(&id, "mirror", &data)?;
>      proxmox_offline_mirror::config::save_config(&config_file, &config)?;
>  
> diff --git a/src/config.rs b/src/config.rs
> index 39b1193..0e19c77 100644
> --- a/src/config.rs
> +++ b/src/config.rs
> @@ -5,7 +5,7 @@ use lazy_static::lazy_static;
>  use proxmox_subscription::{sign::ServerBlob, SubscriptionInfo};
>  use serde::{Deserialize, Serialize};
>  
> -use proxmox_schema::{api, ApiType, Schema, Updater};
> +use proxmox_schema::{api, ApiStringFormat, ApiType, Schema, Updater};
>  use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
>  use proxmox_sys::fs::{replace_file, CreateOptions};
>  
> @@ -46,6 +46,38 @@ pub struct SkipConfig {
>      pub skip_packages: Option<Vec<String>>,
>  }
>  
> +#[api(
> +    properties: {
> +        "allow-sha1": {
> +            type: bool,
> +            default: false,
> +            optional: true,
> +        },
> +        "min-dsa-key-size": {
> +            type: u64,
> +            optional: true,

could we encode minimum value and default of 2048 here? As the existance of the option
indicates that there is some minimum, and not knowing what that is, could be IMO slightly
confusing for users.

I don't found a quick/easy way to get the actual value from sequioa, but raising this limit
is probably either a breaking change or will be highlighted in any release notes so we
should be made aware for when to update it if we just hard-code it here.

> +        },
> +        "min-rsa-key-size": {
> +            type: u64,
> +            optional: true,

same here.

Re: [pve-devel] [PATCH proxmox-offline-mirror 2/2] fix #4632: allow escape hatches for legacy repositories

[Fabian Grünbichler]

On April 6, 2023 1:23 pm, Thomas Lamprecht wrote:
> Am 04/04/2023 um 09:48 schrieb Fabian Grünbichler:
>> there are still repositories out there that are using things like DSA/RSA-1024
>> and SHA1, so let's allow POM users to opt into accepting those insecure
>> cryptographic parameters, but keep the default settings secure.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>  src/bin/proxmox-offline-mirror.rs             |  2 +
>>  src/bin/proxmox_offline_mirror_cmds/config.rs |  4 ++
>>  src/config.rs                                 | 42 ++++++++++++++++++-
>>  src/helpers/verifier.rs                       | 20 ++++++++-
>>  src/mirror.rs                                 | 17 +++++++-
>>  5 files changed, 81 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
>> index 3af33bb..bec366a 100644
>> --- a/src/bin/proxmox-offline-mirror.rs
>> +++ b/src/bin/proxmox-offline-mirror.rs
>> @@ -423,6 +423,7 @@ fn action_add_mirror(config: &SectionConfigData) -> Result<Vec<MirrorConfig>, Er
>>                  use_subscription: None,
>>                  ignore_errors: false,
>>                  skip,
>> +                weak_crypto: None,
>>              });
>>          }
>>      }
>> @@ -438,6 +439,7 @@ fn action_add_mirror(config: &SectionConfigData) -> Result<Vec<MirrorConfig>, Er
>>          use_subscription,
>>          ignore_errors: false,
>>          skip,
>> +        weak_crypto: None,
>>      };
>>  
>>      configs.push(main_config);
>> diff --git a/src/bin/proxmox_offline_mirror_cmds/config.rs b/src/bin/proxmox_offline_mirror_cmds/config.rs
>> index 3ebf4ad..696da11 100644
>> --- a/src/bin/proxmox_offline_mirror_cmds/config.rs
>> +++ b/src/bin/proxmox_offline_mirror_cmds/config.rs
>> @@ -274,6 +274,10 @@ pub fn update_mirror(
>>          data.skip.skip_sections = Some(skip_sections);
>>      }
>>  
>> +    if let Some(weak_crypto) = update.weak_crypto {
>> +        data.weak_crypto = Some(weak_crypto);
>> +    }
>> +
>>      config.set_data(&id, "mirror", &data)?;
>>      proxmox_offline_mirror::config::save_config(&config_file, &config)?;
>>  
>> diff --git a/src/config.rs b/src/config.rs
>> index 39b1193..0e19c77 100644
>> --- a/src/config.rs
>> +++ b/src/config.rs
>> @@ -5,7 +5,7 @@ use lazy_static::lazy_static;
>>  use proxmox_subscription::{sign::ServerBlob, SubscriptionInfo};
>>  use serde::{Deserialize, Serialize};
>>  
>> -use proxmox_schema::{api, ApiType, Schema, Updater};
>> +use proxmox_schema::{api, ApiStringFormat, ApiType, Schema, Updater};
>>  use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
>>  use proxmox_sys::fs::{replace_file, CreateOptions};
>>  
>> @@ -46,6 +46,38 @@ pub struct SkipConfig {
>>      pub skip_packages: Option<Vec<String>>,
>>  }
>>  
>> +#[api(
>> +    properties: {
>> +        "allow-sha1": {
>> +            type: bool,
>> +            default: false,
>> +            optional: true,
>> +        },
>> +        "min-dsa-key-size": {
>> +            type: u64,
>> +            optional: true,
> 
> could we encode minimum value and default of 2048 here? As the existance of the option
> indicates that there is some minimum, and not knowing what that is, could be IMO slightly
> confusing for users.

yes

> I don't found a quick/easy way to get the actual value from sequioa, but raising this limit
> is probably either a breaking change or will be highlighted in any release notes so we
> should be made aware for when to update it if we just hard-code it here.

we could also define our own policy (or overrides for stuff we want to accept
for the time being, irrespective of sequoias standard policy) if we want.

there is some work[0] for making the policies loadable from a file, but that
would then tie us rather directly to sequoia if we offer that via our config
(also, the format is rather involved and not very user friendly).

0: https://docs.rs/sequoia-policy-config/latest/sequoia_policy_config/

>> +        },
>> +        "min-rsa-key-size": {
>> +            type: u64,
>> +            optional: true,
> 
> same here.
>