Add PKCS8 writing support #1759
Conversation
|
Thanks for the contribution! Unfortunately we have a policy of only accepting contributions with a Contributor’s Licence Agreement (CLA), signed or authorised by yourself or your company if this is a company contribution. The easiest way to do this is if you create an mbed account and accept this click through agreement which is applicable if this is a personal contribution. Alternatively, you can find a slightly different agreement to sign here, which can be signed and returned to us, and is applicable if you don't want to create an mbed account or alternatively if this is a corporate contribution. Thanks for your understanding! |
simonbutcher
added
enhancement
CLA requested
component-crypto
Fix parameter name to correspond with documentation
|
Emailed CLA. |
|
@mvgalen Thank you for accepting the CLA. |
RonEld
added
the
needs-review
There was a problem hiding this comment.
Hi @mvgalen,
Thank you for your contribution! I did an initial review of the changes and I think they look good. I annotated the code where I thought appropriate. In addition, I have a couple more general comments:
- It would be good to add some unit tests for the new API function.
- We need a ChangeLog entry that credits you with the work.
Please let us know if you are happy to rework your PR to address the review comments. If this is not possible, I can rework it.
Many thanks,
Andres AG
| int ret; | ||
| unsigned char output_buf[PRV_PKCS8_DER_MAX_BYTES]; | ||
| const char *begin = PEM_BEGIN_PRIVATE_KEY; | ||
| const char *end = PEM_END_PRIVATE_KEY; |
There was a problem hiding this comment.
I think it would be ideal if we used the header and footer for RSA or EC as in mbedtls_pk_write_key_pem().
| const char *oid; | ||
| size_t oid_len; | ||
|
|
||
| if( ( ret = mbedtls_pk_write_key_der( key, output_buf, sizeof(output_buf) ) ) < 0 ) |
There was a problem hiding this comment.
Minor: Please add spaces before ) and after (.
| { | ||
| } | ||
| else | ||
| #endif |
There was a problem hiding this comment.
Minor: Please add matching comment to the #endif. For example, here it would be:
#endif /* MBEDTLS_ECP_C */
| return( MBEDTLS_ERR_PK_FEATURE_UNAVAILABLE ); | ||
|
|
||
| len = ret; | ||
| c = output_buf + sizeof(output_buf) - len; |
There was a problem hiding this comment.
Minor: Please add spaces before ) and after (.
|
|
||
| /* privateKeyAlgorithm */ | ||
| if( ( ret = mbedtls_oid_get_oid_by_pk_alg( mbedtls_pk_get_type( key ), | ||
| &oid, &oid_len ) ) != 0 ) |
There was a problem hiding this comment.
Please use spaces instead of tabs
| len = ret; | ||
| c = output_buf + sizeof(output_buf) - len; | ||
|
|
||
| /* Wrap PKCS1 key in OCTET STRING */ |
| * } | ||
| */ | ||
|
|
||
| #define PRV_PKCS8_DER_MAX_BYTES 22 + PRV_DER_MAX_BYTES |
There was a problem hiding this comment.
Could you please document where the +22 comes from? Perhaps annotate the PrivateKeyInfo structure similar to https://github.com/ARMmbed/mbedtls/pull/1759/files#diff-788a5101fe2618b1e14eb09a62e1d7ecR436
| const char *end = PEM_END_PRIVATE_KEY; | ||
| size_t olen = 0; | ||
| unsigned char *c; | ||
| int len = 0; |
There was a problem hiding this comment.
Its a length, should it be size_t?
| /* privateKeyAlgorithm */ | ||
| if( ( ret = mbedtls_oid_get_oid_by_pk_alg( mbedtls_pk_get_type( key ), | ||
| &oid, &oid_len ) ) != 0 ) | ||
| return ret; |
There was a problem hiding this comment.
Minor: Please add brackets around the return argument as required by the Mbed TLS coding standards.
| @@ -154,10 +155,18 @@ static int write_private_key( mbedtls_pk_context *key, const char *output_file ) | |||
| size_t len = 0; | |||
|
|
|||
| memset(output_buf, 0, 16000); | |||
| if( opt.format == FORMAT_PEM ) | |||
| if( opt.format == FORMAT_PEM || opt.format == FORMAT_PEM_PKCS8) | |||
There was a problem hiding this comment.
Please add space before the )
andresag01
added
needs-work
and removed
needs-review
|
Hi, when can we expect this PR to be merged? |
|
Closing as the version in #2413 is more up-to-date. |
Description
Currently mbedtls can not write private keys in PKCS8 format, although it is able to parse PKCS8 format.
Since PKCS8 also supports not-yet existing key types since it is extendable it would be a future proof format for key exchange.
Another reason would be that some TLS implementations do not support PKCS1 for private key exchange (like JAVA).
Adding support for PKCS8 PEM/DER writing should be relatively simple since it is only an added envelope.
Status
IN DEVELOPMENT
Requires Backporting
NO
Migrations
If there is any API change, what's the incentive and logic for it.
YES
Add function to write PKCS8 PEM private key files:
int mbedtls_pkcs8_write_key_pem( mbedtls_pk_context *key, unsigned char *buf, size_t size );
Todos