fix: Authz fixes (#3132)

* chore: go mod tidy Signed-off-by: Mark Phelps <[email protected]> * fix: authz endpoint skip for getauthself/deleteauthself Signed-off-by: Mark Phelps <[email protected]> * chore: rm claims unmarshal for now * chore: make authorization experimental Signed-off-by: Mark Phelps <[email protected]> * chore: add request methods to auth requests Signed-off-by: Mark Phelps <[email protected]> * chore: add schema * chore: set package name to flipt.authz.v1 * chore: fix telemetry test Signed-off-by: Mark Phelps <[email protected]> --------- Signed-off-by: Mark Phelps <[email protected]>
flipt-io · May 29, 2024 · 5837f9b · 5837f9b
1 parent b15e83e
commit 5837f9b
Show file tree

Hide file tree

Showing 20 changed files with 105 additions and 48 deletions.
diff --git a/config/flipt.schema.cue b/config/flipt.schema.cue
@@ -391,6 +391,9 @@ import "strings"
 	}
 
 	#experimental: {
+		authorization?: {
+			enabled?: bool | *false
+		}
 		cloud?: {
 			enabled?: bool | *false
 		}

diff --git a/config/flipt.schema.json b/config/flipt.schema.json
@@ -52,6 +52,9 @@
     },
     "ui": {
       "$ref": "#/definitions/ui"
+    },
+    "experimental": {
+      "$ref": "#/definitions/experimental"
     }
   },
 
@@ -348,7 +351,7 @@
               "type": "object",
               "additionalProperties": false,
               "properties": {
-                "path": {"type": "string"}
+                "path": { "type": "string" }
               }
             },
             "poll_duration": {
@@ -1324,6 +1327,33 @@
         }
       },
       "title": "Analytics"
+    },
+    "experimental": {
+      "type": "object",
+      "additionalProperties": false,
+      "properties": {
+        "cloud": {
+          "type": "object",
+          "additionalProperties": false,
+          "properties": {
+            "enabled": {
+              "type": "boolean",
+              "default": false
+            }
+          }
+        },
+        "authorization": {
+          "type": "object",
+          "additionalProperties": false,
+          "properties": {
+            "enabled": {
+              "type": "boolean",
+              "default": false
+            }
+          }
+        }
+      },
+      "title": "Experimental"
     }
   }
 }
diff --git a/internal/config/authorization.go b/internal/config/authorization.go
@@ -10,6 +10,7 @@ import (
 
 var (
 	_ defaulter = (*AuthorizationConfig)(nil)
+	_ validator = (*AuthorizationConfig)(nil)
 )
 
 // AuthorizationConfig configures Flipts authorization mechanisms

diff --git a/internal/config/config.go b/internal/config/config.go
@@ -57,7 +57,7 @@ type Config struct {
 	Version        string               `json:"version,omitempty" mapstructure:"version,omitempty" yaml:"version,omitempty"`
 	Audit          AuditConfig          `json:"audit,omitempty" mapstructure:"audit" yaml:"audit,omitempty"`
 	Authentication AuthenticationConfig `json:"authentication,omitempty" mapstructure:"authentication" yaml:"authentication,omitempty"`
-	Authorization  AuthorizationConfig  `json:"authorization,omitempty" mapstructure:"authorization" yaml:"authorization,omitempty"`
+	Authorization  AuthorizationConfig  `json:"authorization,omitempty" mapstructure:"authorization" yaml:"authorization,omitempty" experiment:"authorization"`
 	Cache          CacheConfig          `json:"cache,omitempty" mapstructure:"cache" yaml:"cache,omitempty"`
 	Cloud          CloudConfig          `json:"cloud,omitempty" mapstructure:"cloud" yaml:"cloud,omitempty" experiment:"cloud"`
 	Cors           CorsConfig           `json:"cors,omitempty" mapstructure:"cors" yaml:"cors,omitempty"`

diff --git a/internal/config/config_test.go b/internal/config/config_test.go
@@ -725,6 +725,8 @@ func TestLoad(t *testing.T) {
 						},
 					},
 				}
+
+				cfg.Experimental.Authorization.Enabled = true
 				return cfg
 			},
 		},
@@ -916,6 +918,7 @@ func TestLoad(t *testing.T) {
 					},
 				}
 
+				cfg.Experimental.Authorization.Enabled = true
 				return cfg
 			},
 		},

diff --git a/internal/config/experimental.go b/internal/config/experimental.go
@@ -7,7 +7,8 @@ import (
 // ExperimentalConfig allows for experimental features to be enabled
 // and disabled.
 type ExperimentalConfig struct {
-	Cloud ExperimentalFlag `json:"cloud,omitempty" mapstructure:"cloud" yaml:"cloud,omitempty"`
+	Cloud         ExperimentalFlag `json:"cloud,omitempty" mapstructure:"cloud" yaml:"cloud,omitempty"`
+	Authorization ExperimentalFlag `json:"authorization,omitempty" mapstructure:"authorization" yaml:"authorization,omitempty"`
 }
 
 func (c *ExperimentalConfig) deprecations(v *viper.Viper) []deprecated {

diff --git a/internal/config/testdata/advanced.yml b/internal/config/testdata/advanced.yml
@@ -118,3 +118,7 @@ authorization:
     backend: local
     local:
       path: "/path/to/policy/data.json"
+
+experimental:
+  authorization:
+    enabled: true
diff --git a/internal/config/testdata/authorization/all_authentication_methods_enabled.yml b/internal/config/testdata/authorization/all_authentication_methods_enabled.yml
@@ -46,3 +46,7 @@ authorization:
     local:
       path: "/path/to/policy.rego"
     poll_duration: 30s
+
+experimental:
+  authorization:
+    enabled: true
diff --git a/internal/config/testdata/authorization/authentication_not_required.yml b/internal/config/testdata/authorization/authentication_not_required.yml
@@ -6,5 +6,10 @@ authentication:
       bootstrap:
         token: "s3cr3t!"
         expiration: 24h
+
 authorization:
   required: true
+
+experimental:
+  authorization:
+    enabled: true
diff --git a/internal/config/testdata/authorization/authentication_not_session_enabled.yml b/internal/config/testdata/authorization/authentication_not_session_enabled.yml
@@ -3,5 +3,10 @@ authentication:
   methods:
     token:
       enabled: false
+
 authorization:
   required: true
+
+experimental:
+  authorization:
+    enabled: true
diff --git a/internal/server/authn/method/github/server.go b/internal/server/authn/method/github/server.go
@@ -85,10 +85,6 @@ func (s *Server) SkipsAuthentication(ctx context.Context) bool {
 	return true
 }
 
-func (s *Server) SkipsAuthorization(ctx context.Context) bool {
-	return true
-}
-
 func callbackURL(host string) string {
 	// strip trailing slash from host
 	host = strings.TrimSuffix(host, "/")

diff --git a/internal/server/authn/method/kubernetes/server.go b/internal/server/authn/method/kubernetes/server.go
@@ -79,10 +79,6 @@ func (s *Server) SkipsAuthentication(ctx context.Context) bool {
 	return true
 }
 
-func (s *Server) SkipsAuthorization(ctx context.Context) bool {
-	return true
-}
-
 // VerifyServiceAccount takes a service account token, configured by a kubernetes environment,
 // validates it's authenticity and (if valid) creates a Flipt client token and returns it.
 // The returned client token is valid for the lifetime of the service account JWT.

diff --git a/internal/server/authn/method/oidc/server.go b/internal/server/authn/method/oidc/server.go
@@ -83,10 +83,6 @@ func (s *Server) SkipsAuthentication(ctx context.Context) bool {
 	return true
 }
 
-func (s *Server) SkipsAuthorization(ctx context.Context) bool {
-	return true
-}
-
 // AuthorizeURL constructs and returns a URL directed at the requested OIDC provider
 // based on our internal oauth2 client configuration.
 // The operation is configured to return a URL which ultimately redirects to the

diff --git a/internal/server/authn/middleware/grpc/middleware.go b/internal/server/authn/middleware/grpc/middleware.go
@@ -79,14 +79,14 @@ type InterceptorOptions struct {
 	skippedServers []any
 }
 
-func skipped(ctx context.Context, server any, o InterceptorOptions) bool {
-	if skipSrv, ok := server.(SkipsAuthenticationServer); ok && skipSrv.SkipsAuthentication(ctx) {
+func skipped(ctx context.Context, info *grpc.UnaryServerInfo, o InterceptorOptions) bool {
+	if skipSrv, ok := info.Server.(SkipsAuthenticationServer); ok && skipSrv.SkipsAuthentication(ctx) {
 		return true
 	}
 
 	// TODO: refactor to remove this check
 	for _, s := range o.skippedServers {
-		if s == server {
+		if s == info.Server {
 			return true
 		}
 	}
@@ -122,7 +122,7 @@ func AuthenticationRequiredInterceptor(logger *zap.Logger, o ...containers.Optio
 
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		// skip auth for any preconfigured servers
-		if skipped(ctx, info.Server, opts) {
+		if skipped(ctx, info, opts) {
 			logger.Debug("skipping authentication for server", zap.String("method", info.FullMethod))
 			return handler(ctx, req)
 		}
@@ -157,7 +157,7 @@ func JWTAuthenticationInterceptor(logger *zap.Logger, validator jwt.Validator, e
 
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		// skip auth for any preconfigured servers
-		if skipped(ctx, info.Server, opts) {
+		if skipped(ctx, info, opts) {
 			logger.Debug("skipping authentication for server", zap.String("method", info.FullMethod))
 			return handler(ctx, req)
 		}
@@ -246,7 +246,7 @@ func ClientTokenAuthenticationInterceptor(logger *zap.Logger, authenticator Clie
 
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		// skip auth for any preconfigured servers
-		if skipped(ctx, info.Server, opts) {
+		if skipped(ctx, info, opts) {
 			logger.Debug("skipping authentication for server", zap.String("method", info.FullMethod))
 			return handler(ctx, req)
 		}
@@ -305,7 +305,7 @@ func EmailMatchingInterceptor(logger *zap.Logger, rgxs []*regexp.Regexp, o ...co
 
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		// skip auth for any preconfigured servers
-		if skipped(ctx, info.Server, opts) {
+		if skipped(ctx, info, opts) {
 			logger.Debug("skipping authentication for server", zap.String("method", info.FullMethod))
 			return handler(ctx, req)
 		}
@@ -349,7 +349,7 @@ func NamespaceMatchingInterceptor(logger *zap.Logger, o ...containers.Option[Int
 
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		// skip auth for any preconfigured servers
-		if skipped(ctx, info.Server, opts) {
+		if skipped(ctx, info, opts) {
 			logger.Debug("skipping authentication for server", zap.String("method", info.FullMethod))
 			return handler(ctx, req)
 		}

diff --git a/internal/server/authn/public/server.go b/internal/server/authn/public/server.go
@@ -49,7 +49,3 @@ func (s *Server) RegisterGRPC(server *grpc.Server) {
 func (s *Server) SkipsAuthentication(ctx context.Context) bool {
 	return true
 }
-
-func (s *Server) SkipsAuthorization(ctx context.Context) bool {
-	return true
-}
diff --git a/internal/server/authz/engine.go b/internal/server/authz/engine.go
@@ -151,7 +151,7 @@ func (e *Engine) updatePolicy(ctx context.Context) error {
 	e.policyHash = hash
 
 	r := rego.New(
-		rego.Query("data.authz.v1.allow"),
+		rego.Query("data.flipt.authz.v1.allow"),
 		rego.Module("policy.rego", string(policy)),
 		rego.Store(e.store),
 	)

diff --git a/internal/server/authz/engine_test.go b/internal/server/authz/engine_test.go
@@ -218,7 +218,7 @@ func (d dataSource) Get(context.Context, []byte) (data map[string]any, _ []byte,
 }
 
 var (
-	testRBACPolicy = policySource(`package authz.v1
+	testRBACPolicy = policySource(`package flipt.authz.v1
 
 import data
 import rego.v1

diff --git a/internal/server/authz/middleware/grpc/middleware.go b/internal/server/authz/middleware/grpc/middleware.go
@@ -25,14 +25,32 @@ type InterceptorOptions struct {
 	skippedServers []any
 }
 
-func skipped(ctx context.Context, server any, o InterceptorOptions) bool {
-	if skipSrv, ok := server.(SkipsAuthorizationServer); ok && skipSrv.SkipsAuthorization(ctx) {
+var (
+	// methods which should always skip authorization
+	skippedMethods = map[string]any{
+		"/flipt.auth.AuthenticationService/GetAuthenticationSelf":    struct{}{},
+		"/flipt.auth.AuthenticationService/ExpireAuthenticationSelf": struct{}{},
+	}
+)
+
+func skipped(ctx context.Context, info *grpc.UnaryServerInfo, o InterceptorOptions) bool {
+	// if we skip authentication then we must skip authorization
+	if skipSrv, ok := info.Server.(authmiddlewaregrpc.SkipsAuthenticationServer); ok && skipSrv.SkipsAuthentication(ctx) {
+		return true
+	}
+
+	if skipSrv, ok := info.Server.(SkipsAuthorizationServer); ok && skipSrv.SkipsAuthorization(ctx) {
+		return true
+	}
+
+	// skip authz for any preconfigured methods
+	if _, ok := skippedMethods[info.FullMethod]; ok {
 		return true
 	}
 
 	// TODO: refactor to remove this check
 	for _, s := range o.skippedServers {
-		if s == server {
+		if s == info.Server {
 			return true
 		}
 	}
@@ -56,7 +74,7 @@ func AuthorizationRequiredInterceptor(logger *zap.Logger, policyVerifier authz.V
 
 	return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
 		// skip authz for any preconfigured servers
-		if skipped(ctx, info.Server, opts) {
+		if skipped(ctx, info, opts) {
 			logger.Debug("skipping authorization for server", zap.String("method", info.FullMethod))
 			return handler(ctx, req)
 		}
@@ -73,20 +91,6 @@ func AuthorizationRequiredInterceptor(logger *zap.Logger, policyVerifier authz.V
 			return ctx, errUnauthorized
 		}
 
-		// unmarshal auth.metadata["io.flipt.auth.claims"] if present to make writing policies easier
-		// if auth.Metadata != nil {
-		// 	if claims, ok := auth.Metadata["io.flipt.auth.claims"]; ok {
-		// 		var claimsMap map[string]interface{}
-
-		// 		if err := json.Unmarshal([]byte(claims), &claimsMap); err != nil {
-		// 			logger.Error("unauthorized", zap.String("reason", "failed to unmarshal claims"))
-		// 			return ctx, errUnauthorized
-		// 		}
-
-		// 		auth.Metadata["io.flipt.auth.claims"] = claimsMap
-		// 	}
-		// }
-
 		allowed, err := policyVerifier.IsAllowed(ctx, map[string]interface{}{
 			"request":        requester.Request(),
 			"authentication": auth,

diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go
@@ -87,7 +87,8 @@ func TestShutdown(t *testing.T) {
 }
 
 var experimental = map[string]any{
-	"cloud": map[string]any{},
+	"cloud":         map[string]any{},
+	"authorization": map[string]any{},
 }
 
 func TestPing(t *testing.T) {

diff --git a/rpc/flipt/auth/request.go b/rpc/flipt/auth/request.go
@@ -7,3 +7,15 @@ import (
 func (req *CreateTokenRequest) Request() flipt.Request {
 	return flipt.NewRequest(flipt.ResourceAuthentication, flipt.ActionCreate, flipt.WithSubject(flipt.SubjectToken))
 }
+
+func (req *ListAuthenticationsRequest) Request() flipt.Request {
+	return flipt.NewRequest(flipt.ResourceAuthentication, flipt.ActionRead)
+}
+
+func (req *GetAuthenticationRequest) Request() flipt.Request {
+	return flipt.NewRequest(flipt.ResourceAuthentication, flipt.ActionRead)
+}
+
+func (req *DeleteAuthenticationRequest) Request() flipt.Request {
+	return flipt.NewRequest(flipt.ResourceAuthentication, flipt.ActionDelete)
+}
-Original file line number
+Diff line change
@@ Expand Up / @@ -10,6 +10,7 @@ import ( @@
     var (
     	_ defaulter = (*AuthorizationConfig)(nil)
+    	_ validator = (*AuthorizationConfig)(nil)
     )
     // AuthorizationConfig configures Flipts authorization mechanisms
@@ Expand Down @@