Re: [PATCH common 1/2] ticket: accept RSA tickets signed with SHA-256

["Shannon Sterz" <s.sterz@proxmox.com>]

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 <mark@wolfssl.com>
> ---
>  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 = $2;
>
>          my $full = defined($secret_data) ? "$plain:$secret_data" : $plain;
> +        my $raw_sig = 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 error 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 = 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 =~ m/^\Q$prefix\E:(?:(\S+):)?([A-Z0-9]{8})$/) {
>                  my $data = $1; # Note: not all tickets contains data
>                  my $timestamp = $2;
> diff --git a/test/Makefile b/test/Makefile
> index 1bce185..7eac9ea 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -16,6 +16,7 @@ TESTS = 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 = Crypt::OpenSSL::RSA->generate_key(2048);
> +my $rsa_pub = Crypt::OpenSSL::RSA->new_public_key($rsa_priv->get_public_key_string());
> +
> +# sign like a node would, with an explicitly chosen hash, independent of PVE::Ticket
> +sub sign_ticket($priv, $hash, $plain, $secret_data = undef) {
> +    my $full = defined($secret_data) ? "$plain:$secret_data" : $plain;
> +    my $use_hash = "use_${hash}_hash";
> +    $priv->$use_hash();
> +    return $plain . "::" . encode_base64($priv->sign($full), '');
> +}
> +
> +my $now = sprintf("%08X", time());
> +my $old = sprintf("%08X", time() - 7200);
> +
> +for my $hash (qw(sha256 sha1)) {
> +    my $ticket = sign_ticket($rsa_priv, $hash, "PVE:root\@pam:$now");
> +    my ($data, $age) = 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 >= 0 && $age < 60, "$hash signed ticket has plausible age");
> +
> +    $ticket = 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, 'other', -20, 40, 1),
> +        "$hash signed ticket with wrong secret data is rejected",
> +    );
> +
> +    $ticket = sign_ticket($rsa_priv, $hash, "PVE:root\@pam:$now");
> +    (my $tampered = $ticket) =~ s/^PVE:root\@pam:/PVE:admin\@pam:/;
> +    ok(
> +        !PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVE', $tampered, undef, -1, 60, 1),
> +        "$hash signed ticket with modified data is rejected",
> +    );
> +    eval { PVE::Ticket::verify_rsa_ticket($rsa_pub, 'PVE', $tampered, undef, -1, 60) };
> +    like($@, qr/invalid PVE ticket/, "$hash signed modified ticket raises error without noerr");

nit: Would benefit from a test case that checks whether verification
succeeds without `$noerr` set too.

> +
> +    $ticket = 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 = Crypt::OpenSSL::RSA->generate_key(2048);
> +    $ticket = 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();