Page MenuHomeFreeBSD
Differential D45102

Disable ICMP (v4) redirects by default
Needs ReviewPublic
Actions

Authored by emaste on May 6 2024, 3:14 PM.
Tags
None
Referenced Files
F99603175: D45102.diff
Fri, Oct 11, 5:45 AM
F99582095: D45102.diff
Fri, Oct 11, 12:31 AM
F99508351: D45102.diff
Thu, Oct 10, 8:08 AM
Unknown Object (File)
Tue, Oct 8, 11:44 PM
Unknown Object (File)
Tue, Oct 8, 1:04 PM
Unknown Object (File)
Mon, Oct 7, 2:43 PM
Unknown Object (File)
Sun, Oct 6, 5:05 AM
Unknown Object (File)
Thu, Oct 3, 9:00 AM
View All 50 Files
Subscribers
ceri
donner
glebius
melifaro
rgrimes
zarychtam_plan-b.pwste.edu.pl
network

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
emaste added a subscriber: donner.May 6 2024, 3:20 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.

glebius added a comment.May 6 2024, 7:06 PM

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

emaste updated this revision to Diff 138186.May 6 2024, 7:42 PM

switch userland default as well

emaste added inline comments.May 6 2024, 7:43 PM
libexec/rc/rc.d/routing
341 ↗(On Diff #138186)

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 ↗(On Diff #138186)

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 ↗(On Diff #138186)

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 ↗(On Diff #138186)

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 ↗(On Diff #138186)

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 ↗(On Diff #138186)

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 ↗(On Diff #138186)

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 ↗(On Diff #138186)

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.

zarychtam_plan-b.pwste.edu.pl added a subscriber: zarychtam_plan-b.pwste.edu.pl.May 8 2024, 2:04 PM

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 ↗(On Diff #138186)

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.

Revision Contents
Changeset List

PathSize
sys/
netinet/
2 lines

Diff 138163

View Options

sys/netinet/ip_icmp.c

Loading...