Too many changes needed to add a simple option

[rhabacker]

When trying to add support for the option mentioned at #92, it turned out that 26 adjustments would be needed in various places.

cd ~/src/yast-security
grep -rn net.ipv4.ip_forward | wc -l
26

Since security settings of a system have increasing importance, adding options should be as simple as possible and require changes only in a few places, e.g. in a central configuration file.

If parts of the module cannot process this file directly, a script would be required to make necessary adjustments and/or additions to source code or configuration files.

[ancorgs]

26 sounds like a lot! Let's see:

• One occurrence is a changelog entry. That of course should stay and is irrelevant for the topic.
• Eight of them are spread all over the Security.rb file for different purposes like mapping it to old values for backward compatibility, importing from an AutoYaST profile, exporting to AutoYaST, resetting the associated services. All those occurrences should indeed be grouped by setting in a more sensible way.
• Three of them correspond to the UI. Maybe they could be grouped with the ones mentioned above but I'm not sure. Probably having this three occurrences in the UI is not that bad if it leads to an understandable UI.
• Three of them correspond to the three security levels defined in /data/security. That is not specially bad, although it would be good if the three levels could share the definition of the common parts (the property is set to "false" in the three levels).
• Two of then correspond to the security.rnc schema. Those should stay.
• Eight of them correspond to the testsuite. They should likely stay as well.

So yes, this is pretty bad... but not as dramatic as the initial 26 sounded. :-)

I see we have a GSoC project for this. Looking forward the results!
openSUSE/mentoring#167

Ping me if you need help with the mentoring.

[ancorgs]

Marking this as "tracked" because of openSUSE/mentoring#167

[rhabacker]

Ping me if you need help with the mentoring.

@ancorgs: In the last years it was a requirement from Google that at least 2 mentors are involved in a project (last year even 3 because of Corona) - you would sign up as 2nd mentor ?

[rhabacker]

I see we have a GSoC project for this. Looking forward the results! openSUSE/mentoring#167

Unfortunately, no student took on this task last year.

[rhabacker]

26 sounds like a lot! Let's see:

I'll list the relevant places using the "ip_forward" property as an example::

* One occurrence is a changelog entry. That of course should stay and is irrelevant for the topic.

package/yast2-security.changes:292:  net.ipv4.ip_forward, net.ipv4.tcp_syncookies,

* Eight of them are spread all over the `Security.rb` file for different purposes like mapping it to old values for backward compatibility, importing from an AutoYaST profile, exporting to AutoYaST, resetting the associated services. All those occurrences should indeed be grouped by setting in a more sensible way.

src/modules/Security.rb:131:        "net.ipv4.ip_forward"                       => false,
src/modules/Security.rb:200:        "net.ipv4.ip_forward"          => false,
src/modules/Security.rb:209:        "net.ipv4.ip_forward"          => "IP_FORWARD",
src/modules/Security.rb:262:        "net.ipv4.ip_forward"          => "/usr/bin/systemctl try-restart network",
src/modules/Security.rb:786:        settings["net.ipv4.ip_forward"] = settings.delete("NET.IPV4.IP_FORWARD")
src/modules/Security.rb:793:      ["net.ipv4.tcp_syncookies", "net.ipv4.ip_forward", "net.ipv6.conf.all.forwarding"].each do |key|
src/modules/Security.rb:846:      ["net.ipv4.tcp_syncookies", "net.ipv4.ip_forward", "net.ipv6.conf.all.forwarding"].each do |key|
src/modules/Security.rb:1027:      "net.ipv4.ip_forward"          => :forward_ipv4,

* Three of them correspond to the UI. Maybe they could be grouped with the ones mentioned above but I'm not sure. Probably having this three occurrences in the UI is not that bad if it leads to an understandable UI.

src/include/security/dialogs.rb:101:        "net.ipv4.ip_forward"                       => _("IPv4 forwarding"),
src/include/security/dialogs.rb:257:          "id"        => "net.ipv4.ip_forward",
src/include/security/dialogs.rb:258:          "is_secure" => !Security.Settings["net.ipv4.ip_forward"]

And there is one in the help page:

src/include/security/helps.rb:310:        "net.ipv4.ip_forward"                       => _(

* Three of them correspond to the three security levels defined in `/data/security`. That is not specially bad, although it would be good if the three levels could share the definition of the common parts (the property is set to "false" in the three levels).

src/data/security/level2.yml:36:net.ipv4.ip_forward:              false
src/data/security/level1.yml:36:net.ipv4.ip_forward:              false
src/data/security/level3.yml:36:net.ipv4.ip_forward:              false

* Two of then correspond to the `security.rnc` schema. Those should stay.

src/autoyast-rnc/security.rnc:74:net.ipv4.ip_forward = element net.ipv4.ip_forward { STRING }
src/autoyast-rnc/security.rnc:134: | net.ipv4.ip_forward

* Eight of them correspond to the testsuite. They should likely stay as well.

test/security_test.rb:274:          Security.Settings["net.ipv4.ip_forward"] = ""
test/security_test.rb:280:          Security.Settings["net.ipv4.ip_forward"] = false
test/security_test.rb:287:          Security.Settings["net.ipv4.ip_forward"] = true
test/security_test.rb:541:        Security.Settings["net.ipv4.ip_forward"]          = nil
test/security_test.rb:554:        expect(Security.Settings["net.ipv4.ip_forward"]).to eql(false)
test/security_test.rb:750:        Security.Settings["net.ipv4.ip_forward"] = true
test/security_test.rb:799:          expect(Security.Settings["net.ipv4.ip_forward"]).to eql(false)

And there is on reference in the test data

test/data/system/etc/sysctl.conf:24:net.ipv4.ip_forward = 0