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

optimization: some small optimizations on base64 encoding&decoding. #467

Merged
merged 1 commit into from
Apr 3, 2023

Conversation

zhuizhuhaomeng
Copy link
Contributor

No description provided.

@zhuizhuhaomeng zhuizhuhaomeng force-pushed the opt branch 2 times, most recently from 88bcc4b to f890fbc Compare February 13, 2023 10:15
Copy link
Collaborator

@bodewig bodewig left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. Would you please add yourself to the AUTHORS file?

lib/resty/openidc.lua Outdated Show resolved Hide resolved
@bodewig
Copy link
Collaborator

bodewig commented Feb 15, 2023

Actually the test failures when running the github action here are real, I can also reproduce them locally. See https://github.com/zmartzone/lua-resty-openidc#running-tests for running the tests. bearer_token_verification_spec.lua now fails and it looks as if pkce_spec.lua not only fails but also prevents the tests from shutting down nginx properly.

@bodewig
Copy link
Collaborator

bodewig commented Feb 15, 2023

for the errors in bearer_token_verification_spec it seems the functions from ngx.base64 are no drop in replacements for our implementations.

openidc_base64_url_decode adds padding when necessary as the JWK spec says you are not expected to pad strings - and decode_base64url raises an error if the padding is missing - when parsing https://github.com/zmartzone/lua-resty-openidc/blob/master/tests/spec/longer_rsa_key_jwk_with_n_and_e.json we get "invalid input" when decoding n.

Likewise openidc_base64_url_encode instruct ngx.encode_base64 to not pad the result (the second argument to the b64 call, something encode_base64url doesn't seem to support.

To me it looks as if we can not replace openidc_base64_url_encode at all and as if we can only replace the double gsubs followed by unb64 with a call to ngx.base64 in openidc_base64_url_decode.

I'll have to find the root cause of the error for the PKCE test failures at a different time, but this test is executing the SHA1 changes.

@bodewig
Copy link
Collaborator

bodewig commented Feb 16, 2023

After a second look the bearer_token_verification_spec error is not a problem of padding. The JWK is wrong, it uses base64 instead of URL-safe base64. The existing openidc_base64_url_decode function happens to work for "normal" base64 as well as URL-safe base64 and so successfully decodes the "bad" value.

I will fix the test JWK in master - so once you rebase these tests should pass - but I believe we still need the extra-code that adds the necessary padding, we may just not have any test that requires it.

As a reference: https://openid.net/specs/draft-jones-json-web-key-03.html#base64urllogic - so when parsing JWKs the code must expect values to not be padded.

lib/resty/openidc.lua Outdated Show resolved Hide resolved
@zhuizhuhaomeng
Copy link
Contributor Author

save the following code as t.lua

local b64url = require("ngx.base64").encode_base64url
local unb64url = require("ngx.base64").decode_base64url

print(b64url("1"))
print(b64url("12"))
print(b64url("123"))
print(b64url("1234"))

print(unb64url(b64url("1")))
print(unb64url(b64url("12")))
print(unb64url(b64url("123")))
print(unb64url(b64url("1234")))

and then run resty t.lua, we can see that the urlencode is not padding, so the decode is not need to be padded.

$ resty t.lua                                            
MQ
MTI
MTIz
MTIzNA
1
12
123
1234

@zhuizhuhaomeng
Copy link
Contributor Author

docker run -t --rm lua-resty-openidc/test:latest
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++.+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
497 successes / 0 failures / 0 errors / 1 pending : 323.649386 seconds

Pending -> ./id_token_validation_spec.lua @ 37
when the id_token obtained from the token endpoint doesn't contain a sub claim need to add support for removing claims

The test run on my site. Does the pending test matter? @bodewig

@bodewig
Copy link
Collaborator

bodewig commented Mar 31, 2023

sorry for the delay @zhuizhuhaomeng this looks fine. Could you please rebase your branch and squash your commits? Thank you.

1 similar comment
@bodewig
Copy link
Collaborator

bodewig commented Mar 31, 2023

sorry for the delay @zhuizhuhaomeng this looks fine. Could you please rebase your branch and squash your commits? Thank you.

@zhuizhuhaomeng
Copy link
Contributor Author

@bodewig I have rebased and squashed to the lastest master.

@zhuizhuhaomeng zhuizhuhaomeng changed the title optimization: some small optimizations. optimization: some small optimizations on base64 encoding. Apr 3, 2023
@zhuizhuhaomeng zhuizhuhaomeng changed the title optimization: some small optimizations on base64 encoding. optimization: some small optimizations on base64 encoding&decoding. Apr 3, 2023
@bodewig
Copy link
Collaborator

bodewig commented Apr 3, 2023

Thank you for your patience

@bodewig bodewig merged commit 734a3f4 into zmartzone:master Apr 3, 2023
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