Page MenuHomeFreeBSD

Introduce cap_net a network service for Casper.
Needs ReviewPublic

Authored by oshogbo on May 4 2020, 7:21 PM.

Details

Reviewers
emaste
markj
bcr

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32144
Build 29655: arc lint + arc unit

Event Timeline

oshogbo created this revision.May 4 2020, 7:21 PM
oshogbo requested review of this revision.May 4 2020, 7:21 PM
oshogbo added a reviewer: bcr.May 5 2020, 8:46 AM
bcr added a comment.May 5 2020, 9:05 AM

Man page comments

lib/libcasper/services/cap_net/cap_net.3
166

s/to set/to a set/

171

s/to do bind/to bind/

176

s/to do connect/to connect/

217

s/previews/preview/

222

s/an host/a host/

greg_unrelenting.technology added inline comments.
lib/libcasper/services/cap_net/cap_net.3
123

s/revers/reverse/ (here and below)

also these shouldn't begin with "the"

150

It's not clear what "those specific structures" means.

Are we only allowing specific addresses to be looked up? Why does the signature take a pointer to a single sockaddr? Are we supposed to call this multiple times, one for each allowed address?

(same applies to other similar functions)

lib/libcasper/services/cap_net/cap_net.h
84

s/conenct/connect/

gbe added a subscriber: gbe.May 10 2020, 12:10 PM
oshogbo updated this revision to Diff 71650.May 11 2020, 8:27 PM
oshogbo marked 7 inline comments as done.

Man pages fixes.

lib/libcasper/services/cap_net/cap_net.3
150

Yes there is not about that

Multiple calls to
.Fn cap_net_limit_addr2name_family,
.Fn cap_net_limit_addr2name,
.Fn cap_net_limit_name2addr_family,
.Fn cap_net_limit_name2addr,
.Fn cap_net_limit_conenct,
and
.Fn cap_net_limit_bind
is supported, each call is extending previews capabilities.
bcr accepted this revision.May 13 2020, 7:13 AM

Looks good on the man page side of things, after removing the one extra "the" I found. Thanks for writing the man page!

lib/libcasper/services/cap_dns/cap_dns.3
61

You can remove the "the" here.

This revision is now accepted and ready to land.May 13 2020, 7:13 AM

I hadn't noticed this when I created D24832. Could you add that to cap_net instead?

emaste added inline comments.May 29 2020, 2:27 AM
lib/libcasper/services/cap_dns/cap_dns.3
61

Should we make it "is obsolete and will be removed in .Fx 14"?

lib/libcasper/services/cap_net/cap_net.3
247

igor doesn't like the contraction

emaste added inline comments.May 29 2020, 2:56 AM
lib/libcasper/services/cap_net/cap_net.3
94

s/cep/cap/

emaste added inline comments.May 29 2020, 3:05 AM
lib/libcasper/services/cap_net/cap_net.3
111–112

"small number of the network namespace" doesn't quite make sense - you might say "small portion of the network namespace" or similar.

153

limit*s* the
and similarly elsewhere

However I'd be also happy enough to just do a pass over the man page once it's in the tree for these

208

need a space before the .
it currently renders as may be freed using cap_net_free.()
similarly below

219

Should we have a cap_enter() in the example to clearly distinguish what runs in the sandbox vs what sets up the service?

markj added a comment.Tue, Jun 9, 2:36 PM

I don't quite understand why cap_dns is merged into this service. Aren't they mostly orthogonal?

lib/libcasper/services/cap_net/cap_net.3
250

Missing period.

lib/libcasper/services/cap_net/cap_net.h
117

Style: opening brace should be on its own line, there should be newlines between functions.

145

Why are they obsolete?

Oh sorry @emaste I some how didn't get the emails from your comments. I will address them ASAP.

markj added inline comments.Sun, Jun 28, 7:04 PM
lib/libcasper/services/cap_net/cap_net.3
3

We should avoid adding "All rights reserved" to new copyrights.

150

Typo, "reverse"

lib/libcasper/services/cap_net/cap_net.c
319

If addrinfo_unpack() returns NULL, we don't return EAI_MEMORY here.

322

If prevai == NULL then it must be true that firstai == NULL, so the condition is redundant.

859

Mixing GAI error values with errno values.

1013

Why do we dup the socket?

1046

IMO it would be a bit cleaner to handle salimits == NULL in net_allowed_bsaddr_impl().

1296

Shouldn't it be an errno value? Ditto above.

1340

I think we should start defining constants for the command strings. Otherwise the compiler can't catch typos.

1351

This file is inconsistent in the way that it braces single-line statements. net_command(), below, does not have any.

1377

Remove what?

lib/libcasper/services/cap_net/cap_net.h
40

Extra newline.

58

Maybe add /* Capability functions. */ to complement /* Limit functions. */ below.

107

Hmm, I guess this ensures that we don't trigger error handling in the caller? We could provide a static non-NULL pointer instead, like (void *)(uintptr_t)0x1.

cap_sysctl has the same problem.

145

Mariusz replied on IRC, this just reflects the fact that getaddrinfo() and getnameinfo() are preferred over gethost*() per the man page for these functions.

emaste added inline comments.Fri, Jul 3, 3:14 PM
lib/libcasper/services/cap_net/cap_net.3
250

Difficult to figure out what to do with a period after a domain name, in plain text :)
Maybe a space after .com before the period?

lib/libcasper/services/cap_net/cap_net.h
145

Probably "deprecated" is better

oshogbo updated this revision to Diff 74092.Sun, Jul 5, 10:22 AM
oshogbo marked 10 inline comments as done.

Update after emaste, markj and bcr review.

This revision now requires review to proceed.Sun, Jul 5, 10:22 AM
oshogbo added inline comments.Sun, Jul 5, 10:22 AM
lib/libcasper/services/cap_dns/cap_dns.3
61

Thats a good question I'm not sure.
From one hand yes we would like to on the other hand there may be some consumers of it.

lib/libcasper/services/cap_net/cap_net.3
153

Thank you!!!

219

There is caph_enter_casper which is cap_enter which check if the casper is buildin.

247

Fixded.

250

I added space.

lib/libcasper/services/cap_net/cap_net.c
319

Nice catch.

322

Right.

1013

We don't have to we can just take it from nvlist.

1046

We can't do that. Here if the capdnscache is NULL we return ENOTCAPABLE, but in case of the net_allowed_bsaddr which is used much offten if the right are not defined it means that we allow all bsaddr.

1296

Not really sure what to do with that ;(

1340

Thats a good idea. I started with the limits name for know.

1351

Style fixed.

1377

The flags after the development. The CASPER_SERVICE_FD nor the CASPER_SERVICE_STDIO in not needed.

lib/libcasper/services/cap_net/cap_net.h
107

Yes. As well as we ensure working with the proper descriptor. For example in fileargs we are doing much more checks.