Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/drb/drb.rb
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,9 @@ def initialize(error)
attr_reader :reason
end

# Error raised when an error occurs on the bad configuration.
class DRbBadConfig < DRbError; end

@kou kou Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about using ArgumentError instead of our custom exception class?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I checked the current situation on the master branch.

The reason why I created DRbBadConfig for bad configuration was because I saw the following exceptions classes.

$ grep -r '^.*class.*DRbError' lib
lib/drb/drb.rb:# dRuby-specific errors, all of which are subclasses of DRb::DRbError.
lib/drb/drb.rb:  class DRbError < RuntimeError; end
lib/drb/drb.rb:  class DRbConnError < DRbError; end
lib/drb/drb.rb:  class DRbServerNotFound < DRbError; end
lib/drb/drb.rb:  class DRbBadURI < DRbError; end
lib/drb/drb.rb:  class DRbBadScheme < DRbError; end
lib/drb/drb.rb:  class DRbUnknownError < DRbError
lib/drb/drb.rb:  class DRbRemoteError < DRbError

However, now I notice Ruby standard exception classes ArgumentError, NoMethodError, and SecurityError are also used.

$ grep -r raise lib/ | grep -E 'ArgumentError|NoMethodError|SecurityError'
lib/drb/drb.rb:      raise(ArgumentError, "#{any_to_s(msg_id)} is not a symbol") unless Symbol == msg_id.class
lib/drb/drb.rb:      raise(SecurityError, "insecure method '#{msg_id}'") if insecure_method?(msg_id)
lib/drb/drb.rb:          raise NoMethodError, "private method '#{msg_id}' called for #{desc}"
lib/drb/drb.rb:          raise NoMethodError, "protected method '#{msg_id}' called for #{desc}"
lib/drb/drb.rb:          raise NoMethodError, "private method '#{msg_id}' called for #{desc}"
lib/drb/drb.rb:          raise NoMethodError, "protected method '#{msg_id}' called for #{desc}"

I also see the following code and past commit about raising DRb custom exception classes.

drb/lib/drb/drb.rb

Lines 511 to 513 in 2b002d4

rescue NameError, ArgumentError
DRbUnknown.new($!, s)
end

drb/lib/drb/drb.rb

Lines 619 to 623 in 2b002d4

begin
Marshal::load(str)
rescue NameError, ArgumentError
DRbUnknown.new($!, str)
end

The commit 0be5d9b is notable. This commit replaced the raised exception class from ArgumentError to DRbConnError. Though I don't know the context or reason of this change.

And I personally interpret the above code comment "dRuby-specific errors, all of which are subclasses of DRb::DRbError". We should use DRb specific (custom) exception classes for DRb specific things, that is anything that can happen in ruby/drb.

What is your justification or reason to use Ruby's standard ArgumentError class rather than DRb custom exception classes?


# Class wrapping a marshalled object whose type is unknown locally.
#
# If an object is returned by a method invoked over drb, but the
Expand Down
210 changes: 166 additions & 44 deletions lib/drb/ssl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,25 @@ class SSLConfig
#
# See DRb::DRbSSLSocket::SSLConfig.new for more details
DEFAULT = {
:SSLCertificate => nil,
:SSLPrivateKey => nil,
:SSLClientCA => nil,
:SSLCACertificatePath => nil,
:SSLCACertificateFile => nil,
:SSLTmpDhCallback => nil,
:SSLVerifyMode => ::OpenSSL::SSL::VERIFY_NONE,
:SSLVerifyDepth => nil,
:SSLVerifyCallback => nil, # custom verification
:SSLCertificateStore => nil,
:SSLCertificates => nil,
:SSLCertificate => nil,
:SSLPrivateKey => nil,
:SSLPrivateKeyAlgorithms => ["RSA"],
:SSLClientCA => nil,
:SSLCACertificatePath => nil,
:SSLCACertificateFile => nil,
:SSLSignatureAlgorithms => nil,
:SSLClientSignatureAlgorithms => nil,
:SSLGroups => nil,
:SSLTmpDhCallback => nil,
:SSLVerifyMode => ::OpenSSL::SSL::VERIFY_NONE,
:SSLVerifyDepth => nil,
:SSLVerifyCallback => nil, # custom verification
:SSLCertificateStore => nil,
# Must specify if you use auto generated certificate.
:SSLCertName => nil, # e.g. [["CN","fqdn.example.com"]]
:SSLCertComment => "Generated by Ruby/OpenSSL"
# e.g. [["CN", "fqdn.example.com"]]
:SSLCertName => nil,
:SSLCertComment => "Generated by Ruby/OpenSSL"
}

# Create a new DRb::DRbSSLSocket::SSLConfig instance
Expand All @@ -51,13 +57,29 @@ class SSLConfig
#
# From +config+ Hash:
#
# :SSLCertificates ::
# An Array of [certificate, private_key] pairs. Each element
# is an Array of an OpenSSL::X509::Certificate and its
# corresponding private key. This option is prioritized over
# :SSLCertificate and :SSLPrivateKey. See
# OpenSSL::SSL::SSLContext#add_certificate
#
# :SSLCertificate ::
# An instance of OpenSSL::X509::Certificate. If this is not provided,
# then a generic X509 is generated, with a correspond :SSLPrivateKey
#
# :SSLPrivateKey ::
# A private key instance, like OpenSSL::PKey::RSA. This key must be
# the key that signed the :SSLCertificate
# A private key instance, like OpenSSL::PKey::RSA for RSA
# and OpenSSL::PKey::PKey for ML-DSA-44, ML-DSA-65,
# ML-DSA-87. This key must be the key that signed the
# :SSLCertificate
#
# :SSLPrivateKeyAlgorithms ::
# An Array of private key algorithms used to generate
# certificates when :SSLCertificates, :SSLCertificate and
# :SSLPrivateKey are not provided. Supported algorithms are
# RSA, ML-DSA-44, ML-DSA-65, and ML-DSA-87.
# Defaults to ["RSA"]
#
# :SSLClientCA ::
# An OpenSSL::X509::Certificate, or Array of certificates that will
Expand All @@ -70,6 +92,15 @@ class SSLConfig
# :SSLCACertificateFile ::
# A path to a CA certificate file, in PEM format.
#
# :SSLSignatureAlgorithms ::
# Signature algorithms. See OpenSSL::SSL::SSLContext#sigalgs=
#
# :SSLClientSignatureAlgorithms ::
# Client signature algorithms. See OpenSSL::SSL::SSLContext#client_sigalgs=
#
# :SSLGroups ::
# Key exchange groups. See OpenSSL::SSL::SSLContext#groups=
#
# :SSLTmpDhCallback ::
# A DH callback. See OpenSSL::SSL::SSLContext.tmp_dh_callback
#
Expand Down Expand Up @@ -133,10 +164,15 @@ class SSLConfig
# c.setup_certificate
#
def initialize(config)
@config = config
@cert = config[:SSLCertificate]
@pkey = config[:SSLPrivateKey]
@ssl_ctx = nil
@config = config
@certs = nil
if config[:SSLCertificate] && config[:SSLPrivateKey]
@certs = [[config[:SSLCertificate], config[:SSLPrivateKey]]]
end
@certs = config[:SSLCertificates] if config.key?(:SSLCertificates)
Comment on lines +168 to +172

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we simplify these conditions?

Suggested change
@certs = nil
if config[:SSLCertificate] && config[:SSLPrivateKey]
@certs = [[config[:SSLCertificate], config[:SSLPrivateKey]]]
end
@certs = config[:SSLCertificates] if config.key?(:SSLCertificates)
if config.key?(:SSLCertificates)
@certs = config[:SSLCertificates]
elsif config[:SSLCertificate] && config[:SSLPrivateKey]
@certs = [[config[:SSLCertificate], config[:SSLPrivateKey]]]
else
@certs = nil
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do you ignore :SSLCertificates when :SSLCertificate and :SSLPrivateKey are specified? (Why don't you mix them/)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are right. When I implemented and wrote the logic that :SSLCertificates overrides :SSLCertificate and :SSLPrivateKey. But this is my logic bug. This config option input behavior confuses users. Obviously we shouldn't mix the config option check between SSLCertificates and :SSLCertificate/:SSLPrivateKey even when we manage the internal value as one @certs. Maybe when I got an idea that managing internal value as one @certs for both :SSLCertificates and :SSLCertificate/:SSLPrivateKey, that made me blind, thinking such a mixed input check logic. I will fix it.

      # :SSLCertificates ::
      #   An Array of [certificate, private_key] pairs. Each element
      #   is an Array of an OpenSSL::X509::Certificate and its
      #   corresponding private key.  This option is prioritized over
      #   :SSLCertificate and :SSLPrivateKey.  See
      #   OpenSSL::SSL::SSLContext#add_certificate

@pkey_algs = config[:SSLPrivateKeyAlgorithms] ||
self[:SSLPrivateKeyAlgorithms]
Comment on lines +173 to +174

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need config[:SSLPrivateKeyAlgorithms] || here?

Suggested change
@pkey_algs = config[:SSLPrivateKeyAlgorithms] ||
self[:SSLPrivateKeyAlgorithms]
@pkey_algs = self[:SSLPrivateKeyAlgorithms]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are right. The self[:SSLPrivateKeyAlgorithms] is good enough considering the value by argument config. I will fix it.

      def initialize(config)
        @config   = config
      # A convenience method to access the values like a Hash
      def [](key);
        @config[key] || DEFAULT[key]
      end

@ssl_ctx = nil
end

# A convenience method to access the values like a Hash
Expand All @@ -162,25 +198,121 @@ def accept(tcp)
ssl
end

# Ensures that :SSLCertificate and :SSLPrivateKey have been provided
# or that a new certificate is generated with the other parameters
# provided.
# Ensures that :SSLCertificates or :SSLCertificate and
# :SSLPrivateKey have been provided, or that new certificates
# are generated with the other parameters provided.
def setup_certificate
if @cert && @pkey
if @certs
return
end

rsa = OpenSSL::PKey::RSA.new(2048)
@certs = @pkey_algs.map { |pkey_alg| setup_certificate_one(pkey_alg) }
end

# Establish the OpenSSL::SSL::SSLContext with the configuration
# parameters provided.
def setup_ssl_context

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do you want to make setup_ssl_context public? It's just for test? If so, we should not make it public. We can use __send__ in test.

@junaruga junaruga Aug 12, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In the current master branch 2b002d4, the setup_ssl_context is already public method. I didn't change the method from private to public.

def setup_ssl_context

ctx = ::OpenSSL::SSL::SSLContext.new
@certs&.each do |cert, pkey|
ctx.add_certificate(cert, pkey)
end
ctx.min_version = self[:SSLMinVersion]
ctx.max_version = self[:SSLMaxVersion]
ctx.client_ca = self[:SSLClientCA]
ctx.ca_path = self[:SSLCACertificatePath]
ctx.ca_file = self[:SSLCACertificateFile]

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OpenSSL 1.0.2 is the minimum version supported by ruby/openssl bundled with non-EOL Ruby versions. I think you can forget about even older versions.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK. I will remove the "1.0.2" information.

# 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
Comment on lines +231 to +236

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need this check? I think that NoMethodError is enough without this check.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I intended the this check to notify users the cause of the errors, and how to fix, notifying the information they need to upgrade Ruby OpenSSL or/and OpenSSL. At the same time, I don't understand how the standard libraries like ruby/drb should handle in the missing method cases due to old dependent libraries. So, if you say so, I will remove this check.

ctx.sigalgs = self[:SSLSignatureAlgorithms]
end

if self[:SSLClientSignatureAlgorithms]
# OpenSSL::SSL::SSLContext#client_sigalgs= is supported in
# OpenSSL >= 1.0.2.
# https://github.com/ruby/openssl/blob/v4.0.0/ext/openssl/extconf.rb#L144-L145
unless ctx.respond_to?(:client_sigalgs=)
raise(DRbBadConfig,
unsupported_message(
':SSLClientSignatureAlgorithms', '4.0.0', '1.0.2'
))
end
ctx.client_sigalgs = self[:SSLClientSignatureAlgorithms]
end

if self[:SSLGroups]
# OpenSSL::SSL::SSLContext#groups is supported in all OpenSSL
# versions.
# https://github.com/ruby/openssl/blob/v4.0.0/ext/openssl/ossl_ssl.c#L3031
unless ctx.respond_to?(:groups=)
raise(DRbBadConfig,
unsupported_message(
':SSLGroups', '4.0.0'
))
end
ctx.groups = self[:SSLGroups]
end

ctx.tmp_dh_callback = self[:SSLTmpDhCallback]
ctx.verify_mode = self[:SSLVerifyMode]
ctx.verify_depth = self[:SSLVerifyDepth]
ctx.verify_callback = self[:SSLVerifyCallback]
ctx.cert_store = self[:SSLCertificateStore]
@ssl_ctx = ctx
end

private

def setup_certificate_one(pkey_alg)
cert = OpenSSL::X509::Certificate.new

case pkey_alg
when "RSA"
pkey = OpenSSL::PKey::RSA.new(2048)
cert.public_key = pkey.public_key

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
cert.public_key = pkey.public_key
cert.public_key = pkey

I recognize this was taken from the original code, but the OpenSSL::PKey::RSA#public_key call was unnecessary. In OpenSSL's design, a private key can always act as a public key.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see. The examples of OpenSSL::PKey::RSA#public_key to set to OpenSSL::X509::Certificate#public_key are documented below. Maybe shall we improve the examples not to use OpenSSL::PKey::RSA#public_key in ruby/openssl?

https://github.com/ruby/openssl/blob/e324933034dffe45dce64b83c351d545c0fc93c4/ext/openssl/ossl_x509cert.c#L925

https://github.com/ruby/openssl/blob/e324933034dffe45dce64b83c351d545c0fc93c4/ext/openssl/ossl_x509cert.c#L946

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't realize it was in ruby/openssl's docs. Yes, it's unnecessary and we should clean it up.

The only valid use case for RSA#public_key was in method chains with #to_der/#to_pem (e.g., rsa.public_key.to_der), but it should be updated to use rsa.public_to_der nowadays.

digest = "SHA256"
when "ML-DSA-44", "ML-DSA-65", "ML-DSA-87"
# openssl gem >= 4.0.0 and OpenSSL >= 3.5.0 support ML-DSA.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should work with all ruby/openssl versions as long as it's linked with OpenSSL >= 3.5. Perhaps it's best to pass through the OpenSSL::PKey::PKeyError (for minimum amount of code in drb, and for allowing the user see the raw context information that may be added by OpenSSL)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe you are talking about the following part.

          begin
            pkey = OpenSSL::PKey.generate_key(pkey_alg)
          rescue OpenSSL::PKey::PKeyError

For the topic to pass through OpenSSL::PKey::PKeyError, a similar topic to pass through NoMethodError is also suggested on another thread https://github.com/ruby/drb/pull/53/changes#r3729234016. Considering these 2 similar suggestions from you and kou, I would agree with you guys that we should pass through the OpenSSL::PKey::PKeyError and NoMethodError without rescuing these exception classes. I will fix it.

# https://github.com/ruby/openssl/blob/v4.0.0/History.md#notable-changes
# https://openssl-library.org/post/2025-04-08-openssl-35-final-release/
begin
pkey = OpenSSL::PKey.generate_key(pkey_alg)
rescue OpenSSL::PKey::PKeyError
raise(DRbBadConfig,
unsupported_message(
"#{pkey_alg} algorithm", '4.0.0', '3.5.0'
))
end
# OpenSSL::PKey::PKey#public_to_der was added in openssl gem 2.2.0.
# https://github.com/ruby/openssl/blob/v2.2.0/History.md?plain=1#L72-L75
cert.public_key = OpenSSL::PKey.read(pkey.public_to_der)
# OpenSSL::X509::Certificate#sign doesn't accept explicit digest
# algorithm, in ML-DSA because ML-DSA has a built-in digest.
digest = nil
else
raise(DRbBadConfig,
"#{pkey_alg} algorithm not found. "\
"RSA, ML-DSA-44, ML-DSA-65, and ML-DSA-87 "\
"algorithms are supported")
end

cert.version = 2 # This means v3
cert.serial = 0
name = OpenSSL::X509::Name.new(self[:SSLCertName])
cert.subject = name
cert.issuer = name
cert.not_before = Time.now
cert.not_after = Time.now + (365*24*60*60)
cert.public_key = rsa.public_key

ef = OpenSSL::X509::ExtensionFactory.new(nil,cert)
cert.extensions = [
Expand All @@ -192,29 +324,19 @@ def setup_certificate
if comment = self[:SSLCertComment]
cert.add_extension(ef.create_extension("nsComment", comment))
end
cert.sign(rsa, "SHA256")
cert.sign(pkey, digest)

@cert = cert
@pkey = rsa
[cert, pkey]
end

# Establish the OpenSSL::SSL::SSLContext with the configuration
# parameters provided.
def setup_ssl_context
ctx = ::OpenSSL::SSL::SSLContext.new
ctx.cert = @cert
ctx.key = @pkey
ctx.min_version = self[:SSLMinVersion]
ctx.max_version = self[:SSLMaxVersion]
ctx.client_ca = self[:SSLClientCA]
ctx.ca_path = self[:SSLCACertificatePath]
ctx.ca_file = self[:SSLCACertificateFile]
ctx.tmp_dh_callback = self[:SSLTmpDhCallback]
ctx.verify_mode = self[:SSLVerifyMode]
ctx.verify_depth = self[:SSLVerifyDepth]
ctx.verify_callback = self[:SSLVerifyCallback]
ctx.cert_store = self[:SSLCertificateStore]
@ssl_ctx = ctx
def unsupported_message(name, ruby_openssl_version,
openssl_version=nil)
message = "#{name} not supported. "\
"Install openssl gem >= #{ruby_openssl_version}"
if openssl_version
message += " building with OpenSSL >= #{openssl_version}"
end
message
end
end

Expand Down
87 changes: 87 additions & 0 deletions test/drb/drbtest.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
require 'drb/drb'
require 'drb/extservm'
require 'timeout'
require_relative 'drbtest_utils'

module DRbTests

Expand Down Expand Up @@ -393,4 +394,90 @@ def test_07_break_18

end

# A PQC Utilities module inspired from ruby/rubygems omit_unless_support_pqc
module DRbPQCUtilities
# PQC algorithms ML-KEM and ML-DSA require OpenSSL >= 3.5.
# https://openssl-library.org/post/2025-04-08-openssl-35-final-release/
def support_pqc_openssl?
OpenSSL::OPENSSL_VERSION_NUMBER >= 0x30500000
end

# Ruby OpenSSL >= 4.0 supports OpenSSL::SSL::SSLContext#groups,
# OpenSSL::SSL::SSLSocket#sigalg, #peer_sigalg used in PQC cases, fixing the
# following issue.
# https://github.com/ruby/openssl/blob/v4.0.0/History.md#notable-changes
# https://github.com/ruby/openssl/commit/9614b7f67a629cc8d31b6fe5be5040c68263ca8b
def support_pqc_ruby_openssl?
Gem::Version.new(OpenSSL::VERSION) >= Gem::Version.new('4.0')
end

# Probe an actual PQC handshake between a forced-PQC server and a
# default-configured client, mirroring what the integration tests exercise.
# Memoized so the probe runs at most once per process.
def support_pqc_handshake?
return @support_pqc_handshake unless @support_pqc_handshake.nil?

@support_pqc_handshake = probe_pqc_handshake
end

def probe_pqc_handshake
server = TCPServer.new('127.0.0.1', 0)
ctx = OpenSSL::SSL::SSLContext.new
ctx.add_certificate(
Fixtures.read_cert('mldsa65_server.crt'),
Fixtures.read_pkey('mldsa65_server.key'))
ctx.groups = 'X25519MLKEM768'
ssl_server = OpenSSL::SSL::SSLServer.new(server, ctx)

port = server.addr[1]
server_thread = Thread.new do
client = ssl_server.accept
client.close
rescue OpenSSL::OpenSSLError
nil
end

client_ctx = OpenSSL::SSL::SSLContext.new
client_ctx.verify_mode = OpenSSL::SSL::VERIFY_NONE
socket = TCPSocket.new('127.0.0.1', port)
ssl = OpenSSL::SSL::SSLSocket.new(socket, client_ctx)
ssl.connect
ssl.close
true
rescue OpenSSL::OpenSSLError, SystemCallError
false
ensure
server_thread&.join(5)
server_thread&.kill if server_thread&.alive?
ssl_server&.close
server&.close
end

def omit_unless_support_pqc
unless support_pqc_openssl?
yield if block_given?
omit 'PQC algorithms require OpenSSL >= 3.5'
return
end
unless support_pqc_ruby_openssl?
yield if block_given?
omit 'PQC test requires Ruby OpenSSL >= 4.0'
return
end
unless support_pqc_handshake?
yield if block_given?
omit 'PQC handshake is not available in this OpenSSL configuration'
end
end
end

module DRbPQC
include DRbBase
include DRbPQCUtilities

def test_01_hello
assert_equal('hello', @there.hello)
end
end

end
Loading