[pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256

all lists on lists.proxmox.com
 help / color / mirror / Atom feed

* [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256
@ 2024-11-11  9:32 Maximiliano Sandoval
  2024-11-11  9:32 ` [pmg-devel] [PATCH pmg-api v3 2/2] api: Update regex to accept A-F instead of A-Z Maximiliano Sandoval
  2024-11-11 12:20 ` [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256 Stoiko Ivanov
  0 siblings, 2 replies; 4+ messages in thread
From: Maximiliano Sandoval @ 2024-11-11  9:32 UTC (permalink / raw)
  To: pmg-devel

We use the description from the standard option 'fingerprint-sha256'.
The option itself cannot be used as the regex allows lowercase
characters which don't work here.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---

Differences from v2:
 - Use the same docstring as in pve-common
 - Add regex pattern into the same patch

 src/PMG/API2/Cluster.pm  | 2 +-
 src/PMG/CLI/pmgcm.pm     | 2 +-
 src/PMG/ClusterConfig.pm | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PMG/API2/Cluster.pm b/src/PMG/API2/Cluster.pm
index 84dafabb..6d776548 100644
--- a/src/PMG/API2/Cluster.pm
+++ b/src/PMG/API2/Cluster.pm
@@ -409,7 +409,7 @@ __PACKAGE__->register_method({
 		type => 'string', format => 'ip',
 	    },
 	    fingerprint => {
-		description => "SSL certificate fingerprint.",
+		description => "Certificate SHA 256 fingerprint.",
 		type => 'string',
 		pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
 	    },
diff --git a/src/PMG/CLI/pmgcm.pm b/src/PMG/CLI/pmgcm.pm
index ecf9cc76..b4601de4 100644
--- a/src/PMG/CLI/pmgcm.pm
+++ b/src/PMG/CLI/pmgcm.pm
@@ -167,7 +167,7 @@ __PACKAGE__->register_method({
 		type => 'string', format => 'ip',
 	    },
 	    fingerprint => {
-		description => "SSL certificate fingerprint.",
+		description => "Certificate SHA 256 fingerprint.",
 		type => 'string',
 		pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
 		optional => 1,
diff --git a/src/PMG/ClusterConfig.pm b/src/PMG/ClusterConfig.pm
index c52508dc..ad4acd05 100644
--- a/src/PMG/ClusterConfig.pm
+++ b/src/PMG/ClusterConfig.pm
@@ -73,7 +73,7 @@ sub properties {
 	    pattern => valid_ssh_pubkey_regex(),
 	},
 	fingerprint => {
-	    description => "SSL certificate fingerprint.",
+	    description => "Certificate SHA 256 fingerprint.",
 	    type => 'string',
 	    pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
 	},
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* [pmg-devel] [PATCH pmg-api v3 2/2] api: Update regex to accept A-F instead of A-Z
  2024-11-11  9:32 [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256 Maximiliano Sandoval
@ 2024-11-11  9:32 ` Maximiliano Sandoval
  2024-11-11 12:20 ` [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256 Stoiko Ivanov
  1 sibling, 0 replies; 4+ messages in thread
From: Maximiliano Sandoval @ 2024-11-11  9:32 UTC (permalink / raw)
  To: pmg-devel

Fingerprints are in hex.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/PMG/API2/Cluster.pm  | 2 +-
 src/PMG/CLI/pmgcm.pm     | 2 +-
 src/PMG/ClusterConfig.pm | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/PMG/API2/Cluster.pm b/src/PMG/API2/Cluster.pm
index 6d776548..edc7fdd8 100644
--- a/src/PMG/API2/Cluster.pm
+++ b/src/PMG/API2/Cluster.pm
@@ -411,7 +411,7 @@ __PACKAGE__->register_method({
 	    fingerprint => {
 		description => "Certificate SHA 256 fingerprint.",
 		type => 'string',
-		pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
+		pattern => '^(:?[A-F0-9][A-F0-9]:){31}[A-F0-9][A-F0-9]$',
 	    },
 	    password => {
 		description => "Superuser password.",
diff --git a/src/PMG/CLI/pmgcm.pm b/src/PMG/CLI/pmgcm.pm
index b4601de4..fa105e4c 100644
--- a/src/PMG/CLI/pmgcm.pm
+++ b/src/PMG/CLI/pmgcm.pm
@@ -169,7 +169,7 @@ __PACKAGE__->register_method({
 	    fingerprint => {
 		description => "Certificate SHA 256 fingerprint.",
 		type => 'string',
-		pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
+		pattern => '^(:?[A-F0-9][A-F0-9]:){31}[A-F0-9][A-F0-9]$',
 		optional => 1,
 	    },
 	},
diff --git a/src/PMG/ClusterConfig.pm b/src/PMG/ClusterConfig.pm
index ad4acd05..82727257 100644
--- a/src/PMG/ClusterConfig.pm
+++ b/src/PMG/ClusterConfig.pm
@@ -75,7 +75,7 @@ sub properties {
 	fingerprint => {
 	    description => "Certificate SHA 256 fingerprint.",
 	    type => 'string',
-	    pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
+	    pattern => '^(:?[A-F0-9][A-F0-9]:){31}[A-F0-9][A-F0-9]$',
 	},
     };
 }
-- 
2.39.5



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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256
  2024-11-11  9:32 [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256 Maximiliano Sandoval
  2024-11-11  9:32 ` [pmg-devel] [PATCH pmg-api v3 2/2] api: Update regex to accept A-F instead of A-Z Maximiliano Sandoval
@ 2024-11-11 12:20 ` Stoiko Ivanov
  2024-11-18 13:20   ` Maximiliano Sandoval
  1 sibling, 1 reply; 4+ messages in thread
From: Stoiko Ivanov @ 2024-11-11 12:20 UTC (permalink / raw)
  To: Maximiliano Sandoval; +Cc: pmg-devel

Thanks for the patches!

On Mon, 11 Nov 2024 10:32:30 +0100
Maximiliano Sandoval <m.sandoval@proxmox.com> wrote:

> We use the description from the standard option 'fingerprint-sha256'.
> The option itself cannot be used as the regex allows lowercase
> characters which don't work here.
It would really help to get a bit more information about what exactly did
not work, and what you tested to come to that conclusion.

As I'm quite in favor of reusing our standard-options where possible
I gave your v2 a spin to find out what might not work - from a quick
glance (w/o testing everything possible) - the following diff should cover
most issues:
```
diff --git a/src/PMG/CLI/pmgcm.pm b/src/PMG/CLI/pmgcm.pm
index 699089e..c55ef92 100644
--- a/src/PMG/CLI/pmgcm.pm
+++ b/src/PMG/CLI/pmgcm.pm
@@ -194,7 +194,7 @@ __PACKAGE__->register_method({
            };
            if ($param->{fingerprint}) {
                $setup->{cached_fingerprints} = {
-                   $param->{fingerprint} => 1,
+                   uc($param->{fingerprint}) => 1,
                };
            } else {
                # allow manual fingerprint verification
diff --git a/src/PMG/Cluster.pm b/src/PMG/Cluster.pm
index 17ba44d..789746f 100644
--- a/src/PMG/Cluster.pm
+++ b/src/PMG/Cluster.pm
@@ -148,7 +148,7 @@ sub update_cert_cache {
 
     foreach my $entry (values %{$cinfo->{ids}}) {
        my $node = $entry->{name};
-       my $fp = $entry->{fingerprint};
+       my $fp = uc($entry->{fingerprint});
        if ($node && $fp) {
            $cert_cache_fingerprints->{$fp} = 1;
            $cert_cache_nodes->{$node} = $fp;
@@ -179,7 +179,7 @@ sub check_cert_fingerprint {
 
     my $check = sub {
        for my $expected (keys %$cert_cache_fingerprints) {
-           return 1 if $fp eq $expected;
+           return 1 if uc($fp) eq $expected;
        }
        return 0;
     };
diff --git a/src/PMG/ClusterConfig.pm b/src/PMG/ClusterConfig.pm
index 491fede..e469ea9 100644
--- a/src/PMG/ClusterConfig.pm
+++ b/src/PMG/ClusterConfig.pm
@@ -195,6 +195,7 @@ sub read_cluster_conf {
        $names_hash->{$d->{name}} = 1;
 
        $d->{cid} = $cid;
+       $d->{fingerprint} = uc($d->{fingerprint});
        $maxcid = $cid > $maxcid ? $cid : $maxcid;
        $maxcid = $d->{maxcid} if defined($d->{maxcid}) && $d->{maxcid} > $maxcid;
        $cinfo->{master} = $d if $d->{type} eq 'master';

```

I tested:
* installing this on a cluster-node where I manually changed the
  fingerprint to lower-case in /etc/pmg/cluster.conf
* creating a cluster on the cli - but pasting the fingerprint-option in
  lower-case
* changing the apicert (`pmgconfig apicert --force 1`), restarting
  pmgproxy and running `pmgcm update-fingerprints`) 

I also would rather not reuse the description of a standard-option for
a slightly different copy of that option.



> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 
> Differences from v2:
>  - Use the same docstring as in pve-common
>  - Add regex pattern into the same patch
> 
>  src/PMG/API2/Cluster.pm  | 2 +-
>  src/PMG/CLI/pmgcm.pm     | 2 +-
>  src/PMG/ClusterConfig.pm | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/API2/Cluster.pm b/src/PMG/API2/Cluster.pm
> index 84dafabb..6d776548 100644
> --- a/src/PMG/API2/Cluster.pm
> +++ b/src/PMG/API2/Cluster.pm
> @@ -409,7 +409,7 @@ __PACKAGE__->register_method({
>  		type => 'string', format => 'ip',
>  	    },
>  	    fingerprint => {
> -		description => "SSL certificate fingerprint.",
> +		description => "Certificate SHA 256 fingerprint.",
>  		type => 'string',
>  		pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
>  	    },
> diff --git a/src/PMG/CLI/pmgcm.pm b/src/PMG/CLI/pmgcm.pm
> index ecf9cc76..b4601de4 100644
> --- a/src/PMG/CLI/pmgcm.pm
> +++ b/src/PMG/CLI/pmgcm.pm
> @@ -167,7 +167,7 @@ __PACKAGE__->register_method({
>  		type => 'string', format => 'ip',
>  	    },
>  	    fingerprint => {
> -		description => "SSL certificate fingerprint.",
> +		description => "Certificate SHA 256 fingerprint.",
>  		type => 'string',
>  		pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
>  		optional => 1,
> diff --git a/src/PMG/ClusterConfig.pm b/src/PMG/ClusterConfig.pm
> index c52508dc..ad4acd05 100644
> --- a/src/PMG/ClusterConfig.pm
> +++ b/src/PMG/ClusterConfig.pm
> @@ -73,7 +73,7 @@ sub properties {
>  	    pattern => valid_ssh_pubkey_regex(),
>  	},
>  	fingerprint => {
> -	    description => "SSL certificate fingerprint.",
> +	    description => "Certificate SHA 256 fingerprint.",
>  	    type => 'string',
>  	    pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
>  	},



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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256
  2024-11-11 12:20 ` [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256 Stoiko Ivanov
@ 2024-11-18 13:20   ` Maximiliano Sandoval
  0 siblings, 0 replies; 4+ messages in thread
From: Maximiliano Sandoval @ 2024-11-18 13:20 UTC (permalink / raw)
  To: Stoiko Ivanov; +Cc: pmg-devel


Stoiko Ivanov <s.ivanov@proxmox.com> writes:

> Thanks for the patches!
>
> On Mon, 11 Nov 2024 10:32:30 +0100
> Maximiliano Sandoval <m.sandoval@proxmox.com> wrote:
>
>> We use the description from the standard option 'fingerprint-sha256'.
>> The option itself cannot be used as the regex allows lowercase
>> characters which don't work here.
> It would really help to get a bit more information about what exactly did
> not work, and what you tested to come to that conclusion.

I tested replacing a single character in the fingerprint with a
lowercase character and the web UI stopped working completely.
Refreshing the tab would send me back to a login screen on which it was
not possible to log in.

> As I'm quite in favor of reusing our standard-options where possible
> I gave your v2 a spin to find out what might not work - from a quick
> glance (w/o testing everything possible) - the following diff should cover
> most issues:
>
> ```
> diff --git a/src/PMG/CLI/pmgcm.pm b/src/PMG/CLI/pmgcm.pm
> index 699089e..c55ef92 100644
> --- a/src/PMG/CLI/pmgcm.pm
> +++ b/src/PMG/CLI/pmgcm.pm
> @@ -194,7 +194,7 @@ __PACKAGE__->register_method({
>             };
>             if ($param->{fingerprint}) {
>                 $setup->{cached_fingerprints} = {
> -                   $param->{fingerprint} => 1,
> +                   uc($param->{fingerprint}) => 1,
>                 };
>             } else {
>                 # allow manual fingerprint verification
> diff --git a/src/PMG/Cluster.pm b/src/PMG/Cluster.pm
> index 17ba44d..789746f 100644
> --- a/src/PMG/Cluster.pm
> +++ b/src/PMG/Cluster.pm
> @@ -148,7 +148,7 @@ sub update_cert_cache {
>
>      foreach my $entry (values %{$cinfo->{ids}}) {
>         my $node = $entry->{name};
> -       my $fp = $entry->{fingerprint};
> +       my $fp = uc($entry->{fingerprint});
>         if ($node && $fp) {
>             $cert_cache_fingerprints->{$fp} = 1;
>             $cert_cache_nodes->{$node} = $fp;
> @@ -179,7 +179,7 @@ sub check_cert_fingerprint {
>
>      my $check = sub {
>         for my $expected (keys %$cert_cache_fingerprints) {
> -           return 1 if $fp eq $expected;
> +           return 1 if uc($fp) eq $expected;
>         }
>         return 0;
>      };
> diff --git a/src/PMG/ClusterConfig.pm b/src/PMG/ClusterConfig.pm
> index 491fede..e469ea9 100644
> --- a/src/PMG/ClusterConfig.pm
> +++ b/src/PMG/ClusterConfig.pm
> @@ -195,6 +195,7 @@ sub read_cluster_conf {
>         $names_hash->{$d->{name}} = 1;
>
>         $d->{cid} = $cid;
> +       $d->{fingerprint} = uc($d->{fingerprint});
>         $maxcid = $cid > $maxcid ? $cid : $maxcid;
>         $maxcid = $d->{maxcid} if defined($d->{maxcid}) && $d->{maxcid} > $maxcid;
>         $cinfo->{master} = $d if $d->{type} eq 'master';
>
> ```
>
> I tested:
> * installing this on a cluster-node where I manually changed the
>   fingerprint to lower-case in /etc/pmg/cluster.conf
> * creating a cluster on the cli - but pasting the fingerprint-option in
>   lower-case
> * changing the apicert (`pmgconfig apicert --force 1`), restarting
>   pmgproxy and running `pmgcm update-fingerprints`)

I am not very comfortable adding a new state that might potential break
something that we did not test (or that it might break in the future)
for a feature that does not add anything for the end-user. I think it
makes more sense to simply document the current behavior.

> I also would rather not reuse the description of a standard-option for
> a slightly different copy of that option.

That is sensible, perhaps a different description could be used?


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


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-11-18 13:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-11  9:32 [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256 Maximiliano Sandoval
2024-11-11  9:32 ` [pmg-devel] [PATCH pmg-api v3 2/2] api: Update regex to accept A-F instead of A-Z Maximiliano Sandoval
2024-11-11 12:20 ` [pmg-devel] [PATCH pmg-api v3 1/2] api: document that fingerprints are a SHA 256 Stoiko Ivanov
2024-11-18 13:20   ` Maximiliano Sandoval

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.

Service provided by Proxmox Server Solutions GmbH | Privacy | Legal