Skip to content
This repository has been archived by the owner on Jan 24, 2019. It is now read-only.

Add secure middleware to enable more header options #618

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
30 changes: 23 additions & 7 deletions Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions Gopkg.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
#

[[constraint]]
name = "github.com/18F/hmacauth"
version = "~1.0.1"
name = "github.com/mbland/hmacauth"
version = "~1.0.2"

[[constraint]]
name = "github.com/BurntSushi/toml"
Expand Down Expand Up @@ -36,7 +36,7 @@
name = "google.golang.org/api"

[[constraint]]
name = "gopkg.in/fsnotify.v1"
name = "github.com/fsnotify/fsnotify"
version = "~1.2.0"

[[constraint]]
Expand Down
43 changes: 23 additions & 20 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"log"
"os"
"runtime"
"strings"
"time"

"github.com/BurntSushi/toml"
Expand All @@ -21,6 +20,8 @@ func main() {
upstreams := StringArray{}
skipAuthRegex := StringArray{}
googleGroups := StringArray{}
httpAllowedHosts := StringArray{}
httpHostsProxyHeaders := StringArray{}

config := flagSet.String("config", "", "path to config file")
showVersion := flagSet.Bool("version", false, "print version string")
Expand Down Expand Up @@ -81,6 +82,25 @@ func main() {

flagSet.String("signature-key", "", "GAP-Signature request signature key (algorithm:secretkey)")

// These are options that allow you to tune various parameters for https://github.com/unrolled/secure
flagSet.Var(&httpAllowedHosts, "httpAllowedHosts", "a list of fully qualified domain names that are allowed. Default is empty list, which allows any and all host names.")
flagSet.Var(&httpHostsProxyHeaders, "httpHostsProxyHeaders", "a set of header keys that may hold a proxied hostname value for the request.")
flagSet.Bool("httpSSLRedirect", false, "If set to true, then only allow HTTPS requests. Default is false.")
flagSet.Bool("httpSSLTemporaryRedirect", false, "If true, then a 302 will be used while redirecting. Default is false (301).")
flagSet.String("httpSSLHost", "", "the host name that is used to redirect HTTP requests to HTTPS. Default is \"\", which indicates to use the same host.")
flagSet.Int64("httpSTSSeconds", 0, "The max-age of the Strict-Transport-Security header. Default is 0, which would NOT include the header.")
flagSet.Bool("httpSTSIncludeSubdomains", false, "If set to true, the 'includeSubdomains' will be appended to the Strict-Transport-Security header. Default is false.")
flagSet.Bool("httpSTSPreload", false, "If set to true, the 'preload' flag will be appended to the Strict-Transport-Security header. Default is false.")
flagSet.Bool("httpForceSTSHeader", false, "STS header is only included when the connection is HTTPS. If you want to force it to always be added, set to true. Default is false.")
flagSet.Bool("httpFrameDeny", false, "If set to true, adds the X-Frame-Options header with the value of 'DENY'. Default is false.")
flagSet.String("httpCustomFrameOptionsValue", "", "allows the X-Frame-Options header value to be set with a custom value. This overrides the FrameDeny option. Default is \"\".")
flagSet.Bool("httpContentTypeNosniff", false, "If true, adds the X-Content-Type-Options header with the value 'nosniff'. Default is false.")
flagSet.Bool("httpBrowserXssFilter", false, "If true, adds the X-XSS-Protection header with the value '1; mode=block'. Default is false.")
flagSet.String("httpCustomBrowserXssValue", "", "Allows the X-XSS-Protection header value to be set with a custom value. This overrides the BrowserXssFilter option. Default is \"\".")
flagSet.String("httpContentSecurityPolicy", "", "Allows the Content-Security-Policy header value to be set with a custom value. Default is \"\". Passing a template string will replace '$NONCE' with a dynamic nonce value of 16 bytes for each request which can be later retrieved using the Nonce function.")
flagSet.String("httpPublicKey", "", "Implements HPKP to prevent MITM attacks with forged certificates. Default is \"\".")
flagSet.String("httpReferrerPolicy", "", "Allows the Referrer-Policy header with the value to be set with a custom value. Default is \"\".")

flagSet.Parse(os.Args[1:])

if *showVersion {
Expand All @@ -99,31 +119,14 @@ func main() {
}
cfg.LoadEnvForStruct(opts)
options.Resolve(opts, flagSet, cfg)

err := opts.Validate()
if err != nil {
log.Printf("%s", err)
os.Exit(1)
}
validator := NewValidator(opts.EmailDomains, opts.AuthenticatedEmailsFile)
oauthproxy := NewOAuthProxy(opts, validator)

if len(opts.EmailDomains) != 0 && opts.AuthenticatedEmailsFile == "" {
if len(opts.EmailDomains) > 1 {
oauthproxy.SignInMessage = fmt.Sprintf("Authenticate using one of the following domains: %v", strings.Join(opts.EmailDomains, ", "))
} else if opts.EmailDomains[0] != "*" {
oauthproxy.SignInMessage = fmt.Sprintf("Authenticate using %v", opts.EmailDomains[0])
}
}

if opts.HtpasswdFile != "" {
log.Printf("using htpasswd file %s", opts.HtpasswdFile)
oauthproxy.HtpasswdFile, err = NewHtpasswdFromFile(opts.HtpasswdFile)
oauthproxy.DisplayHtpasswdForm = opts.DisplayHtpasswdForm
if err != nil {
log.Fatalf("FATAL: unable to open %s %s", opts.HtpasswdFile, err)
}
}
validator := NewValidator(opts.EmailDomains, opts.AuthenticatedEmailsFile)
oauthproxy := NewSecureProxy(opts, validator)

s := &Server{
Handler: LoggingHandler(os.Stdout, oauthproxy, opts.RequestLogging, opts.RequestLoggingFormat),
Expand Down
44 changes: 44 additions & 0 deletions oauthproxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/bitly/oauth2_proxy/cookie"
"github.com/bitly/oauth2_proxy/providers"
"github.com/mbland/hmacauth"
"github.com/unrolled/secure"
)

const SignatureHeader = "GAP-Signature"
Expand Down Expand Up @@ -115,6 +116,49 @@ func NewFileServer(path string, filesystemPath string) (proxy http.Handler) {
return http.StripPrefix(path, http.FileServer(http.Dir(filesystemPath)))
}

func NewSecureProxy(opts *Options, validator func(string) bool) http.Handler {
bareproxy := NewOAuthProxy(opts, validator)
var err error

if len(opts.EmailDomains) != 0 && opts.AuthenticatedEmailsFile == "" {
if len(opts.EmailDomains) > 1 {
bareproxy.SignInMessage = fmt.Sprintf("Authenticate using one of the following domains: %v", strings.Join(opts.EmailDomains, ", "))
} else if opts.EmailDomains[0] != "*" {
bareproxy.SignInMessage = fmt.Sprintf("Authenticate using %v", opts.EmailDomains[0])
}
}

if opts.HtpasswdFile != "" {
log.Printf("using htpasswd file %s", opts.HtpasswdFile)
bareproxy.HtpasswdFile, err = NewHtpasswdFromFile(opts.HtpasswdFile)
bareproxy.DisplayHtpasswdForm = opts.DisplayHtpasswdForm
if err != nil {
log.Fatalf("FATAL: unable to open %s %s", opts.HtpasswdFile, err)
}
}

secureMiddleware := secure.New(secure.Options{
AllowedHosts: opts.HttpAllowedHosts,
HostsProxyHeaders: opts.HttpHostsProxyHeaders,
SSLRedirect: opts.HttpSSLRedirect,
SSLTemporaryRedirect: opts.HttpSSLTemporaryRedirect,
SSLHost: opts.HttpSSLHost,
STSSeconds: opts.HttpSTSSeconds,
STSIncludeSubdomains: opts.HttpSTSIncludeSubdomains,
STSPreload: opts.HttpSTSPreload,
ForceSTSHeader: opts.HttpForceSTSHeader,
FrameDeny: opts.HttpFrameDeny,
CustomFrameOptionsValue: opts.HttpCustomFrameOptionsValue,
ContentTypeNosniff: opts.HttpContentTypeNosniff,
BrowserXssFilter: opts.HttpBrowserXssFilter,
CustomBrowserXssValue: opts.HttpCustomBrowserXssValue,
ContentSecurityPolicy: opts.HttpContentSecurityPolicy,
PublicKey: opts.HttpPublicKey,
ReferrerPolicy: opts.HttpReferrerPolicy,
})
return secureMiddleware.Handler(bareproxy)
}

func NewOAuthProxy(opts *Options, validator func(string) bool) *OAuthProxy {
serveMux := http.NewServeMux()
var auth hmacauth.HmacAuth
Expand Down
50 changes: 50 additions & 0 deletions oauthproxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -836,3 +836,53 @@ func TestRequestSignaturePostRequest(t *testing.T) {
assert.Equal(t, 200, st.rw.Code)
assert.Equal(t, st.rw.Body.String(), "signatures match")
}

func TestHttpBrowserXssFilterTrue(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this tests a third-party package but doesn't really test the behavior of this package. What kinds of regressions would this test catch?

Copy link
Author

Choose a reason for hiding this comment

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

Well, other tests further up in the file test the proxy. This tests that the middleware is functioning on top of the proxy. I definitely don't have 100% coverage here. I only am testing one of the secure middleware options, for example. More tests certainly could be written!

These were sufficient for me to verify that the proxy with middleware worked for our purposes, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this tests any of the code you added. As far as I can tell, the test imports a third-party package, instantiates its middleware, and verifies that the middleware behaves as expected. It doesn't really exercise the code you added, or any of the code in this package.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's how main() does it too. Remember how you told me to wrap the middleware outside of NewOAuthProxy? This is the result. As far as I can see, this is the only way I can actually do this sort of thing. As a golang neophyte, I welcome being proven wrong.

opts := NewOptions()
opts.ClientID = "bazquux"
opts.ClientSecret = "foobar"
opts.CookieSecret = "xyzzyplugh"
opts.EmailDomains = []string{"*"}
opts.HttpBrowserXssFilter = true
opts.Validate()

proxy := NewSecureProxy(opts, func(string) bool { return true })
rw := httptest.NewRecorder()
req, _ := http.NewRequest("GET", "/", nil)
proxy.ServeHTTP(rw, req)
assert.Equal(t, "1; mode=block", rw.HeaderMap.Get("X-XSS-Protection"))
}

func TestHttpBrowserXssFilterFalseByDefault(t *testing.T) {
opts := NewOptions()
opts.ClientID = "bazquux"
opts.ClientSecret = "foobar"
opts.CookieSecret = "xyzzyplugh"
opts.EmailDomains = []string{"*"}
opts.Validate()

proxy := NewSecureProxy(opts, func(string) bool { return true })
rw := httptest.NewRecorder()
req, _ := http.NewRequest("GET", "/", nil)
proxy.ServeHTTP(rw, req)
assert.Equal(t, false, opts.HttpBrowserXssFilter)
assert.Equal(t, "", rw.HeaderMap.Get("X-XSS-Protection"))
}

func TestSecureRobotsTxt(t *testing.T) {
opts := NewOptions()
opts.ClientID = "bazquux"
opts.ClientSecret = "foobar"
opts.CookieSecret = "xyzzyplugh"
opts.EmailDomains = []string{"*"}
opts.HttpBrowserXssFilter = true
opts.Validate()

proxy := NewSecureProxy(opts, func(string) bool { return true })
rw := httptest.NewRecorder()
req, _ := http.NewRequest("GET", "/robots.txt", nil)
proxy.ServeHTTP(rw, req)
assert.Equal(t, 200, rw.Code)
assert.Equal(t, "User-agent: *\nDisallow: /", rw.Body.String())
assert.Equal(t, "1; mode=block", rw.HeaderMap.Get("X-XSS-Protection"))
}
19 changes: 19 additions & 0 deletions options.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,25 @@ type Options struct {

SignatureKey string `flag:"signature-key" cfg:"signature_key" env:"OAUTH2_PROXY_SIGNATURE_KEY"`

// These are options that allow you to tune various parameters for https://github.com/unrolled/secure
HttpAllowedHosts []string `flag:"httpAllowedHosts" cfg:"httpAllowedHosts"`
HttpHostsProxyHeaders []string `flag:"httpHostsProxyHeaders" cfg:"httpHostsProxyHeaders"`
HttpSSLRedirect bool `flag:"httpSSLRedirect" cfg:"httpSSLRedirect"`
HttpSSLTemporaryRedirect bool `flag:"httpSSLTemporaryRedirect" cfg:"httpSSLTemporaryRedirect"`
HttpSSLHost string `flag:"httpSSLHost" cfg:"httpSSLHost"`
HttpSTSSeconds int64 `flag:"httpSTSSeconds" cfg:"httpSTSSeconds"`
HttpSTSIncludeSubdomains bool `flag:"httpSTSIncludeSubdomains" cfg:"httpSTSIncludeSubdomains"`
HttpSTSPreload bool `flag:"httpSTSPreload" cfg:"httpSTSPreload"`
HttpForceSTSHeader bool `flag:"httpForceSTSHeader" cfg:"httpForceSTSHeader"`
HttpFrameDeny bool `flag:"httpFrameDeny" cfg:"httpFrameDeny"`
HttpCustomFrameOptionsValue string `flag:"httpCustomFrameOptionsValue" cfg:"httpCustomFrameOptionsValue"`
HttpContentTypeNosniff bool `flag:"httpContentTypeNosniff" cfg:"httpContentTypeNosniff"`
HttpBrowserXssFilter bool `flag:"httpBrowserXssFilter" cfg:"httpBrowserXssFilter"`
HttpCustomBrowserXssValue string `flag:"httpCustomBrowserXssValue" cfg:"httpCustomBrowserXssValue"`
HttpContentSecurityPolicy string `flag:"httpContentSecurityPolicy" cfg:"httpContentSecurityPolicy"`
HttpPublicKey string `flag:"httpPublicKey" cfg:"httpPublicKey"`
HttpReferrerPolicy string `flag:"httpReferrerPolicy" cfg:"httpReferrerPolicy"`

// internal values that are set after config validation
redirectURL *url.URL
proxyURLs []*url.URL
Expand Down
2 changes: 1 addition & 1 deletion test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ for pkg in $(go list ./... | grep -v '/vendor/' ); do
echo "go test -v -race $pkg"
GOMAXPROCS=4 go test -v -timeout 90s0s -race "$pkg" || EXIT_CODE=1
done
exit $EXIT_CODE
exit $EXIT_CODE
2 changes: 1 addition & 1 deletion watcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"path/filepath"
"time"

"gopkg.in/fsnotify.v1"
"github.com/fsnotify/fsnotify"
)

func WaitForReplacement(filename string, op fsnotify.Op,
Expand Down