Add Validation for External OAuth Config

openshift · Mar 19, 2018 · 18cca83 · 18cca83
1 parent da36041
commit 18cca83
Show file tree

Hide file tree

Showing 3 changed files with 127 additions and 1 deletion.
diff --git a/pkg/cmd/server/apis/config/validation/master.go b/pkg/cmd/server/apis/config/validation/master.go
@@ -25,6 +25,7 @@ import (
 	configapi "github.com/openshift/origin/pkg/cmd/server/apis/config"
 	"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
 	"github.com/openshift/origin/pkg/cmd/server/cm"
+	oauthutil "github.com/openshift/origin/pkg/oauth/util"
 	"github.com/openshift/origin/pkg/security/mcs"
 	"github.com/openshift/origin/pkg/security/uid"
 	"github.com/openshift/origin/pkg/util/labelselector"
@@ -144,7 +145,12 @@ func ValidateMasterConfig(config *configapi.MasterConfig, fldPath *field.Path) V
 	if config.OAuthConfig != nil {
 		validationResults.Append(ValidateOAuthConfig(config.OAuthConfig, fldPath.Child("oauthConfig")))
 	}
-
+	if config.ExternalOAuthConfig != nil {
+		if config.OAuthConfig != nil {
+			validationResults.AddErrors(field.Invalid(fldPath.Child("externalOAuthConfig"), config.ExternalOAuthConfig, "Cannot specify External OAuth Config when the internal Oauth Server is configured"))
+		}
+		validationResults.Append(ValidateExternalOAuthConfig(config.ExternalOAuthConfig, fldPath.Child("externalOAuthConfig")))
+	}
 	validationResults.Append(ValidateServiceAccountConfig(config.ServiceAccountConfig, builtInKubernetes, fldPath.Child("serviceAccountConfig")))
 
 	validationResults.Append(ValidateHTTPServingInfo(config.ServingInfo, fldPath.Child("servingInfo")))
@@ -750,3 +756,19 @@ func ValidateDeprecatedClusterNetworkConfig(config *configapi.MasterConfig, fldP
 	}
 	return validationResults
 }
+
+func ValidateExternalOAuthConfig(config *configapi.ExternalOAuthConfig, fldPath *field.Path) ValidationResults {
+	validationResults := ValidationResults{}
+
+	_, err := oauthutil.LoadOAuthMetadataFile(config.MetadataFile)
+	if err != nil {
+		validationResults.AddErrors(field.Invalid(fldPath.Child("metadataFile"), config.MetadataFile, fmt.Sprintf("Metadata validation failed: %v", err)))
+	}
+	if _, urlErrs := ValidateURL(config.MasterPublicURL, fldPath.Child("masterPublicURL")); len(urlErrs) > 0 {
+		validationResults.AddErrors(urlErrs...)
+	}
+	if _, urlErrs := ValidateURL(config.AssetPublicURL, fldPath.Child("assetPublicURL")); len(urlErrs) > 0 {
+		validationResults.AddErrors(urlErrs...)
+	}
+	return validationResults
+}
diff --git a/pkg/cmd/server/apis/config/validation/master_test.go b/pkg/cmd/server/apis/config/validation/master_test.go
@@ -611,3 +611,80 @@ func TestValidateMasterAuthConfig(t *testing.T) {
 		}
 	}
 }
+
+var testMetadataContent = []byte(`{
+	"issuer": "https://127.0.0.1/",
+	"authorization_endpoint": "https://127.0.0.1/",
+	"token_endpoint": "https://127.0.0.1/",
+    "scopes_supported": ["openid", "profile", "email", "address", "phone", "offline_access"],
+    "response_types_supported": ["code", "code token"],
+	"grant_types_supported": ["authorization_code", "implicit"],
+	"code_challenge_methods_supported": ["plain", "S256"]}`)
+
+func TestValidateExternalOAuthConfig(t *testing.T) {
+	metadataFile, err := ioutil.TempFile("", "oauth.metadata")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	defer os.Remove(metadataFile.Name())
+	ioutil.WriteFile(metadataFile.Name(), testMetadataContent, os.FileMode(0644))
+	badMetadataFile, err := ioutil.TempFile("", "badoauth.metadata")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	defer os.Remove(badMetadataFile.Name())
+	ioutil.WriteFile(badMetadataFile.Name(), []byte("bad file"), os.FileMode(0644))
+	testCases := []struct {
+		name   string
+		config *configapi.ExternalOAuthConfig
+		errors []string
+	}{
+		{
+			name: "No Metadata file",
+			config: &configapi.ExternalOAuthConfig{
+				MetadataFile:    "NoFile",
+				MasterPublicURL: "https://127.0.0.1/",
+				AssetPublicURL:  "https://127.0.0.1/",
+			},
+			errors: []string{"Metadata validation failed: Unable to read External OAuth Metadata file: open NoFile: no such file or directory"},
+		},
+		{
+			name: "Bad Metadata file",
+			config: &configapi.ExternalOAuthConfig{
+				MetadataFile:    badMetadataFile.Name(),
+				MasterPublicURL: "https://127.0.0.1/",
+				AssetPublicURL:  "https://127.0.0.1/",
+			},
+			errors: []string{"Metadata validation failed: Unable to decode External OAuth Metadata file: invalid character 'b' looking for beginning of value"},
+		},
+		{
+			name: "Bad Master Public URL",
+			config: &configapi.ExternalOAuthConfig{
+				MetadataFile:    metadataFile.Name(),
+				MasterPublicURL: "bad",
+				AssetPublicURL:  "https://127.0.0.1/",
+			},
+			errors: []string{"must contain a scheme (e.g. https://)", "must contain a host"},
+		},
+		{
+			name: "Bad Asset Public URL",
+			config: &configapi.ExternalOAuthConfig{
+				MetadataFile:    metadataFile.Name(),
+				MasterPublicURL: "https://127.0.0.1/",
+				AssetPublicURL:  "bad",
+			},
+			errors: []string{"must contain a scheme (e.g. https://)", "must contain a host"},
+		},
+	}
+	for _, test := range testCases {
+		results := ValidateExternalOAuthConfig(test.config, nil)
+		actualErrors := sets.NewString()
+		expectedErrors := sets.NewString(test.errors...)
+		for i := range results.Errors {
+			actualErrors.Insert(results.Errors[i].Detail)
+		}
+		if !expectedErrors.Equal(actualErrors) {
+			t.Errorf("Expected errors: %v, actual errors: %v", expectedErrors, actualErrors)
+		}
+	}
+}
diff --git a/pkg/oauth/util/discovery.go b/pkg/oauth/util/discovery.go
@@ -4,6 +4,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"net/url"
 
 	"github.com/RangelReale/osin"
 	"github.com/openshift/origin/pkg/authorization/authorizer/scope"
@@ -56,6 +57,20 @@ func GetOauthMetadata(masterPublicURL string) OauthAuthorizationServerMetadata {
 	}
 }
 
+func validateURL(urlString string) error {
+	urlObj, err := url.Parse(urlString)
+	if err != nil {
+		return fmt.Errorf("%q is an invalid URL: %v", urlString, err)
+	}
+	if urlObj.Scheme != "https" {
+		return fmt.Errorf("must use https scheme")
+	}
+	if len(urlObj.Host) == 0 {
+		return fmt.Errorf("must contain a valid host")
+	}
+	return nil
+}
+
 func LoadOAuthMetadataFile(metadataFile string) ([]byte, error) {
 	data, err := ioutil.ReadFile(metadataFile)
 	if err != nil {
@@ -67,5 +82,17 @@ func LoadOAuthMetadataFile(metadataFile string) ([]byte, error) {
 		return nil, fmt.Errorf("Unable to decode External OAuth Metadata file: %v", err)
 	}
 
+	if err := validateURL(oauthMetadata.Issuer); err != nil {
+		return nil, fmt.Errorf("Error validating External OAuth Metadata Issuer field: %v", err)
+	}
+
+	if err := validateURL(oauthMetadata.AuthorizationEndpoint); err != nil {
+		return nil, fmt.Errorf("Error validating External OAuth Metadata AuthorizationEndpoint field: %v", err)
+	}
+
+	if err := validateURL(oauthMetadata.TokenEndpoint); err != nil {
+		return nil, fmt.Errorf("Error validating External OAuth Metadata TokenEndpoint field: %v", err)
+	}
+
 	return data, nil
 }