[extension/basicauth] Implement `configauth.ClientAuthenticator` #8847

neelayu · 2022-03-25T12:22:07Z

Description:
Enhancement: basicauth extension now supports client auth.

Testing: Manual and Unit Tests added.

jpkrohling · 2022-03-25T16:32:44Z

jpkrohling · 2022-03-25T17:14:48Z

extension/basicauthextension/README.md

@@ -27,16 +27,17 @@ receivers:
 processors:

 exporters:
-  logging:
-    logLevel: debug
+  otlphttp:


How about otlp? It should be used whenever possible instead of otlphttp, from what I understand.

currently the intention is to support only http client

This should then be clearly stated in the readme then. I'm sure I'm missing something obvious, but why can't this be used with gRPC? It would be similar to the bearer token auth, like this:

opentelemetry-collector-contrib/extension/bearertokenauthextension/bearertokenauth.go

Lines 71 to 75 in faf74e4

func (b *BearerTokenAuth) PerRPCCredentials() (credentials.PerRPCCredentials, error) {

return &PerRPCAuth{

metadata: map[string]string{"authorization": b.bearerToken()},

}, nil

}

jpkrohling · 2022-03-25T17:17:14Z

extension/basicauthextension/extension.go

 	if err != nil {
 		return fmt.Errorf("read htpasswd content: %w", err)
 	}
-
+	ba.userPassPair = buff.String()


Am I right to assume that this is getting the username and password from the htpasswd file? If so, that's not right.

It was my bad. I assumed something else. I have reverted this block.

jpkrohling · 2022-03-25T17:17:42Z

extension/basicauthextension/extension.go

+}
+
+func (ba *basicAuth) PerRPCCredentials() (creds.PerRPCCredentials, error) {
+	return nil, nil


Why are you not implementing this?

Is this for gRPC?

jpkrohling · 2022-03-25T17:18:57Z

extension/basicauthextension/extension_test.go

+			Inline: creds,
+		},
+	})
+	assert.NotNil(t, ext)


This should be a require: everything that is required for setting up the stage of the test should use require. When you are exercising the test subject, use assert.

Thanks for the tip. Reworked.

jpkrohling · 2022-03-25T17:24:30Z

extension/basicauthextension/extension.go

 }

-func newExtension(cfg *Config) (configauth.ServerAuthenticator, error) {
+func newExtension(cfg *Config) (*basicAuth, error) {


I think I would prefer the factory to determine whether it needs a client authenticator or a server authenticator (see createExtension in the factory.go).

I also think this extension has two different sets of configuration options:

username and password for the client authenticator, with only one subject

htpasswd for the server authenticator, with potentially multiple subjects

So, if both the username and password are set, this takes the shape of a client authenticator, otherwise it's a server authenticator. The extension cannot be both at the same time. As an operator, I would find it confusing to have one extension instance with the two facets at the same time.

Good points. I have incorporated that logic. I have also enforced that this extention will only act as either a client auth or server auth. If a user needs both, there should be two. Kindly review.

extension/basicauthextension/README.md

Co-authored-by: Stepan Rakitin <stepanr@mailbox.org>

extension/basicauthextension/README.md

neelayu · 2022-04-14T08:43:10Z

@jpkrohling can you give it a one more glance at this. Thanks

jpkrohling · 2022-04-14T16:57:32Z

extension/basicauthextension/README.md

@@ -27,16 +27,17 @@ receivers:
 processors:

 exporters:
-  logging:
-    logLevel: debug
+  otlphttp:


This should then be clearly stated in the readme then. I'm sure I'm missing something obvious, but why can't this be used with gRPC? It would be similar to the bearer token auth, like this:

opentelemetry-collector-contrib/extension/bearertokenauthextension/bearertokenauth.go

Lines 71 to 75 in faf74e4

func (b *BearerTokenAuth) PerRPCCredentials() (credentials.PerRPCCredentials, error) {

return &PerRPCAuth{

metadata: map[string]string{"authorization": b.bearerToken()},

}, nil

}

extension/basicauthextension/README.md

extension/basicauthextension/extension.go

jpkrohling · 2022-04-14T17:01:47Z

extension/basicauthextension/extension.go

+}
+
+func (ba *basicAuth) PerRPCCredentials() (creds.PerRPCCredentials, error) {
+	return nil, nil


extension/basicauthextension/extension_test.go

extension/basicauthextension/extension.go

Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>

…emetry-collector-contrib into client-basicauth

neelayu · 2022-04-15T08:42:13Z

This should then be clearly stated in the readme then. I'm sure I'm missing something obvious, but why can't this be used with gRPC? It would be similar to the bearer token auth, like this:

You are correct. It is indeed similar to bearer auth. The only reason I did not implement it was because it was written HTTP Basic Auth in the readme before. But sure it works for gRPC as well. I have removed HTTP from the readme now and also implemented it for gRPC. Thank you for reviewing it.

jpkrohling

This is looking good, just a couple of minor comments.

extension/basicauthextension/config.go

extension/basicauthextension/extension.go

* Add support for client basic auth * Change Readme * Add changelog entry * separate out client and server authenticator * Fix Readme documentation Co-authored-by: Stepan Rakitin <stepanr@mailbox.org> * Update Readme.md * modify internal components test file * Apply suggestions from code review Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de> * add rpc support * review comments and minor fixes * address comments and make func signatures private * gofmt lint error fix Co-authored-by: Stepan Rakitin <stepanr@mailbox.org> Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de> (cherry picked from commit e841b5e)

neelayu added 2 commits March 25, 2022 17:35

Add support for client basic auth

acbebd0

Change Readme

644203c

neelayu requested review from a team and jpkrohling as code owners March 25, 2022 12:22

github-actions bot assigned djaglowski Mar 25, 2022

Add changelog entry

4b2da9d

jpkrohling reviewed Mar 25, 2022

View reviewed changes

separate out client and server authenticator

342552a

jpkrohling mentioned this pull request Apr 1, 2022

Cannot export using otlp with basic auth #8999

Closed

svrakitin reviewed Apr 4, 2022

View reviewed changes

extension/basicauthextension/README.md Outdated Show resolved Hide resolved

neelayu and others added 2 commits April 5, 2022 11:12

Fix Readme documentation

0a33c89

Co-authored-by: Stepan Rakitin <stepanr@mailbox.org>

Merge branch 'main' into client-basicauth

b0dc172

mar4uk reviewed Apr 12, 2022

View reviewed changes

extension/basicauthextension/README.md Outdated Show resolved Hide resolved

neelayu added 2 commits April 12, 2022 20:42

Update Readme.md

bbc4c1d

modify internal components test file

2ae7118

jpkrohling reviewed Apr 14, 2022

View reviewed changes

neelayu and others added 4 commits April 15, 2022 13:49

Apply suggestions from code review

7ffcfc3

Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de>

add rpc support

2971a6e

Merge branch 'client-basicauth' of https://github.com/neelayu/opentel…

5d64364

…emetry-collector-contrib into client-basicauth

review comments and minor fixes

a83bf79

jpkrohling reviewed Apr 18, 2022

View reviewed changes

neelayu added 2 commits April 19, 2022 11:58

address comments and make func signatures private

2e12e9e

gofmt lint error fix

9d3312c

jpkrohling approved these changes Apr 19, 2022

View reviewed changes

extension/basicauthextension/extension.go Show resolved Hide resolved

extension/basicauthextension/extension.go Show resolved Hide resolved

jpkrohling changed the title ~~Make basicauth implement configauth.ClientAuthenticator to authenticate outgoing http requests using basicauth~~ [extension/basicauth] Implement configauth.ClientAuthenticator Apr 19, 2022

jpkrohling merged commit e841b5e into open-telemetry:main Apr 19, 2022

neelayu mentioned this pull request Apr 19, 2022

Fix Changelog #9362

Merged

raphaelauv mentioned this pull request Apr 28, 2022

basicauth with receiver prometheus_simple #9570

Closed

ahayworth mentioned this pull request May 16, 2022

Update to otel-contrib-col v0.51.0 Shopify/opentelemetry-collector-contrib#796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[extension/basicauth] Implement `configauth.ClientAuthenticator` #8847

[extension/basicauth] Implement `configauth.ClientAuthenticator` #8847

neelayu commented Mar 25, 2022

jpkrohling commented Mar 25, 2022

jpkrohling Mar 25, 2022

neelayu Mar 31, 2022

jpkrohling Apr 14, 2022

jpkrohling Mar 25, 2022

neelayu Mar 31, 2022

jpkrohling Mar 25, 2022

neelayu Mar 31, 2022

jpkrohling Apr 14, 2022

jpkrohling Mar 25, 2022

neelayu Mar 31, 2022

jpkrohling Mar 25, 2022

neelayu Mar 31, 2022

neelayu commented Apr 14, 2022

jpkrohling Apr 14, 2022

jpkrohling Apr 14, 2022

neelayu commented Apr 15, 2022

jpkrohling left a comment

	func (b *BearerTokenAuth) PerRPCCredentials() (credentials.PerRPCCredentials, error) {
	return &PerRPCAuth{
	metadata: map[string]string{"authorization": b.bearerToken()},
	}, nil
	}

[extension/basicauth] Implement configauth.ClientAuthenticator #8847

[extension/basicauth] Implement configauth.ClientAuthenticator #8847

Conversation

neelayu commented Mar 25, 2022

jpkrohling commented Mar 25, 2022

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

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

neelayu commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neelayu commented Apr 15, 2022

jpkrohling left a comment

Choose a reason for hiding this comment

[extension/basicauth] Implement `configauth.ClientAuthenticator` #8847

[extension/basicauth] Implement `configauth.ClientAuthenticator` #8847