Modify SSLContext#add_certificate_chain_file
#309
Conversation
ext/openssl/ossl_ssl.c
Outdated
| if (!pub_pkey) | ||
| rb_raise(rb_eArgError, "certificate does not contain public key"); | ||
|
|
||
| if (EVP_PKEY_cmp(pub_pkey, pkey) != 1) |
There was a problem hiding this comment.
I need a review.
To check arguments in advance, I chose not to use SSL_CTX_check_private_key.
If SSL_CTX_check_private_key is called and returned the value is not 1, the set private key should be cleared.
But, the API to clear the private key is not supported, as far as I can see.
It checks arguments first so that it is not set the certificate chain and the private key to the ctx.
| @@ -186,8 +186,107 @@ def test_add_certificate_multiple_certs | |||
| end | |||
|
|
|||
| def test_add_certificate_chain_file | |||
| ctx = OpenSSL::SSL::SSLContext.new | |||
| assert ctx.add_certificate_chain_file(Fixtures.file_path("chain", "server.crt")) | |||
There was a problem hiding this comment.
Maybe create a separate test, since this is still a useful test, no?
There was a problem hiding this comment.
Thanks!! You commented about assert ctx.add_certificate_chain_file?
Now, SSLContext#add_certificate_chain_file returns self, not boolean.
I have modified tests to call assert_nothing_raised.
There was a problem hiding this comment.
I have modified add_certificate_chain_file to return self. It is the reason the following as:
- It is exceptional situations to fail to add the certificate or the private key. So, when failed to add,
add_certificate_chain_filebe better to raise an error instead of returning false, I think. add_certificate(method for a similar purpose) raises an error if failed to add, too.
… return self add test_add_certificate_chain_file_multiple_certs
thekuwayama
force-pushed
the
modify__add_certificate_chain_file
branch
from
b73ba43
to
417e8cf
Compare
|
I have done the rebase and push. |
Let's revert the changes for now, as it cannot be included in the 2.2.0 release. My comment on ruby#257: > A blocker is OpenSSL::SSL::SSLContext#add_certificate_chain_file. It > has a pending change and I don't want to include it in an incomplete > state. > > The initial implementation in commit 46e4bdb was not really > useful. The issue is described in ruby#305. ruby#309 extended it > to take the corresponding private key together. However, the new > implementation was incompatible on Windows and was reverted by ruby#320 to > the initial one. > > (The prerequisite to implement it in) an alternative way is ruby#288, and > it's still cooking. This effectively reverts the following commits: - dacd089 ("ssl: suppress test failure with SSLContext#add_certificate_chain_file", 2020-03-09) - 46e4bdb ("Add support for SSL_CTX_use_certificate_chain_file. Fixes ruby#254.", 2019-06-13)
Hi!
This PR is related to #305.
This modifies
OpenSSL::SSL::SSLContext#add_certificate_chain_fileto accept the path to the private key.This PR
add_certificate_chain_fileadd_certificate_chain_fileadd_certificate_chain_fileThis PR referenced
SSLContext#add_certificate.