x-pack/filebeat/input/entityanalytics: fix encoding of client_secret

[chrisberkhout]

Proposed commit message

x-pack/filebeat/input/entityanalytics: fix encoding of client_secret (#)

In the Azure Active Directory provider, only encode the value of
`client_secret` once.

Discussion

This bug would corrupt any client secret with a character affected by URL encoding (via url.QueryEscape).

The Entra/AzureAD documentation does say:

The client secret must be URL-encoded before being sent.

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 to doTokenResp,
then in doTokenResp it's passed to URLFormCall,
and URLFormCall encodes it once as form data.

Checklist

• My code follows the style guidelines of this project
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have made corresponding change to the default configuration files
• I have added tests that prove my fix is effective or that my feature works
• I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

[elasticmachine]

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

[mergify]

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @chrisberkhout? 🙏.
For such, you'll need to label your PR with:

• The upcoming major version of the Elastic Stack
• The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

• backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

[mergify]

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

[elasticmachine]

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

[mauri870]

Is it possible to add a testcase to TestRenew to make sure this bug never comes back? Thanks.

[chrisberkhout]

Test assertion added.

Passes on new code. Fails on old code, like this:

--- FAIL: TestRenew (0.00s)
    --- FAIL: TestRenew/new-token (0.00s)
        oauth2_test.go:31:
                Error Trace:    /home/chrisberkhout/Code/beats/x-pack/filebeat/input/entityanalytics/provider/azuread/authenticator/oauth2/oauth2_test.go:31
                                                        /home/chrisberkhout/.gvm/versions/go1.22.8.linux.amd64/src/net/http/server.go:2171
                                                        /home/chrisberkhout/.gvm/versions/go1.22.8.linux.amd64/src/net/http/server.go:3142
                                                        /home/chrisberkhout/.gvm/versions/go1.22.8.linux.amd64/src/net/http/server.go:2044
                                                        /home/chrisberkhout/.gvm/versions/go1.22.8.linux.amd64/src/runtime/asm_amd64.s:1695
                Error:          Not equal:
                                expected: "value&chars=to|escape"
                                actual  : "value%26chars%3Dto%7Cescape"

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -value&chars=to|escape
                                +value%26chars%3Dto%7Cescape
                Test:           TestRenew/new-token
        oauth2_test.go:85:
                Error Trace:    /home/chrisberkhout/Code/beats/x-pack/filebeat/input/entityanalytics/provider/azuread/authenticator/oauth2/oauth2_test.go:85
                Error:          Received unexpected error:
                                failed to renew token: auth token request failed: Post "http://127.0.0.1:40129/tenant-id/oauth2/v2.0/token": EOF
                Test:           TestRenew/new-token
FAIL
exit status 1
FAIL    github.com/elastic/beats/v7/x-pack/filebeat/input/entityanalytics/provider/azuread/authenticator/oauth2 0.036s

[mauri870]

Looks like you have to fix this linter issue as well. Thanks.

[efd6]

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.

[mauri870]

LGTM, tentatively approving. It is only missing the linter fix.

[mergify]

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b fix-double-encoding-of-client-secret upstream/fix-double-encoding-of-client-secret
git merge upstream/main
git push upstream fix-double-encoding-of-client-secret