Page MenuHomeFreeBSD

new MAC policy module - mac_ipacl
AcceptedPublic

Authored by shivank on Jul 16 2019, 9:49 AM.

Details

Reviewers
bz
Group Reviewers
manpages
Summary

GSoC'19 project on creating a new MAC policy module to limit the VNET jail privileges of setting its IP address.

Test Plan

wrote test scripts(Kyua)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bz added inline comments.Jul 17 2019, 10:42 AM
sys/security/mac_ipacl/mac_ipacl.c
270

Oh, I see how this is done; interesting :-)

282

Remove blank line.

290

could be a void function?

293

ifdef INET / IENT6; please use defined constants here (INET_ADDRSTRLEN, INET6_ADDRSTRLEN)

299

This and some of the following line look long in the review system; please make sure they are wrapped at <= 80 chars

300

ifdef INET; again switch might make this easier.

306

ifdef INET6

319

4 spaces indent

334

if (... (add space; be consistent within your code as well)

336

Possibly this and a few lines below look rather long in the review system; please try to wrap them at 80 char

340

ifdef INET

342

Question: why do you do this maths for every check; rather than once when you setup the rules?
(equally for IPv6 below)

346

} else { in one line; here and everywhere else

348

indent

352

ifdef INET6

354

I assume this will move elsewhere; for (i = 0; i < 16; i++)\n ; no {} block needed

360

could you just continue immediately not needing "j"? Hmm within the loop; I see. Maybe make j a bool and call it something more descriptive? Possibly also inverting the true/false logic depending on new name?

363

if (.. (add space)

368

I think the comment can go?

387

ifdef INET

392

Blank line between variable declaration and code

394

Feature request (also equally for IPv6) for once this is all sorted if there is more time. Can we make this a sysctl policy as well defaulting to jails only, but if changed also applying to the base system?

405

ifdef INET6

410

blank line again

428

ifdef INET

429

ifdef INET6

sys/security/mac_ipacl/tests/Makefile
1 ↗(On Diff #59798)
  1. $FreeBSD$
shivank marked 43 inline comments as done.Jul 19 2019, 7:23 AM
shivank added inline comments.
sys/security/mac_ipacl/mac_ipacl.c
32

#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");

#include "opt_inet.h"
#include "opt_inet6.h"

I added the following lines, but it gave an error during compilation - opt_inet.h file not found, similarly for opt_inet6.h

270

Actually, it's implemented like this in from mac_portacl.c, I copied it from there

394

Yeah. Can I add a comment in the code regarding this?

428

Do I also need to add #ifdef ... #endif in the corresponding declaration in the framework?
Same for below comment.

shivank marked 2 inline comments as done.Jul 19 2019, 7:47 AM
shivank updated this revision to Diff 59909.Jul 19 2019, 7:52 AM

indentation and style changes,
moved subnet check code from rules_check to parser,

shivank updated this revision to Diff 59937.Jul 19 2019, 6:49 PM

Add copyright and license.
Add a man page for mac_ipacl

shivank updated this revision to Diff 60010.Jul 22 2019, 7:41 AM

add #ifdef INET and #ifdef in INET6
fix indentation and style issues
add mac_ipacl entry in kernel conf and modules Makefile

bz requested changes to this revision.Jul 22 2019, 10:36 AM

Hey, thank you for all the updates. There are a few more. Please let me know when you think you are done with all of them and I'll have a full look again.

sys/modules/mac_ipacl/Makefile
8

SRCS+= opt_inet.h opt_inet6.h

should solve your module building problem as well.

sys/netinet/in.c
381

Do whitespace /*<space>...<space>*/ still fit on 80 chars?

(also for ipv6 and the other files)

sys/security/mac_ipacl/Makefile
4 ↗(On Diff #60010)

I hadn't realised in the first pass; what is this Makefile for? Should not be needed, right? The proper one is in sys/modules/. Can you delete it?

sys/security/mac_ipacl/mac_ipacl.4
4 ↗(On Diff #60010)

I don't belong on the copyright for any of this.

187 ↗(On Diff #60010)

manpages can you please have a look and guide him? I see overlong lines at least...

sys/security/mac_ipacl/mac_ipacl.c
6

Please remove me everywhere from the (c)

This revision now requires changes to proceed.Jul 22 2019, 10:36 AM
shivank marked 19 inline comments as done.Jul 22 2019, 12:54 PM
shivank updated this revision to Diff 60018.

fix style issues
fix copyright issue

I have a few doubts:

  • I'm not clear about the license, should the TrustedBSD be included? if yes, then how? Also, I have copied the sysctl_rules from mac_portacl, Is it infringing any copyright as of now? I've read BSD license is very open, can I mention the mac_portacl?
  • should #ifdef INET/INET6 be put in mac_policy.h and mac_framework.h?
  • after adding INET/INET6 in mac_ipacl, kyua stopped working for test scripts. It gives errors as "ip4_test:main -> broken: Received signal 6 [0.033s] ip6_test:main -> broken: Received signal 6 [0.032s]" As scripts they are testing fine.
bz added a comment.Jul 22 2019, 2:11 PM

I have a few doubts:

  • I'm not clear about the license, should the TrustedBSD be included? if yes, then how? Also, I have copied the sysctl_rules from mac_portacl, Is it infringing any copyright as of now? I've read BSD license is very open, can I mention the mac_portacl?

In that case maybe take the copyright from mac_portacl for that file and extend it with your copyright; the license seems very close to the same so it's not an issue and the attributions and previous copyrights can stay. That way you acknowledge the ownership of the original work and give you added a significant junk to the file and changed it, you add your own (c). That's what I'd do.

  • should #ifdef INET/INET6 be put in mac_policy.h and mac_framework.h?

no

  • after adding INET/INET6 in mac_ipacl, kyua stopped working for test scripts. It gives errors as "ip4_test:main -> broken: Received signal 6 [0.033s] ip6_test:main -> broken: Received signal 6 [0.032s]" As scripts they are testing fine.

That mostly means that something is not properly handled; I'd suggest checking dmesg -a and see which command crashed; it might be listed there.

bz added a comment.Jul 24 2019, 1:33 PM

Grat work on the cleanup; I think apart from the license there is very few minor nits left which are acceptable.

I left you a suggestion to check for the man page.

Did you find out what causes the crashes of the tests? I'd start looking into them with you next.

sys/security/mac_ipacl/mac_ipacl.4
187 ↗(On Diff #60010)

You could also try igor: https://svnweb.freebsd.org/ports/head/textproc/igor/ and see what it says and fix accordingly.

179 ↗(On Diff #60018)

I don't think you need to list anything but the two mac man pages in yours.

sys/security/mac_ipacl/mac_ipacl.c
10

You might still want to bring the copyrights/attributions/license over from the other mac module.

shivank marked 3 inline comments as done.Jul 25 2019, 1:52 PM
shivank updated this revision to Diff 60135.
  • Correct the license file for mac_ipacl.c and mac_ipacl.4
  • fix Kyua for test shell scripts
  • fix errors in mac_ipacl.4 man page
shivank marked 2 inline comments as not done.Jul 25 2019, 1:56 PM
shivank added inline comments.
sys/security/mac_ipacl/mac_ipacl.4
187 ↗(On Diff #60010)

Thanks for this tool. It helped remove very silly errors from the page.
I have changed the EXAMPLES part a little, please look at it again. In 3rd example, the rules="..." is a long line, so it extends to the whole screen, making it look odd.

sys/security/mac_ipacl/mac_ipacl.c
10

I have added (c) from mac_portacl,
License is the same in all modules,
Please suggest attributions that should be taken.

shivank updated this revision to Diff 60215.Sun, Jul 28, 7:01 PM
  • move man page to its right place
bz added inline comments.Tue, Jul 30, 10:59 PM
sys/conf/files
3434

This seems to have sneaked in; probably a merge error when updating sources?

sys/security/mac_ipacl/mac_ipacl.c
10

All the ones which were on the file where you "copied" the code from. Think of initially copying another file to a new name and then adjusting the code and adding your copyright.

0mp added a subscriber: 0mp.Wed, Jul 31, 3:42 PM

You may also want to run mandoc -Tlint apart from igor. :)

share/man/man4/mac_ipacl.4
90

POlicy is a typo, right?

108

.Ar is not needed as macros are not expanded here.

128

Please start new sentences in a new line.

131

Missing space before (.

170

missing a space before (

shivank added a comment.EditedWed, Jul 31, 6:54 PM

Hi, @0mp thanks for the suggestion :).

shivank marked 4 inline comments as done.Wed, Jul 31, 7:17 PM
shivank updated this revision to Diff 60330.

fix errors shown by mandoc -Tlint for mac_ipacl.4
fix the license and copyrights

shivank updated this revision to Diff 60418.Sat, Aug 3, 9:29 AM
  • make tests more structured with atf
  • update man page mac_ipacl.4
shivank updated this revision to Diff 60419.Sat, Aug 3, 9:43 AM
shivank updated this revision to Diff 60510.Tue, Aug 6, 4:25 PM
  • add ipacl entry in tests Makefile
  • fix minor issues in mac_ipacl.4
bz added a comment.Fri, Aug 9, 8:53 AM

There's a couple of public IP(v6) addresses in the test scripts. We'd prefer not to have accidents with people. Can you please change them?

tests/sys/mac/ipacl/ipacl_test.sh
95

That .112 is not a TEST-NET but a public assigned address of someone.

105

Again .112 is not good.

172

Is that intended? 1:470:... doesn't look like an appropriate address.

182

See above.

267

This also is not a good address to use in testing. It's a public APNIC address (possibly unassigned currently).

shivank marked 3 inline comments as done.Fri, Aug 9, 5:42 PM
shivank updated this revision to Diff 60617.
  • correct the IP addresses which were not in the documentation range
shivank marked 5 inline comments as done.Fri, Aug 9, 5:44 PM
shivank updated this revision to Diff 60618.Fri, Aug 9, 6:54 PM
bz accepted this revision.Mon, Aug 19, 1:22 PM
This revision is now accepted and ready to land.Mon, Aug 19, 1:22 PM