From b15e83ee012a64ad521e2e1ad8bfc0c0917fb5e9 Mon Sep 17 00:00:00 2001 From: Mark Phelps <209477+markphelps@users.noreply.github.com> Date: Tue, 28 May 2024 14:51:31 -0400 Subject: [PATCH] chore: set raw claims if they exist in authz metadata (#3125) * chore: go mod tidy Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> * chore: set raw claims if they exist in authz metadata Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> * chore: fix authn oidc server test Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> * chore: skip authz on auth public server Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> * chore: log for debugging Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> --------- Signed-off-by: Mark Phelps <209477+markphelps@users.noreply.github.com> --- go.mod | 2 +- go.work.sum | 2 + internal/cmd/grpc.go | 2 +- internal/config/authentication.go | 13 ++-- internal/server/authn/method/metadata.go | 6 +- internal/server/authn/method/oidc/server.go | 23 +++---- .../server/authn/method/oidc/server_test.go | 30 ++++---- internal/server/authn/method/util.go | 47 ------------- internal/server/authn/public/server.go | 4 ++ internal/server/authz/engine.go | 2 + .../authz/middleware/grpc/middleware.go | 14 ++++ internal/server/authz/policies/default.json | 26 ------- .../server/authz/policies/policy.schema.json | 68 ------------------- internal/server/authz/policies/schema_test.go | 26 ------- 14 files changed, 59 insertions(+), 206 deletions(-) delete mode 100644 internal/server/authz/policies/default.json delete mode 100644 internal/server/authz/policies/policy.schema.json delete mode 100644 internal/server/authz/policies/schema_test.go diff --git a/go.mod b/go.mod index 6bb47846c5..212de7932f 100644 --- a/go.mod +++ b/go.mod @@ -50,7 +50,6 @@ require ( github.com/hashicorp/golang-lru/v2 v2.0.7 github.com/iancoleman/strcase v0.3.0 github.com/jackc/pgx/v5 v5.5.5 - github.com/jmespath/go-jmespath v0.4.0 github.com/libsql/libsql-client-go v0.0.0-20230917132930-48c310b27e7b github.com/magefile/mage v1.15.0 github.com/mattn/go-sqlite3 v1.14.22 @@ -205,6 +204,7 @@ require ( github.com/jackc/pgservicefile v0.0.0-20221227161230-091c0ba34f0a // indirect github.com/jackc/puddle/v2 v2.2.1 // indirect github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect + github.com/jmespath/go-jmespath v0.4.0 // indirect github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect github.com/kevinburke/ssh_config v1.2.0 // indirect github.com/klauspost/compress v1.17.8 // indirect diff --git a/go.work.sum b/go.work.sum index 6533d77834..db78e23a20 100644 --- a/go.work.sum +++ b/go.work.sum @@ -481,6 +481,7 @@ github.com/gabriel-vasile/mimetype v1.4.1/go.mod h1:05Vi0w3Y9c/lNvJOdmIwvrrAhX3r github.com/garyburd/redigo v0.0.0-20150301180006-535138d7bcd7/go.mod h1:NR3MbYisc3/PwhQ00EMzDiPmrwpPxAn5GI05/YaO1SY= github.com/getsentry/raven-go v0.2.0/go.mod h1:KungGk8q33+aIAZUIVWZDr2OfAEBsO49PX4NzFV5kcQ= github.com/ghodss/yaml v0.0.0-20150909031657-73d445a93680/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04= +github.com/ghodss/yaml v1.0.0 h1:wQHKEahhL6wmXdzwWG11gIVCkOv05bNOh+Rxn0yngAk= github.com/go-ini/ini v1.25.4/go.mod h1:ByCAeIL28uOIIG0E3PJtZPDL8WnHpFKFOtgjp+3Ies8= github.com/go-kit/log v0.2.0/go.mod h1:NwTd00d/i8cPZ3xOwwiv2PO5MOcx78fFErGNcVmBjv0= github.com/go-kit/log v0.2.1/go.mod h1:NwTd00d/i8cPZ3xOwwiv2PO5MOcx78fFErGNcVmBjv0= @@ -550,6 +551,7 @@ github.com/google/go-containerregistry v0.5.1/go.mod h1:Ct15B4yir3PLOP5jsy0GNeYV github.com/google/go-github/v39 v39.2.0/go.mod h1:C1s8C5aCC9L+JXIYpJM5GYytdX52vC1bLvHEF1IhBrE= github.com/google/go-pkcs11 v0.2.1-0.20230907215043-c6f79328ddf9/go.mod h1:6eQoGcuNJpa7jnd5pMGdkSaQpNDYvPlXWMcjXXThLlY= github.com/google/gofuzz v1.1.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= +github.com/google/martian v2.1.0+incompatible h1:/CP5g8u/VJHijgedC/Legn3BAbAaWPgecwXBIDzw5no= github.com/google/martian/v3 v3.1.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0= github.com/google/pprof v0.0.0-20201023163331-3e6fc7fc9c4c/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= github.com/google/pprof v0.0.0-20201203190320-1bf35d6f28c2/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= diff --git a/internal/cmd/grpc.go b/internal/cmd/grpc.go index a5c23c5b32..41e32c3473 100644 --- a/internal/cmd/grpc.go +++ b/internal/cmd/grpc.go @@ -386,7 +386,7 @@ func NewGRPCServer( interceptors = append(interceptors, authzmiddlewaregrpc.AuthorizationRequiredInterceptor(logger, policyEngine, authzOpts...)) - logger.Debug("authorization required") + logger.Info("authorization middleware enabled") } // audit sinks configuration diff --git a/internal/config/authentication.go b/internal/config/authentication.go index 429b0a20a7..603951b6ac 100644 --- a/internal/config/authentication.go +++ b/internal/config/authentication.go @@ -469,13 +469,12 @@ func (a AuthenticationMethodOIDCConfig) validate() error { // AuthenticationOIDCProvider configures provider credentials type AuthenticationMethodOIDCProvider struct { - IssuerURL string `json:"issuerURL,omitempty" mapstructure:"issuer_url" yaml:"issuer_url,omitempty"` - ClientID string `json:"-,omitempty" mapstructure:"client_id" yaml:"-"` - ClientSecret string `json:"-" mapstructure:"client_secret" yaml:"-"` - RedirectAddress string `json:"redirectAddress,omitempty" mapstructure:"redirect_address" yaml:"redirect_address,omitempty"` - Scopes []string `json:"scopes,omitempty" mapstructure:"scopes" yaml:"scopes,omitempty"` - UsePKCE bool `json:"usePKCE,omitempty" mapstructure:"use_pkce" yaml:"use_pkce,omitempty"` - RoleAttributePath string `json:"roleAttributePath,omitempty" mapstructure:"role_attribute_path" yaml:"role_attribute_path,omitempty"` + IssuerURL string `json:"issuerURL,omitempty" mapstructure:"issuer_url" yaml:"issuer_url,omitempty"` + ClientID string `json:"-,omitempty" mapstructure:"client_id" yaml:"-"` + ClientSecret string `json:"-" mapstructure:"client_secret" yaml:"-"` + RedirectAddress string `json:"redirectAddress,omitempty" mapstructure:"redirect_address" yaml:"redirect_address,omitempty"` + Scopes []string `json:"scopes,omitempty" mapstructure:"scopes" yaml:"scopes,omitempty"` + UsePKCE bool `json:"usePKCE,omitempty" mapstructure:"use_pkce" yaml:"use_pkce,omitempty"` } func (a AuthenticationMethodOIDCProvider) validate() error { diff --git a/internal/server/authn/method/metadata.go b/internal/server/authn/method/metadata.go index 8a21169be3..91592ce564 100644 --- a/internal/server/authn/method/metadata.go +++ b/internal/server/authn/method/metadata.go @@ -1,7 +1,7 @@ package method const ( - StorageMetadataRole = "io.flipt.auth.role" - StorageMetadataEmail = "io.flipt.auth.email" - StorageMetadataName = "io.flipt.auth.name" + StorageMetadataClaims = "io.flipt.auth.claims" + StorageMetadataEmail = "io.flipt.auth.email" + StorageMetadataName = "io.flipt.auth.name" ) diff --git a/internal/server/authn/method/oidc/server.go b/internal/server/authn/method/oidc/server.go index a79ba784e7..5fa2848493 100644 --- a/internal/server/authn/method/oidc/server.go +++ b/internal/server/authn/method/oidc/server.go @@ -2,6 +2,7 @@ package oidc import ( "context" + "encoding/json" "fmt" "strings" "time" @@ -145,23 +146,19 @@ func (s *Server) Callback(ctx context.Context, req *auth.CallbackRequest) (_ *au storageMetadataOIDCProvider: req.Provider, } - cfg, err := s.configFor(req.Provider) - if err != nil { + rawClaims := make(map[string]interface{}) + if err := responseToken.IDToken().Claims(&rawClaims); err != nil { return nil, err } - if cfg.RoleAttributePath != "" { - rawClaims := make(map[string]interface{}) - if err := responseToken.IDToken().Claims(&rawClaims); err != nil { - return nil, err - } - - role, err := method.SearchJSONForAttr[string](cfg.RoleAttributePath, rawClaims) - if err != nil { - return nil, err - } + // marshal raw claims to JSON + rawClaimsJSON, err := json.Marshal(rawClaims) + if err != nil { + return nil, err + } - metadata[method.StorageMetadataRole] = role + if rawClaimsJSON != nil { + metadata[method.StorageMetadataClaims] = string(rawClaimsJSON) } // Extract custom claims diff --git a/internal/server/authn/method/oidc/server_test.go b/internal/server/authn/method/oidc/server_test.go index c19c413b31..da9f03d11c 100644 --- a/internal/server/authn/method/oidc/server_test.go +++ b/internal/server/authn/method/oidc/server_test.go @@ -109,11 +109,10 @@ func Test_Server_ImplicitFlow(t *testing.T) { Method: config.AuthenticationMethodOIDCConfig{ Providers: map[string]config.AuthenticationMethodOIDCProvider{ "google": { - IssuerURL: tp.Addr(), - ClientID: id, - ClientSecret: secret, - RedirectAddress: clientAddress, - RoleAttributePath: "contains(roles[*], 'admin') && 'admin' || contains(roles[*], 'editor') && 'editor' || 'viewer'", + IssuerURL: tp.Addr(), + ClientID: id, + ClientSecret: secret, + RedirectAddress: clientAddress, }, }, }, @@ -215,12 +214,11 @@ func Test_Server_PKCE(t *testing.T) { Method: config.AuthenticationMethodOIDCConfig{ Providers: map[string]config.AuthenticationMethodOIDCProvider{ "google": { - IssuerURL: tp.Addr(), - ClientID: id, - ClientSecret: secret, - RedirectAddress: clientAddress, - UsePKCE: true, - RoleAttributePath: "contains(roles[*], 'admin') && 'admin' || contains(roles[*], 'editor') && 'editor' || 'viewer'", + IssuerURL: tp.Addr(), + ClientID: id, + ClientSecret: secret, + RedirectAddress: clientAddress, + UsePKCE: true, }, }, }, @@ -346,15 +344,19 @@ func testOIDCFlow(t *testing.T, ctx context.Context, tpAddr, clientAddress strin assert.Empty(t, response.ClientToken) // middleware moves it to cookie assert.Equal(t, auth.Method_METHOD_OIDC, response.Authentication.Method) - assert.Equal(t, map[string]string{ + + for k, v := range map[string]string{ "io.flipt.auth.oidc.provider": "google", "io.flipt.auth.oidc.email": "mark@flipt.io", "io.flipt.auth.email": "mark@flipt.io", "io.flipt.auth.oidc.name": "Mark Phelps", "io.flipt.auth.name": "Mark Phelps", "io.flipt.auth.oidc.sub": "mark", - "io.flipt.auth.role": "admin", - }, response.Authentication.Metadata) + } { + assert.Equal(t, v, response.Authentication.Metadata[k]) + } + + assert.NotEmpty(t, response.Authentication.Metadata["io.flipt.auth.claims"]) // ensure expiry is set assert.NotNil(t, response.Authentication.ExpiresAt) diff --git a/internal/server/authn/method/util.go b/internal/server/authn/method/util.go index b72bb1430d..c9b25c7122 100644 --- a/internal/server/authn/method/util.go +++ b/internal/server/authn/method/util.go @@ -3,10 +3,6 @@ package method import ( "context" - "encoding/json" - "fmt" - - "github.com/jmespath/go-jmespath" "go.flipt.io/flipt/errors" "google.golang.org/grpc/metadata" ) @@ -30,46 +26,3 @@ func CallbackValidateState(ctx context.Context, state string) error { return nil } - -func SearchJSONForAttr[T ~string](attributePath string, data any) (T, error) { - v, err := searchJSONForAttr(attributePath, data) - if err != nil { - return "", err - } - return v.(T), nil -} - -func searchJSONForAttr(attributePath string, data any) (any, error) { - if attributePath == "" { - return "", fmt.Errorf("attribute path: %q", attributePath) - } - - if data == nil { - return "", fmt.Errorf("empty json, attribute path: %q", attributePath) - } - - // Copy the data to a new variable - var jsonData = data - - // If the data is a byte slice, try to unmarshal it into a JSON object - if dataBytes, ok := data.([]byte); ok { - // If the byte slice is empty, return an error - if len(dataBytes) == 0 { - return "", fmt.Errorf("empty json, attribute path: %q", attributePath) - } - - // Try to unmarshal the byte slice - if err := json.Unmarshal(dataBytes, &jsonData); err != nil { - return "", fmt.Errorf("%v: %w", "failed to unmarshal user info JSON response", err) - } - } - - // Search for the attribute in the JSON object - value, err := jmespath.Search(attributePath, jsonData) - if err != nil { - return "", fmt.Errorf("failed to search user info JSON response with provided path: %q: %w", attributePath, err) - } - - // Return the value and nil error - return value, nil -} diff --git a/internal/server/authn/public/server.go b/internal/server/authn/public/server.go index 6df8691b7f..efbaed92c2 100644 --- a/internal/server/authn/public/server.go +++ b/internal/server/authn/public/server.go @@ -49,3 +49,7 @@ 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 index fba79e0b4b..96ac49a717 100644 --- a/internal/server/authz/engine.go +++ b/internal/server/authz/engine.go @@ -109,6 +109,8 @@ func (e *Engine) IsAllowed(ctx context.Context, input map[string]interface{}) (b e.mu.RLock() defer e.mu.RUnlock() + e.logger.Debug("evaluating policy", zap.Any("input", input)) + results, err := e.query.Eval(ctx, rego.EvalInput(input)) if err != nil { return false, err diff --git a/internal/server/authz/middleware/grpc/middleware.go b/internal/server/authz/middleware/grpc/middleware.go index 0572e94582..0c5ed9f15a 100644 --- a/internal/server/authz/middleware/grpc/middleware.go +++ b/internal/server/authz/middleware/grpc/middleware.go @@ -73,6 +73,20 @@ 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/server/authz/policies/default.json b/internal/server/authz/policies/default.json deleted file mode 100644 index 630aefd8e7..0000000000 --- a/internal/server/authz/policies/default.json +++ /dev/null @@ -1,26 +0,0 @@ -{ - "version": "0.1.0", - "roles": [ - { - "name": "admin", - "rules": { - "*": ["*"] - } - }, - { - "name": "editor", - "rules": { - "create": ["flag", "segment"], - "read": ["*"], - "update": ["flag", "segment"], - "delete": ["flag", "segment"] - } - }, - { - "name": "viewer", - "rules": { - "read": ["*"] - } - } - ] -} diff --git a/internal/server/authz/policies/policy.schema.json b/internal/server/authz/policies/policy.schema.json deleted file mode 100644 index 860b80d992..0000000000 --- a/internal/server/authz/policies/policy.schema.json +++ /dev/null @@ -1,68 +0,0 @@ -{ - "$schema": "http://json-schema.org/draft-07/schema#", - "id": "flipt.authz.policy.schema.json", - "type": "object", - "title": "flipt-authz-policy-schema-v0", - "description": "Flipt authz policy definition.", - - "properties": { - "version": { - "type": "string", - "pattern": "^\\d+\\.\\d+\\.\\d+$" - }, - "roles": { - "type": "array", - "items": { - "type": "object", - "properties": { - "name": { - "type": "string" - }, - "rules": { - "type": "object", - "properties": { - "*": { - "$ref": "#/definitions/ruleArray" - }, - "create": { - "$ref": "#/definitions/ruleArray" - }, - "read": { - "$ref": "#/definitions/ruleArray" - }, - "update": { - "$ref": "#/definitions/ruleArray" - }, - "delete": { - "$ref": "#/definitions/ruleArray" - } - }, - "additionalProperties": false - } - }, - "required": ["name", "rules"], - "additionalProperties": false - } - } - }, - "required": ["version", "roles"], - "additionalProperties": false, - "definitions": { - "ruleArray": { - "type": "array", - "items": { - "type": "string", - "enum": [ - "distribution", - "flag", - "rollout", - "rule", - "segment", - "token", - "variant", - "*" - ] - } - } - } -} diff --git a/internal/server/authz/policies/schema_test.go b/internal/server/authz/policies/schema_test.go deleted file mode 100644 index c457d110fd..0000000000 --- a/internal/server/authz/policies/schema_test.go +++ /dev/null @@ -1,26 +0,0 @@ -package policies - -import ( - "encoding/json" - "os" - "testing" - - "github.com/santhosh-tekuri/jsonschema/v5" - "github.com/stretchr/testify/require" -) - -func TestJSONSchema(t *testing.T) { - schema, err := jsonschema.Compile("policy.schema.json") - require.NoError(t, err) - - file, err := os.ReadFile("default.json") - require.NoError(t, err) - - data := map[string]interface{}{} - - err = json.Unmarshal(file, &data) - require.NoError(t, err) - - err = schema.Validate(data) - require.NoError(t, err) -}