Mutual TLS Auth server and clients #1528

klizhentas · 2017-12-22T03:05:36Z

Description

This PR replaces SSH based Auth server authentication with TLS based mutual authentication. This PR is a first in a series of follow ups but is self-sufficient.

Included in the PR:

2.5.0 Server will serve SSH and TLS on the same auth server socket serving legacy SSH client requests and TLS requests
Trusted clusters will upgrade and re-exchange certificates after upgrade
Nodes will re-register and receive new identities after auth server upgrade
Auth server switched to use mutual TLS client authentication
Added support for proxy line protocol. The protocol support is turned on by default, but can be turned off later.
No reconfiguration required, upgrade is fully automatic.

Not included in the PR

Some edge cases with additional tests
User/Admin upgrade and install documentation

klizhentas · 2017-12-26T19:38:50Z

lib/auth/middleware.go

+	// per connections? E.g. what happens to session tickets. Benchmark this.
+	pool, err := t.AuthServer.ClientCertPool()
+	if err != nil {
+		log.Errorf("failed to retrieve client pool: %v", trace.DebugReport(err))


make error message conformant

klizhentas · 2017-12-26T19:42:54Z

lib/auth/middleware.go

+	}
+	clientCert := peers[0]
+	if len(clientCert.Issuer.Organization) < 1 {
+		log.Warning("missing organization in issuer certificate %v", clientCert.Issuer)


log.Warningf("Missing")...

remove this extra check

klizhentas · 2017-12-26T19:57:53Z

lib/auth/clt.go

@@ -102,6 +178,24 @@ func NewClient(addr string, dialer Dialer, params ...roundtrip.ClientParam) (*Cl
 	}, nil
 }

+// GetDialer returns dialer that will connect to auth server API


Add DELETE IN note

klizhentas · 2017-12-26T20:01:09Z

lib/auth/methods.go

+	if a.Username == "" {
+		return trace.BadParameter("missing parameter 'username'")
+	}
+	if a.Pass == nil && a.U2F == nil && a.OTP == nil {


add web session here as well

klizhentas · 2017-12-26T20:06:17Z

lib/auth/methods.go

+		return nil, trace.Wrap(err)
+	}
+
+	if err := req.CheckAndSetDefaults(); err != nil {


remove second check

klizhentas · 2017-12-26T20:53:40Z

lib/web/sessions.go

+// tryRemoteTLSClient tries creating TLS client and using it (the client may not be available
+// due to older clusters), returns client if it is working properly
+func (c *SessionContext) tryRemoteTLSClient(cluster reversetunnel.RemoteSite) (auth.ClientI, error) {
+	clt, err := c.newRemoteTLSClient(cluster)


test this case

klizhentas · 2017-12-26T20:54:04Z

lib/web/sessions.go

 		// build a ssh connection to the remote auth server
 		config := &ssh.ClientConfig{
-			User:    principal,
-			Auth:    []ssh.AuthMethod{authMethods},
+			User:    cert.ValidPrincipals[0],


check at least one

klizhentas · 2017-12-26T20:55:15Z

lib/web/sessions.go

-		return nil, trace.Wrap(err)
-	}
-	return session, nil
+	return s.proxyClient.AuthenticateWebUser(auth.AuthenticateUserRequest{


add deprecation note to SignIn

klizhentas · 2017-12-26T21:01:21Z

lib/auth/register.go

+		if err != nil {
+			return trace.Wrap(err, "failed to parse certificate at %v", certPath)
+		}
+		log.Infof("Securely joining remote cluster %v.", cert.Subject.CommonName)


remove Securely

klizhentas · 2017-12-26T21:07:49Z

lib/client/client.go

@@ -262,6 +275,56 @@ func (proxy *ProxyClient) isRecordingProxy() (bool, error) {
 	}
 }

+// dialAuthServer returns auth server connection forwarded via proxy
+func (proxy *ProxyClient) dialAuthServer(ctx context.Context, clusterName string) (net.Conn, error) {
+	log.Infof("[CLIENT] client=%v connecting to auth server on cluster ", proxy.clientAddr, clusterName)


remove square brackets

r0mant

Mostly typos and inconsistent log entries.

r0mant · 2017-12-27T01:37:51Z

lib/auth/apiserver.go

@@ -877,6 +923,26 @@ func (s *APIServer) registerNewAuthServer(auth ClientI, w http.ResponseWriter, r
 	return message("ok"), nil
 }

+type generateServerKeysReq struct {
+	HostID   string         `json:"host_id"`


Godocs on exported fields are missing.

r0mant · 2017-12-27T01:39:36Z

lib/auth/apiserver.go

@@ -1119,7 +1185,7 @@ func (s *APIServer) createUserWithToken(auth ClientI, w http.ResponseWriter, r *
 		webSession, err = auth.CreateUserWithOTP(req.Token, req.Password, req.OTPToken)
 	}
 	if err != nil {
-		log.Error(trace.DebugReport(err))
+		log.Warningf("failed to create user: %v", err.Error())


Do you need to log this? If yes, make it conformant to our style.

r0mant · 2017-12-27T01:40:04Z

lib/auth/auth.go

-// GenerateUserCert generates user certificate, it takes pkey as a signing
+// certs is a pair of SSH and TLS certificates
+type certs struct {
+	// sshis PEM encoded SSH certificate


"sshis" -> "ssh is"

r0mant · 2017-12-27T01:41:24Z

lib/auth/auth.go

+	return certs.ssh, certs.tls, nil
+}
+
+// generateUserCert generates user certificate, it takes pkey as a signing


Obsolete comment, as the method does not accept pkey.

r0mant · 2017-12-27T01:45:53Z

lib/auth/auth.go

@@ -579,7 +724,7 @@ func (s *AuthServer) checkTokenTTL(token string) bool {
 // If a token was generated with a TTL=0, it means it's a single-use token and it gets destroyed
 // after a successful registration.
 func (s *AuthServer) RegisterUsingToken(token, hostID string, nodeName string, role teleport.Role) (*PackedKeys, error) {
-	log.Infof("[AUTH] Node %q [%v] trying to join with role: %v", nodeName, hostID, role)
+	log.Infof("node %q [%v] trying to join with role: %v", nodeName, hostID, role)


"Node", dot at the end.

r0mant · 2017-12-27T02:37:33Z

lib/service/service.go

+		storage := utils.NewFileAddrStorage(
+			filepath.Join(process.Config.DataDir, "authservers.json"))
+
+		authUser := identity.Cert.ValidPrincipals[0]


Is the ValidPrincipals length checked anywhere?

yes auth/init.go:696

r0mant · 2017-12-27T02:40:26Z

lib/services/local/presence.go

 	err := s.DeleteBucket([]string{}, tunnelConnectionsPrefix)
+	log.Debugf("Presence delete all tunnel connections: %v", err)


Log entries in this function look like debug leftovers.

r0mant · 2017-12-27T02:40:35Z

lib/services/local/presence.go

@@ -473,10 +473,15 @@ func (s *PresenceService) DeleteTunnelConnections(clusterName string) error {

 // DeleteAllTunnelConnections deletes all tunnel connections
 func (s *PresenceService) DeleteAllTunnelConnections() error {
+	conns, _ := s.GetAllTunnelConnections()


Error is ignored.

r0mant · 2017-12-27T02:40:44Z

lib/services/local/presence.go

 	if trace.IsNotFound(err) {
 		return nil
 	}
+	conns, _ = s.GetAllTunnelConnections()


Error is ignored. This actually, too, looks like a debug leftover. Getting tunnels in "delete" method?

r0mant · 2017-12-27T02:44:33Z

lib/state/cachingaccesspoint.go

+	if err := cs.trust.UpsertCertAuthority(ca); err != nil {
+		return nil, trace.Wrap(err)
+	}
+	return ca, err


return ca, nil?

klizhentas · 2017-12-27T19:38:19Z

closing it in favor of another branch with merged conflicts

klizhentas added this to the 2.5.0 milestone Dec 22, 2017

klizhentas requested review from r0mant, kontsevoy and russjones December 22, 2017 03:05

klizhentas force-pushed the sasha/x509 branch 2 times, most recently from b573946 to 456f166 Compare December 22, 2017 22:42

Mutual TLS Auth server and clients

4926289

klizhentas force-pushed the sasha/x509 branch from 456f166 to 4926289 Compare December 22, 2017 23:01

klizhentas commented Dec 26, 2017

View reviewed changes

address some code review comments

698e6da

r0mant approved these changes Dec 27, 2017

View reviewed changes

klizhentas added 2 commits December 27, 2017 10:45

address code review comments

d69e43b

fix error message

7849c28

russjones approved these changes Dec 27, 2017

View reviewed changes

fix

93564fe

klizhentas closed this Dec 27, 2017

klizhentas deleted the sasha/x509 branch December 27, 2017 22:05

klizhentas mentioned this pull request Feb 19, 2018

Changelog #1703

Closed

hatched pushed a commit that referenced this pull request Jan 30, 2023

Update e ref (#1528)

fddaffe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutual TLS Auth server and clients #1528

Mutual TLS Auth server and clients #1528

klizhentas commented Dec 22, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

klizhentas Dec 26, 2017

r0mant left a comment

r0mant Dec 27, 2017

r0mant Dec 27, 2017

r0mant Dec 27, 2017

r0mant Dec 27, 2017

r0mant Dec 27, 2017

r0mant Dec 27, 2017

klizhentas Dec 27, 2017

r0mant Dec 27, 2017

r0mant Dec 27, 2017

r0mant Dec 27, 2017

r0mant Dec 27, 2017

klizhentas commented Dec 27, 2017

		err := s.DeleteBucket([]string{}, tunnelConnectionsPrefix)
		log.Debugf("Presence delete all tunnel connections: %v", err)

Mutual TLS Auth server and clients #1528

Mutual TLS Auth server and clients #1528

Conversation

klizhentas commented Dec 22, 2017

Description

Included in the PR:

Not included in the PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klizhentas commented Dec 27, 2017