Syslog receiver doesn't support messages without pri header.

[haimrubinstein]

Component(s)

pkg/stanza, receiver/syslog

What happened?

Description

Currently, if I send a syslog message without a pri header I get the following error:
"Expecting a priority value within angle brackets".

in the new standard (RFC 5424) it stated that 'Facility and Severity values are not normative but often used.' meaning that the pri header is not mandatory.
as a result, multiple products are sending Syslog messages without the pri header.
Also, RFC3164 in the pri header section talks about relay messages and implies that it might not be mandatory in other non-relaying options.

I did some digging and found that syslog parser is using a package called 'go-syslog'.
to fix the issue I forked the repo, and managed to fix the behavior by adding a new flag 'WithAllowSkipPri'.

I opened a pull request .
The problem is that the repo is not maintained. the last PR merged was 4 months ago (it was a very! minor dependencies PR) and before that, the last merge PR was more than 3 years ago.
using an unmaintained repo is also a security concern.

to fix the issue here there are a couple of options:

• point to my forked repo instead.
• add code to check if missing pri header and if it's missing and the flag is true -> add a dummy pri header to the message.

I can do either suggestion, or any other idea you may have that will solve the issue.

Steps to Reproduce

Send syslog message without pri header.

Expected Result

expected message to be parsed.

Actual Result

got an error.

Collector version

0.89

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

receivers:
  syslog:
    tcp:
      one_log_per_packet: true
      listen_address: "0.0.0.0:54529"
    protocol: rfc3164

Log output

No response

Additional context

No response

[github-actions]

Pinging code owners:

• pkg/stanza: @djaglowski
• receiver/syslog: @djaglowski

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[crobert-1]

I was a bit unsure about whether PRI is really optional based on the wording of The PRI part MUST have... in the spec, but I believe you're right based on the fact that its only comprised of the facility and severity. Therefore, I agree with your understanding. Thanks for filing and also submitting a PR against the go-syslog package too!

Removing the needs triage label

[djaglowski]

@haim6678, thanks for reporting this and for the upstream PR as well.

I share your concerns about that repo being a bit stale and am open to other options, but I am a bit reluctant to switch to immediately switch to a fork.

Another option might be to embed a copy of the code within this repo, as we've done with other dependencies such as ctimefmt. I don't relish the idea of maintaining this ourselves but at least we would have the ability to quickly fix issues like this. Maybe @open-telemetry/collector-contrib-approvers or @leodido have opinions about this.

[andrzej-stencel]

The fact that the https://github.com/influxdata/go-syslog library is not actively maintained is indeed a major issue. The OpenTelemetry Collector should definitely 1. have confidence in the security posture of the dependencies and 2. be able to customize the behavior of the Syslog receiver, for example by implementing a feature like the optional PRI header.

I'm not opposed to using a fork of the above library as an intermediate measure - to me it doesn't seem worse at all than using a library that seems to be completely abandoned.

I'm not convinced that embedding the library's code in the collector is a good approach. I believe there's space for a standalone Go library to parse syslog messages that the OpenTelemetry Collector and others can use, so it should exist as a separate project. Whether it's the one that Stanza has been using (revived or forked) or a different one, that's probably something to look into.

[djaglowski]

I am not convinced that every dependency needs to be "actively" maintained. Many packages solve relatively simple problems and do not require frequent changes after they've reached a certain point. If there's a reported security issue that isn't getting resolved, that's one thing, but I don't necessarily believe that every dependency requires recent changes in order to be secure. If we apply that standard broadly I think we will end up responsible for way too many things. For example, just quickly looking at hostmetrics's direct dependencies, excluding modules from the collector and several which are immediately recognizable as widely used, I see that github.com/leoluk/perflib_exporter is archived and github.com/yusufpapurcu/wmi is arguably less active than go-syslog. Poking around just a little more, I easily find plenty of other dependencies which are essentially frozen.

As I understand it we use standard security practices which include scanning for known security issues in both direct and indirect dependencies. If we need to fork a dependency in order to resolve an outstanding security issue, we do so. In my opinion, we should not do so proactively only for security reasons. More generally, forking a dependency without a clear plan to maintain it ourselves may be more likely to introduce security issues in the future, if we ourselves aren't properly maintaining the fork.

All that said, we need a dependency which allows us to properly interpret the syslog protocols as they are written. If we need to fork the repo in order to do that, I'm in support of it. My main concern with a fork is that there has in the past been a lot of pressure to diverge from the syslog protocols. In principle I am in support of doing so to the extent it is useful to our users, BUT no one has articulated a reasonable strategy for managing the divergences we would allow. In the absence of an agreed upon strategy, any fork we depend on should continue to strictly support the protocols (as go-syslog has done very well). @haim6678, if you're in agreement here, I'm fine with switching to your fork.

[andrzej-stencel]

I see that github.com/leoluk/perflib_exporter is archived

Yeah that doesn't look good 😅

In my opinion, we should not do so proactively only for security reasons. More generally, forking a dependency without a clear plan to maintain it ourselves may be more likely to introduce security issues in the future, if we ourselves aren't properly maintaining the fork.

Very good points, I agree 💯

In the absence of an agreed upon strategy, any fork we depend on should continue to strictly support the protocols

Again agree, thanks for raising this.

[leodido]

Hello everybody, I am the author of go-syslog.

I don't work anymore at InfluxData since 5 years, but I have been maintaining it in my (little) spare time since then, without any sort of external support and without full admin access to it.

I am in the process of asking InfluxData to move back that repo to my personal GitHub account so that I can try to find sponsorships or any form of support.

Said that, I will properly (protocols are tricky, Ragel even more...) review the PR about the 'pri' as soon I have some more spare time.

If you are in a rush and want to switch to a fork, who am I to block you?

[haimrubinstein]

@leodido thank you for the answer, and for all the work.
have you considered forking the project to your account? I assume it will be better than the current situation.

[leodido]

@haim6678 that's the plan B at this point :)

[haimrubinstein]

@haim6678 that's the plan B at this point :)

I got it. thanks.
I would appreciate if you could spare a few minutes to have a look.

[haimrubinstein]

@djaglowski I opened a pr switching to my forked repo and adding the new flag
#30869

[djaglowski]

Closed by #30869