Add PKCS#8 writing support

[andresag01]

Description

This PR is based on @mvgalen work at #1759. It as support for writing keys in PKCS#8 syntax in either DER or PEM format.

The main implementation was taken from #1759, but this PR also adds some tests and the DER part. In addition, this PR improves the gen_key app so that it is able to write keys in PEM or DER format with PKCS#1 or PKCS#8 syntax.

Status

READY

Requires Backporting

No, this is a new feature

Migrations

NO

Todos

• Tests
• Documentation
• Changelog updated
• Backported

[RonEld]

I left a few comments regarding stack overflow on target, possibility to have these tests run on target without FS. i also addressed
In general, I think there are tests missing. The only test I could find is having an existing pkcs8 file, parse it, write it to pkcs8, and then compare the written data with the original.
I would add another test:

• Have a pkcs 1 file, parse it, write it to pkcs8, and then compare it with a pkcs8 file, which is the pkcs8 format of the pkcs 1 original file

Perhaps some more tests are missing.

In addition, since we are adding API, I would change it to support encrypted private key as well.

As for sample application, you have modified gen_key, but key_app_writer should also be modified.

[RonEld]

This will cause stack overflow on targets with 4 KB stack. Although MBEDTLS_PEM_WRITE_C will probably not be defined on such platforms, I would change the buffer size, if possible

[andresag01]

@RonEld: As you said, this functionality is something that is very unlikely to be used in a small target. I will try to reduce the size or to allocate the buffer dynamically, but here we are limited by the key sizes

[RonEld]

Do you know what's the key size limit? Is 5000 bytes ( times 2...) realy the minimal size?

[RonEld]

I would try adding tests without using file system.

[andresag01]

Well, I effectively used the existing tests as a template for the new one, so we are using this practice even without this PR. I suppose what I could do is hard code the PEM key into the .data file. Is that better?

[RonEld]

I wouldn't say this test is not needed, I would say we are missing tests though

[RonEld]

preexisitng: I think it would be a good idea to change 16000 to a precompilation MACRO definiiton

[andresag01]

Ok

[RonEld]

I would enhance to support encrypted private key.

[k-stachowiak]

Unfortunately there is a minor error in the code, which must be fixed. I also have one remark regarding superfluous code. Otherwise the PR is good.

[k-stachowiak]

This should test q rather than p. Maybe the tests could be extended to also use this option? Currently, they only use pkcs8 format, which is why this was not caught.

[k-stachowiak]

The algorithm from here until the end of the function can be reduced to return( ret )

[andresag01]

@RonEld: I have reworked the PR according to your comments. A few notes:

• I reduced the memory usage a little so that the test data fits but is not too tight. I also moved these buffers to the heap to avoid stack overflow, but is still a lot for an MCU.
• What other tests we could add? I tried thinking about this for a bit, but I am not sure what tests are worth adding
• I think the test you suggested (Have a pkcs 1 file, parse it, write it to pkcs8, and then compare it with a pkcs8 file, which is the pkcs8 format of the pkcs 1 original file) is a bit redundant because this is testing the PKCS#1 parsing more than anything else as the writing PKCS#8 routines tested would be the same as with the current tests because the writing is done based on the pkey struct...
• I am a bit reluctant to add the PKCS#8 writing/reading to key_app_writer because that app does both private and public keys but as far as I am aware of PKCS#8 is only for private keys. So adding PKCS#8 to the app would make it asymmetric and a bit awkward to use.
• I will look into adding PKCS#8 encryption support, but this was not part of the contributor's PR.

[RonEld]

@andresag01 Thank you for the rework

I reduced the memory usage a little so that the test data fits but is not too tight. I also moved these buffers to the heap to avoid stack overflow, but is still a lot for an MCU.

Correct, thanks. That's enough for now, I guess, but I still think we should have a way of testing on target as well.

• I think the test you suggested (Have a pkcs 1 file, parse it, write it to pkcs8, and then compare it with a pkcs8 file, which is the pkcs8 format of the pkcs 1 original file) is a bit redundant because this is testing the PKCS#1 parsing more than anything else as the writing PKCS#8 routines tested would be the same as with the current tests because the writing is done based on the pkey struct...

The intention was to verify the PKCS#8 key we are writing, is as expected. Perhaps: have hard coded input for RSA and EC keys, write them to PKCS#8 key, and compare with an existing key with same values which was written by third party, such as openssl ?

• am a bit reluctant to add the PKCS#8 writing/reading to key_app_writer because that app does both private and public keys but as far as I am aware of PKCS#8 is only for private keys. So adding PKCS#8 to the app would make it asymmetric and a bit awkward to use.

AFAIK, PKCS#8 is also for public keys. They start with -----BEGIN PUBLIC KEY----- , and I believe we already support writing them.

• I will look into adding PKCS#8 encryption support, but this was not part of the contributor's PR.

You are correct that it's not part of the original PR, but this would have been a comment I would add to the original PR as well. If we are adding a function, I would prefer having it to cover all cases. At least the function signature, return "not supported" for password protected at the time being, and open a ticket for tracking this. Obviously, I prefer it to be supported in this PR, but at least the function signature should have it supported.

[RonEld]

Actually, I believe our mbedtls_pk_write_pubkey_pem() API writes public keys in PKCS#8, and we are lacking the functionality to write public keys in PKCS#1 syntax, which is unfortunate, I believe.

[RonEld]

I have just looked again at the RFC, and PKCS#8 is for private key alone, to my understanding.
I guess our documentation in https://tls.mbed.org/kb/cryptography/asn1-key-structures-in-der-and-pem should be updated...

Nonetheless, our private \ public key writing functionality is not symmetric, and we should have support for writing the keys we are able to parse,

[andresag01]

@RonEld: From your comments, I guess we agree that PKCS#8 is only for private keys. I will look into the encryption bit. With regards to the other points:

• Do you still want to modify key_app_writer? I would prefer not to because PKCS#8 does not match very well with the PKCS#1 functionality key_app_writer already offers.
• For the tests you said: "The intention was to verify the PKCS#8 key we are writing, is as expected. Perhaps: have hard coded input for RSA and EC keys, write them to PKCS#8 key, and compare with an existing key with same values which was written by third party, such as openssl ?". This what I first tried to do, but the problem is that privateKey and privateKeyAlgorithmIdentifier fields in the PKCS#8 struct have an optional attributes parameter for EC keys. Unfortunately,OpenSSL and MbedTLS generate these optional fields differently and I could not get them to have both the same behaviour without modifying other parts of Mbed TLS. So instead what I did was manually check everything is OK and generate the keys using Mbed TLS itself.

[k-stachowiak]

@andresag01 Thanks for addressing my comments! I have no further objections.

[RonEld]

@andresag01

Do you still want to modify key_app_writer?

Since these are only example applications, I won't put my foot down. However, the current implementation of key_app_writer is not symmetrical, and doesn't offer PKCS#1 functionality.
For private keys:
we write keys in PKCS#1 syntax, with -----BEGIN RSA PRIVATE KEY----- and -----BEGIN EC PRIVATE KEY-----, however for public keys, we write in X.509 SubjectPublicKeyInfo, with -----BEGIN PUBLIC KEY-----. So, I don't agree with the claim that "* PKCS#8 does not match very well with the PKCS#1 functionality key_app_writer already offers."

Thank for your explanation on the tests. I still think we are lacking some tests, but I understand the limitations.

As for encryption part, I strongly think we should have in the API a parameter for password. For the time being, we can add the parameter, return a not supported error(with documentation), and track it in a seperate issue. I don't like that if we don't add the password parameter, we will need to deprecate this function once we decide to add encryption support., if we can avoid deprecation.

[andresag01]

@RonEld: I had a closer look at your comments:

1. You wrote:

the current implementation of key_app_writer is not symmetrical, and doesn't offer PKCS#1 functionality.
For private keys:
we write keys in PKCS#1 syntax, with -----BEGIN RSA PRIVATE KEY----- and -----BEGIN EC PRIVATE KEY-----

When I meant by asymmetric is that I do not think that one would normally use PKCS#8 to encode public keys, only private ones. I suppose that you could encode a public key with PKCS#8 or something like this and then change the PEM header and footer to say PUBLIC instead of PRIVATE. But I am not sure whats the point of this. Also, I think public keys are generally encoded using the X.509 SubjectPublicKeyInfo, so thats expected from key_app_writer. Anyways, I have tried to fit the PKCS#8 into the key_app_writer, so what I ended up doing was adding a new command line option to say whether you want to encode the output private key as pkcs1 or pkcs8. This parameter has no effect when the output requested is a public key.

2. You mentioned:

As for encryption part, I strongly think we should have in the API a parameter for password.

I think that it would be nice to be able to encrypt PKCS#8 keys. However, I think that this requires more than simply adding a password input parameter for the key file writer function. For example, how would you pick the encryption algorithm? I personally think that this is a further feature altogether and would prefer to leave it for a future PR.

I do not see the need to have to deprecate the current PKCS#8 writer if we were to add an encryption option. If I needed to add that feature, I would just create a wrapper around the unencrypted PKCS#8 writer function. The wrapper would just use the old functionality to write the unencrypted key, then it would encrypt the output and finally it would add the extra ASN.1 structs around it.

[RonEld]

@andresag01 THank you for addressing my comments

When I meant by asymmetric is that I do not think that one would normally use PKCS#8 to encode public keys, only private ones.

Yes, PKCS#8 is for private key only. (starts with -----BEGIN PRIVATE KEY-----). The corresponding public key which starts with -----BEGIN PUBLIC KEY----- is the X.509 SubjectPublicKeyInfo, which we support. The problem, in key_app_writer, is that for private keys we wrote in PKCS#1, ( with your change, now also PKCS#8) and for public keys, X.509 SubjectPublicKeyInfo alone (PKCS#1 is not supported there). But this is a different topic.

I do not see the need to have to deprecate the current PKCS#8 writer if we were to add an encryption option. If I needed to add that feature, I would just create a wrapper around the unencrypted PKCS#8 writer function.

Correct, That's another solution. iowever, I think that the more APIs we have, the more it's confusing. If we can save the additional API in advance, why not do it? I see your point on algorithm parameter as well, and not only password. So perhaps we should open this for discussion, on what sould be the proper API?

[andresag01]

@RonEld: Thanks for reviewing. In summary, I think that the review comments were address except the one related to writing PKCS#8 encrypted keys. As I stated before, I personally think that it would be ideal to keep this PR relatively small and self contained and leave the encryption for a later stage. In any case, I think Mbed TLS does not currently support writing encrypted keys.

I also understand Ron's point of view that he ideally wants to make sure the API for encrypted and unencrypted keys is consistent from now such that we do not regret choices that we might need to deprecate and leave with for a very long time. However, I think that having a function for encrypted and another for unencrypted keys in the future is also acceptable (we can change the naming of the one being introduced here if it helps).

At this stage, I think that it would be ideal if we ask a gatekeeper. @mpg could you take a look? Also, this might be of interest to @gilles-peskine-arm or @Patater because i think this is part of the crypto component.

[gilles-peskine-arm]

On whether to support writing encrypted keys in this PR, I think we shouldn't. This PR adds a meaningful feature and we shouldn't hold it until it adds another feature. So to me, the question is, should the new functions mbedtls_pkcs8_write_key_der and mbedtls_pkcs8_write_key_pem take extra arguments to support encryption, which would currently not be implemented?

On whether to support writing encrypted keys in the new functions, I see good arguments both ways.
On the pro side, this is a useful feature, and it would make the functions more symmetric with mbedtls_pk_parse_key.
On the con side, it would make the API more complicated for users who don't want to encrypt the output.

What exactly would the API look like? How does one specify which encryption algorithm to use, and what parameters does it take (password, salt length, iteration count…)? What successors of RFC 5208 should the API be able to support?

If we don't have a consensus on how the API would look like then we should move on with the current API proposal and do encryption with new functions later.

[RonEld]

@andresag01
If the consensus to write encrypted keys is to do it in a different function, then I withdraw my objection.
Please rename the functions to:
mbedtls_pkcs8_write_clear_key_der and mbedtls_pkcs8_write_clear_key_pem to make it clearer:)

[andresag01]

@RonEld: I have rebased to fix the merge conflict.

[RonEld]

@andresag01 Thanks for resolving the conflicts!

Changes look good, however now CI fails, when MBEDTLS_SSL_KEEP_PEER_CERTIFICATE is undefined

[andresag01]

@RonEld: Thanks for pointing out the CI failure. There was a memory leaky test, but that should be fixed now. Waiting for CI results.

[RonEld]

@andresag01 CI still fails, now when MBEDTLS_RSA_C is not defined.
I suggest you run all the depends-* and similar scripts to avoid some issues

[andresag01]

@RonEld: Thanks for pointing out the CI failure. There were two problems that only showed up when running the EC tests only.

1. The macros to define the internal buffers sizes did not have parenthesis and size was not being set to the correct size.
2. The internal buffer size to hold the PKCS8 key did not account for the EC params.

I added fixes for these two issues in the last commit. Waiting for the CI results...

[RonEld]

@andresag01 Thanks for the changes!

The changes look good, however please explain why the EC Params add an additional 9 bytes.
According to RFC5480:

ECParameters ::= CHOICE {
       namedCurve         OBJECT IDENTIFIER
       -- implicitCurve   NULL
       -- specifiedCurve  SpecifiedECDomain
     }

Couldn't the OID be larger than 9 bytes?

In addition, the CI now fails in the tests for crypto submodule, as new API in libmbedcrypto does not exist in the crypto submodule. CC @Patater

[andresag01]

@RonEld:

The changes look good, however please explain why the EC Params add an additional 9 bytes.

The code in this PR is based on a community contribution. The original code did not add thee EC parameters when the key to write in PKCS#8 is of that kind. So I added it but forgot to take into account the size of this in the internal buffer.

I do not know what is the maximum size of the OID for the EC params. My approach was simply to work out the maximum length of that data from the possible values in Mbed TLS listed here. I think that should be sufficient for our needs and anyways this is safe because (as the test pointed out) there will be a clear failure.

In addition, the CI now fails in the tests for crypto submodule, as new API in libmbedcrypto does not exist in the crypto submodule.

I will not take action regarding this for the moment. But please let me know if anything needs fixing from my end.

[TrinityTonic]

PING.
In my opinion the functionality of writing PKCS#8 keys should be part of a TLS library. However in addition to the feature requests discussed in this PR I would definitely add support the encryption of keys when writing then (minimum according to PKCS5, PKCS12 maybe optional for the first version).

Regards
TrinityTonic

[fractalclone]

Hi, when can we expect this PR to be merged?

[gilles-peskine-arm]

@fractalclone Nobody is actively working on this. Given its age, I expect it needs some rework, probably starting with a rebase, then getting it to pass CI, and address all the review comments.