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

OpenSSL::SSL::SSLContext#add_certificate, then #cert returns nil #303

Closed
thekuwayama opened this issue Dec 11, 2019 · 4 comments
Closed
Milestone

Comments

@thekuwayama
Copy link
Contributor

thekuwayama commented Dec 11, 2019

This issue is related to behavior of OpenSSL::SSL::SSLContext#add_certificate(_chain_file).

When the SSLContext object is called add_certificate method, then it would load certificates.
In my opinion, it was expected to return a Certificate object (not nil) when being called cert method. But the object does not return a Certificate object and returns nil.
You can check this if you run the following code.

require 'openssl'

ctx = OpenSSL::SSL::SSLContext.new
crt = OpenSSL::X509::Certificate.new(
  File.read('/path/to/ruby-openssl/test/fixtures/chain/server.crt')
)
key = OpenSSL::PKey::RSA.new(
  File.read('/path/to/ruby-openssl/test/fixtures/chain/server.key')
)

ctx.add_certificate(cert, key)
pp ctx.cert

The object, in a handshake, uses the server certificate object and the private key object that were loaded by add_certificate.
It is tested by test_add_certificate.
add_certificate_chain_file method is the same as this.
The SSLContext object does not return these objects.

As commented, the cert, key and extra_chain_cert attributes are deprecated so it is as intended?
If so, how do you think to add the note that cert (etc) returns nil?

@ioquatix
Copy link
Member

ioquatix commented Dec 11, 2019

I think add_certificate_chain_file is a bad implementation.

It should be possible to read the certificates out of a context. It should behave like a value object.

I'm happy to receive your ideas regarding this, but I do think it needs attention.

One thing I considered was to expose something like X509::Certificate.load_file(path) -> [certificate, ...].

@thekuwayama
Copy link
Contributor Author

thekuwayama commented Dec 13, 2019

@ioquatix Thank you for comments. I understood that the setter API is deprecated and getter is not deprecated.

I suggested a related issue.

@ioquatix ioquatix added this to the v2.2.0 milestone Dec 28, 2019
@rhenium
Copy link
Member

rhenium commented Feb 17, 2020

Not being able to get the certificate set by OpenSSL::SSL::SSLContext#add_certificate is intentional.

This is because OpenSSL::SSL::SSLContext#add_certificate is allowed to be called multiple times on the same SSLContext. Here is an expected use case on a server:

rsa_cert = OpenSSL::X509::Certificate.new(...)
rsa_pkey = OpenSSL::PKey.read(...)
rsa_cas = [OpenSSL::X509::Certificate.new(...), ...]

ecc_cert = ...
ecc_pkey = ...
ecc_cas = ...

ctx.add_certificate(rsa_cert, rsa_pkey, rsa_cas)
ctx.add_certificate(ecc_cert, ecc_pkey, ecc_cas)

This code adds two sets of (certs, private key) pair to the SSLContext, and either of them will be chosen during the TLS handshake, depending on the request by the client. It's not clear what SSLContext#{cert,key,extra_chain_cert} should return.

The document could be written better, instead of just stating "#cert is deprecated."

@thekuwayama
Copy link
Contributor Author

@rhenium Thank you for comments. I see.
In that case, it is as well that OpenSSL::SSL::SSLContext#add_certificate_chain_file. (#325)

I close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants