Provide a TAP device to enclave application #43

NullHypothesis · 2022-11-21T14:00:29Z

This PR does the following:

Replace our HTTP-01 Let's Encrypt challenge with TLS-ALPN-01, which simplifies the code and obviates the need for opening port 80.
Allow applications to register a SHA-256 over their public key material. Together with the hash over our X509 certificate, this hash is embedded in attestation documents.
Update the documentation and add an architecture diagram.
Turn nitriding into a stand-alone executable and add it to cmd/.
Expose an Internet-facing Web server (for remote attestation, key synchronization, and nonce retrieval) and an enclave-internal Web server (for state operations, signalling readiness, and for requesting key synchronization).
Add a handler that allows the application to signal when it's ready, after which nitriding starts its Internet-facing Web server.
Add a reverse proxy that terminates HTTPS and forwards HTTP requests to the application (if desired).
Add a Python example application to example/.
Add networking code that creates an enclave-internal TAP interface which forwards network packets via the VSOCK interface. Most of this is provided by the package gvisor-tap-vsock.

This fixes #26.

...because nitriding is no longer (just) a package.

Nitriding is going to be running two Web servers: one is public-facing and meant to be accessed by clients and the other is enclave-facing and meant to be accessed by the enclave application.

Nitriding is now a stand-alone application, obviating the need for an API to add HTTP handlers.

The reverse proxy terminates TLS and forwards all but a select few HTTP requests to the enclave application, which runs its own Web server. Since the reverse proxy terminates TLS, the enclave application does not need to bother with certificates and can expose a simple HTTP server.

Non-HTTP enclave applications need a way to link their key material to the attestation document, which serves as our root of trust. This commit adds a new enclave-internal endpoint that allows applications to register a hash over their public key material. This hash (along with a hash over nitriding's HTTPS certificate) is then embedded in attestation documents.

This enclave-internal HTTP handler allows applications to signal their readiness to nitriding. When the handler is called, nitriding starts its Internet-facing Web server.

Domain sockets are great for high-throughput applications but we don't need troughput here; we need ease of use.

A simple Python client that retrieves its IP address by connecting to a Web server.

The endpoint takes as input a SHA-256 hash, so "hash" better reflects what's going on behind the scenes; "key" is too broad of a term.

This patch makes use of Let's Encrypt's tls-alpn-01 challenge which is simpler than the http-01 challenge because it does not require a separate listener on port 80.

...otherwise, the operation fails.

This commit removes the chi import that's not part of its v5 API. It also initializes chi's middleware before we create routes because chi requires that.

cmd/main.go

attestation.go

enclave.go

rillian · 2022-11-22T23:15:46Z

enclave.go

+	// The following paths are handled by nitriding.
+	pathRoot        = "/enclave"
+	pathNonce       = "/enclave/nonce"
+	pathAttestation = "/enclave/attestation"
+	pathState       = "/enclave/state"
+	pathSync        = "/enclave/sync"
+	pathHash        = "/enclave/hash"
+	pathReady       = "/enclave/ready"


I expected a subrouter pattern here, so the /enclave prefix is only specified once and it's easy to move all the enclave-related handlers around if necessary. But there aren't many paths and they're all in the same place, so I'm fine with this as-is.

handlers.go

This commit 1) ensures that all required command line arguments are present and 2) validates the arguments' values. This commit also changes the type of some configuration variables to more appropriate types, e.g., uint16 for AF_INET port numbers.

This is going to facilitate the transition to a different hash function.

cmd/main.go

rillian · 2022-11-28T21:23:35Z

cmd/main.go

+	flag.Int64Var(&hostProxyPort, "host-proxy-port", 1024,
+		"Port of proxy application running on EC2 host.")


This has to be 64 bits because there's no flag.UIntVar? Seems unlikely we'll be running on a 32-bit host, where one would otherwise have to use negative numbers to specify ports with the high bit set?

In fact, there is flag.UintVar and we might as well use it for all three ports. I addressed this in a098ea3.

I like this better!

example/Dockerfile

This adds clarity.

There's no reason to allow negative ports and Go's spec guarantees that a uint is at least 32 bits -- the size of an AF_VSOCK port.

Ralph measured that this saves around 20 MB of space.

kdenhartog

few questions I just want some further clarification on before approving, but overall this is looking safe

kdenhartog · 2023-01-20T00:28:03Z

doc/architecture.md

+nitriding->>nitriding: Set up enclave-internal Web server
+
+nitriding->>+ec2: Packet forwarding
+ec2->>+ca: Request HTTPS certificate (via HTTP-01)


non blocking: Is this still accurate now that TLS-ALPN-01 has replaced HTTP-01?

Good to see these docs here!

Whoops, that's a mistake. Good catch! Fixed in 94d7d99.

kdenhartog · 2023-01-20T00:37:04Z

enclave.go

+	m.Get(pathAttestation, attestationHandler(e.hashes))
+	m.Get(pathNonce, nonceHandler(e))
+	m.Get(pathRoot, rootHandler(e.cfg))
+	m.Post(pathSync, respSyncHandler(e, time.Now))


Non-blocking: How might OS time changing affect this? I've heard of a method that MITM network time protocol to make it possible to reuse expired certs and I'm wondering if that may be something we'd encounter here as well.

From what I could tell, there's no actual way to resolve this without redoing NTP so that's why I'm not blocking on it, but I want to bring up in case you have ideas that might help improve this.

I took another look at the code and noticed that the time argument is not actually used! I deleted it in c850d56.

kdenhartog · 2023-01-20T00:49:20Z

go.mod

+	github.com/songgao/packets v0.0.0-20160404182456-549a10cd4091
+	github.com/songgao/water v0.0.0-20200317203138-2b4b6d7c09d8
+	github.com/vishvananda/netlink v1.2.1-beta.2
+	golang.org/x/crypto v0.3.0


Is there a reason for switching from a specific hash for these to now just setting a version?

Go's tooling made this change because the Go folks finally started using tags for packages in golang.org/x/ (see golang/go#21324). In fact, I just updated to the latest dependencies in beed0b9, which bumps the x/crypto package to v0.5.0.

Cool! good to know this is the direction they're heading to prevent supply chain attacks through redeploying a new package under the same version

kdenhartog · 2023-01-20T00:51:26Z

handlers.go

+func getStateHandler(e *Enclave) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/octet-stream")
+		s, err := e.KeyMaterial()


Do we have a way to validate that this hasn't been changed?

Key material can be set in two ways:

Within the enclave, by the trusted enclave application. In this case, we don't need validation because all involved parties are trusted.

Across the Internet (or ideally: across our private Kubernetes network), when enclaves synchronize with each other. In this case, we do mutual authentication using attestation documents.

Does that answer your question? I hope I didn't miss anything.

kdenhartog · 2023-01-20T00:54:34Z

handlers.go

+// putStateHandler returns a handler that lets the enclave application set
+// state that's synchronized with another enclave in case of horizontal
+// scaling.  The state can be arbitrary bytes.
+func putStateHandler(e *Enclave) http.HandlerFunc {


Do we need to do any validation for this endpoint first? I was thinking previously we were validating the attestation of the other server before allowing the state to be set but maybe thats for a different endpoint.

Right, you are probably thinking about the function respSyncHandler. The purpose of this endpoint is to let the (trusted) enclave application set whatever state it wants to be set. It's an enclave-internal, trusted endpoint. We allow the application to set arbitrary bytes, so we limit our validation to enforcing a length limit by taking advantage of newLimitReader.

Yup, that was the one I was originally thinking about. Ok, that makes sense for why getStateHandler and putStateHandler are not validating at the nitriding application layer here. I think the aspect that's throwing me off is at what level are we isolating the trust boundary for this. From the code it appears as if this is just a publicly available endpoint, but from the sounds of it this is being isolated at maybe(?) the network layer. Can you point me to where that's being done so I can understand how these two endpoints are being secured better? I don't see an issue with leaving these endpoints unvalidated at this point, but I just want to better understand the attack surface to make sure I'm not misunderstanding something.

To isolate public from private endpoints, we are using two separate Web servers: one that binds to localhost (see here) and another (see here) that imposes no such restrictions. I'm going to add a comment to the affected endpoint handlers because this is more confusing than it should be.

9edb2c4 adds a few comments, explaining that some of our endpoints are enclave-internal.

Note that we have to run `go get gvisor.dev/gvisor/runsc@go` in addition to `go get -u` because we need gvisor/runsc's Go branch: https://github.com/google/gvisor#using-go-get

NullHypothesis · 2023-01-23T15:14:20Z

@kdenhartog Are you happy with the changes I made? If so, we should be ready to merge!

rillian

Nice to see the clear comments about which handler is for which interface.

kdenhartog · 2023-01-24T20:52:07Z

This looks great! Thanks for taking the time to add the additional comments.

kdenhartog

LGTM

Philipp Winter added 30 commits November 17, 2022 10:32

PoC for TAP support.

6d1f996

This fixes #26.

Add executable.

772c1db

...because nitriding is no longer (just) a package.

Add config validation step.

63533c0

Add system architecture diagram.

95c1aab

Add handler that allows for secret registration.

63faf97

Nitriding is going to be running two Web servers: one is public-facing and meant to be accessed by clients and the other is enclave-facing and meant to be accessed by the enclave application.

Add Makefile and refactor standalone application.

3ffa59d

Add .gitignore.

523667b

Add target to build nitriding executable.

d2b9202

Add function for creating a unix domain socket.

21d68c8

Refactor.

755834f

Remove AddRoute.

167c5cf

Nitriding is now a stand-alone application, obviating the need for an API to add HTTP handlers.

Re-create networking if necessary.

3f19c82

Rename handler.

a202081

Display domain socket address.

52ad0d6

Add log messages.

ea0971f

Rename handlers and add tests.

3461486

Add sync handler.

cf0284c

Update comment.

f60dcf2

Block forever after starting the enclave.

894f701

Add Go dependencies of command line tool.

02da09e

Provide a function to shut down the enclave.

00f40b8

Add handler to signal readiness.

efb5580

This enclave-internal HTTP handler allows applications to signal their readiness to nitriding. When the handler is called, nitriding starts its Internet-facing Web server.

Replace domain socket with TCP port.

80812c1

Domain sockets are great for high-throughput applications but we don't need troughput here; we need ease of use.

Re-add loopback interface creation.

f86cd67

Add example application.

9ab4012

A simple Python client that retrieves its IP address by connecting to a Web server.

Rename path from "key" to "hash".

e757a4c

The endpoint takes as input a SHA-256 hash, so "hash" better reflects what's going on behind the scenes; "key" is too broad of a term.

Update documentation and architecture diagram.

82c0b1f

Simplify the way we use autocert.

130eb77

This patch makes use of Let's Encrypt's tls-alpn-01 challenge which is simpler than the http-01 challenge because it does not require a separate listener on port 80.

Philipp Winter added 4 commits November 22, 2022 09:33

Set default gateway after activating link.

6990ac2

...otherwise, the operation fails.

Use requests.status_codes.codes.ok instead of 200.

621b1e4

Invoke r.raise_for_status() after request.

41d8af8

Don't mix chi's major version numbers.

a832419

This commit removes the chi import that's not part of its v5 API. It also initializes chi's middleware before we create routes because chi requires that.

rillian reviewed Nov 22, 2022

View reviewed changes

Philipp Winter added 4 commits November 23, 2022 11:23

Validate all command line arguments.

9906e90

This commit 1) ensures that all required command line arguments are present and 2) validates the arguments' values. This commit also changes the type of some configuration variables to more appropriate types, e.g., uint16 for AF_INET port numbers.

Add prefix to serialized attestation hashes.

c568165

This is going to facilitate the transition to a different hash function.

Add FIXME and refer to GitHub issue.

7b6f27e

Use latest Go version in GitHub action.

de88f4d

NullHypothesis mentioned this pull request May 1, 2023

Specify mechanism for enclave synchronization brave/nitriding-daemon#10

Closed

rillian previously approved these changes Nov 28, 2022

View reviewed changes

Replace our own constants with the ones from math.

b78c4b8

This adds clarity.

NullHypothesis dismissed rillian’s stale review via b78c4b8 November 29, 2022 14:22

Philipp Winter added 2 commits November 29, 2022 08:32

Use flag.UintVar for all ports.

a098ea3

There's no reason to allow negative ports and Go's spec guarantees that a uint is at least 32 bits -- the size of an AF_VSOCK port.

Install requests via apk instead of pip.

8710b38

Ralph measured that this saves around 20 MB of space.

rillian previously approved these changes Nov 29, 2022

View reviewed changes

kdenhartog reviewed Jan 20, 2023

View reviewed changes

Delete outdated references to HTTP-01 challenge.

94d7d99

NullHypothesis dismissed rillian’s stale review via 94d7d99 January 20, 2023 14:10

Philipp Winter added 2 commits January 20, 2023 08:20

Update to latest dependencies.

beed0b9

Note that we have to run `go get gvisor.dev/gvisor/runsc@go` in addition to `go get -u` because we need gvisor/runsc's Go branch: https://github.com/google/gvisor#using-go-get

Remove unused curTime argument.

c850d56

rillian previously approved these changes Jan 20, 2023

View reviewed changes

Be explicit about enclave-internal endpoints.

9edb2c4

NullHypothesis dismissed rillian’s stale review via 9edb2c4 January 24, 2023 14:06

rillian approved these changes Jan 24, 2023

View reviewed changes

kdenhartog approved these changes Jan 24, 2023

View reviewed changes

NullHypothesis merged commit 4053741 into master Jan 24, 2023

NullHypothesis deleted the tun-tap branch January 24, 2023 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a TAP device to enclave application #43

Provide a TAP device to enclave application #43

NullHypothesis commented Nov 21, 2022 •

edited

Loading

rillian Nov 22, 2022

rillian Nov 28, 2022

NullHypothesis Nov 29, 2022

rillian Nov 29, 2022

kdenhartog left a comment

kdenhartog Jan 20, 2023

NullHypothesis Jan 20, 2023

kdenhartog Jan 20, 2023

NullHypothesis Jan 20, 2023

kdenhartog Jan 20, 2023

NullHypothesis Jan 20, 2023

kdenhartog Jan 24, 2023

kdenhartog Jan 20, 2023

NullHypothesis Jan 20, 2023

kdenhartog Jan 20, 2023

NullHypothesis Jan 20, 2023

kdenhartog Jan 24, 2023

NullHypothesis Jan 24, 2023

NullHypothesis Jan 24, 2023

NullHypothesis commented Jan 23, 2023

rillian left a comment

kdenhartog commented Jan 24, 2023

kdenhartog left a comment

		flag.Int64Var(&hostProxyPort, "host-proxy-port", 1024,
		"Port of proxy application running on EC2 host.")

Provide a TAP device to enclave application #43

Provide a TAP device to enclave application #43

Conversation

NullHypothesis commented Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdenhartog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullHypothesis commented Jan 23, 2023

rillian left a comment

Choose a reason for hiding this comment

kdenhartog commented Jan 24, 2023

kdenhartog left a comment

Choose a reason for hiding this comment

NullHypothesis commented Nov 21, 2022 •

edited

Loading