Session access controls #1224

russjones · 2017-08-25T20:19:28Z

Purpose

As covered in, #1223, this PR addresses two issues. The first is the rule names that gate access to various resources. The second is getting a client to a remote auth server with the logged in users role.

Implementation

For the first issue, the following three rules gate access to sessions.

services.KindSSHSession this controls active sessions. This should be hidden from user roles because this is only used by proxies and nodes.
services.KindSession this controls recorded sessions. This should be exposed to users and used to gate access to endpoints from the audit log where we fetch session playback related events.
services.KindEvent which is used to access the full audit log and emit events to the audit log.

In addition a new endpoint /events/sesssion and function SearchSessionEvents has been created that can only return events related to session start and end which is used to reconstruct the list of completed sessions.

For the second issue, the Dial function for a reversetunnel.RemoteSite has been updated to accept a special string @remote-auth-server which returns a TCP connection to the remote auth server. We then build the authMethods for the logged in user and establish a SSH connection and from there an client to the auth server. This way we get a client with the users role instead of the role of the proxy.

Related Issues

Fixes #1223

klizhentas · 2017-08-25T20:59:37Z

lib/auth/apiserver.go

@@ -181,6 +181,7 @@ func NewAPIServer(config *APIConfig) http.Handler {
 	// Audit logs AKA events
 	srv.POST("/:version/events", srv.withAuth(srv.emitAuditEvent))
 	srv.GET("/:version/events", srv.withAuth(srv.searchEvents))
+	srv.GET("/:version/events/session", srv.withAuth(srv.searchSessionEvents)) // return session related events


comment is redundant

klizhentas · 2017-08-25T21:00:16Z

lib/auth/clt.go

@@ -1227,6 +1227,26 @@ func (c *Client) SearchEvents(from, to time.Time, query string) ([]events.EventF
 	return retval, nil
 }

+// SearchSessionEvents returns session relayed events to find completed sessions.


session-related

klizhentas · 2017-08-25T21:01:44Z

lib/events/api.go

@@ -141,6 +141,10 @@ type IAuditLog interface {
 	// The only mandatory requirement is a date range (UTC). Results must always
 	// show up sorted by date (newest first)
 	SearchEvents(fromUTC, toUTC time.Time, query string) ([]EventFields, error)


why not simply extend query to add query type for session type events instead of adding extra method?

klizhentas · 2017-08-25T21:02:31Z

lib/reversetunnel/agent.go

+		servers = append(servers, server)
+	}
+
+	log.Infof("got out of band request %v", servers)


klizhentas · 2017-08-25T21:02:48Z

lib/reversetunnel/agent.go

+			break
+		}
+
+		// log the error if we were not able to connect
 		log.Error(trace.DebugReport(err))


this is not an error, connection could fail for various reasons

klizhentas · 2017-08-25T21:03:09Z

lib/reversetunnel/agent.go

+
+	log.Infof("got out of band request %v", servers)
+
+	var connected bool


connected flag seems redundant, because conn != nil should do the same

klizhentas · 2017-08-25T21:04:36Z

lib/web/apiserver.go

 		return fn(w, r, p, ctx, site)
 	})
 }

+// clientWithUserRole will return an auth.ClientI with the role of the user at


this should be a method of SessionContext

klizhentas · 2017-08-25T21:04:50Z

lib/web/apiserver.go

+}
+
+// remoteClient returns a client to a remote site with the role of logged in user.
+func remoteClient(ctx *SessionContext, site reversetunnel.RemoteSite) (auth.ClientI, error) {


klizhentas · 2017-08-25T21:05:35Z

lib/web/apiserver.go

+// the requested site. If the site is local a client with the users local role
+// is returned. If the site is remote a client with the users remote role is
+// returned.
+func clientWithUserRole(ctx *SessionContext, site reversetunnel.RemoteSite) (auth.ClientI, error) {


clientWithUserRole -> ctx.GetUserClient()

klizhentas · 2017-08-25T21:06:46Z

lib/web/apiserver.go

+		return client.Dial(network, addr)
+	}
+
+	clt, err := auth.NewClient("http://stub:0", sshDialer)


you will be creating clients on every request. This client should be associated with active user session and should be deleted when the session closes. Extend the logic of the SessionContext to support additional use case.

klizhentas · 2017-08-25T21:07:21Z

lib/web/apiserver.go

+		// remote cluster from our local cluster
+		agent, err := ctx.GetAgent()
+		if err != nil {
+			return nil, trace.Wrap(err)


connection above should be closed if you failed to get agent here, otherwise it will be orphaned resource.

klizhentas · 2017-08-25T21:07:26Z

lib/web/apiserver.go

+		}
+		principal, authMethods, err := getUserCredentials(agent)
+		if err != nil {
+			return nil, trace.Wrap(err)


klizhentas · 2017-08-25T21:07:34Z

lib/web/apiserver.go

+		}
+		sshConn, sshChans, sshReqs, err := ssh.NewClientConn(netConn, reversetunnel.RemoteAuthServer, config)
+		if err != nil {
+			return nil, trace.Wrap(err)


klizhentas

There are several issues to be fixed with this PR especially around resource handling.

klizhentas · 2017-08-26T00:17:05Z

lib/web/sessions.go

+	}
+
+	// look to see if we already have a connection to this cluster
+	remoteClt, ok := c.remoteClt[site.GetName()]


this could be accessed concurrently, because the session can be used in multiple requests at the same time.

you should protect the access to the client with read write mutex, I suggest to use separate small function with defer for that.

klizhentas · 2017-08-26T00:17:52Z

lib/web/sessions.go

+		// we'll save the remote client in our session context so we don't have to
+		// build a new connection next time. all remote clients will be closed when
+		// the session context is closed.
+		c.remoteClt[site.GetName()] = rClt


I think you should call AddCloser here to close client if it supports closing

klizhentas · 2017-08-26T00:19:07Z

lib/web/sessions.go

+		return nil, trace.Wrap(err)
+	}
+
+	return clt, nil


probably clt does not support closing directly, but underlying netConn does, so you can return it here with clt explicitly and call AddCloser to close the connection when session ends

klizhentas · 2017-08-26T00:19:40Z

lib/web/sessions.go

@@ -158,6 +265,12 @@ func (c *SessionContext) Close() error {
 	if c.clt != nil {
 		return trace.Wrap(c.clt.Close())
 	}
+	for _, remoteClt := range c.remoteClt {
+		err := remoteClt.Close()


ah, it does, actually, but does it close underlying netConn?

russjones requested review from klizhentas and kontsevoy August 25, 2017 20:19

russjones changed the title ~~Session access controls.~~ Session access controls Aug 25, 2017

russjones force-pushed the rjones/rule-fix branch 2 times, most recently from d97655a to 7e347af Compare August 25, 2017 20:45

klizhentas reviewed Aug 25, 2017

View reviewed changes

klizhentas requested changes Aug 25, 2017

View reviewed changes

russjones force-pushed the rjones/rule-fix branch from 6181402 to c580f7d Compare August 26, 2017 00:14

klizhentas reviewed Aug 26, 2017

View reviewed changes

klizhentas approved these changes Aug 26, 2017

View reviewed changes

Session access controls.

444d62e

russjones force-pushed the rjones/rule-fix branch from 4afef95 to 444d62e Compare August 26, 2017 00:55

russjones merged commit 52117ab into master Aug 26, 2017

russjones deleted the rjones/rule-fix branch August 26, 2017 01:04

kimlisa mentioned this pull request Feb 18, 2022

Add missing action VerbRead to ListResources #10422

Merged

hugoShaka mentioned this pull request Sep 2, 2022

helm: Add distributed tracing support in both Helm charts #16095

Open

hatched pushed a commit that referenced this pull request Feb 1, 2023

Elasticsearch audit events. (#1213) (#1224)

45dbaa8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Session access controls #1224

Session access controls #1224

russjones commented Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017 •

edited

Loading

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas Aug 25, 2017

klizhentas left a comment

klizhentas Aug 26, 2017

klizhentas Aug 26, 2017

klizhentas Aug 26, 2017

klizhentas Aug 26, 2017

klizhentas Aug 26, 2017

klizhentas Aug 26, 2017


		log.Infof("got out of band request %v", servers)

		var connected bool

Session access controls #1224

Session access controls #1224

Conversation

russjones commented Aug 25, 2017

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 Aug 25, 2017 • edited Loading

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 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

klizhentas Aug 25, 2017 •

edited

Loading