Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial set of patches for supporting CloudABI #16612

Closed
wants to merge 3 commits into from

Conversation

EdSchouten
Copy link
Contributor

@EdSchouten EdSchouten commented Oct 30, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
    Some of the tests fail on my FreeBSD system (IPv4 address mismatches), but these errors are unrelated to the change at hand.
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

build, src

CloudABI is a compact POSIX-like runtime that makes use of
capability-based security. More details:

	https://github.com/NuxiNL/cloudlibc
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 30, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack refack added build Issues and PRs related to build files or the CI. python PRs and issues that require attention from people who are familiar with Python. labels Oct 30, 2017
@refack
Copy link
Contributor

refack commented Oct 30, 2017

Hello @EdSchouten and welcome! Thank you for the contribution.

Change LTGM on the face of it, but after reading a little bit about CloudABI, it seems that implementing support raises some interesting questions:

image

image

  1. How would you envision the node runtime operating under these axioms? Since most of the common use cases are I/O based...
  2. How would that be implemented? A lot on ifdefs or does Cloudlibc throw EPERM/EOPNOTSUPP for the not implemented operations?
  3. How do we build and test?

/cc @nodejs/build

@refack refack added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 30, 2017
@EdSchouten
Copy link
Contributor Author

Hi @refack. Thanks for taking a look at these changes!

Let me answer your questions, also providing some background/motivation:

How would that be implemented? A lot on ifdefs or does Cloudlibc throw EPERM/EOPNOTSUPP for the not implemented operations?

CloudABI is an offshoot of a sandboxing framework provided by FreeBSD, called Capsicum. The main difference between the two is that Capsicum goes into the direction of keeping system calls around and letting them return a special errno ENOTCAPABLE, whereas CloudABI omits them entirely.

Though CloudABI's approach causes lots of existing builds to break (including Node.js's), it has a big advantage over Capsicum: the compiler errors give you a guided approach to making an application work well with sandboxing. You immediately get a sense of which parts work and which ones don't. The number of compiler errors acts as a rough estimate for tracking progress.

In the process, CloudABI's approach has (unfortunately) allowed us to find many pieces of code that do things like this:

int fd = open("/dev/urandom", O_RDONLY):
if (fd < 0) {
    // Initialize the PRNG with getpid(), etc.
}
// Do some computation that relies on high-quality randomness (crypto, etc).

Which is of course quite dangerous, regardless of which sandboxing/security/container/... framework is being used. You try to make your software more secure by using sandboxing, but in the end you've ended up introducing bugs. This is why I've become a huge advocate of removing incompatible features entirely.

A very brief answer to your question would be "A lot on ifdefs", but that's absolutely not what I'm aiming for in the long run. Right now I'm disabling features in Node.js to at least get it to work, but my hope is that designs of future APIs also take 'sandboxability' into account and thus don't need to be disabled.

A good example of work in this area is Python 3.4's pathlib. Initially intended to make pathname handling less cumbersome, its object oriented API also improves testability/reusability, and in CloudABI's case sandboxability. An improvement, even for people who don't care about CloudABI.

How would you envision the node runtime operating under these axioms? Since most of the common use cases are I/O based...

The idea behind CloudABI is that programs have all of their dependencies on external resources (paths disk, bound sockets, connections to backend services, etc.) injected in the form of file descriptors. Instead of using int main(), CloudABI programs may use an alternative entry point:

void program_main(const argdata_t *ad) {
   ...
}

The argument to this function is a user-provided JSON/YAML-like object that, in addition to strings, integers, etc. can hold file descriptors. One thing I still need to implement is that this tree is exposed within v8. So if you normally spawn a simple web server like this:

http.createServer(function (req, res) {
  ...
}).listen(8080);

You'd have to adjust it like this to work on CloudABI:

http.createServer(function (req, res) {
  ...
}).listen(process.argdata['some_key_from_the_yaml_spec']);

How do we build and test?

Building Node.js for CloudABI should be relatively easy and can be done on most (UNIX-like?) operating systems out there. Even if your OS/distro doesn't have a package for a CloudABI cross compiler yet, just install a vanilla copy of LLVM >= 4 and add symlinks (bin/x86_64-unknown-cloudabi-{cc,c++,ld}) pointing to the Clang/LLD binaries. For certain systems (e.g., macOS, FreeBSD), it's just a matter of installing the right package.

Copies of cross compiled libraries that you'll need (CloudABI's C library, but also all of Node.js's dependencies) are automatically built through CloudABI Ports and can be installed through a variety of package managers (apt-get, rpm, Homebrew). After installing those, it should be a matter of running ./configure with the appropriate environment variables, command line flags, etc. to get the build started. That said, if we want to start doing some continuous builds at some point, be sure to get in touch and we can work out all of the details.

If you don't feel like building Node.js yourself, I've also packaged Node.js through CloudABI Ports. Right now it still has a lot of local patches, of course. My goal is to use that as a patch queue for things that should still be cleaned up, sent out as a pull request, etc.

With regards to testing: so far I haven't really taken a stab at getting Node.js's tests to work. This is, of course, something that's important to pursue in the future.

@@ -111,7 +111,7 @@ typedef int mode_t;
#include <unistd.h> // setuid, getuid
#endif

#if defined(__POSIX__) && !defined(__ANDROID__)
#if defined(__POSIX__) && !defined(__ANDROID__) && !defined(__CloudABI__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO since this is used in three places, it should be aliased to something like UNSANDBOXED_POSIX or defined(__POSIX__) && !SANDBOXED_POSIX

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just added a definition, HAVE_POSIX_CREDENTIALS, that is declared when building for systems that support <pwd.h>, <grp.h>, set*uid(), etc. Does that look all right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I yield to @cjihrig.
Maybe revisit in the future if there are more usages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right. I've just rolled back the HAVE_POSIX_CREDENTIALS changes.

@refack
Copy link
Contributor

refack commented Oct 31, 2017

@EdSchouten thank you for the informative response. If you haven't already PTAL at the platform support strategy (https://github.com/nodejs/node/blob/master/BUILDING.md#strategy). My long term concern is that without CI these changes will rot. So IMHO we next step is to design a CI job that will run some basic tests on this (see nodejs/build#419).

@EdSchouten
Copy link
Contributor Author

Having a CI job to do automated builds for CloudABI does indeed make sense. I suspect that's only useful to have after I've managed to upstream all build fixes, right?

In the meantime I'm also sending patches over to the V8 developers. Let's see how well that goes...

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't block it, but I'm withdrawing my LGTM. I'm not a big fan of going the HAVE_POSIX_CREDENTIALS route.

@EdSchouten
Copy link
Contributor Author

@cjihrig If you want, I can revert that part.
@refack @addaleax Thoughts?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'm fine with both HAVE_POSIX_CREDENTIALS or just spelling it out.

As CloudABI is intended to run applications in cluster contexts (e.g.,
on Kubernetes), they are oblivious of UNIX credentials. Extend the
existing preprocessor checks to disable any use of these interfaces,
just like on Windows, Android, etc.
cares_wrap.cc calls into functions like getnameinfo() and getaddrinfo().
These functions tend to be available implicitly through <uv.h>, but we'd
better still include this header explicitly.

On CloudABI, we make use of a custom implementation of libuv that does
not implicitly include header files like <netdb.h>.
@cjihrig
Copy link
Contributor

cjihrig commented Nov 1, 2017

Re-LGTM

refack pushed a commit to refack/node that referenced this pull request Nov 1, 2017
CloudABI is a compact POSIX-like runtime that makes use of
capability-based security. More details:
https://github.com/NuxiNL/cloudlibc

* src: Disable use of pwd.h, grp.h and get*uid() on CloudABI.
As CloudABI is intended to run applications in cluster contexts (e.g.,
on Kubernetes), they are oblivious of UNIX credentials. Extend the
existing preprocessor checks to disable any use of these interfaces,
just like on Windows, Android, etc.

* src: Explicitly include <netdb.h>.
cares_wrap.cc calls into functions like getnameinfo() and getaddrinfo().
These functions tend to be available implicitly through <uv.h>, but we'd
better still include this header explicitly.

On CloudABI, we make use of a custom implementation of libuv that does
not implicitly include header files like <netdb.h>.

PR-URL: nodejs#16612
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@refack
Copy link
Contributor

refack commented Nov 1, 2017

Landed in 78dbcbe

@refack refack closed this Nov 1, 2017
@refack
Copy link
Contributor

refack commented Nov 1, 2017

@EdSchouten thank you, and best of luck with the project.

As a bonus GitHub promoted you from
image
to
image

@EdSchouten
Copy link
Contributor Author

Heh, awesome. Thanks! \o/

@EdSchouten EdSchouten deleted the cloudabi branch November 1, 2017 21:58
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
CloudABI is a compact POSIX-like runtime that makes use of
capability-based security. More details:
https://github.com/NuxiNL/cloudlibc

* src: Disable use of pwd.h, grp.h and get*uid() on CloudABI.
As CloudABI is intended to run applications in cluster contexts (e.g.,
on Kubernetes), they are oblivious of UNIX credentials. Extend the
existing preprocessor checks to disable any use of these interfaces,
just like on Windows, Android, etc.

* src: Explicitly include <netdb.h>.
cares_wrap.cc calls into functions like getnameinfo() and getaddrinfo().
These functions tend to be available implicitly through <uv.h>, but we'd
better still include this header explicitly.

On CloudABI, we make use of a custom implementation of libuv that does
not implicitly include header files like <netdb.h>.

PR-URL: nodejs/node#16612
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Nov 6, 2017
CloudABI is a compact POSIX-like runtime that makes use of
capability-based security. More details:
https://github.com/NuxiNL/cloudlibc

* src: Disable use of pwd.h, grp.h and get*uid() on CloudABI.
As CloudABI is intended to run applications in cluster contexts (e.g.,
on Kubernetes), they are oblivious of UNIX credentials. Extend the
existing preprocessor checks to disable any use of these interfaces,
just like on Windows, Android, etc.

* src: Explicitly include <netdb.h>.
cares_wrap.cc calls into functions like getnameinfo() and getaddrinfo().
These functions tend to be available implicitly through <uv.h>, but we'd
better still include this header explicitly.

On CloudABI, we make use of a custom implementation of libuv that does
not implicitly include header files like <netdb.h>.

PR-URL: nodejs#16612
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
@MylesBorins
Copy link
Contributor

Does this make sense to backport to v6.x and v8.x now or should we wait for more CloudABI support to be done first?

@EdSchouten
Copy link
Contributor Author

Hi Myles,

I'd say, don't bother for now. Considering that I also need to send a fair number of patches integrated into V8 as well, let's just focus on supporting future versions of Node.

addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
CloudABI is a compact POSIX-like runtime that makes use of
capability-based security. More details:
https://github.com/NuxiNL/cloudlibc

* src: Disable use of pwd.h, grp.h and get*uid() on CloudABI.
As CloudABI is intended to run applications in cluster contexts (e.g.,
on Kubernetes), they are oblivious of UNIX credentials. Extend the
existing preprocessor checks to disable any use of these interfaces,
just like on Windows, Android, etc.

* src: Explicitly include <netdb.h>.
cares_wrap.cc calls into functions like getnameinfo() and getaddrinfo().
These functions tend to be available implicitly through <uv.h>, but we'd
better still include this header explicitly.

On CloudABI, we make use of a custom implementation of libuv that does
not implicitly include header files like <netdb.h>.

PR-URL: nodejs/node#16612
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. notable-change PRs with changes that should be highlighted in changelogs. python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants