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 support to SSL_CTX_set_keylog_callback() #536

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

cdelafuente-r7
Copy link
Contributor

This adds support to OpenSSL's SSL_CTX_set_keylog_callback(). This callback is invoked when TLS key material is generated or received, in order to allow applications to store this keying material for debugging purposes. It is invoked with an SSLSocket and a string containing the key material in the format used by NSS for its SSLKEYLOGFILE debugging output.

This PR adds the Ruby binding keylog_cb and the related tests.

Note that It is only compatible with OpenSSL >= 1.1.1. Even if LibreSSL implements SSL_CTX_set_keylog_callback() from v3.4.2, it does nothing (see libressl/openbsd@648d39f)

  • Example
    context = OpenSSL::SSL::SSLContext.new
    context.keylog_cb = proc do |ary|
      _sock, line = ary
      File.open('ssl_keylog_file', "a") do |f|
        f.write("#{line}\n")
      end
    end

    tcp_socket = TCPSocket.new @host, @port
    ssl_client = OpenSSL::SSL::SSLSocket.new tcp_socket, context
    ssl_client.sync_close = true
    ssl_client.connect

    ssl_client.puts "hello server!"
    puts ssl_client.gets

    ssl_client.close

}
}
#else
#define ossl_sslctx_keylog_cb rb_f_notimplement
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this is required since the call to SSL_CTX_set_keylog_callback() (line 965) is also surrounded by #if #endif.

Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary


ary = rb_ary_new2(2);
rb_ary_push(ary, ssl_obj);
rb_ary_push(ary, rb_str_new2((const char *)line));
Copy link
Member

Choose a reason for hiding this comment

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

rb_ary_new2() and rb_str_new2() (which BTW are renamed to rb_ary_new_capa() and rb_str_new_cstr() respectively) are unsafe to use here because they can potentially raise a Ruby exception; anything that can raise an exception should be called within rb_protect().

Copy link
Member

Choose a reason for hiding this comment

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

I realized existing code does this. They have to be updated...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads up! It should be addressed in 9f4fa70

}
}
#else
#define ossl_sslctx_keylog_cb rb_f_notimplement
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary


rb_protect(ossl_call_keylog_cb, ary, &state);
if (state) {
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks incomplete? You can use ID_callback_state and the exception will be re-raised later (in ossl_start_ssl()).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I used ID_callback_state as suggested in 9f4fa70

context.keylog_cb = proc do |ary|
cb_called = true
_sock, line = ary
assert_equal(line.split.first, prefix)
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal(expected, actual)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9f4fa70

OpenSSL::SSL::TLS1_VERSION,
OpenSSL::SSL::TLS1_1_VERSION,
OpenSSL::SSL::TLS1_2_VERSION
]
Copy link
Member

Choose a reason for hiding this comment

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

I think testing TLS 1.2 and TLS 1.3 is sufficient. It can make the test code cleaner (no need for ignore_listener_error: true or rescue clauses).

In ruby/openssl's tests, we make the implicit assumption that TLS 1.2 is always supported. You can use tls13_supported? helper method to see if TLS 1.3 is available or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I refactored the tests in 9f4fa70

@rhenium
Copy link
Member

rhenium commented Aug 31, 2022

This adds support to OpenSSL's SSL_CTX_set_keylog_callback().

Thank you for the patch! That would be nice to have.

@cdelafuente-r7
Copy link
Contributor Author

Thank you for your review. I'll address everything soon, I'm a bit I'm tied up right now.

Comment on lines 466 to 470
static VALUE
rb_str_new_cstr_stub(VALUE str)
{
return rb_str_new_cstr((const char *)str);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to use a stub to avoid some compilation warnings when using rb_protect(). Apparently, the first argument has to be a pointer to a function accepting an argument of type VALUE and using rb_str_new_cstr() directly was an issue:

VALUE rb_protect(VALUE (*)(VALUE), VALUE, int*);

Please, let me know if there is better way to do this.

ext/openssl/ossl_ssl.c Outdated Show resolved Hide resolved
* It is only compatible with OpenSSL >= 1.1.1. Even if LibreSSL implements
* SSL_CTX_set_keylog_callback() from v3.4.2, it does nothing (see
* https://github.com/libressl-portable/openbsd/commit/648d39f0f035835d0653342d139883b9661e9cb6).
*/
Copy link
Member

Choose a reason for hiding this comment

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

Please indent this comment block

Comment on lines 2820 to 2818
* context.keylog_cb = proc do |ary|
* _sock, line = ary
* File.open('ssl_keylog_file', "a") do |f|
* f.write("#{line}\n")
* end
* end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* context.keylog_cb = proc do |ary|
* _sock, line = ary
* File.open('ssl_keylog_file', "a") do |f|
* f.write("#{line}\n")
* end
* end
* context.keylog_cb = proc do |_sock, line|
* File.open("ssl_keylog_file", "a") do |f|
* f.puts(line)
* end
* end

assert_not_nil(prefixes.delete(line.split.first))
end

start_server(ignore_listener_error: true) do |port|
Copy link
Member

Choose a reason for hiding this comment

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

ignore_listener_error: true is not needed anymore

@cdelafuente-r7
Copy link
Contributor Author

Thanks @rhenium for the examples and guidance. I chose the temporary struct, since I believe it is easier to read. I also addressed the other issues. Please, let me know if there is anything else to change. Thanks!

Copy link
Member

@rhenium rhenium left a comment

Choose a reason for hiding this comment

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

We can use a little cleanup (I added an inline comment), but this otherwise looks good to me. Thanks!

if (state) {
rb_ivar_set(ssl_obj, ID_callback_state, INT2NUM(state));
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

You can let ossl_call_keylog_cb do rb_str_new_cstr() inside it. That way we can remove rb_str_new_cstr_stub() and need just one rb_protect() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely! Thanks for the heads up! I made the changes in 9f99b9e


ssl_obj = (VALUE)SSL_get_ex_data(ssl, ossl_ssl_ex_ptr_idx);
args.ssl_obj = ssl_obj;
args.line = (VALUE)line;
Copy link
Member

Choose a reason for hiding this comment

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

One last thing, we can turn struct ossl_call_keylog_cb_args.line into a const char * to remove unnecessary casts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! It makes sense. I changed this in 97d44f8.

- This callback is invoked when TLS key material is generated or
  received, in order to allow applications to store this keying material
  for debugging purposes.
- It is invoked with an `SSLSocket` and a string containing the key
  material in the format used by NSS for its SSLKEYLOGFILE debugging
  output.
- This commit adds the Ruby binding `keylog_cb` and the related tests
- It is only compatible with OpenSSL >= 1.1.1. Even if LibreSSL implements
  `SSL_CTX_set_keylog_callback()` from v3.4.2, it does nothing (see
  libressl/openbsd@648d39f)
@rhenium
Copy link
Member

rhenium commented Sep 20, 2022

Squashed commits

@rhenium rhenium merged commit 173be66 into ruby:master Sep 20, 2022
@rhenium
Copy link
Member

rhenium commented Sep 20, 2022

Thank you!

@cdelafuente-r7
Copy link
Contributor Author

Thank you for landing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants