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.

Sign up for GitHub

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

Jump to bottom

re-examination about SSLContext#add_certificate_chain_file #305

Closed
thekuwayama opened this issue Dec 13, 2019 · 6 comments
Closed

re-examination about SSLContext#add_certificate_chain_file #305

thekuwayama opened this issue Dec 13, 2019 · 6 comments
Milestone
v2.2.0

Comments

@thekuwayama
Copy link
Contributor

SSLContext#add_certificate_chain_file() is able to load a chain certificates file. I want to re-examine this API.

As commented, the old setters(SSLContext#cert=, key=, extra_chain_cert=) are deprecated.
If add_certificate_chain_file is used, it is needed to call key= to load the private key associated with certificates.
That is, as provided add_certificate_chain_file and then it would be required key=.

The API that parses a chain certificates file is so nice and I think that is needed. So, I suggest 2 plans.

  1. modify add_certificate_chain_file to accept the path to the private key. Also, it is needed to rename too.
  2. provide Certificate.load_file and deprecate add_certificate_chain_file.
@thekuwayama thekuwayama mentioned this issue Dec 13, 2019
Closed
@ioquatix
Copy link
Member

I agree.

Was add_certificate_chain_file actually released in any version?

@thekuwayama
Copy link
Contributor Author

add_certificate_chain_file has not released yet. It is scheduled that is released in 2.2.0 version.

openssl/History.md

Line 33 in 62287be

* Add `OpenSSL::SSL::SSLContext#add_certificate_chain_file` for

@ioquatix
Copy link
Member

So we could consider remove it before it’s released if we can implement the alternatives in time... do you have time to make PR?

@thekuwayama
Copy link
Contributor Author

So sorry... I could not make the PR by the next release.

@ioquatix
Copy link
Member

Don't be sorry, I'll see if I can take a stab. @hsbt if we decide to change this, what is the latest time we can merge this into Ruby 2.7?

@ioquatix ioquatix added this to the v2.2.0 milestone Dec 28, 2019
@ioquatix ioquatix mentioned this issue Dec 28, 2019
Closed
@thekuwayama thekuwayama mentioned this issue Jan 1, 2020
Merged
@thekuwayama
Copy link
Contributor Author

thekuwayama commented Jan 1, 2020 •
edited
Loading

@ioquatix I sent PR(plan 1).

  1. modify add_certificate_chain_file to accept the path to the private key. Also, it is needed to rename too.

You may review this later, I think. #257 (comment)

@rhenium rhenium mentioned this issue May 10, 2020
Closed
rhenium added a commit to rhenium/ruby-openssl that referenced this issue May 13, 2020
@rhenium
ssl: temporarily remove SSLContext#add_certificate_chain_file
ea92561 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
v2.2.0
Development

No branches or pull requests
2 participants
@ioquatix @thekuwayama