-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
x-pack/filebeat/input/entityanalytics: fix encoding of client_secret #41393
base: main
Are you sure you want to change the base?
x-pack/filebeat/input/entityanalytics: fix encoding of client_secret #41393
Conversation
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
455f382
to
1fda687
Compare
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
Is it possible to add a testcase to TestRenew to make sure this bug never comes back? Thanks. |
Test assertion added. Passes on new code. Fails on old code, like this:
|
@@ -62,12 +63,13 @@ func TestRenew(t *testing.T) { | |||
value := "test-value" | |||
expiresIn := 1000 | |||
|
|||
srv := testSetupServer(t, value, expiresIn) | |||
clientSecret := "value&chars=to|escape" |
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 like you have to fix this linter issue as well. Thanks.
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 think I would have just changed the name of the label here. From the docs:
The use of hard-coded passwords increases the possibility of password guessing tremendously. This plugin test looks for all string literals and checks the following conditions:
Variables are considered to look like a password if they have match any one of:
- “password”
- “pass”
- “passwd”
- “pwd”
- “secret”
- “token”
The next line is telling, "Note: this can be noisy and may generate false positives." This linter rule should probably not exist.
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.
LGTM, tentatively approving. It is only missing the linter fix.
This pull request is now in conflicts. Could you fix it? 🙏
|
Proposed commit message
Discussion
This bug would corrupt any client secret with a character affected by URL encoding (via
url.QueryEscape
).The Entra/AzureAD documentation does say:
but that means that the value should be encoded like normal for
application/x-www-form-urlencoded
data, not that the value should be URL-encoded before being encoded again as form data.Here's an example of single vs double encoding in the Go Playground.
In microsoft-authentication-library-for-go,
the client secret is added to a
url.Values{}
and passed todoTokenResp
,then in
doTokenResp
it's passed toURLFormCall
,and
URLFormCall
encodes it once as form data.Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.