From b4a64c9ca5e0978f809525c092f5872245776c58 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Fri, 5 Jul 2024 09:59:38 -0700 Subject: [PATCH 01/15] Add endpoint to serve x5u chains from local storage --- docs/endpoints.md | 6 ++++ handlers.go | 85 ++++++++++++++++++++++++++++++++++++++++++++++ main.go | 1 + monitor_handler.go | 1 + signer/signer.go | 3 ++ 5 files changed, 96 insertions(+) diff --git a/docs/endpoints.md b/docs/endpoints.md index 7dbedd473..4427f3677 100644 --- a/docs/endpoints.md +++ b/docs/endpoints.md @@ -271,6 +271,12 @@ See `/sign/data`, the response format is identical. A successful request return a `201 Created` with a response body containing an S/MIME detached signature encoded with Base 64. +## /x5u/:keyid/:chain + +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. + ## /\_\_monitor\_\_ This is a special endpoint designed to monitor the status of all signers diff --git a/handlers.go b/handlers.go index a794b7a97..00e4028cc 100644 --- a/handlers.go +++ b/handlers.go @@ -14,6 +14,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "path" "time" @@ -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) + if err == nil && parsedX5U.Scheme == "file" { + mirroredX5U := url.URL{ + Scheme: "http", + Host: r.Host, + Path: path.Join("x5u", response.SignerID, path.Base(parsedX5U.Path)), + } + return mirroredX5U.String() + } + + // Otherwise, return the X5U unmodified + return response.X5U +} + // handleSignature endpoint accepts a list of signature requests in a HAWK authenticated POST request // and calls the signers to generate signature responses. func (a *autographer) handleSignature(w http.ResponseWriter, r *http.Request) { @@ -203,6 +220,8 @@ func (a *autographer) handleSignature(w http.ResponseWriter, r *http.Request) { X5U: requestedSignerConfig.X5U, SignerOpts: requestedSignerConfig.SignerOpts, } + sigresps[i].X5U = mirrorLocalX5U(r, sigresps[i]) + // Make sure the signer implements the right interface, then sign the data switch r.URL.RequestURI() { case "/sign/hash": @@ -489,3 +508,69 @@ func (a *autographer) handleGetAuthKeyIDs(w http.ResponseWriter, r *http.Request w.WriteHeader(http.StatusOK) w.Write(signerIDsJSON) } + +func (a *autographer) handleGetX5u(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + httpError(w, r, http.StatusMethodNotAllowed, "%s method not allowed; endpoint accepts GET only", r.Method) + return + } + if r.Body != nil { + body, err := io.ReadAll(r.Body) + if err != nil { + httpError(w, r, http.StatusBadRequest, "failed to read request body: %s", err) + return + } + if len(body) > 0 { + httpError(w, r, http.StatusBadRequest, "endpoint received unexpected request body") + return + } + } + + pathKeyID, ok := mux.Vars(r)["keyid"] + if !ok { + httpError(w, r, http.StatusInternalServerError, "route is improperly configured") + return + } + if !signer.IDFormatRegexp.MatchString(pathKeyID) { + httpError(w, r, http.StatusBadRequest, "keyid in URL path '%s' is invalid, it must match %s", pathKeyID, signer.IDFormat) + return + } + + pathChainFile, ok := mux.Vars(r)["chainfile"] + if !ok { + httpError(w, r, http.StatusInternalServerError, "route is improperly configured") + return + } + if !signer.IDFormatRegexp.MatchString(pathKeyID) { + httpError(w, r, http.StatusBadRequest, "keyid in URL path '%s' is invalid, it must match %s", pathKeyID, signer.IDFormat) + return + } + + // Lookup the signer, and see if it has a local X5U chain upload location. + // Treat all errors as a 404 to avoid leaking unnecessary signer details as this + // endpoint has no authentication. + for _, s := range a.getSigners() { + config := s.Config() + if config.ID != pathKeyID { + continue + } + + // Check if the signer has a chain upload location, and the requested file exists. + parsedURL, err := url.Parse(config.ChainUploadLocation) + if err != nil || parsedURL.Scheme != "file" { + break + } + 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) + return + } + + log.Infof("X5U not found for signer: %s", pathKeyID) + httpError(w, r, http.StatusNotFound, "404 page not found") +} diff --git a/main.go b/main.go index cfea2903d..59ced4ea0 100755 --- a/main.go +++ b/main.go @@ -214,6 +214,7 @@ func run(conf configuration, listen string, debug bool) { router.HandleFunc("/sign/data", statsMiddleware(ag.handleSignature, "http.api.sign/data", stats)).Methods("POST") router.HandleFunc("/sign/hash", statsMiddleware(ag.handleSignature, "http.api.sign/hash", stats)).Methods("POST") router.HandleFunc("/auths/{auth_id:[a-zA-Z0-9-_]{1,255}}/keyids", statsMiddleware(ag.handleGetAuthKeyIDs, "http.api.getauthkeyids", stats)).Methods("GET") + router.HandleFunc("/x5u/{keyid:[a-zA-Z0-9-_]{1,64}}/{chainfile:[a-zA-Z0-9-_.]{1,255}}", statsMiddleware(ag.handleGetX5u, "http.api.x5u", stats)).Methods("GET") if os.Getenv("AUTOGRAPH_PROFILE") == "1" { err = setRuntimeConfig() if err != nil { diff --git a/monitor_handler.go b/monitor_handler.go index 745ce17fc..7965cc7b3 100644 --- a/monitor_handler.go +++ b/monitor_handler.go @@ -47,6 +47,7 @@ func (m *monitor) handleMonitor(w http.ResponseWriter, r *http.Request) { enc := json.NewEncoder(w) for _, response := range m.sigresps { + response.X5U = mirrorLocalX5U(r, response) if err := enc.Encode(&response); err != nil { httpError(w, r, http.StatusInternalServerError, "encoding failed with error: %v", err) return diff --git a/signer/signer.go b/signer/signer.go index 36b43012b..16d5e843b 100644 --- a/signer/signer.go +++ b/signer/signer.go @@ -34,6 +34,9 @@ import ( // IDFormat is a regex for the format IDs must follow const IDFormat = `^[a-zA-Z0-9-_]{1,64}$` +// IDFormatRegexp is the compiled regex from IDFormat +var IDFormatRegexp = regexp.MustCompile(IDFormat) + // RecommendationConfig is a config for the XPI recommendation file type RecommendationConfig struct { // AllowedStates is a map of strings the signer is allowed to From d36d4dc75e033f990cc6406592d9bf928b0aa789 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Sat, 6 Jul 2024 10:42:20 -0700 Subject: [PATCH 02/15] Stop using /tmp to pass files between containers --- bin/run_integration_tests.sh | 3 +-- docker-compose.yml | 4 ---- tools/autograph-monitor/monitor.go | 3 ++- tools/softhsm/Dockerfile | 5 +---- tools/softhsm/hash_signer_cacert.sh | 24 ------------------------ 5 files changed, 4 insertions(+), 35 deletions(-) delete mode 100755 tools/softhsm/hash_signer_cacert.sh diff --git a/bin/run_integration_tests.sh b/bin/run_integration_tests.sh index 1df2aced3..9b817f22b 100755 --- a/bin/run_integration_tests.sh +++ b/bin/run_integration_tests.sh @@ -25,8 +25,7 @@ while test "true" != "$(docker inspect -f {{.State.Running}} autograph-app-hsm)" done # fetch the updated root hash from the app-hsm service -docker cp autograph-app-hsm:/tmp/normandy_dev_root_hash.txt . -APP_HSM_NORMANDY_ROOT_HASH=$(grep '[0-9A-F]' normandy_dev_root_hash.txt | tr -d '\r\n') +APP_HSM_NORMANDY_ROOT_HASH=$(docker compose exec app-hsm yq -r '.signers[] | select(.id == "normandy").cacert' /app/autograph.softhsm.yaml | openssl x509 -outform DER | sha256sum | awk '{print $1}') # start the monitor lambda emulators echo "checking autograph monitors" diff --git a/docker-compose.yml b/docker-compose.yml index 237b99332..fe9858de0 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -87,8 +87,6 @@ services: - app depends_on: - app - volumes: - - apptmpdir:/tmp/ monitor-hsm-lambda-emulator: container_name: autograph-monitor-hsm-lambda-emulator @@ -109,8 +107,6 @@ services: - app-hsm depends_on: - app-hsm - volumes: - - hsmtmpdir:/tmp/ unit-test: container_name: autograph-unit-test diff --git a/tools/autograph-monitor/monitor.go b/tools/autograph-monitor/monitor.go index 6aa998eba..c4a8f1619 100644 --- a/tools/autograph-monitor/monitor.go +++ b/tools/autograph-monitor/monitor.go @@ -10,6 +10,7 @@ import ( "net/http" "os" "runtime" + "strings" "time" "github.com/aws/aws-lambda-go/lambda" @@ -93,7 +94,7 @@ func main() { conf.depTruststore = nil } if os.Getenv("AUTOGRAPH_ROOT_HASH") != "" { - conf.rootHash = os.Getenv("AUTOGRAPH_ROOT_HASH") + conf.rootHash = strings.ToUpper(os.Getenv("AUTOGRAPH_ROOT_HASH")) conf.contentSignatureRootHash = conf.rootHash log.Printf("Using root hash from env var AUTOGRAPH_ROOT_HASH=%q\n", conf.rootHash) } diff --git a/tools/softhsm/Dockerfile b/tools/softhsm/Dockerfile index 9175ce596..adc63f78b 100644 --- a/tools/softhsm/Dockerfile +++ b/tools/softhsm/Dockerfile @@ -48,9 +48,6 @@ RUN cd /app/src/autograph/tools/genpki/ && \ python3 configurator.py -c /app/autograph.softhsm.yaml -i -s kinto \ -p issuercert -v "$(grep 'inter cert path' /app/genpki.out | awk '{print $4}')" && \ python3 configurator.py -c /app/autograph.softhsm.yaml -i -s kinto \ - -p cacert -v "$(grep 'root cert path' /app/genpki.out | awk '{print $4}')" && \ - cp /app/autograph.softhsm.yaml /tmp/ && \ - /bin/bash /app/src/autograph/tools/softhsm/hash_signer_cacert.sh /app/autograph.softhsm.yaml normandy > /tmp/normandy_dev_root_hash.txt && \ - cat /tmp/normandy_dev_root_hash.txt + -p cacert -v "$(grep 'root cert path' /app/genpki.out | awk '{print $4}')" CMD /go/bin/autograph -c /app/autograph.softhsm.yaml diff --git a/tools/softhsm/hash_signer_cacert.sh b/tools/softhsm/hash_signer_cacert.sh deleted file mode 100755 index 05eb79875..000000000 --- a/tools/softhsm/hash_signer_cacert.sh +++ /dev/null @@ -1,24 +0,0 @@ -#!/bin/bash - -# Print the SHA256 hash of the contentsignature pki cacert in an autograph config file with the given name -# -# requires: -# -# awk, jq, and openssl commands -# the python yq command -# -# usage: -# -# ./hash_signer_cacert.sh ../../softhsm/autograph.softhsm.yaml normandy -# EB1D805F4566F38EADC4BF826400F3DC263E2A328A1B597F69D832FF993D00AB -# -[ -z "$2" ] && echo "$0 " && echo "returns the SHA256 of the CACERT" && exit 1 -[ ! -r "$1" ] && echo "configuration file '$1' not found" && exit 1 - -yqcmd=(yq -r -c ".signers | (map(select(.id | contains (\"$2\")))) | .[] .cacert") - -ROOT_HASH=$("${yqcmd[@]}" "$1" | \ - openssl x509 -outform der | \ - openssl dgst -sha256 -hex | \ - awk '{print $2}' | tr '[:lower:]' '[:upper:]') -echo "$ROOT_HASH" From b6d1a5ff3c9ad11a014c475599f2c7cbb14e7480 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Sat, 6 Jul 2024 10:53:10 -0700 Subject: [PATCH 03/15] Clean up some formatting --- bin/run_integration_tests.sh | 3 ++- handlers.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/bin/run_integration_tests.sh b/bin/run_integration_tests.sh index 9b817f22b..6e46d3021 100755 --- a/bin/run_integration_tests.sh +++ b/bin/run_integration_tests.sh @@ -25,7 +25,8 @@ while test "true" != "$(docker inspect -f {{.State.Running}} autograph-app-hsm)" done # fetch the updated root hash from the app-hsm service -APP_HSM_NORMANDY_ROOT_HASH=$(docker compose exec app-hsm yq -r '.signers[] | select(.id == "normandy").cacert' /app/autograph.softhsm.yaml | openssl x509 -outform DER | sha256sum | awk '{print $1}') +APP_HSM_NORMANDY_ROOT_HASH=$(docker compose exec app-hsm yq -r '.signers[] | select(.id == "normandy").cacert' /app/autograph.softhsm.yaml | \ + openssl x509 -outform DER | sha256sum | awk '{print $1}') # start the monitor lambda emulators echo "checking autograph monitors" diff --git a/handlers.go b/handlers.go index 00e4028cc..8e9432fa4 100644 --- a/handlers.go +++ b/handlers.go @@ -80,7 +80,7 @@ func mirrorLocalX5U(r *http.Request, response formats.SignatureResponse) string parsedX5U, err := url.Parse(response.X5U) if err == nil && parsedX5U.Scheme == "file" { mirroredX5U := url.URL{ - Scheme: "http", + Scheme: "http", Host: r.Host, Path: path.Join("x5u", response.SignerID, path.Base(parsedX5U.Path)), } From 37b185627eeac267de42f02c1ba738a19ef2774b Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Sat, 6 Jul 2024 11:14:20 -0700 Subject: [PATCH 04/15] Add extra check to reject path escape shenanigans --- handlers.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/handlers.go b/handlers.go index 8e9432fa4..1514a2fd9 100644 --- a/handlers.go +++ b/handlers.go @@ -17,6 +17,7 @@ import ( "net/url" "os" "path" + "strings" "time" "github.com/gorilla/mux" @@ -546,6 +547,13 @@ func (a *autographer) handleGetX5u(w http.ResponseWriter, r *http.Request) { return } + // The handler regex should reject such paths, but let's be extra certain that we have been given + // a path without any attempt to escape the X5U upload directory. + if strings.Contains(pathChainFile, "/") || strings.Contains(pathChainFile, "\\") || strings.Contains(pathChainFile, "..") { + httpError(w, r, http.StatusBadRequest, "Invalid X5U file name '%s'", pathChainFile) + return + } + // Lookup the signer, and see if it has a local X5U chain upload location. // Treat all errors as a 404 to avoid leaking unnecessary signer details as this // endpoint has no authentication. From 9f384e39624db9a637907070e9b759cb619c23ff Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Sun, 7 Jul 2024 10:48:45 -0700 Subject: [PATCH 05/15] Fix the monitor racing test --- handlers.go | 2 +- monitor_handler_racing_test.go | 21 ++++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/handlers.go b/handlers.go index 1514a2fd9..018b96fd1 100644 --- a/handlers.go +++ b/handlers.go @@ -510,7 +510,7 @@ func (a *autographer) handleGetAuthKeyIDs(w http.ResponseWriter, r *http.Request w.Write(signerIDsJSON) } -func (a *autographer) handleGetX5u(w http.ResponseWriter, r *http.Request) { +func (a *autographer) handleGetX5U(w http.ResponseWriter, r *http.Request) { if r.Method != "GET" { httpError(w, r, http.StatusMethodNotAllowed, "%s method not allowed; endpoint accepts GET only", r.Method) return diff --git a/monitor_handler_racing_test.go b/monitor_handler_racing_test.go index 1bfaecdc0..4c041902e 100644 --- a/monitor_handler_racing_test.go +++ b/monitor_handler_racing_test.go @@ -8,12 +8,14 @@ import ( "crypto/sha256" "encoding/base64" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" "strings" "testing" + "github.com/gorilla/mux" "github.com/mozilla-services/autograph/formats" "github.com/mozilla-services/autograph/signer/apk2" "github.com/mozilla-services/autograph/signer/contentsignature" @@ -28,6 +30,23 @@ import ( const autographDevRootHash = `5E:36:F2:14:DE:82:3F:8B:29:96:89:23:5F:03:41:AC:AF:A0:75:AF:82:CB:4C:D4:30:7C:3D:B3:43:39:2A:FE` +func getMirroredX5U(x5u string) (body []byte, err error) { + req, err := http.NewRequest("GET", x5u, nil) + if err != nil { + return nil, err + } + segs := strings.Split(req.URL.Path, "/") + req = mux.SetURLVars(req, map[string]string{"keyid": segs[len(segs)-2], "chainfile": segs[len(segs)-1]}) + + w := httptest.NewRecorder() + ag.handleGetX5U(w, req) + if w.Code != http.StatusOK || w.Body.String() == "" { + return nil, fmt.Errorf("fetch x5u failed with %d: %s; request was: %+v", w.Code, w.Body.String(), req) + } + + return w.Body.Bytes(), nil +} + func TestMonitorPass(t *testing.T) { t.Parallel() @@ -64,7 +83,7 @@ func TestMonitorPass(t *testing.T) { t.Fatalf("verification of monitoring response failed: %v", err) } case contentsignaturepki.Type: - body, _, err := contentsignaturepki.GetX5U(&http.Client{}, response.X5U) + body, err := getMirroredX5U(response.X5U) if err != nil { t.Fatal(err) } From ad9598e3142fad7cf128d79501eb5dd2e7b45ed0 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Mon, 19 Aug 2024 16:19:39 -0700 Subject: [PATCH 06/15] asdas --- main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.go b/main.go index 59ced4ea0..0b49c8df3 100755 --- a/main.go +++ b/main.go @@ -214,7 +214,7 @@ func run(conf configuration, listen string, debug bool) { router.HandleFunc("/sign/data", statsMiddleware(ag.handleSignature, "http.api.sign/data", stats)).Methods("POST") router.HandleFunc("/sign/hash", statsMiddleware(ag.handleSignature, "http.api.sign/hash", stats)).Methods("POST") router.HandleFunc("/auths/{auth_id:[a-zA-Z0-9-_]{1,255}}/keyids", statsMiddleware(ag.handleGetAuthKeyIDs, "http.api.getauthkeyids", stats)).Methods("GET") - router.HandleFunc("/x5u/{keyid:[a-zA-Z0-9-_]{1,64}}/{chainfile:[a-zA-Z0-9-_.]{1,255}}", statsMiddleware(ag.handleGetX5u, "http.api.x5u", stats)).Methods("GET") + router.HandleFunc("/x5u/{keyid:[a-zA-Z0-9-_]{1,64}}/{chainfile:[a-zA-Z0-9-_.]{1,255}}", statsMiddleware(ag.handleGetX5U, "http.api.x5u", stats)).Methods("GET") if os.Getenv("AUTOGRAPH_PROFILE") == "1" { err = setRuntimeConfig() if err != nil { From a65d2e1f1abab9141caa74585f12222010d5c303 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Fri, 12 Jul 2024 08:50:29 -0700 Subject: [PATCH 07/15] Add some tests for the x5u endpoint --- handlers_test.go | 351 +++++++++++++++++++++++++++++++++++++---------- 1 file changed, 278 insertions(+), 73 deletions(-) diff --git a/handlers_test.go b/handlers_test.go index 1d5d4bca2..92c4f4b34 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -20,6 +20,7 @@ import ( "log" "net/http" "net/http/httptest" + "os" "strings" "testing" "time" @@ -37,6 +38,97 @@ import ( margo "go.mozilla.org/mar" ) +type HandlerTestCase struct { + name string + method string + url string + + // urlRouteVars are https://pkg.go.dev/github.com/gorilla/mux#Vars + // as configured with the handler at /config/{keyid:[a-zA-Z0-9-_]{1,64}} + // there should only be a keyid var and it should match the url value + urlRouteVars map[string]string + + // headers are additional http headers to set + headers *http.Header + + // user/auth ID to build an Authorization header for + authorizeID string + nilBody bool + body string + + expectedStatus int + expectedHeaders http.Header + expectedBody string +} + +func (testcase *HandlerTestCase) NewRequest(t *testing.T) *http.Request { + // test request setup + var ( + req *http.Request + err error + ) + if testcase.nilBody { + req, err = http.NewRequest(testcase.method, testcase.url, nil) + } else { + req, err = http.NewRequest(testcase.method, testcase.url, strings.NewReader(testcase.body)) + } + if err != nil { + t.Fatal(err) + } + req = mux.SetURLVars(req, testcase.urlRouteVars) + if testcase.headers != nil { + req.Header = *testcase.headers + } + + if testcase.authorizeID != "" { + auth, err := ag.getAuthByID(testcase.authorizeID) + if err != nil { + t.Fatal(err) + } + // getAuthHeader requires a content type and body + req.Header.Set("Authorization", hawk.NewRequestAuth(req, + &hawk.Credentials{ + ID: auth.ID, + Key: auth.Key, + Hash: sha256.New}, + 0).RequestHeader()) + } + + return req +} + +func (testcase* HandlerTestCase) ValidateResponse(t *testing.T, w *httptest.ResponseRecorder) { + if w.Code != testcase.expectedStatus { + t.Fatalf("test case %s: got code %d but expected %d", + testcase.name, w.Code, testcase.expectedStatus) + } + if w.Body.String() != testcase.expectedBody { + t.Fatalf("test case %s: got body %q expected %q", testcase.name, w.Body.String(), testcase.expectedBody) + } + for expectedHeader, expectedHeaderVals := range testcase.expectedHeaders { + vals, ok := w.Header()[expectedHeader] + if !ok { + t.Fatalf("test case %s: expected header %q not found", testcase.name, expectedHeader) + } + if strings.Join(vals, "") != strings.Join(expectedHeaderVals, "") { + t.Fatalf("test case %s: header vals %q did not match expected %q ", testcase.name, vals, expectedHeaderVals) + } + } +} + +func (testcase* HandlerTestCase) Run(t *testing.T, handler func(http.ResponseWriter, *http.Request)) { + // test request setup + var req = testcase.NewRequest(t) + + // run the request + w := httptest.NewRecorder() + handler(w, req) + + // validate response + testcase.ValidateResponse(t, w) +} + + func TestBadRequest(t *testing.T) { t.Parallel() @@ -575,28 +667,7 @@ func TestHandleGetAuthKeyIDs(t *testing.T) { const autographDevAliceKeyIDsJSON = "[\"apk_cert_with_ecdsa_sha256\",\"apk_cert_with_ecdsa_sha256_v3\",\"appkey1\",\"appkey2\",\"dummyrsa\",\"dummyrsapss\",\"extensions-ecdsa\",\"extensions-ecdsa-expired-chain\",\"legacy_apk_with_rsa\",\"normandy\",\"pgpsubkey\",\"pgpsubkey-debsign\",\"randompgp\",\"randompgp-debsign\",\"remote-settings\",\"testapp-android\",\"testapp-android-legacy\",\"testapp-android-v3\",\"testauthenticode\",\"testmar\",\"testmarecdsa\",\"webextensions-rsa\",\"webextensions-rsa-with-recommendation\"]" - var testcases = []struct { - name string - method string - url string - - // urlRouteVars are https://pkg.go.dev/github.com/gorilla/mux#Vars - // as configured with the handler at /auths/{auth_id:[a-zA-Z0-9-_]{1,255}}/keyids - // there should only be an auth_id var and it should match the url value - urlRouteVars map[string]string - - // headers are additional http headers to set - headers *http.Header - - // user/auth ID to build an Authorization header for - authorizeID string - nilBody bool - body string - - expectedStatus int - expectedHeaders http.Header - expectedBody string - }{ + var testcases = []HandlerTestCase { { name: "invalid method POST returns 405", method: "POST", @@ -720,60 +791,194 @@ func TestHandleGetAuthKeyIDs(t *testing.T) { expectedHeaders: http.Header{"Content-Type": []string{"application/json"}}, }, } - for i, testcase := range testcases { - // test request setup - var ( - req *http.Request - err error - ) - if testcase.nilBody { - req, err = http.NewRequest(testcase.method, testcase.url, nil) - } else { - req, err = http.NewRequest(testcase.method, testcase.url, strings.NewReader(testcase.body)) - } - if err != nil { - t.Fatal(err) - } - req = mux.SetURLVars(req, testcase.urlRouteVars) + for _, testcase := range testcases { + testcase.Run(t, ag.handleGetAuthKeyIDs) + } +} - if testcase.headers != nil { - req.Header = *testcase.headers - } - if testcase.authorizeID != "" { - auth, err := ag.getAuthByID(testcase.authorizeID) - if err != nil { - t.Fatal(err) - } - // getAuthHeader requires a content type and body - req.Header.Set("Authorization", hawk.NewRequestAuth(req, - &hawk.Credentials{ - ID: auth.ID, - Key: auth.Key, - Hash: sha256.New}, - 0).RequestHeader()) - } +func TestHandleGetX5U(t *testing.T) { + t.Parallel() - // run the request - w := httptest.NewRecorder() - ag.handleGetAuthKeyIDs(w, req) + // This is a bit hacky, but the config is set to use this path for the X5U normandy + // upload location. But to untangle this kind of config dependency we need to be able + // to mock out the signers - and that is hard. + const normandyTestChainFile = "/tmp/autograph/chains/normandydev/example.pem" + const normandyTestChainContents = ` +-----BEGIN CERTIFICATE----- +MIICXDCCAeKgAwIBAgIIFYW6xg9HrnAwCgYIKoZIzj0EAwMwXzELMAkGA1UEBhMC +VVMxCzAJBgNVBAgTAkNBMRYwFAYDVQQHEw1Nb3VudGFpbiBWaWV3MRAwDgYDVQQK +EwdNb3ppbGxhMRkwFwYDVQQDExBjc3Jvb3QxNTUwODUxMDA2MB4XDTE4MTIyMTE1 +NTY0NloXDTI5MDIyMjE1NTY0NlowYDELMAkGA1UEBhMCVVMxCzAJBgNVBAgTAkNB +MRYwFAYDVQQHEw1Nb3VudGFpbiBWaWV3MRAwDgYDVQQKEwdNb3ppbGxhMRowGAYD +VQQDExFjc2ludGVyMTU1MDg1MTAwNjB2MBAGByqGSM49AgEGBSuBBAAiA2IABAwF +9wOPiv/1oBdxSyOO6fe8KkFJCiyRx2KIXhsT4BwWY8AGHoCfBNm/Swdg+OSi+TdH +dF+5eUrKiqG4PvdWoGGS4rtHqY3ayeF9GRaaLpLMdZkhc/MVJygJoecmsXM2O6Nq +MGgwDgYDVR0PAQH/BAQDAgGGMBMGA1UdJQQMMAoGCCsGAQUFBwMDMA8GA1UdEwEB +/wQFMAMBAf8wMAYDVR0eAQH/BCYwJKAiMCCCHi5jb250ZW50LXNpZ25hdHVyZS5t +b3ppbGxhLm9yZzAKBggqhkjOPQQDAwNoADBlAjBss+GLdMdLT2Y/g73OE9x0WyUG +vqzO7klt20yytmhaYMIPT/zRnWsHZbqEijHMzGsCMQDEoKetuWkyBkzAytS6l+ss +mYigBlwySY+gTqsjuIrydWlKaOv1GU+PXbwX0cQuaN8= +-----END CERTIFICATE-----` + err := os.WriteFile(normandyTestChainFile, []byte(normandyTestChainContents), 0644) + if err != nil { + t.Fatal(err) + } - // validate response - if w.Code != testcase.expectedStatus { - t.Fatalf("test case %s (%d): got code %d but expected %d", - testcase.name, i, w.Code, testcase.expectedStatus) - } - if w.Body.String() != testcase.expectedBody { - t.Fatalf("test case %s (%d): got body %q expected %q", testcase.name, i, w.Body.String(), testcase.expectedBody) - } - for expectedHeader, expectedHeaderVals := range testcase.expectedHeaders { - vals, ok := w.Header()[expectedHeader] - if !ok { - t.Fatalf("test case %s (%d): expected header %q not found", testcase.name, i, expectedHeader) - } - if strings.Join(vals, "") != strings.Join(expectedHeaderVals, "") { - t.Fatalf("test case %s (%d): header vals %q did not match expected %q ", testcase.name, i, vals, expectedHeaderVals) - } - } + var testcases = []HandlerTestCase { + { + 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"}}, + }, + { + name: "GET with misconfigured keyid route param returns 500", + method: "GET", + url: "http://foo.bar/x5u/normandy/example.pem", + urlRouteVars: map[string]string{"chainfile": "example.pem"}, // missing keyid + nilBody: true, + expectedStatus: http.StatusInternalServerError, + expectedBody: "route is improperly configured\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with misconfigured chainfile route param returns 500", + method: "GET", + url: "http://foo.bar/x5u/normandy/example.pem", + urlRouteVars: map[string]string{"keyid": "normandy"}, // missing chainfile + nilBody: true, + expectedStatus: http.StatusInternalServerError, + expectedBody: "route is improperly configured\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with directory parent escapes returns 400", + method: "GET", + url: "http://foo.bar/x5u/normandy/../example.pem", + urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "../example.pem"}, + nilBody: true, + expectedStatus: http.StatusBadRequest, + expectedBody: "Invalid X5U file name '../example.pem'\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with directory root escapes returns 400", + method: "GET", + url: "http://foo.bar/x5u/normandy//example.pem", + urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "//example.pem"}, + nilBody: true, + expectedStatus: http.StatusBadRequest, + expectedBody: "Invalid X5U file name '//example.pem'\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with extra directory path returns 400", + method: "GET", + url: "http://foo.bar/x5u/normandy/extra/example.pem", + urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "extra/example.pem"}, + nilBody: true, + expectedStatus: http.StatusBadRequest, + expectedBody: "Invalid X5U file name 'extra/example.pem'\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with backslash path returns 400", + method: "GET", + url: "http://foo.bar/x5u/normandy/extra\\example.pem", + urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "extra\\example.pem"}, + nilBody: true, + expectedStatus: http.StatusBadRequest, + expectedBody: "Invalid X5U file name 'extra\\example.pem'\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with empty body returns 200", + method: "GET", + url: "http://foo.bar/x5u/normandy/example.pem", + urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "example.pem"}, + nilBody: false, + body: "", + expectedStatus: http.StatusOK, + expectedBody: normandyTestChainContents, + expectedHeaders: http.Header{"Content-Type": []string{"application/x-x509-ca-cert"}}, + }, + { + name: "GET with non-empty body returns 400", + method: "GET", + url: "http://foo.bar/x5u/normandy/example.pem", + urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "example.pem"}, + nilBody: false, + body: "foobar/---", + expectedStatus: http.StatusBadRequest, + expectedBody: "endpoint received unexpected request body\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with unknown signer returns 404", + method: "GET", + url: "http://foo.bar/x5u/normandyxyz/example.pem", + urlRouteVars: map[string]string{"keyid": "normandyxyz", "chainfile": "example.pem"}, + nilBody: false, + body: "", + expectedStatus: http.StatusNotFound, + expectedBody: "404 page not found\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with unknown chain returns 404", + method: "GET", + url: "http://foo.bar/x5u/normandy/example.pem", + urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "notfound.pem"}, + nilBody: false, + body: "", + expectedStatus: http.StatusNotFound, + expectedBody: "404 page not found\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + { + name: "GET with non-PKI signer returns 404", + method: "GET", + url: "http://foo.bar/x5u/randompgp/example.pem", + urlRouteVars: map[string]string{"keyid": "randompgp", "chainfile": "example.pem"}, + nilBody: false, + body: "", + expectedStatus: http.StatusNotFound, + expectedBody: "404 page not found\r\nrequest-id: -\n", + expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, + }, + } + for _, testcase := range testcases { + testcase.Run(t, ag.handleGetX5U) } } From 5b4125ad90f9cce6d65fd568f5eaee7537f0abb6 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Fri, 12 Jul 2024 10:06:26 -0700 Subject: [PATCH 08/15] Run go fmt --- handlers_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/handlers_test.go b/handlers_test.go index 92c4f4b34..e6330e0ac 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -97,7 +97,7 @@ func (testcase *HandlerTestCase) NewRequest(t *testing.T) *http.Request { return req } -func (testcase* HandlerTestCase) ValidateResponse(t *testing.T, w *httptest.ResponseRecorder) { +func (testcase *HandlerTestCase) ValidateResponse(t *testing.T, w *httptest.ResponseRecorder) { if w.Code != testcase.expectedStatus { t.Fatalf("test case %s: got code %d but expected %d", testcase.name, w.Code, testcase.expectedStatus) @@ -116,10 +116,10 @@ func (testcase* HandlerTestCase) ValidateResponse(t *testing.T, w *httptest.Resp } } -func (testcase* HandlerTestCase) Run(t *testing.T, handler func(http.ResponseWriter, *http.Request)) { +func (testcase *HandlerTestCase) Run(t *testing.T, handler func(http.ResponseWriter, *http.Request)) { // test request setup var req = testcase.NewRequest(t) - + // run the request w := httptest.NewRecorder() handler(w, req) @@ -128,7 +128,6 @@ func (testcase* HandlerTestCase) Run(t *testing.T, handler func(http.ResponseWri testcase.ValidateResponse(t, w) } - func TestBadRequest(t *testing.T) { t.Parallel() @@ -667,7 +666,7 @@ func TestHandleGetAuthKeyIDs(t *testing.T) { const autographDevAliceKeyIDsJSON = "[\"apk_cert_with_ecdsa_sha256\",\"apk_cert_with_ecdsa_sha256_v3\",\"appkey1\",\"appkey2\",\"dummyrsa\",\"dummyrsapss\",\"extensions-ecdsa\",\"extensions-ecdsa-expired-chain\",\"legacy_apk_with_rsa\",\"normandy\",\"pgpsubkey\",\"pgpsubkey-debsign\",\"randompgp\",\"randompgp-debsign\",\"remote-settings\",\"testapp-android\",\"testapp-android-legacy\",\"testapp-android-v3\",\"testauthenticode\",\"testmar\",\"testmarecdsa\",\"webextensions-rsa\",\"webextensions-rsa-with-recommendation\"]" - var testcases = []HandlerTestCase { + var testcases = []HandlerTestCase{ { name: "invalid method POST returns 405", method: "POST", @@ -824,7 +823,7 @@ mYigBlwySY+gTqsjuIrydWlKaOv1GU+PXbwX0cQuaN8= t.Fatal(err) } - var testcases = []HandlerTestCase { + var testcases = []HandlerTestCase{ { name: "invalid method POST returns 405", method: "POST", From f40b5e22e8b82920068be216c32c306fcd2c81bf Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Mon, 19 Aug 2024 17:30:27 -0700 Subject: [PATCH 09/15] Implement using http.FileServer instead --- main.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/main.go b/main.go index 0b49c8df3..9f7ae0319 100755 --- a/main.go +++ b/main.go @@ -13,6 +13,7 @@ import ( "flag" "fmt" "net/http" + "net/url" "os" "os/signal" "regexp" @@ -214,7 +215,6 @@ func run(conf configuration, listen string, debug bool) { router.HandleFunc("/sign/data", statsMiddleware(ag.handleSignature, "http.api.sign/data", stats)).Methods("POST") router.HandleFunc("/sign/hash", statsMiddleware(ag.handleSignature, "http.api.sign/hash", stats)).Methods("POST") router.HandleFunc("/auths/{auth_id:[a-zA-Z0-9-_]{1,255}}/keyids", statsMiddleware(ag.handleGetAuthKeyIDs, "http.api.getauthkeyids", stats)).Methods("GET") - router.HandleFunc("/x5u/{keyid:[a-zA-Z0-9-_]{1,64}}/{chainfile:[a-zA-Z0-9-_.]{1,255}}", statsMiddleware(ag.handleGetX5U, "http.api.x5u", stats)).Methods("GET") if os.Getenv("AUTOGRAPH_PROFILE") == "1" { err = setRuntimeConfig() if err != nil { @@ -224,6 +224,19 @@ func run(conf configuration, listen string, debug bool) { log.Infof("enabled HTTP perf profiler") } + // For each signer with a local chain upload location (eg: using the file + // scheme) create an handler to serve that directory at the path /x5u/keyid/ + for _, signerConf := range conf.Signers { + parsedURL, err := url.Parse(signerConf.ChainUploadLocation) + if err != nil || parsedURL.Scheme != "file" { + // This signer doesn't upload certificate chains to local storage. + continue + } + + prefix := fmt.Sprintf("/x5u/%s/", signerConf.ID) + router.PathPrefix(prefix).Handler(http.StripPrefix(prefix, http.FileServer(http.Dir(parsedURL.Path)))) + } + server := &http.Server{ IdleTimeout: conf.Server.IdleTimeout, ReadTimeout: conf.Server.ReadTimeout, From 8910ed71b19ff1582eb4642f70d6ab67d3f5dc89 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Mon, 19 Aug 2024 18:22:37 -0700 Subject: [PATCH 10/15] Remove handleGetX5U --- handlers.go | 73 ------------- handlers_test.go | 187 --------------------------------- monitor_handler_racing_test.go | 37 +++++-- 3 files changed, 26 insertions(+), 271 deletions(-) diff --git a/handlers.go b/handlers.go index 018b96fd1..8e8c4fc62 100644 --- a/handlers.go +++ b/handlers.go @@ -509,76 +509,3 @@ func (a *autographer) handleGetAuthKeyIDs(w http.ResponseWriter, r *http.Request w.WriteHeader(http.StatusOK) w.Write(signerIDsJSON) } - -func (a *autographer) handleGetX5U(w http.ResponseWriter, r *http.Request) { - if r.Method != "GET" { - httpError(w, r, http.StatusMethodNotAllowed, "%s method not allowed; endpoint accepts GET only", r.Method) - return - } - if r.Body != nil { - body, err := io.ReadAll(r.Body) - if err != nil { - httpError(w, r, http.StatusBadRequest, "failed to read request body: %s", err) - return - } - if len(body) > 0 { - httpError(w, r, http.StatusBadRequest, "endpoint received unexpected request body") - return - } - } - - pathKeyID, ok := mux.Vars(r)["keyid"] - if !ok { - httpError(w, r, http.StatusInternalServerError, "route is improperly configured") - return - } - if !signer.IDFormatRegexp.MatchString(pathKeyID) { - httpError(w, r, http.StatusBadRequest, "keyid in URL path '%s' is invalid, it must match %s", pathKeyID, signer.IDFormat) - return - } - - pathChainFile, ok := mux.Vars(r)["chainfile"] - if !ok { - httpError(w, r, http.StatusInternalServerError, "route is improperly configured") - return - } - if !signer.IDFormatRegexp.MatchString(pathKeyID) { - httpError(w, r, http.StatusBadRequest, "keyid in URL path '%s' is invalid, it must match %s", pathKeyID, signer.IDFormat) - return - } - - // The handler regex should reject such paths, but let's be extra certain that we have been given - // a path without any attempt to escape the X5U upload directory. - if strings.Contains(pathChainFile, "/") || strings.Contains(pathChainFile, "\\") || strings.Contains(pathChainFile, "..") { - httpError(w, r, http.StatusBadRequest, "Invalid X5U file name '%s'", pathChainFile) - return - } - - // Lookup the signer, and see if it has a local X5U chain upload location. - // Treat all errors as a 404 to avoid leaking unnecessary signer details as this - // endpoint has no authentication. - for _, s := range a.getSigners() { - config := s.Config() - if config.ID != pathKeyID { - continue - } - - // Check if the signer has a chain upload location, and the requested file exists. - parsedURL, err := url.Parse(config.ChainUploadLocation) - if err != nil || parsedURL.Scheme != "file" { - break - } - 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) - return - } - - log.Infof("X5U not found for signer: %s", pathKeyID) - httpError(w, r, http.StatusNotFound, "404 page not found") -} diff --git a/handlers_test.go b/handlers_test.go index e6330e0ac..8887dab0b 100644 --- a/handlers_test.go +++ b/handlers_test.go @@ -20,7 +20,6 @@ import ( "log" "net/http" "net/http/httptest" - "os" "strings" "testing" "time" @@ -795,192 +794,6 @@ func TestHandleGetAuthKeyIDs(t *testing.T) { } } -func TestHandleGetX5U(t *testing.T) { - t.Parallel() - - // This is a bit hacky, but the config is set to use this path for the X5U normandy - // upload location. But to untangle this kind of config dependency we need to be able - // to mock out the signers - and that is hard. - const normandyTestChainFile = "/tmp/autograph/chains/normandydev/example.pem" - const normandyTestChainContents = ` ------BEGIN CERTIFICATE----- -MIICXDCCAeKgAwIBAgIIFYW6xg9HrnAwCgYIKoZIzj0EAwMwXzELMAkGA1UEBhMC -VVMxCzAJBgNVBAgTAkNBMRYwFAYDVQQHEw1Nb3VudGFpbiBWaWV3MRAwDgYDVQQK -EwdNb3ppbGxhMRkwFwYDVQQDExBjc3Jvb3QxNTUwODUxMDA2MB4XDTE4MTIyMTE1 -NTY0NloXDTI5MDIyMjE1NTY0NlowYDELMAkGA1UEBhMCVVMxCzAJBgNVBAgTAkNB -MRYwFAYDVQQHEw1Nb3VudGFpbiBWaWV3MRAwDgYDVQQKEwdNb3ppbGxhMRowGAYD -VQQDExFjc2ludGVyMTU1MDg1MTAwNjB2MBAGByqGSM49AgEGBSuBBAAiA2IABAwF -9wOPiv/1oBdxSyOO6fe8KkFJCiyRx2KIXhsT4BwWY8AGHoCfBNm/Swdg+OSi+TdH -dF+5eUrKiqG4PvdWoGGS4rtHqY3ayeF9GRaaLpLMdZkhc/MVJygJoecmsXM2O6Nq -MGgwDgYDVR0PAQH/BAQDAgGGMBMGA1UdJQQMMAoGCCsGAQUFBwMDMA8GA1UdEwEB -/wQFMAMBAf8wMAYDVR0eAQH/BCYwJKAiMCCCHi5jb250ZW50LXNpZ25hdHVyZS5t -b3ppbGxhLm9yZzAKBggqhkjOPQQDAwNoADBlAjBss+GLdMdLT2Y/g73OE9x0WyUG -vqzO7klt20yytmhaYMIPT/zRnWsHZbqEijHMzGsCMQDEoKetuWkyBkzAytS6l+ss -mYigBlwySY+gTqsjuIrydWlKaOv1GU+PXbwX0cQuaN8= ------END CERTIFICATE-----` - err := os.WriteFile(normandyTestChainFile, []byte(normandyTestChainContents), 0644) - if err != nil { - t.Fatal(err) - } - - var testcases = []HandlerTestCase{ - { - 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"}}, - }, - { - name: "GET with misconfigured keyid route param returns 500", - method: "GET", - url: "http://foo.bar/x5u/normandy/example.pem", - urlRouteVars: map[string]string{"chainfile": "example.pem"}, // missing keyid - nilBody: true, - expectedStatus: http.StatusInternalServerError, - expectedBody: "route is improperly configured\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with misconfigured chainfile route param returns 500", - method: "GET", - url: "http://foo.bar/x5u/normandy/example.pem", - urlRouteVars: map[string]string{"keyid": "normandy"}, // missing chainfile - nilBody: true, - expectedStatus: http.StatusInternalServerError, - expectedBody: "route is improperly configured\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with directory parent escapes returns 400", - method: "GET", - url: "http://foo.bar/x5u/normandy/../example.pem", - urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "../example.pem"}, - nilBody: true, - expectedStatus: http.StatusBadRequest, - expectedBody: "Invalid X5U file name '../example.pem'\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with directory root escapes returns 400", - method: "GET", - url: "http://foo.bar/x5u/normandy//example.pem", - urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "//example.pem"}, - nilBody: true, - expectedStatus: http.StatusBadRequest, - expectedBody: "Invalid X5U file name '//example.pem'\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with extra directory path returns 400", - method: "GET", - url: "http://foo.bar/x5u/normandy/extra/example.pem", - urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "extra/example.pem"}, - nilBody: true, - expectedStatus: http.StatusBadRequest, - expectedBody: "Invalid X5U file name 'extra/example.pem'\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with backslash path returns 400", - method: "GET", - url: "http://foo.bar/x5u/normandy/extra\\example.pem", - urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "extra\\example.pem"}, - nilBody: true, - expectedStatus: http.StatusBadRequest, - expectedBody: "Invalid X5U file name 'extra\\example.pem'\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with empty body returns 200", - method: "GET", - url: "http://foo.bar/x5u/normandy/example.pem", - urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "example.pem"}, - nilBody: false, - body: "", - expectedStatus: http.StatusOK, - expectedBody: normandyTestChainContents, - expectedHeaders: http.Header{"Content-Type": []string{"application/x-x509-ca-cert"}}, - }, - { - name: "GET with non-empty body returns 400", - method: "GET", - url: "http://foo.bar/x5u/normandy/example.pem", - urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "example.pem"}, - nilBody: false, - body: "foobar/---", - expectedStatus: http.StatusBadRequest, - expectedBody: "endpoint received unexpected request body\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with unknown signer returns 404", - method: "GET", - url: "http://foo.bar/x5u/normandyxyz/example.pem", - urlRouteVars: map[string]string{"keyid": "normandyxyz", "chainfile": "example.pem"}, - nilBody: false, - body: "", - expectedStatus: http.StatusNotFound, - expectedBody: "404 page not found\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with unknown chain returns 404", - method: "GET", - url: "http://foo.bar/x5u/normandy/example.pem", - urlRouteVars: map[string]string{"keyid": "normandy", "chainfile": "notfound.pem"}, - nilBody: false, - body: "", - expectedStatus: http.StatusNotFound, - expectedBody: "404 page not found\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - { - name: "GET with non-PKI signer returns 404", - method: "GET", - url: "http://foo.bar/x5u/randompgp/example.pem", - urlRouteVars: map[string]string{"keyid": "randompgp", "chainfile": "example.pem"}, - nilBody: false, - body: "", - expectedStatus: http.StatusNotFound, - expectedBody: "404 page not found\r\nrequest-id: -\n", - expectedHeaders: http.Header{"Content-Type": []string{"text/plain; charset=utf-8"}}, - }, - } - for _, testcase := range testcases { - testcase.Run(t, ag.handleGetX5U) - } -} - func getAuthHeader(req *http.Request, user, token string, hash func() hash.Hash, ext, contenttype string, payload []byte) string { auth := hawk.NewRequestAuth(req, &hawk.Credentials{ diff --git a/monitor_handler_racing_test.go b/monitor_handler_racing_test.go index 4c041902e..d1817d0ab 100644 --- a/monitor_handler_racing_test.go +++ b/monitor_handler_racing_test.go @@ -12,10 +12,12 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" + "os" + "path" "strings" "testing" - "github.com/gorilla/mux" "github.com/mozilla-services/autograph/formats" "github.com/mozilla-services/autograph/signer/apk2" "github.com/mozilla-services/autograph/signer/contentsignature" @@ -31,20 +33,33 @@ import ( const autographDevRootHash = `5E:36:F2:14:DE:82:3F:8B:29:96:89:23:5F:03:41:AC:AF:A0:75:AF:82:CB:4C:D4:30:7C:3D:B3:43:39:2A:FE` func getMirroredX5U(x5u string) (body []byte, err error) { - req, err := http.NewRequest("GET", x5u, nil) - if err != nil { - return nil, err + parsed, err := url.Parse(x5u) + segs := strings.Split(parsed.Path, "/") + + if segs[len(segs)-3] != "x5u" { + return nil, fmt.Errorf("x5u URL '%s' does not appear to be locally hosted", x5u) } - segs := strings.Split(req.URL.Path, "/") - req = mux.SetURLVars(req, map[string]string{"keyid": segs[len(segs)-2], "chainfile": segs[len(segs)-1]}) - w := httptest.NewRecorder() - ag.handleGetX5U(w, req) - if w.Code != http.StatusOK || w.Body.String() == "" { - return nil, fmt.Errorf("fetch x5u failed with %d: %s; request was: %+v", w.Code, w.Body.String(), req) + // If the URL can be parsed into the form /x5u/keyid/chainfilename then + // try to read it directly off the disk, since this is likely a local file + for _, s := range ag.getSigners() { + config := s.Config() + if config.ID != segs[len(segs)-2] { + continue + } + + parsedUploadLocation, err := url.Parse(config.ChainUploadLocation) + if err != nil { + return nil, err + } + if parsedUploadLocation.Scheme != "file" { + break + } + + return os.ReadFile(path.Join(parsedUploadLocation.Path, segs[len(segs)-1])) } - return w.Body.Bytes(), nil + return nil, fmt.Errorf("unable to fetch x5u from '%s'", x5u) } func TestMonitorPass(t *testing.T) { From 3bb5a51b5592a538e5e1e2caa88bc8b3527afcf1 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Mon, 19 Aug 2024 19:41:52 -0700 Subject: [PATCH 11/15] Remove unused strings import --- handlers.go | 1 - 1 file changed, 1 deletion(-) diff --git a/handlers.go b/handlers.go index 8e8c4fc62..9091f304e 100644 --- a/handlers.go +++ b/handlers.go @@ -17,7 +17,6 @@ import ( "net/url" "os" "path" - "strings" "time" "github.com/gorilla/mux" From e5d773d3036f0cc62a6eaa920b5b5284ddffb3b6 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Mon, 19 Aug 2024 19:57:04 -0700 Subject: [PATCH 12/15] Fixup some lint --- monitor_handler_racing_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/monitor_handler_racing_test.go b/monitor_handler_racing_test.go index d1817d0ab..0620a6d73 100644 --- a/monitor_handler_racing_test.go +++ b/monitor_handler_racing_test.go @@ -34,8 +34,11 @@ const autographDevRootHash = `5E:36:F2:14:DE:82:3F:8B:29:96:89:23:5F:03:41:AC:AF func getMirroredX5U(x5u string) (body []byte, err error) { parsed, err := url.Parse(x5u) - segs := strings.Split(parsed.Path, "/") + if err != nil { + return nil, err + } + segs := strings.Split(parsed.Path, "/") if segs[len(segs)-3] != "x5u" { return nil, fmt.Errorf("x5u URL '%s' does not appear to be locally hosted", x5u) } From 2970dc681b5069245370df7f89b10bd3fb2f1461 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Tue, 20 Aug 2024 08:58:39 -0700 Subject: [PATCH 13/15] Address some review comments --- docs/endpoints.md | 10 +++++++--- handlers.go | 15 ++++++++++----- main.go | 2 +- monitor_handler.go | 2 +- monitor_handler_racing_test.go | 18 ++++++++++-------- signer/signer.go | 3 --- 6 files changed, 29 insertions(+), 21 deletions(-) diff --git a/docs/endpoints.md b/docs/endpoints.md index 4427f3677..bf11d3926 100644 --- a/docs/endpoints.md +++ b/docs/endpoints.md @@ -271,11 +271,15 @@ See `/sign/data`, the response format is identical. A successful request return a `201 Created` with a response body containing an S/MIME detached signature encoded with Base 64. -## /x5u/:keyid/:chain +## /x5u/:keyid/ 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. +stored locally. If the signer is configured with an `X5U` using the `file://` +scheme, then the contents of that file location are served under this path. + +This path is only intended to simplify local development and testing. Production +signers should store their X5U certificate chains with a cloud storage provider +such as Amazon S3 or Google Cloud Storage. ## /\_\_monitor\_\_ diff --git a/handlers.go b/handlers.go index 9091f304e..05b8b1cfb 100644 --- a/handlers.go +++ b/handlers.go @@ -75,16 +75,21 @@ 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 { +// rewriteLocalX5U checks for X5U certificate chains using the `file://` scheme +// and rewrites them to use the `/x5u/:keyid/` endpoint instead, which should +// mirror the contents of the signer's X5U location, and returns the updated URL. +// +// If the X5U certificate chain uses any other scheme, then the original URL is returned +// without change. +func rewriteLocalX5U(r *http.Request, response formats.SignatureResponse) string { parsedX5U, err := url.Parse(response.X5U) if err == nil && parsedX5U.Scheme == "file" { - mirroredX5U := url.URL{ + newX5U := url.URL{ Scheme: "http", Host: r.Host, Path: path.Join("x5u", response.SignerID, path.Base(parsedX5U.Path)), } - return mirroredX5U.String() + return newX5U.String() } // Otherwise, return the X5U unmodified @@ -220,7 +225,7 @@ func (a *autographer) handleSignature(w http.ResponseWriter, r *http.Request) { X5U: requestedSignerConfig.X5U, SignerOpts: requestedSignerConfig.SignerOpts, } - sigresps[i].X5U = mirrorLocalX5U(r, sigresps[i]) + sigresps[i].X5U = rewriteLocalX5U(r, sigresps[i]) // Make sure the signer implements the right interface, then sign the data switch r.URL.RequestURI() { diff --git a/main.go b/main.go index 9f7ae0319..9befce792 100755 --- a/main.go +++ b/main.go @@ -227,7 +227,7 @@ func run(conf configuration, listen string, debug bool) { // For each signer with a local chain upload location (eg: using the file // scheme) create an handler to serve that directory at the path /x5u/keyid/ for _, signerConf := range conf.Signers { - parsedURL, err := url.Parse(signerConf.ChainUploadLocation) + parsedURL, err := url.Parse(signerConf.X5U) if err != nil || parsedURL.Scheme != "file" { // This signer doesn't upload certificate chains to local storage. continue diff --git a/monitor_handler.go b/monitor_handler.go index 7965cc7b3..6667e2803 100644 --- a/monitor_handler.go +++ b/monitor_handler.go @@ -47,7 +47,7 @@ func (m *monitor) handleMonitor(w http.ResponseWriter, r *http.Request) { enc := json.NewEncoder(w) for _, response := range m.sigresps { - response.X5U = mirrorLocalX5U(r, response) + response.X5U = rewriteLocalX5U(r, response) if err := enc.Encode(&response); err != nil { httpError(w, r, http.StatusInternalServerError, "encoding failed with error: %v", err) return diff --git a/monitor_handler_racing_test.go b/monitor_handler_racing_test.go index 0620a6d73..0909c0f36 100644 --- a/monitor_handler_racing_test.go +++ b/monitor_handler_racing_test.go @@ -32,34 +32,36 @@ import ( const autographDevRootHash = `5E:36:F2:14:DE:82:3F:8B:29:96:89:23:5F:03:41:AC:AF:A0:75:AF:82:CB:4C:D4:30:7C:3D:B3:43:39:2A:FE` -func getMirroredX5U(x5u string) (body []byte, err error) { +func getLocalX5U(x5u string) (body []byte, err error) { parsed, err := url.Parse(x5u) if err != nil { return nil, err } + // If the URL can be parsed into the form /x5u/keyid/chainfilename then + // try to read it directly off the disk, since this is likely a local file segs := strings.Split(parsed.Path, "/") - if segs[len(segs)-3] != "x5u" { + if (len(segs) < 3) || (segs[len(segs)-3] != "x5u") { return nil, fmt.Errorf("x5u URL '%s' does not appear to be locally hosted", x5u) } - // If the URL can be parsed into the form /x5u/keyid/chainfilename then - // try to read it directly off the disk, since this is likely a local file + // Find the signer matching the keyid URL segment. for _, s := range ag.getSigners() { config := s.Config() if config.ID != segs[len(segs)-2] { continue } - parsedUploadLocation, err := url.Parse(config.ChainUploadLocation) + // If the X5U location uses the `file` scheme, read the chain off disk. + parsedX5U, err := url.Parse(config.X5U) if err != nil { return nil, err } - if parsedUploadLocation.Scheme != "file" { + if parsedX5U.Scheme != "file" { break } - return os.ReadFile(path.Join(parsedUploadLocation.Path, segs[len(segs)-1])) + return os.ReadFile(path.Join(parsedX5U.Path, segs[len(segs)-1])) } return nil, fmt.Errorf("unable to fetch x5u from '%s'", x5u) @@ -101,7 +103,7 @@ func TestMonitorPass(t *testing.T) { t.Fatalf("verification of monitoring response failed: %v", err) } case contentsignaturepki.Type: - body, err := getMirroredX5U(response.X5U) + body, err := getLocalX5U(response.X5U) if err != nil { t.Fatal(err) } diff --git a/signer/signer.go b/signer/signer.go index 16d5e843b..36b43012b 100644 --- a/signer/signer.go +++ b/signer/signer.go @@ -34,9 +34,6 @@ import ( // IDFormat is a regex for the format IDs must follow const IDFormat = `^[a-zA-Z0-9-_]{1,64}$` -// IDFormatRegexp is the compiled regex from IDFormat -var IDFormatRegexp = regexp.MustCompile(IDFormat) - // RecommendationConfig is a config for the XPI recommendation file type RecommendationConfig struct { // AllowedStates is a map of strings the signer is allowed to From 3e19044a33e0f88ed1a7c8fee4267fa72e25f58b Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Tue, 20 Aug 2024 10:19:16 -0700 Subject: [PATCH 14/15] Another hacky fix to the monitor_handler_racing_test --- monitor_handler_racing_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/monitor_handler_racing_test.go b/monitor_handler_racing_test.go index 0909c0f36..d11b2b09e 100644 --- a/monitor_handler_racing_test.go +++ b/monitor_handler_racing_test.go @@ -14,7 +14,6 @@ import ( "net/http/httptest" "net/url" "os" - "path" "strings" "testing" @@ -46,9 +45,10 @@ func getLocalX5U(x5u string) (body []byte, err error) { } // Find the signer matching the keyid URL segment. + keyid := segs[len(segs)-2] for _, s := range ag.getSigners() { config := s.Config() - if config.ID != segs[len(segs)-2] { + if config.ID != keyid { continue } @@ -60,11 +60,12 @@ func getLocalX5U(x5u string) (body []byte, err error) { if parsedX5U.Scheme != "file" { break } - - return os.ReadFile(path.Join(parsedX5U.Path, segs[len(segs)-1])) + return os.ReadFile(parsedX5U.Path) } - return nil, fmt.Errorf("unable to fetch x5u from '%s'", x5u) + // Otherwise, we would need to perform an HTTP request to wherever this + // chain is hosted, but under unit testing there are no such signers. + return nil, fmt.Errorf("unable to read x5u from '%s'", x5u) } func TestMonitorPass(t *testing.T) { From 9ecc59c58deb0159932f7e64adf130c01ca198d6 Mon Sep 17 00:00:00 2001 From: Naomi Kirby Date: Tue, 20 Aug 2024 12:18:47 -0700 Subject: [PATCH 15/15] Tweak rewriteLocalX5U() usage --- handlers.go | 11 +++++------ monitor_handler.go | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/handlers.go b/handlers.go index 05b8b1cfb..e2e420e5b 100644 --- a/handlers.go +++ b/handlers.go @@ -81,19 +81,19 @@ func logSigningRequestFailure(sigreq formats.SignatureRequest, sigresp formats.S // // If the X5U certificate chain uses any other scheme, then the original URL is returned // without change. -func rewriteLocalX5U(r *http.Request, response formats.SignatureResponse) string { - parsedX5U, err := url.Parse(response.X5U) +func rewriteLocalX5U(r *http.Request, keyid string, x5u string) string { + parsedX5U, err := url.Parse(x5u) if err == nil && parsedX5U.Scheme == "file" { newX5U := url.URL{ Scheme: "http", Host: r.Host, - Path: path.Join("x5u", response.SignerID, path.Base(parsedX5U.Path)), + Path: path.Join("x5u", keyid, path.Base(parsedX5U.Path)), } return newX5U.String() } // Otherwise, return the X5U unmodified - return response.X5U + return x5u } // handleSignature endpoint accepts a list of signature requests in a HAWK authenticated POST request @@ -222,10 +222,9 @@ func (a *autographer) handleSignature(w http.ResponseWriter, r *http.Request) { SignerID: requestedSignerConfig.ID, PublicKey: requestedSignerConfig.PublicKey, SignedFile: base64.StdEncoding.EncodeToString(signedfile), - X5U: requestedSignerConfig.X5U, + X5U: rewriteLocalX5U(r, requestedSignerConfig.ID, requestedSignerConfig.X5U), SignerOpts: requestedSignerConfig.SignerOpts, } - sigresps[i].X5U = rewriteLocalX5U(r, sigresps[i]) // Make sure the signer implements the right interface, then sign the data switch r.URL.RequestURI() { diff --git a/monitor_handler.go b/monitor_handler.go index 6667e2803..eb40f361d 100644 --- a/monitor_handler.go +++ b/monitor_handler.go @@ -47,7 +47,7 @@ func (m *monitor) handleMonitor(w http.ResponseWriter, r *http.Request) { enc := json.NewEncoder(w) for _, response := range m.sigresps { - response.X5U = rewriteLocalX5U(r, response) + response.X5U = rewriteLocalX5U(r, response.SignerID, response.X5U) if err := enc.Encode(&response); err != nil { httpError(w, r, http.StatusInternalServerError, "encoding failed with error: %v", err) return