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

Setting Certificate/Private Key Improvements #15

Closed
wants to merge 1 commit into from
Closed

Setting Certificate/Private Key Improvements #15

wants to merge 1 commit into from

Conversation

lgrahl
Copy link
Contributor

@lgrahl lgrahl commented Oct 20, 2016

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_pem
  • tls_set_certificate_der
  • for compatibility, tls_set_certificate has been updated and uses tls_set_certificate_pem underneath

No changes to existing function signatures had to be made. :)

(Let me know if I should rebase the changes and make a PR to merge into the openssl-1.1.0 branch.)

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`
@lgrahl lgrahl mentioned this pull request Oct 21, 2016
@lgrahl
Copy link
Contributor Author

lgrahl commented Oct 21, 2016

Note: I've double-checked that the newly introduced OpenSSL functions are available in 0.9.8 to 1.1.0 and libressl. I've compiled and tested it on OpenSSL 1.0.1f and OpenSSL 1.0.2h.

@alfredh
Copy link
Contributor

alfredh commented Oct 24, 2016

thanks for this nice patch. in general all the code looks of superb quality and
should be good to merge to master.

in general all patches/PRs should be based on master. the order in which we merge
patches/PRs to master can sometimes be random.

it was particularly good that you added this type:

enum tls_key_type {
       TLS_KEY_TYPE_RSA,
       TLS_KEY_TYPE_EC,
};

later we should extend the selfsign function, to include keysize and also
add support for Elliptic Curve (EC).

@lgrahl
Copy link
Contributor Author

lgrahl commented Oct 24, 2016

Thanks for the feedback. Much appreciated. :)

later we should extend the selfsign function, to include keysize and also
add support for Elliptic Curve (EC).

Already wrote some code for that in my own library. But if you're interested in it, I can easily port it and open a PR.

@richaas
Copy link
Contributor

richaas commented Nov 1, 2016

Hi @lgrahl, would it be possible to have a new patch here, were you keep the original variable names from tls_set_certificate (except for the rsa/pkey variable). The diff would be much cleaner/easier to read then.

Also, would it be possible to move the DER related changes to another PR? BTW, in your DER code it looks like you reference updated buf_cert value from d2i_X509(), before checking d2i_X509() return value.

@lgrahl
Copy link
Contributor Author

lgrahl commented Nov 1, 2016

Hi @lgrahl, would it be possible to have a new patch here, were you keep the original variable names from tls_set_certificate (except for the rsa/pkey variable). The diff would be much cleaner/easier to read then.

Don't get me wrong, but I don't think changing the variables names back to make the diff nicer to read but overall have confusing variable names as a result is a good idea.

Edit: I thought you meant the variable names in the signature. Regarding bio_cert, etc: I will switch them back to the original names in the next commit. However, I cannot change x509 back to cert for obvious reasons (see the signature).

Also, would it be possible to move the DER related changes to another PR?

I could do that but I would prefer having it this way unless there is a reason why I should split them up. Also, the function name tls_set_certificate_pem doesn't make much sense without the der counterpart.

BTW, in your DER code it looks like you reference updated buf_cert value from d2i_X509(), before checking d2i_X509() return value.

Nice find, will fix that.

@lgrahl
Copy link
Contributor Author

lgrahl commented Nov 1, 2016

Closing in favour of #25, @richaas #25 also fixes the DER function and addresses your request regarding variable names.

@lgrahl lgrahl closed this Nov 1, 2016
@lgrahl lgrahl deleted the tls-set-certificate-improvements branch November 1, 2016 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants