Add draft proposal for package hardening

[samoht]

I would like to start the discussion about improving the general quality of MirageOS packages. Ideally we should have a tool to help us do that (something like mirage lint) and/or a dashboard as @hannesm suggested a few time already.

I know some of these criteria are very opinionated but I would like to get a sense of what other people in the community is thinking, so please feel free to disagree or to suggest better criteria :-)

[hannesm]

@samoht thanks for writing this up. I'm not sure whether hardening is the right word.

I don't understand why/what "by vendoring opam metadata" means. reproducibility of a MirageOS unikernel should depend only on a) dependencies (which are embedded into it, including version number) b) ld and C compiler c) OCaml compiler.

I'm also unsure what you mean by "have a way to gather licenses" -- you can do this in a (more or less) generic way using opam metadata, can't you?

what I'm missing from the list:

• have a design document specifying the scope of the library (and esp. what is out of scope)
• doesn't expose any exceptions in the public interface
• avoid integer overflows (basically every addition and subtraction, as well as multiplication needs to be guarded unless you know that an overflow can never happen (in this case, a comment should be suggested))
• works on 32bit (esp. in regards to the last point)?
• maintainer: GitHub recently introduced code owners, which may be worth to use for some (of our core) libraries (see https://github.com/blog/2392-introducing-code-owners)
• safe-string (looks like this will be the default in 4.06) switch to safe-string by default (related to MPR#7113) ocaml/ocaml#1252
• provide pretty printers (e.g. by using fmt)
• maybe some best practises: use result type (and rresult combinators)! use logs for logging, astring for string processing, bos for operating system interaction, and fpath for file paths (which unfortunately still is not used by mirage-block/fs)
• follow OCaml programming guidelines https://ocaml.org/learn/tutorials/guidelines.html

EDIT: needed to add some more notes:

• mutable state -- best be avoided
• non-tail recursive functions -- what about termination?
• preprocessors -- use with care (and esp. avoid camlp4)
• some linting tools for OCaml used to be around: http://www.erights.org/download/emily/index.html (thx @cfcs) https://caml.inria.fr/pub/old_caml_site/ocamlexc/ocamlexc.htm (thx @dbuenzli) https://github.com/cryptosense/ocamllint/
• when to use polymorphic equality and when to define your own

[djs55]

I like both @samoht and @hannesm's lists.

Regarding "have a way to gather licenses" it's useful to be able to get the license file itself because it will often have text like

Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following disclaimer in the documentation and/or other materials provided with the distribution.

so if you want to send someone a binary unikernel you need to also include the copyright notice i.e. the copyright holders as distinct from the authors or the maintainers (sometimes they're the same but sometimes they're not) and a bunch of other stuff which isn't readily available from the opam file.

[dbuenzli]

Regarding "have a way to gather licenses" it's useful to be able to get the license file itself because it will often have text like

This will do it:

opam show -f depends: ./opam | cut -f 1 -d ' ' | xargs odig license --no-pager --warn-error

See odig help license for the details.

[cfcs]

• better linting of dependencies would definitely be nice (I've quite often experienced mirage-related packages that didn't properly specify all of their dependencies).

• perhaps a release tool that requires the author to install the pkg in their own opam (could be as switch) to prevent yolo-maintainers from pushing broken commits.

• I would suggest banning int and float entirely. I don't see the reason not to use Int32/Int64/Nativeint instead (or @hannesm's usane), but if someone can argue for the existence of the int type I'd be eager to hear their take on this.

• making package signing mandatory (once https://github.com/hannesm/conex/ takes off) would also be a good addition to the security of the mirage ecosystem.

• re: jbuilder: I'm a bit skeptical about making jbuilder a requirement, since it still seems a bit unstable (or perhaps people are still learning how to use it) -- I just noticed that most of the time where I run into packages that don't build, it's because of jbuilder. I wouldn't mind if this became a general guideline sometime in the future, but for now I'd be sad to have other build systems outlawed.

[dbuenzli]

but if someone can argue for the existence of the int type I'd be eager to hear their take on this.

Indexing arrays maybe ?

[hannesm]

@dbuenzli @cfcs indexing arrays is one use case, another is string or cstruct length operations and arithmetic (e.g. slicing a DNS frame into pieces, and keeping track of offsets... this will never be > 512 (or 2 ^ 16 - 1 in the TCP case)). if you receive a 16bit (8bit/24bit) value from a wire protocol, it is very safe to use int for that (unless you allow clients to fill an int without appropriate checking (since Cstruct wraps (using e.g. set_uint16) 0xFF00FF to 0x00FF)).

[cfcs]

@dbuenzli @hannesm I believe that the native array and string indexing using int is a messy thing leftover from when int was not platform-dependent.
Basically I think int should have stayed 31-bits (preferably named int31, but oh well), and that int63 should have been added as a type. Consequently using it in Cstruct is imho also a design mistake.

The wrapping of(aka silent overflowing/underflowing of) ints is exactly what we want to [EDIT: prevent], so ideally we'd have a functor that defined an integer field, and use operations on that field.

@hannesm if you receive a 8-bit length field (0x00) and subtract one (because the length field was supposed to include the length of the packet) you will get -1, and depending on how you use that value, you'd probably want to have used an unsigned integer type and be alerted to the underflow possibility at compile-time, or when parsing the input, rather than sometime later during the execution.

I'm aware of the fact that it's sometimes a bit awkward to not use the basic types, and I still hope we can find a way to make it less awkward. (some kind of polymorphic arithmetic operations like val (+) : ('A).t -> ('A).t -> ('A).t arithmetic_result - pseudo-code since I don't know how to express that in ocaml)

[samoht]

Thanks all for your feedback!

I think we should aim to make that list actionable in the short term -- so the discussion about switching everything to not using int seems a bit unrealistic for now on (we still have libraries without test/doc, and we just started to add fuzz testing to some of the core libs). I agree that aiming for 32 bit support is great, especially if we want to compile things to Javascript. Any idea how to do this? Maybe we should have a special compiler switch set with 32bits that we could use with TravisCI?

Thanks @dbuenzli for the license tip, that's exactly what we need. @hannesm: for the reproducible bit, we started experimenting with having more control over the dependencies which are used to build a released binary (using things similar to lock files): https://github.com/moby/vpnkit/tree/master/repo so that we have a stronger control over the opam universe used in the CI.

@hannesm @cfcs: thanks for your lists. I will try to merge all of this into something consistent :-)

[mor1]

...aspects...

[mor1]

...filesystems.

[mor1]

...multiple hypervisors including Xen and (via solo5) KVM.

[mor1]

developing

[mor1]

...but things are getting better!

[mor1]

hardening

[mor1]

do you mean opam rather than ocaml here?

[mor1]

spanning rather than spawning?

[mor1]

columns (or combine this bullet with previous, and host a standard ocp-indent file somewhere?)

[hannesm]

On 21/07/2017 14:37, Thomas Gazagnaire wrote: I think we should aim to make that list actionable in the short term -- so the discussion about switching everything to not using `int` seems a bit unrealistic for now on (we still have libraries without test/doc, and we just started to add fuzz testing to some of the core libs). I agree that aiming for 32 bit support is great, especially if we want to compile things to Javascript. Any idea how to do this? Maybe we should have a special compiler switch set with 32bits that we could use with TravisCI?

see also mirage/mirage#614 for further discussion (I think that was planned before the MirageOS 3 release)... I suspect commits such as https://github.com/mirage/irmin/pull/458/files#diff-0b43dc6466ffe5ba4e4629f71c33631eR707 (and https://github.com/mirage/irmin/pull/458/files#diff-0b43dc6466ffe5ba4e4629f71c33631eR776) break 32bit support (and esp in there I don't understand why there's need for an int type - it'll break if you switch platforms)...

[avsm]

@samoht if you make a list of what CI modes would be useful (e.g. afl, 32-bit, ...) I can add those explicitly into mirage-ci

[hannesm]

as mort mentioned on mirageos-devel, hardening has security connotations, whereas this is more about 'towards high-quality packages' (and not only about security)

[hannesm]

the current PR looks more like a laundry list - I agree with mort that pointing to some examples of "good libraries" would be good

and another item would be "semantic versioning" -- although it seems there's no proper definition / understanding inside the MirageOS community (so maybe we should not mention it?)

[samoht]

Yup completely agree about the laundry list, I feel the same. Any opinion on how to organise that better?

Maybe we should split this into "phases" or "levels", e.g:

1. the 'boring paperwork' tasks to complete to make the library compatible with odig/topkg
2. then some basic coding style 'sanity'
3. how to go further towards quality (fuzz testing, discussion about int overflow, etc)
4. bonus: formal verification, etc

(yes, I quite enjoyed the capnproto protocol levels)

And maybe have badges for each levels to put in the README.

Also I will replace "hardening" by something related to "quality".

For the examples of good library, please submit an example or two :-) I am not aware of any library today which fulfils all the basic items on that list ...

[mor1]

That structure into levels LGTM.
How do any of those tasks relate to https://mirage.io/wiki/packaging ?
I don't know of examples of existing libraries that meet all criteria :)
But if restructured as levels, perhaps worth assigning levels to the core libs at least, and we should file relevant issues needed to bring them up to higher levels / next level?
(A good hackathon / pioneer project perhaps?)

[yomimono]

I think this is a great idea for all projects, but the tooling really isn't ready for this to be included yet in a document people might be trying to adhere to. "Have randomized property-based testing" is more achievable and will give people the right foundation to use tools like Crowbar later, once they're more ready to be integrated in a CI pipeline.

[yomimono]

The coding style items are all good goals, but they're at varying levels of subjectivity and apparent-ness (sorry). "avoid potential overflows" and "don't surface exceptions" are testable and concrete; items that begin with "be aware of" are less so. We should either come up with concrete ways in which a library surfaces these qualities, or note that they are goals somewhere rather than specific properties that can be possessed or not possessed.

[samoht]

Pushed a tentative list of levels ... not really sure where this is going and indeed some of the item will be hard to verify automatically, at least in the short term.

[hannesm]

Crucial is to define the behaviour on edge cases (see e.g. the discussion about mirage-console being blocking on Xen vs partial write on Solo5 mirage/mirage-flow#4), and whether and how arguments are checked (e.g. values given to set* in cstruct are not checked, see mirage/ocaml-cstruct#78)

[hannesm]

what does this mean?

[hannesm]

this is intertwined with the previous use alcotest... if you use alcotest, you likely already defined pretty printers

[hannesm]

maybe elaborate a bit with links how difficult it is to get C bindings cross compiled atm!? and indicate that MirageOS does not include a C library, but only very few functions which are required for the OCaml runtime (see https://github.com/mirage/ocaml-freestanding/tree/master/nolibc)

[dinosaure]

We definitely should add a documentation. What I'm afraid with MirageOS 4 is the ability to compile C stubs for the Unix target (with the libc according to the dune context) and be blocked when we want to switch the whole codebase to a new target such as ocaml-freestanding/Solo5.

Currently, mirage-crypto or digestif try to compile properly C stubs with ocaml-freestanding flags (including nolibc) but such test (just to check that these C stubs can be cross-compiled with ocaml-freestanding) is not done by mirage and nothing (as far as I can say) can prevent a bad C stubs for all targets.

[hannesm]

-> in this case a comment is required. and add "you can test this by passing max_int and min_int in your tests for all int which occur"

[yomimono]

I'd like to revisit this, with the goal of making it into a concrete list of things to achieve by interested newcomers, applicable to at least three repositories, by the start of the next Mirage hack retreat on 28 November.

[hannesm]

@yomimono sounds great, given the release of ocamlformat, I'd suggest to jump on the train and reindent all the libraries, and deploy pre-commit hooks to never accept any code which ocamlformat disagrees with.

[cfcs]

@hannesm can we please have some fair warning before anything like that happens - reformatting everything and breaking everyone's local patches sounds to be me like a pretty high cost to pay to get normalized whitespace.

[avsm]

I agree with @cfcs -- can we get the current guidelines in place without creating new ones? I've only just started using ocamlformat and it's a young tool with some way to go before it can be imposed as a standard. For example, object syntax isnt supported, and there are still rewrite issues with portions of the AST. The tool is definitely heading in the right direction, but will take some time to mature.

[hannesm]

ok, so how do we move forward here? I'd appreciate to have such a document in place (and agree that we should as well have some example libraries which are "high quality"). my experience with ocamlformat didn't lead to immediate adaption (main reason: ocp-indent and ocamlformat disagree (with default configuration) on indentation -- I use ocp-indent while developing, and am willing to use ocamlformat as pre-commit hook, but if they disagree on indentation, that's too much hassle for me)).

there are still some comments (from several people). can someone with a fresh pair of eyes review/revise this PR and we get settled to a first public draft!?

[samoht]

I will be very happy to see this moving forward but I have unfortunately a growing list of TODOs that I want to complete first. If anyone else is interested, please feel free to push directly to my branch :-)

[hannesm]

EDIT: sorry for the force-push

I spend some time to revise this PR, and first wrote a paragraph that not every OCaml library works on MirageOS. I introduced tags (ready (works with MirageOS), integrated (uses coding style, packaging, documentation guidelines), automated (uses travis CI), approved (has owners, fuzz testing, etc.), verified) instead of levels (NB: approved has too many subitems).

I think most of our core libraries are in automated state, we should get badges there and strive for approved within the MirageOS4 release cycle!?

[dinosaure]

May be add a note about ocamlformat here.

[dinosaure]

May be add a note about ocaml-ci.

[samoht]

I'd like to revisit that PR at one point, as @TheLortex has started to work on mirage-ci.