ravicious
commented
9 months ago tl;dr scroll to the bottom
Approaches to adjusting diagnostic messages
The connection test for Connect My Computer will be identical to the SSH connection test, but the diagnostic messages need to be adjusted.
There's a bunch of different approaches that we can choose from.
Assume one possible error per trace type
Assume that during an SSH connection test there's only ever a single possible error per trace type and replace the messages with Connect My Computer equivalents before returning the connection diagnostic to the UI.
For example, when we see that RBAC_PRINCIPAL trace is returned with status FAILED…
…we replace the message to something more fitting.
Pros
- Easy to implement.
Cons
- Makes assumptions about trace type which cannot be expressed in type system and thus are easily broken.
- Sometimes we might want to include extra information within the trace message which are hard to get hold of outside of the context where the trace was added.
- For example, when
RBAC_PRINCIPALis added, we would want to adjust the message depending on whether or not the user has the Connect My Computer role in their cert. WithinSSHConnectionTesterwe'd have to somehow peck this information out of the cert.
- For example, when
Use enum values instead of strings for trace messages
This is very similar to the approach described above, with the main difference being that we no longer assume that there's only ever going to be a one possible error per trace type.
Pros
- No assumptions made about uniqueness of trace type.
- Still straightforward to implement.
Cons
- Adds some complexity as the SSH traces, unlike other traces, now cannot be directly shown in the UI and need to be first mapped from an enum value to a user-friendly error message.
- Has the same con as the above method with regards to including extra information.
Replace messages at callsites which add traces
diff --git a/lib/srv/authhandlers.go b/lib/srv/authhandlers.go
index 4efccb5b1f..374312332a 100644
--- a/lib/srv/authhandlers.go
+++ b/lib/srv/authhandlers.go
@@ -325,8 +325,15 @@ func (h *AuthHandlers) UserKeyAuth(conn ssh.ConnMetadata, key ssh.PublicKey) (*s
// only failed attempts are logged right now
recordFailedLogin := func(err error) {
failedLoginCount.Inc()
+ _, isConnectMyComputerNode := h.c.Server.GetInfo().GetLabel(types.ConnectMyComputerNodeOwnerLabel)
message := fmt.Sprintf("Principal %q is not allowed by this certificate. Ensure your roles grants access by adding it to the 'login' property.", conn.User())
+ if isConnectMyComputerNode {
+ connectMyComputerRoleName := connectmycomputer.GetRoleNameForUser(teleportUser)
+
+ message = fmt.Sprintf("Principal %q is not allowed by this certificate. Ensure that the role %s includes %q in the 'login' property.",
+ conn.User(), connectMyComputerRoleName, conn.User())
+ }
traceType := types.ConnectionDiagnosticTrace_RBAC_PRINCIPAL
if trace.IsAccessDenied(err) {Pros
- Easy to understand.
- Conforms to the established conntest approach of just adding traces of different types with specific messages.
- The callsite has access to extra information which can be included in the message and passed along to the UI, without having to pass extra information through other means.
Cons
- We have to somehow detect whether we're dealing with a Connect My Computer node or not at the callsite.
Detecting a Connect My Computer node during a connection test
To recognize a Connect My Computer node within lib/srv/authhandlers.go, we can check if the labels of the node include types.ConnectMyComputerNodeOwnerLabel, as shown in the diff above. Each Connect My Computer node is configured with this label and its not meant to be changed by the user.
_, isConnectMyComputerNode := h.c.Server.GetInfo().GetLabel(types.ConnectMyComputerNodeOwnerLabel)The other place where the connection test happens, lib/client/conntest.SSHConnectionTester, does not have any means to detect a Connect My Computer node. We'd have to add a field to TestConnectionRequest, let's say SSHNodeKind with a special value for Connect My Computer nodes. I considered adding ResourceSubKind, but the backend doesn't have an existing notion of a Connect My Computer node subkind and it'd feel weird to have ResourceKind, which does adhere to backend resource kind, and ResourceSubKind which doesn't.
Fetching server details within SSHConnectionTester and using the same logic as in lib/srv/authhandlers.go doesn't seem to make sense. When we start an SSH connection test from the UI, we already know whether we're performing this test for an SSH node or a Connect My Computer node. So we could just pass an additional param signifying whether Connect My Computer is in play or not.
It's worth noting that we have to enable SSHConnectionTester to detect a Connect My Computer node no matter which approach to adjusting diagnostic messages we pick. SSHConnectionTester both adds messages and returns them to UI, so no matter if we decide to just map the messages to Connect My Computer equivalents or adjust them at callsites, SSHConnectionTester needs to know whether a node is a Connect My Computer node or not.
Verdict
Since SSHConnectionTester needs to be made aware of Connect My Computer anyway, I think the approach with adjusting messages at callsites is best from maintenance perspective. I'm not 100% sure about the SSHNodeKind field that I want to add to TestConnectionRequest, but at least it will be easy to change/extend/rename this field if needed.
marcoandredinis
lib/srv/authhandlers.go(see alsorecordFailedLogin) andlib/client/conntest/ssh.go.Failed to connect to the Node. Ensure teleport service is running using "systemctl status teleport".doesn't make sense as the agent is not managed by systemctl. Instead, we should ask the user to make sure that the app is launched and the agent is running.Invalid user. Please ensure the principal %q is a valid Linux login in the target node. Output from Node: %v– the setup of CMC should guarantee that the user is valid. If anything, we can ask them to retry the setup.