sign: Support x509 signature type #3278

ueno · 2024-07-12T04:34:30Z

The current "ed25519" signing type assumes raw Ed25519 key format for
both public and private keys. That requires custom processing of keys
after generated with openssl tools, and also lacks cryptographic
agility[1]; when Ed25519 becomes vulnerable, it would not be
straightforward to migrate to other algorithms, such as post-quantum
signature algorithms.

This patch adds a new signature type "x509" to use the key formats
natively supported by OpenSSL (PKCS#8 and SubjectPublicKeyInfo) and
capable of embedding algorithm identifier in an X.509 format.

The "x509" signature type prefers keys to be encoded in the PEM
format on disk, while it still accepts base64 encoded keys when given
through the command-line.

https://en.wikipedia.org/wiki/Cryptographic_agility

openshift-ci · 2024-07-12T04:34:42Z

Hi @ueno. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

cgwalters

Thank you so much for this well written patch!

Can you elaborate a bit more on why specficially you chose to work on this now? The rationale makes sense, but...are you planning to actually use Ed448 somewhere?

src/libostree/ostree-sign-ed25519.c

src/libotcore/otcore-ed25519-verify.c

cgwalters · 2024-07-12T12:09:46Z

src/libotcore/otcore-ed25519-verify.c

+  if (pkey == NULL && public_key_size == OSTREE_SIGN_ED25519_PUBKEY_SIZE)
+    pkey = EVP_PKEY_new_raw_public_key (EVP_PKEY_ED25519, NULL, public_key_buf, public_key_size);


I'd be curious if in practice other key types happen to exactly match the ed25519 size...not saying you should spend a lot of time digging to find out, but if you happen to know offhand, that'd be interesting.

I doubt it, but this is one of the reasons I think the PKCS8 handling should be in a separate backend and we should leave the existing raw ed25519 handling alone. Since signers and clients will need to be updated to handle the PKCS8 encoded keys, it might as well be "opt-in" in the form of a new signing type. Then the existing ed25519 signer can be left alone and there's no ambiguity about the key format it expects.

Certainly I think you'd want to have some common code for signing and verification once you've loaded the keys, though.

I doubt it, but this is one of the reasons I think the PKCS8 handling should be in a separate backend

Yeah...this makes total sense to me. What do you think @ueno ?

I agree; if we go that way, the following might be worth considering:

Keys are not always restricted to particular signing parameters (e.g., hash in ECDSA); perhaps it might make sense to embed signing parameters in the signature itself in that case

The current key format (one key per line) is not easy to deal with for longer keys (~2K with ML-DSA); maybe the new backend should support PEM/DER blocks instead (this could require additional API in OsteeSign)

It's still in progress but I've updated the PR with a new signing backend "x509". As a next step, I'll try to add the PEM based keysfile and make it default for the new backend (if that sounds ok), and also reduce the code duplication between "ed25519" and "x509" backends.

dbnicholson · 2024-07-12T16:11:50Z

I haven't reviewed the code yet, but this is awesome and how it should have been done initially. If you can inspect the key algorithm from a standard format, then there's no reason to specify what type of key it is.

Again, I haven't looked at the changes, but my question now becomes what to do with the "sign types". On the one hand, I kind of want to get rid of it entirely since the only reason to specify the type is to know how to interpret the keys. But if the key type can be automatically determined, then there's no need for that. However, there's also the dummy sign type, which is just used for testing.

I think the way I would handle this is:

Keep the existing ed25519 and dummy types. There are already people using the raw ed25519 keys and presumably we want to keep the dummy type for testing.
Add this new functionality in a new type called either pkcs8 or auto. I can't really think of what other form of signing would be desired besides a PKCS8 encoded asymmetric key, but maybe there could be something later. Theoretically, you could move the GPG signing under ostree-sign as a distinct pgp type. The presence of the 2 signing infrastructures is awkward, but I suspect this would be a lot of work and require a long migration period.
Deprecate the ed25519 type with directions for how to convert the key to pkcs8. We'd need to keep the ed25519 type for quite some time as both signers and clients need to be updated to handle PKCS8 keys.

dbnicholson · 2024-07-12T17:24:49Z

Add this new functionality in a new type called either pkcs8 or auto. I can't really think of what other form of signing would be desired besides a PKCS8 encoded asymmetric key, but maybe there could be something later.

I guess technically it's PKCS8 formatted private keys and X.509 SubjectPublicKeyInfo (SPKI) formatted public keys. So, pkcs8 wouldn't be totally accurate. I can't think of a better name, though.

ueno · 2024-07-12T23:40:40Z

Thank you for the reviews and comments so far!

Can you elaborate a bit more on why specficially you chose to work on this now? The rationale makes sense, but...are you planning to actually use Ed448 somewhere?

My main motivation here is to prepare for the upcoming PQC transition. While encryption (key encapsulation) is the current priority as it affects forward secrecy, signatures will come next, and it would be nice if ostree could support PQC signatures transparently.

Suggested in: ostreedev#3278 (comment) Signed-off-by: Daiki Ueno <[email protected]>

cgwalters · 2024-07-13T18:33:31Z

My main motivation here is to prepare for the upcoming PQC transition.

Got it! Makes sense...I think that's the kind of thing that's good to have in the commit message too.

Suggested in: ostreedev#3278 (comment) Signed-off-by: Daiki Ueno <[email protected]>

ueno · 2024-08-13T05:44:58Z

@cgwalters @dbnicholson There is still room for refactoring, but this is mostly feature complete from my point of view. PQ signatures can be used through oqsprovider in both pure/hybrid schemes:

# Create SPHINCS+ public and secret keys
$ openssl genpkey -provider oqsprovider -algorithm sphincssha2128fsimple -outform PEM -out sphincssha2128fsimple-sk.pem
$ openssl pkey -provider oqsprovider -outform PEM -pubout -in sphincssha2128fsimple-sk.pem -out sphincssha2128fsimple-pk.pem

# Set up an OSTree repo
$ mkdir repo work
$ cd work
$ ../ostree --repo=../repo init

# Make a signed commit
$ echo > a.txt
$ ../ostree --repo=../repo commit -b main -s "Signed with x509 module" --sign-from-file ../sphincssha2128fsimple-sk.pem --sign-type=x509
fac8d8df984644e6b814be093c3a5f60f746d90f1aacea020cd73dc22b2c33ce
$ COMMIT=$(../ostree --repo=../repo rev-parse main)
$ ../ostree --repo=../repo sign --verify --sign-type=x509 --keys-file=../sphincssha2128fsimple-pk.pem $COMMIT
x509: Signature verified successfully with key '302d300806062bce0f06040d0321007aac408c97612b9974785e2fcbcc35bfd4b31f7b95b0fc1026485dc8df66c95b'

For hybrid schemes, e.g., Ed25519+ML-KEM (Dilithium), you can simply sign the same commit twice for each algorithm.

cgwalters · 2024-08-16T21:59:42Z

/ok-to-test

src/libostree/ostree-sign-x509.c

cgwalters

Thanks so much for this! It overall looks really good! I mostly have nits and questions.

cgwalters · 2024-08-16T22:09:50Z

src/libostree/ostree-blob-reader-pem.c

+};
+
+#define PEM_SUFFIX "-----"
+#define PEM_PREFIX_BEGIN "-----BEGIN "


OpenSSL really doesn't have something for this? A lot of new string parsing in C is pretty unfortunate.

OpenSSL provides PEM_* functions indeed, though they don't fit in our use-case reading as many PEM blocks incrementally from a file (stream); PEM_read expects a single complete PEM block, while PEM_read_bio requires to implement a BIO method backed by GInputStream (that wouldn't be difficult, but cumbersome as it also requires to use correct memory allocators: OpenSSL vs. GLib).

cgwalters · 2024-08-16T22:12:59Z

tests/test-pem.c

+main (int argc, char **argv)
+{
+  g_test_init (&argc, &argv, NULL);
+  g_test_add_func ("/ostree_read_pem_block", test_ostree_read_pem_block);


Thanks for unit testing this! We should also add some corrupted/invalid test cases...this type of code is very fuzzable and we do have a fuzzer wired up here https://github.com/google/oss-fuzz/tree/master/projects/ostree

I added a couple more tests exercising invalid cases. Would it be OK to add fuzzing in a separate PR?

Yep fuzzing is nonblocking

tests/test-signed-commit-dummy.sh

cgwalters · 2024-08-16T22:22:46Z

src/libostree/ostree-blob-reader-base64.c

+
+  gsize n_elements;
+  g_base64_decode_inplace (line, &n_elements);
+  explicit_bzero (line + n_elements, len - n_elements);


This is worthy of the same comment you have in a similar place /* Don't leak the trailing encoded bytes */ later in the code (maybe also worthy of a shared helper function?).

But also...I don't fully grasp the threat model here...aren't the decoded bytes equally sensitive? In what cases would the trailing data in the malloc buffer be somehow accessible when the full buffer wouldn't?

Don't get me wrong, not arguing against it, but I would just like to understand a little bit more.

cgwalters · 2024-08-16T22:24:43Z

src/libostree/ostree-sign-ed25519.c

        return ret;

      /* Read the key itself */
-      /* base64 encoded key */
-      pk = g_variant_new_string (line);
+      pk = g_variant_new_fixed_array (G_VARIANT_TYPE ("y"), g_bytes_get_data (blob, NULL),


Hmm isn't this...oh, I see ostree_sign_ed25519_add_pk already accepted both strings and byte arrays.

src/libostree/ostree-sign-x509.c

cgwalters · 2024-08-16T22:27:45Z

src/libostree/ostree-sign-x509.c

+    return FALSE;
+
+  if (signatures == NULL)
+    return glnx_throw (error, "x509: commit have no signatures of my type");


Under what scenarios would this happen out of curiosity? Don't we just want g_assert (signatures != NULL);?

I tried this, but got an test failure:

ERROR: tests/test-signed-pull.sh - Bail out! OSTreeSign:ERROR:src/libostree/ostree-sign-x509.c:169:ostree_sign_x509_data_verify: assertion failed: (signatures != NULL)

Looks like ostree_sign_commit_verify may call ostree_sign_data_verify with NULL signatures. Is there any case where this is supposed to succeed?

Probably not but yeah let's continue to throw an error

src/libostree/ostree-sign-x509.h

cgwalters · 2024-08-16T22:39:21Z

My main motivation here is to prepare for the upcoming PQC transition. While encryption (key encapsulation) is the current priority as it affects forward secrecy, signatures will come next, and it would be nice if ostree could support PQC signatures transparently.

But can you elaborate just a bit more - I'm just guessing here: did you do an audit of code shipped e.g. in RHEL linking to openssl? Do you happen to know about ostree specifically beforehand?

Signed-off-by: Daiki Ueno <[email protected]>

Suggested in: ostreedev#3278 (comment) Signed-off-by: Daiki Ueno <[email protected]>

This defines a new interface OstreeBlobReader, which encapsulates the key file parsing logic. This would make it easy to support custom file formats such as PEM. Signed-off-by: Daiki Ueno <[email protected]>

This adds a new class OstreePemReader, which reads PEM blocks from an input stream. This would be useful for the "x509" signing backend, as the keys are typically stored in the PEM format. Signed-off-by: Daiki Ueno <[email protected]>

Signed-off-by: Daiki Ueno <[email protected]>

dbnicholson · 2024-09-12T14:22:33Z

Sorry, I keep meaning to give some time to reviewing this. The one thing I'll say now is that I don't think x509 is an appropriate name for the backend. That implies X.509 certificates, which would be interesting, but it's not what's happening here. The only connection to X.509 is using the SubjectPublicKeyInfo representation of public keys. This is a straightforward asymmetric digital signature scheme without certificates.

My suggested names would be:

pkey - Borrow openssl's naming for public/private key handling.
pkcs8 - A misnomer since PKCS#8 representation only applies to the private key.
pem - Still a little inaccurate since we're really talking about RFC 7468 textual encodings rather than strictly PEM. This also implies arbitrary types of PEM encodings are used, but we're only talking about private and public keys.
key, pubkey or keypair - Straightforward names representing a signature using only public key cryptography rather than certificates or something else. It would just be documented that the "PEM" encodings from RFC 7468 are required.

cgwalters · 2024-09-12T18:54:55Z

Mild preference for pkey given that list

ueno · 2024-09-13T06:21:08Z

I don't have strong opinion about the naming, but colleagues suggested spki or pkcs as well.

On a slightly different note, while this PR only supports SubjectPublicKeyInfo (SPKI) based raw public keys (as in RFC 7250), I was wondering if it could eventually support X.509 certificates as well to take advantage of system trust store (either backed by p11-kit or file/directory based storage on /etc/pki/), instead of the custom directories, i.e., /etc/ostree/{trusted,revoked}.*.

ueno · 2024-09-13T08:51:15Z

After a second thought, I'm leaning to spki, as we are more concerned about public keys here and private keys could be in any form, e.g., PKCS#8, something opaque on a hardware token, etc., with future extensions.

The current "ed25519" signing type assumes raw Ed25519 key format for both public and private keys. That requires custom processing of keys after generated with openssl tools, and also lacks cryptographic agility[1]; when Ed25519 becomes vulnerable, it would not be straightforward to migrate to other algorithms, such as post-quantum signature algorithms. This patch adds a new signature type "spki" which uses the X.509 SubjectPublicKeyInfo format for public keys. Keys in this format can easily be created with openssl tools and provide crypto agility as the format embeds algorithm identifier. Currently, the corresponding private keys shall be in the PKCS#8 format, while future extensions may support other format such as opaque key handles on a hardware token. The "spki" signature type prefers keys to be encoded in the PEM format on disk, while it still accepts base64 encoded keys when given through the command-line. 1. https://en.wikipedia.org/wiki/Cryptographic_agility Signed-off-by: Daiki Ueno <[email protected]>

openshift-ci bot added do-not-merge/work-in-progress needs-ok-to-test labels Jul 12, 2024

ueno force-pushed the wip/dueno/pkcs8 branch 3 times, most recently from 554697d to 70035e3 Compare July 12, 2024 07:00

cgwalters requested changes Jul 12, 2024

View reviewed changes

src/libostree/ostree-sign-ed25519.c Outdated Show resolved Hide resolved

src/libostree/ostree-sign-ed25519.c Outdated Show resolved Hide resolved

src/libostree/ostree-sign-ed25519.c Outdated Show resolved Hide resolved

src/libotcore/otcore-ed25519-verify.c Outdated Show resolved Hide resolved

cgwalters reviewed Jul 12, 2024

View reviewed changes

ueno added a commit to ueno/ostree that referenced this pull request Jul 13, 2024

sign: Use explicit_bzero to clear secret key material

97fdd9c

Suggested in: ostreedev#3278 (comment) Signed-off-by: Daiki Ueno <[email protected]>

ueno force-pushed the wip/dueno/pkcs8 branch from 70035e3 to a98850f Compare July 13, 2024 01:38

ueno force-pushed the wip/dueno/pkcs8 branch from a98850f to 5a0ac2f Compare July 19, 2024 00:20

github-actions bot added the area/rust-bindings Relates to the Rust bindings for the C library label Jul 19, 2024

ueno force-pushed the wip/dueno/pkcs8 branch from 5a0ac2f to d8a7c74 Compare August 9, 2024 07:09

ueno added a commit to ueno/ostree that referenced this pull request Aug 13, 2024

sign: Use explicit_bzero to clear secret key material

4bc9f37

Suggested in: ostreedev#3278 (comment) Signed-off-by: Daiki Ueno <[email protected]>

ueno force-pushed the wip/dueno/pkcs8 branch from d8a7c74 to 9ec192a Compare August 13, 2024 01:44

ueno changed the title ~~RFC: sign: Support keys embedded with public key algorithm~~ sign: Support x509 signature type Aug 13, 2024

ueno requested a review from cgwalters August 13, 2024 05:16

ueno marked this pull request as ready for review August 13, 2024 06:10

openshift-ci bot removed the do-not-merge/work-in-progress label Aug 13, 2024

ueno force-pushed the wip/dueno/pkcs8 branch 2 times, most recently from dc7f0e8 to 9ec192a Compare August 15, 2024 00:17

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Aug 16, 2024

cgwalters reviewed Aug 16, 2024

View reviewed changes

src/libostree/ostree-sign-x509.c Outdated Show resolved Hide resolved

cgwalters reviewed Aug 16, 2024

View reviewed changes

ueno added 5 commits September 11, 2024 16:12

sign: Fix typo in error messages and comments

334e0b0

Signed-off-by: Daiki Ueno <[email protected]>

sign: Use explicit_bzero to clear secret key material

62e5452

Suggested in: ostreedev#3278 (comment) Signed-off-by: Daiki Ueno <[email protected]>

sign: Factor out logic to read key blobs

ef44e33

This defines a new interface OstreeBlobReader, which encapsulates the key file parsing logic. This would make it easy to support custom file formats such as PEM. Signed-off-by: Daiki Ueno <[email protected]>

sign: Add PEM reading facility

0fa1061

This adds a new class OstreePemReader, which reads PEM blocks from an input stream. This would be useful for the "x509" signing backend, as the keys are typically stored in the PEM format. Signed-off-by: Daiki Ueno <[email protected]>

tests: Use tap_ok/tap_end in test-signed-commit.sh

5583563

Signed-off-by: Daiki Ueno <[email protected]>

ueno force-pushed the wip/dueno/pkcs8 branch 2 times, most recently from f070034 to af338b8 Compare September 12, 2024 12:49

ueno force-pushed the wip/dueno/pkcs8 branch from af338b8 to e83eda0 Compare September 17, 2024 07:26

cgwalters mentioned this pull request Sep 17, 2024

signature: add OpenPGP signing mechanism based on Sequoia containers/image#2569

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sign: Support x509 signature type #3278

sign: Support x509 signature type #3278

ueno commented Jul 12, 2024 •

edited

Loading

openshift-ci bot commented Jul 12, 2024

cgwalters left a comment

cgwalters Jul 12, 2024

dbnicholson Jul 12, 2024

cgwalters Jul 13, 2024

ueno Jul 14, 2024

ueno Jul 19, 2024 •

edited

Loading

dbnicholson commented Jul 12, 2024

dbnicholson commented Jul 12, 2024

ueno commented Jul 12, 2024

cgwalters commented Jul 13, 2024

ueno commented Aug 13, 2024

cgwalters commented Aug 16, 2024

cgwalters left a comment

cgwalters Aug 16, 2024

ueno Sep 12, 2024

cgwalters Aug 16, 2024

ueno Sep 12, 2024

cgwalters Sep 12, 2024

cgwalters Aug 16, 2024

cgwalters Aug 16, 2024

cgwalters Aug 16, 2024

ueno Sep 12, 2024

cgwalters Sep 12, 2024

cgwalters commented Aug 16, 2024

dbnicholson commented Sep 12, 2024

cgwalters commented Sep 12, 2024

ueno commented Sep 13, 2024

ueno commented Sep 13, 2024

		if (pkey == NULL && public_key_size == OSTREE_SIGN_ED25519_PUBKEY_SIZE)
		pkey = EVP_PKEY_new_raw_public_key (EVP_PKEY_ED25519, NULL, public_key_buf, public_key_size);

sign: Support x509 signature type #3278

Are you sure you want to change the base?

sign: Support x509 signature type #3278

Conversation

ueno commented Jul 12, 2024 • edited Loading

openshift-ci bot commented Jul 12, 2024

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueno Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

dbnicholson commented Jul 12, 2024

dbnicholson commented Jul 12, 2024

ueno commented Jul 12, 2024

cgwalters commented Jul 13, 2024

ueno commented Aug 13, 2024

cgwalters commented Aug 16, 2024

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Aug 16, 2024

dbnicholson commented Sep 12, 2024

cgwalters commented Sep 12, 2024

ueno commented Sep 13, 2024

ueno commented Sep 13, 2024

ueno commented Jul 12, 2024 •

edited

Loading

ueno Jul 19, 2024 •

edited

Loading