~emersion/soju-dev

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
4 2

[PATCH] Add optional umask parameter to log config option

Details
Message ID
<20210610035634.73744-1-greg@gpanders.com>
DKIM signature
pass
Download raw message
Patch: +27 -6
Add a umask parameter (0077 by default) that specifies the umask to use
when creating new log files. This allows users to specify more
relaxed permissions which allows finer grained access control of log
files on their servers.
---
 cmd/soju/main.go |  1 +
 config/config.go | 13 +++++++++++++
 doc/soju.1.scd   |  5 +++--
 msgstore_fs.go   | 10 +++++++---
 server.go        |  2 ++
 user.go          |  2 +-
 6 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/cmd/soju/main.go b/cmd/soju/main.go
index 974881b..a58dbcc 100644
--- a/cmd/soju/main.go
+++ b/cmd/soju/main.go
@@ -86,6 +86,7 @@ func main() {
	// TODO: load from config/DB
	srv.Hostname = cfg.Hostname
	srv.LogPath = cfg.LogPath
	srv.Umask = cfg.Umask
	srv.HTTPOrigins = cfg.HTTPOrigins
	srv.AcceptProxyIPs = cfg.AcceptProxyIPs
	srv.Debug = debug
diff --git a/config/config.go b/config/config.go
index a9118a8..dc29b8d 100644
--- a/config/config.go
+++ b/config/config.go
@@ -4,6 +4,7 @@ import (
	"fmt"
	"net"
	"os"
	"strconv"

	"git.sr.ht/~emersion/go-scfg"
)
@@ -42,6 +43,7 @@ type Server struct {
	SQLDriver      string
	SQLSource      string
	LogPath        string
	Umask          os.FileMode
	HTTPOrigins    []string
	AcceptProxyIPs IPSet
}
@@ -55,6 +57,7 @@ func Defaults() *Server {
		Hostname:  hostname,
		SQLDriver: "sqlite3",
		SQLSource: "soju.db",
		Umask:     0077,
	}
}

@@ -98,6 +101,16 @@ func parse(cfg scfg.Block) (*Server, error) {
			if driver != "fs" {
				return nil, fmt.Errorf("directive %q: unknown driver %q", d.Name, driver)
			}

			if len(d.Params) > 2 {
				umask := d.Params[2]
				n, err := strconv.ParseUint(umask, 8, 32)
				if err != nil {
					return nil, err
				}

				srv.Umask = os.FileMode(n)
			}
		case "http-origin":
			srv.HTTPOrigins = d.Params
		case "accept-proxy-ip":
diff --git a/doc/soju.1.scd b/doc/soju.1.scd
index 122dca8..589eb50 100644
--- a/doc/soju.1.scd
+++ b/doc/soju.1.scd
@@ -104,9 +104,10 @@ The following directives are supported:
*db* sqlite3 <path>
	Set the SQLite database path (default: "soju.db" in the current directory).

*log* fs <path>
*log* fs <path> [umask]
	Path to the bouncer logs root directory, or empty to disable logging. By
	default, logging is disabled.
	default, logging is disabled. Optionally specify a umask used for new
	log files (defaults to 0077).

*http-origin* <patterns...>
	List of allowed HTTP origins for WebSocket listeners. The parameters are
diff --git a/msgstore_fs.go b/msgstore_fs.go
index d17859d..91c7517 100644
--- a/msgstore_fs.go
+++ b/msgstore_fs.go
@@ -62,15 +62,18 @@ type fsMessageStore struct {
	root string

	files map[string]*os.File // indexed by entity

	umask os.FileMode
}

var _ messageStore = (*fsMessageStore)(nil)
var _ chatHistoryMessageStore = (*fsMessageStore)(nil)

func newFSMessageStore(root, username string) *fsMessageStore {
func newFSMessageStore(root, username string, umask os.FileMode) *fsMessageStore {
	return &fsMessageStore{
		root:  filepath.Join(root, escapeFilename.Replace(username)),
		files: make(map[string]*os.File),
		umask: umask,
	}
}

@@ -129,12 +132,13 @@ func (ms *fsMessageStore) Append(network *network, entity string, msg *irc.Messa
		}

		dir := filepath.Dir(path)
		if err := os.MkdirAll(dir, 0700); err != nil {
		perm := os.ModePerm & ^ms.umask
		if err := os.MkdirAll(dir, perm); err != nil {
			return "", fmt.Errorf("failed to create message logs directory %q: %v", dir, err)
		}

		var err error
		f, err = os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0600)
		f, err = os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_APPEND, perm&0666)
		if err != nil {
			return "", fmt.Errorf("failed to open message log file %q: %v", path, err)
		}
diff --git a/server.go b/server.go
index be91152..497e17b 100644
--- a/server.go
+++ b/server.go
@@ -6,6 +6,7 @@ import (
	"mime"
	"net"
	"net/http"
	"os"
	"strings"
	"sync"
	"sync/atomic"
@@ -51,6 +52,7 @@ type Server struct {
	Logger         Logger
	HistoryLimit   int
	LogPath        string
	Umask          os.FileMode
	Debug          bool
	HTTPOrigins    []string
	AcceptProxyIPs config.IPSet
diff --git a/user.go b/user.go
index adb315d..a2d6507 100644
--- a/user.go
+++ b/user.go
@@ -410,7 +410,7 @@ func newUser(srv *Server, record *User) *user {

	var msgStore messageStore
	if srv.LogPath != "" {
		msgStore = newFSMessageStore(srv.LogPath, record.Username)
		msgStore = newFSMessageStore(srv.LogPath, record.Username, srv.Umask)
	} else {
		msgStore = newMemoryMessageStore()
	}
-- 
2.32.0
Details
Message ID
<EFC7049C-5D01-4F5B-AD6E-C9B3729DF6A9@gpanders.com>
In-Reply-To
<20210610035634.73744-1-greg@gpanders.com> (view parent)
DKIM signature
pass
Download raw message
Now that I think about it I wonder if the best way to handle this is to just delegate to the *actual* umask set by the user/OS instead of re-implementing the functionality in soju. In order to do that, the patch would be much simpler: we’d simply change the mode argument of the MkdirAll call to 0777 and the mode argument of the OpenFile call to 0666.

Of course, for anyone who has a standard umask of 0022 set, this would suddenly make log files world readable, which is undesirable. But I do think having *some* way to handle the permissions on the log files is a useful feature (it’d be useful to me, at least).

I’m curious to hear your thoughts.

Greg
Details
Message ID
<nAIUyCsKcATUa0wWhON6OYkFYsdnkvt7Fnh8lZKuXXLNPXGIQnLtZOiKxCp_UAeAkXFaT2K7RoMO-kwf7dFaxAJjgRkRUhfCBcmlAoEfT-8=@emersion.fr>
In-Reply-To
<EFC7049C-5D01-4F5B-AD6E-C9B3729DF6A9@gpanders.com> (view parent)
DKIM signature
pass
Download raw message
On Thursday, June 10th, 2021 at 06:51, Gregory Anders <greg@gpanders.com> wrote:

> Now that I think about it I wonder if the best way to handle this is
> to just delegate to the actual umask set by the user/OS instead of
> re-implementing the functionality in soju.

I agree. I'd prefer to avoid adding a new knob for this.

> In order to do that, the patch would be much simpler: we’d simply
> change the mode argument of the MkdirAll call to 0777 and the mode
> argument of the OpenFile call to 0666.

Hm. 0777 sounds like it could be dangerous, but it also sounds like
it's a common practice to just use 0777 and rely on umask to do the
right thing. For instance the Linux man page for umask(2) explains that
0666 is "the usual case" for open(2).

> Of course, for anyone who has a standard umask of 0022 set, this
> would suddenly make log files world readable, which is undesirable.

Yeah. Log files can be a little sensitive, since they contain private
communication.

Can you tell a little bit more about your use-case? Why do you need to
access the log files as a different user?
Details
Message ID
<YMTU/YSDmXd+mP5H@gpanders.com>
In-Reply-To
<nAIUyCsKcATUa0wWhON6OYkFYsdnkvt7Fnh8lZKuXXLNPXGIQnLtZOiKxCp_UAeAkXFaT2K7RoMO-kwf7dFaxAJjgRkRUhfCBcmlAoEfT-8=@emersion.fr> (view parent)
DKIM signature
pass
Download raw message
On Sat, 12 Jun 2021 12:21 +0000, Simon Ser wrote:
>Yeah. Log files can be a little sensitive, since they contain private
>communication.
>
>Can you tell a little bit more about your use-case? Why do you need to
>access the log files as a different user?

Sure. I run soju on a single-user server using a dedicated user and 
group (soju:soju). I've added my user to the 'soju' group and chmod'ed 
the soju log directory to g+rX to make browsing and reading log files a 
little easier (i.e. without using sudo).

However, new log files are written with 0600 so even though they are 
owned by the 'soju' group I still can't read them without sudo.

I don't know if my use case is common or not. I can always do something 
ad hoc like run a cron job to update the permissions on the log dir once 
per hour or something. But for users like myself who run soju on a 
dedicated single-user server there is no benefit to using 0600, so the 
ability to relax that restriction (even to 0750/0640) would be useful.

Greg
Details
Message ID
<Ews3sKzU1_PHupgiUkVPQqtWcOwxKlsiZkQkn_Byz3J0A082YoHvKSokmpgn4WI_YwpRHD77mCs6jEvOsZjbqrzmuXTYPNSPOxjrsZlhz6c=@emersion.fr>
In-Reply-To
<YMTU/YSDmXd+mP5H@gpanders.com> (view parent)
DKIM signature
pass
Download raw message
On Saturday, June 12th, 2021 at 17:38, Gregory Anders <greg@gpanders.com> wrote:

> Sure. I run soju on a single-user server using a dedicated user and
> group (soju:soju). I've added my user to the 'soju' group and chmod'ed
> the soju log directory to g+rX to make browsing and reading log files a
> little easier (i.e. without using sudo).
>
> However, new log files are written with 0600 so even though they are
> owned by the 'soju' group I still can't read them without sudo.
>
> I don't know if my use case is common or not. I can always do something
> ad hoc like run a cron job to update the permissions on the log dir once
> per hour or something. But for users like myself who run soju on a
> dedicated single-user server there is no benefit to using 0600, so the
> ability to relax that restriction (even to 0750/0640) would be useful.

Thanks for the explanation, this makes sense to me. Maybe we can just
change the mode to 0750/0640 for now, since that would unblock your
use-case without sacrificing security in case a bad umask is set? We
can always discuss this again if someone comes in with a new use-case.
Reply to thread Export thread (mbox)