Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/reversetunnel/local_cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ func (s *localCluster) dialAndForward(params reversetunnelclient.DialParams) (_
ctx := s.srv.ctx

if params.GetUserAgent == nil && !params.TargetServer.IsOpenSSHNode() {
return nil, trace.BadParameter("agentless node require an agent getter")
return nil, trace.BadParameter("user agent getter is required for teleport nodes (this is a bug)")
}
s.logger.DebugContext(ctx, "Initiating dial and forwarding request",
"source_addr", logutils.StringerAttr(params.From),
Expand Down
13 changes: 8 additions & 5 deletions lib/srv/forward/sshserver.go
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write a test that would have caught this bug?

Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a basic test for CheckDefaults. I'm not sure about adding a more in-depth test (e.g. fully mocked EICE flow in an integration test) given that the EICE flow is deprecated.

Original file line number Diff line number Diff line change
Expand Up @@ -268,14 +268,17 @@ func (s *ServerConfig) CheckDefaults() error {
if s.TargetServer == nil {
return trace.BadParameter("target server is required")
}
if s.TargetServer.IsOpenSSHNode() {
switch s.TargetServer.GetSubKind() {
case types.SubKindTeleportNode:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a "" case too just to be safe? I'm not sure we enforce that the subkind must be "teleport" for regular node heartbeats.

Suggested change
case types.SubKindTeleportNode:
case types.SubKindTeleportNode, "":

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SubKind() defaults to Teleport Node if unset:

func (s *ServerV2) GetSubKind() string {
// if the server is a node subkind isn't set, this is a teleport node.
if s.Kind == KindNode && s.SubKind == "" {
return SubKindTeleportNode
}

if s.UserAgent == nil {
return trace.BadParameter("user agent required for teleport nodes (agentless)")
}
case types.SubKindOpenSSHNode:
if s.AgentlessSigner == nil {
return trace.BadParameter("agentless signer is required for OpenSSH Nodes")
}
} else {
if s.UserAgent == nil {
return trace.BadParameter("user agent required for teleport nodes")
}
case types.SubKindOpenSSHEICENode:
// agentless signer is set once the forwarding server is started.
}
if s.TargetConn == nil {
return trace.BadParameter("connection to target connection required")
Expand Down
113 changes: 113 additions & 0 deletions lib/srv/forward/sshserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,13 @@ import (
"crypto/ed25519"
"crypto/rand"
"errors"
"net"
"os/user"
"sync/atomic"
"testing"

"github.com/google/uuid"
"github.com/jonboulle/clockwork"
"github.com/stretchr/testify/require"
"golang.org/x/crypto/ssh"

Expand All @@ -37,7 +39,10 @@ import (
"github.com/gravitational/teleport/api/types/events"
"github.com/gravitational/teleport/api/utils/keys"
apisshutils "github.com/gravitational/teleport/api/utils/sshutils"
"github.com/gravitational/teleport/lib/auth/authclient"
"github.com/gravitational/teleport/lib/cryptosuites"
"github.com/gravitational/teleport/lib/fixtures"
"github.com/gravitational/teleport/lib/services"
"github.com/gravitational/teleport/lib/srv"
"github.com/gravitational/teleport/lib/sshutils"
"github.com/gravitational/teleport/lib/utils/log/logtest"
Expand Down Expand Up @@ -376,3 +381,111 @@ func TestEventMetadata(t *testing.T) {
})
}
}

func TestServerConfigCheckDefaults(t *testing.T) {
teleportNode, err := types.NewNode("teleport-node", "", types.ServerSpecV2{}, nil)
require.NoError(t, err)

openSSHNode, err := types.NewNode("openssh-node", types.SubKindOpenSSHNode, types.ServerSpecV2{
Addr: "openssh.example.com:22",
Hostname: "openssh.example.com",
}, nil)
require.NoError(t, err)

openSSHEICENode, err := types.NewEICENode(types.ServerSpecV2{
Addr: "openssheice.example.com:22",
Hostname: "openssheice.example.com",
CloudMetadata: &types.CloudMetadata{
AWS: &types.AWSInfo{
AccountID: "123456789012",
InstanceID: "i-123456789012",
Region: "us-east-1",
VPCID: "vpc-abcd",
SubnetID: "subnet-123",
Integration: "teleportdev",
},
},
}, nil)
require.NoError(t, err)

for _, tt := range []struct {
name string
modifyCfg func(c *ServerConfig)
errorAssertion require.ErrorAssertionFunc
}{
{
name: "no targetServer",
modifyCfg: func(c *ServerConfig) {},
errorAssertion: func(tt require.TestingT, err error, i ...interface{}) {
require.Error(t, err)
require.ErrorContains(t, err, "target server is required")
},
}, {
name: "Teleport Node",
modifyCfg: func(c *ServerConfig) {
c.TargetServer = teleportNode
c.UserAgent = &sshutils.AgentChannel{}
},
errorAssertion: require.NoError,
}, {
name: "Teleport Node no agent",
modifyCfg: func(c *ServerConfig) {
c.TargetServer = teleportNode
},
errorAssertion: func(tt require.TestingT, err error, i ...interface{}) {
require.Error(t, err)
require.ErrorContains(t, err, "user agent required")
},
}, {
name: "OpenSSH Node",
modifyCfg: func(c *ServerConfig) {
c.TargetServer = openSSHNode
c.AgentlessSigner = &sshutils.LegacySHA1Signer{}
},
errorAssertion: require.NoError,
}, {
name: "OpenSSH Node no signer",
modifyCfg: func(c *ServerConfig) {
c.TargetServer = openSSHNode
},
errorAssertion: func(tt require.TestingT, err error, i ...interface{}) {
require.Error(t, err)
require.ErrorContains(t, err, "agentless signer is required")
},
}, {
name: "OpenSSH EICE Node",
modifyCfg: func(c *ServerConfig) {
c.TargetServer = openSSHEICENode
},
errorAssertion: require.NoError,
},
} {
t.Run(tt.name, func(t *testing.T) {
config := &ServerConfig{
LocalAuthClient: &authclient.Client{},
TargetClusterAccessPoint: &authclient.Client{},
DataDir: "datadir",
TargetConn: &net.UnixConn{},
SrcAddr: &net.IPAddr{},
DstAddr: &net.IPAddr{},
HostCertificate: &sshutils.LegacySHA1Signer{},
Clock: clockwork.NewFakeClock(),
Emitter: &authclient.Client{},
LockWatcher: &services.LockWatcher{},
EICESigner: func(ctx context.Context, target types.Server, integration types.Integration, login, token string, ap cryptosuites.AuthPreferenceGetter) (ssh.Signer, error) {
return nil, nil
},
}

tt.modifyCfg(config)

err := config.CheckDefaults()
tt.errorAssertion(t, err)
})
}

// UserAgent: userAgent,
// AgentlessSigner: params.AgentlessSigner,
// TargetServer: params.TargetServer,

}
Loading