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

Signature fix #85

Closed
wants to merge 3 commits into from
Closed

Signature fix #85

wants to merge 3 commits into from

Conversation

stuartm
Copy link

@stuartm stuartm commented Feb 29, 2016

This fixes signature creation, the presently generated signature doesn't comply with the RFC and is rejected by other applications/libraries. The reason is that the signature is base64 encoded as a string instead of a hexidecimal representation of octets.

Convert signature to binary before base64 encode results in the expected signature as verified against the example in RFC 7515 and the http://jwt.io debugger.

Results may be validated with:
echo -n "base64_encoded_header.base64_encoded_payload" | openssl dgst -sha256 -hmac "your_secret" -binary | base64 | tr -- '+=/' '- _'

The signatures generated by JWS are invalid because it base64 encodes the
signature as a string instead of a binary value, converting to
binary first produces the correct signature value. Verified with
example from JWS RFC and the debugger at http://jwt.io

Results may be validated with

echo -n "base64_header.base64_payload" | openssl dgst -sha256 -hmac
"your_secret" -binary | base64 | tr -- '+=/' '- _'
…se64 encode."

This reverts commit eb5a6889e058930003b1f787dc3b39ce59023d4f.
@odino
Copy link
Contributor

odino commented Mar 1, 2016

hey @stuartm thanks for this! Could you take a look at the tests on travis?

cheers!

@stuartm
Copy link
Author

stuartm commented Mar 1, 2016

Yes, I saw those, I'll update the pull request with a fix when I get a chance.

@gboor
Copy link

gboor commented Mar 17, 2016

I needed this fix and it works fine for me! I do not understand why the tests fail with "malformed input". It would be really great to have this in the master branch, so I can stop hacking the vendor files on every composer update...

@stuartm
Copy link
Author

stuartm commented Mar 17, 2016

I know why the tests are failing, I'll try to find time in the next couple of days to update the pull request.

@odino
Copy link
Contributor

odino commented Mar 23, 2016

thanks man :)

On Thu, Mar 17, 2016 at 6:23 PM, Stuart Morgan notifications@github.com
wrote:

I know why the tests are failing, I'll try to find time in the next couple
of days to update the pull request.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#85 (comment)

Nadalin Alessandro
http://www.linkedin.com/in/alessandronadalin
www.odino.org
www.twitter.com/_odino_

@odino
Copy link
Contributor

odino commented Mar 31, 2016

@stuartm any update?

@stuartm
Copy link
Author

stuartm commented Mar 31, 2016

Sorry, work has been really crazy. I'll make time tonight to sort this out.

@odino
Copy link
Contributor

odino commented Mar 31, 2016

no worries man, know that feeling :)

On Thu, Mar 31, 2016 at 1:05 PM, Stuart Morgan notifications@github.com
wrote:

Sorry, work has been really crazy. I'll make time tonight to sort this out.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#85 (comment)

Nadalin Alessandro
http://www.linkedin.com/in/alessandronadalin
www.odino.org
www.twitter.com/_odino_

@odino
Copy link
Contributor

odino commented Apr 11, 2016

hey @stuartm any luck with this? :)

@odino
Copy link
Contributor

odino commented Jun 12, 2016

closed for inactivity -- feel free to pick this up again!

@odino odino closed this Jun 12, 2016
@stuartm
Copy link
Author

stuartm commented Jun 12, 2016

Well I will eventually find time to sort the patch out, however I don't think the ticket should be closed since the bug is still present.

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