Merge pull request #22697 from stlaz/enj/i/tighten_oauth

Bug 1703595: oauth/user/authz: tighten API validation checks

Author: openshift-merge-robot

pkg/apiserver/authentication/oauth/tokenauthenticator.go

@@ -50,11 +50,10 @@ func (a *tokenAuthenticator) AuthenticateToken(ctx context.Context, name string)
 	if err != nil {
 		return nil, false, err
 	}
-	groupNames := make([]string, 0, len(groups)+len(user.Groups))
+	groupNames := make([]string, 0, len(groups))
 	for _, group := range groups {
 		groupNames = append(groupNames, group.Name)
 	}
-	groupNames = append(groupNames, user.Groups...)
 
 	return &kauthenticator.Response{
 		User: &kuser.DefaultInfo{

pkg/authorization/authorizer/scope/converter.go

@@ -483,12 +483,13 @@ func removeEscalatingResources(in rbacv1.PolicyRule) rbacv1.PolicyRule {
 }
 
 func ValidateScopeRestrictions(client *oauthapi.OAuthClient, scopes ...string) error {
+	if len(scopes) == 0 {
+		return fmt.Errorf("%s may not request unscoped tokens", client.Name)
+	}
+
 	if len(client.ScopeRestrictions) == 0 {
 		return nil
 	}
-	if len(scopes) == 0 {
-		return fmt.Errorf("%v may not request unscoped tokens", client.Name)
-	}
 
 	errs := []error{}
 	for _, scope := range scopes {
@@ -503,10 +504,6 @@ func ValidateScopeRestrictions(client *oauthapi.OAuthClient, scopes ...string) e
 func validateScopeRestrictions(client *oauthapi.OAuthClient, scope string) error {
 	errs := []error{}
 
-	if len(client.ScopeRestrictions) == 0 {
-		return nil
-	}
-
 	for _, restriction := range client.ScopeRestrictions {
 		if len(restriction.ExactValues) > 0 {
 			if err := validateLiteralScopeRestrictions(scope, restriction.ExactValues); err != nil {

pkg/authorization/authorizer/scope/validate_test.go

@@ -26,9 +26,10 @@ func TestValidateScopeRestrictions(t *testing.T) {
 			client: &oauthapi.OAuthClient{},
 		},
 		{
-			name:   "unrestricted allows none",
-			scopes: []string{},
-			client: &oauthapi.OAuthClient{},
+			name:           "missing scopes check precedes unrestricted",
+			scopes:         []string{},
+			client:         &oauthapi.OAuthClient{},
+			expectedErrors: []string{"may not request unscoped tokens"},
 		},
 		{
 			name:   "simple literal",

pkg/oauth/apis/oauth/validation/validation.go

@@ -12,6 +12,7 @@ import (
 	"k8s.io/apiserver/pkg/authentication/serviceaccount"
 	"k8s.io/kubernetes/pkg/apis/core/validation"
 
+	routev1 "github.com/openshift/api/route/v1"
 	authorizerscopes "github.com/openshift/origin/pkg/authorization/authorizer/scope"
 	oauthapi "github.com/openshift/origin/pkg/oauth/apis/oauth"
 	"github.com/openshift/origin/pkg/oauthserver/authenticator/password/bootstrap"
@@ -49,7 +50,7 @@ func ValidateTokenName(name string, prefix bool) []string {
 
 func ValidateRedirectURI(redirect string) (bool, string) {
 	if len(redirect) == 0 {
-		return true, ""
+		return false, "may not be empty"
 	}
 
 	u, err := url.Parse(redirect)
@@ -81,8 +82,7 @@ func ValidateAccessToken(accessToken *oauthapi.OAuthAccessToken) field.ErrorList
 	}
 	// negative values are not allowed
 	if accessToken.InactivityTimeoutSeconds < 0 {
-		allErrs = append(allErrs, field.Invalid(field.NewPath("inactivityTimeoutSeconds"),
-			accessToken.InactivityTimeoutSeconds, "cannot be a negative value"))
+		allErrs = append(allErrs, field.Invalid(field.NewPath("inactivityTimeoutSeconds"), accessToken.InactivityTimeoutSeconds, "cannot be a negative value"))
 	}
 	if ok, msg := ValidateRedirectURI(accessToken.RedirectURI); !ok {
 		allErrs = append(allErrs, field.Invalid(field.NewPath("redirectURI"), accessToken.RedirectURI, msg))
@@ -178,22 +178,44 @@ func ValidateClient(client *oauthapi.OAuthClient) field.ErrorList {
 		}
 	}
 
+	for i, secret := range client.AdditionalSecrets {
+		if len(secret) == 0 {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("additionalSecrets").Index(i), "", "may not be empty"))
+		}
+	}
+
 	for i, restriction := range client.ScopeRestrictions {
 		allErrs = append(allErrs, ValidateScopeRestriction(restriction, field.NewPath("scopeRestrictions").Index(i))...)
 	}
 
+	if accessTokenMaxAgeSeconds := client.AccessTokenMaxAgeSeconds; accessTokenMaxAgeSeconds != nil {
+		if *accessTokenMaxAgeSeconds < 0 {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("accessTokenMaxAgeSeconds"), *accessTokenMaxAgeSeconds, "value cannot be negative"))
+		}
+	}
+
 	if client.AccessTokenInactivityTimeoutSeconds != nil {
 		timeout := *client.AccessTokenInactivityTimeoutSeconds
 		if timeout > 0 && timeout < MinimumInactivityTimeoutSeconds {
-			allErrs = append(allErrs, field.Invalid(
-				field.NewPath("accessTokenInactivityTimeoutSeconds"),
-				client.AccessTokenInactivityTimeoutSeconds,
-				fmt.Sprintf("The minimum valid timeout value is %d seconds", MinimumInactivityTimeoutSeconds)))
+			msg := fmt.Sprintf("The minimum valid timeout value is %d seconds", MinimumInactivityTimeoutSeconds)
+			allErrs = append(allErrs, field.Invalid(field.NewPath("accessTokenInactivityTimeoutSeconds"), timeout, msg))
 		}
 		if timeout < 0 {
-			allErrs = append(allErrs, field.Invalid(
-				field.NewPath("accessTokenInactivityTimeoutSeconds"),
-				client.AccessTokenInactivityTimeoutSeconds, "value cannot be negative"))
+			allErrs = append(allErrs, field.Invalid(field.NewPath("accessTokenInactivityTimeoutSeconds"), timeout, "value cannot be negative"))
+		}
+	}
+
+	if len(client.GrantMethod) == 0 {
+		allErrs = append(allErrs, field.Required(field.NewPath("grantMethod"),
+			fmt.Sprintf("must be %s or %s", oauthapi.GrantHandlerAuto, oauthapi.GrantHandlerPrompt)))
+	} else {
+		switch client.GrantMethod {
+		case oauthapi.GrantHandlerAuto, oauthapi.GrantHandlerPrompt:
+			// valid grant methods
+		default:
+			allErrs = append(allErrs, field.NotSupported(field.NewPath("grantMethod"), string(client.GrantMethod),
+				[]string{string(oauthapi.GrantHandlerAuto), string(oauthapi.GrantHandlerPrompt)},
+			))
 		}
 	}
 
@@ -329,6 +351,10 @@ func ValidateUserNameField(value string, fldPath *field.Path) field.ErrorList {
 func ValidateScopes(scopes []string, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 
+	if len(scopes) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath, "may not be empty"))
+	}
+
 	for i, scope := range scopes {
 		illegalCharacter := false
 		// https://tools.ietf.org/html/rfc6749#section-3.3 (full list of allowed chars is %x21 / %x23-5B / %x5D-7E)
@@ -370,26 +396,31 @@ func ValidateScopes(scopes []string, fldPath *field.Path) field.ErrorList {
 
 func ValidateOAuthRedirectReference(sref *oauthapi.OAuthRedirectReference) field.ErrorList {
 	allErrs := validation.ValidateObjectMeta(&sref.ObjectMeta, true, path.ValidatePathSegmentName, field.NewPath("metadata"))
-	return append(allErrs, validateRedirectReference(&sref.Reference)...)
+	return append(allErrs, validateRedirectReference(&sref.Reference, field.NewPath("reference"))...)
 }
 
-func validateRedirectReference(ref *oauthapi.RedirectReference) field.ErrorList {
+func validateRedirectReference(ref *oauthapi.RedirectReference, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if len(ref.Name) == 0 {
-		allErrs = append(allErrs, field.Required(field.NewPath("name"), "may not be empty"))
+		allErrs = append(allErrs, field.Required(fldPath.Child("name"), "may not be empty"))
 	} else {
 		for _, msg := range path.ValidatePathSegmentName(ref.Name, false) {
-			allErrs = append(allErrs, field.Invalid(field.NewPath("name"), ref.Name, msg))
+			allErrs = append(allErrs, field.Invalid(fldPath.Child("name"), ref.Name, msg))
 		}
 	}
 	switch ref.Kind {
 	case "":
-		allErrs = append(allErrs, field.Required(field.NewPath("kind"), "may not be empty"))
+		allErrs = append(allErrs, field.Required(fldPath.Child("kind"), "may not be empty"))
 	case "Route":
 		// Valid, TODO add ingress once we support it and update error message
 	default:
-		allErrs = append(allErrs, field.Invalid(field.NewPath("kind"), ref.Kind, "must be Route"))
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("kind"), ref.Kind, "must be Route"))
+	}
+	switch ref.Group {
+	case "", routev1.GroupName:
+	// valid group names
+	default:
+		allErrs = append(allErrs, field.NotSupported(fldPath.Child("group"), ref.Group, []string{"", routev1.GroupName}))
 	}
-	// TODO validate group once we start using it
 	return allErrs
 }