Merge pull request #61166 from hzxuzhonghu/authz-flag-validate

Automatic merge from submit-queue (batch tested with PRs 60519, 61099, 61218, 61166, 61714). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kube-apiserver add BuiltInAuthorizationOptions validation

Validate BuiltInAuthorizationOptions after flags parsed. 

**Release note**:

```release-note
NONE
```

pkg/kubeapiserver/options/BUILD

@@ -90,8 +90,12 @@ go_test(
     name = "go_default_test",
     srcs = [
         "admission_test.go",
+        "authorization_test.go",
         "storage_versions_test.go",
     ],
     embed = [":go_default_library"],
-    deps = ["//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library"],
+    deps = [
+        "//pkg/kubeapiserver/authorizer/modes:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
+    ],
 )

pkg/kubeapiserver/options/authentication.go

@@ -25,6 +25,7 @@ import (
 	"github.com/golang/glog"
 	"github.com/spf13/pflag"
 
+	"k8s.io/apimachinery/pkg/util/sets"
 	genericapiserver "k8s.io/apiserver/pkg/server"
 	genericoptions "k8s.io/apiserver/pkg/server/options"
 	"k8s.io/kubernetes/pkg/kubeapiserver/authenticator"
@@ -365,17 +366,8 @@ func (o *BuiltInAuthenticationOptions) ApplyAuthorization(authorization *BuiltIn
 
 	// authorization ModeAlwaysAllow cannot be combined with AnonymousAuth.
 	// in such a case the AnonymousAuth is stomped to false and you get a message
-	if o.Anonymous.Allow {
-		found := false
-		for _, mode := range strings.Split(authorization.Mode, ",") {
-			if mode == authzmodes.ModeAlwaysAllow {
-				found = true
-				break
-			}
-		}
-		if found {
-			glog.Warningf("AnonymousAuth is not allowed with the AllowAll authorizer.  Resetting AnonymousAuth to false. You should use a different authorizer")
-			o.Anonymous.Allow = false
-		}
+	if o.Anonymous.Allow && sets.NewString(authorization.Modes...).Has(authzmodes.ModeAlwaysAllow) {
+		glog.Warningf("AnonymousAuth is not allowed with the AlwaysAllow authorizer. Resetting AnonymousAuth to false. You should use a different authorizer")
+		o.Anonymous.Allow = false
 	}
 }

pkg/kubeapiserver/options/authorization.go

@@ -17,11 +17,13 @@ limitations under the License.
 package options
 
 import (
+	"fmt"
 	"strings"
 	"time"
 
 	"github.com/spf13/pflag"
 
+	"k8s.io/apimachinery/pkg/util/sets"
 	versionedinformers "k8s.io/client-go/informers"
 	informers "k8s.io/kubernetes/pkg/client/informers/informers_generated/internalversion"
 	"k8s.io/kubernetes/pkg/kubeapiserver/authorizer"
@@ -29,7 +31,7 @@ import (
 )
 
 type BuiltInAuthorizationOptions struct {
-	Mode                        string
+	Modes                       []string
 	PolicyFile                  string
 	WebhookConfigFile           string
 	WebhookCacheAuthorizedTTL   time.Duration
@@ -38,19 +40,57 @@ type BuiltInAuthorizationOptions struct {
 
 func NewBuiltInAuthorizationOptions() *BuiltInAuthorizationOptions {
 	return &BuiltInAuthorizationOptions{
-		Mode: authzmodes.ModeAlwaysAllow,
+		Modes: []string{authzmodes.ModeAlwaysAllow},
 		WebhookCacheAuthorizedTTL:   5 * time.Minute,
 		WebhookCacheUnauthorizedTTL: 30 * time.Second,
 	}
 }
 
 func (s *BuiltInAuthorizationOptions) Validate() []error {
+	if s == nil {
+		return nil
+	}
 	allErrors := []error{}
+
+	if len(s.Modes) == 0 {
+		allErrors = append(allErrors, fmt.Errorf("at least one authorization-mode must be passed"))
+	}
+
+	allowedModes := sets.NewString(authzmodes.AuthorizationModeChoices...)
+	modes := sets.NewString(s.Modes...)
+	for _, mode := range s.Modes {
+		if !allowedModes.Has(mode) {
+			allErrors = append(allErrors, fmt.Errorf("authorization-mode %q is not a valid mode", mode))
+		}
+		if mode == authzmodes.ModeABAC {
+			if s.PolicyFile == "" {
+				allErrors = append(allErrors, fmt.Errorf("authorization-mode ABAC's authorization policy file not passed"))
+			}
+		}
+		if mode == authzmodes.ModeWebhook {
+			if s.WebhookConfigFile == "" {
+				allErrors = append(allErrors, fmt.Errorf("authorization-mode Webhook's authorization config file not passed"))
+			}
+		}
+	}
+
+	if s.PolicyFile != "" && !modes.Has(authzmodes.ModeABAC) {
+		allErrors = append(allErrors, fmt.Errorf("cannot specify --authorization-policy-file without mode ABAC"))
+	}
+
+	if s.WebhookConfigFile != "" && !modes.Has(authzmodes.ModeWebhook) {
+		allErrors = append(allErrors, fmt.Errorf("cannot specify --authorization-webhook-config-file without mode Webhook"))
+	}
+
+	if len(s.Modes) != len(modes.List()) {
+		allErrors = append(allErrors, fmt.Errorf("authorization-mode %q has mode specified more than once", s.Modes))
+	}
+
 	return allErrors
 }
 
 func (s *BuiltInAuthorizationOptions) AddFlags(fs *pflag.FlagSet) {
-	fs.StringVar(&s.Mode, "authorization-mode", s.Mode, ""+
+	fs.StringSliceVar(&s.Modes, "authorization-mode", s.Modes, ""+
 		"Ordered list of plug-ins to do authorization on secure port. Comma-delimited list of: "+
 		strings.Join(authzmodes.AuthorizationModeChoices, ",")+".")
 
@@ -70,17 +110,9 @@ func (s *BuiltInAuthorizationOptions) AddFlags(fs *pflag.FlagSet) {
 		"The duration to cache 'unauthorized' responses from the webhook authorizer.")
 }
 
-func (s *BuiltInAuthorizationOptions) Modes() []string {
-	modes := []string{}
-	if len(s.Mode) > 0 {
-		modes = strings.Split(s.Mode, ",")
-	}
-	return modes
-}
-
 func (s *BuiltInAuthorizationOptions) ToAuthorizationConfig(informerFactory informers.SharedInformerFactory, versionedInformerFactory versionedinformers.SharedInformerFactory) authorizer.AuthorizationConfig {
 	return authorizer.AuthorizationConfig{
-		AuthorizationModes:          s.Modes(),
+		AuthorizationModes:          s.Modes,
 		PolicyFile:                  s.PolicyFile,
 		WebhookConfigFile:           s.WebhookConfigFile,
 		WebhookCacheAuthorizedTTL:   s.WebhookCacheAuthorizedTTL,

pkg/kubeapiserver/options/authorization_test.go

@@ -0,0 +1,104 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package options
+
+import (
+	"testing"
+
+	"k8s.io/kubernetes/pkg/kubeapiserver/authorizer/modes"
+)
+
+func TestAuthzValidate(t *testing.T) {
+	examplePolicyFile := "../../auth/authorizer/abac/example_policy_file.jsonl"
+
+	testCases := []struct {
+		name              string
+		modes             []string
+		policyFile        string
+		webhookConfigFile string
+		expectErr         bool
+	}{
+		{
+			name:      "Unknown modes should return errors",
+			modes:     []string{"DoesNotExist"},
+			expectErr: true,
+		},
+		{
+			name:      "At least one authorizationMode is necessary",
+			modes:     []string{},
+			expectErr: true,
+		},
+		{
+			name:      "ModeAlwaysAllow and ModeAlwaysDeny should return without authorizationPolicyFile",
+			modes:     []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny},
+			expectErr: false,
+		},
+		{
+			name:      "ModeABAC requires a policy file",
+			modes:     []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny, modes.ModeABAC},
+			expectErr: true,
+		},
+		{
+			name:              "Authorization Policy file cannot be used without ModeABAC",
+			modes:             []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny},
+			policyFile:        examplePolicyFile,
+			webhookConfigFile: "",
+			expectErr:         true,
+		},
+		{
+			name:              "ModeABAC should not error if a valid policy path is provided",
+			modes:             []string{modes.ModeAlwaysAllow, modes.ModeAlwaysDeny, modes.ModeABAC},
+			policyFile:        examplePolicyFile,
+			webhookConfigFile: "",
+			expectErr:         false,
+		},
+		{
+			name:      "ModeWebhook requires a config file",
+			modes:     []string{modes.ModeWebhook},
+			expectErr: true,
+		},
+		{
+			name:              "Cannot provide webhook config file without ModeWebhook",
+			modes:             []string{modes.ModeAlwaysAllow},
+			webhookConfigFile: "authz_webhook_config.yaml",
+			expectErr:         true,
+		},
+		{
+			name:              "ModeWebhook should not error if a valid config file is provided",
+			modes:             []string{modes.ModeWebhook},
+			webhookConfigFile: "authz_webhook_config.yaml",
+			expectErr:         false,
+		},
+	}
+
+	for _, testcase := range testCases {
+		t.Run(testcase.name, func(t *testing.T) {
+			options := NewBuiltInAuthorizationOptions()
+			options.Modes = testcase.modes
+			options.WebhookConfigFile = testcase.webhookConfigFile
+			options.PolicyFile = testcase.policyFile
+
+			errs := options.Validate()
+			if len(errs) > 0 && !testcase.expectErr {
+				t.Errorf("got unexpected err %v", errs)
+			}
+			if testcase.expectErr && len(errs) == 0 {
+				t.Errorf("should return an error")
+			}
+		})
+	}
+}