Linux syslog processing cleanup #3036

Merged
merged 9 commits into from Mar 4, 2016

Projects

None yet

5 participants

@Ultra2D
Contributor
Ultra2D commented Feb 18, 2016

Solves issue #3035 for me, but might need some additional testing.

@laf
Member
laf commented Feb 20, 2016

Isn't $entry['program'] empty by that point anyway?

@Ultra2D
Contributor
Ultra2D commented Feb 20, 2016

No, it's not:

Feb 18 12:41:23 librenms postfix/smtp[20284]: 23298404BA: to=mail@example.com, relay=mailserver.example.com[127.0.0.1]:25, delay=0.21, delays=0.01/0/0/0.2, dsn=2.0.0, status=sent (250 2.0.0 Ok: queued as 265BE3A62E0)

$entry['program'] will be postfix/smtp .

@laf
Member
laf commented Feb 20, 2016

I can't say if / what this may break for others. @librenms/reviewers ?

@paulgear
Member

It seems to me this could have unintended side-effects; needs more testing before merge, I say.

@Ultra2D
Contributor
Ultra2D commented Feb 22, 2016

After looking into it some more, it seems like this rule is mainly matching authentication lines (see comments in the original code), for instance:

Feb 22 10:23:27 librenms su[7154]: pam_unix(su:auth): authentication failure; logname=thatsme uid=1000 euid=0 tty=/dev/pts/0 ruser=thatsme rhost= user=root
Feb 22 10:35:01 librenms CRON[16503]: pam_unix(cron:session): session closed for user librenms
Feb 22 10:35:08 librenms sshd[17015]: pam_unix(sshd:session): session opened for user root by (uid=0)
Feb 22 10:35:08 librenms systemd: pam_unix(systemd-user:session): session opened for user root by (uid=0)
Feb 22 10:35:52 librenms sudo: pam_unix(sudo:session): session opened for user librenms by root(uid=0)

It is replacing su/CRON/sshd/systemd/sudo with pam_unix(<program>:session) as $entry['program']. The regexp is loosely defined, and because it is matching either ( or [, this will also match the postfix line posted earlier.
The question then becomes, is this the desired behaviour?

@Ultra2D
Contributor
Ultra2D commented Feb 26, 2016

Would it be easier if I just prevented wrong matches and keep the original behaviour?

@laf
Member
laf commented Feb 26, 2016

The original behaviour would be better, the caution we have with big changes like this is the level of testing we can do :(

#3099 is also dealing with some syslog changes which could conflict as an fyi.

@Ultra2D Ultra2D Keep original behaviour, just prevent false matches
fb9224d
@Ultra2D
Contributor
Ultra2D commented Mar 2, 2016

This commit should match the two examples in the code, and won't match Postfix lines including something like [127.0.0.1]:

// pam_krb5(sshd:auth): authentication failure; logname=root uid=0 euid=0 tty=ssh ruser= rhost=123.213.132.231
// pam_krb5[sshd:auth]: authentication failure; logname=root uid=0 euid=0 tty=ssh ruser= rhost=123.213.132.231

Changes:

  • The part preceding the brackets cannot include :
  • The closing bracket should match the opening one, so either [] or ()
  • The : can be preceded and/or follow by a space
@murrant
Contributor
murrant commented Mar 3, 2016

@Ultra2D we just merged a change that adds unit tests for the syslog parser (tests/SyslogTest.php). It would be great if you can add some test cases there. Thanks.

@Ultra2D Ultra2D Merge branch 'master' into issue-3035
4c57015
@Ultra2D
Contributor
Ultra2D commented Mar 4, 2016

Will do, seems easy enough to extend it now the hard work has been done.

@laf
Member
laf commented Mar 4, 2016

Is this pr still relevant @Ultra2D ?

@Ultra2D
Contributor
Ultra2D commented Mar 4, 2016

Yes, the current code will still match too much when OS is Linux. As requested, I will try to add some tests.

@Ultra2D Ultra2D PHPunit tests for Linux syslog parsing
70ad241
@Ultra2D Ultra2D Fix build failure
24d7926
@Ultra2D Ultra2D Missing trailing delimiter in original code
2b6439d
@Ultra2D Ultra2D Proper tags and facilities; additional example
99cd9f9
@Ultra2D Ultra2D array_walk does not work like that, apparently input is trimmed later on
488f7f4
@Ultra2D
Contributor
Ultra2D commented Mar 4, 2016

Alright, I'm done. Two bugs got squashed in the process.

@Ultra2D
Contributor
Ultra2D commented Mar 4, 2016

On second thought, the dovecot-line was added in 2011 (https://github.com/librenms/librenms/blob/2778e4b90d8a0d38f11b9e6fb29332fae57d19f5/includes/syslog.php#L64) and has never worked, I will remove it completely.

@Ultra2D Ultra2D Remove dovecot match because of invalid pattern
25e88f7
@Ultra2D Ultra2D changed the title from Fixed syslog: only parse messages if program is empty to Linux syslog processing cleanup Mar 4, 2016
@laf laf merged commit 7ea7bd4 into librenms:master Mar 4, 2016

3 checks passed

Auto-Deploy Build finished. No test results found.
Details
Scrutinizer 2 new issues, 1 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Ultra2D Ultra2D deleted the Ultra2D:issue-3035 branch Mar 5, 2016
@Ultra2D Ultra2D referenced this pull request Mar 5, 2016
Merged

Syslog trim whitespace #3171

@paulgear
Member

Thanks for your efforts on this one everyone - it's great to see all these test cases getting added!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment