Make ICMP redirect processing depend on routing daemon

[donner]

Summary

In D22988#503984 is became obvious, that redirect processing
should depend on running a routing daemon. Administrators
should be able to determine their own setting, while the default
should be safe than sorry.

Test Plan

Prepare the relevant part of the diff for some tests

$ cat > t
        _icmp_drop_redirect="${icmp_drop_redirect}"
        case "${_icmp_drop_redirect}" in
        [Aa][Uu][Tt][Oo] | "")
                _icmp_drop_redirect="${routed_enable}"
                ;;
        esac

Test "auto" values:

$ icmp_drop_redirect="auto"
$ routed_enable="no"
$ . t
$ set | grep _
_icmp_drop_redirect=no
icmp_drop_redirect=auto
routed_enable=no

$ routed_enable="yes"
$ . t
$ set | grep _
_icmp_drop_redirect=yes
icmp_drop_redirect=auto
routed_enable=yes

Overwrite the router setting:

$ routed_enable="yes"
$ icmp_drop_redirect="no"
$ . t
$ set | grep _
_icmp_drop_redirect=no
icmp_drop_redirect=no
routed_enable=yes

$ icmp_drop_redirect="yes"
$ . t
$ set | grep _
_icmp_drop_redirect=yes
icmp_drop_redirect=yes
routed_enable=yes

[melifaro]

libexec/rc/rc.conf

237 ↗

(On Diff #67188)

also: icmp6?

libexec/rc/rc.d/routing

309 ↗

(On Diff #67188)

There are a bunch of routing daemons that can be used: frr, quagga, bird, openbgp, ...etc

I guess we should depend on the "dynamicrouting" (if the daemons above are consistent about PROVIDE in their rc.d files).

[rgrimes]

For icmp6 since that is left out of any of this I think starting a new review to add icmpv6 handling is probably in order. I do see a sysctl, net.inet5.icmp.redirectaccept=1, but do not see a log one.

libexec/rc/rc.conf

237 ↗

(On Diff #67188)

The comment of YES no longer matches the set value of Auto nor is the other value (or NO) given. I am mixed here, I think something in the comment needs changed, but can not put my finger on it.

[donner]

libexec/rc/rc.conf

237 ↗

(On Diff #67188)

That was the exact reason to not touch it. It would be either to long or incomprehensible.

libexec/rc/rc.d/routing

309 ↗

(On Diff #67188)

They are third party ports. The setting here is to deal with the base system primary. There might be a better solution, but I'm not in the position to overlook all the consequences and future developments.

[melifaro]

libexec/rc/rc.d/routing

309 ↗

(On Diff #67188)

No one uses routed. In the current form this change is useless.

[donner]

libexec/rc/rc.d/routing

309 ↗

(On Diff #67188)

So let's drop the idea completely, because there is no common pattern available?
Let's instead patch all the ports separately?

[donner]

libexec/rc/rc.d/routing

309 ↗

(On Diff #67188)

The various ports depend on "routing" but that's all they have in common.

[melifaro]

libexec/rc/rc.d/routing

309 ↗

(On Diff #67188)

Well, one can check for "bird frr quagga .." in PROVIDES as a hack.
However, I think it wouldn't be hard to update the most popular ports with relevant "provides" such as routing/dynamicrouting.

@olivier: what do you think?

[olivier]

libexec/rc/rc.d/routing

309 ↗

(On Diff #67188)

Yes I can fix all routing ports to be sure they all PROVIDE the dynamicrouting keyword.

[donner]

libexec/rc/rc.d/routing

309 ↗

(On Diff #67188)

Okay, so I'll depend on this keyword for "auto", too. Will take some time to understand how it can be done at the correct time.

[rgrimes]

melifaro said: No one uses routed. In the current form this change is useless.
I disagree, but the use is sparse, HOWEVER, routed_program, etc all CAN and are overriden. I start what ever routing layer I am using that way, which include but are not limited to: gated, bird, openbgp. And I use routed_enable to turn them on and off.

[rgrimes]

olivier siad: Yes I can fix all routing ports to be sure they all PROVIDE the dynamicrouting keyword.

That would be excellent.

[donner]

Rebase to r357716

[donner]

Is there any framework for obtaining the PROVIDE of enabled services?

[donner]

Checking for enabled dynamicrouting services.

[olivier]

Ports updated to add dynamicrouting to their PROVIDE section

• net/frr5 (r525713)
• net/frr6 (r525715)
• net/frr7 (r525716)
• net/bird2 (r525717)
• net/bird, no need, was already providing this keyword

PR created for other routing daemon I do not maintain:

• net/quagga (PR 244027)
• net/openbgpd (PR 244028)
• net/openbgpd6 (PR 244029)

[donner]

Is there anything missing?

[melifaro]

libexec/rc/rc.d/routing

341 ↗

(On Diff #68054)

Can we just change this to "NO" ?

[donner]

libexec/rc/rc.d/routing

341 ↗

(On Diff #68054)

We can do this, if rc.d/routed would provide "dynamicrouting".
So you propose to set it to "no" and simultanously change routed?
(I'd like to understand before generating a new patch)

[melifaro]

libexec/rc/rc.d/routing

341 ↗

(On Diff #68054)

I propose removing anything related to routed.

[donner]

libexec/rc/rc.d/routing

341 ↗

(On Diff #68054)

Please keep this outside of this patch.
Add another patch to remove routed, there you can replace "$routed_enable" by "no".
So I'll not change this patch here.

[melifaro]

libexec/rc/rc.d/routing

341 ↗

(On Diff #68054)

Let me try to rephrase it. There is nothing specific about routed - technically it's one of the routing daemons, same as bird, frr or quagga. There should be no specific case handling it in rc.d/routing.

If you want, you can add the relevant PROVIDE to the etc/rc.d/routed in the same review.

[donner]

considering routed as a common case, not a special handling

[donner]

If somebody has some spare time to land this, it would be fine.
I do not have any commit rights.

[rgrimes]

In D23329#518006, @olivier wrote:

...

PR created for other routing daemon I do not maintain:

• net/quagga (PR 244027)
• net/openbgpd (PR 244028)
• net/openbgpd6 (PR 244029)

I have added myself to these 3 bugs and if no one else has committed the base change once those are closed I'll do it. Thanks for getting all the others Oliver

[olivier]

Thanks to their maintainers, the RC scripts of net/quagga, net/openbgpd and net/openbgpd6 are now publishing the "dynamicrouting" keyword too.

[hrs]

libexec/rc/rc.d/routing

295 ↗

(On Diff #68218)

This function should simply return true or false, not expect another function use the $_dynamicrouting variable as the return value. I think the function name should also be changed because the search result will not be actually used.

306 ↗

(On Diff #68218)

This loop does not work well because there are more complex cases such as set_rcvar() and set_rcvar_obsolete(). Invoking "rc.d/foo enabled" is the only way to get value of $foo_enable reliably.

I do not like invoking grep(1) multiple times because it slows down the boot time. How about doing grep "^# PROVIDE: .*dynamicrouting.*" for all of rc.d scripts listed by rcorder(8) at once, and then parsing the output and checking the results of "rc.d/foo enabled" individually?

334 ↗

(On Diff #68218)

Why another variable is required here? Just overriding the value depending on _search_dynamicrouting() should work.

[donner]

Switch to a more effienct processing of rc.files

[donner]

libexec/rc/rc.d/routing

306 ↗

(On Diff #68218)

Thank you for the idea of grepping before evaluating.

The >eval "$(grep rcvar $file)"< part should handle all the "set_rcvar" issues, you mentioned. This part was taken from /usr/sbin/service -e

[donner]

Declare temporary variable as local.

[donner]

libexec/rc/rc.d/routing

334 ↗

(On Diff #68218)

I do not want to overwrite a configuration variable. To me this is bad practice. We do not know, if somebody will rely on this variable later on in a third party package.

Trying to handle all cases explicitly would require to duplicate the checkyesno code from rc.subr (i.e. for the debugging output). That's path I'd like to avoid.

But you are right, I updated the patch to avoid environment pollution.

[hrs]

libexec/rc/rc.d/routing

306 ↗

(On Diff #68218)

No, please do not try to parse scripts by using grep. It is quite fragile. service(8) is broken in this regard, too.

"set_rcvar foo_enable YES" is another way to define foo_enable while it was not populated well. And name= and rcvar= can appear multiple times in a single script as in rc.d/sendmail and there is a plan to extend it to support multiple profiles which can have multiple rcvar= lines. So a handcrafted routine to extract foo_enable by parsing in rc.d/routnig is unnecessarily complex and harmful for future maintenance.

Why just calling the listed scripts with "enabled" argument is not enough? Calling load_rc_config_var() is so heavy and has substantially the same load as calling the script itself with "enabled".

[donner]

libexec/rc/rc.d/routing

306 ↗

(On Diff #68218)

I'm lost. What's your proposal to determine the existence of an enabled service providing a special tag (dynamicrouting)?

If there is no workable solution, I'll just modify the man page the clarify the usage of "yes"/"no".

[hrs]

libexec/rc/rc.d/routing

306 ↗

(On Diff #68218)

I have kept saying the same proposal:

Invoking "rc.d/foo enabled" is the only way to get value of $foo_enable reliably.

Why just calling the listed scripts with "enabled" argument is not enough?

Again, please use calls of the scripts listed by grep with "enabled" instead of parsing them. More specifically, replacing l.306 to l.317 with the following should work for your purpose:

for file in $( rcorder ${skip} /etc/rc.d/* ${local_rc} 2>/dev/null |
    xargs grep -lE '^# PROVIDE:.*\<dynamicrouting\>' ); do
        (set -- enabled; . $file) && return 0
done

[donner]

Change to the correct idiom for enabled state.

[donner]

@hrs Are your concerns resolved?

[melifaro]

I'm going to commit this tomorrow.