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

Add 'sigalgs=' method for setting accepted signature algorithms #769

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions ext/openssl/extconf.rb
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ def find_openssl_library
have_func("EVP_PKEY_check(NULL)", evp_h)
have_func("EVP_PKEY_new_raw_private_key(0, NULL, (unsigned char *)\"\", 0)", evp_h)
have_func("SSL_CTX_set_ciphersuites(NULL, \"\")", ssl_h)
have_func("SSL_CTX_set1_sigalgs(NULL, NULL, 0L)", ssl_h)
Copy link
Member

Choose a reason for hiding this comment

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

Please check SSL_CTX_set1_sigalgs_list() (with _list) as it is the function used in ossl_ssl.c.


# added in 3.0.0
have_func("SSL_set0_tmp_dh_pkey(NULL, NULL)", ssl_h)
Expand Down
33 changes: 33 additions & 0 deletions ext/openssl/ossl_ssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1103,6 +1103,36 @@ ossl_sslctx_set_ciphersuites(VALUE self, VALUE v)
}
#endif

#ifdef HAVE_SSL_CTX_SET1_SIGALGS
/*
* call-seq:
* ctx.sigalgs = sigalgs_list -> sigalgs_list
*
* Sets the list of "supported signature algorithms" for this context.
*
* For a TLS client, the list is directly used in the supported
* signature algorithm list in the client hello message. For a server,
* the list is used by OpenSSL to determine the set of shared signature
* algorithms. OpenSSL will pick the most appropriate one from it.
*/
static VALUE
ossl_sslctx_set_sigalgs(VALUE self, VALUE v)
{
SSL_CTX *ctx;

if (NIL_P(v))
return v;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't ssl_ctx.sigalgs = nil be an error (i.e., do nothing and let StringValueCStr() raise TypeError)?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe yes, since one most likely did not expect this to be a no-op. I originally decided to implement it consistent to ctx.ciphersuites = nil, which also silently ignores an nil-Argument.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch... ctx.ciphersuites = nil doesn't seem right either.


rb_check_frozen(self);
GetSSLCTX(self, ctx);

if (!SSL_CTX_set1_sigalgs_list(ctx, StringValueCStr(v)))
ossl_raise(eSSLError, "SSL_CTX_set1_sigalgs_list");

return v;
}
#endif

#ifndef OPENSSL_NO_DH
/*
* call-seq:
Expand Down Expand Up @@ -2898,6 +2928,9 @@ Init_ossl_ssl(void)
#ifdef HAVE_SSL_CTX_SET_CIPHERSUITES
rb_define_method(cSSLContext, "ciphersuites=", ossl_sslctx_set_ciphersuites, 1);
#endif
#ifdef HAVE_SSL_CTX_SET1_SIGALGS
rb_define_method(cSSLContext, "sigalgs=", ossl_sslctx_set_sigalgs, 1);
#endif
#ifndef OPENSSL_NO_DH
rb_define_method(cSSLContext, "tmp_dh=", ossl_sslctx_set_tmp_dh, 1);
#endif
Expand Down
30 changes: 30 additions & 0 deletions test/openssl/test_ssl.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1755,6 +1755,36 @@ def test_ciphers_method_tls_connection
end
end

def test_sigalgs_method_nil_argument
ssl_ctx = OpenSSL::SSL::SSLContext.new
pend 'sigalgs= method is missing' unless ssl_ctx.respond_to?(:sigalgs=)

assert_nothing_raised { ssl_ctx.sigalgs = nil }
end

def test_sigalgs_method_frozen_object
ssl_ctx = OpenSSL::SSL::SSLContext.new
pend 'sigalgs= method is missing' unless ssl_ctx.respond_to?(:sigalgs=)

ssl_ctx.freeze
assert_raise(FrozenError) { ssl_ctx.sigalgs = '"ECDSA+SHA256:RSA+SHA256"' }
end

def test_sigalgs_method_valid_sigalgs
ssl_ctx = OpenSSL::SSL::SSLContext.new
pend 'sigalgs= method is missing' unless ssl_ctx.respond_to?(:sigalgs=)

ssl_ctx.freeze
assert_raise(FrozenError) { ssl_ctx.sigalgs = '"ECDSA+SHA256:RSA+SHA256"' }
end
Comment on lines +1754 to +1760
Copy link
Member

Choose a reason for hiding this comment

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

This seems unfinished - was it supposed to be the successful path?

There should be a test case that involves an actual TLS session with start_server.


def test_sigalgs_method_bogus_sigalgs
ssl_ctx = OpenSSL::SSL::SSLContext.new
pend 'sigalgs= method is missing' unless ssl_ctx.respond_to?(:sigalgs=)

assert_raise(OpenSSL::SSL::SSLError) { ssl_ctx.sigalgs = 'BOGUS' }
end

rhenium marked this conversation as resolved.
Show resolved Hide resolved
def test_ciphers_method_nil_argument
ssl_ctx = OpenSSL::SSL::SSLContext.new
assert_nothing_raised { ssl_ctx.ciphers = nil }
Expand Down