Change OAuth2 Authorization Style to Auto Detection, Avoid Client.Sec… #33342

p-leh · 2022-10-13T07:16:06Z

…ret Validation

Enhancement

What does this PR do?

The option is missing that the credentials going to be passed as header params as well or even better to use the automatic function from the oauth2 package. (oauth2 package - golang.org/x/oauth2 - Go Packages)
The passing of the required client credentials is to strict, because the param validator checks the client secret as well. But the client secret can be empty too and even still valid for oauth2 access process.

Why is it important?

This makes it possible to use the httpjson more generic and more flexible OAuth2 services

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.

Related issues

Closes [Filebeat][xpack][httpjson] - OAuth2 use without client secret and with url params is not possible #33327

Use cases

We have an oauth2 service which supports passing the parameter by header only and not by params in the body.
Furthermore the required credentials for the client contains a id and an empty secret.

…ret Validation

cla-checker-service · 2022-10-13T07:16:12Z

💚 CLA has been signed

botelastic · 2022-10-13T07:16:12Z

This pull request doesn't have a Team:<team> label.

mergify · 2022-10-13T07:16:41Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @p-leh? 🙏.
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-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

elasticmachine · 2022-10-13T07:21:26Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-10-21T09:21:45.540+0000
Duration: 72 min 13 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2287
Skipped	166
Total	2453

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
/package : Generate the packages and run the E2E tests.
/beats-tester : Run the installation tests with beats-tester.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

x-pack/filebeat/input/httpjson/config_auth.go

efd6 · 2022-10-14T05:29:06Z

x-pack/filebeat/input/httpjson/config_auth.go

@@ -211,7 +211,7 @@ func (o *oAuth2Config) Validate() error {
 	case oAuth2ProviderGoogle:
 		return o.validateGoogleProvider()
 	case oAuth2ProviderDefault:
-		if o.TokenURL == "" || o.ClientID == "" || o.ClientSecret == "" {
+		if o.TokenURL == "" || o.ClientID == "" {


This change troubles me. The RFC says that the secret is REQUIRED but that it may be omitted if it's the empty string. This does that, but it allows a user to omit the var completely without error. I would like to have a check that the var is set, even if it is set to "".

The type of ClientSecret is string therefore the inital value is always "".
Do you have any idea to check that this var was set?

The normal approach would be to make the field be a pointer to the value, so a *string, but that will complicate some of the logic. If the field is not set, then it will be nil, otherwise it will be a non-nil pointer to the string, which may be "".

Okay, thx for this hint but I think that going to make it more complicated and needs deeper changes within the logic.
It is an option to set the field as defaulted with "" in the context of beat?

I'll have a look into how complicated it will be to make it aware of set state without using "" as the zero value.

It is an option to set the field as defaulted with "" in the context of beat?

I'm sorry, I don't understand this question.

I'll have a look into how complicated it will be to make it aware of set state without using "" as the zero value.

Thx in advance!

It is an option to set the field as defaulted with "" in the context of beat?

Sry, my bad. I meant it this way, to declare that the client secret is still required but has the default value "" if not set.

to declare that the client secret is still required but has the default value "" if not set.

That is not possible. An unset string is "". This is why the pointer is needed. The zero value of a *string is nil, but the zero value of a string is "".

The following change works (includes tests).

diff --git a/x-pack/filebeat/input/httpjson/config_auth.go b/x-pack/filebeat/input/httpjson/config_auth.go index 71911cd6f4..d4e7c4ab5c 100644 --- a/x-pack/filebeat/input/httpjson/config_auth.go +++ b/x-pack/filebeat/input/httpjson/config_auth.go @@ -84,7 +84,7 @@ type oAuth2Config struct { // common oauth fields ClientID string `config:"client.id"` - ClientSecret string `config:"client.secret"` + ClientSecret *string `config:"client.secret"` EndpointParams map[string][]string `config:"endpoint_params"` Password string `config:"password"` Provider oAuth2Provider `config:"provider"` @@ -114,7 +114,7 @@ func (o *oAuth2Config) isEnabled() bool { func (o *oAuth2Config) clientCredentialsGrant(ctx context.Context, _ *http.Client) *http.Client { creds := clientcredentials.Config{ ClientID: o.ClientID, - ClientSecret: o.ClientSecret, + ClientSecret: maybeString(o.ClientSecret), TokenURL: o.getTokenURL(), Scopes: o.Scopes, EndpointParams: o.getEndpointParams(), @@ -131,10 +131,10 @@ func (o *oAuth2Config) client(ctx context.Context, client *http.Client) (*http.C if o.User != "" || o.Password != "" { conf := &oauth2.Config{ ClientID: o.ClientID, - ClientSecret: o.ClientSecret, + ClientSecret: maybeString(o.ClientSecret), Endpoint: oauth2.Endpoint{ TokenURL: o.TokenURL, - AuthStyle: authStyleInParams, + AuthStyle: oauth2.AuthStyleAutoDetect, }, } token, err := conf.PasswordCredentialsToken(ctx, o.User, o.Password) @@ -167,6 +167,14 @@ func (o *oAuth2Config) client(ctx context.Context, client *http.Client) (*http.C } } +// maybeString returns the string pointed to by p or "" if p in nil. +func maybeString(p *string) string { + if p == nil { + return "" + } + return *p +} + // getTokenURL returns the TokenURL. func (o *oAuth2Config) getTokenURL() string { switch o.getProvider() { @@ -211,7 +219,7 @@ func (o *oAuth2Config) Validate() error { case oAuth2ProviderGoogle: return o.validateGoogleProvider() case oAuth2ProviderDefault: - if o.TokenURL == "" || o.ClientID == "" || o.ClientSecret == "" { + if o.TokenURL == "" || o.ClientID == "" || o.ClientSecret == nil { return errors.New("both token_url and client credentials must be provided") } if (o.User != "" && o.Password == "") || (o.User == "" && o.Password != "") { @@ -228,7 +236,7 @@ func (o *oAuth2Config) Validate() error { var findDefaultGoogleCredentials = google.FindDefaultCredentials func (o *oAuth2Config) validateGoogleProvider() error { - if o.TokenURL != "" || o.ClientID != "" || o.ClientSecret != "" || + if o.TokenURL != "" || o.ClientID != "" || o.ClientSecret != nil || o.AzureTenantID != "" || o.AzureResource != "" || len(o.EndpointParams) != 0 { return errors.New("none of token_url and client credentials can be used, use google.credentials_file, google.jwt_file, google.credentials_json or ADC instead") } @@ -295,7 +303,7 @@ func (o *oAuth2Config) validateAzureProvider() error { if o.TokenURL != "" && o.AzureTenantID != "" { return errors.New("only one of token_url and tenant_id can be used") } - if o.ClientID == "" || o.ClientSecret == "" { + if o.ClientID == "" || o.ClientSecret == nil { return errors.New("client credentials must be provided") } diff --git a/x-pack/filebeat/input/httpjson/config_test.go b/x-pack/filebeat/input/httpjson/config_test.go index 2ee8c4276c..116ea617e3 100644 --- a/x-pack/filebeat/input/httpjson/config_test.go +++ b/x-pack/filebeat/input/httpjson/config_test.go @@ -152,6 +152,32 @@ func TestConfigOauth2Validation(t *testing.T) { "auth.oauth2": map[string]interface{}{}, }, }, + { + name: "client credential secret may be empty", + input: map[string]interface{}{ + "auth.oauth2": map[string]interface{}{ + "enabled": true, + "token_url": "localhost", + "client": map[string]interface{}{ + "id": "a_client_id", + "secret": "", + }, + }, + }, + }, + { + name: "client credential secret may not be missing", + expectedErr: "both token_url and client credentials must be provided accessing 'auth.oauth2'", + input: map[string]interface{}{ + "auth.oauth2": map[string]interface{}{ + "enabled": true, + "token_url": "localhost", + "client": map[string]interface{}{ + "id": "a_client_id", + }, + }, + }, + }, { name: "if user and password is set oauth2 must use user-password authentication", input: map[string]interface{}{

Thank you very much! I've made the changes above.

efd6 · 2022-10-18T21:24:56Z

/test

efd6

After nit is addressed, this LGTM (I will need to get another approver since I wrote the changes).

CHANGELOG-developer.next.asciidoc

Co-authored-by: Dan Kortschak <[email protected]>

efd6 · 2022-10-19T08:48:41Z

/test

efd6 · 2022-10-20T01:58:39Z

/test

mergify · 2022-10-20T22:03:55Z

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 main upstream/main
git merge upstream/main
git push upstream main

p-leh · 2022-10-21T05:30:17Z

/test

efd6 · 2022-10-21T09:21:33Z

/test

p-leh · 2022-10-24T12:55:19Z

Hi @efd6 , sorry about the stupid question but what should I do next with this PR? Do have I any things to do now?

efd6 · 2022-10-24T21:33:35Z

Sorry for the delay. There is nothing for you to do here. I am waiting for a second reviewer.

ShourieG · 2022-10-25T06:30:46Z

x-pack/filebeat/input/httpjson/config_auth.go

@@ -167,6 +164,14 @@ func (o *oAuth2Config) client(ctx context.Context, client *http.Client) (*http.C
 	}
 }

+// maybeString returns the string pointed to by p or "" if p in nil.
+func maybeString(p *string) string {


The name maybeString suggests that the type maybe a string or something else entirely suggesting we are doing a type check here, but in reality we are checking if the string is nil or not and returning a value accordingly. Maybe something like checkIfEmpty or something similar makes more sense ?

It's not a check it's a weak maybe in the vein of a Rust maybe. It either is a string or it is not valid. In the case that it's not valid, an empty string suffices.

ShourieG · 2022-10-25T06:33:20Z

@efd6 @p-leh LGTM, except for the name of the function maybeString . Approving nonetheless, maybe update the func name if possible.

…mpty secrets (#33342) Allow automatic OAuth2 style detection and do not require a non-empty client secret, though do require it to be explicitly empty if an empty secret is needed. Co-authored-by: Dan Kortschak <[email protected]>

Change OAuth2 Authorization Style to Auto Detection, Avoid Client.Sec…

7907209

…ret Validation

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 13, 2022

mergify bot assigned p-leh Oct 13, 2022

p-leh and others added 2 commits October 13, 2022 10:57

Merge branch 'elastic:main' into main

924c437

Add Documenation

a25b38b

p-leh marked this pull request as ready for review October 13, 2022 09:53

p-leh requested a review from a team as a code owner October 13, 2022 09:53

efd6 reviewed Oct 14, 2022

View reviewed changes

p-leh and others added 2 commits October 18, 2022 10:50

Change clientsecret to pointer to allow empty values, add tests

398c838

Merge branch 'main' into main

0524cfa

efd6 reviewed Oct 18, 2022

View reviewed changes

CHANGELOG-developer.next.asciidoc Outdated Show resolved Hide resolved

p-leh and others added 2 commits October 19, 2022 07:24

Merge branch 'main' into main

73f592d

Update CHANGELOG-developer.next.asciidoc

bcd5082

Co-authored-by: Dan Kortschak <[email protected]>

Merge branch 'main' into main

7082443

Merge branch 'main' into main

2b21a12

ShourieG reviewed Oct 25, 2022

View reviewed changes

ShourieG approved these changes Oct 25, 2022

View reviewed changes

efd6 merged commit 5dd5e88 into elastic:main Oct 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change OAuth2 Authorization Style to Auto Detection, Avoid Client.Sec… #33342

Change OAuth2 Authorization Style to Auto Detection, Avoid Client.Sec… #33342

p-leh commented Oct 13, 2022 •

edited

Loading

cla-checker-service bot commented Oct 13, 2022 •

edited

Loading

botelastic bot commented Oct 13, 2022

mergify bot commented Oct 13, 2022

elasticmachine commented Oct 13, 2022 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

efd6 Oct 14, 2022

p-leh Oct 14, 2022

efd6 Oct 14, 2022 •

edited

Loading

p-leh Oct 17, 2022

efd6 Oct 17, 2022

p-leh Oct 17, 2022

efd6 Oct 17, 2022

efd6 Oct 18, 2022

p-leh Oct 18, 2022

efd6 commented Oct 18, 2022

efd6 left a comment

efd6 commented Oct 19, 2022

efd6 commented Oct 20, 2022

mergify bot commented Oct 20, 2022

p-leh commented Oct 21, 2022

efd6 commented Oct 21, 2022

p-leh commented Oct 24, 2022

efd6 commented Oct 24, 2022

ShourieG Oct 25, 2022 •

edited

Loading

efd6 Oct 25, 2022

ShourieG commented Oct 25, 2022

Change OAuth2 Authorization Style to Auto Detection, Avoid Client.Sec… #33342

Change OAuth2 Authorization Style to Auto Detection, Avoid Client.Sec… #33342

Conversation

p-leh commented Oct 13, 2022 • edited Loading

What does this PR do?

Why is it important?

Checklist

Related issues

Use cases

cla-checker-service bot commented Oct 13, 2022 • edited Loading

botelastic bot commented Oct 13, 2022

mergify bot commented Oct 13, 2022

elasticmachine commented Oct 13, 2022 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efd6 Oct 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efd6 commented Oct 18, 2022

efd6 left a comment

Choose a reason for hiding this comment

efd6 commented Oct 19, 2022

efd6 commented Oct 20, 2022

mergify bot commented Oct 20, 2022

p-leh commented Oct 21, 2022

efd6 commented Oct 21, 2022

p-leh commented Oct 24, 2022

efd6 commented Oct 24, 2022

ShourieG Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShourieG commented Oct 25, 2022

p-leh commented Oct 13, 2022 •

edited

Loading

cla-checker-service bot commented Oct 13, 2022 •

edited

Loading

elasticmachine commented Oct 13, 2022 •

edited by jenkins-beats-ci bot

Loading

efd6 Oct 14, 2022 •

edited

Loading

ShourieG Oct 25, 2022 •

edited

Loading