Add feature for loading the chained certificate into Certificate array #361
Conversation
ext/openssl/ossl_x509cert.c
Outdated
| in = BIO_new(BIO_s_file()); | ||
| if (in == NULL) { | ||
| SSL_CTX_free(ctx); | ||
| ossl_raise(eX509CertError, NULL); | ||
| } | ||
|
|
||
| if (BIO_read_filename(in, StringValueCStr(path)) <= 0) { | ||
| SSL_CTX_free(ctx); | ||
| ossl_raise(eX509CertError, NULL); | ||
| } | ||
|
|
||
| x509 = PEM_read_bio_X509_AUX(in, NULL, NULL, NULL); | ||
| if (x509 == NULL) { | ||
| BIO_free(in); | ||
| SSL_CTX_free(ctx); | ||
| ossl_raise(eX509CertError, NULL); | ||
| } | ||
|
|
||
| SSL_CTX_use_certificate(ctx, x509); | ||
| if (ERR_peek_error() != 0) { | ||
| X509_free(x509); | ||
| BIO_free(in); | ||
| SSL_CTX_free(ctx); | ||
| ossl_raise(eX509CertError, NULL); /* Key/certificate mismatch doesn't imply */ | ||
| } | ||
|
|
||
| /* | ||
| * If we could set up our certificate, now proceed to the CA | ||
| * certificates. | ||
| */ | ||
| SSL_CTX_clear_chain_certs(ctx); | ||
| while ((ca = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL) { | ||
| int r = SSL_CTX_add0_chain_cert(ctx, ca); | ||
| /* | ||
| * Note that we must not free ca if it was successfully added to | ||
| * the chain (while we must free the main certificate, since its | ||
| * reference count is increased by SSL_CTX_use_certificate). | ||
| */ | ||
|
|
||
| if (!r) { | ||
| X509_free(ca); | ||
| X509_free(x509); | ||
| BIO_free(in); | ||
| SSL_CTX_free(ctx); | ||
| ossl_raise(eX509CertError, NULL); | ||
| } | ||
| } | ||
|
|
||
| /* When the while loop ends, it's usually just EOF. */ | ||
| err = ERR_peek_last_error(); | ||
| if (! (ERR_GET_LIB(err) == ERR_LIB_PEM | ||
| && ERR_GET_REASON(err) == PEM_R_NO_START_LINE)) { | ||
| /* some real error */ | ||
| X509_free(x509); | ||
| BIO_free(in); | ||
| SSL_CTX_free(ctx); | ||
| ossl_raise(eX509CertError, NULL); | ||
| } | ||
|
|
||
| ERR_clear_error(); |
There was a problem hiding this comment.
|
Commit: 17f55db For proposing
|
ext/openssl/ossl_x509cert.c
Outdated
| X509 *x509, *ca; | ||
| STACK_OF(X509) *certs_stack; | ||
| unsigned long err; | ||
| SSL_CTX *ctx = SSL_CTX_new(TLS_method()); |
There was a problem hiding this comment.
You should be able to avoid the need for this, and just create an array and put the certificates into that array below:
ext/openssl/ossl_x509cert.c
Outdated
| */ | ||
| SSL_CTX_clear_chain_certs(ctx); | ||
| while ((ca = PEM_read_bio_X509(in, NULL, NULL, NULL)) != NULL) { | ||
| int r = SSL_CTX_add0_chain_cert(ctx, ca); |
There was a problem hiding this comment.
Instead of adding to the context, add it to the array here.
Also need to check what is the difference between PEM_read_bio_X509 and PEM_read_bio_x509_AUX.
There was a problem hiding this comment.
Thank you for your important point!
Now I can follow that[1]:
- with
_AUXwill process a trusted X509 certificates - without
_AUXwill process an X509 certificates- will process a trusted X509 certificates but any trusted settings are discarded.
[1] https://www.openssl.org/docs/manmaster/man3/PEM_read_bio_X509.html#DESCRIPTION
As I understand, the trusted setting is only for trusted certificate [2], so using the function without _AUX will be fit for the purpose of this function? (Because of "discarded" meaning in [1], I may try both myself and see the differences anyway)
[2] https://www.openssl.org/docs/man1.0.2/man1/x509.html#TRUST-SETTINGS
There was a problem hiding this comment.
Hi again 😄! Could you please review my new commit d24041a? Thank you~
There was a problem hiding this comment.
I'm continuing the implementation for also reading the DER format certificate.
For now, both PEM format and DER format could be read, but DER format only can read the top one certificate. I'm trying to fix this.
When you have a time, could you please review the commit ff8572a? Thank you~
ext/openssl/ossl_x509cert.c
Outdated
| for (int cert_idx = 0; cert_idx < sk_X509_num(certs_stack); ++cert_idx) { | ||
| X509 * cert_c = sk_X509_value(certs_stack, cert_idx); | ||
| VALUE cert = ossl_x509_new(cert_c); | ||
| rb_ary_push(certs, cert); |
There was a problem hiding this comment.
(Although it's kind of rare condition,) rb_ary_new(), ossl_x509_new(), and rb_ary_push() involve memory allocation and can potentially raise a Ruby exception on failure, which is basically a longjmp().
To prevent possible memory leak, these function calls have to be wrapped by rb_protect().
There was a problem hiding this comment.
Thank you for careful review!
I think I found the proper supplement[3] for this potential Ruby exception with memory leak problem. I'll try apply rb_protect() soon.
sikuan
force-pushed
the
gsoc2020-ruby-sikuan-288
branch
from
17f55db
to
d24041a
Compare
|
Force-pushed commit: Diffs in
|
|
Force-pushed commit: Diffs in
|
|
Sorry, I totally dropped the ball on this one, it looks good to me but I'll need to review it. cc @rhenium |
|
Continuing here: #441 Thanks for your work. |
Load the chained certificate into Certificate array
New feature proposal for issue #288
Create two functions:
OpenSSL::X509::Certificate#load_file(path)string(File path)array(Array ofCertificateobjects)Certificateobjects, which are the chained certificatesBIOwrapper inext/openssl/ossl_x509cert.cOpenSSL::X509::Certificate#read_certificates(cert_content)array(Array ofCertificateobjects)Certificateobjects, which are the chained certificates