Re: rc.d/spamlogd
On 2022/04/21 10:23, Stuart Henderson wrote:
> Anyone fancy giving an explicit ok for this? Preferably someone who
> uses spamd?
Ah I missed that jturner@ already did, sorry for the noise!
>
> On 2022/04/15 22:00, Stuart Henderson wrote:
> > jturner had a problem with this, here's a diff on top of what was
> > committed.
> >
> > - I think the first line is superfluous as /etc/rc.d/rc.subr has some
> > special case for this (and I think this maybe responsible for a problem
> > which jturner ran into)
> >
> > - look for the pflog interface passed in flags and init that
> >
> > I think this is probably correct within the bounds of how spamlogd
> > currently works but some tests would be appreciated (as would
> > any improvements to the chicken scratches)
> >
> > Index: spamlogd
> > ===
> > RCS file: /cvs/src/etc/rc.d/spamlogd,v
> > retrieving revision 1.5
> > diff -u -p -r1.5 spamlogd
> > --- spamlogd11 Apr 2022 09:32:20 - 1.5
> > +++ spamlogd15 Apr 2022 21:00:10 -
> > @@ -9,11 +9,13 @@ daemon="/usr/libexec/spamlogd"
> > rc_reload=NO
> >
> > rc_pre() {
> > - [[ ${spamd_flags} != NO && ${spamd_black} == NO ]] && return 1
> > + pflog=$(echo $daemon_flags | sed -En 's/.*-l *(pflog[0-9]+).*/\1/p')
> > + pflog=${pflog:-pflog0}
> > +
> > if pfctl -si | grep -q Enabled; then
> > - ifconfig pflog0 create
> > - if ifconfig pflog0; then
> > - ifconfig pflog0 up
> > + ifconfig $pflog create
> > + if ifconfig $pflog; then
> > + ifconfig $pflog up
> > else
> > return 1
> > fi
> >
>
Re: rc.d/spamlogd
Anyone fancy giving an explicit ok for this? Preferably someone who
uses spamd?
On 2022/04/15 22:00, Stuart Henderson wrote:
> jturner had a problem with this, here's a diff on top of what was
> committed.
>
> - I think the first line is superfluous as /etc/rc.d/rc.subr has some
> special case for this (and I think this maybe responsible for a problem
> which jturner ran into)
>
> - look for the pflog interface passed in flags and init that
>
> I think this is probably correct within the bounds of how spamlogd
> currently works but some tests would be appreciated (as would
> any improvements to the chicken scratches)
>
> Index: spamlogd
> ===
> RCS file: /cvs/src/etc/rc.d/spamlogd,v
> retrieving revision 1.5
> diff -u -p -r1.5 spamlogd
> --- spamlogd 11 Apr 2022 09:32:20 - 1.5
> +++ spamlogd 15 Apr 2022 21:00:10 -
> @@ -9,11 +9,13 @@ daemon="/usr/libexec/spamlogd"
> rc_reload=NO
>
> rc_pre() {
> - [[ ${spamd_flags} != NO && ${spamd_black} == NO ]] && return 1
> + pflog=$(echo $daemon_flags | sed -En 's/.*-l *(pflog[0-9]+).*/\1/p')
> + pflog=${pflog:-pflog0}
> +
> if pfctl -si | grep -q Enabled; then
> - ifconfig pflog0 create
> - if ifconfig pflog0; then
> - ifconfig pflog0 up
> + ifconfig $pflog create
> + if ifconfig $pflog; then
> + ifconfig $pflog up
> else
> return 1
> fi
>
Re: rc.d/spamlogd
On 2022/04/18 03:13, Clint Pachl wrote: > On Fri, 15 Apr 2022 22:00:35 +0100 > Stuart Henderson wrote: > > > jturner had a problem with this, here's a diff on top of what was > > committed. > > > > - I think the first line is superfluous as /etc/rc.d/rc.subr has some > > special case for this (and I think this maybe responsible for a > > problem which jturner ran into) > > You're right, the same spamlogd start conditions are checked in both > _rc_quirks() in rc.d/rc.subr and rc_pre() in rc.d/spamlogd. > > However, isn't it better to have this spamlogd quirk in rc.d/spamlogd > and remove it from rc.d/rc.subr? Default config is: $ grep spam /etc/rc.conf spamd_flags=NO # also see spamd_black below spamlogd_flags= # use eg. "-i interface" and see spamlogd(8) spamd_black=NO # set to YES to run spamd without greylisting So with that change it will show spamlogd(failed) for people with default config (we could change the default spamlogd_flags to NO to avoid that but then spamd users will need to change config. I prefer to just fix the problem and return to forgetting that spamd exists again (I haven't used it for probably 10 years when I worked out that it causes me more problems than it solves - I now use more selective greylisting, only on high-spam-scoring mail). > The _rc_quirks() function (including the spamlogd pre-start checks) was > added to rc.d/rc.subr,v1.73 (2014). I'm guessing it was to fix spamlogd > because it was broken since the error introduced in rc.d/spamlogd,v1.2 > (2011), where the start conditions were no longer checked properly > in rc_pre(). I think the fix went in the wrong file back then, unless > I'm mistaken as to how the rc system functions. > > > > > > - look for the pflog interface passed in flags and init that > > This is an improvement. > > In fact, this change could be applied to rc.d/pflogd too. For example, > if pflog1 is specified in pflogd_flags, it will not be created via > rc_pre(), which only creates pflog0 (see -i in pflogd(8)). That probably makes sense. > Again, if my thinking is correct, it seems the "special care" for > pflogd startup in _rc_quirks() (i.e. check if pf is enabled) should be > moved to rc_pre() in rc.d/pflogd. Correct? I think this has the same issue with printing "(failed)", this time if PF is disabled, though at least that's not in default config.
Re: rc.d/spamlogd
On Fri, 15 Apr 2022 22:00:35 +0100
Stuart Henderson wrote:
> jturner had a problem with this, here's a diff on top of what was
> committed.
>
> - I think the first line is superfluous as /etc/rc.d/rc.subr has some
> special case for this (and I think this maybe responsible for a
> problem which jturner ran into)
You're right, the same spamlogd start conditions are checked in both
_rc_quirks() in rc.d/rc.subr and rc_pre() in rc.d/spamlogd.
However, isn't it better to have this spamlogd quirk in rc.d/spamlogd
and remove it from rc.d/rc.subr?
The _rc_quirks() function (including the spamlogd pre-start checks) was
added to rc.d/rc.subr,v1.73 (2014). I'm guessing it was to fix spamlogd
because it was broken since the error introduced in rc.d/spamlogd,v1.2
(2011), where the start conditions were no longer checked properly
in rc_pre(). I think the fix went in the wrong file back then, unless
I'm mistaken as to how the rc system functions.
>
> - look for the pflog interface passed in flags and init that
This is an improvement.
In fact, this change could be applied to rc.d/pflogd too. For example,
if pflog1 is specified in pflogd_flags, it will not be created via
rc_pre(), which only creates pflog0 (see -i in pflogd(8)).
Again, if my thinking is correct, it seems the "special care" for
pflogd startup in _rc_quirks() (i.e. check if pf is enabled) should be
moved to rc_pre() in rc.d/pflogd. Correct?
> I think this is probably correct within the bounds of how spamlogd
> currently works but some tests would be appreciated (as would
> any improvements to the chicken scratches)
>
> Index: spamlogd
> ===
> RCS file: /cvs/src/etc/rc.d/spamlogd,v
> retrieving revision 1.5
> diff -u -p -r1.5 spamlogd
> --- spamlogd 11 Apr 2022 09:32:20 - 1.5
> +++ spamlogd 15 Apr 2022 21:00:10 -
> @@ -9,11 +9,13 @@ daemon="/usr/libexec/spamlogd"
> rc_reload=NO
>
> rc_pre() {
> - [[ ${spamd_flags} != NO && ${spamd_black} == NO ]] && return
> 1
> + pflog=$(echo $daemon_flags | sed -En 's/.*-l
> *(pflog[0-9]+).*/\1/p')
> + pflog=${pflog:-pflog0}
> +
> if pfctl -si | grep -q Enabled; then
> - ifconfig pflog0 create
> - if ifconfig pflog0; then
> - ifconfig pflog0 up
> + ifconfig $pflog create
> + if ifconfig $pflog; then
> + ifconfig $pflog up
> else
> return 1
> fi
>
Re: rc.d/spamlogd
On Fri, Apr 15, 2022 at 10:00:35PM +0100, Stuart Henderson wrote:
> jturner had a problem with this, here's a diff on top of what was
> committed.
>
> - I think the first line is superfluous as /etc/rc.d/rc.subr has some
> special case for this (and I think this maybe responsible for a problem
> which jturner ran into)
>
> - look for the pflog interface passed in flags and init that
>
> I think this is probably correct within the bounds of how spamlogd
> currently works but some tests would be appreciated (as would
> any improvements to the chicken scratches)
>
This does indeed fix the issue of spamlogd no starting for me. Thanks.
ok jturner@
> Index: spamlogd
> ===
> RCS file: /cvs/src/etc/rc.d/spamlogd,v
> retrieving revision 1.5
> diff -u -p -r1.5 spamlogd
> --- spamlogd 11 Apr 2022 09:32:20 - 1.5
> +++ spamlogd 15 Apr 2022 21:00:10 -
> @@ -9,11 +9,13 @@ daemon="/usr/libexec/spamlogd"
> rc_reload=NO
>
> rc_pre() {
> - [[ ${spamd_flags} != NO && ${spamd_black} == NO ]] && return 1
> + pflog=$(echo $daemon_flags | sed -En 's/.*-l *(pflog[0-9]+).*/\1/p')
> + pflog=${pflog:-pflog0}
> +
> if pfctl -si | grep -q Enabled; then
> - ifconfig pflog0 create
> - if ifconfig pflog0; then
> - ifconfig pflog0 up
> + ifconfig $pflog create
> + if ifconfig $pflog; then
> + ifconfig $pflog up
> else
> return 1
> fi
>
Re: rc.d/spamlogd
jturner had a problem with this, here's a diff on top of what was
committed.
- I think the first line is superfluous as /etc/rc.d/rc.subr has some
special case for this (and I think this maybe responsible for a problem
which jturner ran into)
- look for the pflog interface passed in flags and init that
I think this is probably correct within the bounds of how spamlogd
currently works but some tests would be appreciated (as would
any improvements to the chicken scratches)
Index: spamlogd
===
RCS file: /cvs/src/etc/rc.d/spamlogd,v
retrieving revision 1.5
diff -u -p -r1.5 spamlogd
--- spamlogd11 Apr 2022 09:32:20 - 1.5
+++ spamlogd15 Apr 2022 21:00:10 -
@@ -9,11 +9,13 @@ daemon="/usr/libexec/spamlogd"
rc_reload=NO
rc_pre() {
- [[ ${spamd_flags} != NO && ${spamd_black} == NO ]] && return 1
+ pflog=$(echo $daemon_flags | sed -En 's/.*-l *(pflog[0-9]+).*/\1/p')
+ pflog=${pflog:-pflog0}
+
if pfctl -si | grep -q Enabled; then
- ifconfig pflog0 create
- if ifconfig pflog0; then
- ifconfig pflog0 up
+ ifconfig $pflog create
+ if ifconfig $pflog; then
+ ifconfig $pflog up
else
return 1
fi
Re: rc.d/spamlogd
Your patch looks good Stuart. It definitely fixes the spamlogd
pre-check.
On Sun, 3 Apr 2022 12:08:06 +0100
Stuart Henderson wrote:
> On 2022/04/03 01:50, Clint Pachl wrote:
> > Setting aside pflog, I still think there may have been an error
> > introduced in revision 1.2, as I stated in my bug description below.
> >
> > Unless "rc_pre" works in a way that I don't understand, the first
> > line of the rc_pre function in rc.d/spamlogd is an error; it's an
> > unchecked condition.
>
> Hmm, I wonder if this code used to run with "set -e" (which does work
> as expected with the unchecked condition).
I initially thought so too, but it doesn't seem to be the case. I
couldn't find any setting of errexit in rc.subr going back 9+ years.
It seems this error was introduced in rc.d/spamlogd revision 1.2 when
the pf/pflog stuff was added to the pre-check function.
> > Here's a patch to explain:
>
> alternatively, avoiding extending the laddered if:
>
> Index: spamlogd
> ===
> RCS file: /cvs/src/etc/rc.d/spamlogd,v
> retrieving revision 1.4
> diff -u -p -r1.4 spamlogd
> --- spamlogd 11 Jan 2018 21:09:26 - 1.4
> +++ spamlogd 3 Apr 2022 11:05:21 -
> @@ -9,7 +9,7 @@ daemon="/usr/libexec/spamlogd"
> rc_reload=NO
>
> rc_pre() {
> - [[ ${spamd_flags} != NO && ${spamd_black} == NO ]]
> + [[ ${spamd_flags} != NO && ${spamd_black} == NO ]] && return
> 1 if pfctl -si | grep -q Enabled; then
> ifconfig pflog0 create
> if ifconfig pflog0; then
>
Re: rc.d/spamlogd
Setting aside pflog, I still think there may have been an error
introduced in revision 1.2, as I stated in my bug description below.
Unless "rc_pre" works in a way that I don't understand, the first line
of the rc_pre function in rc.d/spamlogd is an error; it's an unchecked
condition.
Here's a patch to explain:
--- spamlogd,v 1.4
+++ spamlogd-patch Sun Apr 3 01:04:16 2022
@@ -9,11 +9,14 @@
rc_reload=NO
rc_pre() {
- [[ ${spamd_flags} != NO && ${spamd_black} == NO ]]
- if pfctl -si | grep -q Enabled; then
- ifconfig pflog0 create
- if ifconfig pflog0; then
- ifconfig pflog0 up
+ if [[ ${spamd_flags} != NO && ${spamd_black} == NO ]]; then
+ if pfctl -si | grep -q Enabled; then
+ ifconfig pflog0 create
+ if ifconfig pflog0; then
+ ifconfig pflog0 up
+ else
+ return 1
+ fi
else
return 1
fi
On Wed, 30 Mar 2022 11:57:49 -0600
"Theo de Raadt" wrote:
> Right. However, this was discussed before that there is a wierdness
> here -- the spam tools assume they get that pflog interface, but there
> are other usage cases where that would be wrong. We didn't find a
> clean solution for making spamlogd use a unique interface last time we
> discussed this.
>
>
> Stuart Henderson wrote:
>
> > I think the intent is that the pflog interface gets created in case
> > pflogd is disabled (which is a perfectly valid configuration)
> >
> > --
> > Sent from a phone, apologies for poor formatting.
> >
> > On 30 March 2022 07:53:39 pa...@ecentryx.com wrote:
> >
> > >> Synopsis: rc_pre() not properly checking spamd rc variables
> > >> Category: system
> > >> Environment:
> > > System : OpenBSD 7.0
> > > Details : OpenBSD 7.0 (GENERIC.MP) #5: Mon Jan 31 09:09:02
> > > MST 2022
> > > r...@syspatch-70-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> > >
> > > Architecture: OpenBSD.amd64
> > > Machine : amd64
> > >> Description:
> > > In revision 1.1 of /etc/rc.d/spamlogd, the rc_pre() function
> > > ensures that spamd is enabled and not operating in blacklist-only
> > > mode. Perfect.
> > >
> > > However, revision 1.2 introduced a bug when the code to create
> > > the pflog0 interface was added. Checking whether spamd is enabled
> > > or not is essentially ignored.
> > >
> > >> Fix:
> > > The creation of the pflog0 interface should not be done in
> > > rc.d/spamlogd because the interface is already created in
> > > rc.d/pflogd (it's the exact same in fact). This seems reasonable
> > > because pflogd is enabled by default and starts before spamlogd.
> > >
> > > Instead, rc.d/spamlogd should just check the pflogd flag along
> > > with the spamd flags.
> > >
> > >
> > > --- spamlogd,v 1.4
> > > +++ /tmp/spamlogd Thu Mar 24 04:26:16 2022
> > > @@ -9,17 +9,7 @@
> > > rc_reload=NO
> > >
> > > rc_pre() {
> > > - [[ ${spamd_flags} != NO && ${spamd_black} == NO ]]
> > > - if pfctl -si | grep -q Enabled; then
> > > - ifconfig pflog0 create
> > > - if ifconfig pflog0; then
> > > - ifconfig pflog0 up
> > > - else
> > > - return 1
> > > - fi
> > > - else
> > > - return 1
> > > - fi
> > > + [[ ${spamd_flags} != NO && ${spamd_black} == NO &&
> > > ${pflogd_flags} != NO ]] }
> > >
> > > rc_cmd $1
> >
Re: rc.d/spamlogd
On 2022/04/03 01:50, Clint Pachl wrote:
> Setting aside pflog, I still think there may have been an error
> introduced in revision 1.2, as I stated in my bug description below.
>
> Unless "rc_pre" works in a way that I don't understand, the first line
> of the rc_pre function in rc.d/spamlogd is an error; it's an unchecked
> condition.
Hmm, I wonder if this code used to run with "set -e" (which does work
as expected with the unchecked condition).
> Here's a patch to explain:
alternatively, avoiding extending the laddered if:
Index: spamlogd
===
RCS file: /cvs/src/etc/rc.d/spamlogd,v
retrieving revision 1.4
diff -u -p -r1.4 spamlogd
--- spamlogd11 Jan 2018 21:09:26 - 1.4
+++ spamlogd3 Apr 2022 11:05:21 -
@@ -9,7 +9,7 @@ daemon="/usr/libexec/spamlogd"
rc_reload=NO
rc_pre() {
- [[ ${spamd_flags} != NO && ${spamd_black} == NO ]]
+ [[ ${spamd_flags} != NO && ${spamd_black} == NO ]] && return 1
if pfctl -si | grep -q Enabled; then
ifconfig pflog0 create
if ifconfig pflog0; then
Re: rc.d/spamlogd
Right. However, this was discussed before that there is a wierdness
here -- the spam tools assume they get that pflog interface, but there
are other usage cases where that would be wrong. We didn't find a
clean solution for making spamlogd use a unique interface last time we
discussed this.
Stuart Henderson wrote:
> I think the intent is that the pflog interface gets created in case
> pflogd is disabled (which is a perfectly valid configuration)
>
> --
> Sent from a phone, apologies for poor formatting.
>
> On 30 March 2022 07:53:39 pa...@ecentryx.com wrote:
>
> >> Synopsis: rc_pre() not properly checking spamd rc variables
> >> Category: system
> >> Environment:
> > System : OpenBSD 7.0
> > Details : OpenBSD 7.0 (GENERIC.MP) #5: Mon Jan 31 09:09:02 MST 2022
> >
> > r...@syspatch-70-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
> >
> > Architecture: OpenBSD.amd64
> > Machine : amd64
> >> Description:
> > In revision 1.1 of /etc/rc.d/spamlogd, the rc_pre() function
> > ensures that spamd is enabled and not operating in blacklist-only
> > mode. Perfect.
> >
> > However, revision 1.2 introduced a bug when the code to create
> > the pflog0 interface was added. Checking whether spamd is enabled
> > or not is essentially ignored.
> >
> >> Fix:
> > The creation of the pflog0 interface should not be done in
> > rc.d/spamlogd because the interface is already created in
> > rc.d/pflogd (it's the exact same in fact). This seems reasonable
> > because pflogd is enabled by default and starts before spamlogd.
> >
> > Instead, rc.d/spamlogd should just check the pflogd flag along
> > with the spamd flags.
> >
> >
> > --- spamlogd,v 1.4
> > +++ /tmp/spamlogd Thu Mar 24 04:26:16 2022
> > @@ -9,17 +9,7 @@
> > rc_reload=NO
> >
> > rc_pre() {
> > - [[ ${spamd_flags} != NO && ${spamd_black} == NO ]]
> > - if pfctl -si | grep -q Enabled; then
> > - ifconfig pflog0 create
> > - if ifconfig pflog0; then
> > - ifconfig pflog0 up
> > - else
> > - return 1
> > - fi
> > - else
> > - return 1
> > - fi
> > + [[ ${spamd_flags} != NO && ${spamd_black} == NO && ${pflogd_flags} != NO
> > ]]
> > }
> >
> > rc_cmd $1
>
rc.d/spamlogd
>Synopsis: rc_pre() not properly checking spamd rc variables
>Category: system
>Environment:
System : OpenBSD 7.0
Details : OpenBSD 7.0 (GENERIC.MP) #5: Mon Jan 31 09:09:02 MST 2022
r...@syspatch-70-amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC.MP
Architecture: OpenBSD.amd64
Machine : amd64
>Description:
In revision 1.1 of /etc/rc.d/spamlogd, the rc_pre() function
ensures that spamd is enabled and not operating in blacklist-only
mode. Perfect.
However, revision 1.2 introduced a bug when the code to create
the pflog0 interface was added. Checking whether spamd is enabled
or not is essentially ignored.
>Fix:
The creation of the pflog0 interface should not be done in
rc.d/spamlogd because the interface is already created in
rc.d/pflogd (it's the exact same in fact). This seems reasonable
because pflogd is enabled by default and starts before spamlogd.
Instead, rc.d/spamlogd should just check the pflogd flag along
with the spamd flags.
--- spamlogd,v 1.4
+++ /tmp/spamlogd Thu Mar 24 04:26:16 2022
@@ -9,17 +9,7 @@
rc_reload=NO
rc_pre() {
- [[ ${spamd_flags} != NO && ${spamd_black} == NO ]]
- if pfctl -si | grep -q Enabled; then
- ifconfig pflog0 create
- if ifconfig pflog0; then
- ifconfig pflog0 up
- else
- return 1
- fi
- else
- return 1
- fi
+ [[ ${spamd_flags} != NO && ${spamd_black} == NO && ${pflogd_flags} !=
NO ]]
}
rc_cmd $1
