Re: [pmg-devel] [PATCH pmg-api] utils: check if file changed before reusing its hash

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 31/08/2023 um 15:33 schrieb Maximiliano Sandoval:
> We cache the hash of this file, it makes sense to first check if the
> file changed via `stat` and recompute the hash if needed.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  src/PMG/Utils.pm | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index c19b31f..f8e6b7c 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -49,6 +49,8 @@ postgres_admin_cmd
>  try_decode_utf8
>  );
>  
> +my $host_rsa_key_path = '/etc/ssh/ssh_host_rsa_key.pub';

meh, why is that a global module variable now?

I'd rather keep definition and usage together..

> +
>  my $valid_pmg_realms = ['pam', 'pmg', 'quarantine'];
>  
>  PVE::JSONSchema::register_standard_option('realm', {
> @@ -1353,14 +1355,32 @@ sub scan_journal_for_rbl_rejects {
>  }
>  
>  my $hwaddress;
> +my $hwaddress_st = {};
> +
> +sub get_server_id {
> +    my $sshkey = PVE::Tools::file_get_contents($host_rsa_key_path);
> +    return uc(Digest::MD5::md5_hex($sshkey));
> +}
>  
>  sub get_hwaddress {
> +    my $st = stat($host_rsa_key_path);
>  
> -    return $hwaddress if defined ($hwaddress);
> +    if (! defined($hwaddress)) {

style nit: please drop the extra space between ! and defined

> +	$hwaddress_st->{mtime} = $st->mtime;
> +	$hwaddress_st->{ino} = $st->ino;
> +	$hwaddress_st->{dev} = $st->dev;
> +	$hwaddress = get_server_id();


can we do this such that we still early return if OK, i.e., if HW address
is set and cache still valid, and otherwise update both cache value and
validity metadata unconditionally afterwards?

Would save a bit of code and also the newly added get_server_id method,
which is also a bit confusing, as it competes with get_hwaddress, so
should be either private, or (slightly better) just return the raw host
key, or IMO even better get dropped (see below).

Also, use hash slices for setting the cache validity keys can shorten
things, i.e., in summary something like:

my $st = stat($host_rsa_key_path);

if (
    defined($hwaddress)
    && $hwaddress_st->{ino} == $st->ino
    && $hwaddress_st->{mtime} == $st->mtime
    && $hwaddress_st->{dev} == $st->dev
) {
    return $hwaddress;
}

# else update cache
my $sshkey = PVE::Tools::file_get_contents('/etc/ssh/ssh_host_rsa_key.pub'); 
$hwaddress = uc(Digest::MD5::md5_hex($sshkey));
$hwaddress_st->@{'mtime', 'ino', 'dev'} = ($st->mtime, $st->ino, $st->dev);

return $hwaddress;


Also, is this possibly worth a log? as this happening is something odd for
most setups (at least, after intial provisioning).

> +    }
> +
> +    if ($hwaddress_st->{mtime} != $st->mtime
> +	|| $hwaddress_st->{ino} != $st->ino
> +	|| $hwaddress_st->{dev} != $st->dev) {
> +	$hwaddress_st->{mtime} = $st->mtime;
> +	$hwaddress_st->{ino} = $st->ino;
> +	$hwaddress_st->{dev} = $st->dev;
>  
> -    my $fn = '/etc/ssh/ssh_host_rsa_key.pub';
> -    my $sshkey = PVE::Tools::file_get_contents($fn);
> -    $hwaddress = uc(Digest::MD5::md5_hex($sshkey));
> +	$hwaddress = get_server_id();
> +    }
>  
>      return $hwaddress;
>  }