~emersion/soju-dev

Add optional umask parameter to log config option v1 REJECTED

Gregory Anders: 1
 Add optional umask parameter to log config option

 6 files changed, 27 insertions(+), 6 deletions(-)
> 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?
Export patchset (mbox)
How do I use this?

Copy & paste the following snippet into your terminal to import this patchset into git:

curl -s https://lists.sr.ht/~emersion/soju-dev/patches/23234/mbox | git am -3
Learn more about email & git
View this thread in the archives

[PATCH] Add optional umask parameter to log config option Export this patch

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