Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sign: Support x509 signature type #3278

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

ueno
Copy link
Contributor

@ueno ueno commented Jul 12, 2024

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.

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

Copy link

openshift-ci bot commented Jul 12, 2024

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.

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

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Comment on lines 109 to 116
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@ueno ueno Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

  • 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
Copy link
Contributor Author

ueno commented Jul 12, 2024

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.

ueno added a commit to ueno/ostree that referenced this pull request Jul 13, 2024
@cgwalters
Copy link
Member

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.

@github-actions github-actions bot added the area/rust-bindings Relates to the Rust bindings for the C library label Jul 19, 2024
ueno added a commit to ueno/ostree that referenced this pull request Aug 13, 2024
@ueno ueno changed the title RFC: sign: Support keys embedded with public key algorithm sign: Support x509 signature type Aug 13, 2024
@ueno ueno requested a review from cgwalters August 13, 2024 05:16
@ueno
Copy link
Contributor Author

ueno commented Aug 13, 2024

@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.

@ueno ueno marked this pull request as ready for review August 13, 2024 06:10
@ueno ueno force-pushed the wip/dueno/pkcs8 branch 2 times, most recently from dc7f0e8 to 9ec192a Compare August 15, 2024 00:17
@cgwalters
Copy link
Member

/ok-to-test

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

#define PEM_SUFFIX "-----"
#define PEM_PREFIX_BEGIN "-----BEGIN "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

tests/test-pem.c Outdated
main (int argc, char **argv)
{
g_test_init (&argc, &argv, NULL);
g_test_add_func ("/ostree_read_pem_block", test_ostree_read_pem_block);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep fuzzing is nonblocking

tests/test-signed-commit-dummy.sh Outdated Show resolved Hide resolved

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
return FALSE;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/libostree/ostree-sign-x509.h Outdated Show resolved Hide resolved
src/libostree/ostree-sign-x509.h Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member

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?

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]>
@ueno ueno force-pushed the wip/dueno/pkcs8 branch 2 times, most recently from f070034 to af338b8 Compare September 12, 2024 12:49
@dbnicholson
Copy link
Member

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
Copy link
Member

Mild preference for pkey given that list

@ueno
Copy link
Contributor Author

ueno commented Sep 13, 2024

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
Copy link
Contributor Author

ueno commented Sep 13, 2024

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rust-bindings Relates to the Rust bindings for the C library ok-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants