[pve-devel] [PATCH pve-installer] answer: perform basic input validation for keyboard

[pve-devel] [PATCH pve-installer] answer: perform basic input validation for keyboard

[Christian Ebner]

Currently it is possible to validate and create an iso with an
invalid keyboad layout, only failing later during installation.

Add a basic check for correct keyboard layout by defining an enum
with allowed variants.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-auto-installer/src/answer.rs | 39 +++++++++++++++++++++++++++-
 proxmox-auto-installer/src/utils.rs  |  8 ++++--
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
index a6cf8b7..af7485a 100644
--- a/proxmox-auto-installer/src/answer.rs
+++ b/proxmox-auto-installer/src/answer.rs
@@ -23,7 +23,7 @@ pub struct Answer {
 pub struct Global {
     pub country: String,
     pub fqdn: Fqdn,
-    pub keyboard: String,
+    pub keyboard: KeyboardLayout,
     pub mailto: String,
     pub timezone: String,
     pub root_password: String,
@@ -270,3 +270,40 @@ pub struct BtrfsOptions {
     pub hdsize: Option<f64>,
     pub raid: Option<BtrfsRaidLevel>,
 }
+
+#[derive(Clone, Deserialize, Serialize, Debug, PartialEq)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
+pub enum KeyboardLayout {
+    De,
+    DeCh,
+    Dk,
+    EnGb,
+    EnUs,
+    Es,
+    Fi,
+    Fr,
+    FrBe,
+    FrCa,
+    FrCh,
+    Hu,
+    Is,
+    It,
+    Jp,
+    Lt,
+    Mk,
+    Nl,
+    No,
+    Pl,
+    Pt,
+    PtBr,
+    Se,
+    Si,
+    Tr,
+}
+
+impl std::fmt::Display for KeyboardLayout {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let keyboard_layout = serde_json::to_value(self).unwrap().to_string();
+        write!(f, "{}", keyboard_layout.trim_matches('\"'))
+    }
+}
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 7e1366c..202ad41 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -281,7 +281,11 @@ pub fn verify_locale_settings(answer: &Answer, locales: &LocaleInfo) -> Result<(
     {
         bail!("country code '{}' is not valid", &answer.global.country);
     }
-    if !locales.kmap.keys().any(|i| i == &answer.global.keyboard) {
+    if !locales
+        .kmap
+        .keys()
+        .any(|i| i == &answer.global.keyboard.to_string())
+    {
         bail!("keyboard layout '{}' is not valid", &answer.global.keyboard);
     }
 
@@ -328,7 +332,7 @@ pub fn parse_answer(
 
         country: answer.global.country.clone(),
         timezone: answer.global.timezone.clone(),
-        keymap: answer.global.keyboard.clone(),
+        keymap: answer.global.keyboard.to_string(),
 
         password: answer.global.root_password.clone(),
         mailto: answer.global.mailto.clone(),
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-installer] answer: perform basic input validation for keyboard

[Stoiko Ivanov]

On Wed, 24 Apr 2024 10:48:50 +0200
Christian Ebner <c.ebner@proxmox.com> wrote:

> Currently it is possible to validate and create an iso with an
> invalid keyboad layout, only failing later during installation.
> 
> Add a basic check for correct keyboard layout by defining an enum
> with allowed variants.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  proxmox-auto-installer/src/answer.rs | 39 +++++++++++++++++++++++++++-
>  proxmox-auto-installer/src/utils.rs  |  8 ++++--
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
> index a6cf8b7..af7485a 100644
> --- a/proxmox-auto-installer/src/answer.rs
> +++ b/proxmox-auto-installer/src/answer.rs
> @@ -23,7 +23,7 @@ pub struct Answer {
>  pub struct Global {
>      pub country: String,
>      pub fqdn: Fqdn,
> -    pub keyboard: String,
> +    pub keyboard: KeyboardLayout,
>      pub mailto: String,
>      pub timezone: String,
>      pub root_password: String,
> @@ -270,3 +270,40 @@ pub struct BtrfsOptions {
>      pub hdsize: Option<f64>,
>      pub raid: Option<BtrfsRaidLevel>,
>  }
> +
> +#[derive(Clone, Deserialize, Serialize, Debug, PartialEq)]
> +#[serde(rename_all = "kebab-case", deny_unknown_fields)]
> +pub enum KeyboardLayout {
> +    De,
> +    DeCh,
> +    Dk,
> +    EnGb,
> +    EnUs,
> +    Es,
> +    Fi,
> +    Fr,
> +    FrBe,
> +    FrCa,
> +    FrCh,
> +    Hu,
> +    Is,
> +    It,
> +    Jp,
> +    Lt,
> +    Mk,
> +    Nl,
> +    No,
> +    Pl,
> +    Pt,
> +    PtBr,
> +    Se,
> +    Si,
> +    Tr,
> +}
> +
quickly looked at that as well yesterday - and I also ran into the issue
that we get the relevant data in the installer itself (where actual
validation takes place).

with the target to have the auto-install-assistant available as single
static binary - I think your approach works well enough - the one thing
that might be an improvment is to get the data from country.dat (a
build-artefact output from country.pl based on
/usr/share/iso-codes/json/iso_3166-1.json ) at build-time and embed it in the binary.
(but I did not get around to checking how this is done sensibly in rust)

additionally we could verify the country selection as well with that.


> +impl std::fmt::Display for KeyboardLayout {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        let keyboard_layout = serde_json::to_value(self).unwrap().to_string();
> +        write!(f, "{}", keyboard_layout.trim_matches('\"'))
> +    }
> +}
> diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
> index 7e1366c..202ad41 100644
> --- a/proxmox-auto-installer/src/utils.rs
> +++ b/proxmox-auto-installer/src/utils.rs
> @@ -281,7 +281,11 @@ pub fn verify_locale_settings(answer: &Answer, locales: &LocaleInfo) -> Result<(
>      {
>          bail!("country code '{}' is not valid", &answer.global.country);
>      }
> -    if !locales.kmap.keys().any(|i| i == &answer.global.keyboard) {
> +    if !locales
> +        .kmap
> +        .keys()
> +        .any(|i| i == &answer.global.keyboard.to_string())
> +    {
>          bail!("keyboard layout '{}' is not valid", &answer.global.keyboard);
>      }
>  
> @@ -328,7 +332,7 @@ pub fn parse_answer(
>  
>          country: answer.global.country.clone(),
>          timezone: answer.global.timezone.clone(),
> -        keymap: answer.global.keyboard.clone(),
> +        keymap: answer.global.keyboard.to_string(),
>  
>          password: answer.global.root_password.clone(),
>          mailto: answer.global.mailto.clone(),



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH pve-installer] answer: perform basic input validation for keyboard

[Christian Ebner]

On 4/24/24 11:20, Stoiko Ivanov wrote:
> On Wed, 24 Apr 2024 10:48:50 +0200
> Christian Ebner <c.ebner@proxmox.com> wrote:
> 
>> Currently it is possible to validate and create an iso with an
>> invalid keyboad layout, only failing later during installation.
>>
>> Add a basic check for correct keyboard layout by defining an enum
>> with allowed variants.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   proxmox-auto-installer/src/answer.rs | 39 +++++++++++++++++++++++++++-
>>   proxmox-auto-installer/src/utils.rs  |  8 ++++--
>>   2 files changed, 44 insertions(+), 3 deletions(-)
>>
>> diff --git a/proxmox-auto-installer/src/answer.rs b/proxmox-auto-installer/src/answer.rs
>> index a6cf8b7..af7485a 100644
>> --- a/proxmox-auto-installer/src/answer.rs
>> +++ b/proxmox-auto-installer/src/answer.rs
>> @@ -23,7 +23,7 @@ pub struct Answer {
>>   pub struct Global {
>>       pub country: String,
>>       pub fqdn: Fqdn,
>> -    pub keyboard: String,
>> +    pub keyboard: KeyboardLayout,
>>       pub mailto: String,
>>       pub timezone: String,
>>       pub root_password: String,
>> @@ -270,3 +270,40 @@ pub struct BtrfsOptions {
>>       pub hdsize: Option<f64>,
>>       pub raid: Option<BtrfsRaidLevel>,
>>   }
>> +
>> +#[derive(Clone, Deserialize, Serialize, Debug, PartialEq)]
>> +#[serde(rename_all = "kebab-case", deny_unknown_fields)]
>> +pub enum KeyboardLayout {
>> +    De,
>> +    DeCh,
>> +    Dk,
>> +    EnGb,
>> +    EnUs,
>> +    Es,
>> +    Fi,
>> +    Fr,
>> +    FrBe,
>> +    FrCa,
>> +    FrCh,
>> +    Hu,
>> +    Is,
>> +    It,
>> +    Jp,
>> +    Lt,
>> +    Mk,
>> +    Nl,
>> +    No,
>> +    Pl,
>> +    Pt,
>> +    PtBr,
>> +    Se,
>> +    Si,
>> +    Tr,
>> +}
>> +
> quickly looked at that as well yesterday - and I also ran into the issue
> that we get the relevant data in the installer itself (where actual
> validation takes place).
> 
> with the target to have the auto-install-assistant available as single
> static binary - I think your approach works well enough - the one thing
> that might be an improvment is to get the data from country.dat (a
> build-artefact output from country.pl based on
> /usr/share/iso-codes/json/iso_3166-1.json ) at build-time and embed it in the binary.
> (but I did not get around to checking how this is done sensibly in rust)

Agreed, this would however still produce ISOs with possible inconsistent 
variants, if the `country.dat` in the ISO and the one used for 
compilation of the binary are out of sync (which probably will happen 
not that often).

> 
> additionally we could verify the country selection as well with that.
> 
> 
>> +impl std::fmt::Display for KeyboardLayout {
>> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
>> +        let keyboard_layout = serde_json::to_value(self).unwrap().to_string();
>> +        write!(f, "{}", keyboard_layout.trim_matches('\"'))
>> +    }
>> +}
>> diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
>> index 7e1366c..202ad41 100644
>> --- a/proxmox-auto-installer/src/utils.rs
>> +++ b/proxmox-auto-installer/src/utils.rs
>> @@ -281,7 +281,11 @@ pub fn verify_locale_settings(answer: &Answer, locales: &LocaleInfo) -> Result<(
>>       {
>>           bail!("country code '{}' is not valid", &answer.global.country);
>>       }
>> -    if !locales.kmap.keys().any(|i| i == &answer.global.keyboard) {
>> +    if !locales
>> +        .kmap
>> +        .keys()
>> +        .any(|i| i == &answer.global.keyboard.to_string())
>> +    {
>>           bail!("keyboard layout '{}' is not valid", &answer.global.keyboard);
>>       }
>>   
>> @@ -328,7 +332,7 @@ pub fn parse_answer(
>>   
>>           country: answer.global.country.clone(),
>>           timezone: answer.global.timezone.clone(),
>> -        keymap: answer.global.keyboard.clone(),
>> +        keymap: answer.global.keyboard.to_string(),
>>   
>>           password: answer.global.root_password.clone(),
>>           mailto: answer.global.mailto.clone(),
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH pve-installer] answer: perform basic input validation for keyboard

[Thomas Lamprecht]

Am 24/04/2024 um 10:48 schrieb Christian Ebner:
> Currently it is possible to validate and create an iso with an
> invalid keyboad layout, only failing later during installation.
> 
> Add a basic check for correct keyboard layout by defining an enum
> with allowed variants.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  proxmox-auto-installer/src/answer.rs | 39 +++++++++++++++++++++++++++-
>  proxmox-auto-installer/src/utils.rs  |  8 ++++--
>  2 files changed, 44 insertions(+), 3 deletions(-)
> 
>

applied, with a follow-up to use serde_plain::derive_display_from_serialize
to derive the Display impl, thanks!

btw. if we probably want to use the same perl code that generates the valid
list for the installer environment to output that at build time in such a
way that we can source it from rust on build.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel