chore(daemon): remove untrusted socket (#361)

As part of working on #358, we found out that the untrusted socket is not used, and so can be removed (in preparation for porting the `AccessChecker` changes from snapd in #358). **Indicators that it's not used:** If we look at how `canAccess` works, if we match on `untrustedSocketPath` (`isUntrusted`), the only way for `canAccess` to allow the request is when `c.UntrustedOK` is `true` (otherwise it unconditionally returns `accessUnauthorized` immediately): ```golang if isUntrusted { if c.UntrustedOK { return accessOK } return accessUnauthorized } ``` So in order for any API calls to be allowed with the untrusted socket (assuming all API calls go through `canAccess`), we would need to have a `Command` defined with `UntrustedOK: true`. Checking the Pebble codebase, no such `Command` definition exists, which means that even if any application would use the untrusted socket currently, all API calls would return `accessUnauthorized` unconditionally for this socket. The untrusted socket as well as `UntrustedOK` in `Command` were already part of the initial import commit (50466ba), so seem to be an inheritance from snapd that haven't seen use in Pebble since then. The corresponding [snapd sources from around November 10th, 2020](https://github.com/snapcore/snapd/tree/e2581af241a941856a755035d816047ff9aa15d8) seem to call these [`SnapOK`](https://github.com/snapcore/snapd/blob/e2581af241a941856a755035d816047ff9aa15d8/daemon/daemon.go#L139) (`UntrustedOK`), [`dirs.SnapSocket`](https://github.com/snapcore/snapd/blob/e2581af241a941856a755035d816047ff9aa15d8/daemon/daemon.go#L160C23-L160C38) (`untrustedSocketPath`) and [`snapListener`](https://github.com/snapcore/snapd/blob/489358223f0bd03da01e62a4062174eb7e9e0ffa/daemon/daemon.go#L72) (`untrustedListener`). Due to `gofmt` and removal of struct members with the longest names, this PR is best reviewed with the "hide whitespace" option.
canonical · Feb 19, 2024 · 7a139ff · 7a139ff
1 parent 37f7b02
commit 7a139ff
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 173 deletions.
diff --git a/internals/daemon/daemon.go b/internals/daemon/daemon.go
@@ -81,22 +81,20 @@ type Options struct {
 
 // A Daemon listens for requests and routes them to the right command
 type Daemon struct {
-	Version             string
-	StartTime           time.Time
-	pebbleDir           string
-	normalSocketPath    string
-	untrustedSocketPath string
-	httpAddress         string
-	overlord            *overlord.Overlord
-	state               *state.State
-	generalListener     net.Listener
-	untrustedListener   net.Listener
-	httpListener        net.Listener
-	connTracker         *connTracker
-	serve               *http.Server
-	tomb                tomb.Tomb
-	router              *mux.Router
-	standbyOpinions     *standby.StandbyOpinions
+	Version          string
+	StartTime        time.Time
+	pebbleDir        string
+	normalSocketPath string
+	httpAddress      string
+	overlord         *overlord.Overlord
+	state            *state.State
+	generalListener  net.Listener
+	httpListener     net.Listener
+	connTracker      *connTracker
+	serve            *http.Server
+	tomb             tomb.Tomb
+	router           *mux.Router
+	standbyOpinions  *standby.StandbyOpinions
 
 	// set to what kind of restart was requested (if any)
 	requestedRestart restart.RestartType
@@ -123,14 +121,13 @@ type Command struct {
 	Path       string
 	PathPrefix string
 	//
-	GET         ResponseFunc
-	PUT         ResponseFunc
-	POST        ResponseFunc
-	DELETE      ResponseFunc
-	GuestOK     bool
-	UserOK      bool
-	UntrustedOK bool
-	AdminOnly   bool
+	GET       ResponseFunc
+	PUT       ResponseFunc
+	POST      ResponseFunc
+	DELETE    ResponseFunc
+	GuestOK   bool
+	UserOK    bool
+	AdminOnly bool
 
 	d *Daemon
 }
@@ -153,9 +150,8 @@ const (
 // - GuestOK: anyone can access GET
 // - UserOK: any uid on the local system can access GET
 // - AdminOnly: only the administrator can access this
-// - UntrustedOK: can access this via the untrusted socket
 func (c *Command) canAccess(r *http.Request, user *UserState) accessResult {
-	if c.AdminOnly && (c.UserOK || c.GuestOK || c.UntrustedOK) {
+	if c.AdminOnly && (c.UserOK || c.GuestOK) {
 		logger.Panicf("internal error: command cannot have AdminOnly together with any *OK flag")
 	}
 
@@ -174,15 +170,6 @@ func (c *Command) canAccess(r *http.Request, user *UserState) accessResult {
 		return accessForbidden
 	}
 
-	isUntrusted := (ucred != nil && ucred.Socket == c.d.untrustedSocketPath)
-
-	if isUntrusted {
-		if c.UntrustedOK {
-			return accessOK
-		}
-		return accessUnauthorized
-	}
-
 	// the !AdminOnly check is redundant, but belt-and-suspenders
 	if r.Method == "GET" && !c.AdminOnly {
 		// Guest and user access restricted to GET requests
@@ -369,14 +356,6 @@ func (d *Daemon) Init() error {
 		return fmt.Errorf("when trying to listen on %s: %v", d.normalSocketPath, err)
 	}
 
-	if listener, err := getListener(d.untrustedSocketPath, listenerMap); err == nil {
-		// This listener may also be nil if that socket wasn't among
-		// the listeners, so check it before using it.
-		d.untrustedListener = &ucrednetListener{Listener: listener}
-	} else {
-		logger.Debugf("cannot get listener for %q: %v", d.untrustedSocketPath, err)
-	}
-
 	d.addRoutes()
 
 	if d.httpAddress != "" {
@@ -489,14 +468,6 @@ func (d *Daemon) Start() error {
 	d.overlord.Loop()
 
 	d.tomb.Go(func() error {
-		if d.untrustedListener != nil {
-			d.tomb.Go(func() error {
-				if err := d.serve.Serve(d.untrustedListener); err != http.ErrServerClosed && d.tomb.Err() == tomb.ErrStillAlive {
-					return err
-				}
-				return nil
-			})
-		}
 		if err := d.serve.Serve(d.generalListener); err != http.ErrServerClosed && d.tomb.Err() == tomb.ErrStillAlive {
 			return err
 		}
@@ -853,10 +824,9 @@ func (d *Daemon) SetServiceArgs(serviceArgs map[string][]string) error {
 
 func New(opts *Options) (*Daemon, error) {
 	d := &Daemon{
-		pebbleDir:           opts.Dir,
-		normalSocketPath:    opts.SocketPath,
-		untrustedSocketPath: opts.SocketPath + ".untrusted",
-		httpAddress:         opts.HTTPAddress,
+		pebbleDir:        opts.Dir,
+		normalSocketPath: opts.SocketPath,
+		httpAddress:      opts.HTTPAddress,
 	}
 
 	ovldOptions := overlord.Options{

diff --git a/internals/daemon/daemon_test.go b/internals/daemon/daemon_test.go
@@ -204,10 +204,6 @@ func (s *daemonSuite) TestExplicitPaths(c *C) {
 	info, err := os.Stat(s.socketPath)
 	c.Assert(err, IsNil)
 	c.Assert(info.Mode(), Equals, os.ModeSocket|0666)
-
-	info, err = os.Stat(s.socketPath + ".untrusted")
-	c.Assert(err, IsNil)
-	c.Assert(info.Mode(), Equals, os.ModeSocket|0666)
 }
 
 func (s *daemonSuite) TestCommandMethodDispatch(c *C) {
@@ -370,38 +366,6 @@ func (s *daemonSuite) TestGuestAccess(c *C) {
 	c.Check(cmd.canAccess(del, nil), Equals, accessUnauthorized)
 }
 
-func (s *daemonSuite) TestUntrustedAccessUntrustedOKWithUser(c *C) {
-	d := s.newDaemon(c)
-
-	remoteAddr := "pid=100;uid=1000;socket=" + d.untrustedSocketPath + ";"
-	get := &http.Request{Method: "GET", RemoteAddr: remoteAddr}
-	put := &http.Request{Method: "PUT", RemoteAddr: remoteAddr}
-	pst := &http.Request{Method: "POST", RemoteAddr: remoteAddr}
-	del := &http.Request{Method: "DELETE", RemoteAddr: remoteAddr}
-
-	cmd := &Command{d: d, UntrustedOK: true}
-	c.Check(cmd.canAccess(get, nil), Equals, accessOK)
-	c.Check(cmd.canAccess(put, nil), Equals, accessOK)
-	c.Check(cmd.canAccess(pst, nil), Equals, accessOK)
-	c.Check(cmd.canAccess(del, nil), Equals, accessOK)
-}
-
-func (s *daemonSuite) TestUntrustedAccessUntrustedOKWithRoot(c *C) {
-	d := s.newDaemon(c)
-
-	remoteAddr := "pid=100;uid=0;socket=" + d.untrustedSocketPath + ";"
-	get := &http.Request{Method: "GET", RemoteAddr: remoteAddr}
-	put := &http.Request{Method: "PUT", RemoteAddr: remoteAddr}
-	pst := &http.Request{Method: "POST", RemoteAddr: remoteAddr}
-	del := &http.Request{Method: "DELETE", RemoteAddr: remoteAddr}
-
-	cmd := &Command{d: d, UntrustedOK: true}
-	c.Check(cmd.canAccess(get, nil), Equals, accessOK)
-	c.Check(cmd.canAccess(put, nil), Equals, accessOK)
-	c.Check(cmd.canAccess(pst, nil), Equals, accessOK)
-	c.Check(cmd.canAccess(del, nil), Equals, accessOK)
-}
-
 func (s *daemonSuite) TestUserAccess(c *C) {
 	d := s.newDaemon(c)
 
@@ -423,13 +387,6 @@ func (s *daemonSuite) TestUserAccess(c *C) {
 	cmd = &Command{d: d, GuestOK: true}
 	c.Check(cmd.canAccess(get, nil), Equals, accessOK)
 	c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized)
-
-	// Since this request has a RemoteAddr, it must be coming from the pebble server
-	// socket instead of the pebble one. In that case, UntrustedOK should have no
-	// bearing on the default behavior, which is to deny access.
-	cmd = &Command{d: d, UntrustedOK: true}
-	c.Check(cmd.canAccess(get, nil), Equals, accessUnauthorized)
-	c.Check(cmd.canAccess(put, nil), Equals, accessUnauthorized)
 }
 
 func (s *daemonSuite) TestLoggedInUserAccess(c *C) {
@@ -454,10 +411,6 @@ func (s *daemonSuite) TestLoggedInUserAccess(c *C) {
 	cmd = &Command{d: d, GuestOK: true}
 	c.Check(cmd.canAccess(get, user), Equals, accessOK)
 	c.Check(cmd.canAccess(put, user), Equals, accessOK)
-
-	cmd = &Command{d: d, UntrustedOK: true}
-	c.Check(cmd.canAccess(get, user), Equals, accessOK)
-	c.Check(cmd.canAccess(put, user), Equals, accessOK)
 }
 
 func (s *daemonSuite) TestSuperAccess(c *C) {
@@ -483,10 +436,6 @@ func (s *daemonSuite) TestSuperAccess(c *C) {
 		cmd = &Command{d: d, GuestOK: true}
 		c.Check(cmd.canAccess(get, nil), Equals, accessOK)
 		c.Check(cmd.canAccess(put, nil), Equals, accessOK)
-
-		cmd = &Command{d: d, UntrustedOK: true}
-		c.Check(cmd.canAccess(get, nil), Equals, accessOK)
-		c.Check(cmd.canAccess(put, nil), Equals, accessOK)
 	}
 }
 
@@ -545,15 +494,10 @@ func (s *daemonSuite) TestStartStop(c *C) {
 
 	l1, err := net.Listen("tcp", "127.0.0.1:0")
 	c.Assert(err, IsNil)
-	l2, err := net.Listen("tcp", "127.0.0.1:0")
-	c.Assert(err, IsNil)
 
 	generalAccept := make(chan struct{})
 	d.generalListener = &witnessAcceptListener{Listener: l1, accept: generalAccept}
 
-	untrustedAccept := make(chan struct{})
-	d.untrustedListener = &witnessAcceptListener{Listener: l2, accept: untrustedAccept}
-
 	c.Assert(d.Start(), IsNil)
 
 	generalDone := make(chan struct{})
@@ -566,18 +510,7 @@ func (s *daemonSuite) TestStartStop(c *C) {
 		close(generalDone)
 	}()
 
-	untrustedDone := make(chan struct{})
-	go func() {
-		select {
-		case <-untrustedAccept:
-		case <-time.After(2 * time.Second):
-			c.Fatal("untrusted listener accept was not called")
-		}
-		close(untrustedDone)
-	}()
-
 	<-generalDone
-	<-untrustedDone
 
 	err = d.Stop(nil)
 	c.Check(err, IsNil)
@@ -592,9 +525,6 @@ func (s *daemonSuite) TestRestartWiring(c *C) {
 	generalAccept := make(chan struct{})
 	d.generalListener = &witnessAcceptListener{Listener: l, accept: generalAccept}
 
-	untrustedAccept := make(chan struct{})
-	d.untrustedListener = &witnessAcceptListener{Listener: l, accept: untrustedAccept}
-
 	c.Assert(d.Start(), IsNil)
 	defer d.Stop(nil)
 
@@ -608,18 +538,7 @@ func (s *daemonSuite) TestRestartWiring(c *C) {
 		close(generalDone)
 	}()
 
-	untrustedDone := make(chan struct{})
-	go func() {
-		select {
-		case <-untrustedAccept:
-		case <-time.After(2 * time.Second):
-			c.Fatal("untrusted accept was not called")
-		}
-		close(untrustedDone)
-	}()
-
 	<-generalDone
-	<-untrustedDone
 
 	st := d.overlord.State()
 	st.Lock()
@@ -652,16 +571,10 @@ func (s *daemonSuite) TestGracefulStop(c *C) {
 	generalL, err := net.Listen("tcp", "127.0.0.1:0")
 	c.Assert(err, IsNil)
 
-	untrustedL, err := net.Listen("tcp", "127.0.0.1:0")
-	c.Assert(err, IsNil)
-
 	generalAccept := make(chan struct{})
 	generalClosed := make(chan struct{})
 	d.generalListener = &witnessAcceptListener{Listener: generalL, accept: generalAccept, closed: generalClosed}
 
-	untrustedAccept := make(chan struct{})
-	d.untrustedListener = &witnessAcceptListener{Listener: untrustedL, accept: untrustedAccept}
-
 	c.Assert(d.Start(), IsNil)
 
 	generalAccepting := make(chan struct{})
@@ -674,18 +587,7 @@ func (s *daemonSuite) TestGracefulStop(c *C) {
 		close(generalAccepting)
 	}()
 
-	untrustedAccepting := make(chan struct{})
-	go func() {
-		select {
-		case <-untrustedAccept:
-		case <-time.After(2 * time.Second):
-			c.Fatal("general accept was not called")
-		}
-		close(untrustedAccepting)
-	}()
-
 	<-generalAccepting
-	<-untrustedAccepting
 
 	alright := make(chan struct{})
 
@@ -726,9 +628,6 @@ func (s *daemonSuite) TestRestartSystemWiring(c *C) {
 	generalAccept := make(chan struct{})
 	d.generalListener = &witnessAcceptListener{Listener: l, accept: generalAccept}
 
-	untrustedAccept := make(chan struct{})
-	d.untrustedListener = &witnessAcceptListener{Listener: l, accept: untrustedAccept}
-
 	c.Assert(d.Start(), IsNil)
 	defer d.Stop(nil)
 
@@ -744,18 +643,7 @@ func (s *daemonSuite) TestRestartSystemWiring(c *C) {
 		close(generalDone)
 	}()
 
-	untrustedDone := make(chan struct{})
-	go func() {
-		select {
-		case <-untrustedAccept:
-		case <-time.After(2 * time.Second):
-			c.Fatal("untrusted accept was not called")
-		}
-		close(untrustedDone)
-	}()
-
 	<-generalDone
-	<-untrustedDone
 
 	oldRebootNoticeWait := rebootNoticeWait
 	oldRebootWaitTimeout := rebootWaitTimeout
@@ -849,15 +737,9 @@ func makeDaemonListeners(c *C, d *Daemon) {
 	generalL, err := net.Listen("tcp", "127.0.0.1:0")
 	c.Assert(err, IsNil)
 
-	untrustedL, err := net.Listen("tcp", "127.0.0.1:0")
-	c.Assert(err, IsNil)
-
 	generalAccept := make(chan struct{})
 	generalClosed := make(chan struct{})
 	d.generalListener = &witnessAcceptListener{Listener: generalL, accept: generalAccept, closed: generalClosed}
-
-	untrustedAccept := make(chan struct{})
-	d.untrustedListener = &witnessAcceptListener{Listener: untrustedL, accept: untrustedAccept}
 }
 
 // This test tests that when a restart of the system is called