From 5368c180250c3fca49521ba71e14a9af1edce2eb Mon Sep 17 00:00:00 2001 From: Alvar Penning Date: Thu, 25 Jul 2024 14:29:17 +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 | 58 ++++++++++++++++++++++++++++++----- logging/journald_core_test.go | 39 +++++++++++++++++++++++ 2 files changed, 89 insertions(+), 8 deletions(-) create mode 100644 logging/journald_core_test.go diff --git a/logging/journald_core.go b/logging/journald_core.go index d1943a53..4eabdaea 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 { @@ -62,6 +59,7 @@ func (c *journaldCore) Write(ent zapcore.Entry, fields []zapcore.Field) error { } enc := zapcore.NewMapObjectEncoder() + // Ensure that all field keys are valid journald field keys. If in doubt, use journaldFieldEncode. c.addFields(enc, fields) c.addFields(enc, c.context) enc.Fields["SYSLOG_IDENTIFIER"] = c.identifier @@ -74,11 +72,55 @@ func (c *journaldCore) Write(ent zapcore.Entry, fields []zapcore.Field) error { return journald.Send(message, pri, enc.Fields) } +// addFields adds all given fields to enc with an altered key, prefixed with the journaldCore.identifier and sanitized +// via journaldFieldEncode. 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..e013545b --- /dev/null +++ b/logging/journald_core_test.go @@ -0,0 +1,39 @@ +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 number", "23", "ESC_23"}, + {"leading underscore", "_foo", "ESC__FOO"}, + {"leading invalid", " foo", "ESC__FOO"}, + {"max length", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA1234", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA1234"}, + {"too long", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA12345", "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA1234"}, + {"too long leading number", "1234567890123456789012345678901234567890123456789012345678901234", "ESC_123456789012345678901234567890123456789012345678901234567890"}, + {"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") + }) + } +}