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 support for interface #353

Merged
merged 6 commits into from
Apr 10, 2020
Merged
Show file tree
Hide file tree
Changes from 4 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
8 changes: 8 additions & 0 deletions docs/flagr_env.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,11 @@ The best way to configure service account for Flagr to use pubsub only use:
FLAGR_RECORDER_PUBSUB_PROJECT_ID=google-project-id
FLAGR_RECORDER_PUBSUB_KEYFILE=/path/to/service/account.json
```

Basic Authentication for web interface

```
FLAGR_BASIC_AUTH_ENABLED=true
FLAGR_BASIC_AUTH_USERNAME=admin
FLAGR_BASIC_AUTH_PASSWORD=password
```
7 changes: 7 additions & 0 deletions pkg/config/env.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,13 @@ var Config = struct {
HeaderAuthEnabled bool `env:"FLAGR_HEADER_AUTH_ENABLED" envDefault:"false"`
HeaderAuthUserField string `env:"FLAGR_HEADER_AUTH_USER_FIELD" envDefault:"X-Email"`

// Authenticate with basic auth
BasicAuthEnabled bool `env:"FLAGR_BASIC_AUTH_ENABLED" envDefault:"false"`
BasicAuthUsername string `env:"FLAGR_BASIC_AUTH_USERNAME" envDefault:""`
BasicAuthPassword string `env:"FLAGR_BASIC_AUTH_PASSWORD" envDefault:""`
BasicAuthPrefixWhitelistPaths []string `env:"FLAGR_BASIC_AUTH_WHITELIST_PATHS" envDefault:"/api/v1/flags,/api/v1/evaluation,/api/v1/evaluation/batch" envSeparator:","`
jheth marked this conversation as resolved.
Show resolved Hide resolved
BasicAuthExactWhitelistPaths []string `env:"FLAGR_BASIC_AUTH_EXACT_WHITELIST_PATHS" envDefault:"" envSeparator:","`

// WebPrefix - base path for web and API
// e.g. FLAGR_WEB_PREFIX=/foo
// UI path => localhost:18000/foo"
Expand Down
67 changes: 62 additions & 5 deletions pkg/config/middleware.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package config

import (
"crypto/subtle"
"fmt"
"net/http"
"strconv"
Expand Down Expand Up @@ -78,6 +79,10 @@ func SetupGlobalMiddleware(handler http.Handler) http.Handler {
n.Use(setupJWTAuthMiddleware())
}

if Config.BasicAuthEnabled {
n.Use(setupBasicAuthMiddleware())
}

n.Use(&negroni.Static{
Dir: http.Dir("./browser/flagr-ui/dist/"),
Prefix: Config.WebPrefix,
Expand Down Expand Up @@ -118,7 +123,7 @@ func setupRecoveryMiddleware() *negroni.Recovery {
/**
setupJWTAuthMiddleware setup an JWTMiddleware from the ENV config
*/
func setupJWTAuthMiddleware() *auth {
func setupJWTAuthMiddleware() *jwtAuth {
var signingMethod jwt.SigningMethod
var validationKey interface{}
var errParsingKey error
Expand All @@ -138,7 +143,7 @@ func setupJWTAuthMiddleware() *auth {
validationKey = []byte("")
}

return &auth{
return &jwtAuth{
PrefixWhitelistPaths: Config.JWTAuthPrefixWhitelistPaths,
ExactWhitelistPaths: Config.JWTAuthExactWhitelistPaths,
JWTMiddleware: jwtmiddleware.New(jwtmiddleware.Options{
Expand Down Expand Up @@ -175,13 +180,13 @@ func jwtErrorHandler(w http.ResponseWriter, r *http.Request, err string) {
}
}

type auth struct {
type jwtAuth struct {
PrefixWhitelistPaths []string
ExactWhitelistPaths []string
JWTMiddleware *jwtmiddleware.JWTMiddleware
}

func (a *auth) whitelist(req *http.Request) bool {
func (a *jwtAuth) whitelist(req *http.Request) bool {
path := req.URL.Path

// If we set to 401 unauthorized, let the client handles the 401 itself
Expand All @@ -201,14 +206,66 @@ func (a *auth) whitelist(req *http.Request) bool {
return false
}

func (a *auth) ServeHTTP(w http.ResponseWriter, req *http.Request, next http.HandlerFunc) {
func (a *jwtAuth) ServeHTTP(w http.ResponseWriter, req *http.Request, next http.HandlerFunc) {
if a.whitelist(req) {
next(w, req)
return
}
a.JWTMiddleware.HandlerWithNext(w, req, next)
}

/**
setupBasicAuthMiddleware setup an BasicMiddleware from the ENV config
*/
func setupBasicAuthMiddleware() *basicAuth {
return &basicAuth{
Username: []byte(Config.BasicAuthUsername),
Password: []byte(Config.BasicAuthPassword),
PrefixWhitelistPaths: Config.BasicAuthPrefixWhitelistPaths,
ExactWhitelistPaths: Config.BasicAuthExactWhitelistPaths,
}
}

type basicAuth struct {
Username []byte
Password []byte
PrefixWhitelistPaths []string
ExactWhitelistPaths []string
}

func (a *basicAuth) whitelist(req *http.Request) bool {
path := req.URL.Path

for _, p := range a.ExactWhitelistPaths {
if p == path {
return true
}
}

for _, p := range a.PrefixWhitelistPaths {
if p != "" && strings.HasPrefix(path, p) {
return true
}
}
return false
}

func (a *basicAuth) ServeHTTP(w http.ResponseWriter, req *http.Request, next http.HandlerFunc) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you mind adding a unit test for it? thanks

if a.whitelist(req) {
next(w, req)
return
}

username, password, ok := req.BasicAuth()
if !ok || subtle.ConstantTimeCompare(a.Username, []byte(username)) != 1 || subtle.ConstantTimeCompare(a.Password, []byte(password)) != 1 {
w.Header().Set("WWW-Authenticate", `Basic realm="you shall not pass"`)
http.Error(w, "Not authorized", http.StatusUnauthorized)
return
}

next(w, req)
}

type statsdMiddleware struct {
StatsdClient *statsd.Client
}
Expand Down
63 changes: 61 additions & 2 deletions pkg/config/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func TestSetupGlobalMiddleware(t *testing.T) {
Config.PProfEnabled = true
}

func TestAuthMiddleware(t *testing.T) {
func TestJWTAuthMiddleware(t *testing.T) {
h := &okHandler{}

t.Run("it will redirect if jwt enabled but no cookie passed", func(t *testing.T) {
Expand Down Expand Up @@ -255,7 +255,7 @@ o2kQ+X5xK9cipRgEKwIDAQAB
})
}

func TestAuthMiddlewareWithUnauthorized(t *testing.T) {
func TestJWTAuthMiddlewareWithUnauthorized(t *testing.T) {
h := &okHandler{}

t.Run("it will return 401 if no cookie passed", func(t *testing.T) {
Expand Down Expand Up @@ -315,3 +315,62 @@ func TestAuthMiddlewareWithUnauthorized(t *testing.T) {
}
})
}

func TestBasicAuthMiddleware(t *testing.T) {
h := &okHandler{}

t.Run("it will return 200 for web paths when disabled", func(t *testing.T) {
testPaths := []string{"/", "", "/#", "/#/", "/static", "/static/"}
for _, path := range testPaths {
t.Run(fmt.Sprintf("path: %s", path), func(t *testing.T) {
hh := SetupGlobalMiddleware(h)
res := httptest.NewRecorder()
res.Body = new(bytes.Buffer)
req, _ := http.NewRequest("GET", fmt.Sprintf("http://localhost:18000%s", path), nil)
hh.ServeHTTP(res, req)
assert.Equal(t, http.StatusOK, res.Code)
})
}
})

t.Run("it will return 200 for whitelist path if basic auth is enabled", func(t *testing.T) {
Config.BasicAuthEnabled = true
Config.BasicAuthUsername = "admin"
Config.BasicAuthPassword = "password"
defer func() {
Config.BasicAuthEnabled = false
Config.BasicAuthUsername = ""
Config.BasicAuthPassword = ""
}()

hh := SetupGlobalMiddleware(h)
res := httptest.NewRecorder()
res.Body = new(bytes.Buffer)
req, _ := http.NewRequest("GET", "http://localhost:18000/api/v1/flags", nil)
jheth marked this conversation as resolved.
Show resolved Hide resolved
hh.ServeHTTP(res, req)
assert.Equal(t, http.StatusOK, res.Code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually one more thing to test - a happy code path that the basicauth is serving the traffic with username/password passed. this test didn't cover that because /api/v1/flags was one of the default whitelist.

That's why this line wasn't covered yet. https://codecov.io/gh/checkr/flagr/pull/353/diff?src=pr&el=tree#D2-266

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Thanks for catching that. It's been added.

})

t.Run("it will return 401 for web paths when enabled", func(t *testing.T) {
Config.BasicAuthEnabled = true
Config.BasicAuthUsername = "admin"
Config.BasicAuthPassword = "password"
defer func() {
Config.BasicAuthEnabled = false
Config.BasicAuthUsername = ""
Config.BasicAuthPassword = ""
}()

testPaths := []string{"/", "", "/#", "/#/", "/static", "/static/"}
for _, path := range testPaths {
t.Run(fmt.Sprintf("path: %s", path), func(t *testing.T) {
hh := SetupGlobalMiddleware(h)
res := httptest.NewRecorder()
res.Body = new(bytes.Buffer)
req, _ := http.NewRequest("GET", fmt.Sprintf("http://localhost:18000%s", path), nil)
hh.ServeHTTP(res, req)
assert.Equal(t, http.StatusUnauthorized, res.Code)
})
}
})
}