Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v15] ensure moderated file transfers only perform allowed operations #39351

Merged
merged 2 commits into from Mar 14, 2024
Merged

[v15] ensure moderated file transfers only perform allowed operations #39351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
365773b
ensure moderated file transfers only perform allowed operations
capnspacehook Mar 14, 2024
53d1fff
Remove unnecessary buffered reader wrapper
zmb3 Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
331 changes: 331 additions & 0 deletions integration/integration_test.go
View file
Open in desktop
Expand Up @@ -100,6 +100,7 @@ import (
"github.com/gravitational/teleport/lib/session"
"github.com/gravitational/teleport/lib/srv/alpnproxy/common"
"github.com/gravitational/teleport/lib/sshutils"
telesftp "github.com/gravitational/teleport/lib/sshutils/sftp"
"github.com/gravitational/teleport/lib/sshutils/x11"
"github.com/gravitational/teleport/lib/tlsca"
"github.com/gravitational/teleport/lib/utils"
Expand Down Expand Up @@ -193,6 +194,7 @@ func TestIntegrations(t *testing.T) {
t.Run("DifferentPinnedIP", suite.bind(testDifferentPinnedIP))
t.Run("JoinOverReverseTunnelOnly", suite.bind(testJoinOverReverseTunnelOnly))
t.Run("SFTP", suite.bind(testSFTP))
t.Run("ModeratedSFTP", suite.bind(testModeratedSFTP))
t.Run("EscapeSequenceTriggers", suite.bind(testEscapeSequenceTriggers))
t.Run("AuthLocalNodeControlStream", suite.bind(testAuthLocalNodeControlStream))
t.Run("AgentlessConnection", suite.bind(testAgentlessConnection))
Expand Down Expand Up @@ -8038,6 +8040,332 @@ func getRemoteAddrString(sshClientString string) string {
return fmt.Sprintf("%s:%s", parts[0], parts[1])
}

func isNilOrEOFErr(t *testing.T, err error) {
t.Helper()

if err != nil {
require.ErrorIs(t, err, io.EOF)
}
}

func testModeratedSFTP(t *testing.T, suite *integrationTestSuite) {
modules.SetTestModules(t, &modules.TestModules{
TestBuildType: modules.BuildEnterprise,
})

// Create Teleport instance
instance := suite.newTeleport(t, nil, true)
t.Cleanup(func() {
instance.StopAll()
})

ctx := context.Background()
authServer := instance.Process.GetAuthServer()

// Create peer and moderator users and roles
username := suite.Me.Username
peerUsername := username + "-peer"
sshAccessRole, err := types.NewRole("ssh-access", types.RoleSpecV6{
Allow: types.RoleConditions{
Logins: []string{username},
NodeLabels: types.Labels{
types.Wildcard: []string{types.Wildcard},
},
},
})
require.NoError(t, err)
_, err = authServer.CreateRole(ctx, sshAccessRole)
require.NoError(t, err)

peerRole, err := types.NewRole("peer", types.RoleSpecV6{
Allow: types.RoleConditions{
RequireSessionJoin: []*types.SessionRequirePolicy{
{
Name: "Requires oversight",
Filter: `equals("true", "true")`,
Kinds: []string{
string(types.SSHSessionKind),
},
Count: 1,
Modes: []string{
string(types.SessionModeratorMode),
},
OnLeave: string(types.OnSessionLeaveTerminate),
},
},
},
})
require.NoError(t, err)
_, err = authServer.CreateRole(ctx, peerRole)
require.NoError(t, err)

peerUser, err := types.NewUser(peerUsername)
require.NoError(t, err)
peerUser.SetLogins([]string{username})
peerUser.SetRoles([]string{sshAccessRole.GetName(), peerRole.GetName()})
_, err = authServer.CreateUser(ctx, peerUser)
require.NoError(t, err)

modUsername := username + "-moderator"
modRole, err := types.NewRole("moderator", types.RoleSpecV6{
Allow: types.RoleConditions{
JoinSessions: []*types.SessionJoinPolicy{{
Name: "Session moderator",
Roles: []string{peerRole.GetName()},
Kinds: []string{string(types.SSHSessionKind)},
Modes: []string{string(types.SessionModeratorMode), string(types.SessionObserverMode)},
}},
},
})
require.NoError(t, err)
_, err = authServer.CreateRole(ctx, modRole)
require.NoError(t, err)

moderatorUser, err := types.NewUser(modUsername)
require.NoError(t, err)
moderatorUser.SetLogins([]string{username})
moderatorUser.SetRoles([]string{sshAccessRole.GetName(), modRole.GetName()})
_, err = authServer.CreateUser(ctx, moderatorUser)
require.NoError(t, err)

waitForNodesToRegister(t, instance, helpers.Site)

// Start a shell so a moderated session is created
peerClient, err := instance.NewClient(helpers.ClientConfig{
TeleportUser: peerUsername,
Login: username,
Cluster: helpers.Site,
Host: Host,
})
require.NoError(t, err)

peerClusterClient, err := peerClient.ConnectToCluster(ctx)
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, peerClusterClient.Close())
})

nodeDetails := client.NodeDetails{
Addr: instance.Config.SSH.Addr.Addr,
Namespace: peerClient.Namespace,
Cluster: helpers.Site,
}
peerNodeClient, err := peerClient.ConnectToNode(
ctx,
peerClusterClient,
nodeDetails,
username,
)
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, peerNodeClient.Close())
})

peerSSH := peerNodeClient.Client
peerSess, err := peerSSH.NewSession(ctx)
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, peerSess.Close())
})

peerTerm := NewTerminal(250)
peerSess.Stdin = peerTerm
peerSess.Stdout = peerTerm
peerSess.Stderr = peerTerm
err = peerSess.Shell(ctx)
require.NoError(t, err)

var sessTracker types.SessionTracker
require.EventuallyWithT(t, func(t *assert.CollectT) {
trackers, err := peerClusterClient.AuthClient.GetActiveSessionTrackers(ctx)
assert.NoError(t, err)
if assert.Len(t, trackers, 1) {
sessTracker = trackers[0]
}
}, 5*time.Second, 100*time.Millisecond)

// Join the waiting session so it is approved
modTC, err := instance.NewClient(helpers.ClientConfig{
TeleportUser: modUsername,
Login: username,
Cluster: helpers.Site,
Host: Host,
})
require.NoError(t, err)

modClusterClient, err := modTC.ConnectToCluster(ctx)
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, modClusterClient.Close())
})

conn, details, err := modClusterClient.ProxyClient.DialHost(ctx, nodeDetails.Addr, nodeDetails.Cluster, modTC.LocalAgent().ExtendedAgent)
require.NoError(t, err)
sshConfig := modClusterClient.ProxyClient.SSHConfig(username)
modSSHConn, modSSHChans, modSSHReqs, err := tracessh.NewClientConn(ctx, conn, nodeDetails.ProxyFormat(), sshConfig)
require.NoError(t, err)

// We pass an empty channel which we close right away to ssh.NewClient
// because the client need to handle requests itself.
emptyCh := make(chan *ssh.Request)
close(emptyCh)
modNodeCli := client.NodeClient{
Client: tracessh.NewClient(modSSHConn, modSSHChans, emptyCh),
Namespace: nodeDetails.Namespace,
TC: modTC,
Tracer: modTC.Tracer,
FIPSEnabled: details.FIPS,
ProxyPublicAddr: modTC.WebProxyAddr,
}

modSess, err := modNodeCli.Client.NewSession(ctx)
require.NoError(t, err)
err = modSess.Setenv(ctx, sshutils.SessionEnvVar, sessTracker.GetSessionID())
require.NoError(t, err)
err = modSess.Setenv(ctx, teleport.EnvSSHJoinMode, string(types.SessionModeratorMode))
require.NoError(t, err)

modTerm := NewTerminal(250)
modSess.Stdin = modTerm
modSess.Stdout = modTerm
modSess.Stderr = modTerm
err = modSess.Shell(ctx)
require.NoError(t, err)

// Create and approve a file download request
tempDir := t.TempDir()
reqFile := filepath.Join(tempDir, "req-file")
err = os.WriteFile(reqFile, []byte("contents"), 0o666)
require.NoError(t, err)

err = peerSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{
Download: true,
Location: reqFile,
})
require.NoError(t, err)

sshReq := <-modSSHReqs
var joinEvent apievents.SessionJoin
err = json.Unmarshal(sshReq.Payload, &joinEvent)
require.NoError(t, err)

sshReq = <-modSSHReqs
var fileReq apievents.FileTransferRequestEvent
err = json.Unmarshal(sshReq.Payload, &fileReq)
require.NoError(t, err)

err = modSess.ApproveFileTransferRequest(ctx, fileReq.RequestID)
require.NoError(t, err)

// Ignore file transfer request approve event
<-modSSHReqs

// Test that only operations needed to complete the download
// are allowed
transferSess, err := peerSSH.NewSession(ctx)
require.NoError(t, err)
t.Cleanup(func() {
isNilOrEOFErr(t, transferSess.Close())
})

err = transferSess.Setenv(ctx, string(telesftp.ModeratedSessionID), sessTracker.GetSessionID())
require.NoError(t, err)

err = transferSess.RequestSubsystem(ctx, teleport.SFTPSubsystem)
require.NoError(t, err)
w, err := transferSess.StdinPipe()
require.NoError(t, err)
r, err := transferSess.StdoutPipe()
require.NoError(t, err)
sftpClient, err := sftp.NewClientPipe(r, w)
require.NoError(t, err)

// A file not in the request shouldn't be allowed
_, err = sftpClient.Open(filepath.Join(tempDir, "bad-file"))
require.ErrorContains(t, err, `method get is not allowed`)
// Since this is a download no files should be allowed to be written to
_, err = sftpClient.OpenFile(filepath.Join(tempDir, reqFile), os.O_WRONLY)
require.ErrorContains(t, err, `method put is not allowed`)
// Only stats and reads should be allowed
err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir"))
require.ErrorContains(t, err, `method mkdir is not allowed`)
// Since this is a download no files should be allowed to have
// their permissions changed
err = sftpClient.Chmod(reqFile, 0o777)
require.ErrorContains(t, err, `method setstat is not allowed`)

// Only necessary operations should be allowed
_, err = sftpClient.Stat(reqFile)
require.NoError(t, err)
_, err = sftpClient.Lstat(reqFile)
require.NoError(t, err)
rf, err := sftpClient.Open(reqFile)
require.NoError(t, err)
require.NoError(t, rf.Close())

require.NoError(t, sftpClient.Close())

// Create and approve a file upload request
err = peerSess.RequestFileTransfer(ctx, tracessh.FileTransferReq{
Download: false,
Filename: "upload-file",
Location: reqFile,
})
require.NoError(t, err)

sshReq = <-modSSHReqs
err = json.Unmarshal(sshReq.Payload, &fileReq)
require.NoError(t, err)

err = modSess.ApproveFileTransferRequest(ctx, fileReq.RequestID)
require.NoError(t, err)

// Ignore file transfer request approve event
<-modSSHReqs

isNilOrEOFErr(t, transferSess.Close())
transferSess, err = peerSSH.NewSession(ctx)
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, transferSess.Close())
})

err = transferSess.Setenv(ctx, string(telesftp.ModeratedSessionID), sessTracker.GetSessionID())
require.NoError(t, err)

// Test that only operations needed to complete the download
// are allowed
err = transferSess.RequestSubsystem(ctx, teleport.SFTPSubsystem)
require.NoError(t, err)
w, err = transferSess.StdinPipe()
require.NoError(t, err)
r, err = transferSess.StdoutPipe()
require.NoError(t, err)
sftpClient, err = sftp.NewClientPipe(r, w)
require.NoError(t, err)

// A file not in the request shouldn't be allowed
_, err = sftpClient.Open(filepath.Join(tempDir, "bad-file"))
require.ErrorContains(t, err, `method get is not allowed`)
// Since this is an upload no files should be allowed to be read from
_, err = sftpClient.OpenFile(filepath.Join(tempDir, reqFile), os.O_RDONLY)
require.ErrorContains(t, err, `method get is not allowed`)
// Only stats, writes, and chmods should be allowed
err = sftpClient.Mkdir(filepath.Join(tempDir, "new-dir"))
require.ErrorContains(t, err, `method mkdir is not allowed`)

// Only necessary operations should be allowed
_, err = sftpClient.Stat(reqFile)
require.NoError(t, err)
_, err = sftpClient.Lstat(reqFile)
require.NoError(t, err)
err = sftpClient.Chmod(reqFile, 0o777)
require.NoError(t, err)
wf, err := sftpClient.OpenFile(reqFile, os.O_WRONLY)
require.NoError(t, err)
require.NoError(t, wf.Close())
}

func testSFTP(t *testing.T, suite *integrationTestSuite) {
// Create Teleport instance.
teleport := suite.newTeleport(t, nil, true)
Expand Down Expand Up @@ -8073,6 +8401,9 @@ func testSFTP(t *testing.T, suite *integrationTestSuite) {
suite.Me.Username,
)
require.NoError(t, err)
t.Cleanup(func() {
require.NoError(t, nodeClient.Close())
})

sftpClient, err := sftp.NewClient(nodeClient.Client.Client)
require.NoError(t, err)
Expand Down
24 changes: 24 additions & 0 deletions lib/srv/ctx.go
View file
Open in desktop
Expand Up @@ -452,6 +452,10 @@ type ServerContext struct {

// UserCreatedByTeleport is true when the system user was created by Teleport user auto-provision.
UserCreatedByTeleport bool

// approvedFileReq is an approved file transfer request that will only be
// set when the session's pending file transfer request is approved.
approvedFileReq *FileTransferRequest
}

// NewServerContext creates a new *ServerContext which is used to pass and
Expand Down Expand Up @@ -1380,3 +1384,23 @@ func (c *ServerContext) GetSessionMetadata() apievents.SessionMetadata {
PrivateKeyPolicy: c.Identity.Certificate.Extensions[teleport.CertExtensionPrivateKeyPolicy],
}
}

func (c *ServerContext) setApprovedFileTransferRequest(req *FileTransferRequest) {
c.mu.Lock()
c.approvedFileReq = req
c.mu.Unlock()
}

// ConsumeApprovedFileTransferRequest will return the approved file transfer
// request for this session if there is one present. Note that if an
// approved request is returned future calls to this method will return
// nil to prevent an approved request getting reused incorrectly.
func (c *ServerContext) ConsumeApprovedFileTransferRequest() *FileTransferRequest {
c.mu.Lock()
defer c.mu.Unlock()

req := c.approvedFileReq
c.approvedFileReq = nil

return req
}