From 18bd900ed61eef61d26faa8ee125eace8f853111 Mon Sep 17 00:00:00 2001
From: Alvar Penning <alvar.penning@icinga.com>
Date: Thu, 25 Jul 2024 12:26:08 +0200
Subject: [PATCH] logging: Sanitize Journald Field Key Names

As seen over at Icinga Notifications[0], the use of non-alphanumeric
characters for journaldCore causes fields to be silently discarded.

This was due to journald's field key validation, which is unfortunately
not very well documented. Looking at its implementation[1], this library
code could be adapted to ensure that valid field keys are always used.

[0]: https://github.com/Icinga/icinga-notifications/pull/254
[1]: https://github.com/systemd/systemd/blob/11d5e2b5fbf9f6bfa5763fd45b56829ad4f0777f/src/libsystemd/sd-journal/journal-file.c#L1703
---
 logging/journald_core.go      | 57 +++++++++++++++++++++++++++++------
 logging/journald_core_test.go | 38 +++++++++++++++++++++++
 2 files changed, 86 insertions(+), 9 deletions(-)
 create mode 100644 logging/journald_core_test.go

diff --git a/logging/journald_core.go b/logging/journald_core.go
index d1943a53..13324086 100644
--- a/logging/journald_core.go
+++ b/logging/journald_core.go
@@ -5,7 +5,6 @@ import (
 	"github.com/pkg/errors"
 	"github.com/ssgreg/journald"
 	"go.uber.org/zap/zapcore"
-	"strings"
 )
 
 // priorities maps zapcore.Level to journal.Priority.
@@ -25,15 +24,13 @@ func NewJournaldCore(identifier string, enab zapcore.LevelEnabler) zapcore.Core
 	return &journaldCore{
 		LevelEnabler: enab,
 		identifier:   identifier,
-		identifierU:  strings.ToUpper(identifier),
 	}
 }
 
 type journaldCore struct {
 	zapcore.LevelEnabler
-	context     []zapcore.Field
-	identifier  string
-	identifierU string
+	context    []zapcore.Field
+	identifier string
 }
 
 func (c *journaldCore) Check(ent zapcore.Entry, ce *zapcore.CheckedEntry) *zapcore.CheckedEntry {
@@ -64,7 +61,7 @@ func (c *journaldCore) Write(ent zapcore.Entry, fields []zapcore.Field) error {
 	enc := zapcore.NewMapObjectEncoder()
 	c.addFields(enc, fields)
 	c.addFields(enc, c.context)
-	enc.Fields["SYSLOG_IDENTIFIER"] = c.identifier
+	enc.Fields[journaldFieldEncode("SYSLOG_IDENTIFIER")] = c.identifier
 
 	message := ent.Message
 	if ent.LoggerName != c.identifier {
@@ -76,9 +73,51 @@ func (c *journaldCore) Write(ent zapcore.Entry, fields []zapcore.Field) error {
 
 func (c *journaldCore) addFields(enc zapcore.ObjectEncoder, fields []zapcore.Field) {
 	for _, field := range fields {
-		field.Key = c.identifierU +
-			"_" +
-			strcase.ScreamingSnake(field.Key)
+		field.Key = journaldFieldEncode(c.identifier + "_" + field.Key)
 		field.AddTo(enc)
 	}
 }
+
+// journaldFieldEncode alters an input string to be used as a journald field key.
+//
+// When journald receives a field with an invalid key, it silently discards this field. This makes syntactically correct
+// keys a necessity. Unfortunately, there was no specific documentation about the field key syntax available. This
+// function follows the logic enforced in systemd's journal_field_valid function[0].
+//
+// This boils down to:
+// - Key length MUST be within (0, 64] characters.
+// - Key MUST start with [A-Z].
+// - Key characters MUST be [A-Z0-9_].
+//
+//	[0]: https://github.com/systemd/systemd/blob/11d5e2b5fbf9f6bfa5763fd45b56829ad4f0777f/src/libsystemd/sd-journal/journal-file.c#L1703
+func journaldFieldEncode(key string) string {
+	if len(key) == 0 {
+		// While this is definitely an error, panicking would be too destructive and silently dropping fields is against
+		// the very idea of ensuring key conformity.
+		return "EMPTY_KEY"
+	}
+
+	isAlpha := func(r rune) bool { return 'A' <= r && r <= 'Z' }
+	isDigit := func(r rune) bool { return '0' <= r && r <= '9' }
+
+	keyParts := []rune(strcase.ScreamingSnake(key))
+	for i, r := range keyParts {
+		if isAlpha(r) || isDigit(r) || r == '_' {
+			continue
+		}
+		keyParts[i] = '_'
+	}
+	key = string(keyParts)
+
+	if !isAlpha(rune(key[0])) {
+		// Escape invalid leading characters with a generic "ESC_" prefix. This was seen as a safer choice instead of
+		// iterating over the key and removing parts.
+		key = "ESC_" + key
+	}
+
+	if len(key) > 64 {
+		key = key[:64]
+	}
+
+	return key
+}
diff --git a/logging/journald_core_test.go b/logging/journald_core_test.go
new file mode 100644
index 00000000..a0c20f80
--- /dev/null
+++ b/logging/journald_core_test.go
@@ -0,0 +1,38 @@
+package logging
+
+import (
+	"github.com/stretchr/testify/require"
+	"regexp"
+	"testing"
+)
+
+func Test_journaldFieldEncode(t *testing.T) {
+	tests := []struct {
+		name   string
+		input  string
+		output string
+	}{
+		{"empty", "", "EMPTY_KEY"},
+		{"lowercase", "foo", "FOO"},
+		{"uppercase", "FOO", "FOO"},
+		{"dash", "foo-bar", "FOO_BAR"},
+		{"non ascii", "snow_☃", "SNOW__"},
+		{"leading numb", "23", "ESC_23"},
+		{"leading underscore", "_foo", "ESC__FOO"},
+		{"leading invalid", " foo", "ESC__FOO"},
+		{"max length", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"},
+		{"too long", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"},
+		{"concrete example", "icinga-notifications" + "_" + "error", "ICINGA_NOTIFICATIONS_ERROR"},
+		{"example syslog_identifier", "SYSLOG_IDENTIFIER", "SYSLOG_IDENTIFIER"},
+	}
+
+	check := regexp.MustCompile(`^[A-Z][A-Z0-9_]{0,63}$`)
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			out := journaldFieldEncode(test.input)
+			require.Equal(t, test.output, out)
+			require.True(t, check.MatchString(out), "check regular expression")
+		})
+	}
+}