Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logging: Sanitize Journald Field Key Names #48

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Jul 24, 2024

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

@oxzi oxzi requested review from julianbrost and lippserd July 24, 2024 15:44
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Jul 24, 2024
@oxzi oxzi added the bug Something isn't working label Jul 25, 2024
@oxzi oxzi marked this pull request as draft July 25, 2024 08:23
@oxzi oxzi force-pushed the fix-syslog-identifier-for-journald branch from 0e88947 to 18bd900 Compare July 25, 2024 10:31
@oxzi oxzi changed the title logging: Verify Journald Identifier logging: Sanitize Journald Field Key Names Jul 25, 2024
@oxzi
Copy link
Member Author

oxzi commented Jul 25, 2024

Based on @julianbrost's comment Icinga/icinga-notifications#254 (comment) at the other PR, I have taken another look. As it turns out, previously I blamed the wrong part, however, I was very self-confident doing so.

After some further investigation, which I also commented over there, Icinga/icinga-notifications#254 (comment), I came to the following conclusion:

Short update: it's not the SYSLOG_IDENTIFIER, but using logger's the name, which is then used to set the SYSLOG_IDENTIFIER as prefix for each journald field.

Unfortunately, the field key is not really specified. For starters, the Native Journal Protocol documentation only defines those keys as "environment-like". The systemd.journal-fields man page shows a multitude of potential keys, but has no specification either. After some digging, I found it in the source, …/libsystemd/sd-journal/journal-file.c.

In a nutshell:

  • Key length MUST be within (0, 64] characters.
  • Key MUST NOT start with _, unless it is an protected variable.
  • Key MUST NOT start with a digit.
  • Key MUST only contain [A-Z0-9_].

Thus, i have changed the logic here and introduced a function to ensure that only valid field keys are being set. Please refer to the updated commit message.

Using this commit as the icinga-go-library version in icinga-notification seems to work as intended. Take a look at the following (redacted) journalctl -o json output:

{
  "PRIORITY": "4",
  "_AUDIT_SESSION": "2",
  "_GID": "100",
  "_UID": "1000",
  "_SYSTEMD_USER_SLICE": "-.slice",
  "_SYSTEMD_OWNER_UID": "1000",
  "ICINGA_NOTIFICATIONS_ERROR": "dial tcp [::1]:5432: connect: connection refused",
  "_COMM": "icinga-notifica",
  "_SYSTEMD_CGROUP": "/user.slice/user-1000.slice/[email protected]/tmux-spawn-be8f4c8f-da78-4934-af4e-f9de076f568f.scope",
  "_SELINUX_CONTEXT": "kernel",
  "SYSLOG_IDENTIFIER": "icinga-notifications",
  "_SOURCE_REALTIME_TIMESTAMP": "1721903588451251",
  "_CMDLINE": "./icinga-notifications --config config.yml",
  "_RUNTIME_SCOPE": "system",
  "_AUDIT_LOGINUID": "1000",
  "MESSAGE": "database: Can't connect to database. Retrying",
  "_TRANSPORT": "journal",
  "_SYSTEMD_UNIT": "[email protected]",
  "_PID": "50939",
  "_CAP_EFFECTIVE": "0",
  "_SYSTEMD_SLICE": "user-1000.slice",
  "_EXE": "__/git/github.com/Icinga/icinga-notifications/icinga-notifications"
}

logging/journald_core.go Outdated Show resolved Hide resolved
logging/journald_core_test.go Outdated Show resolved Hide resolved
@oxzi oxzi force-pushed the fix-syslog-identifier-for-journald branch from 18bd900 to a7d3972 Compare July 25, 2024 12:11
@oxzi oxzi requested a review from julianbrost July 25, 2024 12:11
@oxzi oxzi force-pushed the fix-syslog-identifier-for-journald branch from a7d3972 to 5368c18 Compare July 25, 2024 12:29
Comment on lines 103 to 115
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])) {
Copy link
Member

@lippserd lippserd Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, Go already provides utilities for this:

Suggested change
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])) {
key := strcase.ScreamingSnake(key)
for i, r := range key {
if unicode.IsUpper(r) || unicode.IsDigit(r) || r == '_' {
continue
}
key[i] = '_'
}
if !unicode.IsUpper(rune(key[0])) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

journald requires ASCII A-Z, whereas (as the package name already suggests), unicode.IsUpper() also returns true for non-ASCII uppercase characters like Ä: https://go.dev/play/p/ayqkoWoCL8E

isUpper would still be a better name than isAlpha though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting key[i] = '_' in a string is not supported. To do so, it needs to be converted to a []rune or some shenanigans like key[:i] + "_" + key[i+1:] are necessary. However, the second option messes up some Unicode characters being bigger than a byte. For example: https://go.dev/play/p/qo3nfS83ybc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

journald requires ASCII A-Z, whereas (as the package name already suggests), unicode.IsUpper() also returns true for non-ASCII uppercase characters like Ä

My bad, I didn't expect UTF-8 strings to be supported and overlooked the test for it.

logging/journald_core.go Outdated Show resolved Hide resolved
@oxzi oxzi force-pushed the fix-syslog-identifier-for-journald branch from 5368c18 to 1cc15bb Compare July 26, 2024 07:03
@oxzi oxzi requested review from lippserd and julianbrost July 26, 2024 07:03
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]: Icinga/icinga-notifications#254
[1]: https://github.com/systemd/systemd/blob/11d5e2b5fbf9f6bfa5763fd45b56829ad4f0777f/src/libsystemd/sd-journal/journal-file.c#L1703
@oxzi oxzi force-pushed the fix-syslog-identifier-for-journald branch from 1cc15bb to 3f3ec3a Compare July 26, 2024 07:10
@julianbrost julianbrost merged commit ee07796 into main Jul 29, 2024
2 checks passed
@julianbrost julianbrost deleted the fix-syslog-identifier-for-journald branch July 29, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants