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

Contents of x5c JWT header invalid #13

Open
onematchfox opened this issue Oct 15, 2019 · 4 comments
Open

Contents of x5c JWT header invalid #13

onematchfox opened this issue Oct 15, 2019 · 4 comments

Comments

@onematchfox
Copy link

onematchfox commented Oct 15, 2019

The x5c JWT header produced by this plugin is currently invalid. This is because the header is set at

b64_encode(get_kong_key("pubder", get_public_key_location(conf)))
using the local method at
local function b64_encode(input)
which also performs the additional base64 url encoding. Thus, the header value itself is unnecessarily encoded twice. I.e. this substitution only needs to be performed prior during the final base64 url encoding of the header segments at
local segments = {

Can be verified at https://jwt.io/.

Will submit PR to fix in the next day or so (along with a couple other changes/fixes).

@jeremyjpj0916
Copy link
Collaborator

jeremyjpj0916 commented Oct 16, 2019

cc @rsbrisci on the x5c logic. It's working for us internally right now 😀, but I did check our validation library we offer upstream providers internally and I see it b64 decoding the x5c element itself too post decoding of the jwt for parsing. I don't know of a way to move forward with that fix for us without potentially breaking our various api providers(some don't even use our offered library and wrote their own too). Hmm.

As for execution time, fully agree there, we don't do request body transformations internally so never been an issue for us but I can see it happening with other folks.

Also appreciate the PR refactor of schema to meet the new standard and not leverage the legacy format. I am slightly unsure the behavior of Kong when we do so as our plugin sits globally on the gateway enabled, changing the underlying schema.lua file with the plugin existing in the db. I will have to play with that in a sandbox to see how we can merge that and be safe to not hurt our environments too as its good to modernize. All our plugins currently still use the legacy schema format 💀 .

@onematchfox
Copy link
Author

onematchfox commented Oct 16, 2019

Thanks for the quick feedback!

re: x5c logic

I'll leave that up to you guys then as to how you want to handle it. Ultimately I don't want the header as I don't want the overhead of the extra data. I started out by just making the header optional but in doing so noticed that the header being generated is not according to specification so I figured I might as well fix it.

I see it b64 decoding the x5c element itself

You mean b64 URL decoding? The certificate should be b64, it's the additional URL encoding to cater for +/= that is the issue.

appreciate the PR refactor of schema

You're welcome. I thought I'd throw that in as a little thank you for the initial work that went into this.

I will have to play with that in a sandbox to see how we can merge that and be safe to not hurt our environments

I upgraded all of my own plugins back when 1.0 was released and I don't remember there being any issues with plugins that were already configured so long as the fields on the schema didn't change. I haven't added/removed any additional schema values so AFAIK it should just work out the box - Kong has been doing the conversion for you in the background until now. However, be aware that the schema is slightly different in that it is restricted to no_consumer and protocols_http

@jeremyjpj0916
Copy link
Collaborator

jeremyjpj0916 commented Nov 5, 2019

@onematchfox still trying to work in time to review all this. We do agile sprints and I have to get internal approval from our product owner for these sorts of engagements(and the changes that come along with your pr's have to be discussed among a few folk). Have not forgotten about yah!

cc @rsbrisci

@onematchfox
Copy link
Author

All good. I'm operating off an internal fork of this for the time being. Thanks for letting me know.

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

No branches or pull requests

2 participants