Add RSASSA-PSS signature signing support #285

oliver-hohn · 2018-09-10T15:10:30Z

Changes

Adds support for PS256, PS384, and PS512 signing algorithms. Note As can be seen from the changes in the gemspec, a requirement for OpenSSL +2.1 is made, as older versions do not include the native support for RSASSA-PSS. We understand that there can be pushback on this requirement, and are thus open to ways of making the dependency optional or removing it and leaving it up to the client of the gem to handle this dependecy.
Updates README with examples of PS* signing.
Adds specs for PS* signing of JWTs.

…g JWTs

…lude RSASSA-PSS support

…t's own variables.

sourcelevel-bot · 2018-09-10T15:11:00Z

lib/jwt/security_utils.rb

@@ -20,6 +20,12 @@ def verify_rsa(algorithm, public_key, signing_input, signature)
      public_key.verify(OpenSSL::Digest.new(algorithm.sub('RS', 'sha')), signature, signing_input)
    end

+    def verify_ps(algorithm, public_key, signing_input, signature)


JWT::SecurityUtils#verify_ps has 4 parameters

Read more about it here.

sourcelevel-bot · 2018-09-10T15:11:01Z

lib/jwt/algos/ps.rb

@@ -0,0 +1,25 @@
+module JWT
+  module Algos
+    module Ps


JWT::Algos::Ps has no descriptive comment

Read more about it here.

tobypinder · 2018-09-10T15:37:23Z

Hi folks,

A little context on this. Credit Kudos (the company myself and @oliver-hohn work for) are one of many working in the Open Banking space. This set of specifications is aligned to the OpenID foundation's FAPI standards, which is a structured international means of communicating with a bank.

As part of FAPI R&W section 8.6 - JWS algorithm considerations

Both clients and authorisation servers:

shall use PS256 or ES256 algorithms;

should not use algorithms that use RSASSA-PKCS1-v1_5 (e.g. RS256);

shall not use none;

As these are requirements from the standards body and as for various technical reasons many banks are unwilling to adopt ES256, there is a fairly urgent need for industry-wide PS256 support.

Myself and @oliver-hohn recognise the shortcomings of an approach dependent on OpenSSL in this manner. Our aim with raising this PR is to start the conversation and signal our intent to help in this area. If OpenSSL is to be used, we defer to the project maintainers to discuss how best to introduce the openssl gem as an optional dependency and not break Ruby 2.2 support (as in the Travis build). If we are happy to proceed with an OpenSSL based "progressive enhancement" that does not break implementations without OpenSSL or with incompatible ruby versions, we will be happy to introduce those changes prior to a merge. Or if a more integrated approach is viable, we will assist where possible.

excpt · 2018-09-10T16:35:52Z

@oliver-hohn, @tobypinder

Thank you for the great contribution.

Ruby 2.2 reached end of life this year. (Source)

I think it is fine to drop ruby 2.2 support entirely.

tobypinder · 2018-09-11T10:39:37Z

Thanks for your response.

We will probably workshop this into a format where the openssl integration is optional in this case, and add documentation for how to add it to gain PS functionality. There's probably environments where for whatever reason that kind of integration is undesirable - I'd presume we want to ensure that all existing ruby-jwt users maintain existing access and that this PS support is seen as a progressive enhancement where users can support it.

On the ebert issues - Documenting the classes is something we can certainly introduce but the method signature issues seem to come from existing design patterns in the RS classes - is this something we should look at in this PR or are we OK to leave it in the same format for now?

excpt · 2018-09-11T11:14:37Z

@tobypinder That sounds great. An optional dependency would be the best way to tackle the openssl version issue.

Currently we have an optional dependency on EDDSA algos. You need the RbNaCl library to use them. You maybe can have a look a this too.

Please ignore the ebert issues I'm currently (re)tuning the linter settings.

…* algorithms.

sourcelevel-bot · 2018-09-11T17:12:29Z

lib/jwt/error.rb

-  DecodeError = Class.new(StandardError)
+  EncodeError      = Class.new(StandardError)
+  DecodeError      = Class.new(StandardError)
+  RequiredGemError = Class.new(StandardError)


JWT::RequiredGemError has no descriptive comment

Read more about it here.

sourcelevel-bot · 2018-09-11T17:12:29Z

lib/jwt/error.rb

@@ -1,8 +1,9 @@
 # frozen_string_literal: true

 module JWT
-  EncodeError = Class.new(StandardError)
-  DecodeError = Class.new(StandardError)
+  EncodeError      = Class.new(StandardError)


JWT::EncodeError has no descriptive comment

Read more about it here.

sourcelevel-bot · 2018-09-11T17:12:29Z

lib/jwt/error.rb

-  EncodeError = Class.new(StandardError)
-  DecodeError = Class.new(StandardError)
+  EncodeError      = Class.new(StandardError)
+  DecodeError      = Class.new(StandardError)


JWT::DecodeError has no descriptive comment

Read more about it here.

sourcelevel-bot · 2018-09-11T17:12:29Z

lib/jwt/algos/ps.rb

+
+      SUPPORTED = %w[PS256 PS384 PS512].freeze
+
+      def sign(to_sign)


JWT::Algos::Ps#sign has approx 6 statements

Read more about it here.

sourcelevel-bot · 2018-09-11T17:12:29Z

lib/jwt/algos/ps.rb

+      end
+
+      def require_openssl!
+        unless Gem.loaded_specs['openssl'] && Gem.loaded_specs['openssl'].version.release >= Gem::Version.new('2.1')


JWT::Algos::Ps#require_openssl! calls 'Gem.loaded_specs['openssl']' 2 times

Read more about it here.

sourcelevel-bot · 2018-09-11T17:12:29Z

lib/jwt/algos/ps.rb

+      end
+
+      def require_openssl!
+        unless Gem.loaded_specs['openssl'] && Gem.loaded_specs['openssl'].version.release >= Gem::Version.new('2.1')


JWT::Algos::Ps#require_openssl! calls 'Gem.loaded_specs' 2 times

Read more about it here.

sourcelevel-bot · 2018-09-11T17:15:18Z

Ebert has finished reviewing this Pull Request and has found:

5 possible new issues (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/jwt/ruby-jwt/pulls/285.

oliver-hohn · 2018-09-11T17:22:23Z

@excpt, I've just pushed a few code style changes and made OpenSSL an optional dependency for the gem. However it is still required when signing when using RSASSA-PSS algorithms (see https://github.com/jwt/ruby-jwt/pull/285/files#diff-089b700b34a76152d9a779083b1112a9R30).

It seems that Ebert is still not happy with the code style changes. As you mentioned that you are revising the linter settings, could you mark which code style changes you want to be made and which can be omitted, so that I can make the corrections in the PR?

For the EDDSA algos, we were thinking of making similar changes to it's dependency management in a separate PR.

jtara · 2018-09-11T17:49:26Z

I'd like to add a couple of cents here on behalf of those of us who use this library in Ruby environments that do not support Gems. I am one - I use this library specifically (as opposed to https://github.com/nov/json-jwt) in a hybrid mobile application built with the Rhodes framework. Rhodes has an embedded Ruby 2.4.1, and allows mobile development to be done in Ruby.

In this environment, it is possible to use many pure-Ruby Gems, so long as they are compatible with the environment. One simply extracts the /lib directory, which is then placed under an "extensions" directory in the project.

It's even possible to adapt Gems that have some native C code, with a little bit of work writing a build file.

This library is especially easy to use in the Rhodes environment, because (currently) it is both pure Ruby, and has no external dependencies. (So, I don't have to check every one of a laundry list of dependencies for compatibility.)

FWIW, we do have OpenSSL (have to check version) in this environment. What we don't have is support for Gems, as it would make no sense in this environment. (To satisfy Apple requirements on interpretive languages, the ruby code is compiled to MRI bytecode at build time). Loading Gems in this environment is a nonsensical thing. So, I can load OpenSSL. I don't have Gem, and OpenSSL isn't loaded as a Gem.

I'd like to ask that some consideration be made toward this usage. Maybe it is no more than providing some documentation on how to stub-off the calls to Gem.whatever, or perhaps providing the stubs.

It would be a shame for a library that is currently so robustly stand-alone to now start acquiring Gem baggage.

FWIW, in my server environment, I do use the "other" library I mentioned above, because it's already a requirement for the openid-connect Gem I use there. I use THIS library in the mobile client environment specifically because of it's simple purity which makes it easy to use in that environment.

P.S. I think that rather than directly stubbing out any calls on the Gem module, it would be better to abstract it. I'd rather be overriding some methods in this library, than providing a "fake" Gem module.

oliver-hohn · 2018-09-12T08:50:31Z

@jtara Am I correct in understanding that the issue is with the usage of the rubygems library that provides Gem#loaded_specs?
Apologies if I misunderstood, but from my understanding rubygems comes with Ruby since version 1.9 (see https://www.ruby-lang.org/en/libraries/), and as ruby-jwt supports Ruby 2.2 upwards, I would have thought that using Gem#loaded_specs provided by rubygems would be safe.

Another solution for this, so as to not rely on the rubygems library being present, would be that verify_ssl! would require openssl and catch the LoadError exception, i.e.

# in algos/ps.rb
def require_openssl!
  require 'openssl'
rescue LoadError
  raise JWT::RequiredGemError, 'OpenSSL +2.1 is required to support RSASSA-PSS algorithms'
end

And then similarly in the #sign and #verify methods to catch NoMethodErrors when trying to call sign_pss and verify_ps, i.e.

# in algos/ps.rb
def sign(to_sign)
  ...
  begin
    key.sign_pss(translated_algorithm, msg, salt_length: :max, mgf1_hash: translated_algorithm)
  rescue NoMethodError
      raise JWT::RequiredGemError, 'OpenSSL +2.1 is required to support RSASSA-PSS algorithms'
  end
end

# in security_utils.rb
def verify_ps(algorithm, public_key, signing_input, signature)
  ...
  begin
    public_key.verify_pss(formatted_algorithm, signature, signing_input, salt_length: :auto, mgf1_hash: formatted_algorithm)
  rescue NoMethodError
          raise JWT::RequiredGemError, 'OpenSSL +2.1 is required to support RSASSA-PSS algorithms'
  end
end

Thoughts on this approach?

jtara · 2018-09-12T20:08:25Z

@oliver-hohn rescue NoMethodError looks like a good approach to me, as it keep the library dependency-free (with OPTIONAL OpenSSL dependency) and no rubygems dependency.

FYI, Ruby in Rhodes is a specialized environment, and omits RubyGems. It also omits (most uses of) eval(), and the ability to load .rb files at run-time. There is a build environment, which is a standard Ruby desktop environment that does indeed include RubyGems (just standard distributions). During build, .rb files are compiled to ruby bytecode in .iseq files. require is overridden to call require_compiled.

This is partly for efficiently, but more importantly to satisfy Apple requirements for apps in the App Store that use interpretive languages (other than Javascript running in either UIWebView or WKWebView. It is not allowed to use an interpretive language implement that permits source code to be loaded in the execution environment.

So, the mobile app includes a modified build of Ruby MRI that is tailored to the requirements of the execution environment.

It is similar to how precompiling ruby works on Heroku presented here:

http://www.atdot.net/~ko1/activities/2016_railsconf_pub.pdf

with the additional caveat that it is not possible to compile code in the execution environment. What Heroku does during "boot" time is done instead at "build" time.

Rhodes is not a well-publicized mobile platform, but is very much quietly in use by a number of large enterprises.

http://tau-technologies.com/products/rhomobile/

I am not a user of RubyMotion, but it works similarly, and for the same reasons.

http://www.rubymotion.com/

There are probably additional Ruby environments that do not permit compilation in the execution environment.

I think it is valuable to maintain the ability to use ruby-jwt in these and similar environments (without having to fork a small change), and only takes the small difference in approach that you have proposed above.

tobypinder · 2018-09-13T09:33:58Z

@jtara the feedback and explanations of these use cases is really helpful, we're keen to introduce this in a way that works for everyone.

…ire OpenSSL +2.1. Replaces RequiredGemError with RequiredDependencyError

oliver-hohn · 2018-09-13T17:06:36Z

lib/jwt/algos/ps.rb

+      end
+
+      def require_openssl!
+        if Object.const_defined?('OpenSSL')


@jtara Thoughts on this approach of using Object#const_defined? over catching NoMethodErrors?

I like this belt-and-suspenders approach even better.

It uses a check for constant def rather than using a Gem function

It's ALSO easy to monkey-patch for those who have some other special requirement. While in MY use case, OpenSSL would have already been loaded, perhaps others need to load dynamically, so they can monkey-patch.

I will ask the Rhodes maintainers to comment as well.

Sorry, I see you already had the belt-and-suspenders (def require_openssl! method) in the previous proposed solution as well.

I've asked the Rhodes developers to comment on the two two proposals.

Thanks for your willingness to keep the library as pure as possible to make it easily usable in strange execution environments!

I have this preliminary feedback from the Rhodes team:

Tau Technologies Inc. Wednesday at 16:20
Jon,

Team is investigating what preference is more suitable for Rhodes.
We'll post an update to ruby-jwt github issue thread as we get results.

Regards,
Tau Technologies Team.

Tau Technologies Inc. Thursday at 16:54
Jon,

We assume the both methods are well.

What about another possible approach on the application level: to create stub Gem object with stub method loaded_specs?

Regards,
Tau Technologies Team.

Thanks for taking this into consideration. Of course, they are right, Gem object could be stubbed in application. But it invites further creeping of dependencies. Since the library is currently dependency-free, it's a worthy goal, I think, to keep it that way, i think we are agreed.

I think we are good with whatever method you deem best, so please don't hold anything up on account of this.

oliver-hohn · 2018-09-25T09:12:33Z

@excpt Since there has been inactivity in the past days on this PR, can it be reviewed for merging? If the Rhodes maintainers require changes to be made, I suggest they are introduced in a separate PR.

excpt · 2018-09-26T09:12:05Z

@oliver-hohn I start reviewing the code and merge the PR when I am done.

@oliver-hohn, @tobypinder, @jtara
Thank you very much for all the time and effort improving this project!

oliver-hohn · 2019-02-13T11:18:33Z

@excpt Is there any plans for a release in the near future? We are having to fork until the next release for our purposes.

excpt · 2019-02-13T12:56:03Z

Release is in the works. Will happen on the weekend.

…

On Wed 13. Feb 2019 at 12:18, Oliver Höhn ***@***.***> wrote: @excpt <https://github.com/excpt> Is there any plans for a release in the near future? We are having to fork until the next release for our purposes. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#285 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOBmrtMNaQAw40B5tVUCKWtiypqRxQjks5vM_SKgaJpZM4WhmmE> .

Oliver added 5 commits September 10, 2018 15:28

Adds PS* (RSA with probabilistic signature scheme) support for signin…

da2ac2d

…g JWTs

Updates README with examples for PS* supported signing algorithm

598be5c

Adds specs for PS* signing algorithm

fa92222

Sets OpenSSL dependency, as versions before OpenSSL 2.1.0 did not inc…

0979f73

…lude RSASSA-PSS support

Refactors repeated code for algorithm translation and class type to i…

5d841fa

…t's own variables.

sourcelevel-bot bot reviewed Sep 10, 2018

View reviewed changes

excpt self-requested a review September 10, 2018 16:24

excpt self-assigned this Sep 10, 2018

excpt added enhancement documentation feature discussion security labels Sep 10, 2018

excpt added this to the Version 2.2.0 milestone Sep 10, 2018

Makes OpenSSL +2.1 an optional gem, however it is required for the PS…

c14d70b

…* algorithms.

sourcelevel-bot bot reviewed Sep 11, 2018

View reviewed changes

Refactors "Gem.loaded_specs['openssl']" into it's own variable

1d16558

Replaces usage of Gem#loaded_specs with Object#const_defined? to requ…

df315b5

…ire OpenSSL +2.1. Replaces RequiredGemError with RequiredDependencyError

oliver-hohn commented Sep 13, 2018

View reviewed changes

excpt added the WIP label Sep 13, 2018

excpt approved these changes Sep 27, 2018

View reviewed changes

excpt merged commit f60d78a into jwt:master Sep 27, 2018

tobypinder mentioned this pull request Mar 8, 2019

Fix Salt length for conformance with PS family specification. #300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RSASSA-PSS signature signing support #285

Add RSASSA-PSS signature signing support #285

oliver-hohn commented Sep 10, 2018

sourcelevel-bot bot Sep 10, 2018

sourcelevel-bot bot Sep 10, 2018

tobypinder commented Sep 10, 2018 •

edited

Loading

excpt commented Sep 10, 2018

tobypinder commented Sep 11, 2018

excpt commented Sep 11, 2018

sourcelevel-bot bot Sep 11, 2018

sourcelevel-bot bot Sep 11, 2018

sourcelevel-bot bot Sep 11, 2018

sourcelevel-bot bot Sep 11, 2018

sourcelevel-bot bot Sep 11, 2018

sourcelevel-bot bot Sep 11, 2018

sourcelevel-bot bot commented Sep 11, 2018

oliver-hohn commented Sep 11, 2018

jtara commented Sep 11, 2018 •

edited

Loading

oliver-hohn commented Sep 12, 2018 •

edited

Loading

jtara commented Sep 12, 2018 •

edited

Loading

tobypinder commented Sep 13, 2018

oliver-hohn Sep 13, 2018 •

edited

Loading

jtara Sep 19, 2018

jtara Sep 19, 2018

jtara Sep 25, 2018 •

edited

Loading

oliver-hohn commented Sep 25, 2018 •

edited

Loading

excpt commented Sep 26, 2018

oliver-hohn commented Feb 13, 2019

excpt commented Feb 13, 2019 via email

Add RSASSA-PSS signature signing support #285

Add RSASSA-PSS signature signing support #285

Conversation

oliver-hohn commented Sep 10, 2018

Changes

sourcelevel-bot bot Sep 10, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 10, 2018

Choose a reason for hiding this comment

tobypinder commented Sep 10, 2018 • edited Loading

excpt commented Sep 10, 2018

tobypinder commented Sep 11, 2018

excpt commented Sep 11, 2018

sourcelevel-bot bot Sep 11, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 11, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 11, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 11, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 11, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot Sep 11, 2018

Choose a reason for hiding this comment

sourcelevel-bot bot commented Sep 11, 2018

oliver-hohn commented Sep 11, 2018

jtara commented Sep 11, 2018 • edited Loading

oliver-hohn commented Sep 12, 2018 • edited Loading

jtara commented Sep 12, 2018 • edited Loading

tobypinder commented Sep 13, 2018

oliver-hohn Sep 13, 2018 • edited Loading

Choose a reason for hiding this comment

jtara Sep 19, 2018

Choose a reason for hiding this comment

jtara Sep 19, 2018

Choose a reason for hiding this comment

jtara Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

oliver-hohn commented Sep 25, 2018 • edited Loading

excpt commented Sep 26, 2018

oliver-hohn commented Feb 13, 2019

excpt commented Feb 13, 2019 via email

tobypinder commented Sep 10, 2018 •

edited

Loading

jtara commented Sep 11, 2018 •

edited

Loading

oliver-hohn commented Sep 12, 2018 •

edited

Loading

jtara commented Sep 12, 2018 •

edited

Loading

oliver-hohn Sep 13, 2018 •

edited

Loading

jtara Sep 25, 2018 •

edited

Loading

oliver-hohn commented Sep 25, 2018 •

edited

Loading