Fix: Allow non-padded base64 data to be decoded by decode_base64_field #27311
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
botelastic
bot
added
the
needs_team
|
💚 CLA has been signed |
michaelarnauts
force-pushed
the
base64_unpadded
branch
from
5b82d72
to
0f25a96
Compare
💔 Build Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Steps errors
Expand to view the steps failures
|
| Test | Results |
|---|---|
| Failed | 0 |
| Passed | 52574 |
| Skipped | 5133 |
| Total | 57707 |
The CLA has been signed now. |
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
Sure, I can fix the PR easily if somebody from the team could indicate if this is an accepted fix. |
|
/test |
|
Pinging @elastic/agent (Team:Agent) |
botelastic
bot
removed
the
needs_team
|
Hey @michaelarnauts, thanks for your contribution! I think the fix looks good, but not sure about the implications of decoding unpadded base64 data (if any), maybe @adriansr or @kvch can review or give their opinions? Thanks! |
|
I've moved the changelog entry from Breaking Changes to Added as I don't think anyone could have been taking advantage of the fact that this processor failed for non-padded strings. Also the documentation didn't make any claims regarding padding. For the same reason it's also not a bug fix. |
|
/test |
Sorry, I didn't mean to put it in a breaking change. Not sure if it should be called a feature though, especially since the docs didn't indicate that the base64 strings should be padded. I assumed it would support all variants. I don't know what the policy of backporting to a point release is, and if that depends if it's a bug or a feature :) |
|
I would consider it a feature. With this change, Beats can process non-padded base64 data. 🎉 |
kvch
approved these changes
Aug 23, 2021
|
Failing tests are unrelated. |
|
@michaelarnauts Thank you! |
mergify bot
pushed a commit
that referenced
this pull request
Aug 23, 2021
…7311) ## What does this PR do? This change allows the decoding of any raw base64 input strings that were previously encoded without standard padding character (`=`). By stripping the padding, we can use `base64.RawStdEncoding.DecodeString` to decode the base64 string. This is easier than appending the padding characters. Another attempt to fix this has been made in #25817, but that PR has been closed. ## Why is it important? When attempting to decode the payload (middle) section of a JWT token, it was discovered that the decode was failing, because padding characters are not included in a JWT token string. Padding is not required in base64, so it makes sense to allow to decode both unpadded and padded strings. Currently, there is a workaround for some beats by appending the `=`-signs in a javascript processor, but that isn't available in all beats and is an ugly workaround anyway. See https://medium.com/@guyromb/decode-jwt-traefik-access-logs-using-filebeat-95d935eb7c4f Co-authored-by: Adrian Serrano <adrisr83@gmail.com> (cherry picked from commit 9c4f7f9)
kvch
pushed a commit
that referenced
this pull request
Aug 24, 2021
…7311) (#27554) ## What does this PR do? This change allows the decoding of any raw base64 input strings that were previously encoded without standard padding character (`=`). By stripping the padding, we can use `base64.RawStdEncoding.DecodeString` to decode the base64 string. This is easier than appending the padding characters. Another attempt to fix this has been made in #25817, but that PR has been closed. ## Why is it important? When attempting to decode the payload (middle) section of a JWT token, it was discovered that the decode was failing, because padding characters are not included in a JWT token string. Padding is not required in base64, so it makes sense to allow to decode both unpadded and padded strings. Currently, there is a workaround for some beats by appending the `=`-signs in a javascript processor, but that isn't available in all beats and is an ugly workaround anyway. See https://medium.com/@guyromb/decode-jwt-traefik-access-logs-using-filebeat-95d935eb7c4f Co-authored-by: Adrian Serrano <adrisr83@gmail.com> (cherry picked from commit 9c4f7f9) Co-authored-by: Michaël Arnauts <michael.arnauts@gmail.com>
Icedroid
pushed a commit
to Icedroid/beats
that referenced
this pull request
Nov 1, 2021
…astic#27311) ## What does this PR do? This change allows the decoding of any raw base64 input strings that were previously encoded without standard padding character (`=`). By stripping the padding, we can use `base64.RawStdEncoding.DecodeString` to decode the base64 string. This is easier than appending the padding characters. Another attempt to fix this has been made in elastic#25817, but that PR has been closed. ## Why is it important? When attempting to decode the payload (middle) section of a JWT token, it was discovered that the decode was failing, because padding characters are not included in a JWT token string. Padding is not required in base64, so it makes sense to allow to decode both unpadded and padded strings. Currently, there is a workaround for some beats by appending the `=`-signs in a javascript processor, but that isn't available in all beats and is an ugly workaround anyway. See https://medium.com/@guyromb/decode-jwt-traefik-access-logs-using-filebeat-95d935eb7c4f Co-authored-by: Adrian Serrano <adrisr83@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This change allows decoding of any raw base64 input strings that were previously encoded without standard padding character (
=).By stripping the padding, we can use
base64.RawStdEncoding.DecodeStringto decode the base64 string. This is easier then appending the padding characters.Another attempt to fix this has been made in #25817, but that PR has been closed.
Why is it important?
When attempting to decode the payload (middle) section of a JWT token, it was discovered that the decode was failing, because padding characters are not included in a JWT token string. Padding is not required in base64, so it makes sense to allow to decode both unpadded and padded strings.
Currently, there is a workaround for some beats by appending the
=-signs in a javascript processor, but that isn't available in all beats, and is a ugly workaround anyway. See https://medium.com/@guyromb/decode-jwt-traefik-access-logs-using-filebeat-95d935eb7c4fChecklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Author's Checklist
How to test this PR locally
Tests have been added that will test this behavior.
Related issues
Use cases
Screenshots
Logs