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

Uncomment algorithms for jwt. #3589

Closed
wants to merge 3 commits into from
Closed

Uncomment algorithms for jwt. #3589

wants to merge 3 commits into from

Conversation

kepkin
Copy link

@kepkin kepkin commented Jul 4, 2018

Summary

Uncommented HS384 & HS512 algorithms for jwt

Full changelog

  • Uncomment HS384 & HS512
  • Add related tests

@kepkin
Copy link
Author

kepkin commented Jul 5, 2018

Can somebody restart travis build? It looks like a random timeout error.

[ RUN      ] spec/03-plugins/01-tcp-log/01-tcp-log_spec.lua @ 106: Plugin: tcp-log (log) [#postgres] performs a TLS handshake on the remote TCP server
Error from thread: ./spec/helpers.lua:449: timeout
stack traceback:
	[C]: in function 'assert'
	./spec/helpers.lua:449: in function <./spec/helpers.lua:442>
spec/03-plugins/01-tcp-log/01-tcp-log_spec.lua:121: Expected objects to be the same.
Passed in:
(boolean) false
Expected:
(boolean) true
stack traceback:
	spec/03-plugins/01-tcp-log/01-tcp-log_spec.lua:121: in function <spec/03-plugins/01-tcp-log/01-tcp-log_spec.lua:106>
[  FAILED  ] spec/03-plugins/01-tcp-log/01-tcp-l

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Thank you @kepkin for the contribution! Looking forward to a few changes in the tests (thanks for providing them), and this should be good to go. Would you mind taking the time to address the comments? Thanks!

sub = "1234567890"
}, "secret", 'HS384')

if helpers.openresty_ver_num < 11123 then
Copy link
Member

Choose a reason for hiding this comment

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

Since 0.14.0 (now in the master branch), we only support OpenResty 1.13.2.6 and above, so we do not need these branches anymore. Our CI also runs with recent OpenResty version(s) as well: https://github.com/Kong/kong/blob/master/.travis.yml#L28-L29

5MCJ9.fAk3ps-s7_vQk6XpiVq5GiNjZ__cMW37kE9MEQoR6MwX
ELTFtSIoNOpmhOe_wnCS
]], true), token)
assert.equal('not', 'implemented')
Copy link
Member

Choose a reason for hiding this comment

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

If the feature is supposedly not implemented on some older dependency version (although I am not sure why it wouldn't), then isn't the assertion doomed to fail?

Also, 'not' and 'implemented' clearly aren't equal, so the second assertion doesn't really make any sense? If anything, jwt_parser.encode() will throw an error, not return one.

This while branch goes away anyway since we only support OpenResty 1.13.6.2 and above from now on :)

name = "John Doe",
admin = true,
sub = "1234567890"
}, "secret", 'HS384')
Copy link
Member

Choose a reason for hiding this comment

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

We consistently use double quotes throughout this codebase, will you please convert these single ones? There are other similar places in this patch. Thanks!

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/please review labels Jul 6, 2018
@thibaultcha
Copy link
Member

@kepkin Hi there,

Any chance you could fix the few comments on this PR? Thanks!

@kepkin
Copy link
Author

kepkin commented Aug 6, 2018

Hi @thibaultcha, sorry for the delay. Was busy with other tasks :-)

@thibaultcha
Copy link
Member

@kepkin No worries. Will you also be able to provide a few integration tests? See how 4e2e1d7#diff-7169eb138e87d0233774868e385f8dcdR272 added a couple of tests - we should do the same for these new algorithms and make sure that their implementation works end-to-end. Will you have some time to do this? The tests will likely be very similar to the ones I linked you to, and can simply be adapted.

@kepkin
Copy link
Author

kepkin commented Aug 7, 2018

@thibaultcha thank you for pointing out these tests. It required some fixes.

@thibaultcha
Copy link
Member

@kepkin Very nice! Thank you for taking care of the tests. In order for us to merge your feature, we would also need an update to our documentation: https://github.com/Kong/docs.konghq.com

Would you be able to open a PR there (against the master branch is fine, and we'll rebase it appropriately for the next release).

Thank you!

@thibaultcha thibaultcha added pr/needs docs and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Aug 9, 2018
@kepkin kepkin mentioned this pull request Aug 13, 2018
4 tasks
@kepkin
Copy link
Author

kepkin commented Aug 13, 2018

Looks like there is not much to add
Kong/docs.konghq.com#818

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Aug 13, 2018
thibaultcha pushed a commit that referenced this pull request Aug 13, 2018
@thibaultcha
Copy link
Member

@kepkin Thanks for the docs PR! This is merged as well to master with minor edits.

Feel free to reach out to us if you wish to receive a Contributor T-shirt 😉

thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Aug 21, 2018
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Aug 21, 2018
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.

2 participants