From d99467eebad2270ffeb8882df3cd0e0bfab3839f Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Thu, 18 Jul 2024 16:45:05 +0100 Subject: [PATCH 1/9] Add code to support role filtering once populated on LogCtx --- base/logger_audit.go | 12 +++++++++++- base/logging_context.go | 3 +++ rest/admin_api.go | 8 ++++++++ rest/config.go | 5 +++++ 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/base/logger_audit.go b/base/logger_audit.go index ce837e65bb..97a78d734c 100644 --- a/base/logger_audit.go +++ b/base/logger_audit.go @@ -215,7 +215,7 @@ func (al *AuditLogger) shouldLog(id AuditID, ctx context.Context) bool { // shouldLogAuditEventForUserAndRole returns true if the request should be logged func shouldLogAuditEventForUserAndRole(logCtx *LogContext) bool { if logCtx.UserDomain == "" && logCtx.Username == "" || - len(logCtx.DbLogConfig.Audit.DisabledUsers) == 0 { + len(logCtx.DbLogConfig.Audit.DisabledRoles) == 0 && len(logCtx.DbLogConfig.Audit.DisabledUsers) == 0 { // early return for common cases: no user on context or no disabled users or roles return true } @@ -229,5 +229,15 @@ func shouldLogAuditEventForUserAndRole(logCtx *LogContext) bool { } } + // if any of the user's roles are disabled, then don't log the event + for role := range logCtx.UserRoles { + if _, isDisabled := logCtx.DbLogConfig.Audit.DisabledRoles[AuditLoggingPrincipal{ + Domain: string(logCtx.UserDomain), + Name: role, + }]; isDisabled { + return false + } + } + return true } diff --git a/base/logging_context.go b/base/logging_context.go index 6759a85ba7..ff7c5c502b 100644 --- a/base/logging_context.go +++ b/base/logging_context.go @@ -51,6 +51,8 @@ type LogContext struct { Username string // UserDomain can determine whether the authenticated user is a sync gateway user or a couchbase RBAC user UserDomain userIDDomain + // UserRoles is a list of the authenticated user's roles, the domain for these roles is the same as the UserDomain + UserRoles map[string]struct{} // RequestHost is the HTTP Host of the request associated with this log. RequestHost string @@ -94,6 +96,7 @@ type DbAuditLogConfig struct { Enabled bool EnabledEvents map[AuditID]struct{} DisabledUsers map[AuditLoggingPrincipal]struct{} + DisabledRoles map[AuditLoggingPrincipal]struct{} } type AuditLoggingPrincipal struct { diff --git a/rest/admin_api.go b/rest/admin_api.go index 4748cd50e2..9b9aa6a7f2 100644 --- a/rest/admin_api.go +++ b/rest/admin_api.go @@ -701,6 +701,7 @@ type HandleDbAuditConfigBody struct { Enabled *bool `json:"enabled,omitempty"` Events map[string]any `json:"events,omitempty"` DisabledUsers []base.AuditLoggingPrincipal `json:"disabled_users,omitempty"` + DisabledRoles []base.AuditLoggingPrincipal `json:"disabled_roles,omitempty"` } type HandleDbAuditConfigBodyVerboseEvent struct { @@ -721,6 +722,7 @@ func (h *handler) handleGetDbAuditConfig() error { etagVersion string dbAuditEnabled bool dbAuditDisabledUsers []base.AuditLoggingPrincipal + dbAuditDisabledRoles []base.AuditLoggingPrincipal enabledEvents = make(map[base.AuditID]struct{}) ) @@ -748,6 +750,7 @@ func (h *handler) handleGetDbAuditConfig() error { enabledEvents[base.AuditID(event)] = struct{}{} } dbAuditDisabledUsers = runtimeConfig.Logging.Audit.DisabledUsers + dbAuditDisabledRoles = runtimeConfig.Logging.Audit.DisabledRoles } } else { return base.HTTPErrorf(http.StatusServiceUnavailable, "audit config not available in non-persistent mode") @@ -778,6 +781,7 @@ func (h *handler) handleGetDbAuditConfig() error { Enabled: &dbAuditEnabled, Events: events, DisabledUsers: dbAuditDisabledUsers, + DisabledRoles: dbAuditDisabledRoles, } h.setEtag(etagVersion) @@ -861,6 +865,7 @@ func mutateConfigFromDbAuditConfigBody(isReplace bool, existingAuditConfig *DbAu if isReplace { existingAuditConfig.Enabled = requestAuditConfig.Enabled existingAuditConfig.DisabledUsers = requestAuditConfig.DisabledUsers + existingAuditConfig.DisabledRoles = requestAuditConfig.DisabledRoles // we don't need to do anything to "disable" events, other than not enable them existingAuditConfig.EnabledEvents = func() []uint { @@ -879,6 +884,9 @@ func mutateConfigFromDbAuditConfigBody(isReplace bool, existingAuditConfig *DbAu if requestAuditConfig.DisabledUsers != nil { existingAuditConfig.DisabledUsers = requestAuditConfig.DisabledUsers } + if requestAuditConfig.DisabledRoles != nil { + existingAuditConfig.DisabledRoles = requestAuditConfig.DisabledRoles + } for i, event := range existingAuditConfig.EnabledEvents { if shouldEnable, ok := eventsToChange[base.AuditID(event)]; ok { diff --git a/rest/config.go b/rest/config.go index 26210c3add..625d9856bc 100644 --- a/rest/config.go +++ b/rest/config.go @@ -2286,14 +2286,19 @@ func (c *DbConfig) toDbLogConfig(ctx context.Context) *base.DbLogConfig { // user/role filtering disabledUsers := make(map[base.AuditLoggingPrincipal]struct{}, len(c.Logging.Audit.DisabledUsers)) + disabledRoles := make(map[base.AuditLoggingPrincipal]struct{}, len(c.Logging.Audit.DisabledRoles)) for _, user := range c.Logging.Audit.DisabledUsers { disabledUsers[user] = struct{}{} } + for _, role := range c.Logging.Audit.DisabledRoles { + disabledRoles[role] = struct{}{} + } aud = &base.DbAuditLogConfig{ Enabled: base.BoolDefault(l.Audit.Enabled, false), EnabledEvents: enabledEvents, DisabledUsers: disabledUsers, + DisabledRoles: disabledRoles, } } From f8c367540aedde6436a3f99f800ab5d9bd13d668 Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Thu, 18 Jul 2024 17:19:58 +0100 Subject: [PATCH 2/9] Add (failing) tests for role audit filtering --- rest/audit_test.go | 50 +++++++++++++++++++++++++++------- rest/utilities_testing.go | 6 ++-- rest/utilities_testing_user.go | 7 +++-- 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/rest/audit_test.go b/rest/audit_test.go index 0afa182b01..1753428b15 100644 --- a/rest/audit_test.go +++ b/rest/audit_test.go @@ -38,13 +38,17 @@ func TestAuditLoggingFields(t *testing.T) { base.InitializeMemoryLoggers() const ( - requestInfoHeaderName = "extra-audit-logging-header" - requestUser = "alice" - filteredPublicUsername = "bob" - filteredAdminUsername = "TestAuditLoggingFields-charlie" - filteredAdminPassword = "password" - unauthorizedAdminUsername = "TestAuditLoggingFields-alice" - unauthorizedAdminPassword = "password" + requestInfoHeaderName = "extra-audit-logging-header" + requestUser = "alice" + filteredPublicUsername = "bob" + filteredPublicRoleUsername = "charlie" + filteredPublicRoleName = "observer" + filteredAdminUsername = "TestAuditLoggingFields-charlie" + filteredAdminRoleUsername = "TestAuditLoggingFields-bob" + unauthorizedAdminUsername = "TestAuditLoggingFields-alice" + ) + var ( + filteredAdminRoleName = BucketFullAccessRole.RoleName ) rt := NewRestTester(t, &RestTesterConfig{ @@ -79,6 +83,10 @@ func TestAuditLoggingFields(t *testing.T) { {Name: filteredPublicUsername, Domain: string(base.UserDomainSyncGateway)}, {Name: filteredAdminUsername, Domain: string(base.UserDomainCBServer)}, }, + DisabledRoles: []base.AuditLoggingPrincipal{ + {Name: filteredPublicRoleName, Domain: string(base.UserDomainSyncGateway)}, + {Name: filteredAdminRoleName, Domain: string(base.UserDomainCBServer)}, + }, }, } @@ -87,12 +95,16 @@ func TestAuditLoggingFields(t *testing.T) { rt.CreateUser(requestUser, nil) rt.CreateUser(filteredPublicUsername, nil) + rt.CreateUser(filteredPublicRoleUsername, nil, filteredPublicRoleName) if runServerRBACTests { eps, httpClient, err := rt.ServerContext().ObtainManagementEndpointsAndHTTPClient() require.NoError(t, err) - MakeUser(t, httpClient, eps[0], filteredAdminUsername, filteredAdminPassword, []string{fmt.Sprintf("%s[%s]", MobileSyncGatewayRole.RoleName, rt.Bucket().GetName())}) + + MakeUser(t, httpClient, eps[0], filteredAdminUsername, RestTesterDefaultUserPassword, []string{fmt.Sprintf("%s[%s]", MobileSyncGatewayRole.RoleName, rt.Bucket().GetName())}) defer DeleteUser(t, httpClient, eps[0], filteredAdminUsername) - MakeUser(t, httpClient, eps[0], unauthorizedAdminUsername, unauthorizedAdminPassword, []string{}) + MakeUser(t, httpClient, eps[0], filteredAdminRoleUsername, RestTesterDefaultUserPassword, []string{fmt.Sprintf("%s[%s]", filteredAdminRoleName, rt.Bucket().GetName())}) + defer DeleteUser(t, httpClient, eps[0], filteredAdminRoleUsername) + MakeUser(t, httpClient, eps[0], unauthorizedAdminUsername, RestTesterDefaultUserPassword, []string{}) defer DeleteUser(t, httpClient, eps[0], unauthorizedAdminUsername) } @@ -295,6 +307,12 @@ func TestAuditLoggingFields(t *testing.T) { RequireStatus(t, rt.SendUserRequest(http.MethodGet, "/db/", "", filteredPublicUsername), http.StatusOK) }, }, + { + name: "filtered public role request", + auditableAction: func(t testing.TB) { + RequireStatus(t, rt.SendUserRequest(http.MethodGet, "/db/", "", filteredPublicRoleUsername), http.StatusOK) + }, + }, { name: "filtered admin request", auditableAction: func(t testing.TB) { @@ -304,7 +322,19 @@ func TestAuditLoggingFields(t *testing.T) { if !rt.AdminInterfaceAuthentication { t.Skip("Skipping subtest that requires admin auth") } - RequireStatus(t, rt.SendAdminRequestWithAuth(http.MethodGet, "/db/", "", filteredAdminUsername, filteredAdminPassword), http.StatusOK) + RequireStatus(t, rt.SendAdminRequestWithAuth(http.MethodGet, "/db/", "", filteredAdminUsername, RestTesterDefaultUserPassword), http.StatusOK) + }, + }, + { + name: "filtered admin role request", + auditableAction: func(t testing.TB) { + if !runServerRBACTests { + t.Skip("Skipping subtest that requires admin RBAC") + } + if !rt.AdminInterfaceAuthentication { + t.Skip("Skipping subtest that requires admin auth") + } + RequireStatus(t, rt.SendAdminRequestWithAuth(http.MethodGet, "/db/", "", filteredAdminRoleUsername, RestTesterDefaultUserPassword), http.StatusOK) }, }, } diff --git a/rest/utilities_testing.go b/rest/utilities_testing.go index 33882212dd..55e40c9d32 100644 --- a/rest/utilities_testing.go +++ b/rest/utilities_testing.go @@ -503,12 +503,12 @@ func (rt *RestTester) GetDatabase() *db.DatabaseContext { } // CreateUser creates a user with the default password and channels scoped to a single test collection. -func (rt *RestTester) CreateUser(username string, channels []string) { +func (rt *RestTester) CreateUser(username string, channels []string, roles ...string) { var response *TestResponse if rt.AdminInterfaceAuthentication { - response = rt.SendAdminRequestWithAuth(http.MethodPut, "/{{.db}}/_user/"+username, GetUserPayload(rt.TB(), "", RestTesterDefaultUserPassword, "", rt.GetSingleDataStore(), channels, nil), base.TestClusterUsername(), base.TestClusterPassword()) + response = rt.SendAdminRequestWithAuth(http.MethodPut, "/{{.db}}/_user/"+username, GetUserPayload(rt.TB(), "", RestTesterDefaultUserPassword, "", rt.GetSingleDataStore(), channels, roles), base.TestClusterUsername(), base.TestClusterPassword()) } else { - response = rt.SendAdminRequest(http.MethodPut, "/{{.db}}/_user/"+username, GetUserPayload(rt.TB(), "", RestTesterDefaultUserPassword, "", rt.GetSingleDataStore(), channels, nil)) + response = rt.SendAdminRequest(http.MethodPut, "/{{.db}}/_user/"+username, GetUserPayload(rt.TB(), "", RestTesterDefaultUserPassword, "", rt.GetSingleDataStore(), channels, roles)) } RequireStatus(rt.TB(), response, http.StatusCreated) } diff --git a/rest/utilities_testing_user.go b/rest/utilities_testing_user.go index 873a25f980..47699e958f 100644 --- a/rest/utilities_testing_user.go +++ b/rest/utilities_testing_user.go @@ -39,11 +39,12 @@ func MakeUser(t *testing.T, httpClient *http.Client, serverURL, username, passwo return true, err, nil } defer func() { assert.NoError(t, resp.Body.Close()) }() + var bodyResp []byte if resp.StatusCode != http.StatusOK { - bodyResp, err := io.ReadAll(resp.Body) - assert.NoError(t, err, "Failed to create user: %s", bodyResp) + bodyResp, err = io.ReadAll(resp.Body) + require.NoError(t, err, "Failed to create user: %s", bodyResp) } - require.Equal(t, http.StatusOK, resp.StatusCode) + require.Equalf(t, http.StatusOK, resp.StatusCode, "Failed to create user: %s", bodyResp) return false, err, nil } From 0082d20dfd17a91a206ca2bf57c5ed3d642cfa88 Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Thu, 18 Jul 2024 18:22:10 +0100 Subject: [PATCH 3/9] Make SG role filtering work --- auth/principal.go | 3 +++ base/logging_context.go | 9 ++++++++- rest/audit_test.go | 2 ++ rest/handler.go | 15 ++++++++++++--- rest/utilities_testing.go | 13 ++++++++++++- 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/auth/principal.go b/auth/principal.go index 0f9d83ef7d..f40aed8e8e 100644 --- a/auth/principal.go +++ b/auth/principal.go @@ -94,6 +94,9 @@ type User interface { // Changes the user's password. SetPassword(password string) error + // GetRoles returns the set of roles the user belongs to, initializing them if necessary. + GetRoles() []Role + // The set of Roles the user belongs to (including ones given to it by the sync function and by OIDC/JWT) // Returns nil if invalidated RoleNames() ch.TimedSet diff --git a/base/logging_context.go b/base/logging_context.go index ff7c5c502b..5e1c4b5f95 100644 --- a/base/logging_context.go +++ b/base/logging_context.go @@ -160,6 +160,7 @@ func (lc *LogContext) getCopy() LogContext { RequestAdditionalAuditFields: lc.RequestAdditionalAuditFields, Username: lc.Username, UserDomain: lc.UserDomain, + UserRoles: lc.UserRoles, RequestHost: lc.RequestHost, RequestRemoteAddr: lc.RequestRemoteAddr, EffectiveUserID: lc.EffectiveUserID, @@ -271,10 +272,16 @@ const ( UserDomainBuiltin = "builtin" // internal users (e.g. SG bootstrap user) ) -func UserLogCtx(parent context.Context, username string, domain userIDDomain) context.Context { +func UserLogCtx(parent context.Context, username string, domain userIDDomain, roles []string) context.Context { newCtx := getLogCtx(parent) newCtx.Username = username newCtx.UserDomain = domain + if len(roles) > 0 { + newCtx.UserRoles = make(map[string]struct{}, len(roles)) + for _, role := range roles { + newCtx.UserRoles[role] = struct{}{} + } + } return LogContextWith(parent, &newCtx) } diff --git a/rest/audit_test.go b/rest/audit_test.go index 1753428b15..75db52b371 100644 --- a/rest/audit_test.go +++ b/rest/audit_test.go @@ -18,6 +18,7 @@ import ( "github.com/couchbase/sync_gateway/auth" "github.com/couchbase/sync_gateway/base" + "github.com/couchbase/sync_gateway/channels" "github.com/couchbase/sync_gateway/db" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -95,6 +96,7 @@ func TestAuditLoggingFields(t *testing.T) { rt.CreateUser(requestUser, nil) rt.CreateUser(filteredPublicUsername, nil) + rt.CreateRole(filteredPublicRoleName, []string{channels.AllChannelWildcard}) rt.CreateUser(filteredPublicRoleUsername, nil, filteredPublicRoleName) if runServerRBACTests { eps, httpClient, err := rt.ServerContext().ObtainManagementEndpointsAndHTTPClient() diff --git a/rest/handler.go b/rest/handler.go index f39d31854d..7cf7ae37fa 100644 --- a/rest/handler.go +++ b/rest/handler.go @@ -572,7 +572,7 @@ func (h *handler) validateAndWriteHeaders(method handlerMethod, accessPermission } h.authorizedAdminUser = username h.permissionsResults = permissions - h.rqCtx = base.UserLogCtx(h.ctx(), username, base.UserDomainCBServer) + h.rqCtx = base.UserLogCtx(h.ctx(), username, base.UserDomainCBServer, nil) base.Audit(h.ctx(), base.AuditIDAdminUserAuthenticated, base.AuditFields{}) base.DebugfCtx(h.ctx(), base.KeyAuth, "%s: User %s was successfully authorized as an admin", h.formatSerialNumber(), base.UD(username)) @@ -821,10 +821,19 @@ func (h *handler) checkPublicAuth(dbCtx *db.DatabaseContext) (err error) { } else { dbCtx.DbStats.Security().AuthSuccessCount.Add(1) + var roleNames []string + roles := h.user.GetRoles() + if len(roles) > 0 { + roleNames = make([]string, 0, len(roles)) + for _, role := range roles { + roleNames = append(roleNames, role.Name()) + } + } + if h.isGuest() { - h.rqCtx = base.UserLogCtx(h.ctx(), base.GuestUsername, base.UserDomainSyncGateway) + h.rqCtx = base.UserLogCtx(h.ctx(), base.GuestUsername, base.UserDomainSyncGateway, roleNames) } else { - h.rqCtx = base.UserLogCtx(h.ctx(), h.user.Name(), base.UserDomainSyncGateway) + h.rqCtx = base.UserLogCtx(h.ctx(), h.user.Name(), base.UserDomainSyncGateway, roleNames) } base.Audit(h.ctx(), base.AuditIDPublicUserAuthenticated, auditFields) } diff --git a/rest/utilities_testing.go b/rest/utilities_testing.go index 55e40c9d32..8997e8201d 100644 --- a/rest/utilities_testing.go +++ b/rest/utilities_testing.go @@ -513,6 +513,17 @@ func (rt *RestTester) CreateUser(username string, channels []string, roles ...st RequireStatus(rt.TB(), response, http.StatusCreated) } +// CreateRole creates a role with channels scoped to a single test collection. +func (rt *RestTester) CreateRole(rolename string, channels []string) { + var response *TestResponse + if rt.AdminInterfaceAuthentication { + response = rt.SendAdminRequestWithAuth(http.MethodPut, "/{{.db}}/_role/"+rolename, GetRolePayload(rt.TB(), rolename, rt.GetSingleDataStore(), channels), base.TestClusterUsername(), base.TestClusterPassword()) + } else { + response = rt.SendAdminRequest(http.MethodPut, "/{{.db}}/_role/"+rolename, GetRolePayload(rt.TB(), rolename, rt.GetSingleDataStore(), channels)) + } + RequireStatus(rt.TB(), response, http.StatusCreated) +} + func (rt *RestTester) GetUserAdminAPI(username string) auth.PrincipalConfig { response := rt.SendAdminRequest(http.MethodGet, "/{{.db}}/_user/"+username, "") RequireStatus(rt.TB(), response, http.StatusOK) @@ -1731,7 +1742,7 @@ func GetUserPayload(t testing.TB, username, password, email string, collection s // GetRolePayload will take roleName and channels you want to assign a particular role and return the appropriate payload for the _role endpoint // For default collection, follows same handling as GetUserPayload for chans. -func GetRolePayload(t *testing.T, roleName string, collection sgbucket.DataStore, chans []string) string { +func GetRolePayload(t testing.TB, roleName string, collection sgbucket.DataStore, chans []string) string { config := PrincipalConfigForWrite{} if roleName != "" { config.Name = &roleName From 00710bb096d6081964a31445b6af9142bbe19819 Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Thu, 18 Jul 2024 19:32:37 +0100 Subject: [PATCH 4/9] Rename field to be audit specific --- base/logger_audit.go | 2 +- base/logging_context.go | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/base/logger_audit.go b/base/logger_audit.go index 97a78d734c..f36718cf00 100644 --- a/base/logger_audit.go +++ b/base/logger_audit.go @@ -230,7 +230,7 @@ func shouldLogAuditEventForUserAndRole(logCtx *LogContext) bool { } // if any of the user's roles are disabled, then don't log the event - for role := range logCtx.UserRoles { + for role := range logCtx.UserRolesForAuditFiltering { if _, isDisabled := logCtx.DbLogConfig.Audit.DisabledRoles[AuditLoggingPrincipal{ Domain: string(logCtx.UserDomain), Name: role, diff --git a/base/logging_context.go b/base/logging_context.go index 5e1c4b5f95..8d59f74eac 100644 --- a/base/logging_context.go +++ b/base/logging_context.go @@ -50,9 +50,9 @@ type LogContext struct { // Username is the name of the authenticated user Username string // UserDomain can determine whether the authenticated user is a sync gateway user or a couchbase RBAC user - UserDomain userIDDomain - // UserRoles is a list of the authenticated user's roles, the domain for these roles is the same as the UserDomain - UserRoles map[string]struct{} + UserDomain UserIDDomain + // UserRolesForAuditFiltering is a list of the authenticated user's roles to be used to determine audit log filtering, the domain for these roles is the same as the UserDomain + UserRolesForAuditFiltering map[string]struct{} // RequestHost is the HTTP Host of the request associated with this log. RequestHost string @@ -160,7 +160,7 @@ func (lc *LogContext) getCopy() LogContext { RequestAdditionalAuditFields: lc.RequestAdditionalAuditFields, Username: lc.Username, UserDomain: lc.UserDomain, - UserRoles: lc.UserRoles, + UserRolesForAuditFiltering: lc.UserRolesForAuditFiltering, RequestHost: lc.RequestHost, RequestRemoteAddr: lc.RequestRemoteAddr, EffectiveUserID: lc.EffectiveUserID, @@ -264,22 +264,22 @@ type EffectiveUserPair struct { Domain string `json:"domain"` } -type userIDDomain string +type UserIDDomain string const ( - UserDomainSyncGateway userIDDomain = "sgw" - UserDomainCBServer userIDDomain = "cbs" + UserDomainSyncGateway UserIDDomain = "sgw" + UserDomainCBServer UserIDDomain = "cbs" UserDomainBuiltin = "builtin" // internal users (e.g. SG bootstrap user) ) -func UserLogCtx(parent context.Context, username string, domain userIDDomain, roles []string) context.Context { +func UserLogCtx(parent context.Context, username string, domain UserIDDomain, roles []string) context.Context { newCtx := getLogCtx(parent) newCtx.Username = username newCtx.UserDomain = domain if len(roles) > 0 { - newCtx.UserRoles = make(map[string]struct{}, len(roles)) + newCtx.UserRolesForAuditFiltering = make(map[string]struct{}, len(roles)) for _, role := range roles { - newCtx.UserRoles[role] = struct{}{} + newCtx.UserRolesForAuditFiltering[role] = struct{}{} } } return LogContextWith(parent, &newCtx) From 734ee52bcfda6fb3026760e680d5833eb19b607a Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Thu, 18 Jul 2024 19:35:51 +0100 Subject: [PATCH 5/9] Only store roles on LogContext when we need them for audit filtering --- rest/handler.go | 68 +++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 14 deletions(-) diff --git a/rest/handler.go b/rest/handler.go index 7cf7ae37fa..111b8d097f 100644 --- a/rest/handler.go +++ b/rest/handler.go @@ -570,9 +570,16 @@ func (h *handler) validateAndWriteHeaders(method handlerMethod, accessPermission base.Audit(h.ctx(), base.AuditIDAdminUserAuthorizationFailed, base.AuditFields{base.AuditFieldUserName: username}) return base.HTTPErrorf(statusCode, "") } + + // we already did the work to get the full list of user roles as part of the above check, + // but we only need to keep them stored if we want to filter based on CB Server roles. + var auditRoleNames []string + if needRolesForAudit(dbContext, base.UserDomainCBServer) { + auditRoleNames = roles + } h.authorizedAdminUser = username h.permissionsResults = permissions - h.rqCtx = base.UserLogCtx(h.ctx(), username, base.UserDomainCBServer, nil) + h.rqCtx = base.UserLogCtx(h.ctx(), username, base.UserDomainCBServer, auditRoleNames) base.Audit(h.ctx(), base.AuditIDAdminUserAuthenticated, base.AuditFields{}) base.DebugfCtx(h.ctx(), base.KeyAuth, "%s: User %s was successfully authorized as an admin", h.formatSerialNumber(), base.UD(username)) @@ -797,6 +804,47 @@ func (h *handler) logStatus(status int, message string) { h.logDuration(false) // don't track actual time } +func needRolesForAudit(db *db.DatabaseContext, domain base.UserIDDomain) bool { + if db == nil || + db.Options.LoggingConfig == nil || + db.Options.LoggingConfig.Audit == nil || + len(db.Options.LoggingConfig.Audit.DisabledRoles) == 0 { + return false + } + + // if we have any matching domains then we need the given roles + for principal := range db.Options.LoggingConfig.Audit.DisabledRoles { + if principal.Domain == string(domain) { + return true + } + } + + return false +} + +// getSGUserRolesForAudit returns a list of role names for the given user, if audit role filtering is enabled. +func getSGUserRolesForAudit(db *db.DatabaseContext, user auth.User) []string { + if user == nil { + return nil + } + + if !needRolesForAudit(db, base.UserDomainSyncGateway) { + return nil + } + + roles := user.GetRoles() + if len(roles) == 0 { + return nil + } + + roleNames := make([]string, 0, len(roles)) + for _, role := range roles { + roleNames = append(roleNames, role.Name()) + } + + return roleNames +} + // checkPublicAuth verifies that the current request is authenticated for the given database. // // NOTE: checkPublicAuth is not used for the admin interface. @@ -821,20 +869,12 @@ func (h *handler) checkPublicAuth(dbCtx *db.DatabaseContext) (err error) { } else { dbCtx.DbStats.Security().AuthSuccessCount.Add(1) - var roleNames []string - roles := h.user.GetRoles() - if len(roles) > 0 { - roleNames = make([]string, 0, len(roles)) - for _, role := range roles { - roleNames = append(roleNames, role.Name()) - } - } - - if h.isGuest() { - h.rqCtx = base.UserLogCtx(h.ctx(), base.GuestUsername, base.UserDomainSyncGateway, roleNames) - } else { - h.rqCtx = base.UserLogCtx(h.ctx(), h.user.Name(), base.UserDomainSyncGateway, roleNames) + username := base.GuestUsername + if !h.isGuest() { + username = h.user.Name() } + roleNames := getSGUserRolesForAudit(dbCtx, h.user) + h.rqCtx = base.UserLogCtx(h.ctx(), username, base.UserDomainSyncGateway, roleNames) base.Audit(h.ctx(), base.AuditIDPublicUserAuthenticated, auditFields) } }(time.Now()) From 83c366226972446c71765de95d939e9a36f7efcf Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Mon, 22 Jul 2024 15:38:30 +0100 Subject: [PATCH 6/9] Duplicate whoami call to fetch for audit role filtering --- rest/audit_test.go | 2 +- rest/handler.go | 16 +++++++++++++--- rest/server_context.go | 31 ++++++++++++++++++++----------- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/rest/audit_test.go b/rest/audit_test.go index 75db52b371..e91a92c7ad 100644 --- a/rest/audit_test.go +++ b/rest/audit_test.go @@ -291,7 +291,7 @@ func TestAuditLoggingFields(t *testing.T) { if !rt.AdminInterfaceAuthentication { t.Skip("Skipping subtest that requires admin auth") } - RequireStatus(t, rt.SendAdminRequestWithAuth(http.MethodGet, "/db/", "", unauthorizedAdminUsername, unauthorizedAdminPassword), http.StatusForbidden) + RequireStatus(t, rt.SendAdminRequestWithAuth(http.MethodGet, "/db/", "", unauthorizedAdminUsername, RestTesterDefaultUserPassword), http.StatusForbidden) }, expectedAuditEventFields: map[base.AuditID]base.AuditFields{ base.AuditIDAdminUserAuthorizationFailed: { diff --git a/rest/handler.go b/rest/handler.go index 111b8d097f..544ef757b8 100644 --- a/rest/handler.go +++ b/rest/handler.go @@ -571,11 +571,21 @@ func (h *handler) validateAndWriteHeaders(method handlerMethod, accessPermission return base.HTTPErrorf(statusCode, "") } - // we already did the work to get the full list of user roles as part of the above check, - // but we only need to keep them stored if we want to filter based on CB Server roles. + // even though `checkAdminAuth` _can_ issue whoami to find user's roles, it doesn't always... + // to reduce code complexity, we'll potentially be making this whoami request twice if we need it for audit filtering var auditRoleNames []string if needRolesForAudit(dbContext, base.UserDomainCBServer) { - auditRoleNames = roles + whoAmIResponse, whoAmIStatus, whoAmIErr := cbRBACWhoAmI(h.ctx(), httpClient, managementEndpoints, username, password) + if whoAmIErr != nil || whoAmIStatus != http.StatusOK { + base.WarnfCtx(h.ctx(), "An error occurred whilst fetching the user's roles for audit filtering - will not filter: %v", whoAmIErr) + } else { + for _, role := range whoAmIResponse.Roles { + // only filter roles applied to this bucket or cluster-scope + if bucketName == authScope || bucketName == "" { + auditRoleNames = append(auditRoleNames, role.RoleName) + } + } + } } h.authorizedAdminUser = username h.permissionsResults = permissions diff --git a/rest/server_context.go b/rest/server_context.go index 73825152a4..1568ce0ac0 100644 --- a/rest/server_context.go +++ b/rest/server_context.go @@ -2012,26 +2012,35 @@ func CheckPermissions(ctx context.Context, httpClient *http.Client, managementEn return http.StatusForbidden, nil, nil } -func CheckRoles(ctx context.Context, httpClient *http.Client, managementEndpoints []string, username, password string, requestedRoles []RouteRole, bucketName string) (statusCode int, err error) { +type WhoAmIResponse struct { + Roles []struct { + RoleName string `json:"role"` + BucketName string `json:"bucket_name"` + } `json:"roles"` +} + +func cbRBACWhoAmI(ctx context.Context, httpClient *http.Client, managementEndpoints []string, username, password string) (response *WhoAmIResponse, statusCode int, err error) { statusCode, bodyResponse, err := doHTTPAuthRequest(ctx, httpClient, username, password, "GET", "/whoami", managementEndpoints, nil) if err != nil { - return http.StatusInternalServerError, err + return nil, http.StatusInternalServerError, err } if statusCode != http.StatusOK { - return statusCode, nil + return nil, statusCode, nil } - var whoAmIResults struct { - Roles []struct { - RoleName string `json:"role"` - BucketName string `json:"bucket_name"` - } `json:"roles"` + err = base.JSONUnmarshal(bodyResponse, &response) + if err != nil { + return nil, http.StatusInternalServerError, err } - err = base.JSONUnmarshal(bodyResponse, &whoAmIResults) - if err != nil { - return http.StatusInternalServerError, err + return response, statusCode, nil +} + +func CheckRoles(ctx context.Context, httpClient *http.Client, managementEndpoints []string, username, password string, requestedRoles []RouteRole, bucketName string) (statusCode int, err error) { + whoAmIResults, statusCode, err := cbRBACWhoAmI(ctx, httpClient, managementEndpoints, username, password) + if err != nil || statusCode != http.StatusOK { + return statusCode, err } for _, roleResult := range whoAmIResults.Roles { From ee8da4894ec44cc87485e83ed78206a9abfb29b4 Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Mon, 22 Jul 2024 15:47:55 +0100 Subject: [PATCH 7/9] Add test case to cover cross-bucket RBAC role grants for filtering --- base/main_test_bucket_pool_util.go | 8 ++-- rest/audit_test.go | 61 +++++++++++++++++++++++++----- rest/handler.go | 37 +++++++++++------- 3 files changed, 78 insertions(+), 28 deletions(-) diff --git a/base/main_test_bucket_pool_util.go b/base/main_test_bucket_pool_util.go index 7f538202f7..1fc8d760dc 100644 --- a/base/main_test_bucket_pool_util.go +++ b/base/main_test_bucket_pool_util.go @@ -51,8 +51,8 @@ func getTestBucketSpec(testBucketName tbpBucketName) BucketSpec { } // RequireNumTestBuckets skips the given test if there are not enough test buckets available to use. -func RequireNumTestBuckets(t *testing.T, numRequired int) { - usable := GTestBucketPool.numUsableBuckets() +func RequireNumTestBuckets(t testing.TB, numRequired int) { + usable := GTestBucketPool.NumUsableBuckets() if usable < numRequired { t.Skipf("Only had %d usable test buckets available (test requires %d)", usable, numRequired) } @@ -67,8 +67,8 @@ func RequireNumTestDataStores(t testing.TB, numRequired int) { } } -// numUsableBuckets returns the total number of buckets in the pool that can be used by a test. -func (tbp *TestBucketPool) numUsableBuckets() int { +// NumUsableBuckets returns the total number of buckets in the pool that can be used by a test. +func (tbp *TestBucketPool) NumUsableBuckets() int { if !tbp.integrationMode { // we can create virtually endless walrus buckets, // so report back 10 to match a fully available CBS bucket pool. diff --git a/rest/audit_test.go b/rest/audit_test.go index e91a92c7ad..8e9c3da900 100644 --- a/rest/audit_test.go +++ b/rest/audit_test.go @@ -39,14 +39,15 @@ func TestAuditLoggingFields(t *testing.T) { base.InitializeMemoryLoggers() const ( - requestInfoHeaderName = "extra-audit-logging-header" - requestUser = "alice" - filteredPublicUsername = "bob" - filteredPublicRoleUsername = "charlie" - filteredPublicRoleName = "observer" - filteredAdminUsername = "TestAuditLoggingFields-charlie" - filteredAdminRoleUsername = "TestAuditLoggingFields-bob" - unauthorizedAdminUsername = "TestAuditLoggingFields-alice" + requestInfoHeaderName = "extra-audit-logging-header" + requestUser = "alice" + filteredPublicUsername = "bob" + filteredPublicRoleUsername = "charlie" + filteredPublicRoleName = "observer" + filteredAdminUsername = "TestAuditLoggingFields-charlie" + unfilteredAdminRoleUsername = "TestAuditLoggingFields-diana" + filteredAdminRoleUsername = "TestAuditLoggingFields-bob" + unauthorizedAdminUsername = "TestAuditLoggingFields-alice" ) var ( filteredAdminRoleName = BucketFullAccessRole.RoleName @@ -102,13 +103,29 @@ func TestAuditLoggingFields(t *testing.T) { eps, httpClient, err := rt.ServerContext().ObtainManagementEndpointsAndHTTPClient() require.NoError(t, err) - MakeUser(t, httpClient, eps[0], filteredAdminUsername, RestTesterDefaultUserPassword, []string{fmt.Sprintf("%s[%s]", MobileSyncGatewayRole.RoleName, rt.Bucket().GetName())}) + MakeUser(t, httpClient, eps[0], filteredAdminUsername, RestTesterDefaultUserPassword, []string{ + fmt.Sprintf("%s[%s]", MobileSyncGatewayRole.RoleName, rt.Bucket().GetName()), + }) defer DeleteUser(t, httpClient, eps[0], filteredAdminUsername) - MakeUser(t, httpClient, eps[0], filteredAdminRoleUsername, RestTesterDefaultUserPassword, []string{fmt.Sprintf("%s[%s]", filteredAdminRoleName, rt.Bucket().GetName())}) + MakeUser(t, httpClient, eps[0], filteredAdminRoleUsername, RestTesterDefaultUserPassword, []string{ + fmt.Sprintf("%s[%s]", filteredAdminRoleName, rt.Bucket().GetName()), + }) defer DeleteUser(t, httpClient, eps[0], filteredAdminRoleUsername) MakeUser(t, httpClient, eps[0], unauthorizedAdminUsername, RestTesterDefaultUserPassword, []string{}) defer DeleteUser(t, httpClient, eps[0], unauthorizedAdminUsername) + // if we have another bucket available, use it to test cross-bucket role filtering (to ensure it doesn't) + if base.GTestBucketPool.NumUsableBuckets() >= 2 { + differentBucket := base.GetTestBucket(t) + defer differentBucket.Close(base.TestCtx(t)) + differentBucketName := differentBucket.GetName() + + MakeUser(t, httpClient, eps[0], unfilteredAdminRoleUsername, RestTesterDefaultUserPassword, []string{ + fmt.Sprintf("%s[%s]", filteredAdminRoleName, differentBucketName), + fmt.Sprintf("%s[%s]", MobileSyncGatewayRole.RoleName, rt.Bucket().GetName()), + }) + defer DeleteUser(t, httpClient, eps[0], unfilteredAdminRoleUsername) + } } // auditFieldValueIgnored is a special value for an audit field to skip value-specific checks whilst still ensuring the field property is set @@ -339,6 +356,30 @@ func TestAuditLoggingFields(t *testing.T) { RequireStatus(t, rt.SendAdminRequestWithAuth(http.MethodGet, "/db/", "", filteredAdminRoleUsername, RestTesterDefaultUserPassword), http.StatusOK) }, }, + { + name: "authed admin request role filtered on different bucket", + auditableAction: func(t testing.TB) { + if !rt.AdminInterfaceAuthentication { + t.Skip("Skipping subtest that requires admin auth") + } + base.RequireNumTestBuckets(t, 2) + RequireStatus(t, rt.SendAdminRequestWithAuth(http.MethodGet, "/db/", "", unfilteredAdminRoleUsername, RestTesterDefaultUserPassword), http.StatusOK) + }, + expectedAuditEventFields: map[base.AuditID]base.AuditFields{ + base.AuditIDAdminUserAuthenticated: { + base.AuditFieldCorrelationID: auditFieldValueIgnored, + // base.AuditFieldRealUserID: map[string]any{"domain": "cbs", "user": unfilteredAdminRoleUsername}, + }, + base.AuditIDReadDatabase: { + base.AuditFieldCorrelationID: auditFieldValueIgnored, + base.AuditFieldRealUserID: map[string]any{"domain": "cbs", "user": unfilteredAdminRoleUsername}, + }, + base.AuditIDAdminHTTPAPIRequest: { + base.AuditFieldHTTPMethod: http.MethodGet, + base.AuditFieldHTTPPath: "/db/", + }, + }, + }, } for _, testCase := range testCases { rt.Run(testCase.name, func(t *testing.T) { diff --git a/rest/handler.go b/rest/handler.go index 544ef757b8..e314bb6a52 100644 --- a/rest/handler.go +++ b/rest/handler.go @@ -573,20 +573,8 @@ func (h *handler) validateAndWriteHeaders(method handlerMethod, accessPermission // even though `checkAdminAuth` _can_ issue whoami to find user's roles, it doesn't always... // to reduce code complexity, we'll potentially be making this whoami request twice if we need it for audit filtering - var auditRoleNames []string - if needRolesForAudit(dbContext, base.UserDomainCBServer) { - whoAmIResponse, whoAmIStatus, whoAmIErr := cbRBACWhoAmI(h.ctx(), httpClient, managementEndpoints, username, password) - if whoAmIErr != nil || whoAmIStatus != http.StatusOK { - base.WarnfCtx(h.ctx(), "An error occurred whilst fetching the user's roles for audit filtering - will not filter: %v", whoAmIErr) - } else { - for _, role := range whoAmIResponse.Roles { - // only filter roles applied to this bucket or cluster-scope - if bucketName == authScope || bucketName == "" { - auditRoleNames = append(auditRoleNames, role.RoleName) - } - } - } - } + auditRoleNames := getCBUserRolesForAudit(dbContext, authScope, h.ctx(), httpClient, managementEndpoints, username, password) + h.authorizedAdminUser = username h.permissionsResults = permissions h.rqCtx = base.UserLogCtx(h.ctx(), username, base.UserDomainCBServer, auditRoleNames) @@ -678,6 +666,27 @@ func (h *handler) validateAndWriteHeaders(method handlerMethod, accessPermission return nil } +func getCBUserRolesForAudit(db *db.DatabaseContext, authScope string, ctx context.Context, httpClient *http.Client, managementEndpoints []string, username, password string) []string { + if !needRolesForAudit(db, base.UserDomainCBServer) { + return nil + } + + whoAmIResponse, whoAmIStatus, whoAmIErr := cbRBACWhoAmI(ctx, httpClient, managementEndpoints, username, password) + if whoAmIErr != nil || whoAmIStatus != http.StatusOK { + base.WarnfCtx(ctx, "An error occurred whilst fetching the user's roles for audit filtering - will not filter: %v", whoAmIErr) + return nil + } + + var auditRoleNames []string + for _, role := range whoAmIResponse.Roles { + // only filter roles applied to this bucket or cluster-scope + if role.BucketName == "" || role.BucketName == authScope { + auditRoleNames = append(auditRoleNames, role.RoleName) + } + } + return auditRoleNames +} + // removeCorruptConfigIfExists will remove the config from the bucket and remove it from the map if it exists on the invalid database config map func (h *handler) removeCorruptConfigIfExists(ctx context.Context, bucket, configGroupID, dbName string) error { _, ok := h.server.invalidDatabaseConfigTracking.exists(dbName) From ca64060d6a3f9681d67933806982961f4fce05a2 Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Mon, 22 Jul 2024 16:00:50 +0100 Subject: [PATCH 8/9] pr feedback --- base/logger_audit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/logger_audit.go b/base/logger_audit.go index f36718cf00..17518307bd 100644 --- a/base/logger_audit.go +++ b/base/logger_audit.go @@ -214,7 +214,7 @@ func (al *AuditLogger) shouldLog(id AuditID, ctx context.Context) bool { // shouldLogAuditEventForUserAndRole returns true if the request should be logged func shouldLogAuditEventForUserAndRole(logCtx *LogContext) bool { - if logCtx.UserDomain == "" && logCtx.Username == "" || + if (logCtx.UserDomain == "" && logCtx.Username == "") || len(logCtx.DbLogConfig.Audit.DisabledRoles) == 0 && len(logCtx.DbLogConfig.Audit.DisabledUsers) == 0 { // early return for common cases: no user on context or no disabled users or roles return true From b4cdf9457b534436ddeabb2eea5a3b148790a02d Mon Sep 17 00:00:00 2001 From: Ben Brooks <ben.brooks@couchbase.com> Date: Mon, 22 Jul 2024 16:04:24 +0100 Subject: [PATCH 9/9] tweak needRolesForAudit check --- rest/handler.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rest/handler.go b/rest/handler.go index e314bb6a52..08a3f8ec6e 100644 --- a/rest/handler.go +++ b/rest/handler.go @@ -824,14 +824,16 @@ func (h *handler) logStatus(status int, message string) { } func needRolesForAudit(db *db.DatabaseContext, domain base.UserIDDomain) bool { + // early returns when auditing is disabled if db == nil || db.Options.LoggingConfig == nil || db.Options.LoggingConfig.Audit == nil || - len(db.Options.LoggingConfig.Audit.DisabledRoles) == 0 { + !db.Options.LoggingConfig.Audit.Enabled { return false } // if we have any matching domains then we need the given roles + // this loop should be ~free for len(0) for principal := range db.Options.LoggingConfig.Audit.DisabledRoles { if principal.Domain == string(domain) { return true