From 4e1c99c8976b2f73ec9fb39a55210487f952964a Mon Sep 17 00:00:00 2001
From: "github-actions[bot]"
 <41898282+github-actions[bot]@users.noreply.github.com>
Date: Fri, 7 Aug 2020 12:58:17 -0700
Subject: [PATCH] authorize: add databroker url check (#1228) (#1231)

Signed-off-by: Bobby DeSimone <bobbydesimone@gmail.com>

Co-authored-by: bobby <1544881+desimone@users.noreply.github.com>
---
 authenticate/authenticate.go |  3 ++
 authorize/authorize.go       |  7 +++--
 authorize/authorize_test.go  | 57 ++++++++++++++++++++++++------------
 cache/cache_test.go          |  3 +-
 4 files changed, 47 insertions(+), 23 deletions(-)

diff --git a/authenticate/authenticate.go b/authenticate/authenticate.go
index 1e39fa118..7c343f0c4 100644
--- a/authenticate/authenticate.go
+++ b/authenticate/authenticate.go
@@ -56,6 +56,9 @@ func ValidateOptions(o *config.Options) error {
 	if o.AuthenticateCallbackPath == "" {
 		return errors.New("authenticate: 'AUTHENTICATE_CALLBACK_PATH' is required")
 	}
+	if err := urlutil.ValidateURL(o.DataBrokerURL); err != nil {
+		return fmt.Errorf("authenticate: invalid 'DATABROKER_SERVICE_URL': %w", err)
+	}
 	return nil
 }
 
diff --git a/authorize/authorize.go b/authorize/authorize.go
index 8d51f8563..bfa4b9505 100644
--- a/authorize/authorize.go
+++ b/authorize/authorize.go
@@ -105,10 +105,13 @@ func New(opts *config.Options) (*Authorize, error) {
 
 func validateOptions(o *config.Options) error {
 	if _, err := cryptutil.NewAEADCipherFromBase64(o.SharedKey); err != nil {
-		return fmt.Errorf("bad shared_secret: %w", err)
+		return fmt.Errorf("authorize: bad 'SHARED_SECRET': %w", err)
 	}
 	if err := urlutil.ValidateURL(o.AuthenticateURL); err != nil {
-		return fmt.Errorf("invalid 'AUTHENTICATE_SERVICE_URL': %w", err)
+		return fmt.Errorf("authorize: invalid 'AUTHENTICATE_SERVICE_URL': %w", err)
+	}
+	if err := urlutil.ValidateURL(o.DataBrokerURL); err != nil {
+		return fmt.Errorf("authorize: invalid 'DATABROKER_SERVICE_URL': %w", err)
 	}
 	return nil
 }
diff --git a/authorize/authorize_test.go b/authorize/authorize_test.go
index e36c75c20..1d083d0ea 100644
--- a/authorize/authorize_test.go
+++ b/authorize/authorize_test.go
@@ -1,6 +1,7 @@
 package authorize
 
 import (
+	"net/url"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -13,31 +14,49 @@ func TestNew(t *testing.T) {
 	t.Parallel()
 	policies := testPolicies(t)
 	tests := []struct {
-		name      string
-		SharedKey string
-		Policies  []config.Policy
-		wantErr   bool
+		name    string
+		config  config.Options
+		wantErr bool
 	}{
-		{"good", "gXK6ggrlIW2HyKyUF9rUO4azrDgxhDPWqw9y+lJU7B8=", policies, false},
-		{"bad shared secret", "AZA85podM73CjLCjViDNz1EUvvejKpWp7Hysr0knXA==", policies, true},
-		{"really bad shared secret", "sup", policies, true},
-		{"validation error, short secret", "AZA85podM73CjLCjViDNz1EUvvejKpWp7Hysr0knXA==", policies, true},
-		{"empty options", "", []config.Policy{}, true}, // special case
-
+		{"good",
+			config.Options{
+				AuthenticateURL: mustParseURL("https://authN.example.com"),
+				DataBrokerURL:   mustParseURL("https://cache.example.com"),
+				SharedKey:       "2p/Wi2Q6bYDfzmoSEbKqYKtg+DUoLWTEHHs7vOhvL7w=",
+				Policies:        policies},
+			false},
+		{"bad shared secret",
+			config.Options{
+				AuthenticateURL: mustParseURL("https://authN.example.com"),
+				DataBrokerURL:   mustParseURL("https://cache.example.com"),
+				SharedKey:       "AZA85podM73CjLCjViDNz1EUvvejKpWp7Hysr0knXA==",
+				Policies:        policies}, true},
+		{"really bad shared secret",
+			config.Options{
+				AuthenticateURL: mustParseURL("https://authN.example.com"),
+				DataBrokerURL:   mustParseURL("https://cache.example.com"),
+				SharedKey:       "sup",
+				Policies:        policies}, true},
+		{"validation error, short secret",
+			config.Options{
+				AuthenticateURL: mustParseURL("https://authN.example.com"),
+				DataBrokerURL:   mustParseURL("https://cache.example.com"),
+				SharedKey:       "AZA85podM73CjLCjViDNz1EUvvejKpWp7Hysr0knXA==",
+				Policies:        policies}, true},
+		{"empty options", config.Options{}, true},
+		{"bad cache url",
+			config.Options{
+				AuthenticateURL: mustParseURL("https://authN.example.com"),
+				DataBrokerURL:   &url.URL{},
+				SharedKey:       "AZA85podM73CjLCjViDNz1EUvvejKpWp7Hysr0knXA==",
+				Policies:        policies},
+			true},
 	}
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			o := &config.Options{
-				AuthenticateURL: mustParseURL("https://authN.example.com"),
-				DataBrokerURL:   mustParseURL("https://cache.example.com"),
-				SharedKey:       tt.SharedKey,
-				Policies:        tt.Policies}
-			if tt.name == "empty options" {
-				o = &config.Options{}
-			}
-			_, err := New(o)
+			_, err := New(&tt.config)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("New() error = %v, wantErr %v", err, tt.wantErr)
 				return
diff --git a/cache/cache_test.go b/cache/cache_test.go
index e3c7cca94..dccefdd69 100644
--- a/cache/cache_test.go
+++ b/cache/cache_test.go
@@ -23,10 +23,9 @@ func TestNew(t *testing.T) {
 		opts    config.Options
 		wantErr bool
 	}{
-		{"good - autocache", config.Options{SharedKey: cryptutil.NewBase64Key(), DataBrokerURL: &url.URL{Scheme: "http", Host: "example"}}, false},
+		{"good", config.Options{SharedKey: cryptutil.NewBase64Key(), DataBrokerURL: &url.URL{Scheme: "http", Host: "example"}}, false},
 		{"bad shared secret", config.Options{SharedKey: string([]byte(cryptutil.NewBase64Key())[:31]), DataBrokerURL: &url.URL{Scheme: "http", Host: "example"}}, true},
 		{"bad cache url", config.Options{SharedKey: cryptutil.NewBase64Key(), DataBrokerURL: &url.URL{}}, true},
-		{"good - bolt", config.Options{SharedKey: cryptutil.NewBase64Key(), DataBrokerURL: &url.URL{Scheme: "http", Host: "example"}}, false},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {