Introduce Public_Key::raw_public_key_bits() #3985

reneme · 2024-04-08T15:01:54Z

Context

As suggested in #3706, this introduces a new top-level method for all public keys: Public_Key::raw_public_key_bits(), additional to ::public_key_bits(). Similarly, Private_Key already provides private_key_bits() and raw_private_key_bits() (#3437).

For a reference of how this new method is useful see the changes in the TLS 1.3 hybrid key handling.

Pull Request Dependencies

With the old concat helper, there was an obscure compile error on S390. I'm assuming some sort of alignment issue.

Generic Description

For most algorithms this returns the canonical byte encoding of the public key's intrinsic object(s). The byte buffer of the raw_...() methods typically does not include any algorithm or parameter specifiers.

This is particularly useful for the algorithms of the PQC NIST competition where the public key encoding is a simple byte-concatenation of the internal objects.
Some algorithms may not have such a canonical byte serialization and may throw Not_Implemented in this case (e.g. RSA).

Key Exchange Schemes

Note that key exchange schemes will return the canonical 'public_value' which wasn't easily accessible from a generic Public_Key or Private_Key before. Until now, one had to downcast the private key into a PK_Key_Agreement_Key and invoke public_value(). This was cumbersome (for instance in the implementation of hybrid PQ/T TLS 1.3 - #3609)

Testing

I've somewhat abused the PK_Key_Generation_Test to exercise round-trips of encoding the raw key data and decoding it with algorithm-specific domain knowledge. Basically, there's a new method that subclasses of this test must implement and return a generic Botan::Public_Key object constructed from the output of raw_public_key_bits() and some parameter context information.

Future Work

For the new PQC algorithms we currently return the same bare byte concatenation of the key for both public_key_bits() and raw_public_key_bits(). Technically this is incorrect. Earlier, we did opt to not implement any pre-mature ASN.1 specification for PQC keys.

I suggest to depart from this behavior when implementing the final standards and let public_key_bits() either throw Not_Implemented, or (eventually) return a standardized ASN.1 encoding.

randombit

Seems fine to me. We should document somewhere the anticipated change to public_key_bits once an asn1 encoding for the pqc algorithms is defined.

src/lib/tls/tls13_pqc/kex_to_kem_adapter.cpp

FAlbertDev

LGTM so far :)

reneme · 2024-04-09T14:35:18Z

We should document somewhere the anticipated change to public_key_bits once an asn1 encoding for the pqc algorithms is defined.

I added some details in pubkey.rst.

coveralls · 2024-04-10T10:14:20Z

coverage: 91.851% (+0.001%) from 91.85%
when pulling db3cbe6 on Rohde-Schwarz:feature/raw_public_key_bits
into 19567e3 on randombit:master.

randombit

Another pass

randombit · 2024-04-12T09:52:43Z

doc/api_ref/pubkey.rst

+      Returns a binary representation of the public key's canonical structure.
+      Typically, this does not include any metadata like an algorithm identifier
+      or parameter set. Decoding this might require knowledge of the algorithm
+      and parameters used to generate the key.


This should also mention that some algorithms like RSA do not have such a representation and will throw Not_Implemented

randombit · 2024-04-12T10:00:03Z

src/lib/pubkey/dilithium/dilithium_common/dilithium.cpp

 std::vector<uint8_t> Dilithium_PublicKey::public_key_bits() const {
+   // Technically, that's not really correct. The docstring for public_key_bits()
+   // states that it should return a BER-encoding of the public key.


I think the doc for public_key_bits is not really correct/precise. It should return the bytes that, if we stuff it into an X.509 SPKI, result in a valid public key DER encoding. For example for X25519 we just return the 32 byte encoded point with no outer encoding, because that's what RFC 8410 specifies we should do.

So the effect of the PQC algorithms returning raw_public_key_bits here is that we just stuff the "native" representation of the key as defined by the various reference implementations into the BIT STRING of the ASN.1 structure. Which is IMO the correct thing to do, but who knows what will end up getting standardized.

I changed this (and the other similar) comments to:

// Currently, there isn't a finalized definition of an ASN.1 structure for // Dilithium aka ML-DSA public keys. Therefore, we return the raw public key bits.

Regarding API stability and "the principle of least surprise": I'm hoping that a finalized implementation of ML-DSA (and the other to-be-standardized algorithms) would return a standardized ASN.1 structure here. While the pre-standard implementations stay with raw_public_key_bits() until they are deprecated and eventually phased out. (I'm open for better suggestions.)

randombit · 2024-04-12T10:04:08Z

src/tests/test_pubkey.cpp

@@ -630,6 +630,31 @@ std::vector<Test::Result> PK_Key_Generation_Test::run() {
                           !public_key->supports_operation(Botan::PublicKeyOperation::KeyAgreement));
         }

+         // Test that the raw public key can be encoded. RSA is excused, as it
+         // does not have an obvious raw encoding.
+         if(public_key->algo_name() != "RSA") {


I'd rephrase this test slightly.

Always invoke raw_public_key_bits (for example, here we never call it on RSA, so if it simply crashed or threw some unexpected exception, we wouldn't notice).

The test may throw an exception. If the exception is not Not_Implemented, failure. If the exception is Not_Implemented, failure unless algo_name() == "RSA".

Rest of test as is.

randombit · 2024-04-12T10:06:32Z

src/lib/pubkey/pk_keys.h

+      *
+      * Note: some algorithms (for example RSA) do not have an obvious encoding
+      * for this value due to having many different values, and thus not implement
+      * this function.


Also mention that this function (should) always throw Not_Implemented in this case.

randombit · 2024-04-12T10:09:27Z

src/lib/pubkey/dilithium/dilithium_common/dilithium.cpp

 std::vector<uint8_t> Dilithium_PublicKey::public_key_bits() const {
+   // Technically, that's not really correct. The docstring for public_key_bits()
+   // states that it should return a BER-encoding of the public key.
   return m_public->raw_pk();


For cases like this where we are currently having an equivalence between raw_public_key_bits and public_key_bits I'd prefer the latter invoke the former so this relation is clear.

reneme · 2024-05-23T09:44:33Z

Rebased to latest master (resolved a few conflicts) and addressed all review remarks. This is now ready for a (hopefully) final review pass. /cc @randombit

randombit

LGTM. One minor test nit

src/tests/test_pubkey.cpp

For most algorithms this returns the canonical byte encoding of the public key's intrinsic object(s). This is particularly useful for the algorithms of the PQC NIST competition where the public key encoding is a simple byte-concatenation of the internal objects. Some algorithm may not have such a canonical byte serialization and may throw Not_Implemented in this case (e.g. RSA). For Key Exchange Schemes this is returning the bare 'public_value'.

The encodings returned by Public_Key::raw_public_key_bits() do not contain any identifiers of the algorithms' parameter sets. Instead, we have to infer them from the test data.

reneme · 2024-05-23T13:43:00Z

Rebased again after #3716 just got merged. LMS always contains the necessary algorithm information in its encoding, so the implementation of raw_public_key_bits() is trivial.

reneme added this to the Botan 3.5.0 milestone Apr 8, 2024

reneme requested a review from randombit April 8, 2024 15:01

reneme self-assigned this Apr 8, 2024

reneme mentioned this pull request Apr 8, 2024

Improve API of asymmetric algorithms #3706

Open

5 tasks

reneme requested a review from FAlbertDev April 8, 2024 15:08

randombit reviewed Apr 8, 2024

View reviewed changes

src/lib/tls/tls13_pqc/kex_to_kem_adapter.cpp Show resolved Hide resolved

reneme force-pushed the feature/raw_public_key_bits branch from 90ec5a4 to a49fafe Compare April 9, 2024 06:36

FAlbertDev reviewed Apr 9, 2024

View reviewed changes

reneme force-pushed the feature/raw_public_key_bits branch from a49fafe to 1c9ed51 Compare April 9, 2024 14:12

reneme mentioned this pull request Apr 9, 2024

[std::span] some pubkey things #3993

Merged

reneme marked this pull request as ready for review April 9, 2024 14:37

reneme requested a review from randombit April 9, 2024 14:37

reneme force-pushed the feature/raw_public_key_bits branch 2 times, most recently from 8443b28 to 7f77a39 Compare April 10, 2024 09:30

randombit reviewed Apr 12, 2024

View reviewed changes

reneme force-pushed the feature/raw_public_key_bits branch 2 times, most recently from 7a22fb8 to e516c86 Compare May 23, 2024 09:43

reneme requested a review from randombit May 23, 2024 09:44

randombit approved these changes May 23, 2024

View reviewed changes

src/tests/test_pubkey.cpp Show resolved Hide resolved

reneme added 3 commits May 23, 2024 15:41

TEST: reconstruct keys from raw_public_key_bits

c5da3cb

The encodings returned by Public_Key::raw_public_key_bits() do not contain any identifiers of the algorithms' parameter sets. Instead, we have to infer them from the test data.

Doc: details about Public_Key::(raw_)public_key_bits()

db3cbe6

reneme force-pushed the feature/raw_public_key_bits branch from e516c86 to db3cbe6 Compare May 23, 2024 13:41

reneme merged commit 2d52a43 into randombit:master May 23, 2024
43 checks passed

reneme mentioned this pull request May 24, 2024

PQC: Classic McEliece #3883

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Public_Key::raw_public_key_bits() #3985

Introduce Public_Key::raw_public_key_bits() #3985

reneme commented Apr 8, 2024 •

edited

randombit left a comment

FAlbertDev left a comment

reneme commented Apr 9, 2024

coveralls commented Apr 10, 2024 •

edited

randombit left a comment

randombit Apr 12, 2024

randombit Apr 12, 2024

randombit Apr 12, 2024

reneme May 23, 2024

randombit Apr 12, 2024 •

edited

randombit Apr 12, 2024

randombit Apr 12, 2024

reneme commented May 23, 2024

randombit left a comment

reneme commented May 23, 2024

Introduce Public_Key::raw_public_key_bits() #3985

Introduce Public_Key::raw_public_key_bits() #3985

Conversation

reneme commented Apr 8, 2024 • edited

Context

Pull Request Dependencies

Generic Description

Key Exchange Schemes

Testing

Future Work

randombit left a comment

Choose a reason for hiding this comment

FAlbertDev left a comment

Choose a reason for hiding this comment

reneme commented Apr 9, 2024

coveralls commented Apr 10, 2024 • edited

randombit left a comment

Choose a reason for hiding this comment

randombit Apr 12, 2024

Choose a reason for hiding this comment

randombit Apr 12, 2024

Choose a reason for hiding this comment

randombit Apr 12, 2024

Choose a reason for hiding this comment

reneme May 23, 2024

Choose a reason for hiding this comment

randombit Apr 12, 2024 • edited

Choose a reason for hiding this comment

randombit Apr 12, 2024

Choose a reason for hiding this comment

randombit Apr 12, 2024

Choose a reason for hiding this comment

reneme commented May 23, 2024

randombit left a comment

Choose a reason for hiding this comment

reneme commented May 23, 2024

reneme commented Apr 8, 2024 •

edited

coveralls commented Apr 10, 2024 •

edited

randombit Apr 12, 2024 •

edited