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
Revert Encoding/Decoding changes for better compatibility #117
Conversation
go test --bench=Bench --run=NONE . Before Change: |
Just to understand it correctly: is only the decoding part problematic or also the encoding part? In my opinion we should definitely not promote the creation of invalid JWTs. I am still torn about whether we should accept malformed tokens or not - or at least have it configurable. Unfortunately, I did not have time to look at the AWS Problem in more detail yet. |
Hi @oxisto, the issue here is on the decoding part, so we can definitely keep the Encoding section as is -> just was not sure if we wanted to make the encoding/decoding utilize different encodings. I just updated it to undo the encoding change, and instead added a check to switch between decoding schemes based on presence of padded characters - similar to what was there before, except not adding extra characters for each token. To drill down into the specific issue -> The AWS ELB guide indicates that they will provide tokens in JWT that is base64 url encoded with padding at the end. With regards to accepting malformed tokens, we definitely should have it be configurable if possible, but from initial inspection, don't see a way of doing it without introducing major changes (which probably requires a re-design of the package), or introducing global variables. Hence why a suggestion to allow compatibility for now until we have a better solution down the line. Based on the update, I re-ran the benchmark, seems like its more similar to main's benchmarks:
|
Introducing a global variable (for now) would not be an issue I would say. The default could be the "proper" way we currently have on main. In a future overhaul we are likely to de-export I would assume from a performance standpoint a simple-if-boolean check should be negligible. Maybe the compiler even removes it in optimisation if you never change the default value, not sure. |
Just to add on this, this behaviour on AWS's puts us in a though spot. What they claim to be a JWT is simply just not a JWT as defined in JWT/JWS RFC. This leaves implementations such as this in the uncomfortable place to deviate from the standard to support something which should not be there in the first place, just because it is "well used". Does anyone know anyone close to the AWS ELB team? Maybe we could got this fixed at AWS rather than having everyone bend to their wishes. Not being too hopeful about this though. I have opened an issue on their docs repo, although this is probably not the right place for it: awsdocs/elb-application-load-balancers-user-guide#42. |
Agreed @oxisto that it should be theoretically be fixed on ELB's side, but like you said, not keeping hopes up for now. If they do make they update, then agreed we should remove this. In terms of keeping it scoped to the parser -> seems like the signing methods also utilize this function, so it seemed awkward to somehow pass this to the underlying signing method etc. But if an overhaul is coming, hopefully can be included there (make the signing methods accept the decoded string directly instead of handling the decoding). I just pushed another update going back to the original proposal in #92 and incorporated @lggomez's recommendation to make it goroutine safe via atomic operations. Benchmark below:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good as far as implementation goes I would say, except the minor things I added in the comments. I want to give it another couple of days if we somehow get a response from AWS before we "approve" this.
Thanks. I do see some benefit in using the |
Make sense! Since this change should be targeted at specific usecases, enabling/disabling is pretty niche as you stated. I've gone ahead and updated the original atomic method here and updated it to the bool approach (with comment stating it is not go-routine safe) here. Let me know which approach you all want to take and can update accordingly! Or hopefully ELB team will come back with an update. |
Sorry it too so long to reply.
I agree with this if it is the current state of affairs. Also, I'm not too worried about the performance and quite sympathetic to the issue, because at a previous co. we used AWS and I imagine my old team is in a similar predicament. It is very unlikely you'll see any movement from AWS to change their implementation. What if we reverted the original PR as mentioned by OP in this ticket? Is the performance of the old code really that noticeable in production systems? |
Probably not, however having this switched on/off with the variable as it is now implemented using this PR seems to be a fair compromise. |
@@ -435,6 +435,88 @@ func TestParser_ParseUnverified(t *testing.T) { | |||
} | |||
} | |||
|
|||
var setPaddingTestData = []struct { | |||
name string | |||
tokenString string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we can toss in the example JWT token from this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added! Note that I had to add the public key to the repo, added a note where we reference it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor things
parser_test.go
Outdated
) | ||
|
||
func init() { | ||
// Load public keys | ||
jwtTestDefaultKey = test.LoadRSAPublicKeyFromDisk("test/sample_key.pub") | ||
jwtTestEC256PublicKey = test.LoadECPublicKeyFromDisk("test/ec256-public.pem") | ||
|
||
// Load cognito public key - note there is only a public key for this key pair and should only be used for the | ||
// two test cases below. | ||
exampleCognitoPublicKey = test.LoadECPublicKeyFromDisk("test/exampleCognito-public.pem") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest avoiding a product name here and just name it paddedKey or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
parser_test.go
Outdated
valid: true, | ||
}, | ||
{ | ||
name: "Error for padded cognito token with padding disabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
parser_test.go
Outdated
valid: false, | ||
}, | ||
{ | ||
name: "Validated padded cognito token with padding enabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here avoid the product name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
token.go
Outdated
@@ -7,6 +7,14 @@ import ( | |||
"time" | |||
) | |||
|
|||
|
|||
// DecodePaddingAllowed will switch the codec used for encoding/decoding JWTs respectively. Note that the JWS RFC7515 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is only used for decoding, not encoding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
This is a small revert to the optimizations made to the encoding/decoding in #33.
While technically JWTs should not have padded characters in Base64 URL encoding, it seems like not every provider may follow this construct, specifically AWS Cognito as seen in #92.
While the change initially was made to have better optimization, it has left compatibility issues, forcing dependencies to stay on v3.2.1 until an update occurs. Thus the proposal is to simply undo the changes made in that section of code, and create a new ticket to better optimize this section.