Add post-quantum cryptography (PQC) support to SSLConfig - #53
Conversation
|
I see the following CI failure. Let me fix it. https://github.com/ruby/drb/actions/runs/30444292244/job/90550763624?pr=53 |
7e5fc39 to
2a55f51
Compare
The cause of the CI failure was that https://github.com/ruby/openssl/blob/v2.2.0/History.md?plain=1#L72-L75 Now use And use the following logic in ML-DSA-NN case. |
I tested this PR in RubyCI's RHEL 9 and OpenBSD servers. In RHEL 9 server, I confirmed the tests passed. The ML-DSA tests are successfully omitted by In OpenBSD server, I confirmed the tests passed as well. The used OpenSSL version was In I was not sure if However, |
|
@seki or other maintainers, I updated notes section on the first comment. This PR is ready to review. I would appreciate your reviews. |
|
@rhenium I would appreciate your review for this PR as Ruby OpenSSL (openssl gem) maintainer. |
2a55f51 to
692613d
Compare
692613d to
a3c95bc
Compare
|
I added and updated the code comments. |
I won't be the one deciding whether to merge this, but I'm not really convinced there's much value in being able to set up a server with multiple self-signed certificates. |
|
Sorry for my late response. Thanks for your reviews. I will check the reviews and fix for them. |
This commit fixes the following CI failures in Windows. I don't know why this change fixed. Perhaps Bundler's handling with git repository is different. https://github.com/ruby/drb/actions/runs/32029577375/job/95386338154?pr=54 ``` D:/a/drb/drb/test/lib/helper.rb:2:in 'Kernel#require': cannot load such file -- core_assertions (LoadError) from D:/a/drb/drb/test/lib/helper.rb:2:in '<top (required)>' from <internal:D:/rubyinstaller-head-x64/lib/ruby/4.1.0+4/rubygems/core_ext/kernel_require.rb>:139:in 'Kernel#require' rake aborted! Command failed with status (1) D:/a/drb/drb/vendor/bundle/ruby/4.1.0+4/gems/rake-13.4.2/exe/rake:27:in '<top (required)>' D:/rubyinstaller-head-x64/bin/bundle:33:in '<main>' Tasks: TOP => default => test (See full trace by running task with --trace) Error: Process completed with exit code 1. ```
OpenSSL::PKey::RSA#public_key is obsolete according to ruby/openssl#1092.
a3c95bc to
328508f
Compare
|
I rebased this PR, fixing the things mentioned by reviews. First, I backed up the initial PR into my fork repository's wip/support-pqc-pr1 branch. Now this PR has 3 commits. The 1st commit is the same with #54 to pass the CI. I will remove this commit on this PR after #54 is merged or someone fixed the issue causing failing Windows CI cases. The 2nd commit is for refactoring, which was mentioned in the review. But it is not related to this PR. The 3rd commit is the main commit. I simplified the tests in test/drb/test_drbssl.rb without new service classes and Commit messagesThe 2nd commitRemove OpenSSL::PKey::RSA#public_key OpenSSL::PKey::RSA#public_key is obsolete according to ruby/openssl#1092. The 3rd commit (main commit)Add post-quantum cryptography (PQC) support to SSLConfig DRb::DRbSSLSocket::SSLConfig already works with pre-generated ML-DSA cert/key by This commit adds the following features in PQC use cases to #setup_certificate
Assisted-by: Claude:claude-opus-4-6[1m] |
328508f to
7908ac0
Compare
This is a valid point. I believe multiple (dual) certification server accepting both non-PQC (RSA) and PQC (ML-DSA) is useful in the process of migrating the server from non-PQC to PQC. But the I think as |
7908ac0 to
02435ab
Compare
DRb::DRbSSLSocket::SSLConfig already works with pre-generated ML-DSA cert/key by
:SSLCertificate and :SSLPrivateKey config options.
This commit adds the following features in PQC use cases to #setup_certificate
and #setup_ssl_context.
* Add :SSLCertificates config option accepting an Array of
[certificate, private_key] pairs for multiple certificate
support via OpenSSL::SSL::SSLContext#add_certificate
changing from OpenSSL::SSL::SSLContext#cert and #key.
Because an SSL server that accepts clients with either ML-DSA-NN
or RSA certificates is useful in the use case of PQC migration from RSA
(non-PQC) to ML-DSA (PQC).
:SSLCertificate and :SSLPrivateKey are available for backward compatibility.
But use OpenSSL::SSL::SSLContext#add_certificate internally.
This is a behavior change.
* Add :SSLPrivateKeyAlgorithms option accepting an Array of key
algorithms (RSA, ML-DSA-44, ML-DSA-65, ML-DSA-87) for
multi-certificate generation, defaulting to ["RSA"] for backward
compatibility.
* Add :SSLGroups option to control key exchange group such as ML-KEM in PQC.
* Add :SSLSignatureAlgorithms, :SSLClientSignatureAlgorithms to control
signature algorithms such as ML-DSA-NN in PQC, and RSA in non-PQC.
* Add tool/create_certs.rb to generate test/drb/fixtures/*.{crt,key}
to test with pre-generated certs/keys and add DRbTests::Fixtures module
to read the certs/keys.
* Add test/drb/test_ssl.rb to test lib/drb/ssl.rb at the unit test level.
This approach aligns with test/drb/test_acl.rb to test lib/drb/acl.rb
at the unit test level.
* Add TestDRbSSLPQC, TestDRbSSLPQCMultipleCert in test/drb/test_drbssl.rb.
TestDRbSSLPQC is to test single PQC ML-KEM/ML-DSA server and client.
TestDRbSSLPQCMultipleCert is to test ML-DSA-65 (PQC) and RSA (non-PQC)
multiple certificate server with client certificate verification.
The test is designed for a high-security use case with :SSLVerifyMode
OpenSSL::SSL::VERIFY_PEER | OpenSSL::SSL::VERIFY_FAIL_IF_NO_PEER_CERT.
Assisted-by: Claude:claude-opus-4-6[1m]
02435ab to
4d037ce
Compare
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
Could you review auto generated code including tests carefully before you request a review by maintainers...?
| # Establish the OpenSSL::SSL::SSLContext with the configuration | ||
| # parameters provided. | ||
| def setup_ssl_context | ||
| ctx = ::OpenSSL::SSL::SSLContext.new |
There was a problem hiding this comment.
| ctx = ::OpenSSL::SSL::SSLContext.new | |
| ctx = OpenSSL::SSL::SSLContext.new |
There was a problem hiding this comment.
This :: has already existed on the master branch before this PR. But as this line is included in this PR's change, I will fix it.
Line 204 in a3d1558
| # parameters provided. | ||
| def setup_ssl_context | ||
| ctx = ::OpenSSL::SSL::SSLContext.new | ||
| @certs&.each do |cert, pkey| |
There was a problem hiding this comment.
Do we need & here? setup_certificate ensures @certs, right?
There was a problem hiding this comment.
Yes, I think we need & here. Because there is the case that @certs is nil in the following part, when the config have neither :SSLCertificates, and :SSLCertificate and :SSLPrivateKey.
def initialize(config)
...
@certs = if config.key?(:SSLCertificates)
config[:SSLCertificates]
elsif config[:SSLCertificate] && config[:SSLPrivateKey]
[[config[:SSLCertificate], config[:SSLPrivateKey]]]
end
One case @certs is nil is for example below.
config = {
SSLCertName: CERT_NAME
}
ssl_conf = DRb::DRbSSLSocket::SSLConfig.new(config)
ssl_conf.setup_certificate
We may want to write explicitly the nil case as follows. However, rubocop deals with this as style error. So, I removed the else nil part.
diff --git a/lib/drb/ssl.rb b/lib/drb/ssl.rb
index 3f46d09..6e923d4 100644
--- a/lib/drb/ssl.rb
+++ b/lib/drb/ssl.rb
@@ -169,6 +169,8 @@ module DRb
config[:SSLCertificates]
elsif config[:SSLCertificate] && config[:SSLPrivateKey]
[[config[:SSLCertificate], config[:SSLPrivateKey]]]
+ else
+ nil
end
@pkey_algs = self[:SSLPrivateKeyAlgorithms]
@ssl_ctx = nil|
|
||
| private | ||
|
|
||
| def setup_certificate_one(pkey_alg) |
There was a problem hiding this comment.
Could you rename this to generate_certificate or something? This doesn't set generated cert to any instance variables.
There was a problem hiding this comment.
The used term in setup_certificate_one is not set but setup. In my understanding, this doesn't mean setter method to set something to instance variables. For example Test::Unit::TestCase#setup is not a setter method. The term setup is like prepare. So, the setup_certificate_one means prepare one certificate. But if this method name confuses you, I will rename this method to generate_certificate.
| end | ||
|
|
||
| def probe_pqc_handshake | ||
| server = TCPServer.new('127.0.0.1', 0) |
There was a problem hiding this comment.
Could you use TCPServer.open do ... end instead?
There was a problem hiding this comment.
I created the probe_pqc_handshake based on the content of probe_pqc_handshake in ruby/rubygems. Because I wanted to align with the content between rubygems and drb. I am considering moving these PQC related methods into test-unit-ruby-core gem in the future if more ruby/* projects uses these methods.
On the other hand, I can understand a merit of applying the TCPServer.open do ... end.
This is a trade-off. And I will fix on your way.
| ssl_conf.setup_ssl_context | ||
| ssl_ctx = ssl_conf.instance_variable_get(:@ssl_ctx) | ||
|
|
||
| assert_instance_of(OpenSSL::SSL::SSLContext, ssl_ctx) |
There was a problem hiding this comment.
The following lines in lib/drb/ssl.rb are tested in this test test_setup_ssl_context_sigalgs in the case of non-PQC (RSA) with SSLSignatureAlgorithms: 'rsa_pss_rsae_sha256'. The reason why I added this test is because I believe that increasing the testing coverage rate is better.
def setup_ssl_context
...
if self[:SSLSignatureAlgorithms]
ctx.sigalgs = self[:SSLSignatureAlgorithms]
end
...
| ssl_conf.setup_ssl_context | ||
| ssl_ctx = ssl_conf.instance_variable_get(:@ssl_ctx) | ||
|
|
||
| assert_instance_of(OpenSSL::SSL::SSLContext, ssl_ctx) |
There was a problem hiding this comment.
Again the following lines in lib/drb/ssl.rb are tested in this test test_setup_ssl_context_sigalgs_pqc in the case of PQC with SSLSignatureAlgorithms: 'mldsa65:rsa_pss_rsae_sha256'. The ctx.sigalgs to set 'mldsa65:rsa_pss_rsae_sha256' means the signature algorithm accepts mldsa65 (ML-DSA-65) which is a typical PQC supported signature algorithm and rsa_pss_rsae_sha256 (RSA) which is also a typical non-PQC signature algorithm. I think that the dual PQC, non-PQC certificates case is typical in PQC case in the process of migration from non-PQC to PQC.
The reason why I added this test is because I believe that increasing the testing coverage rate is better.
def setup_ssl_context
...
if self[:SSLSignatureAlgorithms]
ctx.sigalgs = self[:SSLSignatureAlgorithms]
end
...
I am inspired by the ruby/openssl's test_pqc_sigalg test using ctx.sigalgs = "rsa_pss_rsae_sha256:mldsa65" which I added there in the past.
The dual PQC, non-PQC certificate server is encouraged in PQC case. Below is my company Red Hat's blog showing ML-DSA, RSA dual certificate server in PQC as a reference.
https://www.redhat.com/en/blog/post-quantum-cryptography-red-hat-enterprise-linux-10
$ openssl s_server
-cert localhost-mldsa.crt -key localhost-mldsa.key
-dcert localhost-rsa.crt -dkey localhost-rsa.key >/dev/null &
| def test_setup_ssl_context_sigalgs_method_not_supported | ||
| ctx_class = OpenSSL::SSL::SSLContext | ||
| unless ctx_class.method_defined?(:sigalgs=) | ||
| omit 'sigalgs= already not supported' | ||
| end | ||
|
|
||
| orig = ctx_class.instance_method(:sigalgs=) | ||
| ctx_class.undef_method(:sigalgs=) | ||
| begin | ||
| config = { | ||
| SSLCertificate: Fixtures.read_cert('rsa_server.crt'), | ||
| SSLPrivateKey: Fixtures.read_pkey('rsa_server.key'), | ||
| SSLSignatureAlgorithms: 'rsa_pss_rsae_sha256' | ||
| } | ||
| ssl_conf = DRb::DRbSSLSocket::SSLConfig.new(config) | ||
| error = assert_raise(NoMethodError) do | ||
| ssl_conf.setup_ssl_context | ||
| end | ||
| assert_equal("undefined method 'sigalgs=' for "\ | ||
| "an instance of OpenSSL::SSL::SSLContext", | ||
| error.message) | ||
| ensure | ||
| ctx_class.define_method(:sigalgs=, orig) | ||
| end | ||
| end |
There was a problem hiding this comment.
The answer is similar with my response for test_setup_certificate_unsupported_mldsa.
I originally created the test_setup_ssl_context_sigalgs_method_not_supported is to test the following part. After initial reviews, I removed the unless ... raise ... end part. However I still thought the test might be useful because OpenSSL::SSL::SSLContext#sigalgs= doesn't exist in majority of the cases on Ruby OpenSSL < 4.0.0. However, I will remove it.
def setup_ssl_context
...
if self[:SSLSignatureAlgorithms]
# OpenSSL::SSL::SSLContext#sigalgs=, #client_sigalgs=, #groups= were
# added in openssl gem 4.0.0.
# https://github.com/ruby/openssl/blob/v4.0.0/History.md?plain=1#L25-L31
# OpenSSL::SSL::SSLContext#sigalgs= is supported in OpenSSL >= 1.0.2.
# https://github.com/ruby/openssl/blob/v4.0.0/ext/openssl/extconf.rb#L142-L143
unless ctx.respond_to?(:sigalgs=)
raise(DRbBadConfig,
unsupported_message(
':SSLSignatureAlgorithms', '4.0.0', '1.0.2'
))
end
ctx.sigalgs = self[:SSLSignatureAlgorithms]
end
...
| def test_setup_ssl_context_client_sigalgs | ||
| ctx_class = OpenSSL::SSL::SSLContext | ||
| unless ctx_class.method_defined?(:client_sigalgs=) | ||
| omit 'client_sigalgs= not supported' | ||
| end | ||
|
|
||
| config = { | ||
| SSLCertificate: Fixtures.read_cert('rsa_server.crt'), | ||
| SSLPrivateKey: Fixtures.read_pkey('rsa_server.key'), | ||
| SSLClientSignatureAlgorithms: 'rsa_pss_rsae_sha256' | ||
| } | ||
| ssl_conf = DRb::DRbSSLSocket::SSLConfig.new(config) | ||
| ssl_conf.setup_ssl_context | ||
| ssl_ctx = ssl_conf.instance_variable_get(:@ssl_ctx) | ||
|
|
||
| assert_instance_of(OpenSSL::SSL::SSLContext, ssl_ctx) | ||
| end |
There was a problem hiding this comment.
I think this is a similar topic with https://github.com/ruby/drb/pull/53/changes#r3817894245. I assume your question here is do we need the assignment for ctx_class = OpenSSL::SSL::SSLContext?
And I think we need this assignment ctx_class = OpenSSL::SSL::SSLContext. The reason is
DRb::DRbSSLSocket::SSLConfig#setup_ssl_context uses OpenSSL::SSL::SSLContext#client_sigalgs internally in the test if the :SLClientSignatureAlgorithms config option is set. And OpenSSL::SSL::SSLContext#client_sigalgs was dded in Ruby OpenSSL (openssl gem) 4.0.0. Only Ruby 4.0.x uses Ruby OpenSSL version 4.0.x.
https://github.com/ruby/openssl/blob/v4.0.0/History.md?plain=1#L25-L27
def setup_ssl_context
...
if self[:SSLClientSignatureAlgorithms]
ctx.client_sigalgs = self[:SSLClientSignatureAlgorithms]
end
...
I think without this assignment and omit logic, the test raises NoMethodError.
| def test_setup_ssl_context_client_sigalgs_pqc | ||
| ctx_class = OpenSSL::SSL::SSLContext | ||
| unless ctx_class.method_defined?(:client_sigalgs=) | ||
| omit 'client_sigalgs= not supported' | ||
| end | ||
| omit_unless_support_pqc | ||
|
|
||
| config = { | ||
| SSLCertificate: Fixtures.read_cert('mldsa65_server.crt'), | ||
| SSLPrivateKey: Fixtures.read_pkey('mldsa65_server.key'), | ||
| SSLClientSignatureAlgorithms: 'mldsa44:mldsa65:mldsa87' | ||
| } | ||
| ssl_conf = DRb::DRbSSLSocket::SSLConfig.new(config) | ||
| ssl_conf.setup_ssl_context | ||
| ssl_ctx = ssl_conf.instance_variable_get(:@ssl_ctx) | ||
|
|
||
| assert_instance_of(OpenSSL::SSL::SSLContext, ssl_ctx) | ||
| end |
There was a problem hiding this comment.
Too many ditto... I give up reviewing this...
There was a problem hiding this comment.
I assume your question is do we need the assignment ctx_class = OpenSSL::SSL::SSLContext. And my answer is yes I think we need the assignment. The test test_setup_ssl_context_client_sigalgs is for a non-PQC case with SSLClientSignatureAlgorithms: 'rsa_pss_rsae_sha256', and this test test_setup_ssl_context_client_sigalgs_pqc is for a PQC case with SSLClientSignatureAlgorithms: 'mldsa44:mldsa65:mldsa87'.
| sh 'openssl', 'genpkey', *genpkey_args, | ||
| '-out', File.join(dir, 'ca.key') |
There was a problem hiding this comment.
Do we really think that this .sh -> .rb conversion meaningful...? Could you use openssl gem instead of opnessl command?
There was a problem hiding this comment.
I think you expected I would use openssl gem here. But I did implement differently against your expectation.
Because when I saw your suggestion to change the tool/create_certs.sb to tool/create_certs.rb. I assumed you didn't like the syntax of sh/bash, because of lower maintainability. And the openssl command is already used and verified in ruby/rubygems in tool/create_certs.sb. I would prioritized the alignment with ruby/rubygems repository over effectiveness or Ruby friendly code.
The merits of create_certs.rb with opessl gem
- Ruby friendly. It's good for people who are familiar with openssl gem.
- Effective without calling sub process
The merits of create_certs.rb with openssl command
- Consistency with ruby/rubygems's
tool/create_certs.sh. - We don't need to create new logic with openssl gem
- The
opensslcommands are good for people who are more familiar withopensslcommands than openssl gem like me.
I think our 2 different design decisions comes from different priorities here.
As I understood your preference now, I will use openssl gem in tool/create_cert.rb.
I understand you are busy across ruby/* libraries. Some contexts and reasons might be obvious in your mind, and you might think you didn't have to say the contexts and reasons. But it would be helpful if you would tel me the context and why reasons for your suggestion.
@kou Where did you see the code where the code is generated and but not carefully reviewed by me in this PR? I uses Claude Code as I added the text I understand you are busy across ruby/* projects and you are experienced developer. I would appreciate your reviews. Perhaps you might have bad experiences for PRs using Claude Code. But I don't want you to see me as a lazy person here. |
4 participants
This PR is related to #52.
Proof of concept
I prepared proof-of-concept scripts for this PR.
The document (
drb/README.md) is here. A list of the sections in thedrb/README.mdis below. The sections without "(development)" test with the current master branch. These work. The sections with "(development)" test with this PR.The script is here. The CI result is here.
Commit message
DRb::DRbSSLSocket::SSLConfig already works
with pre-generated ML-DSA cert/key by
:SSLCertificate and :SSLPrivateKey config options.
This commit adds the following features in PQC use cases to #setup_certificate and #setup_ssl_context.
Assisted-by: Claude:claude-opus-4-6[1m]
Notes
CI cases supporting PQC
PQC (ML-KEM/ML-DSA) works in OpenSSL >= 3.5. OpenSSL 3.5 added the feature. And works in Ruby OpenSSL >= 4.0. Ruby OpenSSL 4.0.0 added some functions in PQC use cases, and fixed a bug.
The
.github/workflows/test.ymluses runner ubuntu-latest, macos-latest, windows-latest.According to https://github.com/actions/runner-images, ubuntu-latest is ubuntu-24.04, macos-latest is macOS-26-arm64, windows-latest is windows-2025-vs2026.
The ubuntu-24.04 uses OpenSSL 3.0.13 which doesn't support PQC. The windows-2025-vs2026 uses OpenSSL 3.6.3 which supports PQC. The macos-26 uses OpenSSL 3.6.2 which supports PQC. The new PQC tests should be executed on the macos-latest and windows-latest CI cases.
test/drb/test_ssl.rb testing lib/drb/ssl.rb as an unit test level
I checked all the existing
test/drb/test_*.rbfiles.So, I created test/drb/test_ssl.rb to test
DRb::DRbSSLSocket::SSLConfigin lib/drb/ssl.rb as an unit test level.tool/create_certs.sh design decisions
The tool/create_certs.sh is inspired by ruby/rubygems#9678. The directory test/drb/fixtures to manage SSL key/cert files is inspired by ruby/openssl managing SSL keys in test/openssl/fixtures/pkey directory. ruby/openssl manages only key files, not certificate files. ruby/openssl is generating testing certificates from the keys in tests.
Testing in RubyCI servers
I plan to test this PR with RHEL 9 server (OpenSSL version is >= 3.5, but OpenSSL config disables PQC by default), which is the case of
support_pqc_handshake?isfalse,and OpenBSD server as I saw the following logic inDRbSSLService, test/drb/test_drbssl.rb.Edited: I confirmed the tests passed in RubyCI RHEL 9 and OpenBSD servers.
drbssl PQC tests
Single cert tests ML-DSA-65
This test with
TestDRbSSLPQCandDRbSSLPQCServiceis inspired by my proof-of-concept "## drbssl (SSL) auto-generated ML-DSA-65 cert". Also closed to theTestDRbSSLCoreandDRbSSLService.Multi cert tests ML-DSA-65/RSA
This test is inspired by my proof-of-concept "drbssl (SSL) pre-generated ML-DSA-65/RSA multi cert with client cert" case, and also inspired by ruby/openssl test/openssl/test_ssl.rb test_pqc_sigalg (https://github.com/ruby/openssl/blob/9796ee8f47003ec78fd8e052bc8dd6d1fcb0ae2b/test/openssl/test_ssl.rb#L2097).
PQC multi cert tests are executed via the following classes.
It is hard to understand the testing framework in test/drb.
In
DRbSSLPQCMultiCertMLDSA65Service, there aremanager_configandclient_config.manager_configis to manage the config used in manager to manage a child process ut_drb_drbssl_pqc_multi_cert.rb in this case. The main test workflow is between the server test/drb/ut_drb_drbssl_pqc_multi_cert.rb and client withclient_configpart. Client withclient_configconnects to the server ut_drb_drbssl_pqc_multi_cert.rb. However, it seems the ut_drb_drbssl_pqc_multi_cert.rb's config is also used as a client to connect to the servermanager_config.