diff --git a/deployments/examples/ocis_keycloak/config/keycloak/ocis-realm.dist.json b/deployments/examples/ocis_keycloak/config/keycloak/ocis-realm.dist.json index a5e9187c112..63200a3ac07 100644 --- a/deployments/examples/ocis_keycloak/config/keycloak/ocis-realm.dist.json +++ b/deployments/examples/ocis_keycloak/config/keycloak/ocis-realm.dist.json @@ -1877,7 +1877,7 @@ "description": "OpenID Connect scope for add acr (authentication context class reference) to the token", "protocol": "openid-connect", "attributes": { - "include.in.token.scope": "false", + "include.in.token.scope": "true", "display.on.consent.screen": "false" }, "protocolMappers": [ @@ -2899,7 +2899,7 @@ "config": {} } ], - "browserFlow": "browser", + "browserFlow": "step up flow", "registrationFlow": "registration", "directGrantFlow": "direct grant", "resetCredentialsFlow": "reset credentials", diff --git a/deployments/examples/ocis_keycloak/docker-compose.yml b/deployments/examples/ocis_keycloak/docker-compose.yml index 5dc84447d8c..f9881577a3c 100644 --- a/deployments/examples/ocis_keycloak/docker-compose.yml +++ b/deployments/examples/ocis_keycloak/docker-compose.yml @@ -81,8 +81,8 @@ services: OCIS_PASSWORD_POLICY_BANNED_PASSWORDS_LIST: "banned-password-list.txt" PROXY_CSP_CONFIG_FILE_LOCATION: /etc/ocis/csp.yaml KEYCLOAK_DOMAIN: ${KEYCLOAK_DOMAIN:-keycloak.owncloud.test} - OCIS_MFA_ENABLED: "false" - OCIS_MFA_AUTH_LEVEL_NAME: "advanced" + OCIS_MFA_ENABLED: ${OCIS_MFA_ENABLED:-false} + WEB_OIDC_SCOPE: "openid profile email acr" volumes: - ./config/ocis/banned-password-list.txt:/etc/ocis/banned-password-list.txt - ./config/ocis/csp.yaml:/etc/ocis/csp.yaml diff --git a/ocis-pkg/mfa/mfa.go b/ocis-pkg/mfa/mfa.go index ee6d26f9474..3f38cd32411 100644 --- a/ocis-pkg/mfa/mfa.go +++ b/ocis-pkg/mfa/mfa.go @@ -1,4 +1,4 @@ -// package mfa provides functionality for multi-factor authentication (MFA). +// Package mfa provides functionality for multi-factor authentication (MFA). package mfa import ( @@ -8,10 +8,10 @@ import ( // MFAHeader is the header to be used across grpc and http services // to forward the access token. -const MFAHeader = "x-multi-factor-authentication" +const MFAHeader = "X-Multi-Factor-Authentication" // MFARequiredHeader is the header returned by the server if step-up authentication is required. -const MFARequiredHeader = "X-OCIS-MFA-Required" +const MFARequiredHeader = "X-Ocis-Mfa-Required" type mfaKeyType struct{} diff --git a/ocis-pkg/oidc/checkers/acrchecker.go b/ocis-pkg/oidc/checkers/acrchecker.go deleted file mode 100644 index 6c8c37b101d..00000000000 --- a/ocis-pkg/oidc/checkers/acrchecker.go +++ /dev/null @@ -1,41 +0,0 @@ -package checkers - -import ( - "fmt" - - "github.com/owncloud/ocis/v2/ocis-pkg/oidc" -) - -// AcrChecker check if the acr in the claims has the exact same value -// as the provided one -type AcrChecker struct { - value string -} - -// NewAcrChecker creates a new AcrChecker -func NewAcrChecker(value string) *AcrChecker { - return &AcrChecker{ - value: value, - } -} - -// CheckClaims checks if the provided value matches the acr value in the -// claims. It's an exact match. -func (ac *AcrChecker) CheckClaims(claims map[string]interface{}) error { - value, err := oidc.ReadStringClaim("acr", claims) - if err != nil { - return err - } - - if ac.value != value { - return fmt.Errorf("wrong value for 'acr' - expected '%s' actual '%s'", ac.value, value) - } - return nil -} - -func (ac *AcrChecker) RequireMap() map[string]string { - return map[string]string{ - "Type": "Acr", - "Data": "acr=" + ac.value, - } -} diff --git a/ocis-pkg/oidc/checkers/boolchecker.go b/ocis-pkg/oidc/checkers/boolchecker.go deleted file mode 100644 index a28f36d7d08..00000000000 --- a/ocis-pkg/oidc/checkers/boolchecker.go +++ /dev/null @@ -1,43 +0,0 @@ -package checkers - -import ( - "fmt" - "strconv" - - "github.com/owncloud/ocis/v2/ocis-pkg/oidc" -) - -// BooleanChecker checks whether the specified key has a matching boolean value -type BooleanChecker struct { - key string - value bool -} - -// NewBooleanChecker creates a new BooleanChecker -func NewBooleanChecker(key string, value bool) *BooleanChecker { - return &BooleanChecker{ - key: key, - value: value, - } -} - -// CheckClaims checks the claims so the key held by the BooleanChecker matches -// its boolean value. -func (bcc *BooleanChecker) CheckClaims(claims map[string]interface{}) error { - value, err := oidc.ReadBoolClaim(bcc.key, claims) - if err != nil { - return err - } - - if value != bcc.value { - return fmt.Errorf("wrong value for claim '%s' - expected '%t' actual '%t'", bcc.key, bcc.value, value) - } - return nil -} - -func (bcc *BooleanChecker) RequireMap() map[string]string { - return map[string]string{ - "Type": "Bool", - "Data": bcc.key + "=" + strconv.FormatBool(bcc.value), - } -} diff --git a/ocis-pkg/oidc/checkers/checker.go b/ocis-pkg/oidc/checkers/checker.go deleted file mode 100644 index 3b543861ec5..00000000000 --- a/ocis-pkg/oidc/checkers/checker.go +++ /dev/null @@ -1,30 +0,0 @@ -package checkers - -// Checker will allow different checks based on the OIDC claims. -// Each implementation will perform different checks. -type Checker interface { - // CheckClaims will check whether the claims match specific criteria. - // If the check passes, nil will be returned, otherwise a proper error - // will be returned instead. - CheckClaims(claims map[string]interface{}) error - // RequireMap returns a map with the expected headers in the failed - // response. The headers should have enough information for the client - // to know what's going on. - // Expected keys to be returned are "Type" and "Data". - // All the returned keys will be prepended with "X-OCIS--Requires-*" - // For example: - // {"X-OCIS-OIDC-Requires-Type": "Bool", "X-OCIS-OIDC-Requires-Data": "email_verified=true"} - // or - // {"X-OCIS-OIDC-Requires-Type": "Acr", "X-OCIS-OIDC-Requires-Data": "acr=advanced"} - // - // It's up to the client to decide what to do with those headers. - // Usually, if any "X-OCIS--Requires-*" header is received, the client - // might just show a popup with a message such as "not enough permissions - // to access to this resource", because the client might not be able - // to do anything to fix the problem. - // For the step up auth scenario, the client should be able to detect it - // with the headers ("X-OCIS-OIDC-Requires-Type": "Acr") and - // ("X-OCIS-OIDC-Requires-Data": "acr=advanced" - which means the acr should - // be advanced), and then act accordingly. - RequireMap() map[string]string -} diff --git a/ocis-pkg/oidc/checkers/factory.go b/ocis-pkg/oidc/checkers/factory.go deleted file mode 100644 index e44117dcca3..00000000000 --- a/ocis-pkg/oidc/checkers/factory.go +++ /dev/null @@ -1,57 +0,0 @@ -package checkers - -import ( - "strconv" - "strings" -) - -// Factory takes care of creating new claims checkers -type Factory struct { -} - -// NewFactory creates a new factory -func NewFactory() *Factory { - return &Factory{} -} - -// GetChecker gets a new claims checker based on the provided name, and -// configured with the provided parameter string. -// If the name is unknown or there are problems with the parameters, a -// NoopChecker will be returned instead. -func (f *Factory) GetChecker(name, paramString string) Checker { - params := f.parseParamString(paramString) - switch name { - case "Bool": - key, keyok := params["key"] - value, valueok := params["value"] - if keyok && valueok { - if boolValue, err := strconv.ParseBool(value); err == nil { - return NewBooleanChecker(key, boolValue) - } - } - case "Regexp": - key, keyok := params["key"] - pattern, patternok := params["value"] - if keyok && patternok { - return NewRegexpChecker(key, pattern) - } - case "Acr": - value, valueok := params["value"] - if valueok { - return NewAcrChecker(value) - } - } - return NewNoopChecker() -} - -func (f *Factory) parseParamString(paramString string) map[string]string { - params := make(map[string]string) - paramList := strings.Split(paramString, ";") - for _, keyvalue := range paramList { - p := strings.SplitN(keyvalue, "=", 2) - if len(p) == 2 { - params[p[0]] = p[1] - } - } - return params -} diff --git a/ocis-pkg/oidc/checkers/noopchecker.go b/ocis-pkg/oidc/checkers/noopchecker.go deleted file mode 100644 index 5ddd200ef76..00000000000 --- a/ocis-pkg/oidc/checkers/noopchecker.go +++ /dev/null @@ -1,19 +0,0 @@ -package checkers - -// NoopChecker doesn't check anything -type NoopChecker struct { -} - -// NewNoopChecker creates a new NoopChecker -func NewNoopChecker() *NoopChecker { - return &NoopChecker{} -} - -// CheckClaims won't do anything and won't return an error -func (nc *NoopChecker) CheckClaims(_ map[string]interface{}) error { - return nil -} - -func (nc *NoopChecker) RequireMap() map[string]string { - return nil -} diff --git a/ocis-pkg/oidc/checkers/regexpchecker.go b/ocis-pkg/oidc/checkers/regexpchecker.go deleted file mode 100644 index 670b31cee88..00000000000 --- a/ocis-pkg/oidc/checkers/regexpchecker.go +++ /dev/null @@ -1,44 +0,0 @@ -package checkers - -import ( - "fmt" - "regexp" - - "github.com/owncloud/ocis/v2/ocis-pkg/oidc" -) - -// RegexpChecker checks whether the specific key matches the provided regular -// expresion. -type RegexpChecker struct { - key string - exp *regexp.Regexp -} - -// NewRegexpChecker creates a new RegexpChecker -func NewRegexpChecker(key, pattern string) *RegexpChecker { - return &RegexpChecker{ - key: key, - exp: regexp.MustCompile(pattern), - } -} - -// CheckClaims checks in the claims if the claims key's value matches the -// provided regular expresion. If it doesn't match, an error is returned. -func (rc *RegexpChecker) CheckClaims(claims map[string]interface{}) error { - value, err := oidc.ReadStringClaim(rc.key, claims) - if err != nil { - return err - } - - if !rc.exp.MatchString(value) { - return fmt.Errorf("wrong value for claim '%s' - value '%s' doesn't match regular expresion '%s'", rc.key, value, rc.exp.String()) - } - return nil -} - -func (rc *RegexpChecker) RequireMap() map[string]string { - return map[string]string{ - "Type": "Regexp", - "Data": rc.key + "=" + rc.exp.String(), - } -} diff --git a/ocis-pkg/oidc/claims.go b/ocis-pkg/oidc/claims.go index 1e1d2896637..2eadcee2986 100644 --- a/ocis-pkg/oidc/claims.go +++ b/ocis-pkg/oidc/claims.go @@ -74,21 +74,3 @@ func ReadStringClaim(path string, claims map[string]interface{}) (string, error) return value, fmt.Errorf("claim path '%s' not set or empty", path) } - -func ReadBoolClaim(path string, claims map[string]interface{}) (bool, error) { - // check the simple case first - if value, ok := claims[path].(bool); ok { - return value, nil - } - - claim, err := WalkSegments(SplitWithEscaping(path, ".", "\\"), claims) - if err != nil { - return false, err - } - - if value, ok := claim.(bool); ok { - return value, nil - } - - return false, fmt.Errorf("claim path '%s' not set or not have a boolean value", path) -} diff --git a/services/frontend/pkg/config/config.go b/services/frontend/pkg/config/config.go index 73c58075a91..68050c85522 100644 --- a/services/frontend/pkg/config/config.go +++ b/services/frontend/pkg/config/config.go @@ -63,6 +63,8 @@ type Config struct { ServerManagedSpaces bool `yaml:"server_managed_spaces" env:"OCIS_CLAIM_MANAGED_SPACES_ENABLED" desc:"Enables Space management through OIDC claims. See the text description for more details." introductionVersion:"7.2.0"` + MultiFactorAuthentication MFAConfig `yaml:"mfa"` + Context context.Context `yaml:"-"` } @@ -200,3 +202,9 @@ type PasswordPolicy struct { type Validation struct { MaxTagLength int `yaml:"max_tag_length" env:"OCIS_MAX_TAG_LENGTH" desc:"Define the maximum tag length. Defaults to 100 if not set. Set to 0 to not limit the tag length. Changes only impact the validation of new tags." introductionVersion:"7.2.0"` } + +// MFAConfig configures multi factor multifactor authentication +type MFAConfig struct { + Enabled bool `yaml:"enabled" env:"OCIS_MFA_ENABLED" desc:"Set to true to enable multi factor authentication. See the documentation for more details." introductionVersion:"Balch"` + AuthLevelNames []string `yaml:"auth_level_names" env:"OCIS_MFA_AUTH_LEVEL_NAMES" desc:"A list of authentication level names that indicate that multi factor authentication has been performed. The names must match the acr claim in the access token. Web will use the first one in the list when requesting mfa." introductionVersion:"Balch"` +} diff --git a/services/frontend/pkg/config/defaults/defaultconfig.go b/services/frontend/pkg/config/defaults/defaultconfig.go index 692415eedfa..e601062a3d6 100644 --- a/services/frontend/pkg/config/defaults/defaultconfig.go +++ b/services/frontend/pkg/config/defaults/defaultconfig.go @@ -141,6 +141,9 @@ func DefaultConfig() *config.Config { Validation: config.Validation{ MaxTagLength: 100, }, + MultiFactorAuthentication: config.MFAConfig{ + AuthLevelNames: []string{"advanced"}, + }, } } diff --git a/services/frontend/pkg/revaconfig/config.go b/services/frontend/pkg/revaconfig/config.go index 0e002605bfe..2dcde1acf25 100644 --- a/services/frontend/pkg/revaconfig/config.go +++ b/services/frontend/pkg/revaconfig/config.go @@ -340,6 +340,12 @@ func FrontendConfigFromStruct(cfg *config.Config, logger log.Logger) (map[string "endpoints": []string{"list", "get", "delete"}, "configurable": cfg.ConfigurableNotifications, }, + "auth": map[string]interface{}{ + "mfa": map[string]interface{}{ + "enabled": cfg.MultiFactorAuthentication.Enabled, + "levelnames": cfg.MultiFactorAuthentication.AuthLevelNames, + }, + }, }, "version": map[string]interface{}{ "product": "Infinite Scale", diff --git a/services/graph/pkg/service/v0/graph_test.go b/services/graph/pkg/service/v0/graph_test.go index 86333c5faf7..4eea336a1f4 100644 --- a/services/graph/pkg/service/v0/graph_test.go +++ b/services/graph/pkg/service/v0/graph_test.go @@ -29,6 +29,7 @@ import ( "github.com/tidwall/gjson" "google.golang.org/grpc" + "github.com/owncloud/ocis/v2/ocis-pkg/mfa" "github.com/owncloud/ocis/v2/ocis-pkg/shared" v0 "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0" settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" @@ -116,19 +117,31 @@ var _ = Describe("Graph", func() { Expect(rr.Code).To(Equal(http.StatusOK)) }) - It("can list an empty list of all spaces", func() { + It("can list an empty list of all spaces when having 2fa", func() { gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Times(1).Return(&provider.ListStorageSpacesResponse{ Status: status.NewOK(ctx), StorageSpaces: []*provider.StorageSpace{}, }, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/drives", nil) - r = r.WithContext(ctx) + r = r.WithContext(mfa.Set(ctx, true)) rr := httptest.NewRecorder() svc.GetAllDrivesV1(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) }) + It("denies getting all spaces when not having 2fa", func() { + gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Times(1).Return(&provider.ListStorageSpacesResponse{ + Status: status.NewOK(ctx), + StorageSpaces: []*provider.StorageSpace{}, + }, nil) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/drives", nil) + rr := httptest.NewRecorder() + svc.GetAllDrivesV1(rr, r) + Expect(rr.Code).To(Equal(http.StatusForbidden)) + }) + It("can list a space without owner", func() { gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Times(1).Return(&provider.ListStorageSpacesResponse{ Status: status.NewOK(ctx), diff --git a/services/graph/pkg/service/v0/groups_test.go b/services/graph/pkg/service/v0/groups_test.go index 869a6892a38..7ee94ce4cf7 100644 --- a/services/graph/pkg/service/v0/groups_test.go +++ b/services/graph/pkg/service/v0/groups_test.go @@ -15,6 +15,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" libregraph "github.com/owncloud/libre-graph-api-go" + "github.com/owncloud/ocis/v2/ocis-pkg/mfa" "github.com/owncloud/ocis/v2/ocis-pkg/shared" settingsmsg "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0" settings "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" @@ -108,9 +109,10 @@ var _ = Describe("Groups", func() { Constraint: settingsmsg.Permission_CONSTRAINT_ALL, }, }, nil) - identityBackend.On("GetGroups", ctx, mock.Anything).Return([]*libregraph.Group{newGroup}, nil) + identityBackend.On("GetGroups", mock.Anything, mock.Anything).Return([]*libregraph.Group{newGroup}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups?$orderby=invalid", nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetGroups(rr, r) Expect(rr.Code).To(Equal(http.StatusBadRequest)) @@ -130,9 +132,10 @@ var _ = Describe("Groups", func() { Constraint: settingsmsg.Permission_CONSTRAINT_ALL, }, }, nil) - identityBackend.On("GetGroups", ctx, mock.Anything).Return(nil, errors.New("failed")) + identityBackend.On("GetGroups", mock.Anything, mock.Anything).Return(nil, errors.New("failed")) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetGroups(rr, r) Expect(rr.Code).To(Equal(http.StatusInternalServerError)) data, err := io.ReadAll(rr.Body) @@ -151,9 +154,10 @@ var _ = Describe("Groups", func() { Constraint: settingsmsg.Permission_CONSTRAINT_ALL, }, }, nil) - identityBackend.On("GetGroups", ctx, mock.Anything).Return(nil, errorcode.New(errorcode.AccessDenied, "access denied")) + identityBackend.On("GetGroups", mock.Anything, mock.Anything).Return(nil, errorcode.New(errorcode.AccessDenied, "access denied")) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetGroups(rr, r) Expect(rr.Code).To(Equal(http.StatusForbidden)) @@ -166,16 +170,17 @@ var _ = Describe("Groups", func() { Expect(odataerr.Error.Code).To(Equal("accessDenied")) }) - It("renders an empty list of groups", func() { + It("renders an empty list of groups with 2fa", func() { permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{ Permission: &settingsmsg.Permission{ Operation: settingsmsg.Permission_OPERATION_UNKNOWN, Constraint: settingsmsg.Permission_CONSTRAINT_ALL, }, }, nil) - identityBackend.On("GetGroups", ctx, mock.Anything).Return([]*libregraph.Group{}, nil) + identityBackend.On("GetGroups", mock.Anything, mock.Anything).Return([]*libregraph.Group{}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetGroups(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) @@ -188,16 +193,17 @@ var _ = Describe("Groups", func() { Expect(res.Value).To(Equal([]interface{}{})) }) - It("renders a list of groups", func() { + It("renders a list of groups with 2fa", func() { permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{ Permission: &settingsmsg.Permission{ Operation: settingsmsg.Permission_OPERATION_UNKNOWN, Constraint: settingsmsg.Permission_CONSTRAINT_ALL, }, }, nil) - identityBackend.On("GetGroups", ctx, mock.Anything).Return([]*libregraph.Group{newGroup}, nil) + identityBackend.On("GetGroups", mock.Anything, mock.Anything).Return([]*libregraph.Group{newGroup}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetGroups(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) @@ -211,6 +217,20 @@ var _ = Describe("Groups", func() { Expect(len(res.Value)).To(Equal(1)) Expect(res.Value[0].GetId()).To(Equal("group1")) }) + It("denies accessing a list of groups without 2fa", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{ + Permission: &settingsmsg.Permission{ + Operation: settingsmsg.Permission_OPERATION_UNKNOWN, + Constraint: settingsmsg.Permission_CONSTRAINT_ALL, + }, + }, nil) + identityBackend.On("GetGroups", ctx, mock.Anything).Return([]*libregraph.Group{newGroup}, nil) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) + svc.GetGroups(rr, r) + + Expect(rr.Code).To(Equal(http.StatusForbidden)) + }) It("denies listing for unprivileged users", func() { permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index d2586fcd32d..a4ba9983593 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -26,6 +26,7 @@ import ( "go-micro.dev/v4/client" "google.golang.org/grpc" + "github.com/owncloud/ocis/v2/ocis-pkg/mfa" "github.com/owncloud/ocis/v2/ocis-pkg/shared" settingsmsg "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0" settings "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" @@ -242,7 +243,7 @@ var _ = Describe("Users", func() { Expect(rr.Code).To(Equal(http.StatusBadRequest)) }) - It("lists the users", func() { + It("lists the users with 2fa", func() { permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{ Permission: &settingsmsg.Permission{ Operation: settingsmsg.Permission_OPERATION_UNKNOWN, @@ -257,6 +258,7 @@ var _ = Describe("Users", func() { identityBackend.On("GetUsers", mock.Anything, mock.Anything, mock.Anything).Return(users, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) @@ -270,6 +272,18 @@ var _ = Describe("Users", func() { Expect(len(res.Value)).To(Equal(1)) Expect(res.Value[0].GetId()).To(Equal("user1")) }) + It("denies listing without 2fa", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{ + Permission: &settingsmsg.Permission{ + Operation: settingsmsg.Permission_OPERATION_UNKNOWN, + Constraint: settingsmsg.Permission_CONSTRAINT_ALL, + }, + }, nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) + svc.GetUsers(rr, r) + + Expect(rr.Code).To(Equal(http.StatusForbidden)) + }) It("denies listing for unprivileged users", func() { permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) @@ -353,6 +367,7 @@ var _ = Describe("Users", func() { getUsers := func(path string) []*libregraph.User { r := httptest.NewRequest(http.MethodGet, path, nil) + r = r.WithContext(mfa.Set(r.Context(), true)) rec := httptest.NewRecorder() svc.GetUsers(rec, r) @@ -412,6 +427,7 @@ var _ = Describe("Users", func() { // Handles invalid sort field r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$orderby=invalid", nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(http.StatusBadRequest)) @@ -450,6 +466,7 @@ var _ = Describe("Users", func() { r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$expand=appRoleAssignments", nil) r = r.WithContext(revactx.ContextSetUser(ctx, currentUser)) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) @@ -490,6 +507,7 @@ var _ = Describe("Users", func() { }, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$filter="+url.QueryEscape(filter), nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(status)) @@ -550,6 +568,7 @@ var _ = Describe("Users", func() { }} }, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$filter="+url.QueryEscape(filter), nil) + r = r.WithContext(mfa.Set(r.Context(), true)) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(status)) diff --git a/services/proxy/pkg/config/config.go b/services/proxy/pkg/config/config.go index c2e3b3637dd..d2930aeb071 100644 --- a/services/proxy/pkg/config/config.go +++ b/services/proxy/pkg/config/config.go @@ -128,8 +128,8 @@ type JWKS struct { } type MFAConfig struct { - Enabled bool `yaml:"enabled" env:"OCIS_MFA_ENABLED" desc:"Enable MFA enforcement. If enabled users need to complete MFA before they can access specific paths" introductionVersion:"Balch"` - AuthLevelName string `yaml:"auth_level_name" env:"OCIS_MFA_AUTH_LEVEL_NAME" desc:"The name of the auth level that is used to enforce MFA. This needs to match the value in the 'acr' claim of the access token." introductionVersion:"Balch"` + Enabled bool `yaml:"enabled" env:"OCIS_MFA_ENABLED" desc:"Enable MFA enforcement. If enabled users need to complete MFA before they can access specific paths" introductionVersion:"Balch"` + AuthLevelNames []string `yaml:"auth_level_name" env:"OCIS_MFA_AUTH_LEVEL_NAMES" desc:"The names of the auth levels that contain MFA. One of these needs to match the value in the 'acr' claim of the access token." introductionVersion:"Balch"` } // Cache is a TTL cache configuration. diff --git a/services/proxy/pkg/config/defaults/defaultconfig.go b/services/proxy/pkg/config/defaults/defaultconfig.go index aab86ff0615..160dbe2c62a 100644 --- a/services/proxy/pkg/config/defaults/defaultconfig.go +++ b/services/proxy/pkg/config/defaults/defaultconfig.go @@ -100,6 +100,9 @@ func DefaultConfig() *config.Config { Cluster: "ocis-cluster", EnableTLS: false, }, + MultiFactorAuthentication: config.MFAConfig{ + AuthLevelNames: []string{"advanced"}, + }, } } diff --git a/services/proxy/pkg/middleware/mfa.go b/services/proxy/pkg/middleware/mfa.go index 6689b1e1943..389c458e1f5 100644 --- a/services/proxy/pkg/middleware/mfa.go +++ b/services/proxy/pkg/middleware/mfa.go @@ -9,19 +9,6 @@ import ( "github.com/owncloud/ocis/v2/services/proxy/pkg/config" ) -var ( -// ResponseHeaderBase is the prefix for all auth related response headers. -// ResponseHeaderBase = "X-OCIS-AUTH-" - -// The list of paths that require mfa if no $search query is present -// we use a map here for easier lookups -// _protectedPaths = map[string]struct{}{ -// "/graph/v1.0/users": struct{}{}, -// "/graph/v1.0/groups": struct{}{}, -// "/graph/v1beta1/drives": struct{}{}, -// } -) - // MultiFactor returns a middleware that checks requests for mfa func MultiFactor(cfg config.MFAConfig, opts ...Option) func(next http.Handler) http.Handler { options := newOptions(opts...) @@ -29,95 +16,23 @@ func MultiFactor(cfg config.MFAConfig, opts ...Option) func(next http.Handler) h return func(next http.Handler) http.Handler { return &MultiFactorAuthentication{ - next: next, - logger: logger, - enabled: cfg.Enabled, - authLevelName: cfg.AuthLevelName, - // claimsChecker: checkers.NewAcrChecker(cfg.AuthLevelName), // ? + next: next, + logger: logger, + enabled: cfg.Enabled, + authLevelNames: cfg.AuthLevelNames, } } } // MultiFactorAuthentication is a authenticator that checks for mfa on specific paths type MultiFactorAuthentication struct { - next http.Handler - logger log.Logger - enabled bool - authLevelName string - // claimsChecker checkers.Checker // ? + next http.Handler + logger log.Logger + enabled bool + authLevelNames []string } -// Authenticate implenents the authenticator interface and checks the access token for the correct acr claim -// func (mfa MultiFactorAuthentication) ServeHTTP(w http.ResponseWriter, req *http.Request) { -// if !mfa.shouldCheckClaims(req) { -// mfa.next.ServeHTTP(w, req) -// return -// } - -// log := mfa.logger.Error().Str("path", req.URL.Path).Str("required", mfa.authLevelName) -// claims := oidc.FromContext(req.Context()) - -// // either we use the claims checker here: -// if false { - -// err := mfa.claimsChecker.CheckClaims(claims) -// if err == nil { -// // acr claim is correct -// mfa.next.ServeHTTP(w, req) -// return -// } - -// log.Err(err).Interface("checker", mfa.claimsChecker.RequireMap()).Msg("can't access protected path without valid claims") -// } - -// // or we read the acr claim directly here: -// if true { - -// // acr is a standard OIDC claim. -// value, err := oidc.ReadStringClaim("acr", claims) -// if err != nil { -// log.Err(err).Interface("claims", claims).Msg("no acr claim found in access token") -// w.Header().Add(ResponseHeaderBase+"Requires-Claim", "acr") -// w.WriteHeader(http.StatusUnauthorized) -// return -// } - -// if value == mfa.authLevelName { -// // acr claim is corrct -// mfa.next.ServeHTTP(w, req) -// return -// } - -// log.Err(err).Str("acr", value).Msg("can't access protected path without valid claims") -// } - -// w.Header().Add(ResponseHeaderBase+"Requires-AuthLevel", mfa.authLevelName) -// w.WriteHeader(http.StatusUnauthorized) -// return -// } - -// // shouldCheckClaims returns true if we should check the claims for the provided request. -// func (mfa MultiFactorAuthentication) shouldCheckClaims(r *http.Request) bool { -// if !mfa.enabled { -// return false -// } - -// if _, protected := _protectedPaths[r.URL.Path]; !protected { -// return false -// } - -// q := r.URL.Query() - -// // We need to be careful here. We don't want to block access if this is a search query as this can be done without mfa. -// // But we don't want to allow bypassing mfa by just adding an empty (or ignored) $search parameter. -// // We should only check for the presence of the $search parameter if the endpoint is actually using it. -// if q.Get("$search") != "" { // if $query isn't present, it will return the empty string -// return false -// } - -// return true -// } - +// ServeHTTP adds the mfa header if the request contains a valid mfa token func (m MultiFactorAuthentication) ServeHTTP(w http.ResponseWriter, req *http.Request) { defer m.next.ServeHTTP(w, req) @@ -129,17 +44,16 @@ func (m MultiFactorAuthentication) ServeHTTP(w http.ResponseWriter, req *http.Re // overwrite the mfa header to avoid passing on wrong information mfa.SetHeader(req, false) - log := m.logger.Error().Str("path", req.URL.Path).Str("required", m.authLevelName) claims := oidc.FromContext(req.Context()) // acr is a standard OIDC claim. value, err := oidc.ReadStringClaim("acr", claims) if err != nil { - log.Err(err).Interface("claims", claims).Msg("no acr claim found in access token") + m.logger.Error().Str("path", req.URL.Path).Interface("required", m.authLevelNames).Err(err).Interface("claims", claims).Msg("no acr claim found in access token") return } - if value != m.authLevelName { + if !m.containsMFA(value) { m.logger.Debug().Str("acr", value).Str("url", req.URL.Path).Msg("accessing path without mfa") return } @@ -147,3 +61,13 @@ func (m MultiFactorAuthentication) ServeHTTP(w http.ResponseWriter, req *http.Re mfa.SetHeader(req, true) m.logger.Debug().Str("acr", value).Str("url", req.URL.Path).Msg("mfa authenticated") } + +// containsMFA checks if the given value is in the list of authentication level names +func (m MultiFactorAuthentication) containsMFA(value string) bool { + for _, v := range m.authLevelNames { + if v == value { + return true + } + } + return false +} diff --git a/services/proxy/pkg/middleware/options.go b/services/proxy/pkg/middleware/options.go index c68cdc42c11..fc45d26589b 100644 --- a/services/proxy/pkg/middleware/options.go +++ b/services/proxy/pkg/middleware/options.go @@ -7,7 +7,6 @@ import ( gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" "github.com/owncloud/ocis/v2/ocis-pkg/log" "github.com/owncloud/ocis/v2/ocis-pkg/oidc" - "github.com/owncloud/ocis/v2/ocis-pkg/oidc/checkers" policiessvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/policies/v0" settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" "github.com/owncloud/ocis/v2/services/proxy/pkg/config" @@ -42,8 +41,6 @@ type Options struct { OIDCClient oidc.OIDCClient // OIDCIss is the oidcAuth-issuer OIDCIss string - // ClaimsChecker makes checks over the OIDC claims - ClaimsChecker checkers.Checker // RevaGatewaySelector to send requests to the reva gateway RevaGatewaySelector pool.Selectable[gateway.GatewayAPIClient] // PreSignedURLConfig to configure the middleware @@ -142,13 +139,6 @@ func OIDCIss(iss string) Option { } } -// OIDCIss sets the oidcAuth issuer url -func ClaimsChecker(cc checkers.Checker) Option { - return func(o *Options) { - o.ClaimsChecker = cc - } -} - // CredentialsByUserAgent sets UserAgentChallenges. func CredentialsByUserAgent(v map[string]string) Option { return func(o *Options) {