Disable ICMP (v4) redirects by default
Needs ReviewPublic
Actions

Authored by emaste on May 6 2024, 3:14 PM.

Details

Reviewers

philip
gordon
melifaro
donner

Summary

Relnotes: Yes

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

emaste created this revision.May 6 2024, 3:14 PM

Herald added subscribers: glebius, melifaro. · View Herald TranscriptMay 6 2024, 3:14 PM

emaste requested review of this revision.May 6 2024, 3:14 PM

Based on discussion on a recent secteam call. After putting this together I discovered D23329, which provides an rc.conf setting defaulting to AUTO which is set to yes (drop) if a routing daemon is enabled, and no if not - so if we do want to make this change we'll want to update rc.d/routing as well.

CC @donner and @melifaro.

I agree that this default is a long due to be changed. Needs to be mentioned in Release Notes, though.

switch userland default as well

emaste added inline comments.May 6 2024, 7:43 PM

libexec/rc/rc.d/routing
341	I don't think there's an issue with just changing the var itself from AUTO to YES (i.e., avoiding the underscore-prefixed dance)

emaste added reviewers: melifaro, donner.May 6 2024, 7:44 PM

emaste added a subscriber: network.May 6 2024, 11:13 PM

ceri added a subscriber: ceri.May 7 2024, 7:41 AM

ceri added inline comments.

libexec/rc/rc.d/routing
340	Is the \|”” still appropriate? It seems to have the wrong default behaviour if this is somehow unset.

emaste added inline comments.May 7 2024, 12:29 PM

libexec/rc/rc.d/routing
340	Default is now yes, so choosing yes if unset seems appropriate?

ceri added inline comments.May 7 2024, 12:53 PM

libexec/rc/rc.d/routing
340	A very fine point; was reading backwards.

rgrimes added a subscriber: rgrimes.May 7 2024, 3:02 PM

rgrimes added inline comments.

libexec/rc/rc.d/routing
341	For systems doing "updates" just switching auto to yes may break some installations.

gordon added inline comments.May 7 2024, 5:08 PM

libexec/rc/rc.d/routing
341	That's kind of the point here. At some point we have to break some eggs. We will doing this for 15 and not intending to MFC. It will be a release note item.
346	Should we print this on the yes case given it is now the default?

emaste added inline comments.May 7 2024, 5:33 PM

libexec/rc/rc.d/routing
346	Hrm, good question. This is one of the unfortunate side effects of negative-sense sysctls; we print a message in all of the "= 1" cases so there's some argument for keeping that for consistency. We could instead print `ignore ICMP redirect=NO` in the no case I suppose.

We probably need a similar change for IPv6 redirects.

emaste added inline comments.Jun 13 2024, 1:22 AM

libexec/rc/rc.d/routing
341	Also note that redirects are a performance optimization, if a system changes to yes after upgrade it won't "break" in the sense of network unreachability.