Skip to content

Commit

Permalink
Convert lib/services to use slog (#50348)
Browse files Browse the repository at this point in the history
  • Loading branch information
rosstimothy authored Dec 21, 2024
1 parent 5e097c4 commit 659db2b
Show file tree
Hide file tree
Showing 34 changed files with 436 additions and 359 deletions.
2 changes: 1 addition & 1 deletion lib/auth/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -1080,7 +1080,7 @@ func createPresetRoles(ctx context.Context, rm PresetRoleManager) error {
return trace.Wrap(err)
}

role, err := services.AddRoleDefaults(currentRole)
role, err := services.AddRoleDefaults(gctx, currentRole)
if trace.IsAlreadyExists(err) {
return nil
}
Expand Down
98 changes: 68 additions & 30 deletions lib/services/access_checker.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ import (
"cmp"
"context"
"fmt"
"log/slog"
"net"
"slices"
"strings"
"time"

"github.com/gravitational/trace"
log "github.com/sirupsen/logrus"
"golang.org/x/crypto/ssh"

"github.com/gravitational/teleport/api/constants"
Expand All @@ -39,6 +39,7 @@ import (
"github.com/gravitational/teleport/lib/services/readonly"
"github.com/gravitational/teleport/lib/tlsca"
"github.com/gravitational/teleport/lib/utils"
logutils "github.com/gravitational/teleport/lib/utils/log"
)

// AccessChecker interface checks access to resources based on roles, traits,
Expand Down Expand Up @@ -406,10 +407,11 @@ func (a *accessChecker) checkAllowedResources(r AccessCheckable) error {
return nil
}

// Note: logging in this function only happens in debug mode. This is because
// Note: logging in this function only happens in trace mode. This is because
// adding logging to this function (which is called on every resource returned
// by the backend) can slow down this function by 50x for large clusters!
isDebugEnabled, debugf := rbacDebugLogger()
ctx := context.Background()
isLoggingEnabled := rbacLogger.Enabled(ctx, logutils.TraceLevel)

for _, resourceID := range a.info.AllowedResourceIDs {
if resourceID.ClusterName == a.localCluster &&
Expand All @@ -421,23 +423,33 @@ func (a *accessChecker) checkAllowedResources(r AccessCheckable) error {
(resourceID.Kind == r.GetKind() || (slices.Contains(types.KubernetesResourcesKinds, resourceID.Kind) && r.GetKind() == types.KindKubernetesCluster)) &&
resourceID.Name == r.GetName() {
// Allowed to access this resource by resource ID, move on to role checks.
if isDebugEnabled {
debugf("Matched allowed resource ID %q", types.ResourceIDToString(resourceID))

if isLoggingEnabled {
rbacLogger.LogAttrs(ctx, logutils.TraceLevel, "Matched allowed resource ID",
slog.String("resource_id", types.ResourceIDToString(resourceID)),
)
}

return nil
}
}

if isDebugEnabled {
if isLoggingEnabled {
allowedResources, err := types.ResourceIDsToString(a.info.AllowedResourceIDs)
if err != nil {
return trace.Wrap(err)
}
err = trace.AccessDenied("access to %v denied, %q not in allowed resource IDs %s",

slog.LogAttrs(ctx, logutils.TraceLevel, "Access to resource denied, not in allowed resource IDs",
slog.String("resource_kind", r.GetKind()),
slog.String("resource_name", r.GetName()),
slog.Any("allowed_resources", allowedResources),
)

return trace.AccessDenied("access to %v denied, %q not in allowed resource IDs %s",
r.GetKind(), r.GetName(), allowedResources)
debugf("Access denied: %v", err)
return err
}

return trace.AccessDenied("access to %v denied, not in allowed resource IDs", r.GetKind())
}

Expand Down Expand Up @@ -875,10 +887,11 @@ func (a *accessChecker) CheckAccessToRemoteCluster(rc types.RemoteCluster) error
return trace.AccessDenied("access to cluster denied")
}

// Note: logging in this function only happens in debug mode, this is because
// Note: logging in this function only happens in trace mode, this is because
// adding logging to this function (which is called on every server returned
// by GetRemoteClusters) can slow down this function by 50x for large clusters!
isDebugEnabled, debugf := rbacDebugLogger()
ctx := context.Background()
isLoggingEnabled := rbacLogger.Enabled(ctx, logutils.TraceLevel)

rcLabels := rc.GetMetadata().Labels

Expand All @@ -898,53 +911,68 @@ func (a *accessChecker) CheckAccessToRemoteCluster(rc types.RemoteCluster) error
}

if !usesLabels && len(rcLabels) == 0 {
debugf("Grant access to cluster %v - no role in %v uses cluster labels and the cluster is not labeled.",
rc.GetName(), a.RoleNames())
rbacLogger.LogAttrs(ctx, logutils.TraceLevel, "Grant access to cluster - no role uses cluster labels and the cluster is not labeled",
slog.String("cluster_name", rc.GetName()),
slog.Any("roles", a.RoleNames()),
)
return nil
}

// Check deny rules first: a single matching label from
// the deny role set prohibits access.
var errs []error
for _, role := range a.RoleSet {
matchLabels, labelsMessage, err := checkRoleLabelsMatch(types.Deny, role, a.info.Traits, rc, isDebugEnabled)
matchLabels, labelsMessage, err := checkRoleLabelsMatch(types.Deny, role, a.info.Traits, rc, isLoggingEnabled)
if err != nil {
return trace.Wrap(err)
}
if matchLabels {
// This condition avoids formatting calls on large scale.
debugf("Access to cluster %v denied, deny rule in %v matched; match(%s)",
rc.GetName(), role.GetName(), labelsMessage)
rbacLogger.LogAttrs(ctx, logutils.TraceLevel, "Access to cluster denied, deny rule matched",
slog.String("cluster", rc.GetName()),
slog.String("role", role.GetName()),
slog.String("label_message", labelsMessage),
)
return trace.AccessDenied("access to cluster denied")
}
}

// Check allow rules: label has to match in any role in the role set to be granted access.
for _, role := range a.RoleSet {
matchLabels, labelsMessage, err := checkRoleLabelsMatch(types.Allow, role, a.info.Traits, rc, isDebugEnabled)
matchLabels, labelsMessage, err := checkRoleLabelsMatch(types.Allow, role, a.info.Traits, rc, isLoggingEnabled)
if err != nil {
return trace.Wrap(err)
}
labelMatchers, err := role.GetLabelMatchers(types.Allow, types.KindRemoteCluster)
if err != nil {
return trace.Wrap(err)
}
debugf("Check access to role(%v) rc(%v, labels=%v) matchLabels=%v, msg=%v, err=%v allow=%v rcLabels=%v",
role.GetName(), rc.GetName(), rcLabels, matchLabels, labelsMessage, err, labelMatchers, rcLabels)
rbacLogger.LogAttrs(ctx, logutils.TraceLevel, "Check access to role",
slog.String("role", role.GetName()),
slog.String("cluster", rc.GetName()),
slog.Any("cluster_labels", rcLabels),
slog.Any("match_labels", matchLabels),
slog.String("labels_message", labelsMessage),
slog.Any("error", err),
slog.Any("allow", labelMatchers),
)
if err != nil {
return trace.Wrap(err)
}
if matchLabels {
return nil
}
if isDebugEnabled {
if isLoggingEnabled {
deniedError := trace.AccessDenied("role=%v, match(%s)",
role.GetName(), labelsMessage)
errs = append(errs, deniedError)
}
}

debugf("Access to cluster %v denied, no allow rule matched; %v", rc.GetName(), errs)
rbacLogger.LogAttrs(ctx, logutils.TraceLevel, "Access to cluster denied, no allow rule matched",
slog.String("cluster", rc.GetName()),
slog.Any("error", errs),
)
return trace.AccessDenied("access to cluster denied")
}

Expand Down Expand Up @@ -1095,7 +1123,10 @@ func (a *accessChecker) HostUsers(s types.Server) (*HostUsersInfo, error) {
fmt.Fprintf(b, "%s=%v ", shell, roles)
}

log.Warnf("Host user shell resolution is ambiguous due to conflicting roles. %q will be used, but consider unifying roles around a single shell. Current shell assignments: %s", shell, b)
slog.WarnContext(context.Background(), "Host user shell resolution is ambiguous due to conflicting roles, consider unifying roles around a single shell",
"selected_shell", shell,
"shell_assignments", b,
)
}

for _, role := range a.RoleSet {
Expand Down Expand Up @@ -1247,8 +1278,11 @@ func AccessInfoFromRemoteCertificate(cert *ssh.Certificate, roleMap types.RoleMa
if len(roles) == 0 {
return nil, trace.AccessDenied("no roles mapped for user with remote roles %v", unmappedRoles)
}
log.Debugf("Mapped remote roles %v to local roles %v and traits %v.",
unmappedRoles, roles, traits)
slog.DebugContext(context.Background(), "Mapped remote roles to local roles and traits",
"remote_roles", unmappedRoles,
"local_roles", roles,
"traits", traits,
)

allowedResourceIDs, err := ExtractAllowedResourcesFromCert(cert)
if err != nil {
Expand Down Expand Up @@ -1281,10 +1315,10 @@ func AccessInfoFromLocalIdentity(identity tlsca.Identity, access UserGetter) (*A
return nil, trace.Wrap(err)
}

log.Warnf("Failed to find roles in x509 identity for %v. Fetching "+
"from backend. If the identity provider allows username changes, this can "+
"potentially allow an attacker to change the role of the existing user.",
identity.Username)
const msg = "Failed to find roles in x509 identity. Fetching " +
"from backend. If the identity provider allows username changes, this can " +
"potentially allow an attacker to change the role of the existing user."
slog.WarnContext(context.Background(), msg, "username", identity.Username)
roles = u.GetRoles()
traits = u.GetTraits()
}
Expand Down Expand Up @@ -1335,8 +1369,12 @@ func AccessInfoFromRemoteIdentity(identity tlsca.Identity, roleMap types.RoleMap
if len(roles) == 0 {
return nil, trace.AccessDenied("no roles mapped for remote user %q from cluster %q with remote roles %v", identity.Username, identity.TeleportCluster, unmappedRoles)
}
log.Debugf("Mapped roles %v of remote user %q to local roles %v and traits %v.",
unmappedRoles, identity.Username, roles, traits)
slog.DebugContext(context.Background(), "Mapped roles of remote user to local roles and traits",
"remote_roles", unmappedRoles,
"user", identity.Username,
"local_roles", roles,
"traits", traits,
)

allowedResourceIDs := identity.AllowedResourceIDs

Expand Down
8 changes: 4 additions & 4 deletions lib/services/access_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
apiutils "github.com/gravitational/teleport/api/utils"
"github.com/gravitational/teleport/lib/tlsca"
"github.com/gravitational/teleport/lib/utils"
logutils "github.com/gravitational/teleport/lib/utils/log"
"github.com/gravitational/teleport/lib/utils/parse"
"github.com/gravitational/teleport/lib/utils/typical"
)
Expand Down Expand Up @@ -2170,10 +2171,9 @@ func (m *RequestValidator) pruneResourceRequestRoles(

for _, resourceID := range resourceIDs {
if resourceID.ClusterName != localClusterName {
_, debugf := rbacDebugLogger()
debugf("Requested resource %q is in a foreign cluster, unable to prune roles. "+
`All available "search_as_roles" will be requested.`,
types.ResourceIDToString(resourceID))
rbacLogger.LogAttrs(ctx, logutils.TraceLevel, `Requested resource is in a foreign cluster, unable to prune roles - All available "search_as_roles" will be requested`,
slog.Any("requested_resources", types.ResourceIDToString(resourceID)),
)
return roles, nil
}
}
Expand Down
8 changes: 6 additions & 2 deletions lib/services/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ package services
import (
"context"
"errors"
"log/slog"
"net"
"net/netip"
"net/url"
"slices"
"strings"

"github.com/gravitational/trace"
log "github.com/sirupsen/logrus"
"go.mongodb.org/mongo-driver/mongo/readpref"
"go.mongodb.org/mongo-driver/x/mongo/driver/connstring"

Expand Down Expand Up @@ -277,7 +277,11 @@ func validateMongoDB(db types.Database) error {
// DNS errors here by replacing the scheme and then ParseAndValidate again
// to validate as much as we can.
if isDNSError(err) {
log.Warnf("MongoDB database %q (connection string %q) failed validation with DNS error: %v.", db.GetName(), db.GetURI(), err)
slog.WarnContext(context.Background(), "MongoDB database %q (connection string %q) failed validation with DNS error",
"database_name", db.GetName(),
"database_uri", db.GetURI(),
"error", err,
)

connString, err = connstring.ParseAndValidate(strings.Replace(
db.GetURI(),
Expand Down
11 changes: 7 additions & 4 deletions lib/services/local/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ package local

import (
"context"
"log/slog"
"strings"
"time"

"github.com/gravitational/trace"
"github.com/sirupsen/logrus"

"github.com/gravitational/teleport"
"github.com/gravitational/teleport/api/client/proto"
Expand All @@ -36,14 +36,14 @@ import (
// AccessService manages roles
type AccessService struct {
backend.Backend
log *logrus.Entry
logger *slog.Logger
}

// NewAccessService returns new access service instance
func NewAccessService(backend backend.Backend) *AccessService {
return &AccessService{
Backend: backend,
log: logrus.WithFields(logrus.Fields{teleport.ComponentKey: "AccessService"}),
logger: slog.With(teleport.ComponentKey, "AccessService"),
}
}

Expand Down Expand Up @@ -124,7 +124,10 @@ func (s *AccessService) ListRoles(ctx context.Context, req *proto.ListRolesReque
services.WithRevision(item.Revision),
)
if err != nil {
s.log.Warnf("Failed to unmarshal role at %q: %v", item.Key, err)
s.logger.WarnContext(ctx, "Failed to unmarshal role",
"key", item.Key,
"error", err,
)
continue
}

Expand Down
4 changes: 0 additions & 4 deletions lib/services/local/access_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ import (
"github.com/google/uuid"
"github.com/gravitational/trace"
"github.com/jonboulle/clockwork"
"github.com/sirupsen/logrus"

"github.com/gravitational/teleport"
accesslistv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/accesslist/v1"
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/api/types/accesslist"
Expand Down Expand Up @@ -75,7 +73,6 @@ const (
// consistent view to the rest of the Teleport application. It makes no decisions
// about granting or withholding list membership.
type AccessListService struct {
log logrus.FieldLogger
clock clockwork.Clock
service *generic.Service[*accesslist.AccessList]
memberService *generic.Service[*accesslist.AccessListMember]
Expand Down Expand Up @@ -144,7 +141,6 @@ func NewAccessListService(b backend.Backend, clock clockwork.Clock, opts ...Serv
}

return &AccessListService{
log: logrus.WithFields(logrus.Fields{teleport.ComponentKey: "access-list:local-service"}),
clock: clock,
service: service,
memberService: memberService,
Expand Down
11 changes: 7 additions & 4 deletions lib/services/local/dynamic_access.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ package local

import (
"context"
"log/slog"
"slices"
"time"

"github.com/gravitational/trace"
"github.com/sirupsen/logrus"

"github.com/gravitational/teleport"
"github.com/gravitational/teleport/api/client/proto"
Expand All @@ -38,14 +38,14 @@ import (
// DynamicAccessService manages dynamic RBAC
type DynamicAccessService struct {
backend.Backend
log *logrus.Entry
logger *slog.Logger
}

// NewDynamicAccessService returns new dynamic access service instance
func NewDynamicAccessService(backend backend.Backend) *DynamicAccessService {
return &DynamicAccessService{
Backend: backend,
log: logrus.WithFields(logrus.Fields{teleport.ComponentKey: "DynamicAccess"}),
logger: slog.With(teleport.ComponentKey, "DynamicAccess"),
}
}

Expand Down Expand Up @@ -348,7 +348,10 @@ func (s *DynamicAccessService) ListAccessRequests(ctx context.Context, req *prot

accessRequest, err := itemToAccessRequest(item)
if err != nil {
s.log.Warnf("Failed to unmarshal access request at %q: %v", item.Key, err)
s.logger.WarnContext(ctx, "Failed to unmarshal access request",
"key", item.Key,
"error", err,
)
continue
}

Expand Down
Loading

0 comments on commit 659db2b

Please sign in to comment.