Page MenuHomeFreeBSD

Make ICMP redirect processing depend on routing daemon
ClosedPublic

Authored by lutz_donnerhacke.de on Jan 23 2020, 11:04 AM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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 added a subscriber: 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.

This revision is now accepted and ready to land.Jan 27 2020, 6:14 PM
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.

libexec/rc/rc.d/routing
309 ↗(On Diff #67188)

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

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?

libexec/rc/rc.d/routing
309 ↗(On Diff #67188)

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

melifaro added inline comments.
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?

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.

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.

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.

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

That would be excellent.

lutz_donnerhacke.de edited the test plan for this revision. (Show Details)

Rebase to r357716

This revision now requires review to proceed.Feb 10 2020, 7:24 AM

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

Checking for enabled dynamicrouting services.

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)
libexec/rc/rc.d/routing
341 ↗(On Diff #68054)

Can we just change this to "NO" ?

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)

libexec/rc/rc.d/routing
341 ↗(On Diff #68054)

I propose removing anything related to routed.

lutz_donnerhacke.de added inline comments.
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.

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.

lutz_donnerhacke.de marked an inline comment as done.

considering routed as a common case, not a special handling

This revision is now accepted and ready to land.Feb 12 2020, 8:20 PM

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

...

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

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

hrs requested changes to this revision.Feb 23 2020, 10:22 PM
hrs added a subscriber: hrs.
hrs added inline comments.
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.

This revision now requires changes to proceed.Feb 23 2020, 10:22 PM

Switch to a more effienct processing of rc.files

lutz_donnerhacke.de added inline comments.
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

lutz_donnerhacke.de marked an inline comment as done.

Declare temporary variable as local.

lutz_donnerhacke.de added inline comments.
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.

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".

lutz_donnerhacke.de added inline comments.
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".

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

Change to the correct idiom for enabled state.

I'm going to commit this tomorrow.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 23 2020, 3:27 PM
This revision was automatically updated to reflect the committed changes.