From b1b65486110bb7dd7b89e725a19ed1689a313059 Mon Sep 17 00:00:00 2001 From: "STeve (Xin) Huang" Date: Wed, 15 Jan 2025 10:47:41 -0500 Subject: [PATCH] GitHub Proxy: complete audit event flow and add an enterprise guard (#51049) --- .../integrationv1/credentials_test.go | 3 +- .../integration/integrationv1/github_test.go | 3 +- lib/auth/integration/integrationv1/service.go | 5 +- .../integration/integrationv1/service_test.go | 4 +- lib/events/dynamic.go | 2 + lib/srv/ctx.go | 4 +- lib/srv/git/forward.go | 90 +++++++++++++++++-- lib/srv/git/forward_test.go | 74 ++++++++++++++- 8 files changed, 173 insertions(+), 12 deletions(-) diff --git a/lib/auth/integration/integrationv1/credentials_test.go b/lib/auth/integration/integrationv1/credentials_test.go index 6e44eafe0a6cb..f227ffa59b366 100644 --- a/lib/auth/integration/integrationv1/credentials_test.go +++ b/lib/auth/integration/integrationv1/credentials_test.go @@ -29,10 +29,11 @@ import ( integrationpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/integration/v1" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/authz" + "github.com/gravitational/teleport/lib/modules" ) func TestExportIntegrationCertAuthorities(t *testing.T) { - t.Parallel() + modules.SetTestModules(t, &modules.TestModules{TestBuildType: modules.BuildEnterprise}) ca := newCertAuthority(t, types.HostCA, "test-cluster") ctx, localClient, resourceSvc := initSvc(t, ca, ca.GetClusterName(), "127.0.0.1") diff --git a/lib/auth/integration/integrationv1/github_test.go b/lib/auth/integration/integrationv1/github_test.go index 44fccf55ccb4a..35ae05f3f8f78 100644 --- a/lib/auth/integration/integrationv1/github_test.go +++ b/lib/auth/integration/integrationv1/github_test.go @@ -32,10 +32,11 @@ import ( "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/cryptosuites" + "github.com/gravitational/teleport/lib/modules" ) func TestGenerateGitHubUserCert(t *testing.T) { - t.Parallel() + modules.SetTestModules(t, &modules.TestModules{TestBuildType: modules.BuildEnterprise}) ca := newCertAuthority(t, types.HostCA, "test-cluster") ctx, _, resourceSvc := initSvc(t, ca, ca.GetClusterName(), "127.0.0.1.nip.io") diff --git a/lib/auth/integration/integrationv1/service.go b/lib/auth/integration/integrationv1/service.go index 2bb00fe4543b1..5f673dd58714d 100644 --- a/lib/auth/integration/integrationv1/service.go +++ b/lib/auth/integration/integrationv1/service.go @@ -36,6 +36,7 @@ import ( "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/cryptosuites" "github.com/gravitational/teleport/lib/events" + "github.com/gravitational/teleport/lib/modules" "github.com/gravitational/teleport/lib/services" ) @@ -223,7 +224,9 @@ func (s *Service) CreateIntegration(ctx context.Context, req *integrationpb.Crea switch req.Integration.GetSubKind() { case types.IntegrationSubKindGitHub: - // TODO(greedy52) add entitlement check + if modules.GetModules().BuildType() != modules.BuildEnterprise { + return nil, trace.AccessDenied("GitHub integration requires a Teleport Enterprise license") + } if err := s.createGitHubCredentials(ctx, req.Integration); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/integration/integrationv1/service_test.go b/lib/auth/integration/integrationv1/service_test.go index 10a1e03eff206..32ed9740618ae 100644 --- a/lib/auth/integration/integrationv1/service_test.go +++ b/lib/auth/integration/integrationv1/service_test.go @@ -37,13 +37,15 @@ import ( "github.com/gravitational/teleport/lib/backend/memory" "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/fixtures" + "github.com/gravitational/teleport/lib/modules" "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/services/local" "github.com/gravitational/teleport/lib/tlsca" ) func TestIntegrationCRUD(t *testing.T) { - t.Parallel() + modules.SetTestModules(t, &modules.TestModules{TestBuildType: modules.BuildEnterprise}) + clusterName := "test-cluster" proxyPublicAddr := "127.0.0.1.nip.io" diff --git a/lib/events/dynamic.go b/lib/events/dynamic.go index de975792b0db7..f8784a646a77e 100644 --- a/lib/events/dynamic.go +++ b/lib/events/dynamic.go @@ -390,6 +390,8 @@ func FromEventFields(fields EventFields) (events.AuditEvent, error) { e = &events.AccessGraphSettingsUpdate{} case DatabaseSessionSpannerRPCEvent: e = &events.SpannerRPC{} + case GitCommandEvent: + e = &events.GitCommand{} case UnknownEvent: e = &events.Unknown{} diff --git a/lib/srv/ctx.go b/lib/srv/ctx.go index 3318b3755f92c..492cdce87d7f5 100644 --- a/lib/srv/ctx.go +++ b/lib/srv/ctx.go @@ -840,7 +840,9 @@ func (c *ServerContext) reportStats(conn utils.Stater) { // Never emit session data events for the proxy or from a Teleport node if // sessions are being recorded at the proxy (this would result in double // events). - if c.GetServer().Component() == teleport.ComponentProxy { + // Do not emit session data for git commands as they have their own events. + if c.GetServer().Component() == teleport.ComponentProxy || + c.GetServer().Component() == teleport.ComponentForwardingGit { return } if services.IsRecordAtProxy(c.SessionRecordingConfig.GetMode()) && diff --git a/lib/srv/git/forward.go b/lib/srv/git/forward.go index 1e7dae1ada6b3..ce0dc2fb23dad 100644 --- a/lib/srv/git/forward.go +++ b/lib/srv/git/forward.go @@ -23,6 +23,7 @@ import ( "io" "log/slog" "net" + "strconv" "github.com/google/uuid" "github.com/gravitational/trace" @@ -323,12 +324,19 @@ func (s *ForwardServer) onConnection(ctx context.Context, ccx *sshutils.Connecti s.logger.Log(ctx, logutils.TraceLevel, "New connection accepted") ccx.AddCloser(serverCtx) - return ctx, nil + return context.WithValue(ctx, serverContextKey{}, serverCtx), nil } func (s *ForwardServer) onChannel(ctx context.Context, ccx *sshutils.ConnectionContext, nch ssh.NewChannel) { s.logger.DebugContext(ctx, "Handling channel request", "channel", nch.ChannelType()) + serverCtx, ok := ctx.Value(serverContextKey{}).(*srv.ServerContext) + if !ok { + // This should not happen. Double check just in case. + s.reply.RejectChannel(ctx, nch, ssh.ResourceShortage, "server context not found") + return + } + // Only expecting a session to execute a command. if nch.ChannelType() != teleport.ChanSession { s.reply.RejectUnknownChannel(ctx, nch) @@ -353,7 +361,7 @@ func (s *ForwardServer) onChannel(ctx context.Context, ccx *sshutils.ConnectionC } defer ch.Close() - sctx := newSessionContext(ch, remoteSession) + sctx := newSessionContext(serverCtx, ch, remoteSession) for { select { case req := <-in: @@ -382,13 +390,16 @@ func (s *ForwardServer) onChannel(ctx context.Context, ccx *sshutils.ConnectionC } type sessionContext struct { + *srv.ServerContext + channel ssh.Channel remoteSession *tracessh.Session waitExec chan error } -func newSessionContext(ch ssh.Channel, remoteSession *tracessh.Session) *sessionContext { +func newSessionContext(serverCtx *srv.ServerContext, ch ssh.Channel, remoteSession *tracessh.Session) *sessionContext { return &sessionContext{ + ServerContext: serverCtx, channel: ch, remoteSession: remoteSession, waitExec: make(chan error, 1), @@ -415,13 +426,27 @@ func (s *ForwardServer) dispatch(ctx context.Context, sctx *sessionContext, req } // handleExec proxies the Git command between client and the target server. -func (s *ForwardServer) handleExec(ctx context.Context, sctx *sessionContext, req *ssh.Request) error { +func (s *ForwardServer) handleExec(ctx context.Context, sctx *sessionContext, req *ssh.Request) (err error) { var r sshutils.ExecReq + defer func() { + if err != nil { + s.emitEvent(s.makeGitCommandEvent(sctx, r.Command, err)) + } + }() + if err := ssh.Unmarshal(req.Payload, &r); err != nil { return trace.Wrap(err, "failed to unmarshal exec request") } - // TODO(greedy52) enable command recorder for audit log + command, err := ParseSSHCommand(r.Command) + if err != nil { + return trace.Wrap(err) + } + if err := checkSSHCommand(s.cfg.TargetServer, command); err != nil { + return trace.Wrap(err) + } + recorder := NewCommandRecorder(ctx, *command) + sctx.remoteSession.Stdout = sctx.channel sctx.remoteSession.Stderr = sctx.channel.Stderr() remoteStdin, err := sctx.remoteSession.StdinPipe() @@ -430,7 +455,7 @@ func (s *ForwardServer) handleExec(ctx context.Context, sctx *sessionContext, re } go func() { defer remoteStdin.Close() - if _, err := io.Copy(remoteStdin, sctx.channel); err != nil { + if _, err := io.Copy(io.MultiWriter(remoteStdin, recorder), sctx.channel); err != nil { s.logger.WarnContext(ctx, "Failed to copy git command stdin", "error", err) } }() @@ -442,10 +467,61 @@ func (s *ForwardServer) handleExec(ctx context.Context, sctx *sessionContext, re go func() { execErr := sctx.remoteSession.Wait() sctx.waitExec <- execErr + s.emitEvent(s.makeGitCommandEventWithExecResult(sctx, recorder, execErr)) }() return nil } +func (s *ForwardServer) emitEvent(event apievents.AuditEvent) { + if err := s.cfg.Emitter.EmitAuditEvent(s.cfg.ParentContext, event); err != nil { + s.logger.WarnContext(s.cfg.ParentContext, "Failed to emit event", + "error", err, + "event_type", event.GetType(), + "event_code", event.GetCode(), + ) + } +} + +func (s *ForwardServer) makeGitCommandEvent(sctx *sessionContext, command string, err error) *apievents.GitCommand { + event := &apievents.GitCommand{ + Metadata: apievents.Metadata{ + Type: events.GitCommandEvent, + Code: events.GitCommandCode, + }, + UserMetadata: sctx.Identity.GetUserMetadata(), + SessionMetadata: sctx.GetSessionMetadata(), + CommandMetadata: apievents.CommandMetadata{ + Command: command, + }, + ConnectionMetadata: apievents.ConnectionMetadata{ + RemoteAddr: sctx.ServerConn.RemoteAddr().String(), + LocalAddr: sctx.ServerConn.LocalAddr().String(), + }, + ServerMetadata: s.TargetMetadata(), + } + if err != nil { + event.Metadata.Code = events.GitCommandFailureCode + event.Error = err.Error() + } + return event +} + +func (s *ForwardServer) makeGitCommandEventWithExecResult(sctx *sessionContext, recorder CommandRecorder, execErr error) *apievents.GitCommand { + event := s.makeGitCommandEvent(sctx, recorder.GetCommand().SSHCommand, execErr) + + event.ExitCode = strconv.Itoa(sshutils.ExitCodeFromExecError(execErr)) + event.Path = string(recorder.GetCommand().Repository) + event.Service = recorder.GetCommand().Service + + actions, err := recorder.GetActions() + if err != nil { + s.logger.WarnContext(s.cfg.ParentContext, "Failed to get actions from Git command recorder. No actions will be recorded in the event.", "error", err) + } else { + event.Actions = actions + } + return event +} + // handleEnv sets env on the target server. func (s *ForwardServer) handleEnv(ctx context.Context, sctx *sessionContext, req *ssh.Request) error { var e sshutils.EnvReqParams @@ -594,6 +670,8 @@ func (s *ForwardServer) GetHostSudoers() srv.HostSudoers { return nil } +type serverContextKey struct{} + const ( gitUser = "git" ) diff --git a/lib/srv/git/forward_test.go b/lib/srv/git/forward_test.go index c9e2ca0fcc1c6..5ca51bcef8261 100644 --- a/lib/srv/git/forward_test.go +++ b/lib/srv/git/forward_test.go @@ -28,18 +28,21 @@ import ( "time" "github.com/gravitational/trace" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/crypto/ssh" "github.com/gravitational/teleport/api/constants" tracessh "github.com/gravitational/teleport/api/observability/tracing/ssh" "github.com/gravitational/teleport/api/types" + apievents "github.com/gravitational/teleport/api/types/events" "github.com/gravitational/teleport/api/types/wrappers" apisshutils "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib/auth/authclient" "github.com/gravitational/teleport/lib/auth/testauthority" "github.com/gravitational/teleport/lib/backend/memory" "github.com/gravitational/teleport/lib/cryptosuites" + libevents "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/events/eventstest" "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/services/local" @@ -66,6 +69,7 @@ func TestForwardServer(t *testing.T) { verifyRemoteHost ssh.HostKeyCallback wantNewClientError bool verifyWithClient func(t *testing.T, ctx context.Context, client *tracessh.Client, m *mockGitHostingService) + verifyEvent func(t *testing.T, event apievents.AuditEvent) }{ { name: "success", @@ -85,6 +89,45 @@ func TestForwardServer(t *testing.T) { require.NoError(t, err) require.Equal(t, gitCommand, m.receivedExec.Command) }, + verifyEvent: func(t *testing.T, event apievents.AuditEvent) { + gitEvent, ok := event.(*apievents.GitCommand) + require.True(t, ok) + assert.Equal(t, libevents.GitCommandEvent, gitEvent.Metadata.Type) + assert.Equal(t, libevents.GitCommandCode, gitEvent.Metadata.Code) + assert.Equal(t, "alice", gitEvent.User) + assert.Equal(t, "0", gitEvent.CommandMetadata.ExitCode) + assert.Equal(t, "git-upload-pack", gitEvent.Service) + assert.Equal(t, "org/my-repo.git", gitEvent.Path) + }, + }, + { + name: "command failure", + allowedGitHubOrg: "*", + clientLogin: "git", + verifyRemoteHost: ssh.InsecureIgnoreHostKey(), + wantNewClientError: false, + verifyWithClient: func(t *testing.T, ctx context.Context, client *tracessh.Client, m *mockGitHostingService) { + m.exitCode = 1 + + session, err := client.NewSession(ctx) + require.NoError(t, err) + defer session.Close() + + gitCommand := "git-receive-pack 'org/my-repo.git'" + session.Stderr = io.Discard + session.Stdout = io.Discard + require.Error(t, session.Run(ctx, gitCommand)) + }, + verifyEvent: func(t *testing.T, event apievents.AuditEvent) { + gitEvent, ok := event.(*apievents.GitCommand) + require.True(t, ok) + assert.Equal(t, libevents.GitCommandEvent, gitEvent.Metadata.Type) + assert.Equal(t, libevents.GitCommandFailureCode, gitEvent.Metadata.Code) + assert.Equal(t, "alice", gitEvent.User) + assert.Equal(t, "1", gitEvent.CommandMetadata.ExitCode) + assert.Equal(t, "git-receive-pack", gitEvent.Service) + assert.Equal(t, "org/my-repo.git", gitEvent.Path) + }, }, { name: "failed RBAC", @@ -124,6 +167,31 @@ func TestForwardServer(t *testing.T) { require.Contains(t, err.Error(), "unknown channel type") }, }, + { + name: "org mismatch", + allowedGitHubOrg: "*", + clientLogin: "git", + verifyRemoteHost: ssh.InsecureIgnoreHostKey(), + wantNewClientError: false, + verifyWithClient: func(t *testing.T, ctx context.Context, client *tracessh.Client, m *mockGitHostingService) { + session, err := client.NewSession(ctx) + require.NoError(t, err) + defer session.Close() + + gitCommand := "git-upload-pack 'some-other-org/my-repo.git'" + session.Stderr = io.Discard + session.Stdout = io.Discard + require.Error(t, session.Run(ctx, gitCommand)) + }, + verifyEvent: func(t *testing.T, event apievents.AuditEvent) { + gitEvent, ok := event.(*apievents.GitCommand) + require.True(t, ok) + assert.Equal(t, libevents.GitCommandEvent, gitEvent.Metadata.Type) + assert.Equal(t, libevents.GitCommandFailureCode, gitEvent.Metadata.Code) + assert.Equal(t, "alice", gitEvent.User) + assert.Contains(t, gitEvent.Error, "expect organization") + }, + }, } for _, test := range tests { @@ -189,6 +257,9 @@ func TestForwardServer(t *testing.T) { defer client.Close() test.verifyWithClient(t, ctx, client, mockGitService) + if test.verifyEvent != nil { + test.verifyEvent(t, mockEmitter.LastEvent()) + } }) } @@ -248,6 +319,7 @@ type mockGitHostingService struct { *sshutils.Server *sshutils.Reply receivedExec sshutils.ExecReq + exitCode int } func newMockGitHostingService(t *testing.T, caSigner ssh.Signer) *mockGitHostingService { @@ -300,7 +372,7 @@ func (m *mockGitHostingService) HandleNewChan(ctx context.Context, ccx *sshutils m.ReplyRequest(ctx, req, true, nil) } slog.DebugContext(ctx, "mock git service receives new exec request", "req", m.receivedExec) - m.SendExitStatus(ctx, ch, 0) + m.SendExitStatus(ctx, ch, m.exitCode) return case <-ctx.Done():