From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 1643C1FF09C for ; Mon, 02 Nov 2026 15:07:36 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 96D3121788; Mon, 02 Nov 2026 15:07:34 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Mon, 02 Nov 2026 15:07:29 +0100 Message-Id: Subject: Re: [PATCH common 1/2] ticket: accept RSA tickets signed with SHA-256 To: "Mark Atwood" , X-Mailer: aerc 0.20.0 References: <20261023185749.509347-1-mark@wolfssl.com> In-Reply-To: <20261023185749.509347-1-mark@wolfssl.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793628449305 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.669 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 56WFOAGNHP3THIURDDUBYSEDQG5N6D45 X-Message-ID-Hash: 56WFOAGNHP3THIURDDUBYSEDQG5N6D45 X-MailFrom: s.sterz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri Oct 23, 2026 at 8:57 PM CEST, Mark Atwood wrote: > Crypt::OpenSSL::RSA signs and verifies with SHA-1 unless told otherwise, > so all RSA tickets (login, TFA, VNC, tunnel) are RSA PKCS#1 v1.5 > signatures over SHA-1. NIST SP 800-131A Rev. 2 no longer allows SHA-1 for > creating digital signatures. FIPS providers for OpenSSL 3 can enforce > this, for example wolfProvider, or the OpenSSL FIPS provider with its > signature digest check enabled. With such a provider no ticket can be > issued and nobody can log in. > > As a first step, make verification accept tickets signed with SHA-256 in > addition to SHA-1. Once all nodes of a cluster run a version with this > change, the signing side can switch to SHA-256 without locking users out > of nodes that were not yet upgraded. > > The hash is set explicitly before each attempt, since callers cache and > reuse the key object. A provider that does not allow SHA-1 raises an > error from verify() instead of returning false, so the call is wrapped > in eval and such a ticket is treated like any other invalid ticket. > > Add a test that signs tickets directly with Crypt::OpenSSL::RSA using > either hash, and checks that they are accepted and that modified, > expired, foreign-key or wrong-secret tickets are rejected. > > Signed-off-by: Mark Atwood > --- > src/PVE/Ticket.pm | 15 +++++++++- > test/Makefile | 1 + > test/ticket_test.pl | 68 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 83 insertions(+), 1 deletion(-) > create mode 100755 test/ticket_test.pl > > diff --git a/src/PVE/Ticket.pm b/src/PVE/Ticket.pm > index 764c02d..f700f39 100644 > --- a/src/PVE/Ticket.pm > +++ b/src/PVE/Ticket.pm > @@ -79,8 +79,21 @@ sub verify_rsa_ticket { > my $sig =3D $2; > > my $full =3D defined($secret_data) ? "$plain:$secret_data" : $pl= ain; > + my $raw_sig =3D decode_base64($sig); > + > + # Older versions sign with SHA-1, so during a rolling upgrade a = cluster has tickets > + # with either hash. A provider that refuses SHA-1 raises an erro= r rather than > + # returning false, which simply makes such a ticket invalid. nit: This could probably benefit from comment like: # TODO: Remove SHA-1 fallback once SHA-256 signed tickets are the # default across PMG and PVE for at least one major release. > + my $valid; > + for my $use_hash ('use_sha256_hash', 'use_sha1_hash') { > + $valid =3D eval { > + $rsa_pub->$use_hash(); > + $rsa_pub->verify($full, $raw_sig); It might be worth to add documentation that the state of the RSA pub key object may be modified depending on what ticket is being verified. If a SHA-256 ticket is verified, other users of `$rsa_pub` will now use SHA-256 for verifying signatures in other contexts; if the ticket is SHA-1, then SHA-1 will be used. This can make it hard to tell from a caller's perspective in what state the object will be after a call to this function. IMO, this should at least be documented or maybe we should reset the object after the last `verify()` call unconditionally. What do you think? Note: From a quick check, none of the users should have a reason to call `verify()` outside of the functions in `PVE::Ticket`. Here you always correctly set the hash function before using the object, so it shouldn't be an issue in practice. > + }; The eval call also swallows *all* errors that could happen during verification, might be worth checking what `$@` contains and warning about it. > + last if $valid; > + } > > - if ($rsa_pub->verify($full, decode_base64($sig))) { > + if ($valid) { > if ($plain =3D~ m/^\Q$prefix\E:(?:(\S+):)?([A-Z0-9]{8})$/) { > my $data =3D $1; # Note: not all tickets contains data > my $timestamp =3D $2; > diff --git a/test/Makefile b/test/Makefile > index 1bce185..7eac9ea 100644 > --- a/test/Makefile > +++ b/test/Makefile > @@ -16,6 +16,7 @@ TESTS =3D lock_file.test \ > section_config_property_isolation_test.test \ > file-test.test \ > cmd-test.test \ > + ticket_test.test \ > > all: > > diff --git a/test/ticket_test.pl b/test/ticket_test.pl > new file mode 100755 > index 0000000..37e30b4 > --- /dev/null > +++ b/test/ticket_test.pl > @@ -0,0 +1,68 @@ > +#!/usr/bin/perl > + > +# Tests for signing and verifying RSA tickets with PVE::Ticket > + > +use v5.36; > + > +use lib '../src'; > + > +use Crypt::OpenSSL::RSA; > +use MIME::Base64; > +use Test::More; > + > +use PVE::Ticket; > + > +my $rsa_priv =3D Crypt::OpenSSL::RSA->generate_key(2048); > +my $rsa_pub =3D Crypt::OpenSSL::RSA->new_public_key($rsa_priv->get_publi= c_key_string()); > + > +# sign like a node would, with an explicitly chosen hash, independent of= PVE::Ticket > +sub sign_ticket($priv, $hash, $plain, $secret_data =3D undef) { > + my $full =3D defined($secret_data) ? "$plain:$secret_data" : $plain; > + my $use_hash =3D "use_${hash}_hash"; > + $priv->$use_hash(); > + return $plain . "::" . encode_base64($priv->sign($full), ''); > +} > + > +my $now =3D sprintf("%08X", time()); > +my $old =3D sprintf("%08X", time() - 7200); > + > +for my $hash (qw(sha256 sha1)) { > + my $ticket =3D sign_ticket($rsa_priv, $hash, "PVE:root\@pam:$now"); > + my ($data, $age) =3D PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVE',= $ticket, undef, -1, 60, 1); > + is($data, 'root@pam', "$hash signed ticket is accepted"); > + ok(defined($age) && $age >=3D 0 && $age < 60, "$hash signed ticket h= as plausible age"); > + > + $ticket =3D sign_ticket($rsa_priv, $hash, "PVEVNC:$now", 'vnc-secret= '); > + ok( > + PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVEVNC', $ticket, 'vnc= -secret', -20, 40, 1), > + "$hash signed ticket with secret data is accepted", > + ); > + ok( > + !PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVEVNC', $ticket, 'ot= her', -20, 40, 1), > + "$hash signed ticket with wrong secret data is rejected", > + ); > + > + $ticket =3D sign_ticket($rsa_priv, $hash, "PVE:root\@pam:$now"); > + (my $tampered =3D $ticket) =3D~ s/^PVE:root\@pam:/PVE:admin\@pam:/; > + ok( > + !PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVE', $tampered, unde= f, -1, 60, 1), > + "$hash signed ticket with modified data is rejected", > + ); > + eval { PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVE', $tampered, un= def, -1, 60) }; > + like($@, qr/invalid PVE ticket/, "$hash signed modified ticket raise= s error without noerr"); nit: Would benefit from a test case that checks whether verification succeeds without `$noerr` set too. > + > + $ticket =3D sign_ticket($rsa_priv, $hash, "PVE:root\@pam:$old"); > + ok( > + !PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVE', $ticket, undef,= -1, 60, 1), > + "$hash signed expired ticket is rejected", > + ); > + > + my $other_priv =3D Crypt::OpenSSL::RSA->generate_key(2048); > + $ticket =3D sign_ticket($other_priv, $hash, "PVE:root\@pam:$now"); > + ok( > + !PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVE', $ticket, undef,= -1, 60, 1), > + "$hash signed ticket from another key is rejected", > + ); > +} > + > +done_testing();