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

cmd/icinga-notifications: Fix SYSLOG_IDENTIFIER for zap fields in journald #254

Closed
wants to merge 1 commit into from

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Jul 24, 2024

One line diff with a bit of history ahead.

When logging into "systemd-journald", the journaldCore type will be used. Eventually, a log event will be written, resulting in *journaldCore.Write() to be called. There, the name, now called identifier, will be set as the "SYSLOG_IDENTIFIER" field.

This is now passed down to journald, where the "SYSLOG_IDENTIFIER" is handled as the "[s]yslog compatibility fields containing […] the identifier string (i.e. "tag")"1. After some digging, I found a specification of this tag in RFC 3164, stating that a "TAG is a string of ABNF alphanumeric characters that MUST NOT exceed 32 characters"2.

As it turns out, "icinga-notifications" does not match this specification due to the presence of "-". When removing it, suddenly journalctl shows the fields prefixed with "ICINGANOTIFICATIONS_".

However, as this looks like a typo and someone is going to "fix" it in the future, I decided to truncate the tag (a.k.a. identifier a.k.a. name) to be only "notifications". Now, an error might look like "NOTIFICATIONS_ERROR".

Footnotes

  1. https://www.freedesktop.org/software/systemd/man/latest/systemd.journal-fields.html#SYSLOG_FACILITY=

  2. https://datatracker.ietf.org/doc/html/rfc3164#section-4.1.3

…rnald

One line diff with a bit of history ahead.

When logging into "systemd-journald", the journaldCore type will be
used. Eventually, a log event will be written, resulting in
*journaldCore.Write() to be called. There, the name, now called
identifier, will be set as the "SYSLOG_IDENTIFIER" field.

This is now passed down to journald, where the "SYSLOG_IDENTIFIER" is
handled as the "[s]yslog compatibility fields containing […] the
identifier string (i.e. "tag")"[0]. After some digging, I found a
specification of this tag in RFC 3164, stating that a "TAG is a string
of ABNF alphanumeric characters that MUST NOT exceed 32 characters"[1].

As it turns out, "icinga-notifications" does not match this
specification due to the presence of "-". When removing it, suddenly
journalctl shows the fields prefixed with "ICINGANOTIFICATIONS_".

However, as this looks like a typo and someone is going to "fix" it in
the future, I decided to truncate the tag (a.k.a. identifier a.k.a.
name) to be only "notifications". Now, an error might look like
"NOTIFICATIONS_ERROR".

[0]: https://www.freedesktop.org/software/systemd/man/latest/systemd.journal-fields.html#SYSLOG_FACILITY=
[1]: https://datatracker.ietf.org/doc/html/rfc3164#section-4.1.3
@oxzi oxzi added the bug Something isn't working label Jul 24, 2024
@oxzi oxzi requested a review from julianbrost July 24, 2024 15:10
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Jul 24, 2024
oxzi added a commit to Icinga/icinga-go-library that referenced this pull request Jul 24, 2024
As seen over at Icinga Notifications[0], using non-alphanumeric
characters for the journaldCore.identifier results in the fields being
silently discarded. Now, a check enforces a correct identifier.

[0]: Icinga/icinga-notifications#254
@julianbrost
Copy link
Collaborator

Some puzzle piece is missing here, at least I don't see why SYSLOG_IDENTIFIER should be the culprit here. This is a full log message written by the v0.1.0 to journal:

{
  "_COMM" : "icinga-notifica",
  "_BOOT_ID" : "f74ea6b780ab457aa78b2588f84dccef",
  "_SYSTEMD_SLICE" : "system.slice",
  "_GID" : "113",
  "_RUNTIME_SCOPE" : "system",
  "__REALTIME_TIMESTAMP" : "1721829899105587",
  "_EXE" : "/usr/sbin/icinga-notifications",
  "_SYSTEMD_CGROUP" : "/system.slice/icinga-notifications.service",
  "_CMDLINE" : "/usr/sbin/icinga-notifications",
  "MESSAGE" : "incident: Successfully sent a notification via channel plugin",
  "_SYSTEMD_UNIT" : "icinga-notifications.service",
  "_PID" : "22933",
  "_CAP_EFFECTIVE" : "0",
  "__CURSOR" : "s=aed145c9a2e34842977fe930812a9ac8;i=f1c;b=f74ea6b780ab457aa78b2588f84dccef;m=d91b3842;t=61dfec37ff533;x=d2f77f8ce330bc66",
  "_SOURCE_REALTIME_TIMESTAMP" : "1721829899105532",
  "PRIORITY" : "6",
  "SYSLOG_IDENTIFIER" : "icinga-notifications",
  "_SYSTEMD_INVOCATION_ID" : "142cb77a33714e70ba0f66130c329b9b",
  "_TRANSPORT" : "journal",
  "_HOSTNAME" : "jb-d12",
  "_MACHINE_ID" : "c8e230a4abf54885a3be78a5a0317b59",
  "__MONOTONIC_TIMESTAMP" : "3642439746",
  "_SELINUX_CONTEXT" : "unconfined\n",
  "_UID" : "108"
}

It has "SYSLOG_IDENTIFIER" : "icinga-notifications" set, so it doesn't look like - there is a problem for journald.

@oxzi oxzi marked this pull request as draft July 25, 2024 08:23
@oxzi
Copy link
Member Author

oxzi commented Jul 25, 2024

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_].

oxzi added a commit to Icinga/icinga-go-library that referenced this pull request Jul 25, 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]: Icinga/icinga-notifications#254
[1]: https://github.com/systemd/systemd/blob/11d5e2b5fbf9f6bfa5763fd45b56829ad4f0777f/src/libsystemd/sd-journal/journal-file.c#L1703
@oxzi
Copy link
Member Author

oxzi commented Jul 25, 2024

I have made the relevant changes over at the icinga-go-library in the just updated PR Icinga/icinga-go-library#48. More context is available in this comment, Icinga/icinga-go-library#48 (comment).

Please note, for the current fix, changes are only necessary in the icinga-go-library and there is nothing to do in this repository, unless bumping the dependency.

oxzi added a commit to Icinga/icinga-go-library that referenced this pull request Jul 25, 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]: Icinga/icinga-notifications#254
[1]: https://github.com/systemd/systemd/blob/11d5e2b5fbf9f6bfa5763fd45b56829ad4f0777f/src/libsystemd/sd-journal/journal-file.c#L1703
oxzi added a commit to Icinga/icinga-go-library that referenced this pull request Jul 25, 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]: Icinga/icinga-notifications#254
[1]: https://github.com/systemd/systemd/blob/11d5e2b5fbf9f6bfa5763fd45b56829ad4f0777f/src/libsystemd/sd-journal/journal-file.c#L1703
oxzi added a commit to Icinga/icinga-go-library that referenced this pull request Jul 26, 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]: Icinga/icinga-notifications#254
[1]: https://github.com/systemd/systemd/blob/11d5e2b5fbf9f6bfa5763fd45b56829ad4f0777f/src/libsystemd/sd-journal/journal-file.c#L1703
oxzi added a commit to Icinga/icinga-go-library that referenced this pull request Jul 26, 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]: Icinga/icinga-notifications#254
[1]: https://github.com/systemd/systemd/blob/11d5e2b5fbf9f6bfa5763fd45b56829ad4f0777f/src/libsystemd/sd-journal/journal-file.c#L1703
@oxzi
Copy link
Member Author

oxzi commented Jul 29, 2024

Created #264 to keep track of this.

@oxzi oxzi closed this Jul 29, 2024
@yhabteab yhabteab deleted the fix-syslog-identifier-for-journald branch July 29, 2024 12:56
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.

2 participants