Add basic auth middleware #605

DimaSalakhov · 2017-08-19T22:32:44Z

Issue #604

seh · 2017-08-27T19:32:26Z

auth/basic/middleware.go

+}
+
+func credsAreValid(givenUser, givenPass, requiredUser, requiredPass string) bool {
+ // Equalize lengths of supplied and required credentials


Why is it important to make the lengths equal? Are you trying to preclude subtle.ConstantTimeCompare from divulging the difference in length by way of it short-circuiting the rest of the comparison?

Though highly unlikely, it looks like you're risking accepting a potential hash collision in trade for defending against a timing attack.

seh · 2017-08-27T19:34:17Z

auth/basic/middleware.go

+ // by hashing them
+ givenUserBytes := sha256.Sum256([]byte(givenUser))
+ givenPassBytes := sha256.Sum256([]byte(givenPass))
+ requiredUserBytes := sha256.Sum256([]byte(requiredUser))


Since the required user name and password values are fixed at line 85 below, if you decide to retain this hashing technique, consider computing the hash on these two values once just before line 77, and reusing the hash instead of the raw values here.

seh · 2017-08-27T19:37:40Z

auth/basic/middleware.go

+ return http.StatusUnauthorized
+}
+
+// Error is an implemntation of Error interface


s/implemntation of Error/implementation of the Error/

Punctuate this sentence.

seh · 2017-08-27T19:38:11Z

auth/basic/middleware.go

+ httptransport "github.com/go-kit/kit/transport/http"
+)
+
+// AuthError represents generic Authorization error


s/represents generic Authorization/represents an authoriation/

Punctuate this sentence.

seh · 2017-08-27T19:38:48Z

auth/basic/middleware.go

+ Realm string
+}
+
+// StatusCode is an implemntation of StatusCoder interface in go-kit/http


s/implemntation of StatusCoder/implementation of the StatusCoder/

Punctuate this sentence.

seh · 2017-08-27T19:40:23Z

auth/basic/middleware.go

+ return http.StatusText(http.StatusUnauthorized)
+}
+
+// Headers is an implemntation of Headerer interface in go-kit/http


s/an implemntation of Headerer/an implementation of the Headerer/

Punctuate this sentence.

seh · 2017-08-27T19:43:01Z

auth/basic/middleware.go

+ "WWW-Authenticate": []string{fmt.Sprintf(`Basic realm=%q`, e.Realm)}}
+}
+
+func credsAreValid(givenUser, givenPass, requiredUser, requiredPass string) bool {


Given that the currency of your implementation is byte vectors, consider demanding byte vectors as parameters.
You could then return byte vectors from your parseBasicAuth function below, rather than converting the byte vectors resulting from decoding the base64-encoded values to strings at line 67, only to convert them back—with allocation and copying cost—here.

seh · 2017-08-27T19:44:08Z

auth/basic/middleware.go

+ return cs[:s], cs[s+1:], true
+}
+
+// AuthMiddleware returns a Basic Authentication middleware for a particular user and password


Punctuate this sentence.

seh · 2017-08-27T19:45:05Z

auth/basic/README.md

@@ -0,0 +1,16 @@
+`package auth/basic` provides a basic auth middleware [Mozilla article](https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication)


s/basic auth/Basic Authorization/

Punctuate this sentence.

seh · 2017-08-27T19:46:49Z

auth/basic/README.md

+ )
+```
+
+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)```


s/up Authentication/up the "Authentication"/

s/a http request/an HTTP request,/

Punctuate this sentence, or s/like/like the following:/.

DimaSalakhov · 2017-08-28T04:53:12Z

@seh Thanks for your review, everything was on point, I very much appreciate it!

In regards to the hash sum calculation sha256.Sum256() according to my very limited knowledge on the topic of timing attack, there are 2 suggestions to prevent them:

use constant time algorithm to prevent bruteforcing the exact position of the first difference in byte arrays. That's exactly what subtle/ConstantTimeCompare does
use byte arrays of the same length for the similar reason. In our case subtle/ConstantTimeCompare short circuit if arrays have a different length.
I might be wrong in that, but this was a premise for me to compare hashes rather than slices. You're right about a potential collision in hashes, but my assumption is that the chances that person would successfully use that for bruteforce are higher than a chance of hash collision. However, I don't have a data to prove it and would be happy to hear a security advice.

seh · 2017-08-28T13:49:14Z

I agree with the first benefit of using subtle.ConstantTimeCompare. Note, though, that computing the hash requires various length-dependent operations, so while we eliminate length difference detection threat from the final byte comparison, we still face timing differences in computing the hashes.

However, if you compute the hashes for the expected user name and password values ahead of time, as I had suggested earlier, then an attacker wouldn't be able to detect any relationship between computing the hashes of the expected and supplied values. You would then face only the threat of a hash collision yielding a false positive.

If the two hashes differ, we know that the supplied values do not match the expected values. If we wanted to be completely sure that a match in the hashes means the underlying values match, we'd then have to compare those underlying values directly—which brings us back to divulging differences in length via timing attack (even with subtle.ConstantTimeCompare), and wastes the utility of using the hash function.

I too would like to hear from others whether we should worry about the likelihood of a hash collision here.

seh · 2017-08-28T13:49:54Z

auth/basic/middleware.go

@@ -13,49 +14,32 @@ import (
 httptransport "github.com/go-kit/kit/transport/http"
 )

-// AuthError represents generic Authorization error
+// AuthError represents an authoriation error.


s/authoriation/authorization/

seh · 2017-08-28T13:50:09Z

auth/basic/middleware.go

 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.


s/iimplementation/implementation/

seh · 2017-08-28T13:51:33Z

auth/basic/middleware.go

 func AuthMiddleware(requiredUser, requiredPassword, realm string) endpoint.Middleware {
+ requiredUserBytes := sha256.Sum256([]byte(requiredUser))


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

seh · 2017-08-28T13:52:15Z

auth/basic/middleware.go

 func AuthMiddleware(requiredUser, requiredPassword, realm string) endpoint.Middleware {
+ requiredUserBytes := sha256.Sum256([]byte(requiredUser))
+ requiredPassBytes := sha256.Sum256([]byte(requiredPassword))


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."

seh · 2017-08-28T13:53:05Z

auth/basic/README.md

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

DimaSalakhov · 2017-08-29T10:53:57Z

@seh I agree with your statement about creating and comparing hashes.
As for the hash collision, I'm leaning more and more towards calling it possible, but unrealistic with the state of the modern hardware. Here is an article from Google about collision they've found in sha128: https://security.googleblog.com/2017/02/announcing-first-sha1-collision.html
From what I see, it takes 1year, 110GPU and right algorithm to achieve it. SHA256 is twice the size and it would be a very long and expensive attack.

seh

These recent changes alleviate my stated concerns.

seh · 2017-08-29T15:39:34Z

auth/basic/middleware.go


 return func(next endpoint.Endpoint) endpoint.Endpoint {
 return func(ctx context.Context, request interface{}) (interface{}, error) {
 auth := ctx.Value(httptransport.ContextKeyRequestAuthorization).(string)
- givenUser, givenPass, ok := parseBasicAuth(auth)
+ givenUser, givenPassword, ok := parseBasicAuth(auth)
 if !ok {
 return nil, AuthError{realm}
 }

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


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

peterbourgon · 2017-08-31T09:42:36Z

auth/basic/README.md

@@ -0,0 +1,16 @@
+`package auth/basic` provides a Basic Authentication middleware [Mozilla article](https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication).


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

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

peterbourgon · 2017-08-31T09:43:06Z

auth/basic/README.md

+import httptransport "github.com/go-kit/kit/transport/http"
+
+httptransport.NewServer(
+ endpoint.Chain(AuthMiddleware(cfg.auth.user, cfg.auth.password, "Example Realm"))(makeUppercaseEndpoint()),


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

peterbourgon · 2017-08-31T09:43:40Z

auth/basic/middleware.go

+ 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)}}


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

peterbourgon · 2017-08-31T09:44:43Z

auth/basic/middleware.go

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


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.

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.

peterbourgon · 2017-08-31T09:46:20Z

With those changes, this looks good to me. Thanks for the initiative and sorry for my delays!

peterbourgon · 2017-09-17T19:34:46Z

Yep! That works 👍

Add basic auth middleware

b48aaea

DimaSalakhov mentioned this pull request Aug 19, 2017

Add Basic Auth support #604

Closed

seh suggested changes Aug 27, 2017

View reviewed changes

Basic Auth: optimize memory allocation.

8c6ad4e

seh suggested changes Aug 28, 2017

View reviewed changes

cache required creds' slices

37a2080

seh approved these changes Aug 29, 2017

View reviewed changes

Clean up comment

6b4ca49

peterbourgon reviewed Aug 31, 2017

View reviewed changes

improve error handling and style

355d3e3

peterbourgon merged commit 7862b3f into go-kit:master Sep 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic auth middleware #605

Add basic auth middleware #605

DimaSalakhov commented Aug 19, 2017

seh Aug 27, 2017

seh Aug 27, 2017

seh Aug 27, 2017

seh Aug 27, 2017

seh Aug 27, 2017

seh Aug 27, 2017

seh Aug 27, 2017

seh Aug 27, 2017

seh Aug 27, 2017

seh Aug 27, 2017

DimaSalakhov commented Aug 28, 2017

seh commented Aug 28, 2017

seh Aug 28, 2017

seh Aug 28, 2017

seh Aug 28, 2017

seh Aug 28, 2017

seh Aug 28, 2017

DimaSalakhov commented Aug 29, 2017

seh left a comment

seh Aug 29, 2017

peterbourgon Aug 31, 2017

peterbourgon Aug 31, 2017

peterbourgon Aug 31, 2017

peterbourgon Aug 31, 2017 •

edited

Loading

DimaSalakhov Sep 2, 2017

peterbourgon commented Aug 31, 2017

peterbourgon commented Sep 17, 2017

		@@ -0,0 +1,16 @@
		`package auth/basic` provides a basic auth middleware [Mozilla article](https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication)

		func AuthMiddleware(requiredUser, requiredPassword, realm string) endpoint.Middleware {
		requiredUserBytes := sha256.Sum256([]byte(requiredUser))

Add basic auth middleware #605

Add basic auth middleware #605

Conversation

DimaSalakhov commented Aug 19, 2017

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

DimaSalakhov commented Aug 28, 2017

seh commented Aug 28, 2017

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

DimaSalakhov commented Aug 29, 2017

seh left a 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

peterbourgon Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbourgon commented Aug 31, 2017

peterbourgon commented Sep 17, 2017

peterbourgon Aug 31, 2017 •

edited

Loading