Page MenuHomeFreeBSD
Differential D14407

Introduce cap_fileargs.
ClosedPublic
Actions

Authored by oshogbo on Feb 17 2018, 12:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 31, 7:36 AM
Unknown Object (File)
Tue, Dec 31, 1:00 AM
Unknown Object (File)
Fri, Dec 27, 8:43 PM
Unknown Object (File)
Sat, Dec 7, 2:44 PM
Unknown Object (File)
Thu, Dec 5, 6:14 AM
Unknown Object (File)
Nov 13 2024, 11:30 PM
Unknown Object (File)
Nov 8 2024, 5:16 PM
Unknown Object (File)
Oct 20 2024, 4:53 PM
View All Files
Subscribers
araujo
bdrewery
bkidney_briankidney.ca
imp
kalle.carlbark_freebsd_kcbark.net
kevans

Details

Reviewers
cem
allanjude
emaste
bapt
jonathan
Group Reviewers
manpages
Commits
rS340373: libcasper: introduce cap_fileargs service
Summary

Casper service for handle files passed over argv/argc.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

oshogbo created this revision.Feb 17 2018, 12:27 PM
Herald added a subscriber: bdrewery. · View Herald TranscriptFeb 17 2018, 12:27 PM
oshogbo added a reviewer: bapt.Feb 17 2018, 12:29 PM
emaste added inline comments.Feb 17 2018, 12:59 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.c
159–160

I wonder if it makes sense to allow empty args as a trivial case, which could slightly simplify cap_fileargs use?

oshogbo updated this revision to Diff 39588.Feb 21 2018, 9:58 PM

We handle now empty argc and argv as @emaste suggested.

emaste added a comment.Feb 22 2018, 12:12 AM

I think this looks good. (Note that I haven't yet reviewed in detail.)

I wonder how hard it would be to extend / provide an alternate API allowing per-file flags/modes? Allowing us to support something like pdftk with a commandline such as pdftk in1.pdf in2.pdf cat output out1.pdf. It would require application-specific code to parse all arguments in advance, but would be quite convenient to create a fileargs context with a readable in1.pdf and in2.pdf and writable out1.pdf.

I noticed that we're missing manpages for the casper services and submitted PR 226102 to track that; we should add a manpage for cap_fileargs too. I can try to help create it.

oshogbo added a comment.Feb 23 2018, 2:46 PM

So for know the idea is to just create two or three whatever fileargs instance as you need with difference rights.
If we want that in one service the question is do we need it in fileargs or in filesystem one?

emaste added a comment.Feb 23 2018, 3:25 PM

See D14408 and D14409 for example use case

emaste added a comment.Feb 23 2018, 4:46 PM
In D14407#303719, @oshogbo wrote:

So for know the idea is to just create two or three whatever fileargs instance as you need with difference rights.

Ok. IMO then we should keep this as is, and just note in the man page that the API may be changed or modified in the future.

emaste added a subscriber: kevans.Feb 26 2018, 2:43 AM

@cem, @kevans: I think hexdump is probably a good candidate for the next program to apply fileargs, as we've currently implemented enter-capability-mode-on-last-argument.

I've applied this patch (and head, wc) to my WIP tree for test and further review.

lib/libcasper/services/cap_fileargs/cap_fileargs.h
48

Perhaps the stubs deserve a comment?

emaste added a comment.Feb 26 2018, 3:26 AM

Other than a couple of small comments I think this is good to go.

lib/libcasper/services/cap_fileargs/Makefile
10

Consider using 1 as with other casper services? (I think users may see a lone .so.0 and assume it is outdated.)

lib/libcasper/services/cap_fileargs/cap_fileargs.h
3

Ought to add a SPDX-License-Identifier: BSD-2-Clause-FreeBSD (and to the .c)

oshogbo updated this revision to Diff 39765.Feb 26 2018, 7:06 PM
oshogbo marked an inline comment as done.

Fortuneteller I'm very poor in writing man pages but here my try!

I also updated most of the request from @emaste .
And fix one mock.

oshogbo marked 2 inline comments as done.Feb 26 2018, 7:07 PM
oshogbo added inline comments.
lib/libcasper/services/cap_fileargs/cap_fileargs.h
48

Not sure what kind of comment you thinking about?

emaste added inline comments.Feb 27 2018, 9:18 PM
lib/libcasper/services/cap_fileargs/cap_fileargs.h
84
In file included from /scratch/tmp/emaste/freebsd/usr.bin/head/head.c:60:
/scratch/tmp/emaste/obj/scratch/tmp/emaste/freebsd/amd64.amd64/tmp/usr/include/casper/cap_fileargs.h:82:6: error: implicit declaration of function 'dnvlist_get_number' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
            dnvlist_get_number(limits, "mode", 0));
            ^
emaste added a comment.Feb 27 2018, 9:30 PM

With a trivial patch to cap_fileargs.h to fix the build and with D14409 applied, head fails like so:

% make WITHOUT_CASPER=yes -C usr.bin/head
...
% usr.bin/head/obj/head /tmp/foo
head: /tmp/foo: Not permitted in capability mode
emaste added a subscriber: bkidney_briankidney.ca.Mar 9 2018, 3:45 PM
bapt added a comment.Mar 21 2018, 5:04 PM

do we have any measurement on the performance penalty?

oshogbo added a comment.Mar 21 2018, 8:03 PM

@bapt
Yes:
https://people.freebsd.org/~oshogbo/fa_perf.txt
wcc - is capsicumized version

araujo added a subscriber: araujo.Apr 4 2018, 5:39 AM
oshogbo added a reviewer: jonathan.Apr 6 2018, 5:38 PM
oshogbo updated this revision to Diff 43157.May 30 2018, 6:34 PM

Fix compilation without Casper.

jonathan added inline comments.Jun 6 2018, 10:47 PM
lib/libcasper/services/Makefile
12

I don't see a tests directory in the review... am I just missing it?

lib/libcasper/services/cap_fileargs/cap_fileargs.3
102

Rather than providing two separate functions, might it make sense to have one fileargs_init function that takes an optionally-NULL argument?

lib/libcasper/services/cap_fileargs/cap_fileargs.h
48

It feels a bit odd to me that the user of this interface is required to know the details of the implementation, e.g., whether fileargs or an nvlist are used to store the descriptors. Couldn't this be hidden behind an opaque type with the nv-or-plain-structure choice as an implementation detail?

oshogbo added inline comments.Jun 9 2018, 5:36 PM
lib/libcasper/services/Makefile
12

This is a service Makefile:
https://svnweb.freebsd.org/base/head/lib/libcasper/services/

lib/libcasper/services/cap_fileargs/cap_fileargs.3
102

I open for such change.
When I was creating this I was thinking that this will allow us to mitigate error when the creation of casper process will fail and somebody will pass NULL by mistake to this function.

But I don't have strong opinion about that.

lib/libcasper/services/cap_fileargs/cap_fileargs.h
48

This is optional function only for optimization purpose. Normally you would use fileargs_init, fileargs_initnv I found usefully when I need to pares somehow argc/argv and I don't want to create extra buffer and copy memory etc... I left all that operation to be handled by nvlist.

araujo added a comment.Oct 26 2018, 4:13 AM

Any news about this patch? I need it to be able to finish this review: https://reviews.freebsd.org/D12419

Best,

kalle.carlbark_freebsd_kcbark.net added a subscriber: kalle.carlbark_freebsd_kcbark.net.Oct 27 2018, 11:25 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 12 2018, 5:41 PM
Closed by commit rS340373: libcasper: introduce cap_fileargs service (authored by oshogbo). · Explain Why
This revision was automatically updated to reflect the committed changes.
Herald added a reviewer: manpages. · View Herald TranscriptNov 12 2018, 5:41 PM
Herald added a subscriber: imp. · View Herald Transcript

Revision Contents
Changeset List

PathSize
lib/
libcasper/
services/
1 line
cap_fileargs/
35 lines
105 lines
241 lines
502 lines
share/
mk/
2 lines

Diff 39765

View Options

lib/libcasper/services/Makefile

Loading...
View Options

lib/libcasper/services/cap_fileargs/Makefile

Loading...
View Options

lib/libcasper/services/cap_fileargs/cap_fileargs.h

Loading...
View Options

lib/libcasper/services/cap_fileargs/cap_fileargs.3

Loading...
View Options

lib/libcasper/services/cap_fileargs/cap_fileargs.c

Loading...
View Options

share/mk/src.libnames.mk

Loading...