AUT-54: Add endpoint to serve local x5u chains #917

oskirby · 2024-07-06T17:52:11Z

Description

It has slightly bothered me that our integration tests, and local development experience frequently relies on /tmp being mounted into the container to serve certificate chains which are generated and stored locally using the file:// scheme. It is very janky, inadequately documented, and can lead to a brittle development experience. This caught my attention when my /tmp volume got corrupted with some stale state and it wound up breaking everything until I manually deleted the volume.

It has also caused friction for downstream consumers of the Autograph container during development, such as remote-settings

I propose the following solution, we create a new endpoint: /x5u/:keyid/:chainfile which acts as a mirror to serve the contents of the configured chainuploadlocation for a signer, if, and only if, that signer has been configured with a local file upload location (eg: uses the file:// scheme).

Then, when generating signatures, we check if the signature contains an x5u URL with the file scheme, and replace it with the URL to the new x5u endpoint.

This should mean that we no longer need to share /tmp between autograph containers, and we can eliminate the use of this volume altogether.

I have added some tests for the endpoint, but I am not completely happy with the coverage as we would need a way to mock out the signers and mess around with their chain upload locations to really get full coverage.

Reference

See issue: #350 (AUT-54)

handlers.go

bhearsum

This probably ought to be reviewed by @jmhodges or someone else that knows the codebase better, but the idea is certainly solid (and obviously desired by other stakeholders), and this implementation seems fine.

If I understand correctly, these files are written to ChainUploadLocation over at

autograph/signer/contentsignaturepki/upload.go

Lines 30 to 31 in 5f97ba3

    
           case "file": 
        
           	return writeLocalFile(data, name, parsedURL)

, and we're just serving those indirectly through autograph.

The most robust and production-like way to do this would probably be to add a mock s3 container (such as https://github.com/adobe/S3Mock), but that is both a) more work, and b) may not satisfy the needs of Remote Settings, so I suspect it's probably never going to be worthwhile to do.

jmhodges · 2024-08-01T21:27:11Z

handlers.go

+		httpError(w, r, http.StatusMethodNotAllowed, "%s method not allowed; endpoint accepts GET only", r.Method)
+		return
+	}
+	if r.Body != nil {


All of this code can be deleted. There's no Body ever sent on GETs and we shouldn't do anything with them, anyway. Go will handle the reading of bogus requests when we return the 405.

IIUC, technically this is incorrect. The HTTP protocol doesn't prohibit a GET from providing a body, it is just that by convention no one does so.

Sure, but I forgot to mention that the Go http library will close out the Body for us, anyhow, so it's still redundant.

jmhodges · 2024-08-01T21:27:42Z

handlers.go

+	if r.Method != "GET" {
+		httpError(w, r, http.StatusMethodNotAllowed, "%s method not allowed; endpoint accepts GET only", r.Method)
+		return


This is already being handled in the routing later, no? That Methods("GET") call?

(I know we have some other APIs doing this repetitive work, but I think that was a bit of a mistake. I could be convinced the other way, but I know this might all be mooted with the http.FileServer work I describe below)

I assumed that there was a reason for having that code, so I simply copied what the other handlers were doing. However, if the routing layer will do this for us I agree it would be a lot cleaner and I can spend another cycle updating the rest of the handlers to drop the unsupported method cases and cleaning up the tests.

jmhodges · 2024-08-01T21:30:33Z

handlers_test.go

+			name:            "invalid method POST returns 405",
+			method:          "POST",
+			url:             "http://foo.bar/x5u/normandy/example.pem",
+			nilBody:         true,
+			expectedStatus:  http.StatusMethodNotAllowed,
+			expectedBody:    "POST method not allowed; endpoint accepts GET only\r\nrequest-id: -\n",
+			expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}},
+		},
+		{
+			name:            "invalid method PUT returns 405",
+			method:          "PUT",
+			url:             "http://foo.bar/x5u/normandy/example.pem",
+			nilBody:         true,
+			expectedStatus:  http.StatusMethodNotAllowed,
+			expectedBody:    "PUT method not allowed; endpoint accepts GET only\r\nrequest-id: -\n",
+			expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}},
+		},
+		{
+			name:            "invalid method OPTIONS returns 405",
+			method:          "OPTIONS",
+			url:             "http://foo.bar/x5u/normandy/example.pem",
+			nilBody:         true,
+			expectedStatus:  http.StatusMethodNotAllowed,
+			expectedBody:    "OPTIONS method not allowed; endpoint accepts GET only\r\nrequest-id: -\n",
+			expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}},
+		},
+		{
+			name:            "invalid method HEAD returns 405",
+			method:          "HEAD",
+			url:             "http://foo.bar/x5u/normandy/example.pem",
+			nilBody:         true,
+			expectedStatus:  http.StatusMethodNotAllowed,
+			expectedBody:    "HEAD method not allowed; endpoint accepts GET only\r\nrequest-id: -\n",
+			expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}},
+		},


I totally understand the desire here, but I think we want this HTTP method rejection work to be done in the routing layer. It feels a lil like "testing the framework". If you want, you could refactor the routing set up to make it easier to test?

jmhodges · 2024-08-01T21:31:29Z

handlers_test.go

+			expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}},
+		},
+		{
+			name:            "GET with directory parent escapes returns 400",


I love these tests!

Sadly, all of these tests are no longer relevant :)

jmhodges · 2024-08-01T21:31:45Z

handlers_test.go

+			expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}},
+		},
+		{
+			name:            "GET with directory root escapes returns 400",


nit (and so ignorable if you disagree):Since we make so many of these might be an opportunity to reduce the size of the test with a tiny helper func to generate these "GET for this particular API" structs

I don't think a method would really clean this up any significant amount and we would still be typing out all the same things, just as method arguments instead of struct initialization values.

Oh, I was just seeing that most of the fields on the structs were all the same except for like 3. But, again, a nit, and to your taste.

jmhodges · 2024-08-01T21:42:04Z

handlers.go

+		chainFilePath := path.Join(parsedURL.Path, pathChainFile)
+		stat, err := os.Stat(chainFilePath)
+		if err != nil || !stat.Mode().IsRegular() {
+			break
+		}
+
+		// Serve files from the chain upload location
+		http.ServeFile(w, r, chainFilePath)


I wrote all the rest of this review and then got here and thought:

Stepping back, could some of this work be done with http.FileServer instead? Like, in the routing layer, we iterate over all of the getSigners just once and create a http.FileServer(http.Dir(chainUploadLocationDir)) per keyid with a file:// chainUploadLocation? It would mean not having to do basically any of these checks since http.Dir and FileServer do them for us. The small cost is it would serve the subdirectories under those locations, but I think we're okay with that for dev and test.

jmhodges · 2024-08-01T21:47:18Z

This is a great idea! I think the "http.FileServer(http.Dir(..)) in the routing layer" change is worth investigating before we ship

oskirby · 2024-08-20T03:37:32Z

I have refactored this work so that it now uses registers an http.FileServer for each signer, and we can rely on the mux router to map requests to the directories for each signer.

jmhodges

Love how small this is getting! And thanks for investigating the Dir stuff! Have some questions about what's left and some tweaks (like the godoc and the extra var kicking around)

jmhodges · 2024-08-20T06:59:47Z

monitor_handler_racing_test.go

-			body, _, err := contentsignaturepki.GetX5U(&http.Client{}, response.X5U)
+			body, err := getMirroredX5U(response.X5U)


Why do we need to do this change? I thought the new handler would be loaded here and available to be fetched from.

My understanding of the unit tests is that they are not starting the HTTP server, and we are running the tests by invoking the handlers directly. That used to be fine for the contentsignaturepki.GetX5U because the file:// scheme would bypass any HTTP server anyways and just grab the certificate chain off the disk.

And this had to change again to work with the http.FileServer implementation because there is no single handler function to call to fetch x5u chains anymore.

jmhodges · 2024-08-20T07:00:34Z

signer/signer.go

+// IDFormatRegexp is the compiled regex from IDFormat
+var IDFormatRegexp = regexp.MustCompile(IDFormat)
+


I believe this can be removed now as it doesn't seem to be used

handlers_test.go

jmhodges · 2024-08-20T07:07:10Z

handlers.go

@@ -74,6 +75,22 @@ func logSigningRequestFailure(sigreq formats.SignatureRequest, sigresp formats.S
 	}).Info(fmt.Sprintf("signing operation failed with error: %v", err))
 }

+// If the x5u is a local file, mirror it via the `x5u` endpoint instead.
+func mirrorLocalX5U(r *http.Request, response formats.SignatureResponse) string {


I'm getting this confused with other similarly named things. Perhaps rewriteX5UIfLocalFile? Something about rewriting, maybe. The mirror sounds like it's copying the object from one place to another.

Also, nit: godoc format is to start the first sentence with the function's name

jmhodges · 2024-08-20T07:22:58Z

handlers.go

@@ -74,6 +75,22 @@ func logSigningRequestFailure(sigreq formats.SignatureRequest, sigresp formats.S
 	}).Info(fmt.Sprintf("signing operation failed with error: %v", err))
 }

+// If the x5u is a local file, mirror it via the `x5u` endpoint instead.
+func mirrorLocalX5U(r *http.Request, response formats.SignatureResponse) string {
+	parsedX5U, err := url.Parse(response.X5U)


Now that there's less to look at I'm wondering: could we do this URL rewriting earlier in the process? That is to say, is it possible to rewrite the X5U URL on the signer at signer creation time in contentsignaturepki.New or some such and not need all this happening at request time?

A couple follow-up questions I have after that: Would doing that rewrite earlier (if we can) allow us to simplify contentsignaturepki.GetX5U? And would doing the url rewrite earlier also mean the getMirroredX5U helper could go away since it doesn't need to know about files anymore? (I know I have a comment on getMirroredX5U wondering about whether it needs to exist given the current state of the code, but I'm asking here in case that other question gets a "yes it needs to exist" answer)

In theory it can be done statically by changing the X5U in the signer config to match what gets served by http.FileServer but to figure that out what the X5U should be, autograph would need to know its hostname to fully form the URL. And the hostname might not even be knowable at startup if it's running behind proxies or across container boundaries.

On the other hand, handling this at the time of the request is easy because we can just copy the hostname from the HTTP request headers.

Although, now that you mention it... all of this work should be acting upon config.X5U which is the public URL intended for users to fetch content from where config.ChainUploadLocation is the private URL to which chains are pushed. They just happen to have the same value for local testing.

Hypothetically, someone could setup a system where certs are saved locally to some dir, then a cron job uploads them somewhere else and users are expected to fetch the chains from the "somewhere else". This PR would break that hypothetical use case.

jmhodges · 2024-08-20T07:29:56Z

docs/endpoints.md

+
+This is an endpoint used to fetch certificate chains which are generated and
+stored locally. If the signer is configured with a `chainuploadlocation` with
+the `file://` scheme, then the contents of that file location are mirrored here.


nit: a brief mention of only wanting to use this for local dev and testing might be nice

jmhodges · 2024-08-20T17:55:24Z

handlers.go

@@ -203,6 +225,8 @@ func (a *autographer) handleSignature(w http.ResponseWriter, r *http.Request) {
 			X5U:        requestedSignerConfig.X5U,
 			SignerOpts: requestedSignerConfig.SignerOpts,
 		}
+		sigresps[i].X5U = rewriteLocalX5U(r, sigresps[i])


nit: this could be done in the struct set up but no more review cycles required to do that

github-advanced-security bot found potential problems Jul 6, 2024

View reviewed changes

handlers.go Fixed Show fixed Hide fixed

handlers.go Fixed Show fixed Hide fixed

oskirby force-pushed the aut-54-x5u-file-endpoint branch from ab9c54f to ec81360 Compare July 12, 2024 15:50

oskirby marked this pull request as ready for review July 16, 2024 17:02

oskirby requested a review from a team August 1, 2024 17:40

bhearsum previously approved these changes Aug 1, 2024

View reviewed changes

jmhodges requested changes Aug 1, 2024

View reviewed changes

oskirby added 10 commits August 19, 2024 16:16

Add endpoint to serve x5u chains from local storage

b4a64c9

Stop using /tmp to pass files between containers

d36d4dc

Clean up some formatting

b6d1a5f

Add extra check to reject path escape shenanigans

37b1856

Fix the monitor racing test

9f384e3

asdas

ad9598e

Add some tests for the x5u endpoint

a65d2e1

Run go fmt

5b4125a

Implement using http.FileServer instead

f40b5e2

Remove handleGetX5U

8910ed7

oskirby dismissed bhearsum’s stale review via 8910ed7 August 20, 2024 01:23

oskirby force-pushed the aut-54-x5u-file-endpoint branch from 45f756c to 8910ed7 Compare August 20, 2024 01:23

oskirby requested a review from a team as a code owner August 20, 2024 01:23

oskirby requested review from jmhodges and removed request for a team August 20, 2024 01:23

oskirby added 2 commits August 19, 2024 19:41

Remove unused strings import

3bb5a51

Fixup some lint

e5d773d

jmhodges requested changes Aug 20, 2024

View reviewed changes

oskirby added 2 commits August 20, 2024 08:58

Address some review comments

2970dc6

Another hacky fix to the monitor_handler_racing_test

3e19044

oskirby requested a review from jmhodges August 20, 2024 17:30

jmhodges reviewed Aug 20, 2024

View reviewed changes

jmhodges previously approved these changes Aug 20, 2024

View reviewed changes

Tweak rewriteLocalX5U() usage

9ecc59c

oskirby dismissed jmhodges’s stale review via 9ecc59c August 20, 2024 19:19

oskirby requested a review from jmhodges August 20, 2024 19:42

jmhodges approved these changes Aug 21, 2024

View reviewed changes

oskirby merged commit 08048d6 into main Aug 21, 2024
16 checks passed

This was referenced Nov 20, 2024

Fix #706: fix local server with new Autograph behaviour mozilla/remote-settings#707

Merged

X5U urls are sent as inaccessible file:// URLs in development #350

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AUT-54: Add endpoint to serve local x5u chains #917

AUT-54: Add endpoint to serve local x5u chains #917

oskirby commented Jul 6, 2024 •

edited

Loading

bhearsum left a comment

jmhodges Aug 1, 2024

oskirby Aug 6, 2024

jmhodges Aug 6, 2024

jmhodges Aug 1, 2024 •

edited

Loading

oskirby Aug 6, 2024

jmhodges Aug 1, 2024

jmhodges Aug 1, 2024

oskirby Aug 20, 2024

jmhodges Aug 1, 2024

oskirby Aug 6, 2024

jmhodges Aug 6, 2024

jmhodges Aug 1, 2024 •

edited

Loading

jmhodges commented Aug 1, 2024 •

edited

Loading

oskirby commented Aug 20, 2024

jmhodges left a comment

jmhodges Aug 20, 2024

oskirby Aug 20, 2024

jmhodges Aug 20, 2024

jmhodges Aug 20, 2024

jmhodges Aug 20, 2024 •

edited

Loading

oskirby Aug 20, 2024

oskirby Aug 20, 2024

jmhodges Aug 20, 2024

jmhodges Aug 20, 2024

		body, _, err := contentsignaturepki.GetX5U(&http.Client{}, response.X5U)
		body, err := getMirroredX5U(response.X5U)

		// IDFormatRegexp is the compiled regex from IDFormat
		var IDFormatRegexp = regexp.MustCompile(IDFormat)

AUT-54: Add endpoint to serve local x5u chains #917

AUT-54: Add endpoint to serve local x5u chains #917

Conversation

oskirby commented Jul 6, 2024 • edited Loading

Description

Reference

bhearsum 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

jmhodges Aug 1, 2024 • 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

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

jmhodges Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

jmhodges commented Aug 1, 2024 • edited Loading

oskirby commented Aug 20, 2024

jmhodges 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

jmhodges Aug 20, 2024 • 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

Choose a reason for hiding this comment

oskirby commented Jul 6, 2024 •

edited

Loading

jmhodges Aug 1, 2024 •

edited

Loading

jmhodges Aug 1, 2024 •

edited

Loading

jmhodges commented Aug 1, 2024 •

edited

Loading

jmhodges Aug 20, 2024 •

edited

Loading