-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[extension/basicauth] Implement configauth.ClientAuthenticator
#8847
Changes from 1 commit
acbebd0
644203c
4b2da9d
342552a
0a33c89
b0dc172
bbc4c1d
2ae7118
7ffcfc3
2971a6e
5d64364
a83bf79
2e12e9e
9d3312c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,18 +15,19 @@ | |
package basicauthextension // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/basicauthextension" | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
"encoding/base64" | ||
"errors" | ||
"fmt" | ||
"io" | ||
"os" | ||
"io/ioutil" | ||
"net/http" | ||
"strings" | ||
|
||
"github.com/tg123/go-htpasswd" | ||
"go.opentelemetry.io/collector/client" | ||
"go.opentelemetry.io/collector/component" | ||
"go.opentelemetry.io/collector/config/configauth" | ||
creds "google.golang.org/grpc/credentials" | ||
) | ||
|
||
var ( | ||
|
@@ -38,50 +39,54 @@ var ( | |
) | ||
|
||
type basicAuth struct { | ||
htpasswd HtpasswdSettings | ||
matchFunc func(username, password string) bool | ||
htpasswd HtpasswdSettings | ||
matchFunc func(username, password string) bool | ||
userPassPair string | ||
} | ||
|
||
func newExtension(cfg *Config) (configauth.ServerAuthenticator, error) { | ||
func newExtension(cfg *Config) (*basicAuth, error) { | ||
if cfg.Htpasswd.File == "" && cfg.Htpasswd.Inline == "" { | ||
return nil, errNoCredentialSource | ||
} | ||
ba := basicAuth{ | ||
htpasswd: cfg.Htpasswd, | ||
} | ||
return configauth.NewServerAuthenticator(configauth.WithStart(ba.start), configauth.WithAuthenticate(ba.authenticate)), nil | ||
return &ba, nil | ||
} | ||
|
||
func (ba *basicAuth) start(ctx context.Context, host component.Host) error { | ||
var rs []io.Reader | ||
func (ba *basicAuth) Start(ctx context.Context, host component.Host) error { | ||
var buff bytes.Buffer | ||
|
||
if ba.htpasswd.File != "" { | ||
f, err := os.Open(ba.htpasswd.File) | ||
bytes, err := ioutil.ReadFile(ba.htpasswd.File) | ||
if err != nil { | ||
return fmt.Errorf("open htpasswd file: %w", err) | ||
return fmt.Errorf("open file error: %w", err) | ||
} | ||
defer f.Close() | ||
|
||
rs = append(rs, f) | ||
rs = append(rs, strings.NewReader("\n")) | ||
buff.Write(bytes) | ||
buff.WriteString("\n") | ||
} | ||
|
||
// Ensure that the inline content is read the last. | ||
// This way the inline content will override the content from file. | ||
rs = append(rs, strings.NewReader(ba.htpasswd.Inline)) | ||
mr := io.MultiReader(rs...) | ||
if len(ba.htpasswd.Inline) > 0 { | ||
buff.Truncate(buff.Len()) | ||
buff.WriteString(ba.htpasswd.Inline) | ||
} | ||
|
||
htp, err := htpasswd.NewFromReader(mr, htpasswd.DefaultSystems, nil) | ||
htp, err := htpasswd.NewFromReader(bytes.NewBuffer(buff.Bytes()), htpasswd.DefaultSystems, nil) | ||
if err != nil { | ||
return fmt.Errorf("read htpasswd content: %w", err) | ||
} | ||
|
||
ba.userPassPair = buff.String() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I right to assume that this is getting the username and password from the htpasswd file? If so, that's not right. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was my bad. I assumed something else. I have reverted this block. |
||
ba.matchFunc = htp.Match | ||
return nil | ||
} | ||
|
||
func (ba *basicAuth) Shutdown(ctx context.Context) error { | ||
return nil | ||
} | ||
|
||
func (ba *basicAuth) authenticate(ctx context.Context, headers map[string][]string) (context.Context, error) { | ||
func (ba *basicAuth) Authenticate(ctx context.Context, headers map[string][]string) (context.Context, error) { | ||
auth := getAuthHeader(headers) | ||
if auth == "" { | ||
return ctx, errNoAuth | ||
|
@@ -155,6 +160,17 @@ func parseBasicAuth(auth string) (*authData, error) { | |
}, nil | ||
} | ||
|
||
func getBasicAuth(auth string) (*authData, error) { | ||
si := strings.Index(auth, ":") | ||
if si < 0 { | ||
return nil, errInvalidFormat | ||
} | ||
return &authData{ | ||
username: auth[:si], | ||
password: auth[si+1:], | ||
}, nil | ||
} | ||
|
||
var _ client.AuthData = (*authData)(nil) | ||
|
||
type authData struct { | ||
|
@@ -177,3 +193,29 @@ func (a *authData) GetAttribute(name string) interface{} { | |
func (*authData) GetAttributeNames() []string { | ||
return []string{"username", "raw"} | ||
} | ||
|
||
type basicAuthRoundTripper struct { | ||
base http.RoundTripper | ||
authData *authData | ||
} | ||
|
||
func (b *basicAuthRoundTripper) RoundTrip(request *http.Request) (*http.Response, error) { | ||
newRequest := request.Clone(request.Context()) | ||
newRequest.SetBasicAuth(b.authData.username, b.authData.password) | ||
return b.base.RoundTrip(newRequest) | ||
} | ||
|
||
func (ba *basicAuth) RoundTripper(base http.RoundTripper) (http.RoundTripper, error) { | ||
authData, err := getBasicAuth(ba.userPassPair) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return &basicAuthRoundTripper{ | ||
base: base, | ||
authData: authData, | ||
}, nil | ||
} | ||
|
||
func (ba *basicAuth) PerRPCCredentials() (creds.PerRPCCredentials, error) { | ||
return nil, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you not implementing this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this for gRPC? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ import ( | |
"encoding/base64" | ||
"fmt" | ||
"io/ioutil" | ||
"net/http" | ||
"os" | ||
"testing" | ||
|
||
|
@@ -127,6 +128,18 @@ func TestBasicAuth_InvalidPrefix(t *testing.T) { | |
assert.Equal(t, errInvalidSchemePrefix, err) | ||
} | ||
|
||
func TestBasicAuth_NoFile(t *testing.T) { | ||
ext, err := newExtension(&Config{ | ||
Htpasswd: HtpasswdSettings{ | ||
File: "/non/existing/file", | ||
}, | ||
}) | ||
require.NoError(t, err) | ||
require.NotNil(t, ext) | ||
|
||
require.Error(t, ext.Start(context.Background(), componenttest.NewNopHost())) | ||
} | ||
|
||
func TestBasicAuth_InvalidFormat(t *testing.T) { | ||
ext, err := newExtension(&Config{ | ||
Htpasswd: HtpasswdSettings{ | ||
|
@@ -193,3 +206,108 @@ func TestBasicAuth_SupportedHeaders(t *testing.T) { | |
assert.NoError(t, err) | ||
} | ||
} | ||
|
||
type mockRoundTripper struct{} | ||
|
||
func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { | ||
resp := &http.Response{StatusCode: http.StatusOK, Header: map[string][]string{}} | ||
for k, v := range req.Header { | ||
resp.Header[k] = v | ||
} | ||
return resp, nil | ||
} | ||
|
||
func TestBasicAuth_ClientValid(t *testing.T) { | ||
creds := "username:password" | ||
ext, err := newExtension(&Config{ | ||
Htpasswd: HtpasswdSettings{ | ||
Inline: creds, | ||
}, | ||
}) | ||
assert.NotNil(t, ext) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the tip. Reworked. |
||
require.NoError(t, err) | ||
|
||
assert.Nil(t, ext.Start(context.Background(), componenttest.NewNopHost())) | ||
|
||
base := &mockRoundTripper{} | ||
c, err := ext.RoundTripper(base) | ||
assert.NoError(t, err) | ||
assert.NotNil(t, c) | ||
|
||
authCreds := base64.StdEncoding.EncodeToString([]byte(creds)) | ||
orgHeaders := http.Header{ | ||
"Test-Header-1": []string{"test-value-1"}, | ||
} | ||
expectedHeaders := http.Header{ | ||
"Test-Header-1": []string{"test-value-1"}, | ||
"Authorization": {fmt.Sprintf("Basic %s", authCreds)}, | ||
} | ||
|
||
resp, err := c.RoundTrip(&http.Request{Header: orgHeaders}) | ||
assert.NoError(t, err) | ||
assert.Equal(t, expectedHeaders, resp.Header) | ||
assert.Nil(t, ext.Shutdown(context.Background())) | ||
neelayu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
func TestBasicAuth_ClientInValid(t *testing.T) { | ||
neelayu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
creds := "invalid" | ||
ext, err := newExtension(&Config{ | ||
Htpasswd: HtpasswdSettings{ | ||
Inline: creds, | ||
}, | ||
}) | ||
assert.NotNil(t, ext) | ||
require.NoError(t, err) | ||
|
||
assert.Nil(t, ext.Start(context.Background(), componenttest.NewNopHost())) | ||
|
||
base := &mockRoundTripper{} | ||
_, err = ext.RoundTripper(base) | ||
assert.Error(t, err) | ||
} | ||
|
||
func Test_GetBasicAuth(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
auth string | ||
want *authData | ||
wantErr bool | ||
}{ | ||
{ | ||
name: "valid", | ||
auth: "username:password", | ||
want: &authData{ | ||
username: "username", | ||
password: "password", | ||
}, | ||
}, | ||
{ | ||
name: "invalid", | ||
auth: "invalid", | ||
wantErr: true, | ||
}, | ||
{ | ||
name: "empty", | ||
auth: "", | ||
wantErr: true, | ||
}, | ||
{ | ||
name: "multiple colons", | ||
auth: "username:password:extra", | ||
want: &authData{ | ||
username: "username", | ||
password: "password:extra", | ||
}, | ||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
got, err := getBasicAuth(tt.auth) | ||
if tt.wantErr { | ||
require.Error(t, err) | ||
return | ||
} | ||
require.Equal(t, tt.want, got) | ||
}) | ||
} | ||
} |
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 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:
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.
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.
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.