Rescue RbNaCl exception for EdDSA wrong key #491
Conversation
|
Hello, @n-studio! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
![sourcelevel-bot[bot]](https://avatars.githubusercontent.com/in/597?s=60&v=4){kind=small}
ruby-jwt.gemspec
Outdated
| @@ -29,6 +29,7 @@ Gem::Specification.new do |spec| | |||
| spec.add_development_dependency 'appraisal' | |||
| spec.add_development_dependency 'bundler' | |||
| spec.add_development_dependency 'rake' | |||
| spec.add_development_dependency 'rbnacl' | |||
There was a problem hiding this comment.
Would rather not have the rbnacl dependency as a default dependency, as including this will require some other libs to be installed locally also.
Also including this will change the behaviour for some functionality.
The combination of different dependencies are tested using Appraisal and this set of gemfiles.
There was a problem hiding this comment.
To run tests with the rbnacl dependency, you can either run everything using bundle exec appraisal or for only the rbnacl dependency:
BUNDLE_GEMFILE=gemfiles/rbnacl.gemfile bundle install
BUNDLE_GEMFILE=gemfiles/rbnacl.gemfile bundle exec rspec
| @@ -65,6 +65,24 @@ | |||
| ] | |||
| end | |||
|
|
|||
| it 'EDDSA' do | |||
There was a problem hiding this comment.
The readme_examples_spec.rb is dedicated to tests for the examples in the readme file. I think the test (with some good context/explanation) could be in the block for rbnacl tests
There was a problem hiding this comment.
As an addition to this keyfinder with multiple keys a more simple test could be
context 'when wrong ED25519 key is used to verify signature' do
it 'raises a ::JWT:DecodeError' do
token = JWT.encode(payload, RbNaCl::Signatures::Ed25519::SigningKey.generate, 'ED25519')
expect { JWT.decode(token, RbNaCl::Signatures::Ed25519::SigningKey.generate.verify_key, true, algorithm: 'ED25519') }.to raise_error(::JWT::DecodeError)
end
end
|
This looks great, good catch. It seems that the signature verification is leaking the RbNaCl errors when the signature is invalid. Think it's a more generic issue than the key rotation issue. Added a few comments about the changes. |
|
@anakinj Thank you for the review! I updated my code accordingly. |
n-studio
changed the title
| @@ -218,7 +218,7 @@ | |||
|
|
|||
| it 'wrong key should raise JWT::DecodeError' do | |||
| expect do | |||
| JWT.decode data[alg], wrong_key | |||
| JWT.decode data[alg], wrong_key, true, algorithm: alg | |||
There was a problem hiding this comment.
Oh, this was a good catch, the test has been wrong all the time, right. Like passing for the wrong reasons.
|
This is great stuff. Thank you for the fix. |
Fixes #490
Instead of raising an error when a public key is invalid, we should return
falseso the key finder can test the next key.