Setting Certificate/Private Key Improvements #25
Conversation
Add possibility to use DER-encoding when setting a certificate Add `tls_set_certificate_pem` and `tls_set_certificate_der` function Add `enum tls_key_type` Deprecate `tls_set_certificate`
| * Set the certificate and private key on a TLS context | ||
| * | ||
| * @param tls TLS Context | ||
| * @param cert Certificate in DER format |
There was a problem hiding this comment.
Use same order of parameter as signature
|
|
||
| /** | ||
| * Set the certificate and private key on a TLS context | ||
| * @deprecated Use tls_set_certificate_pem or tls_set_certificate_der instead |
There was a problem hiding this comment.
No need to mark this as deprecated
| X509_free(cert); | ||
| if (rsa) | ||
| RSA_free(rsa); | ||
| out: |
There was a problem hiding this comment.
Oh... I didn't even notice that. Out of curiosity: Any particular reason why you're adding a space before a label?
| len_key = len_cert - (buf_cert - cert); | ||
| } | ||
| else { | ||
| buf_key = key; |
There was a problem hiding this comment.
variable buf_key is not needed, use key directly
There was a problem hiding this comment.
Using key directly would advance key as described here:
Using a temporary variable is mandatory. A common mistake is to attempt to use a buffer directly as follows:
int len; unsigned char *buf; len = i2d_X509(x, NULL); buf = OPENSSL_malloc(len); ... i2d_X509(x, &buf); ... OPENSSL_free(buf);This code will result in buf apparently containing garbage because it was incremented after the call to point after the data just written. Also buf will no longer contain the pointer allocated by OPENSSL_malloc() and the subsequent call to OPENSSL_free() is likely to crash.
There was a problem hiding this comment.
I'm aware that the pointer is updated, but this is not an issue in our use case.
There was a problem hiding this comment.
Because? The caller doesn't expect key to be advanced.
There was a problem hiding this comment.
caller will not be affected; caller pass *, not **.
| if (!tls || !cert || !len_cert || (key && !len_key)) | ||
| return EINVAL; | ||
|
|
||
| switch (key_type) { |
There was a problem hiding this comment.
move this into a separate mapping function, as this can be useful for several functions in the future:
static int keytype(enum tls_keytype type)
|
|
||
| err = 0; | ||
|
|
||
| out: |
| len_key = len_cert; | ||
| } | ||
|
|
||
| bio = BIO_new_mem_buf((char *)cert, (int)len_cert); |
There was a problem hiding this comment.
keep double space after bio, to simplify diff
Undo deprecation of `tls_set_certificate` Fix order of parameters in `tls_set_certificate_der`'s docstring Address various stylistic issues
| @@ -24,11 +24,21 @@ enum tls_fingerprint { | |||
| TLS_FINGERPRINT_SHA256, | |||
| }; | |||
|
|
|||
| enum tls_key_type { | |||
There was a problem hiding this comment.
Please use naming enum tls_keytype (TLS_KEYTYPE_RSA, TLS_KEYTYPE_EC)
| static int key_type2int(enum tls_key_type type) | ||
| { | ||
| switch (type) { | ||
| case TLS_KEY_TYPE_EC: |
There was a problem hiding this comment.
fix indentation inside switch
Rename `key_type2int` to `keytype2int` Fix indentation in `keytype2int`
|
all these changes looks good. I have tested that it works with perhaps we should update the TLS test to use a DER-encoded |
|
extra testing after merge (718583f)
|
These changes give the user a little bit more fine granularity when setting a certificate (and the private key).
Other private key types than RSA are now supported.
PEM certificate and private key can now be contained in separate strings.
Moreover, the user is able to use the DER format for certificates and private keys.
To make this possible, the following functions have been added:
tls_set_certificate_pemtls_set_certificate_dertls_set_certificatehas been updated and usestls_set_certificate_pemunderneathNo changes to existing function signatures had to be made. :)
(Rebased and fixed version as a replacement for #15)