Make syslog enterprise_number configurable (fixes #218) - #445
atanishka308 wants to merge 2 commits into
Conversation
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at ad2e7ea87b0fc48dd18937b5d57316ab0ca79ff6.
Thank you for this. The first thing I want to name is rfc5424_round_trip_through_syslog_loose: you changed the round trip to 55512, a PEN that is neither the default nor the one any other test uses, then derived expected_sd_id from that variable instead of writing the string a second time. A test that re-reads the literal it declared four lines above passes however wrong the literal becomes, and you avoided that without being asked. rfc5424_uses_configured_enterprise_number shows the same care by asserting the old value is absent, not only that the new one is present.
maintainer screen 445 returns DO NOT EXECUTE, so everything below ran in rootless podman with --network=none against an extracted copy of your head.
What I verified
The knob works end to end. I added two probe tests of my own, which are not part of your diff: one parses a real config.toml through LacsConfig::try_load, the other does the UDP loopback round trip that spawn_sends_udp_datagrams_to_listener uses.
$ out="$(timeout 1800 podman run --rm --network=none -v /tmp/wt445p:/repo:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim sh -c 'cargo test -p sysknife-core --lib --offline probe_ -- --nocapture 2>&1 | tail -14; cargo test -p sysknife-daemon --lib --offline probe_ -- --nocapture 2>&1 | tail -14' 2>&1)"; rc=$?; printf '%s\n' "$out" | grep -v 'level=warning' | grep -vE 'Compiling|Finished'; echo "podman rc=$rc"
PROBE parsed enterprise_number = 99999
PROBE parsed facility = 1
PROBE try_load returned Ok with enterprise_number = 0
PROBE validate() says: Err("[audit.forward.syslog] enterprise_number must be a nonzero IANA Private Enterprise Number")
test config::tests::probe_enterprise_number_parses_from_a_real_config_file ... ok
test config::tests::probe_zero_in_a_real_config_file_is_what_validate_rejects ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 101 filtered out; finished in 0.00s
PROBE zero-PEN datagram on the wire: <13>1 2026-04-25T08:30:00Z 7c0a6f47c08a sysknife-daemon 833 AUDIT [sysknife@0 seq="42" tx="tx-abc" action="InstallFlatpak" risk="medium" approval="appr-xyz" role="Dev" principal="uid:1000" chain_hash="deadbeef" key_id="v1"] [InstallFlatpak] Install Firefox
PROBE datagram on the wire: <13>1 2026-04-25T08:30:00Z 7c0a6f47c08a sysknife-daemon 833 AUDIT [sysknife@99999 seq="42" tx="tx-abc" action="InstallFlatpak" risk="medium" approval="appr-xyz" role="Dev" principal="uid:1000" chain_hash="deadbeef" key_id="v1"] [InstallFlatpak] Install Firefox
test audit_forward::tests::probe_zero_pen_reaches_the_udp_wire_unchallenged ... ok
test audit_forward::tests::probe_configured_pen_reaches_the_udp_wire ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 901 filtered out; finished in 0.00s
podman rc=0
An operator who writes enterprise_number = 99999 into config.toml gets [sysknife@99999 ...] on the wire. That is the observable consequence, and it holds.
Two of your three guards bite. I reverted the format-string substitution to the literal 32473 in one copy of the tree, and replaced the zero comparison with if false in another, then ran both:
$ for m in m2 m3; do echo "=== M:$m ==="; out="$(timeout 1800 podman run --rm --network=none -v /tmp/wt445$m:/repo:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim sh -c 'cargo test -p sysknife-core --lib --offline syslog_forward_section 2>&1 | tail -6; cargo test -p sysknife-daemon --lib --offline audit_forward::tests::rfc5424 2>&1 | tail -12' 2>&1)"; rc=$?; printf '%s\n' "$out" | grep -v 'level=warning' | grep -vE 'Compiling|Finished'; echo "podman rc=$rc"; echo; done
=== M:m2 ===
test config::tests::syslog_forward_section_accepts_custom_enterprise_number ... ok
test config::tests::syslog_forward_section_rejects_zero_enterprise_number ... ok
test config::tests::syslog_forward_section_default_enterprise_number_is_documentation_pen ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 98 filtered out; finished in 0.00s
thread 'audit_forward::tests::rfc5424_round_trip_through_syslog_loose' (315) panicked at crates/sysknife-daemon/src/audit_forward.rs:754:32:
frame has the sysknife@55512 SD element: <13>1 2026-04-25T08:30:00Z 8350d129770e sysknife-daemon 309 AUDIT [sysknife@32473 seq="42" tx="tx-abc" action="InstallFlatpak" risk="medium" approval="appr-xyz" role="Dev" principal="uid:1000" chain_hash="deadbeef" key_id="v1"] [InstallFlatpak] Install Firefox
failures:
audit_forward::tests::rfc5424_round_trip_through_syslog_loose
audit_forward::tests::rfc5424_uses_configured_enterprise_number
test result: FAILED. 6 passed; 2 failed; 0 ignored; 0 measured; 893 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p sysknife-daemon --lib`
podman rc=0
=== M:m3 ===
failures:
config::tests::syslog_forward_section_rejects_zero_enterprise_number
test result: FAILED. 2 passed; 1 failed; 0 ignored; 0 measured; 98 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p sysknife-core --lib`
running 8 tests
test audit_forward::tests::rfc5424_missing_approval_renders_empty_string ... ok
test audit_forward::tests::rfc5424_facility_changes_pri ... ok
test audit_forward::tests::rfc5424_message_section_contains_summary_and_action_tag ... ok
test audit_forward::tests::rfc5424_starts_with_pri_and_version ... ok
test audit_forward::tests::rfc5424_contains_sd_with_chain_hash_and_seq ... ok
test audit_forward::tests::rfc5424_caller_role_with_quote_is_escaped ... ok
test audit_forward::tests::rfc5424_round_trip_through_syslog_loose ... ok
test audit_forward::tests::rfc5424_uses_configured_enterprise_number ... ok
test result: ok. 8 passed; 0 failed; 0 ignored; 0 measured; 893 filtered out; finished in 0.00s
podman rc=0
Blocking
1. The validate() call in main.rs has nothing behind it
I deleted the three-line hunk from build_forwarder and re-ran every test your PR adds:
$ echo "--- validate() call sites in mutated main.rs ---"; grep -n 'validate()' /tmp/wt445m1/crates/sysknife-daemon/src/main.rs || echo "(none: production hunk removed)"
--- validate() call sites in mutated main.rs ---
(none: production hunk removed)
$ cd /tmp/wt445m1 && out="$(timeout 1800 podman run --rm --network=none -v /tmp/wt445m1:/repo:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim sh -c 'cargo test -p sysknife-core --lib --offline syslog_forward_section 2>&1 | tail -8; echo "core rc=$?"; cargo test -p sysknife-daemon --lib --bins --offline enterprise 2>&1 | tail -8; echo "daemon rc=$?"' 2>&1)"; rc=$?; printf '%s\n' "$out" | grep -v 'level=warning' | grep -vE 'Compiling|Finished'; echo "podman rc=$rc"
running 3 tests
test config::tests::syslog_forward_section_accepts_custom_enterprise_number ... ok
test config::tests::syslog_forward_section_default_enterprise_number_is_documentation_pen ... ok
test config::tests::syslog_forward_section_rejects_zero_enterprise_number ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 98 filtered out; finished in 0.00s
core rc=0
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 900 filtered out; finished in 0.00s
Running unittests src/main.rs (.container-target/debug/deps/sysknife_daemon-39bdf58124b15e95)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 4 filtered out; finished in 0.00s
daemon rc=0
podman rc=0
Green with the rejection removed. build_forwarder has two references in the tree, its definition and one call from main(), and no test touches either. Your unit tests prove validate() returns Err; nothing proves the daemon calls it, and the zero-PEN probe above shows what the formatter emits when it does not.
Every field is pub, so the missing test is short. This one passes on your head and fails with the hunk removed:
#[tokio::test(flavor = "current_thread")]
async fn build_forwarder_refuses_a_zero_enterprise_number() {
use sysknife_core::config::{AuditForwardSection, AuditSection, SyslogForwardSection};
let audit = AuditSection {
forward: Some(AuditForwardSection {
syslog: Some(SyslogForwardSection {
host: "127.0.0.1:65000".to_string(),
facility: 1,
enterprise_number: 0,
}),
}),
..Default::default()
};
let err = build_forwarder(Some(&audit))
.expect_err("a zero PEN must stop the daemon building a forwarder");
assert!(err.to_string().contains("enterprise_number"), "got: {err}");
}Both directions, with that test inserted into a copy of your head and a copy of the mutated tree:
$ for pair in "PR-head:/tmp/wt445p" "M1-validate-removed:/tmp/wt445m1p"; do name=${pair%%:*}; dir=${pair##*:}; echo "=== $name ==="; out="$(timeout 1800 podman run --rm --network=none -v $dir:/repo:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim cargo test -p sysknife-daemon --bins --offline probe_build_forwarder -- --nocapture 2>&1)"; rc=$?; printf '%s\n' "$out" | grep -v 'level=warning' | grep -vE 'Compiling|Finished' | tail -12; echo "podman rc=$rc"; echo; done
=== PR-head ===
Running unittests src/main.rs (.container-target/debug/deps/sysknife_daemon-39bdf58124b15e95)
running 1 test
PROBE build_forwarder returned: [audit.forward.syslog] enterprise_number must be a nonzero IANA Private Enterprise Number
test tests::probe_build_forwarder_refuses_a_zero_enterprise_number ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 4 filtered out; finished in 0.00s
podman rc=0
=== M1-validate-removed ===
a zero PEN must stop the daemon building a forwarder: Some(AuditForwarder { queue_capacity: 4096, queue_max: 4096, drops: 0 })
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test tests::probe_build_forwarder_refuses_a_zero_enterprise_number ... FAILED
failures:
failures:
tests::probe_build_forwarder_refuses_a_zero_enterprise_number
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 4 filtered out; finished in 0.00s
error: test failed, to rerun pass `-p sysknife-daemon --bin sysknife-daemon`
podman rc=101
tokio::test matters: spawn() needs a reactor, and a plain #[test] goes red for the wrong reason.
2. The test baseline, and the three published numbers coupled to it
This is what turned rust red:
test_baseline: rust suite has 1865 tests, baseline says 1861.
Four new tests, four to account for. UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh rewrites tests/evidence/workspace-tests.json together with the provenance fields that record when the suite ran, so please regenerate rather than edit the number by hand. Then the three published literals, which a different job checks against that artifact:
README.md:317,**1,861 Rust tests and 72 frontend tests**docs/distro-support.md:94,baseline is 1,861 Rust tests plus 72 frontenddocs/introduction.md:147,192 typed actions · 1,861 Rust tests + 72 frontend tests · MIT
The coupling runs both ways, which is what catches people: bump the artifact alone and rust goes green while docs-and-hygiene goes red on those three lines.
3. docs/configuration.md does not carry the new key
Line 80 onward is the operator's reference for config.toml, and its [audit.forward.syslog] block lists host and facility. Someone reading that page has no way to find out enterprise_number exists. One line beside facility closes it.
Optional
DOCUMENTATION_PEN now exists twice, once per crate, and the two have to agree by hand. sysknife-daemon already depends on sysknife-core and already names sysknife_core::config in auth.rs, main.rs, and in your own doc comments in audit_forward.rs, so the import costs nothing. Two copies of one value that must match is a shape this repo has been bitten by in its validators, and the header comment asserting parity is what goes stale first. Your note asked whether there is a preferred pattern. There is, and it is the import.
Smaller: the PR body says the zero value is rejected "at config-parse time". try_load parses it without complaint, as the second probe shows, and build_forwarder is what rejects it. Worth correcting so the next reader looks in the right place.
The five failures you flagged are not yours
You were right, and here is the recount. Same command on main and on your head, single-process cargo test:
$ cd /home/entropia/.local/state/sysknife-maint/checkout && rm -rf /tmp/wtmain445 && mkdir -p /tmp/wtmain445 && git archive origin/main | tar -x -C /tmp/wtmain445 && echo "main tree: $(git rev-parse origin/main)" && for pair in "main-b2c823e6:/tmp/wtmain445" "pr445-ad2e7ea8:/tmp/wt445"; do name=${pair%%:*}; dir=${pair##*:}; echo "=== cargo test -p sysknife-daemon --lib (single process) on $name ==="; out="$(timeout 2400 podman run --rm --network=none -v $dir:/repo:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim cargo test -p sysknife-daemon --lib --offline 2>&1)"; rc=$?; printf '%s\n' "$out" | grep -v 'level=warning' | grep -E '^(failures:| [a-z_]+::|test result:)' | tail -15; echo "cargo rc=$rc"; echo; done
main tree: b2c823e68cd5829de77976cb3be2e9b163a6fe05
=== cargo test -p sysknife-daemon --lib (single process) on main-b2c823e6 ===
failures:
failures:
transactions::tests::multiple_records_produce_one_watermark_each
transactions::tests::no_watermark_emitted_before_any_record
transactions::tests::record_emits_one_watermark_per_entry
transactions::tests::record_previewed_emits_one_watermark
transactions::tests::watermark_seq_and_hash_match_chain_row
test result: FAILED. 895 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.06s
cargo rc=101
=== cargo test -p sysknife-daemon --lib (single process) on pr445-ad2e7ea8 ===
failures:
failures:
executor::tests::a_child_that_outlives_a_stdout_read_failure_is_stopped_not_orphaned
transactions::tests::multiple_records_produce_one_watermark_each
transactions::tests::no_watermark_emitted_before_any_record
transactions::tests::record_emits_one_watermark_per_entry
transactions::tests::record_previewed_emits_one_watermark
transactions::tests::watermark_seq_and_hash_match_chain_row
test result: FAILED. 895 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 5.64s
cargo rc=101
The same five on b2c823e6 with nothing of yours applied. It is tracked as #356, a test that sets a process-global env var, and I am not asking you to take it.
The sixth name on your side is a load-sensitive flake, not a regression. Three isolated runs on each tree:
$ for pair in "main:/tmp/wtmain445" "pr445:/tmp/wt445"; do name=${pair%%:*}; dir=${pair##*:}; echo "=== $name: executor stdout-read test, 3 isolated runs ==="; out="$(timeout 1200 podman run --rm --network=none -v $dir:/repo:z -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/repo/.container-target -e CARGO_NET_OFFLINE=true docker.io/library/rust:1-slim sh -c 'for i in 1 2 3; do cargo test -p sysknife-daemon --lib --offline a_child_that_outlives_a_stdout_read_failure 2>&1 | grep -E "^test result:"; done' 2>&1)"; rc=$?; printf '%s\n' "$out" | grep -v 'level=warning'; echo "rc=$rc"; echo; done
=== main: executor stdout-read test, 3 isolated runs ===
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 899 filtered out; finished in 0.06s
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 899 filtered out; finished in 0.06s
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 899 filtered out; finished in 0.00s
rc=0
=== pr445: executor stdout-read test, 3 isolated runs ===
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 900 filtered out; finished in 0.06s
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 900 filtered out; finished in 0.06s
test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 900 filtered out; finished in 0.06s
rc=0
The repo's gate is cargo nextest run --workspace --locked, which gives each test its own process and misses none of this; CI reported 1865 tests run: 1865 passed on your head. For a clean local run, cargo nextest is the one to reach for.
Next
If you want another after this one, #268 is the closest fit to what you have been reading all week: the audit export publishes request_hash, an unsalted hash over unredacted params, with nothing stating its sensitivity. You have spent this PR inside the audit forwarding path thinking about what a SIEM does with the fields it receives, which is most of the context that issue needs. Say the word and I will hold it for you.
One more thing, easy to decline. Somebody who cares whether an SD-ID carries a real IANA PEN is running syslog into a SIEM that checks. If you have an Ubuntu box where that matters, the read-only side of SysKnife costs nothing to try: sysknife --dry-run "show me the firewall rules" plans, prints, and touches nothing. I would be glad to hear what it gets wrong on a real host.
|
Correcting myself on the last paragraph of my review: I offered you #268 without checking what you already hold. You have #440 assigned and active, and this PR to finish. Three open threads is more than I should be handing anybody. So: #268 is not going anywhere and I am not holding it for you yet. Finish #440 and this one at whatever pace suits you, and if #268 still looks interesting after that, say so and it is yours then. Nothing in my review changes otherwise. |
2 participants
Summary
Adds an optional
enterprise_numberfield to[audit.forward.syslog]inconfig.toml, and threads it through to the syslog SD-ID (
sysknife@<PEN>)in
format_rfc5424instead of the hardcoded literalsysknife@32473.32473is RFC 5612's reserved documentation/test Private EnterpriseNumber, not a real assigned PEN — so every syslog frame this daemon
emitted was mislabeled for any production SIEM ingest that cares about
SD-ID correctness. The field defaults to
32473(unchanged behavior foranyone who doesn't set it), but an operator with a real IANA PEN can now
configure it instead of patching source.
A value of
0is rejected at config-parse time viaSyslogForwardSection::validate(), called frombuild_forwarderinmain.rs.Related Issue
Fixes #218
Validation
Notes for Reviewers
SyslogForwardSection(config.rs),AuditSinkSpec::SyslogUdpandformat_rfc5424(audit_forward.rs), and thebuild_forwarderconstructor + a test helper in
main.rs/state.rs. The compilercaught all of these via
missing fielderrors.DOCUMENTATION_PEN(32473) is now defined as apub constin bothsysknife-core::configandsysknife-daemon::audit_forward, since Iwasn't sure whether daemon should depend on core's config module just
for this constant — happy to consolidate if there's already a
preferred pattern for that.
transactions.rs/audit_watermark.rsunrelated to this change(a shared test-only "watermark sink" singleton conflicts across
tests in the same process, even single-threaded). Confirmed via
git stashthat these fail identically onmainbefore this PR.Not fixed here since it's out of scope, but flagging in case it's
not already tracked.