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

Modify SSLContext#add_certificate_chain_file #309

Merged
merged 8 commits into from
Jan 24, 2020
87 changes: 82 additions & 5 deletions ext/openssl/ossl_ssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1322,15 +1322,92 @@ ossl_sslctx_add_certificate(int argc, VALUE *argv, VALUE self)
return self;
}

/*
* call-seq:
* ctx.add_certificate_chain_file(certs_path, pkey_path) -> self
*
* Loads chain certificates from _certs_path_ and a private key from
* _pkey_path_.
*
* === Parameters
* _certs_path_::
* A path to a chain certificates file. It may be a single certificate.
* An instance of String.
* _pkey_path_::
* A path to a private key file. An instance of String.
*
* === Example
* ctx.add_certificate_chain(rsa_cert_path, rsa_key_path)
thekuwayama marked this conversation as resolved.
Show resolved Hide resolved
*
* ctx.add_certificate_chain(ecdsa_cert_path, ecdsa_key_path)
*
* === Note
* The file format of the certificate and private key must be PEM.
*
* The certificate file must be starting with the subject's certificate and
* followed by intermediate CA certificate(s).
*
* OpenSSL before the version 1.0.2 could handle only one extra chain across
* all key types. Calling this method discards the chain set previously.
*/
static VALUE
ossl_sslctx_add_certificate_chain_file(VALUE self, VALUE path)
ossl_sslctx_add_certificate_chain_file(VALUE self, VALUE certs_path, VALUE pkey_path)
{
StringValue(path);
SSL_CTX *ctx = NULL;
SSL_CTX *ctx;
X509 *x509;
char *ccerts_path, *cpkey_path;
FILE *fp;
EVP_PKEY *pkey, *pub_pkey;

GetSSLCTX(self, ctx);

return SSL_CTX_use_certificate_chain_file(ctx, RSTRING_PTR(path)) == 1 ? Qtrue : Qfalse;
/* Retrieve private key */
cpkey_path = StringValueCStr(pkey_path);
fp = fopen(cpkey_path, "r");
if (!fp)
rb_raise(rb_eArgError, "failed to open pkey file");
pkey = PEM_read_PrivateKey(fp, NULL, 0, NULL);
fclose(fp);
if (!pkey)
rb_raise(rb_eArgError, "failed to open pkey file");

/* Retrieve public key */
ccerts_path = StringValueCStr(certs_path);
fp = fopen(ccerts_path, "r");
if (!fp) {
EVP_PKEY_free(pkey);
rb_raise(rb_eArgError, "failed to open certs file");
}
x509 = PEM_read_X509(fp, NULL, 0, NULL);
fclose(fp);
if (!x509) {
EVP_PKEY_free(pkey);
rb_raise(rb_eArgError, "failed to open certs file");
}
pub_pkey = X509_get_pubkey(x509);
if (!pub_pkey) {
EVP_PKEY_free(pkey);
X509_free(x509);
rb_raise(rb_eArgError, "certificate does not contain public key");
}
if (EVP_PKEY_cmp(pub_pkey, pkey) != 1) {
EVP_PKEY_free(pkey);
X509_free(x509);
EVP_PKEY_free(pub_pkey);
rb_raise(rb_eArgError, "public key mismatch");
}
EVP_PKEY_free(pkey);
X509_free(x509);
EVP_PKEY_free(pub_pkey);

/* SSL_CTX_use_certificate_chain_file() loads PEM format file. */
if (SSL_CTX_use_certificate_chain_file(ctx, ccerts_path) != 1)
ossl_raise(eSSLError, "SSL_CTX_use_certificate_chain_file");

if (SSL_CTX_use_PrivateKey_file(ctx, cpkey_path, SSL_FILETYPE_PEM) != 1)
ossl_raise(eSSLError, "SSL_CTX_use_PrivateKey_file");

return self;
}

/*
Expand Down Expand Up @@ -2784,7 +2861,7 @@ Init_ossl_ssl(void)
rb_define_method(cSSLContext, "enable_fallback_scsv", ossl_sslctx_enable_fallback_scsv, 0);
#endif
rb_define_method(cSSLContext, "add_certificate", ossl_sslctx_add_certificate, -1);
rb_define_method(cSSLContext, "add_certificate_chain_file", ossl_sslctx_add_certificate_chain_file, 1);
rb_define_method(cSSLContext, "add_certificate_chain_file", ossl_sslctx_add_certificate_chain_file, 2);

rb_define_method(cSSLContext, "setup", ossl_sslctx_setup, 0);
rb_define_alias(cSSLContext, "freeze", "setup");
Expand Down
103 changes: 101 additions & 2 deletions test/test_ssl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe create a separate test, since this is still a useful test, no?

Copy link
Contributor Author

@thekuwayama thekuwayama Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@thekuwayama thekuwayama Jan 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have modified add_certificate_chain_file to return self. It is the reason the following as:

  1. It is exceptional situations to fail to add the certificate or the private key. So, when failed to add, add_certificate_chain_file be better to raise an error instead of returning false, I think.
  2. add_certificate(method for a similar purpose) raises an error if failed to add, too.

# Create chain certificates file
certs = Tempfile.open { |f| f << @svr_cert.to_pem << @ca_cert.to_pem; f }
pkey = Tempfile.open { |f| f << @svr_key.to_pem; f }

ctx_proc = -> ctx {
# Unset values set by start_server
ctx.cert = ctx.key = ctx.extra_chain_cert = nil
assert_nothing_raised { ctx.add_certificate_chain_file(certs.path, pkey.path) }
}

start_server(ctx_proc: ctx_proc) { |port|
server_connect(port) { |ssl|
assert_equal @svr_cert.subject, ssl.peer_cert.subject
assert_equal [@svr_cert.subject, @ca_cert.subject],
ssl.peer_cert_chain.map(&:subject)

ssl.puts "abc"; assert_equal "abc\n", ssl.gets
}
}
ensure
certs&.close
pkey&.close
certs&.unlink
pkey&.unlink
end

def test_add_certificate_chain_file_multiple_certs
pend "EC is not supported" unless defined?(OpenSSL::PKey::EC)
pend "TLS 1.2 is not supported" unless tls12_supported?

# SSL_CTX_set0_chain() is needed for setting multiple certificate chains
add0_chain_supported = openssl?(1, 0, 2)

if add0_chain_supported
ca2_key = Fixtures.pkey("rsa2048")
ca2_exts = [
["basicConstraints", "CA:TRUE", true],
["keyUsage", "cRLSign, keyCertSign", true],
]
ca2_dn = OpenSSL::X509::Name.parse_rfc2253("CN=CA2")
ca2_cert = issue_cert(ca2_dn, ca2_key, 123, ca2_exts, nil, nil)
else
# Use the same CA as @svr_cert
ca2_key = @ca_key; ca2_cert = @ca_cert
end

ecdsa_key = Fixtures.pkey("p256")
exts = [
["keyUsage", "digitalSignature", false],
]
ecdsa_dn = OpenSSL::X509::Name.parse_rfc2253("CN=localhost2")
ecdsa_cert = issue_cert(ecdsa_dn, ecdsa_key, 456, exts, ca2_cert, ca2_key)

# Create chain certificates file
certs1 = Tempfile.open { |f| f << @svr_cert.to_pem << @ca_cert.to_pem; f }
pkey1 = Tempfile.open { |f| f << @svr_key.to_pem; f }
certs2 = Tempfile.open { |f| f << ecdsa_cert.to_pem << ca2_cert.to_pem; f }
pkey2 = Tempfile.open { |f| f << ecdsa_key.to_pem; f }

ctx_proc = -> ctx {
# Unset values set by start_server
ctx.cert = ctx.key = ctx.extra_chain_cert = nil
ctx.ecdh_curves = "P-256" unless openssl?(1, 0, 2)
assert_nothing_raised {
ctx.add_certificate_chain_file(certs1.path, pkey1.path) # RSA
ctx.add_certificate_chain_file(certs2.path, pkey2.path) # ECDSA
}
}

start_server(ctx_proc: ctx_proc) do |port|
ctx = OpenSSL::SSL::SSLContext.new
ctx.max_version = :TLS1_2
ctx.ciphers = "aRSA"
server_connect(port, ctx) { |ssl|
assert_equal @svr_cert.subject, ssl.peer_cert.subject
assert_equal [@svr_cert.subject, @ca_cert.subject],
ssl.peer_cert_chain.map(&:subject)

ssl.puts "abc"; assert_equal "abc\n", ssl.gets
}

ctx = OpenSSL::SSL::SSLContext.new
ctx.max_version = :TLS1_2
ctx.ciphers = "aECDSA"
server_connect(port, ctx) { |ssl|
assert_equal ecdsa_cert.subject, ssl.peer_cert.subject
assert_equal [ecdsa_cert.subject, ca2_cert.subject],
ssl.peer_cert_chain.map(&:subject)

ssl.puts "123"; assert_equal "123\n", ssl.gets
}
end
ensure
certs1&.close
pkey1&.close
certs1&.unlink
pkey1&.unlink
certs2&.close
pkey2&.close
certs2&.unlink
pkey2&.unlink
end

def test_sysread_and_syswrite
Expand Down