[pve-devel] [PATCH acme] Fix EBA MAC key decoding
Accroding to RFC 8555:
> The MAC key SHOULD be provided in base64url-encoded form...
However, currently we are only decoding the MAC key as base64.
This patch uses the correct function to decode the user provided
MAC key as base64url format. This can fix authentication error
when a user uses command `pvenode acme account register` and paste
the EBA MAC key as prompted.
Signed-off-by: YU Jincheng
---
src/PVE/ACME.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
index bf5410d..428cdda 100644
--- a/src/PVE/ACME.pm
+++ b/src/PVE/ACME.pm
@@ -7,7 +7,7 @@ use POSIX;
use Data::Dumper;
use Date::Parse;
-use MIME::Base64 qw(encode_base64url decode_base64);
+use MIME::Base64 qw(encode_base64url decode_base64url);
use File::Path qw(make_path);
use JSON;
use Digest::SHA qw(sha256 sha256_hex hmac_sha256);
@@ -365,7 +365,7 @@ sub new_account {
my %payload = ( contact => $info{contact} );
if (defined($info{eab})) {
- my $eab_hmac_key = decode_base64($info{eab}->{hmac_key});
+ my $eab_hmac_key = decode_base64url($info{eab}->{hmac_key});
$payload{externalAccountBinding} = external_account_binding_jws(
$info{eab}->{kid},
$eab_hmac_key,
--
2.39.3 (Apple Git-145)
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH v2 storage] lvm: improve warning in case vgs output contains unexpected lines
If the metadata archive under /etc/lvm/archive for a particular VG has
a lot of files or is overly large, `vgs` occasionally prints a message
to stdout [1]. Currently, the LVM plugin tries to parse this message
and thus produces the following confusing warnings in the output of
`pvesm status` or the journal (via pvestatd):
Use of uninitialized value $size in int at [...]/LVMPlugin.pm line 133
Use of uninitialized value $free in int at [...]/LVMPlugin.pm line 133
Use of uninitialized value $lvcount in int [...]/LVMPlugin.pm line 133
Reported in enterprise support where LVM picked up on VGs on VM disks
(due to a missing KRBD device filter in the LVM config), and since
several VM disks had VGs with the same name, LVM quickly produced a
lot of files in /etc/lvm/archive.
Reproducible as follows with a VG named `spam`:
for i in $(seq 8192);
do vgrename spam spam2; vgrename spam2 spam; done
rm /etc/lvm/backup/spam; vgs -o vg_name
Output (linebreak for readability):
Consider pruning spam VG archive with more then 8 MiB in 8268\n
files (check archiving is needed in lvm.conf).
VG
spam
With this patch, the LVM plugin instead prints a human-readable
warning about the unexpected line.
[1]
https://sourceware.org/git/?p=lvm2.git;a=blob;f=lib/format_text/archive.c;h=5acf0c04a;hb=38e0c7a1#l222
Signed-off-by: Friedrich Weber
---
Notes:
changes from v1 [2]:
* warn about the unexpected line instead of simply ignoring it
[2] https://lists.proxmox.com/pipermail/pve-devel/2024-January/061333.html
src/PVE/Storage/LVMPlugin.pm | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
index 4b951e7..5377823 100644
--- a/src/PVE/Storage/LVMPlugin.pm
+++ b/src/PVE/Storage/LVMPlugin.pm
@@ -130,6 +130,11 @@ sub lvm_vgs {
my ($name, $size, $free, $lvcount, $pvname, $pvsize, $pvfree) =
split (':', $line);
+ if (!defined($size) || !defined($free) || !defined($lvcount)) {
+ warn "unexpected output from vgs: $line\n";
+ return;
+ }
+
$vgs->{$name} //= {
size => int ($size),
free => int ($free),
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH widget-toolkit] certificates: don't display name if there is no name
The default certificate does not have a name.
Reported-by: Dietmar Maurer
Signed-off-by: Maximiliano Sandoval
---
src/panel/Certificates.js | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/panel/Certificates.js b/src/panel/Certificates.js
index a522ab6..f924512 100644
--- a/src/panel/Certificates.js
+++ b/src/panel/Certificates.js
@@ -237,10 +237,16 @@ Ext.define('Proxmox.panel.Certificates', {
{
xtype: 'proxmoxButton',
text: gettext('Delete Custom Certificate'),
- confirmMsg: rec => Ext.String.format(
- gettext('Are you sure you want to remove the certificate
used for {0}'),
- me.certById[rec.id].name,
- ),
+ confirmMsg: function(rec) {
+ let cert = me.certById[rec.id];
+ if (cert.name) {
+ return Ext.String.format(
+ gettext('Are you sure you want to remove the
certificate used for {0}'),
+ cert.name,
+ );
+ }
+ return Ext.String.format(gettext('Are you sure you want to
remove the certificate'));
+ },
callback: () => me.reload(),
selModel: me.selModel,
disabled: true,
--
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 acme] Fix EBA MAC key decoding
Hey,
thanks for contributing!
Your right, the RFC suggests that the key the user supplies should be
interpreted as base64url and not base64. Since I'm not the first and
probably won't be the last to run into this mistake, I wonder if it
might be worth to be a bit more lenient and implement a simple check on
the '/' and '+' characters, to check if base64 or base64url has been
used to encode the key.
Tested-By: Folke Gleumes
On Thu, 2024-01-18 at 18:40 +0800, YU Jincheng wrote:
> Accroding to RFC 8555:
> > The MAC key SHOULD be provided in base64url-encoded form...
>
> However, currently we are only decoding the MAC key as base64.
> This patch uses the correct function to decode the user provided
> MAC key as base64url format. This can fix authentication error
> when a user uses command `pvenode acme account register` and paste
> the EBA MAC key as prompted.
>
> Signed-off-by: YU Jincheng
> ---
> src/PVE/ACME.pm | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/PVE/ACME.pm b/src/PVE/ACME.pm
> index bf5410d..428cdda 100644
> --- a/src/PVE/ACME.pm
> +++ b/src/PVE/ACME.pm
> @@ -7,7 +7,7 @@ use POSIX;
>
> use Data::Dumper;
> use Date::Parse;
> -use MIME::Base64 qw(encode_base64url decode_base64);
> +use MIME::Base64 qw(encode_base64url decode_base64url);
> use File::Path qw(make_path);
> use JSON;
> use Digest::SHA qw(sha256 sha256_hex hmac_sha256);
> @@ -365,7 +365,7 @@ sub new_account {
> my %payload = ( contact => $info{contact} );
>
> if (defined($info{eab})) {
> - my $eab_hmac_key = decode_base64($info{eab}->{hmac_key});
> + my $eab_hmac_key = decode_base64url($info{eab}->{hmac_key});
> $payload{externalAccountBinding} =
> external_account_binding_jws(
> $info{eab}->{kid},
> $eab_hmac_key,
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH cluster 4/4] pvecm: stop merging SSH known hosts by default
--- Begin Message ---
On Thu, Jan 11, 2024 at 11:51:18AM +0100, Fabian Grünbichler wrote:
> and allow explicitly unmerging to remove the symlink altogether.
Apologies if I am second guessing here, but this is meant to be explicitly
later "unmerging" on pveproxy start of new version of PVE? If so, this is risky
if people used the shared known_hosts for own keys to reach e.g. backup
servers, etc. It would be much safer to leave it as-is (or provide the facility
otherwise as opt-in).
>
> Signed-off-by: Fabian Grünbichler
> ---
> src/PVE/CLI/pvecm.pm | 10 --
> src/PVE/Cluster/Setup.pm | 9 ++---
> 2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/src/PVE/CLI/pvecm.pm b/src/PVE/CLI/pvecm.pm
> index 0005e4b..0e8ca8f 100755
> --- a/src/PVE/CLI/pvecm.pm
> +++ b/src/PVE/CLI/pvecm.pm
> @@ -567,12 +567,18 @@ __PACKAGE__->register_method ({
> type => 'boolean',
> optional => 1,
> },
> + 'unmerge-known-hosts' => {
> + description => "Unmerge legacy SSH known hosts.",
> + type => 'boolean',
> + optional => 1,
> + default => 0,
> + },
> },
> },
> returns => { type => 'null' },
> code => sub {
> my ($param) = @_;
> - my ($force_new_cert, $silent) = $param->@{qw(force silent)};
> + my ($force_new_cert, $silent, $unmerge) = $param->@{qw(force silent
> unmerge-known-hosts)};
>
> # pveproxy's ExecStartPre calls this, and as we do IO (on /etc/pve)
> that can hang
> # (uninterruptible D state) we could fail the whole service, rendering
> the API guaranteed
> @@ -585,7 +591,7 @@ __PACKAGE__->register_method ({
> usleep(100 * 1000);
> }
>
> - PVE::Cluster::Setup::updatecerts_and_ssh($force_new_cert, $silent);
> + PVE::Cluster::Setup::updatecerts_and_ssh($force_new_cert, $silent,
> $unmerge);
> PVE::Cluster::prepare_observed_file_basedirs();
> });
> if ($got_timeout) {
> diff --git a/src/PVE/Cluster/Setup.pm b/src/PVE/Cluster/Setup.pm
> index 4b6f013..42dff85 100644
> --- a/src/PVE/Cluster/Setup.pm
> +++ b/src/PVE/Cluster/Setup.pm
> @@ -816,7 +816,7 @@ sub generate_local_files {
> }
>
> sub updatecerts_and_ssh {
> -my ($force_new_cert, $silent) = @_;
> +my ($force_new_cert, $silent, $unmerge_ssh) = @_;
>
> my $p = sub { print "$_[0]\n" if !$silent };
>
> @@ -834,9 +834,12 @@ sub updatecerts_and_ssh {
> $p->("generate new node certificate") if $force_new_cert;
> gen_pve_node_files($nodename, $local_ip_address, $force_new_cert);
>
> -$p->("merge authorized SSH keys and known hosts");
> +$p->("merge authorized SSH keys");
> ssh_merge_keys();
> -ssh_merge_known_hosts($nodename, $local_ip_address, 1);
> +if ($unmerge_ssh) {
> + $p->("unmerge SSH known hosts");
> + ssh_unmerge_known_hosts();
> +}
> ssh_create_node_known_hosts($nodename);
> gen_pve_vzdump_files();
> }
> --
> 2.39.2
>
>
>
> ___
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
--- End Message ---
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH cluster 2/4] fix #4886: SSH: pin node's host key if available
--- Begin Message --- I have just re-read the cover and looked at the applied changes as of now (assuming nothing more is coming) and considering what it was supposed to fix (4886 only) and what it breaks (original myself quoted below). I can't help, but wonder, if the whole change was about avoiding having ssh-keygen -R fail to produce expected result on local symlink, why all the code changes (if they do not bring anything additional). This (with the limited scope only) could have been accomplished very safely by dropping into /etc/ssh/ssh_config.d/: > GlobalKnownHostsFile /etc/ssh/ssh_known_hosts > /etc/pve/nodes/%k/ssh_known_hosts That's if snippets are required to be supported. It would have also worked just fine with the combined file on pmxcfs (easier to maintain if only node keys are in scope and thus no hashed format support needed.) On Tue, Jan 16, 2024 at 01:39:56PM +, Esi Y via pve-devel wrote: > Date: Tue, 16 Jan 2024 13:39:56 + > From: Esi Y > To: pve-devel@lists.proxmox.com > Subject: Re: [pve-devel] [PATCH cluster 2/4] fix #4886: SSH: pin node's > host key if available > > Thank you for the responses below, I just want to explain all the fuss in my > reasoning, i.e. why I am bringing it up before this goes into a release. > > The original way of handling SSH was meant to be pretty transparent to the > user: > > A. If they ran plain ssh, without any extra argv[], it would "just work" and > it would not go on creating extra duplicate entries in e.g. local files. > B. If they added some stuff of their own, intuitively into local user config, > it would "just blend in" and magically work for PVE-* code. > C. The "hijacked" part of the SSH config was the global one, but this made > most sense logically as why would a machine that is part of a cluster need > distinctive single-host-wide config. > D. Even then, the "hijacked" file was well-known (pun intended) format and > use case (this I think is important), the idea was that the existing (ssh) > tooling follows the symlinks. It was meant to be transparent, i.e. adding > global item should be added globally, extrapolated to cluste-wide. > > The code below runs well for PVE-* tooling and it plays nice with the old > versions of PVE, but it does break: A, B, C and creates at least some > inconvenience with D (I would expect extra support case load). Arguably the > code now fixed the logical bug (not playing nice with ssh tooling) by simply > removing C altogether. > > > I. The case for combo of UserKnownHostsFile (UserKHF) and > GlobalKnownHostsFile (GlobalKHF) and use of "none" (in either case): > > I-1. Injecting UserKHF instead of GlobalKHF for injecting the pinned keys > breaks B, changes known behaviour C, there's no benefit to use it in this > reversed combo. > > I-2. Use of "none" (suppose we are talking UserKHF now in respect to (1) > above) does not gain anything (maybe except for clarity during alpha testing) > as you agree based on (2) in the quoted below, omitting it allows B to be > preserved. > > > II. The case for KnownHostsCommand (KnownHC): > > II-1. Even with amendments from I above, the A remains broken for human user, > they might start piling up stale (duplicate) keys by invoking commands > towards other cluster members without -o's, they will not know why e.g. PVE-* > tooling works, but theirs only works from one node, but not another, or only > with migration IP, not regular one, or why config change of PVE did not > reflect into their shell experience. With KnownHC injected in their user > config, this will never become an issue. > > II-2. Should a user become acquainted with the two (now mandatory) -o's, they > will at some point start running into cases where e.g. dues to > StrictHostKeyChecking (StrictHKC), they are asked to add a machine, only to > be asked again and again due to (3) as quoted below. With KnownHC injected in > their user config, this can never happen. > > II-3. Even better than the original setup, the use of KnownHC allows for e.g. > adjustment in case of migration network is changed. I.e. the next ssh command > trying to reach that new IP will be instantly pointed to the right hostkey, > no need for somehow updating it in further locations. > > II-4. Also, KnownHC allows for very good logging of situations when there is > something wrong with specifically cluster hostkeys. The command is only > executed if there was no match in UserKHF and GlobalKHF, there is no way it > would silently fail because everytime there was no match in those two, the > command will get to have its last say. So you catch every error explicitly, > it won't become a red herring ruon forums. > > II-5. The KnownHC is basically equivalent to -o GlobalKHF > extra_maintained_file_that_can_become_stale, without taking away the Global > feature from the user (and even current cluster config). > > II-6. The KnownHC allows for much easier management of multipl
