Page MenuHomeFreeBSD

Introduce cap_net a network service for Casper.
ClosedPublic

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Man page comments

lib/libcasper/services/cap_net/cap_net.3
165 ↗(On Diff #71378)

s/to set/to a set/

170 ↗(On Diff #71378)

s/to do bind/to bind/

175 ↗(On Diff #71378)

s/to do connect/to connect/

216 ↗(On Diff #71378)

s/previews/preview/

221 ↗(On Diff #71378)

s/an host/a host/

greg_unrelenting.technology added inline comments.
lib/libcasper/services/cap_net/cap_net.3
122 ↗(On Diff #71378)

s/revers/reverse/ (here and below)

also these shouldn't begin with "the"

149 ↗(On Diff #71378)

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

s/conenct/connect/

oshogbo marked 7 inline comments as done.

Man pages fixes.

lib/libcasper/services/cap_net/cap_net.3
149 ↗(On Diff #71378)

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.

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

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?

lib/libcasper/services/cap_dns/cap_dns.3
61 ↗(On Diff #71650)

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

lib/libcasper/services/cap_net/cap_net.3
246 ↗(On Diff #71650)

igor doesn't like the contraction

lib/libcasper/services/cap_net/cap_net.3
93 ↗(On Diff #71650)

s/cep/cap/

lib/libcasper/services/cap_net/cap_net.3
110–111 ↗(On Diff #71650)

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

152 ↗(On Diff #71650)

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

207 ↗(On Diff #71650)

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

218 ↗(On Diff #71650)

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

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

Missing period.

lib/libcasper/services/cap_net/cap_net.h
116 ↗(On Diff #71650)

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

144 ↗(On Diff #71650)

Why are they obsolete?

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

lib/libcasper/services/cap_net/cap_net.3
2 ↗(On Diff #71650)

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

149 ↗(On Diff #71650)

Typo, "reverse"

lib/libcasper/services/cap_net/cap_net.c
318 ↗(On Diff #71650)

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

321 ↗(On Diff #71650)

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

858 ↗(On Diff #71650)

Mixing GAI error values with errno values.

1012 ↗(On Diff #71650)

Why do we dup the socket?

1045 ↗(On Diff #71650)

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

1295 ↗(On Diff #71650)

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

1339 ↗(On Diff #71650)

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

1350 ↗(On Diff #71650)

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

1376 ↗(On Diff #71650)

Remove what?

lib/libcasper/services/cap_net/cap_net.h
39 ↗(On Diff #71650)

Extra newline.

57 ↗(On Diff #71650)

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

106 ↗(On Diff #71650)

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.

144 ↗(On Diff #71650)

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

lib/libcasper/services/cap_net/cap_net.3
249 ↗(On Diff #71650)

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

Probably "deprecated" is better

oshogbo marked 10 inline comments as done.

Update after emaste, markj and bcr review.

This revision now requires review to proceed.Jul 5 2020, 10:22 AM
lib/libcasper/services/cap_dns/cap_dns.3
61 ↗(On Diff #71650)

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

Thank you!!!

218 ↗(On Diff #71650)

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

246 ↗(On Diff #71650)

Fixded.

249 ↗(On Diff #71650)

I added space.

lib/libcasper/services/cap_net/cap_net.c
318 ↗(On Diff #71650)

Nice catch.

321 ↗(On Diff #71650)

Right.

1012 ↗(On Diff #71650)

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

1045 ↗(On Diff #71650)

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.

1295 ↗(On Diff #71650)

Not really sure what to do with that ;(

1339 ↗(On Diff #71650)

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

1350 ↗(On Diff #71650)

Style fixed.

1376 ↗(On Diff #71650)

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

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

lib/libcasper/services/cap_dns/cap_dns.3
61 ↗(On Diff #71650)

Perhaps say that it may be removed after FreeBSD 13, then? I think it would be good to give some encouragement for people to migrate.

We could handle this in later followup though (after this gets committed.)

lib/libcasper/services/cap_net/cap_net.h
160 ↗(On Diff #74092)

typo, "Deprecated"

39 ↗(On Diff #71650)

Still an extra newline here?

If we're happy with the API (aside from a couple of typos in #defines) what do you think about committing this, and iterating further in the tree?

lib/libcasper/services/cap_net/cap_net.3
125–127 ↗(On Diff #74092)

deprecated

lib/libcasper/services/cap_net/cap_net.h
51–52 ↗(On Diff #74092)

deprecated

91 ↗(On Diff #74092)

Deprecated

This revision was not accepted when it landed; it landed in state Needs Review.Aug 16 2020, 6:12 PM
This revision was automatically updated to reflect the committed changes.