Skip to content

Comments

added token date checks & signature is now forced to lowercase hex before validation.#9

Open
mhassman wants to merge 2 commits intoOlivine-Labs:masterfrom
mhassman:master
Open

added token date checks & signature is now forced to lowercase hex before validation.#9
mhassman wants to merge 2 commits intoOlivine-Labs:masterfrom
mhassman:master

Conversation

@mhassman
Copy link

For some reason, i couldn't get token verification to work as-is..
signature = basexx.from_url64(str:sub(dotSecond+1)) was returning binary, but
['HS256'] = function(data, signature, key) return signature == tohex(hmac.new (key, 'sha256'):final (data)) end, was returning lowercase hex.
So, forced extracted message signature to lowercase hex.

Also, added token date (nbf, and exp) checks if they are defined within the token.

Hope this helps..

- signature is now forced to lowercase hex before validation.
@DorianGray
Copy link
Member

Would you mind adding some tests that describe the behavior and fail with the old code and then pass with the new?

- added check - if json decode fails, do not continue and propagate error to caller.
@mhassman
Copy link
Author

Hi Robert,

Apologies on delayed response - crazy week.

I'm happy to help, but not quite understanding what you're asking. Are you
asking to enhance the code for robustness? or something else?

fyi.. i committed a minor change that permits errors in json decode to flow
upward so they can be handled by the calling lua code. otherwise, nginx
fails with 500 error. I don't know if i should do another pull request? or
if you will see the second commit? Please advise..

Thnx!
:-)

-Mark


From: Robert Andrew Ditthardt [mailto:notifications@github.com]
Sent: Monday, June 13, 2016 8:06 PM
To: Olivine-Labs/lua-jwt
Cc: mhassman; Author
Subject: Re: [Olivine-Labs/lua-jwt] added token date checks & signature is
now forced to lowercase hex before validation. (#9)

Would you mind adding some tests that describe the behavior and fail with
the old code and then pass with the new?

You are receiving this because you authored the thread.
Reply to this email directly, view
#9 (comment) it
on GitHub, or mute
<https://github.com/notifications/unsubscribe/AA37PCEf8UXue0FbDzUxKGyKj2q0Za
qyks5qLfCAgaJpZM4I0FPn> the thread.
<https://github.com/notifications/beacon/AA37PKfPPdqjvjmpTOkdxZOttpkvLK8Iks5
qLfCAgaJpZM4I0FPn.gif>

@DorianGray
Copy link
Member

Specifically I'm asking for tests to be written in the spec folder so that running busted confirms that this code works. There are existing tests over all of the functionality in this library.

@moteus
Copy link
Contributor

moteus commented Mar 16, 2017

This PR add nginx as dependency.
I think better use os module for that.

@moteus
Copy link
Contributor

moteus commented Apr 11, 2017

I have idea to add options.checks table which will allows provide some arbitrary values (and may be functions). E.g. it can provide current timestamp to checks nbf and exp. Also e.g. server name to check iss.

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.

3 participants