[pmg-devel] [PATCH pmg-api] use fingerprint-sha256 option for fingerprints

[Fabian Grünbichler]

On October 23, 2025 10:51 am, Maximiliano Sandoval wrote:
> Fabian Grünbichler <f.gruenbichler at proxmox.com> writes:
> 
>> On October 22, 2025 2:06 pm, Maximiliano Sandoval wrote:
>>> The previous parameter had a regex expression that allowed letters [A-Z]
>>> instead of [A-F] and it was uppercase-only.
>>> 
>>> Tested via:
>>> 
>>> ```
>>> pmgcm join $HOST --fingerprint $FINGERPRINT
>>> pmgcm status
>>> ```
>>> 
>>> Additionally it was tested that 'pmgcm status' worked after changing the
>>> fingerprints at /etc/pmg/cluster.conf to lowercase.
>>> 
>>> Here $FINGERPRINT was modified so it contained both upper-and-lowercase
>>> characters.
>>
>> Isn't this basically a v4 of an older series of yours, with
>> suggestions/diffs by Stoiko folded in?
>>
>> https://lore.proxmox.com/pmg-devel/20241111132057.0ea5b2c2@rosa.proxmox.com/
> 
> The previous one was about docs only, I intentionally did not send this
> as v4.

the first patch yes, the second and Stoiko's reply are semantically
equivalent to this patch here, so this patch is definitely the fourth
version of that patch series, and this patch here should at least have a
Suggested-by tag..

>>> Signed-off-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
>>> ---
>>>  src/PMG/API2/Cluster.pm  |  9 ++++-----
>>>  src/PMG/CLI/pmgcm.pm     | 12 ++++++------
>>>  src/PMG/ClusterConfig.pm | 12 +++++++-----
>>>  3 files changed, 17 insertions(+), 16 deletions(-)
>>> 
>>> diff --git a/src/PMG/API2/Cluster.pm b/src/PMG/API2/Cluster.pm
>>> index fbfbcfc2..69096df6 100644
>>> --- a/src/PMG/API2/Cluster.pm
>>> +++ b/src/PMG/API2/Cluster.pm
>>> @@ -421,11 +421,7 @@ __PACKAGE__->register_method({
>>>                  type => 'string',
>>>                  format => 'ip',
>>>              },
>>> -            fingerprint => {
>>> -                description => "SSL certificate fingerprint.",
>>> -                type => 'string',
>>> -                pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
>>> -            },
>>> +            fingerprint => get_standard_option('fingerprint-sha256'),
>>>              password => {
>>>                  description => "Superuser password.",
>>>                  type => 'string',
>>> @@ -439,6 +435,9 @@ __PACKAGE__->register_method({
>>>  
>>>          my $rpcenv = PMG::RESTEnvironment->get();
>>>          my $authuser = $rpcenv->get_user();
>>> +        if (my $fp = $param->{fingerprint}) {
>>> +            $param->{fingerprint} = uc($fp);
>>> +        }
>>>  
>>>          my $realcmd = sub {
>>>              my $cinfo = PMG::ClusterConfig->new();
>>> diff --git a/src/PMG/CLI/pmgcm.pm b/src/PMG/CLI/pmgcm.pm
>>> index 401f6801..07ea0741 100644
>>> --- a/src/PMG/CLI/pmgcm.pm
>>> +++ b/src/PMG/CLI/pmgcm.pm
>>> @@ -6,6 +6,7 @@ use Data::Dumper;
>>>  use POSIX qw(strftime);
>>>  use JSON;
>>>  
>>> +use PVE::JSONSchema qw(get_standard_option);
>>>  use PVE::SafeSyslog;
>>>  use PVE::Tools qw(extract_param);
>>>  use PVE::INotify;
>>> @@ -171,18 +172,17 @@ __PACKAGE__->register_method({
>>>                  type => 'string',
>>>                  format => 'ip',
>>>              },
>>> -            fingerprint => {
>>> -                description => "SSL certificate fingerprint.",
>>> -                type => 'string',
>>> -                pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
>>> -                optional => 1,
>>> -            },
>>> +            fingerprint => get_standard_option('fingerprint-sha256'),
>>>          },
>>>      },
>>>      returns => { type => 'null' },
>>>      code => sub {
>>>          my ($param) = @_;
>>>  
>>> +        if (my $fp = $param->{fingerprint}) {
>>> +            $param->{fingerprint} = uc($fp);
>>> +        }
>>> +
>>>          my $code = sub {
>>>              my $cinfo = PMG::ClusterConfig->new();
>>>  
>>> diff --git a/src/PMG/ClusterConfig.pm b/src/PMG/ClusterConfig.pm
>>> index bf8405c0..eeab326b 100644
>>> --- a/src/PMG/ClusterConfig.pm
>>> +++ b/src/PMG/ClusterConfig.pm
>>> @@ -75,11 +75,7 @@ sub properties {
>>>              type => 'string',
>>>              pattern => valid_ssh_pubkey_regex(),
>>>          },
>>> -        fingerprint => {
>>> -            description => "SSL certificate fingerprint.",
>>> -            type => 'string',
>>> -            pattern => '^(:?[A-Z0-9][A-Z0-9]:){31}[A-Z0-9][A-Z0-9]$',
>>> -        },
>>> +        fingerprint => PVE::JSONSchema::get_standard_option('fingerprint-sha256'),
>>>      };
>>>  }
>>>  
>>> @@ -203,6 +199,8 @@ sub read_cluster_conf {
>>>          $maxcid = $d->{maxcid} if defined($d->{maxcid}) && $d->{maxcid} > $maxcid;
>>>          $cinfo->{master} = $d if $d->{type} eq 'master';
>>>          $cinfo->{'local'} = $d if $d->{name} eq $localname;
>>> +
>>> +        $d->{fingerprint} = uc($d->{fingerprint});
>>>      }
>>>  
>>>      if ($maxcid) {
>>> @@ -224,6 +222,10 @@ sub read_cluster_conf {
>>>  sub write_cluster_conf {
>>>      my ($filename, $fh, $cfg) = @_;
>>>  
>>> +    foreach my $entry (values %{ $cfg->{ids} }) {
>>> +        $entry->{fingerprint} = uc($entry->{fingerprint});
>>> +    }
>>
>> this should not be needed, since the reader above already mangles it..
> 
> Does the $cfg here necesarily have to come from the reader above?

That is a question that you should be able to answer? :-P

AFAICT there is one code path where it isn't, but that returns uppercase
already for the fingerprint.

Note that there is a 'read_local_ssl_cert_fingerprint' helper involved
in that codepath in PMG::Cluster that can probably be replaced wholesale
by PVE::Certificate::get_certificate_fingerprint from pve-common, which
would fit with the general theme of this patch (series)

>>> +
>>>      my $raw = PMG::ClusterConfig::Base->write_config($filename, $cfg);
>>>  
>>>      PVE::Tools::safe_print($filename, $fh, $raw);
>>> -- 
>>> 2.47.3
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> pmg-devel mailing list
>>> pmg-devel at lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
>>> 
>>> 
>>> 
> 
> -- 
> Maximiliano
>