Skip to content
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

Add basic auth middleware #605

Merged
merged 5 commits into from
Sep 17, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions auth/basic/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
`package auth/basic` provides a Basic Authentication middleware [Mozilla article](https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this sentence make grammatical sense :) For example,

This package provides a Basic Authentication middleware.
Here is a [Mozilla article](...) with details.


## Usage

```go
import httptransport "github.com/go-kit/kit/transport/http"

httptransport.NewServer(
endpoint.Chain(AuthMiddleware(cfg.auth.user, cfg.auth.password, "Example Realm"))(makeUppercaseEndpoint()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no reason to use the Chain helper here, I don't think?

decodeMappingsRequest,
httptransport.EncodeJSONResponse,
httptransport.ServerBefore(httptransport.PopulateRequestContext),
)
```

For AuthMiddleware to be able to pick up the Authentication header from a HTTP request we need to pass it through the context with something like ```httptransport.ServerBefore(httptransport.PopulateRequestContext)```.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • We could go either way, but in American English, it sounds better to my ear to write "an HTTP."

85 changes: 85 additions & 0 deletions auth/basic/middleware.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package basic

import (
"bytes"
"context"
"crypto/sha256"
"crypto/subtle"
"encoding/base64"
"fmt"
"net/http"
"strings"

"github.com/go-kit/kit/endpoint"
httptransport "github.com/go-kit/kit/transport/http"
)

// AuthError represents an authoriation error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • s/authoriation/authorization/

type AuthError struct {
Realm string
}

// StatusCode is an iimplementation of the StatusCoder interface in go-kit/http.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • s/iimplementation/implementation/

func (AuthError) StatusCode() int {
return http.StatusUnauthorized
}

// Error is an implementation of the Error interface.
func (AuthError) Error() string {
return http.StatusText(http.StatusUnauthorized)
}

// Headers is an implemntation of the Headerer interface in go-kit/http.
func (e AuthError) Headers() http.Header {
return http.Header{
"Content-Type": []string{"text/plain; charset=utf-8"},
"X-Content-Type-Options": []string{"nosniff"},
"WWW-Authenticate": []string{fmt.Sprintf(`Basic realm=%q`, e.Realm)}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a trailing comma and put the closing brace on the next line.

}

// parseBasicAuth parses an HTTP Basic Authentication string.
// "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==" returns ([]byte("Aladdin"), []byte("open sesame"), true).
func parseBasicAuth(auth string) (username, password []byte, ok bool) {
const prefix = "Basic "
if !strings.HasPrefix(auth, prefix) {
return
}
c, err := base64.StdEncoding.DecodeString(auth[len(prefix):])
if err != nil {
return
}

s := bytes.IndexByte(c, ':')
if s < 0 {
return
}
return c[:s], c[s+1:], true
}

// AuthMiddleware returns a Basic Authentication middleware for a particular user and password.
func AuthMiddleware(requiredUser, requiredPassword, realm string) endpoint.Middleware {
requiredUserBytes := sha256.Sum256([]byte(requiredUser))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Convert this array to a slice here, rather than down at line 77 each time you use it.

requiredPassBytes := sha256.Sum256([]byte(requiredPassword))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Convert this array to a slice here, rather than down at line 78 each time you use it.
  • Consider using four extra characters to name the variable "requiredPasswordBytes."


return func(next endpoint.Endpoint) endpoint.Endpoint {
return func(ctx context.Context, request interface{}) (interface{}, error) {
auth := ctx.Value(httptransport.ContextKeyRequestAuthorization).(string)
Copy link
Member

@peterbourgon peterbourgon Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for the presence of the value in the context before type-asserting it to a string. (The type-assert should also be checked.) If there's a failure in either of those steps, the middleware should return an appropriate error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's necessary to check the presence of the value as type assertion will do it as well. From the authentication process view, a missing header is as invalid as empty.

givenUser, givenPass, ok := parseBasicAuth(auth)
if !ok {
return nil, AuthError{realm}
}

// Equalize lengths of supplied and required credentials by hashing them.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Since this comment points out what is happening but not why, consider removing this comment.

givenUserBytes := sha256.Sum256(givenUser)
givenPassBytes := sha256.Sum256(givenPass)

// Compare the supplied credentials to those set in our options.
if subtle.ConstantTimeCompare(givenUserBytes[:], requiredUserBytes[:]) == 0 ||
subtle.ConstantTimeCompare(givenPassBytes[:], requiredPassBytes[:]) == 0 {
return nil, AuthError{realm}
}

return next(ctx, request)
}
}
}
50 changes: 50 additions & 0 deletions auth/basic/middleware_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package basic

import (
"context"
"encoding/base64"
"fmt"
"testing"

httptransport "github.com/go-kit/kit/transport/http"
)

func TestWithBasicAuth(t *testing.T) {
requiredUser := "test-user"
requiredPassword := "test-pass"
realm := "test realm"

type want struct {
result interface{}
err error
}
tests := []struct {
name string
authHeader string
want want
}{
{"Isn't valid without authHeader", "", want{nil, AuthError{realm}}},
{"Isn't valid for wrong user", makeAuthString("wrong-user", requiredPassword), want{nil, AuthError{realm}}},
{"Isn't valid for wrong password", makeAuthString(requiredUser, "wrong-password"), want{nil, AuthError{realm}}},
{"Is valid for correct creds", makeAuthString(requiredUser, requiredPassword), want{true, nil}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ctx := context.WithValue(context.TODO(), httptransport.ContextKeyRequestAuthorization, tt.authHeader)

result, err := AuthMiddleware(requiredUser, requiredPassword, realm)(passedValidation)(ctx, nil)
if result != tt.want.result || err != tt.want.err {
t.Errorf("WithBasicAuth() = result: %v, err: %v, want result: %v, want error: %v", result, err, tt.want.result, tt.want.err)
}
})
}
}

func makeAuthString(user string, password string) string {
data := []byte(fmt.Sprintf("%s:%s", user, password))
return fmt.Sprintf("Basic %s", base64.StdEncoding.EncodeToString(data))
}

func passedValidation(ctx context.Context, request interface{}) (response interface{}, err error) {
return true, nil
}