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 1 commit
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
4 changes: 2 additions & 2 deletions auth/basic/README.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
`package auth/basic` provides a basic auth middleware [Mozilla article](https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication)
`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

Expand All @@ -13,4 +13,4 @@ httptransport.NewServer(
)
```

For AuthMiddleware to be able to pick up Authentication header from a http request we need to pass it through the context with something like ```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."

49 changes: 21 additions & 28 deletions auth/basic/middleware.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package basic

import (
"bytes"
"context"
"crypto/sha256"
"crypto/subtle"
Expand All @@ -13,49 +14,32 @@ import (
httptransport "github.com/go-kit/kit/transport/http"
)

// AuthError represents generic Authorization error
// 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 implemntation of StatusCoder interface in go-kit/http
// 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 implemntation of Error interface
// Error is an implementation of the Error interface.
func (AuthError) Error() string {
return http.StatusText(http.StatusUnauthorized)
}

// Headers is an implemntation of Headerer interface in go-kit/http
// 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.

}

func credsAreValid(givenUser, givenPass, requiredUser, requiredPass string) bool {
// Equalize lengths of supplied and required credentials
// by hashing them
givenUserBytes := sha256.Sum256([]byte(givenUser))
givenPassBytes := sha256.Sum256([]byte(givenPass))
requiredUserBytes := sha256.Sum256([]byte(requiredUser))
requiredPassBytes := sha256.Sum256([]byte(requiredPass))

// Compare the supplied credentials to those set in our options
if subtle.ConstantTimeCompare(givenUserBytes[:], requiredUserBytes[:]) == 1 &&
subtle.ConstantTimeCompare(givenPassBytes[:], requiredPassBytes[:]) == 1 {
return true
}

return false
}

// parseBasicAuth parses an HTTP Basic Authentication string.
// "Basic QWxhZGRpbjpvcGVuIHNlc2FtZQ==" returns ("Aladdin", "open sesame", true).
func parseBasicAuth(auth string) (username, password string, ok bool) {
// "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
Expand All @@ -64,16 +48,19 @@ func parseBasicAuth(auth string) (username, password string, ok bool) {
if err != nil {
return
}
cs := string(c)
s := strings.IndexByte(cs, ':')

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

// AuthMiddleware returns a Basic Authentication middleware for a particular user and password
// 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.

Expand All @@ -82,7 +69,13 @@ func AuthMiddleware(requiredUser, requiredPassword, realm string) endpoint.Middl
return nil, AuthError{realm}
}

if !credsAreValid(givenUser, givenPass, requiredUser, requiredPassword) {
// 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}
}

Expand Down