Fix encoding of SSH certs with critical options

[lkubb]

This corrects loading and serialization of SSH certificate critical options/extensions with non-empty values. Note that it does not add support for value types other than string since there is no official option that uses any.

Also breaks loading of certificates that were generated with previous versions of cryptography, I'm not sure if you want to keep backwards-compatibility with that custom format.

Thanks for your hard work on cryptography!

Fixes: #9207

[alex]

Thanks for sending a PR. Can you please add an extra test case so that this has full coverage?

[lkubb]

Thanks for sending a PR. Can you please add an extra test case so that this has full coverage?

Yup, on it already. Would it be sufficient to add a single new vector that tests unexpected data for critical options only or should I add one for extensions as well?

[alex]

Only one is required, but having both is fine.

[reaperhulk]

Thanks for catching this. That is a very confusing subtlety for a spec that is otherwise pretty well documented!

[reaperhulk]

So when we encode an empty string for value should we expect to see UNKNOWN FLAG OPTION as part of the parsed output for ssh-keygen?

[lkubb]

So when we encode an empty string for value should we expect to see UNKNOWN FLAG OPTION as part of the parsed output for ssh-keygen?

No, empty strings should behave as before. UNKNOWN FLAG OPTION indicates the option/extension name is not specified as an official one.

[reaperhulk]

When I parse p256-ed25519-non-singular-ext-val.pub I get:

p256-ed25519-non-singular-ext-val.pub:
        Type: ecdsa-sha2-nistp256-cert-v01@openssh.com user certificate
        Public key: ECDSA-CERT SHA256:W6Wr6d8N5R5y1rzZl8L03NTgrxc8adxeET7GkXdJSvU
        Signing CA: ED25519 SHA256:knottK/0LBWlxvM2cDgzzCJdQ0ppFlY/hzlHWlZTOLk (using ssh-ed25519)
        Key ID: ""
        Serial: 0
        Valid: from 2022-12-31T18:00:00 to 2023-01-01T18:00:00
        Principals: (none)
        Critical Options: 
                verify-required UNKNOWN FLAG OPTION
        Extensions: 
                contains-extra-value UNKNOWN OPTION: 0000000568656c6c6f0000000620776f726c64 (len 19)

So we get the error in extensions that we expect, but I wouldn't have expected verify-required to display that message given that it is an official option.

[lkubb]

That is indeed surprising, I will investigate when I have the time later. Note that I cannot reproduce this with ssh-keygen 9.3:

ssh-keygen -L -f vectors/cryptography_vectors/asymmetric/OpenSSH/certs/p256-ed25519-non-singular-ext-val.pub
vectors/cryptography_vectors/asymmetric/OpenSSH/certs/p256-ed25519-non-singular-ext-val.pub:
        Type: ecdsa-sha2-nistp256-cert-v01@openssh.com user certificate
        Public key: ECDSA-CERT SHA256:W6Wr6d8N5R5y1rzZl8L03NTgrxc8adxeET7GkXdJSvU
        Signing CA: ED25519 SHA256:knottK/0LBWlxvM2cDgzzCJdQ0ppFlY/hzlHWlZTOLk (using ssh-ed25519)
        Key ID: ""
        Serial: 0
        Valid: from 2023-01-01T00:00:00 to 2023-01-02T00:00:00
        Principals: (none)
        Critical Options:
                verify-required
        Extensions:
                contains-extra-value UNKNOWN OPTION: 0000000568656c6c6f0000000620776f726c64 (len 19)

[lkubb]

@reaperhulk

It seems verify-required was added to ssh-keygen in release 9.1, hence it's unknown below that version:

• ssh-keygen(1): implement the "verify-required" certificate option.
  This was already documented when support for user-verified FIDO
  keys was added, but the ssh-keygen(1) code was missing.

https://www.openssh.com/txt/release-9.1

[reaperhulk]

And I'm on 9.0, so alright then 😄