Make RSA-alt keys useable for cert signing #313

Closed

snej wants to merge 1 commit from fix/rsa-alt-keydata into development

snej commented

2019-11-04 17:38:32 -06:00

(Migrated from github.com)

mbedtls_pk_write_pubkey_der fails if called with an RSA-alt key, because (obviously) it doesn't have access to the actual bits of the key. Unfortunately this makes it impossible to generate a certificate or CSR using an external RSA-alt key key, because the operation will fail when it tries to write the public key to the cert/CSR data. (See ARMmbed/mbedtls#2768 for details.)

This commit fixes that by adding mbedtls_pk_rsa_alt_write_pubkey_func, a new callback for RSA-alt keys that writes the RSAPublicKey sequence just like the internal pk_write_rsa_pubkey function. The app developer will have to add that callback, but then cert creation will work. (See my patch to test_suite_pk.function for an example.)

I was unsure what to do with the mbedtls_pk_setup_rsa_alt function — I needed to add a parameter for the new callback, but that would break compatibility. (Too bad C doesn't have default argument values!) So I added a new function mbedtls_pk_setup_rsa_alt2 that adds the extra parameter. If there's a different convention for how to do this kind of API evolution in mbed, I can change it.

`mbedtls_pk_write_pubkey_der` fails if called with an RSA-alt key, because (obviously) it doesn't have access to the actual bits of the key. Unfortunately this makes it impossible to generate a certificate or CSR using an external RSA-alt key key, because the operation will fail when it tries to write the public key to the cert/CSR data. (See ARMmbed/mbedtls#2768 for details.) This commit fixes that by adding `mbedtls_pk_rsa_alt_write_pubkey_func`, a new callback for RSA-alt keys that writes the RSAPublicKey sequence just like the internal `pk_write_rsa_pubkey` function. The app developer will have to add that callback, but then cert creation will work. (See my patch to test_suite_pk.function for an example.) I was unsure what to do with the `mbedtls_pk_setup_rsa_alt` function — I needed to add a parameter for the new callback, but that would break compatibility. (Too bad C doesn't have default argument values!) So I added a new function `mbedtls_pk_setup_rsa_alt2` that adds the extra parameter. If there's a different convention for how to do this kind of API evolution in mbed, I can change it.

gilles-peskine-arm commented

2019-11-12 11:09:25 -06:00

(Migrated from github.com)

Thank you for your contribution.

In the long term, we intend to remove all the ALT interfaces in Mbed Crypto, and switch everything to the psa_xxx interfaces. The inflexibility of the ALT interfaces is one of the reasons why we're going to switch to a completely different interface. Under the hood, psa_xxx functions will call drivers for cryptographic accelerators and external engines. This doesn't work yet: we're still in the design phase for the driver interfaces (accelerators, opaque engines). I expect it to take a couple of years before we declare ALT interfaces obsolete.

In the short term, I think adding a write_pubkey method is fine. But I don't like mbedtls_pk_setup_rsa_alt2, which is as inflexible as the current interface. I'd prefer to add a new function mbedtls_pk_rsa_alt_set_write_pubkey() to be called after mbedtls_pk_setup_rsa_alt (and returning an error if the key type is not RSA_ALT).

Thank you for your contribution. In the long term, we intend to remove all the ALT interfaces in Mbed Crypto, and switch everything to the `psa_xxx` interfaces. The inflexibility of the ALT interfaces is one of the reasons why we're going to switch to a completely different interface. Under the hood, `psa_xxx` functions will call drivers for cryptographic accelerators and external engines. This doesn't work yet: we're still in the design phase for the driver interfaces ([accelerators](https://armmbed.github.io/mbed-crypto/psa/accel/), [opaque engines](https://armmbed.github.io/mbed-crypto/psa/se/)). I expect it to take a couple of years before we declare ALT interfaces obsolete. In the short term, I think adding a `write_pubkey` method is fine. But I don't like `mbedtls_pk_setup_rsa_alt2`, which is as inflexible as the current interface. I'd prefer to add a new function `mbedtls_pk_rsa_alt_set_write_pubkey()` to be called after `mbedtls_pk_setup_rsa_alt` (and returning an error if the key type is not `RSA_ALT`).

👍 1

mpg commented

2020-01-30 05:00:12 -06:00

(Migrated from github.com)

@snej Thanks for your contribution! I've given your PR a high-level review, and I fully agree with Gilles regarding the API question: I think a new function mbedtls_pk_rsa_alt_set_write_pubkey() would be preferable.

Would you be willing to rework your PR accordingly?

Other than that, the PR is looking pretty good (thank you for adding unit tests, which are often overlooked by contributors) so if you can rework it there's a pretty good change we can merge it quickly.

@snej Thanks for your contribution! I've given your PR a high-level review, and I fully agree with Gilles regarding the API question: I think a new function `mbedtls_pk_rsa_alt_set_write_pubkey()` would be preferable. Would you be willing to rework your PR accordingly? Other than that, the PR is looking pretty good (thank you for adding unit tests, which are often overlooked by contributors) so if you can rework it there's a pretty good change we can merge it quickly.

mpg commented

2020-03-26 06:27:09 -05:00

(Migrated from github.com)

Hi @snej! We've recently merged back mbed-crypto to the mbedtls repo, as announced here. As a result, we are no longer accepting PRs to the mbed-crypto repo (except a short grace period for finalizing currently active PR).

Since this PR hasn't been active very recently and isn't ready to be merged as it is, I'm closing it and encouraging you to re-submit your contribution as a new PR on the mbedtls repository: https://github.com/ARMmbed/mbedtls/pulls

If you do so, you might want to take this occasion to review our new contribution licensing rules (no more CLA, but we now require a Signed-of-by tag in each commit) and update your commit messages accordingly.

Thanks again for your contribution, and I hope to see you soon with a new PR in the mbedtls repo!

Hi @snej! We've recently merged back mbed-crypto to the mbedtls repo, as [announced here](https://github.com/ARMmbed/mbedtls/issues/3064). As a result, we are no longer accepting PRs to the mbed-crypto repo (except a short grace period for finalizing currently active PR). Since this PR hasn't been active very recently and isn't ready to be merged as it is, I'm closing it and encouraging you to re-submit your contribution as a new PR on the mbedtls repository: https://github.com/ARMmbed/mbedtls/pulls If you do so, you might want to take this occasion to review our new contribution licensing rules ([no more CLA, but we now require a Signed-of-by tag in each commit](https://github.com/ARMmbed/mbedtls/issues/3054)) and update your commit messages accordingly. Thanks again for your contribution, and I hope to see you soon with a new PR in the mbedtls repo!