Skip to content

Add support for ruby-jwt v2.6.0 and above, bump development deps, new Ruby vers #91

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

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

oskarpearson
Copy link
Contributor

@oskarpearson oskarpearson commented Jan 29, 2025

Description

  • Remove reference to 'secure_compare' method in ruby-jwt that this code shouldn't be using, so as to support ruby-jwt v2.6.0 and above
  • Add support for ruby-jwt up to and including version 3.* (currently in beta) so that we don't have to bump support again later
  • Add recent Ruby versions to the test matrix (I left version 3.0 in place, although it's no longer officially supported, so as to avoid inconveniencing existing users that are stuck on version 3.0)

Prior versions of this code depended on the method JWT::SecurityUtils.secure_compare

Release v2.6.0 of ruby-jwt moved the JWT::SecurityUtils.secure_compare method to a different namespace as part of jwt/ruby-jwt#521

While we could simply change this code to instead refer to that new namespace, I believe that the intention of ruby-jwt was that secure_compare should not have been used outside of the internals of ruby-jwt.

This is supported by the The ruby-jwt README examples where the ruby-jwt team recommend using OpenSSL.fixed_length_secure_compare instead of JWT::SecurityUtils.secure_compare

The code in this change is based on the logic in
https://github.com/rails/rails/blob/cf6ff17e9a3c6c1139040b519a341f55f0be16cf/activesupport/lib/active_support/security_utils.rb#L33 so as to avoid adding a dependency on ActiveSupport for this single method.

Unlike the code in the activesupport url above we don't fall back to a custom implementation of fixed_length_secure_compare, since OpenSSL.fixed_length_secure_compare is present in OpenSSL 2.2 and this gem already depends on Ruby 3.0 and above, which already includes that version of OpenSSL

This code also doesn't need to handle nil/empty cases, unlike the original implementation of JWT::SecurityUtils.secure_compare because these are already handled in the call to validate_url in one case, and validate_payload itself in the other.

Changelog for Automated Release

Note: this will be used by release.yml to automatically adjust CHANGELOG and publish the gem.

CHANGELOG

  • [CHANGED] Run tests against newer supported versions of ruby
  • [CHANGED] Replace dependency on JWT::SecurityUtils.secure_compare with OpenSSL.fixed_length_secure_compare
  • [CHANGED] Support recent versions of ruby-jwt

… Ruby vers

- Remove reference to 'secure_compare' method in ruby-jwt that this
  code shouldn't be using, so as to support ruby-jwt v2.6.0 and above
- Add support for ruby-jwt up to and including version 3.* (currently in beta)
  so that we don't have to bump support again later
- Add recent Ruby versions to the test matrix (I left version 3.0 in place,
  although it's no longer officially supported, so as to avoid inconveniencing
  existing users that are stuck on version 3.0)

Prior versions of this code depended on the method `JWT::SecurityUtils.secure_compare`

Release v2.6.0 of ruby-jwt moved the `JWT::SecurityUtils.secure_compare` method to
a different namespace as part of jwt/ruby-jwt#521

While we could simply change this code to instead refer to that new namespace,
I believe that the intention of ruby-jwt was that `secure_compare` should not
have been used outside of the internals of ruby-jwt.

This is supported by the [The ruby-jwt README examples](https://github.com/jwt/ruby-jwt/blob/v3.0.0.beta1/README.md#custom-algorithms)
where the ruby-jwt team recommend using `OpenSSL.fixed_length_secure_compare`
instead of `JWT::SecurityUtils.secure_compare`

The code in this change is based on the logic in
https://github.com/rails/rails/blob/cf6ff17e9a3c6c1139040b519a341f55f0be16cf/activesupport/lib/active_support/security_utils.rb#L33
so as to avoid adding a dependency on ActiveSupport for this single method.

Unlike the code in the activesupport url above we don't fall back to a custom
implementation of `fixed_length_secure_compare`, since
`OpenSSL.fixed_length_secure_compare` [is present in OpenSSL 2.2](https://github.com/ruby/openssl/blob/master/History.md#version-220)
and this gem already depends on Ruby 3.0 and above, which already
[includes that version of OpenSSL](https://stdgems.org/openssl/)

This code also doesn't need to handle nil/empty cases, unlike the
[original implementation of JWT::SecurityUtils.secure_compare](https://github.com/jwt/ruby-jwt/pull/15/files#diff-2961689829b1ae20b5e8222673335c323355389f753c50b8a5b98380086f63b9R98)
because these are already handled in the call to `validate_url` in one case,
and `validate_payload` itself in the other.
@oskarpearson
Copy link
Contributor Author

A note here on ruby-jwt versions: I've tested this with both the beta version of ruby-jwt and recent versions of ruby-jwt (2.10.1, 2.6, 2.5) and they also work.

I've thus relaxed the constraint on ruby-jwt to allow ruby-jwt 3.* when it's been released, so that we don't need to keep re-pinning the supported version through PRs

@oskarpearson
Copy link
Contributor Author

oskarpearson commented Feb 4, 2025

NOTE to the messagebird reviewers:

PLEASE SET A RELEASE LABEL on this PR before merging. You should be able to find this on the right hand side of the page.

I suggest using release-major to build a new version 5.0 of the gem

Please add the label here:
Screenshot 2025-02-04 at 11 10 01

Why?

If merged correctly, this PR should be able to automatically create Version 5 and publish the new gem

See #92 for a detailed breakdown of the code and process.

The code in release.yml appears to:

  • Get a label from the PR (you'll need to set this)
  • Update CHANGELOG based on the text in my original PR description above
  • Bump the version number and adjust version.rb based on the label chosen
  • Commit CHANGELOG and version.rb changes to the repo

Then, gh-release.yml and release.yml will publish new github and gem releases with these version numbers.

You can see this automation in action with these previous PRs:

# of fixed_length_secure_compare, since OpenSSL.fixed_length_secure_compare is present in OpenSSL 2.2
# https://github.com/ruby/openssl/blob/master/History.md#version-220 which is included in Ruby 3.0 and above
def secure_compare(first, second)
first.bytesize == second.bytesize && OpenSSL.fixed_length_secure_compare(first, second)
Copy link
Contributor

Choose a reason for hiding this comment

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

are we missing

require "openssl"

here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bottom Line Up Front

Super happy to add it to do that and make the requirement explicit, but it works without it because of the below.

I'm aware this PR is already getting quite large - so let me know if you'd like to do this.

How do I know it'll work?

There's existing similar code in signed_request.rb that uses OpenSSL without the need for require 'openssl'

Why does it work?

My understanding is that this is because all recent versions of Ruby's https://ruby-doc.org/stdlib-2.6.1/libdoc/net/http/rdoc/Net/HTTP.html library depend on OpenSSL (search on that page for OpenSSL and you'll see references to OpenSSL specific errors and symbols)

So, by using ruby's net-related code, it loads OpenSSL. The existing codebase explicitly loads net/https (which then, in turn, loads OpenSSL) in a few places.

The most key of these are lib/messagebird.rb loading lib/messagebird/client.rb:

require 'messagebird/client'

That, in turn, then requires net/https:

require 'net/https'

Verification

As a test I ran irb without bundle exec and tried to access the OpenSSL namespace:

irb(main):001> OpenSSL
(irb):1:in `<main>': uninitialized constant OpenSSL (NameError)

OpenSSL
^^^^^^^
        from <internal:kernel>:187:in `loop'
        from .../versions/3.3.7/lib/ruby/gems/3.3.0/gems/irb-1.15.1/exe/irb:9:in `<top (required)>'
        from .../.rbenv/versions/3.3.7/bin/irb:25:in `load'
        from .../.rbenv/versions/3.3.7/bin/irb:25:in `<main>'

I then loaded net/https and it loaded OpenSSL

irb(main):002> require 'net/https'
=> true
irb(main):003> OpenSSL
=> OpenSSL
irb(main):004>

As I mentioned, OpenSSL is being implicitly loaded at the moment. I'm happy to change that, but am aware that either I need to make this code inconsistent with signed_request.rb, or I need to make changes there too, or I need to add a require for OpenSSL in lib/messagebird.rb

Let me know what you think

@marcelcorso
Copy link
Contributor

thank you @oskarpearson for the PR!

I have only 1 small comment

@marcelcorso
Copy link
Contributor

@oskarpearson another small request: please add a unit test 🙏 ?

@oskarpearson
Copy link
Contributor Author

@oskarpearson another small request: please add a unit test 🙏 ?

Thanks for the review, @marcelcorso 🎉

I had a look at this, and encountered a few issues. Wanted to get your opinion on how to proceed.

Unfortunately the code isn't really written in a way that makes it easy to test without lots of mocking or similar tradeoffs.

Option 1 - Add a test for the private secure_compare method

In general I try and avoid testing private methods like secure_compare, and instead focus on the externally-visible behaviour.

Adding a test to ensure secure_compare works as expected would be quite simple and easy, though.

Option 2 - Lots of mocking

If I want to avoid testing the private method, then I need to exercise all parts of the code to ensure that bytesize and OpenSSL.fixed_length_secure_compare are called/not called for the URL and payloads.

This turns out to be "challenging" with the current code. The spec ends up being a mess of interception of methods on JWT and Digest::SHA256 to return mocked objects where I can check that bytesize is run on them.

I've given an example of a single "happy path test" after Option 4 below. Consider how messy this gets if I have to test that bytesize/fixed_length_secure_compare are called for all code paths for all variations of the URL and payload parameters.

Option 3 - Significant refactoring of request_validator.rb to be more testable

Honestly I'd like to avoid this given time constraints. Making it more testable would also likely involve changing it's method signature, which then has knock-on effects on other code, or adding another object under the existing code layer that I can then mock/stub specific methods on.

Option 4 - Don't test all code paths through secure_compare

This is probably my preference.

If I test the 'happy path' where the url and payload are valid and check that the underlying calls to bytesize/OpenSSL.fixed_length_secure_compare are done, it would mean one test that does a significant amount of mocking.

It would still not be very readable, and would likely need to change the mocking if the code in request_validator.rb changes.

Example of 'Happy Path' test

Here's an example of a test of request_validator that ensures bytesize is called for the url and payload values, and that fixed_length_secure_compare is also called for both parameters.

Note that to be able to ensure that .bytesize is called on specific strings (payload_hash and url_hash) I need to mock the request to JWT and ensure it returns something that I've previously been able to set an expectation on. Otherwise JWT constructs its own strings (with their own object IDs) and the rspec expect doesn't trigger.

Similarly, to be able to ensure that .bytesize is called on the output of Digest::SHA256.hexdigest I need to mock those too.

You can see how this starts to get very long if I'm trying to exercise all the codepaths of both MessageBird::RequestValidator#validate_url and MessageBird::RequestValidator#validate_payload as per option 2 above

  describe 'secure string comparison' do
    subject { MessageBird::RequestValidator.new('test_signature_key') }

    let(:url) { 'https://example.com/?example=42' }
    let(:payload) { 'test_payload' }

    let!(:url_hash) { Digest::SHA256.hexdigest(url) }
    let!(:payload_hash) { Digest::SHA256.hexdigest(payload) }

    let(:mocked_jwt_decoded_response) do
      {
        'payload_hash' => payload_hash.dup,
        'url_hash' => url_hash.dup
      }
    end

    it 'compares hashes using secure_compare' do
      expect(JWT).to receive(:decode).with(any_args).and_return(mocked_jwt_decoded_response)

      expect(Digest::SHA256).to receive(:hexdigest).with(url).and_return(url_hash)
      expect(url_hash).to receive(:bytesize).and_call_original
      expect(mocked_jwt_decoded_response['url_hash']).to receive(:bytesize).and_call_original
      expect(OpenSSL).to receive(:fixed_length_secure_compare).with(
        url_hash,
        mocked_jwt_decoded_response['url_hash']
      ).and_call_original

      expect(Digest::SHA256).to receive(:hexdigest).with(payload).and_return(payload_hash)
      expect(payload_hash).to receive(:bytesize).and_call_original
      expect(mocked_jwt_decoded_response['payload_hash']).to receive(:bytesize).and_call_original

      expect(OpenSSL).to receive(:fixed_length_secure_compare).with(
        payload_hash,
        mocked_jwt_decoded_response['payload_hash']
      ).and_call_original

      subject.validate_signature('irrelevant_as_mocked', url, payload)
    end
  end

@oskarpearson
Copy link
Contributor Author

@marcelcorso Wanted to check in here - we're running this in production without any incidents. Could you let me know what approach you'd like me to take with the tests?

@marcelcorso
Copy link
Contributor

hey @oskarpearson ! It all looks good. We'll merge and release ~tomorrow.

Thank you so much 🙏

@marcelcorso marcelcorso merged commit 8c09d21 into messagebird:master Apr 9, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants