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 - subversion
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
sys/security/mac_ipacl/mac_ipacl.c
333

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

335

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

339

ifdef INET

341

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

345

} else { in one line; here and everywhere else

347

indent

351

ifdef INET6

353

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

359

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?

362

if (.. (add space)

367

I think the comment can go?

386

ifdef INET

391

Blank line between variable declaration and code

393

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?

404

ifdef INET6

409

blank line again

427

ifdef INET

428

ifdef INET6

sys/security/mac_ipacl/tests/Makefile
1 ↗(On Diff #59798)
  1. $FreeBSD$
shivank added inline comments.
sys/security/mac_ipacl/mac_ipacl.c
31

#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

269

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

393

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

427

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

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

Add copyright and license.
Add a man page for mac_ipacl

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
7

SRCS+= opt_inet.h opt_inet6.h

should solve your module building problem as well.

sys/netinet/in.c
380

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
5

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.

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.

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.

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
9

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

shivank marked 3 inline comments as done.
  • 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
9

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

  • move man page to its right place
sys/conf/files
3434

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

sys/security/mac_ipacl/mac_ipacl.c
9

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.

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

share/man/man4/mac_ipacl.4
89

POlicy is a typo, right?

107

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

127

Please start new sentences in a new line.

130

Missing space before (.

169

missing a space before (

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

shivank marked 4 inline comments as done.

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

  • make tests more structured with atf
  • update man page mac_ipacl.4
  • add ipacl entry in tests Makefile
  • fix minor issues in mac_ipacl.4

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
94

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

104

Again .112 is not good.

171

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

181

See above.

266

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.
  • correct the IP addresses which were not in the documentation range
This revision is now accepted and ready to land.Aug 19 2019, 1:22 PM

This appears to have been accepted but not merged - it would be great to have it get into 13.0 if there's still time

In D20967#632260, @dch wrote:

This appears to have been accepted but not merged - it would be great to have it get into 13.0 if there's still time

If that's the case, I can test this patch again. I can also devote some time to work on it, if there's suggestion from community.
What do you think @bz?