From 4213a190d5204176132e2f27e7df235639d4adbf Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Tue, 27 Feb 2024 16:17:09 +0100 Subject: [PATCH 01/16] Use `X-Request-Id` as canonical request identifier (if available) If `X-Request-Id` is available in an HTTP request made against the CA server, it'll be used as the identifier for the request. This slightly changes the existing behavior, which relied on the custom `X-Smallstep-Id` header, but usage of that header is currently not very widespread, and `X-Request-Id` is more generally known for the use case `X-Smallstep-Id` is used for. `X-Smallstep-Id` is currently still considered, but it'll only be used if `X-Request-Id` is not set. --- logging/context.go | 23 +++++++--- logging/context_test.go | 94 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 logging/context_test.go diff --git a/logging/context.go b/logging/context.go index b24b3638..ab8464d0 100644 --- a/logging/context.go +++ b/logging/context.go @@ -21,14 +21,27 @@ func NewRequestID() string { return xid.New().String() } -// RequestID returns a new middleware that gets the given header and sets it -// in the context so it can be written in the logger. If the header does not -// exists or it's the empty string, it uses github.com/rs/xid to create a new -// one. +// defaultRequestIDHeader is the header name used for propagating +// request IDs. If available in an HTTP request, it'll be used instead +// of the X-Smallstep-Id header. +const defaultRequestIDHeader = "X-Request-Id" + +// RequestID returns a new middleware that obtains the current request ID +// and sets it in the context. It first tries to read the request ID from +// the "X-Request-Id" header. If that's not set, it tries to read it from +// the provided header name. If the header does not exist or its value is +// the empty string, it uses github.com/rs/xid to create a new one. func RequestID(headerName string) func(next http.Handler) http.Handler { + if headerName == "" { + headerName = defaultTraceIDHeader + } return func(next http.Handler) http.Handler { fn := func(w http.ResponseWriter, req *http.Request) { - requestID := req.Header.Get(headerName) + requestID := req.Header.Get(defaultRequestIDHeader) + if requestID == "" { + requestID = req.Header.Get(headerName) + } + if requestID == "" { requestID = NewRequestID() req.Header.Set(headerName, requestID) diff --git a/logging/context_test.go b/logging/context_test.go new file mode 100644 index 00000000..c519539d --- /dev/null +++ b/logging/context_test.go @@ -0,0 +1,94 @@ +package logging + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newRequest(t *testing.T) *http.Request { + r, err := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + require.NoError(t, err) + return r +} + +func TestRequestID(t *testing.T) { + requestWithID := newRequest(t) + requestWithID.Header.Set("X-Request-Id", "reqID") + requestWithoutID := newRequest(t) + requestWithEmptyHeader := newRequest(t) + requestWithEmptyHeader.Header.Set("X-Request-Id", "") + requestWithSmallstepID := newRequest(t) + requestWithSmallstepID.Header.Set("X-Smallstep-Id", "smallstepID") + + tests := []struct { + name string + headerName string + handler http.HandlerFunc + req *http.Request + }{ + { + name: "default-request-id", + headerName: defaultTraceIDHeader, + handler: func(_ http.ResponseWriter, r *http.Request) { + assert.Empty(t, r.Header.Get("X-Smallstep-Id")) + assert.Equal(t, "reqID", r.Header.Get("X-Request-Id")) + reqID, ok := GetRequestID(r.Context()) + if assert.True(t, ok) { + assert.Equal(t, "reqID", reqID) + } + }, + req: requestWithID, + }, + { + name: "no-request-id", + headerName: "X-Request-Id", + handler: func(_ http.ResponseWriter, r *http.Request) { + assert.Empty(t, r.Header.Get("X-Smallstep-Id")) + value := r.Header.Get("X-Request-Id") + assert.NotEmpty(t, value) + reqID, ok := GetRequestID(r.Context()) + if assert.True(t, ok) { + assert.Equal(t, value, reqID) + } + }, + req: requestWithoutID, + }, + { + name: "empty-header-name", + headerName: "", + handler: func(_ http.ResponseWriter, r *http.Request) { + assert.Empty(t, r.Header.Get("X-Request-Id")) + value := r.Header.Get("X-Smallstep-Id") + assert.NotEmpty(t, value) + reqID, ok := GetRequestID(r.Context()) + if assert.True(t, ok) { + assert.Equal(t, value, reqID) + } + }, + req: requestWithEmptyHeader, + }, + { + name: "fallback-header-name", + headerName: defaultTraceIDHeader, + handler: func(_ http.ResponseWriter, r *http.Request) { + assert.Empty(t, r.Header.Get("X-Request-Id")) + assert.Equal(t, "smallstepID", r.Header.Get("X-Smallstep-Id")) + reqID, ok := GetRequestID(r.Context()) + if assert.True(t, ok) { + assert.Equal(t, "smallstepID", reqID) + } + }, + req: requestWithSmallstepID, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := RequestID(tt.headerName) + h(tt.handler).ServeHTTP(httptest.NewRecorder(), tt.req) + }) + } +} From c1c2e73475f4333267aa855d758800dae0255278 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Tue, 27 Feb 2024 17:04:21 +0100 Subject: [PATCH 02/16] Add `X-Request-Id` to all requests made by our CA clients --- ca/acmeClient.go | 3 +++ ca/client.go | 20 ++++++++++++++++---- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/ca/acmeClient.go b/ca/acmeClient.go index bb3b1d84..3ef2f191 100644 --- a/ca/acmeClient.go +++ b/ca/acmeClient.go @@ -48,6 +48,7 @@ func NewACMEClient(endpoint string, contact []string, opts ...ClientOption) (*AC return nil, errors.Wrapf(err, "creating GET request %s failed", endpoint) } req.Header.Set("User-Agent", UserAgent) + enforceRequestID(req) resp, err := ac.client.Do(req) if err != nil { return nil, errors.Wrapf(err, "client GET %s failed", endpoint) @@ -109,6 +110,7 @@ func (c *ACMEClient) GetNonce() (string, error) { return "", errors.Wrapf(err, "creating GET request %s failed", c.dir.NewNonce) } req.Header.Set("User-Agent", UserAgent) + enforceRequestID(req) resp, err := c.client.Do(req) if err != nil { return "", errors.Wrapf(err, "client GET %s failed", c.dir.NewNonce) @@ -188,6 +190,7 @@ func (c *ACMEClient) post(payload []byte, url string, headerOps ...withHeaderOpt } req.Header.Set("Content-Type", "application/jose+json") req.Header.Set("User-Agent", UserAgent) + enforceRequestID(req) resp, err := c.client.Do(req) if err != nil { return nil, errors.Wrapf(err, "client POST %s failed", c.dir.NewOrder) diff --git a/ca/client.go b/ca/client.go index ac13e1fe..5e2d98c8 100644 --- a/ca/client.go +++ b/ca/client.go @@ -24,6 +24,7 @@ import ( "strings" "github.com/pkg/errors" + "github.com/rs/xid" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority" "github.com/smallstep/certificates/authority/provisioner" @@ -83,8 +84,7 @@ func (c *uaClient) GetWithContext(ctx context.Context, u string) (*http.Response if err != nil { return nil, errors.Wrapf(err, "create GET %s request failed", u) } - req.Header.Set("User-Agent", UserAgent) - return c.Client.Do(req) + return c.Do(req) } func (c *uaClient) Post(u, contentType string, body io.Reader) (*http.Response, error) { @@ -97,12 +97,24 @@ func (c *uaClient) PostWithContext(ctx context.Context, u, contentType string, b return nil, errors.Wrapf(err, "create POST %s request failed", u) } req.Header.Set("Content-Type", contentType) - req.Header.Set("User-Agent", UserAgent) - return c.Client.Do(req) + return c.Do(req) +} + +// requestIDHeader is the header name used for propagating request IDs from +// the CA client to the CA and back again. +const requestIDHeader = "X-Request-Id" + +// enforceRequestID checks if the X-Request-Id HTTP header is filled. If it's +// empty, it'll generate a new request ID and set the header. +func enforceRequestID(r *http.Request) { + if r.Header.Get(requestIDHeader) == "" { + r.Header.Set(requestIDHeader, xid.New().String()) + } } func (c *uaClient) Do(req *http.Request) (*http.Response, error) { req.Header.Set("User-Agent", UserAgent) + enforceRequestID(req) return c.Client.Do(req) } From a58f5956e31255b8784be5da1484f3afc634a32c Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Tue, 27 Feb 2024 20:48:56 +0100 Subject: [PATCH 03/16] Add reflection of request ID in `X-Request-Id` response header --- logging/context.go | 14 +++++++++----- logging/context_test.go | 12 ++++++++---- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/logging/context.go b/logging/context.go index ab8464d0..9d7a7071 100644 --- a/logging/context.go +++ b/logging/context.go @@ -21,10 +21,10 @@ func NewRequestID() string { return xid.New().String() } -// defaultRequestIDHeader is the header name used for propagating -// request IDs. If available in an HTTP request, it'll be used instead -// of the X-Smallstep-Id header. -const defaultRequestIDHeader = "X-Request-Id" +// requestIDHeader is the header name used for propagating request IDs. If +// available in an HTTP request, it'll be used instead of the X-Smallstep-Id +// header. It'll always be used in response and set to the request ID. +const requestIDHeader = "X-Request-Id" // RequestID returns a new middleware that obtains the current request ID // and sets it in the context. It first tries to read the request ID from @@ -37,7 +37,7 @@ func RequestID(headerName string) func(next http.Handler) http.Handler { } return func(next http.Handler) http.Handler { fn := func(w http.ResponseWriter, req *http.Request) { - requestID := req.Header.Get(defaultRequestIDHeader) + requestID := req.Header.Get(requestIDHeader) if requestID == "" { requestID = req.Header.Get(headerName) } @@ -47,6 +47,10 @@ func RequestID(headerName string) func(next http.Handler) http.Handler { req.Header.Set(headerName, requestID) } + // immediately set the request ID to be reflected in the response + w.Header().Set(requestIDHeader, requestID) + + // continue down the handler chain ctx := WithRequestID(req.Context(), requestID) next.ServeHTTP(w, req.WithContext(ctx)) } diff --git a/logging/context_test.go b/logging/context_test.go index c519539d..da993f7b 100644 --- a/logging/context_test.go +++ b/logging/context_test.go @@ -33,20 +33,21 @@ func TestRequestID(t *testing.T) { { name: "default-request-id", headerName: defaultTraceIDHeader, - handler: func(_ http.ResponseWriter, r *http.Request) { + handler: func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("X-Smallstep-Id")) assert.Equal(t, "reqID", r.Header.Get("X-Request-Id")) reqID, ok := GetRequestID(r.Context()) if assert.True(t, ok) { assert.Equal(t, "reqID", reqID) } + assert.Equal(t, "reqID", w.Header().Get("X-Request-Id")) }, req: requestWithID, }, { name: "no-request-id", headerName: "X-Request-Id", - handler: func(_ http.ResponseWriter, r *http.Request) { + handler: func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("X-Smallstep-Id")) value := r.Header.Get("X-Request-Id") assert.NotEmpty(t, value) @@ -54,13 +55,14 @@ func TestRequestID(t *testing.T) { if assert.True(t, ok) { assert.Equal(t, value, reqID) } + assert.Equal(t, value, w.Header().Get("X-Request-Id")) }, req: requestWithoutID, }, { name: "empty-header-name", headerName: "", - handler: func(_ http.ResponseWriter, r *http.Request) { + handler: func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("X-Request-Id")) value := r.Header.Get("X-Smallstep-Id") assert.NotEmpty(t, value) @@ -68,19 +70,21 @@ func TestRequestID(t *testing.T) { if assert.True(t, ok) { assert.Equal(t, value, reqID) } + assert.Equal(t, value, w.Header().Get("X-Request-Id")) }, req: requestWithEmptyHeader, }, { name: "fallback-header-name", headerName: defaultTraceIDHeader, - handler: func(_ http.ResponseWriter, r *http.Request) { + handler: func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("X-Request-Id")) assert.Equal(t, "smallstepID", r.Header.Get("X-Smallstep-Id")) reqID, ok := GetRequestID(r.Context()) if assert.True(t, ok) { assert.Equal(t, "smallstepID", reqID) } + assert.Equal(t, "smallstepID", w.Header().Get("X-Request-Id")) }, req: requestWithSmallstepID, }, From cf8a50157f7a662501a4f6b4728ad771af6052bb Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 01:05:38 +0100 Subject: [PATCH 04/16] Add a basic e2e test for `X-Request-Id` reflection --- api/api_test.go | 8 +-- ca/ca_test.go | 25 +++++++-- ca/client.go | 51 ++++++++++--------- errs/error.go | 9 ++-- test/e2e/requestid_test.go | 102 +++++++++++++++++++++++++++++++++++++ 5 files changed, 155 insertions(+), 40 deletions(-) create mode 100644 test/e2e/requestid_test.go diff --git a/api/api_test.go b/api/api_test.go index cf988593..8090c6d4 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -884,16 +884,12 @@ func Test_Sign(t *testing.T) { CsrPEM: CertificateRequest{csr}, OTT: "foobarzar", }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) invalid, err := json.Marshal(SignRequest{ CsrPEM: CertificateRequest{csr}, OTT: "", }) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) expected1 := []byte(`{"crt":"` + strings.ReplaceAll(certPEM, "\n", `\n`) + `\n","ca":"` + strings.ReplaceAll(rootPEM, "\n", `\n`) + `\n","certChain":["` + strings.ReplaceAll(certPEM, "\n", `\n`) + `\n","` + strings.ReplaceAll(rootPEM, "\n", `\n`) + `\n"]}`) expected2 := []byte(`{"crt":"` + strings.ReplaceAll(stepCertPEM, "\n", `\n`) + `\n","ca":"` + strings.ReplaceAll(rootPEM, "\n", `\n`) + `\n","certChain":["` + strings.ReplaceAll(stepCertPEM, "\n", `\n`) + `\n","` + strings.ReplaceAll(rootPEM, "\n", `\n`) + `\n"]}`) diff --git a/ca/ca_test.go b/ca/ca_test.go index 7ad25cc6..a8c173c4 100644 --- a/ca/ca_test.go +++ b/ca/ca_test.go @@ -289,6 +289,9 @@ ZEp7knvU2psWRw== if assert.Equals(t, rr.Code, tc.status) { body := &ClosingBuffer{rr.Body} + resp := &http.Response{ + Body: body, + } if rr.Code < http.StatusBadRequest { var sign api.SignResponse assert.FatalError(t, readJSON(body, &sign)) @@ -325,7 +328,7 @@ ZEp7knvU2psWRw== assert.FatalError(t, err) assert.Equals(t, intermediate, realIntermediate) } else { - err := readError(body) + err := readError(resp) if tc.errMsg == "" { assert.FatalError(t, errors.New("must validate response error")) } @@ -369,6 +372,9 @@ func TestCAProvisioners(t *testing.T) { if assert.Equals(t, rr.Code, tc.status) { body := &ClosingBuffer{rr.Body} + resp := &http.Response{ + Body: body, + } if rr.Code < http.StatusBadRequest { var resp api.ProvisionersResponse @@ -379,7 +385,7 @@ func TestCAProvisioners(t *testing.T) { assert.FatalError(t, err) assert.Equals(t, a, b) } else { - err := readError(body) + err := readError(resp) if tc.errMsg == "" { assert.FatalError(t, errors.New("must validate response error")) } @@ -436,12 +442,15 @@ func TestCAProvisionerEncryptedKey(t *testing.T) { if assert.Equals(t, rr.Code, tc.status) { body := &ClosingBuffer{rr.Body} + resp := &http.Response{ + Body: body, + } if rr.Code < http.StatusBadRequest { var ek api.ProvisionerKeyResponse assert.FatalError(t, readJSON(body, &ek)) assert.Equals(t, ek.Key, tc.expectedKey) } else { - err := readError(body) + err := readError(resp) if tc.errMsg == "" { assert.FatalError(t, errors.New("must validate response error")) } @@ -498,12 +507,15 @@ func TestCARoot(t *testing.T) { if assert.Equals(t, rr.Code, tc.status) { body := &ClosingBuffer{rr.Body} + resp := &http.Response{ + Body: body, + } if rr.Code < http.StatusBadRequest { var root api.RootResponse assert.FatalError(t, readJSON(body, &root)) assert.Equals(t, root.RootPEM.Certificate, rootCrt) } else { - err := readError(body) + err := readError(resp) if tc.errMsg == "" { assert.FatalError(t, errors.New("must validate response error")) } @@ -641,6 +653,9 @@ func TestCARenew(t *testing.T) { if assert.Equals(t, rr.Code, tc.status) { body := &ClosingBuffer{rr.Body} + resp := &http.Response{ + Body: body, + } if rr.Code < http.StatusBadRequest { var sign api.SignResponse assert.FatalError(t, readJSON(body, &sign)) @@ -673,7 +688,7 @@ func TestCARenew(t *testing.T) { assert.Equals(t, *sign.TLSOptions, authority.DefaultTLSOptions) } else { - err := readError(body) + err := readError(resp) if tc.errMsg == "" { assert.FatalError(t, errors.New("must validate response error")) } diff --git a/ca/client.go b/ca/client.go index 5e2d98c8..8930d8ee 100644 --- a/ca/client.go +++ b/ca/client.go @@ -622,7 +622,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var version api.VersionResponse if err := readJSON(resp.Body, &version); err != nil { @@ -652,7 +652,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var health api.HealthResponse if err := readJSON(resp.Body, &health); err != nil { @@ -687,7 +687,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var root api.RootResponse if err := readJSON(resp.Body, &root); err != nil { @@ -726,7 +726,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var sign api.SignResponse if err := readJSON(resp.Body, &sign); err != nil { @@ -765,7 +765,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var sign api.SignResponse if err := readJSON(resp.Body, &sign); err != nil { @@ -802,7 +802,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var sign api.SignResponse if err := readJSON(resp.Body, &sign); err != nil { @@ -842,7 +842,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var sign api.SignResponse if err := readJSON(resp.Body, &sign); err != nil { @@ -883,7 +883,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var revoke api.RevokeResponse if err := readJSON(resp.Body, &revoke); err != nil { @@ -926,7 +926,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var provisioners api.ProvisionersResponse if err := readJSON(resp.Body, &provisioners); err != nil { @@ -958,7 +958,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var key api.ProvisionerKeyResponse if err := readJSON(resp.Body, &key); err != nil { @@ -988,7 +988,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var roots api.RootsResponse if err := readJSON(resp.Body, &roots); err != nil { @@ -1018,7 +1018,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var federation api.FederationResponse if err := readJSON(resp.Body, &federation); err != nil { @@ -1052,7 +1052,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var sign api.SSHSignResponse if err := readJSON(resp.Body, &sign); err != nil { @@ -1086,7 +1086,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var renew api.SSHRenewResponse if err := readJSON(resp.Body, &renew); err != nil { @@ -1120,7 +1120,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var rekey api.SSHRekeyResponse if err := readJSON(resp.Body, &rekey); err != nil { @@ -1154,7 +1154,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var revoke api.SSHRevokeResponse if err := readJSON(resp.Body, &revoke); err != nil { @@ -1184,7 +1184,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var keys api.SSHRootsResponse if err := readJSON(resp.Body, &keys); err != nil { @@ -1214,7 +1214,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var keys api.SSHRootsResponse if err := readJSON(resp.Body, &keys); err != nil { @@ -1248,7 +1248,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var cfg api.SSHConfigResponse if err := readJSON(resp.Body, &cfg); err != nil { @@ -1287,7 +1287,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var check api.SSHCheckPrincipalResponse if err := readJSON(resp.Body, &check); err != nil { @@ -1316,7 +1316,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var hosts api.SSHGetHostsResponse if err := readJSON(resp.Body, &hosts); err != nil { @@ -1348,7 +1348,7 @@ retry: retried = true goto retry } - return nil, readError(resp.Body) + return nil, readError(resp) } var bastion api.SSHBastionResponse if err := readJSON(resp.Body, &bastion); err != nil { @@ -1516,12 +1516,13 @@ func readProtoJSON(r io.ReadCloser, m proto.Message) error { return protojson.Unmarshal(data, m) } -func readError(r io.ReadCloser) error { - defer r.Close() +func readError(r *http.Response) error { + defer r.Body.Close() apiErr := new(errs.Error) - if err := json.NewDecoder(r).Decode(apiErr); err != nil { + if err := json.NewDecoder(r.Body).Decode(apiErr); err != nil { return err } + apiErr.RequestID = r.Header.Get("X-Request-Id") return apiErr } diff --git a/errs/error.go b/errs/error.go index ba066925..c9ad92a6 100644 --- a/errs/error.go +++ b/errs/error.go @@ -49,10 +49,11 @@ func WithKeyVal(key string, val interface{}) Option { // Error represents the CA API errors. type Error struct { - Status int - Err error - Msg string - Details map[string]interface{} + Status int + Err error + Msg string + Details map[string]interface{} + RequestID string `json:"-"` } // ErrorResponse represents an error in JSON format. diff --git a/test/e2e/requestid_test.go b/test/e2e/requestid_test.go new file mode 100644 index 00000000..7eccb4f4 --- /dev/null +++ b/test/e2e/requestid_test.go @@ -0,0 +1,102 @@ +package e2e + +import ( + "context" + "encoding/json" + "fmt" + "net" + "path/filepath" + "sync" + "testing" + + "github.com/smallstep/certificates/authority/config" + "github.com/smallstep/certificates/ca" + "github.com/smallstep/certificates/errs" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.step.sm/crypto/minica" + "go.step.sm/crypto/pemutil" +) + +func TestXxx(t *testing.T) { + dir := t.TempDir() + m, err := minica.New(minica.WithName("Step E2E")) + require.NoError(t, err) + + rootFilepath := filepath.Join(dir, "root.crt") + _, err = pemutil.Serialize(m.Root, pemutil.WithFilename(rootFilepath)) + require.NoError(t, err) + + intermediateCertFilepath := filepath.Join(dir, "intermediate.crt") + _, err = pemutil.Serialize(m.Intermediate, pemutil.WithFilename(intermediateCertFilepath)) + require.NoError(t, err) + + intermediateKeyFilepath := filepath.Join(dir, "intermediate.key") + _, err = pemutil.Serialize(m.Signer, pemutil.WithFilename(intermediateKeyFilepath)) + require.NoError(t, err) + + // get a random address to listen on and connect to; currently no nicer way to get one before starting the server + l, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + randomAddress := l.Addr().String() + err = l.Close() + require.NoError(t, err) + + cfg := &config.Config{ + Root: []string{rootFilepath}, + IntermediateCert: intermediateCertFilepath, + IntermediateKey: intermediateKeyFilepath, + Address: randomAddress, // reuse the address that was just "reserved" + DNSNames: []string{"127.0.0.1", "stepca.localhost"}, + AuthorityConfig: &config.AuthConfig{ + AuthorityID: "stepca-test", + DeploymentType: "standalone-test", + }, + Logger: json.RawMessage(`{"format": "text"}`), + } + c, err := ca.New(cfg) + require.NoError(t, err) + + // instantiate a client for the CA + client, err := ca.NewClient( + fmt.Sprintf("https://%s", randomAddress), + ca.WithRootFile(rootFilepath), + ) + require.NoError(t, err) + + var wg sync.WaitGroup + wg.Add(1) + + go func() { + defer wg.Done() + err = c.Run() + require.Error(t, err) // expect error when server is stopped + }() + + // require OK health response as the baseline + ctx := context.Background() + healthResponse, err := client.HealthWithContext(ctx) + assert.NoError(t, err) + require.Equal(t, "ok", healthResponse.Status) + + // expect an error when retrieving an invalid root + rootResponse, err := client.RootWithContext(ctx, "invalid") + if assert.Error(t, err) { + apiErr := &errs.Error{} + if assert.ErrorAs(t, err, &apiErr) { + assert.Equal(t, 404, apiErr.StatusCode()) + assert.Equal(t, "The requested resource could not be found. Please see the certificate authority logs for more info.", apiErr.Err.Error()) + assert.NotEmpty(t, apiErr.RequestID) + + // TODO: include the below error in the JSON? It's currently only output to the CA logs + //assert.Equal(t, "/root/invalid was not found: certificate with fingerprint invalid was not found", apiErr.Msg) + } + } + assert.Nil(t, rootResponse) + + // done testing; stop and wait for the server to quit + err = c.Stop() + require.NoError(t, err) + + wg.Wait() +} From 5c2572c44397bbaf77a4e744e22a43aeb3dc30cf Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 01:55:35 +0100 Subject: [PATCH 05/16] Add support for user provider `X-Request-Id` header value --- ca/client.go | 16 +++++++++++++--- ca/client/requestid.go | 17 +++++++++++++++++ test/e2e/requestid_test.go | 26 +++++++++++++++++++++----- 3 files changed, 51 insertions(+), 8 deletions(-) create mode 100644 ca/client/requestid.go diff --git a/ca/client.go b/ca/client.go index 8930d8ee..d7ec2875 100644 --- a/ca/client.go +++ b/ca/client.go @@ -28,6 +28,7 @@ import ( "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority" "github.com/smallstep/certificates/authority/provisioner" + "github.com/smallstep/certificates/ca/client" "github.com/smallstep/certificates/ca/identity" "github.com/smallstep/certificates/errs" "go.step.sm/cli-utils/step" @@ -105,10 +106,19 @@ func (c *uaClient) PostWithContext(ctx context.Context, u, contentType string, b const requestIDHeader = "X-Request-Id" // enforceRequestID checks if the X-Request-Id HTTP header is filled. If it's -// empty, it'll generate a new request ID and set the header. +// empty, the context is searched for a request ID. If that's also empty, a new +// request ID is generated. func enforceRequestID(r *http.Request) { - if r.Header.Get(requestIDHeader) == "" { - r.Header.Set(requestIDHeader, xid.New().String()) + requestID := r.Header.Get(requestIDHeader) + if requestID == "" { + if reqID, ok := client.GetRequestID(r.Context()); ok && reqID != "" { + // TODO(hs): ensure the request ID from the context is fresh, and thus hasn't been + // used before by the client (unless it's a retry for the same request)? + requestID = reqID + } else { + requestID = xid.New().String() + } + r.Header.Set(requestIDHeader, requestID) } } diff --git a/ca/client/requestid.go b/ca/client/requestid.go new file mode 100644 index 00000000..de92f8c0 --- /dev/null +++ b/ca/client/requestid.go @@ -0,0 +1,17 @@ +package client + +import "context" + +type requestIDKey struct{} + +// WithRequestID returns a new context with the given requestID added to the +// context. +func WithRequestID(ctx context.Context, requestID string) context.Context { + return context.WithValue(ctx, requestIDKey{}, requestID) +} + +// GetRequestID returns the request id from the context if it exists. +func GetRequestID(ctx context.Context) (string, bool) { + v, ok := ctx.Value(requestIDKey{}).(string) + return v, ok +} diff --git a/test/e2e/requestid_test.go b/test/e2e/requestid_test.go index 7eccb4f4..a1afd423 100644 --- a/test/e2e/requestid_test.go +++ b/test/e2e/requestid_test.go @@ -11,6 +11,7 @@ import ( "github.com/smallstep/certificates/authority/config" "github.com/smallstep/certificates/ca" + "github.com/smallstep/certificates/ca/client" "github.com/smallstep/certificates/errs" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -57,8 +58,8 @@ func TestXxx(t *testing.T) { c, err := ca.New(cfg) require.NoError(t, err) - // instantiate a client for the CA - client, err := ca.NewClient( + // instantiate a client for the CA running at the random address + caClient, err := ca.NewClient( fmt.Sprintf("https://%s", randomAddress), ca.WithRootFile(rootFilepath), ) @@ -75,12 +76,12 @@ func TestXxx(t *testing.T) { // require OK health response as the baseline ctx := context.Background() - healthResponse, err := client.HealthWithContext(ctx) + healthResponse, err := caClient.HealthWithContext(ctx) assert.NoError(t, err) - require.Equal(t, "ok", healthResponse.Status) + assert.Equal(t, "ok", healthResponse.Status) // expect an error when retrieving an invalid root - rootResponse, err := client.RootWithContext(ctx, "invalid") + rootResponse, err := caClient.RootWithContext(ctx, "invalid") if assert.Error(t, err) { apiErr := &errs.Error{} if assert.ErrorAs(t, err, &apiErr) { @@ -94,6 +95,21 @@ func TestXxx(t *testing.T) { } assert.Nil(t, rootResponse) + // expect an error when retrieving an invalid root and provided request ID + rootResponse, err = caClient.RootWithContext(client.WithRequestID(ctx, "reqID"), "invalid") + if assert.Error(t, err) { + apiErr := &errs.Error{} + if assert.ErrorAs(t, err, &apiErr) { + assert.Equal(t, 404, apiErr.StatusCode()) + assert.Equal(t, "The requested resource could not be found. Please see the certificate authority logs for more info.", apiErr.Err.Error()) + assert.Equal(t, "reqID", apiErr.RequestID) + + // TODO: include the below error in the JSON? It's currently only output to the CA logs + //assert.Equal(t, "/root/invalid was not found: certificate with fingerprint invalid was not found", apiErr.Msg) + } + } + assert.Nil(t, rootResponse) + // done testing; stop and wait for the server to quit err = c.Stop() require.NoError(t, err) From 2255857b3a59a6e9bb8a665e9543770889451e41 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 10:50:49 +0100 Subject: [PATCH 06/16] Fix `client` shadowing and e2e request ID test case --- ca/client.go | 20 ++++++++++---------- test/e2e/requestid_test.go | 10 ++++++---- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/ca/client.go b/ca/client.go index d7ec2875..0c0f9907 100644 --- a/ca/client.go +++ b/ca/client.go @@ -397,8 +397,8 @@ func getTransportFromSHA256(endpoint, sum string) (http.RoundTripper, error) { if err != nil { return nil, err } - client := &Client{endpoint: u} - root, err := client.Root(sum) + caClient := &Client{endpoint: u} + root, err := caClient.Root(sum) if err != nil { return nil, err } @@ -759,14 +759,14 @@ func (c *Client) Renew(tr http.RoundTripper) (*api.SignResponse, error) { func (c *Client) RenewWithContext(ctx context.Context, tr http.RoundTripper) (*api.SignResponse, error) { var retried bool u := c.endpoint.ResolveReference(&url.URL{Path: "/renew"}) - client := &http.Client{Transport: tr} + caClient := &http.Client{Transport: tr} retry: req, err := http.NewRequestWithContext(ctx, "POST", u.String(), http.NoBody) if err != nil { return nil, err } req.Header.Set("Content-Type", "application/json") - resp, err := client.Do(req) + resp, err := caClient.Do(req) if err != nil { return nil, clientError(err) } @@ -836,14 +836,14 @@ func (c *Client) RekeyWithContext(ctx context.Context, req *api.RekeyRequest, tr return nil, errors.Wrap(err, "error marshaling request") } u := c.endpoint.ResolveReference(&url.URL{Path: "/rekey"}) - client := &http.Client{Transport: tr} + caClient := &http.Client{Transport: tr} retry: httpReq, err := http.NewRequestWithContext(ctx, "POST", u.String(), bytes.NewReader(body)) if err != nil { return nil, err } httpReq.Header.Set("Content-Type", "application/json") - resp, err := client.Do(httpReq) + resp, err := caClient.Do(httpReq) if err != nil { return nil, clientError(err) } @@ -875,16 +875,16 @@ func (c *Client) RevokeWithContext(ctx context.Context, req *api.RevokeRequest, if err != nil { return nil, errors.Wrap(err, "error marshaling request") } - var client *uaClient + var uaClient *uaClient retry: if tr != nil { - client = newClient(tr) + uaClient = newClient(tr) } else { - client = c.client + uaClient = c.client } u := c.endpoint.ResolveReference(&url.URL{Path: "/revoke"}) - resp, err := client.PostWithContext(ctx, u.String(), "application/json", bytes.NewReader(body)) + resp, err := uaClient.PostWithContext(ctx, u.String(), "application/json", bytes.NewReader(body)) if err != nil { return nil, clientError(err) } diff --git a/test/e2e/requestid_test.go b/test/e2e/requestid_test.go index a1afd423..2653039c 100644 --- a/test/e2e/requestid_test.go +++ b/test/e2e/requestid_test.go @@ -19,7 +19,7 @@ import ( "go.step.sm/crypto/pemutil" ) -func TestXxx(t *testing.T) { +func Test_reflectRequestID(t *testing.T) { dir := t.TempDir() m, err := minica.New(minica.WithName("Step E2E")) require.NoError(t, err) @@ -37,9 +37,11 @@ func TestXxx(t *testing.T) { require.NoError(t, err) // get a random address to listen on and connect to; currently no nicer way to get one before starting the server - l, err := net.Listen("tcp", "127.0.0.1:0") + l, err := net.Listen("tcp4", ":0") require.NoError(t, err) randomAddress := l.Addr().String() + _, port, err := net.SplitHostPort(l.Addr().String()) + require.NoError(t, err) err = l.Close() require.NoError(t, err) @@ -48,7 +50,7 @@ func TestXxx(t *testing.T) { IntermediateCert: intermediateCertFilepath, IntermediateKey: intermediateKeyFilepath, Address: randomAddress, // reuse the address that was just "reserved" - DNSNames: []string{"127.0.0.1", "stepca.localhost"}, + DNSNames: []string{"127.0.0.1", "[::1]", "localhost"}, AuthorityConfig: &config.AuthConfig{ AuthorityID: "stepca-test", DeploymentType: "standalone-test", @@ -60,7 +62,7 @@ func TestXxx(t *testing.T) { // instantiate a client for the CA running at the random address caClient, err := ca.NewClient( - fmt.Sprintf("https://%s", randomAddress), + fmt.Sprintf("https://localhost:%s", port), ca.WithRootFile(rootFilepath), ) require.NoError(t, err) From b83b8aa079b6c6711e27a2bfb2fd3404efefb402 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 11:09:40 +0100 Subject: [PATCH 07/16] Make random TCP address reservation more contained --- test/e2e/requestid_test.go | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/test/e2e/requestid_test.go b/test/e2e/requestid_test.go index 2653039c..e87b46e5 100644 --- a/test/e2e/requestid_test.go +++ b/test/e2e/requestid_test.go @@ -19,6 +19,25 @@ import ( "go.step.sm/crypto/pemutil" ) +// reserveAddress "reserves" a TCP address by opening a listener on a random +// port and immediately closing it. The address can then be assumed to be +// available for running a server on. +func reserveAddress(t *testing.T) string { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + if l, err = net.Listen("tcp6", "[::1]:0"); err != nil { + require.NoError(t, err, "failed to listen on a port") + } + } + + address := l.Addr().String() + err = l.Close() + require.NoError(t, err) + + return address +} + func Test_reflectRequestID(t *testing.T) { dir := t.TempDir() m, err := minica.New(minica.WithName("Step E2E")) @@ -37,19 +56,14 @@ func Test_reflectRequestID(t *testing.T) { require.NoError(t, err) // get a random address to listen on and connect to; currently no nicer way to get one before starting the server - l, err := net.Listen("tcp4", ":0") - require.NoError(t, err) - randomAddress := l.Addr().String() - _, port, err := net.SplitHostPort(l.Addr().String()) - require.NoError(t, err) - err = l.Close() - require.NoError(t, err) + // TODO(hs): find/implement a nicer way to expose the CA URL, similar to how e.g. httptest.Server exposes it? + address := reserveAddress(t) cfg := &config.Config{ Root: []string{rootFilepath}, IntermediateCert: intermediateCertFilepath, IntermediateKey: intermediateKeyFilepath, - Address: randomAddress, // reuse the address that was just "reserved" + Address: address, // reuse the address that was just "reserved" DNSNames: []string{"127.0.0.1", "[::1]", "localhost"}, AuthorityConfig: &config.AuthConfig{ AuthorityID: "stepca-test", @@ -62,7 +76,7 @@ func Test_reflectRequestID(t *testing.T) { // instantiate a client for the CA running at the random address caClient, err := ca.NewClient( - fmt.Sprintf("https://localhost:%s", port), + fmt.Sprintf("https://%s", address), ca.WithRootFile(rootFilepath), ) require.NoError(t, err) @@ -91,7 +105,7 @@ func Test_reflectRequestID(t *testing.T) { assert.Equal(t, "The requested resource could not be found. Please see the certificate authority logs for more info.", apiErr.Err.Error()) assert.NotEmpty(t, apiErr.RequestID) - // TODO: include the below error in the JSON? It's currently only output to the CA logs + // TODO: include the below error in the JSON? It's currently only output to the CA logs. Also see https://github.com/smallstep/certificates/pull/759 //assert.Equal(t, "/root/invalid was not found: certificate with fingerprint invalid was not found", apiErr.Msg) } } @@ -105,9 +119,6 @@ func Test_reflectRequestID(t *testing.T) { assert.Equal(t, 404, apiErr.StatusCode()) assert.Equal(t, "The requested resource could not be found. Please see the certificate authority logs for more info.", apiErr.Err.Error()) assert.Equal(t, "reqID", apiErr.RequestID) - - // TODO: include the below error in the JSON? It's currently only output to the CA logs - //assert.Equal(t, "/root/invalid was not found: certificate with fingerprint invalid was not found", apiErr.Msg) } } assert.Nil(t, rootResponse) From 535e2a96d5eb099f7af88ce622d13b89c0a25878 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 11:23:51 +0100 Subject: [PATCH 08/16] Fix the e2e request ID test (again) --- test/e2e/requestid_test.go | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/test/e2e/requestid_test.go b/test/e2e/requestid_test.go index e87b46e5..62b2feb1 100644 --- a/test/e2e/requestid_test.go +++ b/test/e2e/requestid_test.go @@ -19,23 +19,22 @@ import ( "go.step.sm/crypto/pemutil" ) -// reserveAddress "reserves" a TCP address by opening a listener on a random -// port and immediately closing it. The address can then be assumed to be +// reservePort "reserves" a TCP port by opening a listener on a random +// port and immediately closing it. The port can then be assumed to be // available for running a server on. -func reserveAddress(t *testing.T) string { +func reservePort(t *testing.T) (host, port string) { t.Helper() - l, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - if l, err = net.Listen("tcp6", "[::1]:0"); err != nil { - require.NoError(t, err, "failed to listen on a port") - } - } + l, err := net.Listen("tcp", ":0") + require.NoError(t, err) address := l.Addr().String() err = l.Close() require.NoError(t, err) - return address + host, port, err = net.SplitHostPort(address) + require.NoError(t, err) + + return } func Test_reflectRequestID(t *testing.T) { @@ -57,13 +56,13 @@ func Test_reflectRequestID(t *testing.T) { // get a random address to listen on and connect to; currently no nicer way to get one before starting the server // TODO(hs): find/implement a nicer way to expose the CA URL, similar to how e.g. httptest.Server exposes it? - address := reserveAddress(t) + host, port := reservePort(t) cfg := &config.Config{ Root: []string{rootFilepath}, IntermediateCert: intermediateCertFilepath, IntermediateKey: intermediateKeyFilepath, - Address: address, // reuse the address that was just "reserved" + Address: net.JoinHostPort(host, port), // reuse the address that was just "reserved" DNSNames: []string{"127.0.0.1", "[::1]", "localhost"}, AuthorityConfig: &config.AuthConfig{ AuthorityID: "stepca-test", @@ -76,7 +75,7 @@ func Test_reflectRequestID(t *testing.T) { // instantiate a client for the CA running at the random address caClient, err := ca.NewClient( - fmt.Sprintf("https://%s", address), + fmt.Sprintf("https://localhost:%s", port), ca.WithRootFile(rootFilepath), ) require.NoError(t, err) @@ -93,8 +92,10 @@ func Test_reflectRequestID(t *testing.T) { // require OK health response as the baseline ctx := context.Background() healthResponse, err := caClient.HealthWithContext(ctx) - assert.NoError(t, err) - assert.Equal(t, "ok", healthResponse.Status) + require.NoError(t, err) + if assert.NotNil(t, healthResponse) { + require.Equal(t, "ok", healthResponse.Status) + } // expect an error when retrieving an invalid root rootResponse, err := caClient.RootWithContext(ctx, "invalid") From 7e5f10927feb34d446e2b28ca394d65f9bbb72d8 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 13:18:10 +0100 Subject: [PATCH 09/16] Decouple request ID middleware from logging middleware --- authority/provisioner/webhook.go | 7 +- authority/provisioner/webhook_test.go | 8 +- ca/ca.go | 7 ++ errs/errors_test.go | 27 +++--- internal/requestid/requestid.go | 82 +++++++++++++++++++ .../requestid/requestid_test.go | 53 ++++++------ logging/context.go | 72 +--------------- logging/handler.go | 20 ++--- monitoring/monitoring.go | 3 +- 9 files changed, 155 insertions(+), 124 deletions(-) create mode 100644 internal/requestid/requestid.go rename logging/context_test.go => internal/requestid/requestid_test.go (65%) diff --git a/authority/provisioner/webhook.go b/authority/provisioner/webhook.go index c33dfa23..1e08b8b7 100644 --- a/authority/provisioner/webhook.go +++ b/authority/provisioner/webhook.go @@ -15,7 +15,7 @@ import ( "time" "github.com/pkg/errors" - "github.com/smallstep/certificates/logging" + "github.com/smallstep/certificates/internal/requestid" "github.com/smallstep/certificates/templates" "github.com/smallstep/certificates/webhook" "go.step.sm/linkedca" @@ -171,9 +171,8 @@ retry: return nil, err } - requestID, ok := logging.GetRequestID(ctx) - if ok { - req.Header.Set("X-Request-ID", requestID) + if requestID, ok := requestid.FromContext(ctx); ok { + req.Header.Set("X-Request-Id", requestID) } secret, err := base64.StdEncoding.DecodeString(w.Secret) diff --git a/authority/provisioner/webhook_test.go b/authority/provisioner/webhook_test.go index 60dcdbc7..4c80796f 100644 --- a/authority/provisioner/webhook_test.go +++ b/authority/provisioner/webhook_test.go @@ -17,7 +17,7 @@ import ( "testing" "time" - "github.com/smallstep/certificates/logging" + "github.com/smallstep/certificates/internal/requestid" "github.com/smallstep/certificates/webhook" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -101,10 +101,10 @@ func TestWebhookController_isCertTypeOK(t *testing.T) { } } -// withRequestID is a helper that calls into [logging.WithRequestID] and returns -// a new context with the requestID added to the provided context. +// withRequestID is a helper that calls into [requestid.NewContext] and returns +// a new context with the requestID added. func withRequestID(ctx context.Context, requestID string) context.Context { - return logging.WithRequestID(ctx, requestID) + return requestid.NewContext(ctx, requestID) } func TestWebhookController_Enrich(t *testing.T) { diff --git a/ca/ca.go b/ca/ca.go index 4146466d..ab4a1a9b 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -29,6 +29,7 @@ import ( "github.com/smallstep/certificates/cas/apiv1" "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/internal/metrix" + "github.com/smallstep/certificates/internal/requestid" "github.com/smallstep/certificates/logging" "github.com/smallstep/certificates/monitoring" "github.com/smallstep/certificates/scep" @@ -329,15 +330,21 @@ func (ca *CA) Init(cfg *config.Config) (*CA, error) { } // Add logger if configured + var legacyTraceHeader string if len(cfg.Logger) > 0 { logger, err := logging.New("ca", cfg.Logger) if err != nil { return nil, err } + legacyTraceHeader = logger.GetTraceHeader() handler = logger.Middleware(handler) insecureHandler = logger.Middleware(insecureHandler) } + // always use request ID middleware; traceHeader is provided for backwards compatibility (for now) + handler = requestid.New(legacyTraceHeader).Middleware(handler) + insecureHandler = requestid.New(legacyTraceHeader).Middleware(insecureHandler) + // Create context with all the necessary values. baseContext := buildContext(auth, scepAuthority, acmeDB, acmeLinker) diff --git a/errs/errors_test.go b/errs/errors_test.go index 7b83c8d9..11590d7d 100644 --- a/errs/errors_test.go +++ b/errs/errors_test.go @@ -2,8 +2,9 @@ package errs import ( "fmt" - "reflect" "testing" + + "github.com/stretchr/testify/assert" ) func TestError_MarshalJSON(t *testing.T) { @@ -27,13 +28,14 @@ func TestError_MarshalJSON(t *testing.T) { Err: tt.fields.Err, } got, err := e.MarshalJSON() - if (err != nil) != tt.wantErr { - t.Errorf("Error.MarshalJSON() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + assert.Error(t, err) + assert.Empty(t, got) return } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Error.MarshalJSON() = %s, want %s", got, tt.want) - } + + assert.NoError(t, err) + assert.Equal(t, tt.want, got) }) } } @@ -54,13 +56,14 @@ func TestError_UnmarshalJSON(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { e := new(Error) - if err := e.UnmarshalJSON(tt.args.data); (err != nil) != tt.wantErr { - t.Errorf("Error.UnmarshalJSON() error = %v, wantErr %v", err, tt.wantErr) - } - //nolint:govet // best option - if !reflect.DeepEqual(tt.expected, e) { - t.Errorf("Error.UnmarshalJSON() wants = %+v, got %+v", tt.expected, e) + err := e.UnmarshalJSON(tt.args.data) + if tt.wantErr { + assert.Error(t, err) + return } + + assert.NoError(t, err) + assert.Equal(t, tt.expected, e) }) } } diff --git a/internal/requestid/requestid.go b/internal/requestid/requestid.go new file mode 100644 index 00000000..97f58f8c --- /dev/null +++ b/internal/requestid/requestid.go @@ -0,0 +1,82 @@ +package requestid + +import ( + "context" + "net/http" + + "github.com/rs/xid" +) + +const ( + // requestIDHeader is the header name used for propagating request IDs. If + // available in an HTTP request, it'll be used instead of the X-Smallstep-Id + // header. It'll always be used in response and set to the request ID. + requestIDHeader = "X-Request-Id" + + // defaultTraceHeader is the default Smallstep tracing header that's currently + // in use. It is used as a fallback to retrieve a request ID from, if the + // "X-Request-Id" request header is not set. + defaultTraceHeader = "X-Smallstep-Id" +) + +type Handler struct { + legacyTraceHeader string +} + +// New creates a new request ID [handler]. It takes a trace header, +// which is used keep the legacy behavior intact, which relies on the +// X-Smallstep-Id header instead of X-Request-Id. +func New(legacyTraceHeader string) *Handler { + if legacyTraceHeader == "" { + legacyTraceHeader = defaultTraceHeader + } + + return &Handler{legacyTraceHeader: legacyTraceHeader} +} + +// Middleware wraps an [http.Handler] with request ID extraction +// from the X-Reqeust-Id header by default, or from the X-Smallstep-Id +// header if not set. If both are not set, a new request ID is generated. +// In all cases, the request ID is added to the request context, and +// set to be reflected in the response. +func (h *Handler) Middleware(next http.Handler) http.Handler { + fn := func(w http.ResponseWriter, req *http.Request) { + requestID := req.Header.Get(requestIDHeader) + if requestID == "" { + requestID = req.Header.Get(h.legacyTraceHeader) + } + + if requestID == "" { + requestID = newRequestID() + req.Header.Set(h.legacyTraceHeader, requestID) // legacy behavior + } + + // immediately set the request ID to be reflected in the response + w.Header().Set(requestIDHeader, requestID) + + // continue down the handler chain + ctx := NewContext(req.Context(), requestID) + next.ServeHTTP(w, req.WithContext(ctx)) + } + return http.HandlerFunc(fn) +} + +// newRequestID creates a new request ID using github.com/rs/xid. +func newRequestID() string { + return xid.New().String() +} + +type requestIDKey struct{} + +// NewContext returns a new context with the given request ID added to the +// context. +func NewContext(ctx context.Context, requestID string) context.Context { + return context.WithValue(ctx, requestIDKey{}, requestID) +} + +// FromContext returns the request ID from the context if it exists and +// is not the empty value. +func FromContext(ctx context.Context) (string, bool) { + v, ok := ctx.Value(requestIDKey{}).(string) + return v, ok && v != "" +} diff --git a/logging/context_test.go b/internal/requestid/requestid_test.go similarity index 65% rename from logging/context_test.go rename to internal/requestid/requestid_test.go index da993f7b..4d0e872d 100644 --- a/logging/context_test.go +++ b/internal/requestid/requestid_test.go @@ -1,4 +1,4 @@ -package logging +package requestid import ( "net/http" @@ -10,12 +10,13 @@ import ( ) func newRequest(t *testing.T) *http.Request { + t.Helper() r, err := http.NewRequest(http.MethodGet, "https://example.com", http.NoBody) require.NoError(t, err) return r } -func TestRequestID(t *testing.T) { +func Test_Middleware(t *testing.T) { requestWithID := newRequest(t) requestWithID.Header.Set("X-Request-Id", "reqID") requestWithoutID := newRequest(t) @@ -23,20 +24,19 @@ func TestRequestID(t *testing.T) { requestWithEmptyHeader.Header.Set("X-Request-Id", "") requestWithSmallstepID := newRequest(t) requestWithSmallstepID.Header.Set("X-Smallstep-Id", "smallstepID") - tests := []struct { - name string - headerName string - handler http.HandlerFunc - req *http.Request + name string + traceHeader string + next http.HandlerFunc + req *http.Request }{ { - name: "default-request-id", - headerName: defaultTraceIDHeader, - handler: func(w http.ResponseWriter, r *http.Request) { + name: "default-request-id", + traceHeader: defaultTraceHeader, + next: func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("X-Smallstep-Id")) assert.Equal(t, "reqID", r.Header.Get("X-Request-Id")) - reqID, ok := GetRequestID(r.Context()) + reqID, ok := FromContext(r.Context()) if assert.True(t, ok) { assert.Equal(t, "reqID", reqID) } @@ -45,13 +45,13 @@ func TestRequestID(t *testing.T) { req: requestWithID, }, { - name: "no-request-id", - headerName: "X-Request-Id", - handler: func(w http.ResponseWriter, r *http.Request) { + name: "no-request-id", + traceHeader: "X-Request-Id", + next: func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("X-Smallstep-Id")) value := r.Header.Get("X-Request-Id") assert.NotEmpty(t, value) - reqID, ok := GetRequestID(r.Context()) + reqID, ok := FromContext(r.Context()) if assert.True(t, ok) { assert.Equal(t, value, reqID) } @@ -60,13 +60,13 @@ func TestRequestID(t *testing.T) { req: requestWithoutID, }, { - name: "empty-header-name", - headerName: "", - handler: func(w http.ResponseWriter, r *http.Request) { + name: "empty-header", + traceHeader: "", + next: func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("X-Request-Id")) value := r.Header.Get("X-Smallstep-Id") assert.NotEmpty(t, value) - reqID, ok := GetRequestID(r.Context()) + reqID, ok := FromContext(r.Context()) if assert.True(t, ok) { assert.Equal(t, value, reqID) } @@ -75,12 +75,12 @@ func TestRequestID(t *testing.T) { req: requestWithEmptyHeader, }, { - name: "fallback-header-name", - headerName: defaultTraceIDHeader, - handler: func(w http.ResponseWriter, r *http.Request) { + name: "fallback-header-name", + traceHeader: defaultTraceHeader, + next: func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("X-Request-Id")) assert.Equal(t, "smallstepID", r.Header.Get("X-Smallstep-Id")) - reqID, ok := GetRequestID(r.Context()) + reqID, ok := FromContext(r.Context()) if assert.True(t, ok) { assert.Equal(t, "smallstepID", reqID) } @@ -91,8 +91,11 @@ func TestRequestID(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - h := RequestID(tt.headerName) - h(tt.handler).ServeHTTP(httptest.NewRecorder(), tt.req) + handler := New(tt.traceHeader).Middleware(tt.next) + + w := httptest.NewRecorder() + handler.ServeHTTP(w, tt.req) + assert.NotEmpty(t, w.Header().Get("X-Request-Id")) }) } } diff --git a/logging/context.go b/logging/context.go index 9d7a7071..212e2560 100644 --- a/logging/context.go +++ b/logging/context.go @@ -2,82 +2,18 @@ package logging import ( "context" - "net/http" - - "github.com/rs/xid" -) - -type key int - -const ( - // RequestIDKey is the context key that should store the request identifier. - RequestIDKey key = iota - // UserIDKey is the context key that should store the user identifier. - UserIDKey ) -// NewRequestID creates a new request id using github.com/rs/xid. -func NewRequestID() string { - return xid.New().String() -} - -// requestIDHeader is the header name used for propagating request IDs. If -// available in an HTTP request, it'll be used instead of the X-Smallstep-Id -// header. It'll always be used in response and set to the request ID. -const requestIDHeader = "X-Request-Id" - -// RequestID returns a new middleware that obtains the current request ID -// and sets it in the context. It first tries to read the request ID from -// the "X-Request-Id" header. If that's not set, it tries to read it from -// the provided header name. If the header does not exist or its value is -// the empty string, it uses github.com/rs/xid to create a new one. -func RequestID(headerName string) func(next http.Handler) http.Handler { - if headerName == "" { - headerName = defaultTraceIDHeader - } - return func(next http.Handler) http.Handler { - fn := func(w http.ResponseWriter, req *http.Request) { - requestID := req.Header.Get(requestIDHeader) - if requestID == "" { - requestID = req.Header.Get(headerName) - } - - if requestID == "" { - requestID = NewRequestID() - req.Header.Set(headerName, requestID) - } - - // immediately set the request ID to be reflected in the response - w.Header().Set(requestIDHeader, requestID) - - // continue down the handler chain - ctx := WithRequestID(req.Context(), requestID) - next.ServeHTTP(w, req.WithContext(ctx)) - } - return http.HandlerFunc(fn) - } -} - -// WithRequestID returns a new context with the given requestID added to the -// context. -func WithRequestID(ctx context.Context, requestID string) context.Context { - return context.WithValue(ctx, RequestIDKey, requestID) -} - -// GetRequestID returns the request id from the context if it exists. -func GetRequestID(ctx context.Context) (string, bool) { - v, ok := ctx.Value(RequestIDKey).(string) - return v, ok -} +type userIDKey struct{} // WithUserID decodes the token, extracts the user from the payload and stores // it in the context. func WithUserID(ctx context.Context, userID string) context.Context { - return context.WithValue(ctx, UserIDKey, userID) + return context.WithValue(ctx, userIDKey{}, userID) } // GetUserID returns the request id from the context if it exists. func GetUserID(ctx context.Context) (string, bool) { - v, ok := ctx.Value(UserIDKey).(string) - return v, ok + v, ok := ctx.Value(userIDKey{}).(string) + return v, ok && v != "" } diff --git a/logging/handler.go b/logging/handler.go index a8b77d60..77287690 100644 --- a/logging/handler.go +++ b/logging/handler.go @@ -9,6 +9,7 @@ import ( "time" "github.com/sirupsen/logrus" + "github.com/smallstep/certificates/internal/requestid" ) // LoggerHandler creates a logger handler @@ -29,16 +30,15 @@ type options struct { // NewLoggerHandler returns the given http.Handler with the logger integrated. func NewLoggerHandler(name string, logger *Logger, next http.Handler) http.Handler { - h := RequestID(logger.GetTraceHeader()) onlyTraceHealthEndpoint, _ := strconv.ParseBool(os.Getenv("STEP_LOGGER_ONLY_TRACE_HEALTH_ENDPOINT")) - return h(&LoggerHandler{ + return &LoggerHandler{ name: name, logger: logger.GetImpl(), options: options{ onlyTraceHealthEndpoint: onlyTraceHealthEndpoint, }, next: next, - }) + } } // ServeHTTP implements the http.Handler and call to the handler to log with a @@ -54,14 +54,14 @@ func (l *LoggerHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { // writeEntry writes to the Logger writer the request information in the logger. func (l *LoggerHandler) writeEntry(w ResponseLogger, r *http.Request, t time.Time, d time.Duration) { - var reqID, user string + var requestID, userID string ctx := r.Context() - if v, ok := ctx.Value(RequestIDKey).(string); ok && v != "" { - reqID = v + if v, ok := requestid.FromContext(ctx); ok { + requestID = v } - if v, ok := ctx.Value(UserIDKey).(string); ok && v != "" { - user = v + if v, ok := GetUserID(ctx); ok && v != "" { + userID = v } // Remote hostname @@ -85,10 +85,10 @@ func (l *LoggerHandler) writeEntry(w ResponseLogger, r *http.Request, t time.Tim status := w.StatusCode() fields := logrus.Fields{ - "request-id": reqID, + "request-id": requestID, "remote-address": addr, "name": l.name, - "user-id": user, + "user-id": userID, "time": t.Format(time.RFC3339), "duration-ns": d.Nanoseconds(), "duration": d.String(), diff --git a/monitoring/monitoring.go b/monitoring/monitoring.go index a0d0886b..7c88ab3b 100644 --- a/monitoring/monitoring.go +++ b/monitoring/monitoring.go @@ -9,6 +9,7 @@ import ( "github.com/newrelic/go-agent/v3/newrelic" "github.com/pkg/errors" + "github.com/smallstep/certificates/internal/requestid" "github.com/smallstep/certificates/logging" ) @@ -82,7 +83,7 @@ func newRelicMiddleware(app *newrelic.Application) Middleware { txn.AddAttribute("httpResponseCode", strconv.Itoa(status)) // Add custom attributes - if v, ok := logging.GetRequestID(r.Context()); ok { + if v, ok := requestid.FromContext(r.Context()); ok { txn.AddAttribute("request.id", v) } From 06696e64926f5247b9e1dea2a8839b398f731fbd Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 13:37:51 +0100 Subject: [PATCH 10/16] Move user ID handling to `userid` package --- internal/userid/userid.go | 20 ++++++++++++++++++++ logging/context.go | 19 ------------------- logging/handler.go | 3 ++- 3 files changed, 22 insertions(+), 20 deletions(-) create mode 100644 internal/userid/userid.go delete mode 100644 logging/context.go diff --git a/internal/userid/userid.go b/internal/userid/userid.go new file mode 100644 index 00000000..bab4908f --- /dev/null +++ b/internal/userid/userid.go @@ -0,0 +1,20 @@ +package userid + +import "context" + +type userIDKey struct{} + +// NewContext returns a new context with the given user ID added to the +// context. +// TODO(hs): this doesn't seem to be used / set currently; implement +// when/where it makes sense. +func NewContext(ctx context.Context, userID string) context.Context { + return context.WithValue(ctx, userIDKey{}, userID) +} + +// FromContext returns the user ID from the context if it exists +// and is not empty. +func FromContext(ctx context.Context) (string, bool) { + v, ok := ctx.Value(userIDKey{}).(string) + return v, ok && v != "" +} diff --git a/logging/context.go b/logging/context.go deleted file mode 100644 index 212e2560..00000000 --- a/logging/context.go +++ /dev/null @@ -1,19 +0,0 @@ -package logging - -import ( - "context" -) - -type userIDKey struct{} - -// WithUserID decodes the token, extracts the user from the payload and stores -// it in the context. -func WithUserID(ctx context.Context, userID string) context.Context { - return context.WithValue(ctx, userIDKey{}, userID) -} - -// GetUserID returns the request id from the context if it exists. -func GetUserID(ctx context.Context) (string, bool) { - v, ok := ctx.Value(userIDKey{}).(string) - return v, ok && v != "" -} diff --git a/logging/handler.go b/logging/handler.go index 77287690..a29383b2 100644 --- a/logging/handler.go +++ b/logging/handler.go @@ -10,6 +10,7 @@ import ( "github.com/sirupsen/logrus" "github.com/smallstep/certificates/internal/requestid" + "github.com/smallstep/certificates/internal/userid" ) // LoggerHandler creates a logger handler @@ -60,7 +61,7 @@ func (l *LoggerHandler) writeEntry(w ResponseLogger, r *http.Request, t time.Tim if v, ok := requestid.FromContext(ctx); ok { requestID = v } - if v, ok := GetUserID(ctx); ok && v != "" { + if v, ok := userid.FromContext(ctx); ok { userID = v } From 532b9df0a3cbf312ef0e54aa8b350c00309e6bab Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 13:57:37 +0100 Subject: [PATCH 11/16] Improve CA client request ID handling --- ca/client.go | 15 +++-- ca/client/requestid.go | 11 ++-- ca/client_test.go | 120 +++++++++++++++++++++++++------------ test/e2e/requestid_test.go | 2 +- 4 files changed, 95 insertions(+), 53 deletions(-) diff --git a/ca/client.go b/ca/client.go index 0c0f9907..9e245cd7 100644 --- a/ca/client.go +++ b/ca/client.go @@ -109,9 +109,8 @@ const requestIDHeader = "X-Request-Id" // empty, the context is searched for a request ID. If that's also empty, a new // request ID is generated. func enforceRequestID(r *http.Request) { - requestID := r.Header.Get(requestIDHeader) - if requestID == "" { - if reqID, ok := client.GetRequestID(r.Context()); ok && reqID != "" { + if requestID := r.Header.Get(requestIDHeader); requestID == "" { + if reqID, ok := client.RequestIDFromContext(r.Context()); ok { // TODO(hs): ensure the request ID from the context is fresh, and thus hasn't been // used before by the client (unless it's a retry for the same request)? requestID = reqID @@ -759,14 +758,14 @@ func (c *Client) Renew(tr http.RoundTripper) (*api.SignResponse, error) { func (c *Client) RenewWithContext(ctx context.Context, tr http.RoundTripper) (*api.SignResponse, error) { var retried bool u := c.endpoint.ResolveReference(&url.URL{Path: "/renew"}) - caClient := &http.Client{Transport: tr} + httpClient := &http.Client{Transport: tr} retry: req, err := http.NewRequestWithContext(ctx, "POST", u.String(), http.NoBody) if err != nil { return nil, err } req.Header.Set("Content-Type", "application/json") - resp, err := caClient.Do(req) + resp, err := httpClient.Do(req) if err != nil { return nil, clientError(err) } @@ -836,14 +835,14 @@ func (c *Client) RekeyWithContext(ctx context.Context, req *api.RekeyRequest, tr return nil, errors.Wrap(err, "error marshaling request") } u := c.endpoint.ResolveReference(&url.URL{Path: "/rekey"}) - caClient := &http.Client{Transport: tr} + httpClient := &http.Client{Transport: tr} retry: httpReq, err := http.NewRequestWithContext(ctx, "POST", u.String(), bytes.NewReader(body)) if err != nil { return nil, err } httpReq.Header.Set("Content-Type", "application/json") - resp, err := caClient.Do(httpReq) + resp, err := httpClient.Do(httpReq) if err != nil { return nil, clientError(err) } @@ -1530,7 +1529,7 @@ func readError(r *http.Response) error { defer r.Body.Close() apiErr := new(errs.Error) if err := json.NewDecoder(r.Body).Decode(apiErr); err != nil { - return err + return fmt.Errorf("failed decoding CA error response: %w", err) } apiErr.RequestID = r.Header.Get("X-Request-Id") return apiErr diff --git a/ca/client/requestid.go b/ca/client/requestid.go index de92f8c0..2bebb7e5 100644 --- a/ca/client/requestid.go +++ b/ca/client/requestid.go @@ -4,14 +4,15 @@ import "context" type requestIDKey struct{} -// WithRequestID returns a new context with the given requestID added to the +// NewRequestIDContext returns a new context with the given request ID added to the // context. -func WithRequestID(ctx context.Context, requestID string) context.Context { +func NewRequestIDContext(ctx context.Context, requestID string) context.Context { return context.WithValue(ctx, requestIDKey{}, requestID) } -// GetRequestID returns the request id from the context if it exists. -func GetRequestID(ctx context.Context) (string, bool) { +// RequestIDFromContext returns the request ID from the context if it exists. +// and is not empty. +func RequestIDFromContext(ctx context.Context) (string, bool) { v, ok := ctx.Value(requestIDKey{}).(string) - return v, ok + return v, ok && v != "" } diff --git a/ca/client_test.go b/ca/client_test.go index 6292e3ea..6fe8a135 100644 --- a/ca/client_test.go +++ b/ca/client_test.go @@ -17,16 +17,17 @@ import ( "testing" "time" - "go.step.sm/crypto/x509util" - "golang.org/x/crypto/ssh" - - "github.com/smallstep/assert" + sassert "github.com/smallstep/assert" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/api/read" "github.com/smallstep/certificates/api/render" "github.com/smallstep/certificates/authority" "github.com/smallstep/certificates/authority/provisioner" + "github.com/smallstep/certificates/ca/client" "github.com/smallstep/certificates/errs" + "github.com/stretchr/testify/assert" + "go.step.sm/crypto/x509util" + "golang.org/x/crypto/ssh" ) const ( @@ -196,7 +197,7 @@ func TestClient_Version(t *testing.T) { if got != nil { t.Errorf("Client.Version() = %v, want nil", got) } - assert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) + sassert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Version() = %v, want %v", got, tt.response) @@ -247,7 +248,7 @@ func TestClient_Health(t *testing.T) { if got != nil { t.Errorf("Client.Health() = %v, want nil", got) } - assert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) + sassert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Health() = %v, want %v", got, tt.response) @@ -304,7 +305,7 @@ func TestClient_Root(t *testing.T) { if got != nil { t.Errorf("Client.Root() = %v, want nil", got) } - assert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) + sassert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Root() = %v, want %v", got, tt.response) @@ -359,7 +360,7 @@ func TestClient_Sign(t *testing.T) { body := new(api.SignRequest) if err := read.JSON(req.Body, body); err != nil { e, ok := tt.response.(error) - assert.Fatal(t, ok, "response expected to be error type") + sassert.Fatal(t, ok, "response expected to be error type") render.Error(w, e) return } else if !equalJSON(t, body, tt.request) { @@ -386,7 +387,7 @@ func TestClient_Sign(t *testing.T) { if got != nil { t.Errorf("Client.Sign() = %v, want nil", got) } - assert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) + sassert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Sign() = %v, want %v", got, tt.response) @@ -431,7 +432,7 @@ func TestClient_Revoke(t *testing.T) { body := new(api.RevokeRequest) if err := read.JSON(req.Body, body); err != nil { e, ok := tt.response.(error) - assert.Fatal(t, ok, "response expected to be error type") + sassert.Fatal(t, ok, "response expected to be error type") render.Error(w, e) return } else if !equalJSON(t, body, tt.request) { @@ -458,7 +459,7 @@ func TestClient_Revoke(t *testing.T) { if got != nil { t.Errorf("Client.Revoke() = %v, want nil", got) } - assert.HasPrefix(t, err.Error(), tt.expectedErr.Error()) + sassert.HasPrefix(t, err.Error(), tt.expectedErr.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Revoke() = %v, want %v", got, tt.response) @@ -520,10 +521,10 @@ func TestClient_Renew(t *testing.T) { } var sc render.StatusCodedError - if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - assert.Equals(t, sc.StatusCode(), tt.responseCode) + if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { + sassert.Equals(t, sc.StatusCode(), tt.responseCode) } - assert.HasPrefix(t, err.Error(), tt.err.Error()) + sassert.HasPrefix(t, err.Error(), tt.err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Renew() = %v, want %v", got, tt.response) @@ -589,10 +590,10 @@ func TestClient_RenewWithToken(t *testing.T) { } var sc render.StatusCodedError - if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - assert.Equals(t, sc.StatusCode(), tt.responseCode) + if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { + sassert.Equals(t, sc.StatusCode(), tt.responseCode) } - assert.HasPrefix(t, err.Error(), tt.err.Error()) + sassert.HasPrefix(t, err.Error(), tt.err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.RenewWithToken() = %v, want %v", got, tt.response) @@ -659,10 +660,10 @@ func TestClient_Rekey(t *testing.T) { } var sc render.StatusCodedError - if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - assert.Equals(t, sc.StatusCode(), tt.responseCode) + if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { + sassert.Equals(t, sc.StatusCode(), tt.responseCode) } - assert.HasPrefix(t, err.Error(), tt.err.Error()) + sassert.HasPrefix(t, err.Error(), tt.err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Renew() = %v, want %v", got, tt.response) @@ -722,7 +723,7 @@ func TestClient_Provisioners(t *testing.T) { if got != nil { t.Errorf("Client.Provisioners() = %v, want nil", got) } - assert.HasPrefix(t, errs.InternalServerErrorDefaultMsg, err.Error()) + sassert.HasPrefix(t, errs.InternalServerErrorDefaultMsg, err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Provisioners() = %v, want %v", got, tt.response) @@ -781,10 +782,10 @@ func TestClient_ProvisionerKey(t *testing.T) { } var sc render.StatusCodedError - if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - assert.Equals(t, sc.StatusCode(), tt.responseCode) + if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { + sassert.Equals(t, sc.StatusCode(), tt.responseCode) } - assert.HasPrefix(t, tt.err.Error(), err.Error()) + sassert.HasPrefix(t, tt.err.Error(), err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.ProvisionerKey() = %v, want %v", got, tt.response) @@ -841,10 +842,10 @@ func TestClient_Roots(t *testing.T) { t.Errorf("Client.Roots() = %v, want nil", got) } var sc render.StatusCodedError - if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - assert.Equals(t, sc.StatusCode(), tt.responseCode) + if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { + sassert.Equals(t, sc.StatusCode(), tt.responseCode) } - assert.HasPrefix(t, err.Error(), tt.err.Error()) + sassert.HasPrefix(t, err.Error(), tt.err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Roots() = %v, want %v", got, tt.response) @@ -900,10 +901,10 @@ func TestClient_Federation(t *testing.T) { t.Errorf("Client.Federation() = %v, want nil", got) } var sc render.StatusCodedError - if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - assert.Equals(t, sc.StatusCode(), tt.responseCode) + if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { + sassert.Equals(t, sc.StatusCode(), tt.responseCode) } - assert.HasPrefix(t, tt.err.Error(), err.Error()) + sassert.HasPrefix(t, tt.err.Error(), err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.Federation() = %v, want %v", got, tt.response) @@ -963,10 +964,10 @@ func TestClient_SSHRoots(t *testing.T) { t.Errorf("Client.SSHKeys() = %v, want nil", got) } var sc render.StatusCodedError - if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - assert.Equals(t, sc.StatusCode(), tt.responseCode) + if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { + sassert.Equals(t, sc.StatusCode(), tt.responseCode) } - assert.HasPrefix(t, tt.err.Error(), err.Error()) + sassert.HasPrefix(t, tt.err.Error(), err.Error()) default: if !reflect.DeepEqual(got, tt.response) { t.Errorf("Client.SSHKeys() = %v, want %v", got, tt.response) @@ -1069,11 +1070,11 @@ func TestClient_RootFingerprintWithServer(t *testing.T) { defer srv.Close() client, err := NewClient(srv.URL+"/sign", WithRootFile("testdata/secrets/root_ca.crt")) - assert.FatalError(t, err) + sassert.FatalError(t, err) fp, err := client.RootFingerprint() - assert.FatalError(t, err) - assert.Equals(t, "ef742f95dc0d8aa82d3cca4017af6dac3fce84290344159891952d18c53eefe7", fp) + sassert.FatalError(t, err) + sassert.Equals(t, "ef742f95dc0d8aa82d3cca4017af6dac3fce84290344159891952d18c53eefe7", fp) } func TestClient_SSHBastion(t *testing.T) { @@ -1126,10 +1127,10 @@ func TestClient_SSHBastion(t *testing.T) { } if tt.responseCode != 200 { var sc render.StatusCodedError - if assert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - assert.Equals(t, sc.StatusCode(), tt.responseCode) + if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { + sassert.Equals(t, sc.StatusCode(), tt.responseCode) } - assert.HasPrefix(t, err.Error(), tt.err.Error()) + sassert.HasPrefix(t, err.Error(), tt.err.Error()) } default: if !reflect.DeepEqual(got, tt.response) { @@ -1164,3 +1165,44 @@ func TestClient_GetCaURL(t *testing.T) { }) } } + +func Test_enforceRequestID(t *testing.T) { + set := httptest.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + set.Header.Set("X-Request-Id", "already-set") + inContext := httptest.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + inContext = inContext.WithContext(client.NewRequestIDContext(inContext.Context(), "from-context")) + new := httptest.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + + tests := []struct { + name string + r *http.Request + want string + }{ + { + name: "set", + r: set, + want: "already-set", + }, + { + name: "context", + r: inContext, + want: "from-context", + }, + { + name: "new", + r: new, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + enforceRequestID(tt.r) + + v := tt.r.Header.Get("X-Request-Id") + if assert.NotEmpty(t, v) { + if tt.want != "" { + assert.Equal(t, tt.want, v) + } + } + }) + } +} diff --git a/test/e2e/requestid_test.go b/test/e2e/requestid_test.go index 62b2feb1..d2f968c3 100644 --- a/test/e2e/requestid_test.go +++ b/test/e2e/requestid_test.go @@ -113,7 +113,7 @@ func Test_reflectRequestID(t *testing.T) { assert.Nil(t, rootResponse) // expect an error when retrieving an invalid root and provided request ID - rootResponse, err = caClient.RootWithContext(client.WithRequestID(ctx, "reqID"), "invalid") + rootResponse, err = caClient.RootWithContext(client.NewRequestIDContext(ctx, "reqID"), "invalid") if assert.Error(t, err) { apiErr := &errs.Error{} if assert.ErrorAs(t, err, &apiErr) { From b9d6bfc1eb5a5476530c5ce26b91aade6ed36bad Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 28 Feb 2024 14:39:38 +0100 Subject: [PATCH 12/16] Cleanup CA client tests by removing `smallstep/assert` --- ca/client_test.go | 582 +++++++++++++++-------------------------- ca/tls_options_test.go | 22 +- ca/tls_test.go | 30 +-- 3 files changed, 242 insertions(+), 392 deletions(-) diff --git a/ca/client_test.go b/ca/client_test.go index 6fe8a135..5fd11179 100644 --- a/ca/client_test.go +++ b/ca/client_test.go @@ -9,15 +9,14 @@ import ( "encoding/json" "encoding/pem" "errors" - "fmt" "net/http" "net/http/httptest" "net/url" "reflect" + "strings" "testing" "time" - sassert "github.com/smallstep/assert" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/api/read" "github.com/smallstep/certificates/api/render" @@ -26,6 +25,7 @@ import ( "github.com/smallstep/certificates/ca/client" "github.com/smallstep/certificates/errs" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.step.sm/crypto/x509util" "golang.org/x/crypto/ssh" ) @@ -107,52 +107,49 @@ DCbKzWTW8lqVdp9Kyf7XEhhc2R8C5w== -----END CERTIFICATE REQUEST-----` ) -func mustKey() *ecdsa.PrivateKey { +func mustKey(t *testing.T) *ecdsa.PrivateKey { + t.Helper() priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) - if err != nil { - panic(err) - } + require.NoError(t, err) return priv } -func parseCertificate(data string) *x509.Certificate { +func parseCertificate(t *testing.T, data string) *x509.Certificate { + t.Helper() block, _ := pem.Decode([]byte(data)) if block == nil { - panic("failed to parse certificate PEM") + require.Fail(t, "failed to parse certificate PEM") + return nil } cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - panic("failed to parse certificate: " + err.Error()) - } + require.NoError(t, err, "failed to parse certificate") return cert } -func parseCertificateRequest(string) *x509.CertificateRequest { +func parseCertificateRequest(t *testing.T, csrPEM string) *x509.CertificateRequest { + t.Helper() block, _ := pem.Decode([]byte(csrPEM)) if block == nil { - panic("failed to parse certificate request PEM") + require.Fail(t, "failed to parse certificate request PEM") + return nil } csr, err := x509.ParseCertificateRequest(block.Bytes) - if err != nil { - panic("failed to parse certificate request: " + err.Error()) - } + require.NoError(t, err, "failed to parse certificate request") return csr } func equalJSON(t *testing.T, a, b interface{}) bool { + t.Helper() if reflect.DeepEqual(a, b) { return true } + ab, err := json.Marshal(a) - if err != nil { - t.Error(err) - return false - } + require.NoError(t, err) + bb, err := json.Marshal(b) - if err != nil { - t.Error(err) - return false - } + require.NoError(t, err) + return bytes.Equal(ab, bb) } @@ -177,32 +174,23 @@ func TestClient_Version(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.Version() - if (err != nil) != tt.wantErr { - t.Errorf("Client.Version() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + assert.EqualError(t, err, tt.expectedErr.Error()) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Version() = %v, want nil", got) - } - sassert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Version() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } @@ -227,40 +215,30 @@ func TestClient_Health(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.Health() - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.Health() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + assert.EqualError(t, err, tt.expectedErr.Error()) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Health() = %v, want nil", got) - } - sassert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Health() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } func TestClient_Root(t *testing.T) { ok := &api.RootResponse{ - RootPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + RootPEM: api.Certificate{Certificate: parseCertificate(t, rootPEM)}, } tests := []struct { @@ -281,10 +259,7 @@ func TestClient_Root(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { expected := "/root/" + tt.shasum @@ -295,37 +270,31 @@ func TestClient_Root(t *testing.T) { }) got, err := c.Root(tt.shasum) - if (err != nil) != tt.wantErr { - t.Errorf("Client.Root() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + assert.EqualError(t, err, tt.expectedErr.Error()) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Root() = %v, want nil", got) - } - sassert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Root() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } func TestClient_Sign(t *testing.T) { ok := &api.SignResponse{ - ServerPEM: api.Certificate{Certificate: parseCertificate(certPEM)}, - CaPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + ServerPEM: api.Certificate{Certificate: parseCertificate(t, certPEM)}, + CaPEM: api.Certificate{Certificate: parseCertificate(t, rootPEM)}, CertChainPEM: []api.Certificate{ - {Certificate: parseCertificate(certPEM)}, - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, certPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, } request := &api.SignRequest{ - CsrPEM: api.CertificateRequest{CertificateRequest: parseCertificateRequest(csrPEM)}, + CsrPEM: api.CertificateRequest{CertificateRequest: parseCertificateRequest(t, csrPEM)}, OTT: "the-ott", NotBefore: api.NewTimeDuration(time.Now()), NotAfter: api.NewTimeDuration(time.Now().AddDate(0, 1, 0)), @@ -351,16 +320,13 @@ func TestClient_Sign(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { body := new(api.SignRequest) if err := read.JSON(req.Body, body); err != nil { e, ok := tt.response.(error) - sassert.Fatal(t, ok, "response expected to be error type") + require.True(t, ok, "response expected to be error type") render.Error(w, e) return } else if !equalJSON(t, body, tt.request) { @@ -376,23 +342,16 @@ func TestClient_Sign(t *testing.T) { }) got, err := c.Sign(tt.request) - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.Sign() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + assert.EqualError(t, err, tt.expectedErr.Error()) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Sign() = %v, want nil", got) - } - sassert.HasPrefix(t, tt.expectedErr.Error(), err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Sign() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } @@ -423,16 +382,13 @@ func TestClient_Revoke(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { body := new(api.RevokeRequest) if err := read.JSON(req.Body, body); err != nil { e, ok := tt.response.(error) - sassert.Fatal(t, ok, "response expected to be error type") + require.True(t, ok, "response expected to be error type") render.Error(w, e) return } else if !equalJSON(t, body, tt.request) { @@ -448,34 +404,27 @@ func TestClient_Revoke(t *testing.T) { }) got, err := c.Revoke(tt.request, nil) - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.Revoke() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + assert.True(t, strings.HasPrefix(err.Error(), tt.expectedErr.Error())) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Revoke() = %v, want nil", got) - } - sassert.HasPrefix(t, err.Error(), tt.expectedErr.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Revoke() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } func TestClient_Renew(t *testing.T) { ok := &api.SignResponse{ - ServerPEM: api.Certificate{Certificate: parseCertificate(certPEM)}, - CaPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + ServerPEM: api.Certificate{Certificate: parseCertificate(t, certPEM)}, + CaPEM: api.Certificate{Certificate: parseCertificate(t, rootPEM)}, CertChainPEM: []api.Certificate{ - {Certificate: parseCertificate(certPEM)}, - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, certPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, } @@ -498,49 +447,38 @@ func TestClient_Renew(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.Renew(nil) - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.Renew() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + var sc render.StatusCodedError + if assert.ErrorAs(t, err, &sc) { + assert.Equal(t, tt.responseCode, sc.StatusCode()) + } + assert.True(t, strings.HasPrefix(err.Error(), tt.err.Error())) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Renew() = %v, want nil", got) - } - - var sc render.StatusCodedError - if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - sassert.Equals(t, sc.StatusCode(), tt.responseCode) - } - sassert.HasPrefix(t, err.Error(), tt.err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Renew() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } func TestClient_RenewWithToken(t *testing.T) { ok := &api.SignResponse{ - ServerPEM: api.Certificate{Certificate: parseCertificate(certPEM)}, - CaPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + ServerPEM: api.Certificate{Certificate: parseCertificate(t, certPEM)}, + CaPEM: api.Certificate{Certificate: parseCertificate(t, rootPEM)}, CertChainPEM: []api.Certificate{ - {Certificate: parseCertificate(certPEM)}, - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, certPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, } @@ -563,10 +501,7 @@ func TestClient_RenewWithToken(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.Header.Get("Authorization") != "Bearer token" { @@ -577,44 +512,36 @@ func TestClient_RenewWithToken(t *testing.T) { }) got, err := c.RenewWithToken("token") - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.RenewWithToken() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + var sc render.StatusCodedError + if assert.ErrorAs(t, err, &sc) { + assert.Equal(t, tt.responseCode, sc.StatusCode()) + } + assert.True(t, strings.HasPrefix(err.Error(), tt.err.Error())) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.RenewWithToken() = %v, want nil", got) - } - - var sc render.StatusCodedError - if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - sassert.Equals(t, sc.StatusCode(), tt.responseCode) - } - sassert.HasPrefix(t, err.Error(), tt.err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.RenewWithToken() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } func TestClient_Rekey(t *testing.T) { ok := &api.SignResponse{ - ServerPEM: api.Certificate{Certificate: parseCertificate(certPEM)}, - CaPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + ServerPEM: api.Certificate{Certificate: parseCertificate(t, certPEM)}, + CaPEM: api.Certificate{Certificate: parseCertificate(t, rootPEM)}, CertChainPEM: []api.Certificate{ - {Certificate: parseCertificate(certPEM)}, - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, certPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, } request := &api.RekeyRequest{ - CsrPEM: api.CertificateRequest{CertificateRequest: parseCertificateRequest(csrPEM)}, + CsrPEM: api.CertificateRequest{CertificateRequest: parseCertificateRequest(t, csrPEM)}, } tests := []struct { @@ -637,38 +564,27 @@ func TestClient_Rekey(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.Rekey(tt.request, nil) - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.Renew() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + var sc render.StatusCodedError + if assert.ErrorAs(t, err, &sc) { + assert.Equal(t, tt.responseCode, sc.StatusCode()) + } + assert.True(t, strings.HasPrefix(err.Error(), tt.err.Error())) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Renew() = %v, want nil", got) - } - - var sc render.StatusCodedError - if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - sassert.Equals(t, sc.StatusCode(), tt.responseCode) - } - sassert.HasPrefix(t, err.Error(), tt.err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Renew() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } @@ -700,10 +616,7 @@ func TestClient_Provisioners(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.RequestURI != tt.expectedURI { @@ -713,22 +626,16 @@ func TestClient_Provisioners(t *testing.T) { }) got, err := c.Provisioners(tt.args...) - if (err != nil) != tt.wantErr { - t.Errorf("Client.Provisioners() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + assert.True(t, strings.HasPrefix(err.Error(), errs.InternalServerErrorDefaultMsg)) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Provisioners() = %v, want nil", got) - } - sassert.HasPrefix(t, errs.InternalServerErrorDefaultMsg, err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Provisioners() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } @@ -756,10 +663,7 @@ func TestClient_ProvisionerKey(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { expected := "/provisioners/" + tt.kid + "/encrypted-key" @@ -770,27 +674,20 @@ func TestClient_ProvisionerKey(t *testing.T) { }) got, err := c.ProvisionerKey(tt.kid) - if (err != nil) != tt.wantErr { - t.Errorf("Client.ProvisionerKey() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + var sc render.StatusCodedError + if assert.ErrorAs(t, err, &sc) { + assert.Equal(t, tt.responseCode, sc.StatusCode()) + } + assert.True(t, strings.HasPrefix(err.Error(), tt.err.Error())) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.ProvisionerKey() = %v, want nil", got) - } - - var sc render.StatusCodedError - if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - sassert.Equals(t, sc.StatusCode(), tt.responseCode) - } - sassert.HasPrefix(t, tt.err.Error(), err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.ProvisionerKey() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } @@ -798,7 +695,7 @@ func TestClient_ProvisionerKey(t *testing.T) { func TestClient_Roots(t *testing.T) { ok := &api.RootsResponse{ Certificates: []api.Certificate{ - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, } @@ -820,37 +717,27 @@ func TestClient_Roots(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.Roots() - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.Roots() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + var sc render.StatusCodedError + if assert.ErrorAs(t, err, &sc) { + assert.Equal(t, tt.responseCode, sc.StatusCode()) + } + assert.True(t, strings.HasPrefix(err.Error(), tt.err.Error())) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Roots() = %v, want nil", got) - } - var sc render.StatusCodedError - if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - sassert.Equals(t, sc.StatusCode(), tt.responseCode) - } - sassert.HasPrefix(t, err.Error(), tt.err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Roots() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } @@ -858,7 +745,7 @@ func TestClient_Roots(t *testing.T) { func TestClient_Federation(t *testing.T) { ok := &api.FederationResponse{ Certificates: []api.Certificate{ - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, } @@ -879,46 +766,34 @@ func TestClient_Federation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.Federation() - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.Federation() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + var sc render.StatusCodedError + if assert.ErrorAs(t, err, &sc) { + assert.Equal(t, tt.responseCode, sc.StatusCode()) + } + assert.True(t, strings.HasPrefix(err.Error(), tt.err.Error())) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.Federation() = %v, want nil", got) - } - var sc render.StatusCodedError - if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - sassert.Equals(t, sc.StatusCode(), tt.responseCode) - } - sassert.HasPrefix(t, tt.err.Error(), err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.Federation() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } func TestClient_SSHRoots(t *testing.T) { - key, err := ssh.NewPublicKey(mustKey().Public()) - if err != nil { - t.Fatal(err) - } + key, err := ssh.NewPublicKey(mustKey(t).Public()) + require.NoError(t, err) ok := &api.SSHRootsResponse{ HostKeys: []api.SSHPublicKey{{PublicKey: key}}, @@ -942,37 +817,27 @@ func TestClient_SSHRoots(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.SSHRoots() - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.SSHKeys() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + if assert.Error(t, err) { + var sc render.StatusCodedError + if assert.ErrorAs(t, err, &sc) { + assert.Equal(t, tt.responseCode, sc.StatusCode()) + } + assert.True(t, strings.HasPrefix(err.Error(), tt.err.Error())) + } + assert.Nil(t, got) return } - switch { - case err != nil: - if got != nil { - t.Errorf("Client.SSHKeys() = %v, want nil", got) - } - var sc render.StatusCodedError - if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - sassert.Equals(t, sc.StatusCode(), tt.responseCode) - } - sassert.HasPrefix(t, tt.err.Error(), err.Error()) - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.SSHKeys() = %v, want %v", got, tt.response) - } - } + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } @@ -1004,13 +869,14 @@ func Test_parseEndpoint(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := parseEndpoint(tt.args.endpoint) - if (err != nil) != tt.wantErr { - t.Errorf("parseEndpoint() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + assert.Error(t, err) + assert.Nil(t, got) return } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("parseEndpoint() = %v, want %v", got, tt.want) - } + + assert.NoError(t, err) + assert.Equal(t, tt.want, got) }) } } @@ -1043,24 +909,21 @@ func TestClient_RootFingerprint(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tr := tt.server.Client().Transport c, err := NewClient(tt.server.URL, WithTransport(tr)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) tt.server.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.RootFingerprint() - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.RootFingerprint() error = %v, wantErr %v", err, tt.wantErr) + if tt.wantErr { + assert.Error(t, err) + assert.Empty(t, got) return } - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("Client.RootFingerprint() = %v, want %v", got, tt.want) - } + + assert.NoError(t, err) + assert.Equal(t, tt.want, got) }) } } @@ -1069,12 +932,12 @@ func TestClient_RootFingerprintWithServer(t *testing.T) { srv := startCABootstrapServer() defer srv.Close() - client, err := NewClient(srv.URL+"/sign", WithRootFile("testdata/secrets/root_ca.crt")) - sassert.FatalError(t, err) + caClient, err := NewClient(srv.URL+"/sign", WithRootFile("testdata/secrets/root_ca.crt")) + require.NoError(t, err) - fp, err := client.RootFingerprint() - sassert.FatalError(t, err) - sassert.Equals(t, "ef742f95dc0d8aa82d3cca4017af6dac3fce84290344159891952d18c53eefe7", fp) + fp, err := caClient.RootFingerprint() + assert.NoError(t, err) + assert.Equal(t, "ef742f95dc0d8aa82d3cca4017af6dac3fce84290344159891952d18c53eefe7", fp) } func TestClient_SSHBastion(t *testing.T) { @@ -1104,39 +967,29 @@ func TestClient_SSHBastion(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(srv.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } + require.NoError(t, err) srv.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { render.JSONStatus(w, tt.response, tt.responseCode) }) got, err := c.SSHBastion(tt.request) - if (err != nil) != tt.wantErr { - fmt.Printf("%+v", err) - t.Errorf("Client.SSHBastion() error = %v, wantErr %v", err, tt.wantErr) - return - } - - switch { - case err != nil: - if got != nil { - t.Errorf("Client.SSHBastion() = %v, want nil", got) - } - if tt.responseCode != 200 { - var sc render.StatusCodedError - if sassert.True(t, errors.As(err, &sc), "error does not implement StatusCodedError interface") { - sassert.Equals(t, sc.StatusCode(), tt.responseCode) + if tt.wantErr { + if assert.Error(t, err) { + if tt.responseCode != 200 { + var sc render.StatusCodedError + if assert.ErrorAs(t, err, &sc) { + assert.Equal(t, tt.responseCode, sc.StatusCode()) + } + assert.True(t, strings.HasPrefix(err.Error(), tt.err.Error())) } - sassert.HasPrefix(t, err.Error(), tt.err.Error()) - } - default: - if !reflect.DeepEqual(got, tt.response) { - t.Errorf("Client.SSHBastion() = %v, want %v", got, tt.response) } + assert.Nil(t, got) + return } + + assert.NoError(t, err) + assert.Equal(t, tt.response, got) }) } } @@ -1155,13 +1008,10 @@ func TestClient_GetCaURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { c, err := NewClient(tt.caURL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Errorf("NewClient() error = %v", err) - return - } - if got := c.GetCaURL(); got != tt.want { - t.Errorf("Client.GetCaURL() = %v, want %v", got, tt.want) - } + require.NoError(t, err) + + got := c.GetCaURL() + assert.Equal(t, tt.want, got) }) } } @@ -1171,7 +1021,7 @@ func Test_enforceRequestID(t *testing.T) { set.Header.Set("X-Request-Id", "already-set") inContext := httptest.NewRequest(http.MethodGet, "https://example.com", http.NoBody) inContext = inContext.WithContext(client.NewRequestIDContext(inContext.Context(), "from-context")) - new := httptest.NewRequest(http.MethodGet, "https://example.com", http.NoBody) + newRequestID := httptest.NewRequest(http.MethodGet, "https://example.com", http.NoBody) tests := []struct { name string @@ -1190,7 +1040,7 @@ func Test_enforceRequestID(t *testing.T) { }, { name: "new", - r: new, + r: newRequestID, }, } for _, tt := range tests { diff --git a/ca/tls_options_test.go b/ca/tls_options_test.go index 7dea3dc8..c29947ad 100644 --- a/ca/tls_options_test.go +++ b/ca/tls_options_test.go @@ -130,7 +130,7 @@ func TestVerifyClientCertIfGiven(t *testing.T) { //nolint:gosec // test tls config func TestAddRootCA(t *testing.T) { - cert := parseCertificate(rootPEM) + cert := parseCertificate(t, rootPEM) pool := x509.NewCertPool() pool.AddCert(cert) @@ -163,7 +163,7 @@ func TestAddRootCA(t *testing.T) { //nolint:gosec // test tls config func TestAddClientCA(t *testing.T) { - cert := parseCertificate(rootPEM) + cert := parseCertificate(t, rootPEM) pool := x509.NewCertPool() pool.AddCert(cert) @@ -214,7 +214,7 @@ func TestAddRootsToRootCAs(t *testing.T) { t.Fatal(err) } - cert := parseCertificate(string(root)) + cert := parseCertificate(t, string(root)) pool := x509.NewCertPool() pool.AddCert(cert) @@ -269,7 +269,7 @@ func TestAddRootsToClientCAs(t *testing.T) { t.Fatal(err) } - cert := parseCertificate(string(root)) + cert := parseCertificate(t, string(root)) pool := x509.NewCertPool() pool.AddCert(cert) @@ -329,8 +329,8 @@ func TestAddFederationToRootCAs(t *testing.T) { t.Fatal(err) } - crt1 := parseCertificate(string(root)) - crt2 := parseCertificate(string(federated)) + crt1 := parseCertificate(t, string(root)) + crt2 := parseCertificate(t, string(federated)) pool := x509.NewCertPool() pool.AddCert(crt1) pool.AddCert(crt2) @@ -394,8 +394,8 @@ func TestAddFederationToClientCAs(t *testing.T) { t.Fatal(err) } - crt1 := parseCertificate(string(root)) - crt2 := parseCertificate(string(federated)) + crt1 := parseCertificate(t, string(root)) + crt2 := parseCertificate(t, string(federated)) pool := x509.NewCertPool() pool.AddCert(crt1) pool.AddCert(crt2) @@ -454,7 +454,7 @@ func TestAddRootsToCAs(t *testing.T) { t.Fatal(err) } - cert := parseCertificate(string(root)) + cert := parseCertificate(t, string(root)) pool := x509.NewCertPool() pool.AddCert(cert) @@ -514,8 +514,8 @@ func TestAddFederationToCAs(t *testing.T) { t.Fatal(err) } - crt1 := parseCertificate(string(root)) - crt2 := parseCertificate(string(federated)) + crt1 := parseCertificate(t, string(root)) + crt2 := parseCertificate(t, string(federated)) pool := x509.NewCertPool() pool.AddCert(crt1) pool.AddCert(crt2) diff --git a/ca/tls_test.go b/ca/tls_test.go index dbcc6023..a19685ce 100644 --- a/ca/tls_test.go +++ b/ca/tls_test.go @@ -401,13 +401,13 @@ func TestClient_GetServerTLSConfig_renew(t *testing.T) { } func TestCertificate(t *testing.T) { - cert := parseCertificate(certPEM) + cert := parseCertificate(t, certPEM) ok := &api.SignResponse{ ServerPEM: api.Certificate{Certificate: cert}, - CaPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + CaPEM: api.Certificate{Certificate: parseCertificate(t, rootPEM)}, CertChainPEM: []api.Certificate{ {Certificate: cert}, - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, } tests := []struct { @@ -434,12 +434,12 @@ func TestCertificate(t *testing.T) { } func TestIntermediateCertificate(t *testing.T) { - intermediate := parseCertificate(rootPEM) + intermediate := parseCertificate(t, rootPEM) ok := &api.SignResponse{ - ServerPEM: api.Certificate{Certificate: parseCertificate(certPEM)}, + ServerPEM: api.Certificate{Certificate: parseCertificate(t, certPEM)}, CaPEM: api.Certificate{Certificate: intermediate}, CertChainPEM: []api.Certificate{ - {Certificate: parseCertificate(certPEM)}, + {Certificate: parseCertificate(t, certPEM)}, {Certificate: intermediate}, }, } @@ -467,24 +467,24 @@ func TestIntermediateCertificate(t *testing.T) { } func TestRootCertificateCertificate(t *testing.T) { - root := parseCertificate(rootPEM) + root := parseCertificate(t, rootPEM) ok := &api.SignResponse{ - ServerPEM: api.Certificate{Certificate: parseCertificate(certPEM)}, - CaPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + ServerPEM: api.Certificate{Certificate: parseCertificate(t, certPEM)}, + CaPEM: api.Certificate{Certificate: parseCertificate(t, rootPEM)}, CertChainPEM: []api.Certificate{ - {Certificate: parseCertificate(certPEM)}, - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, certPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, TLS: &tls.ConnectionState{VerifiedChains: [][]*x509.Certificate{ {root, root}, }}, } noTLS := &api.SignResponse{ - ServerPEM: api.Certificate{Certificate: parseCertificate(certPEM)}, - CaPEM: api.Certificate{Certificate: parseCertificate(rootPEM)}, + ServerPEM: api.Certificate{Certificate: parseCertificate(t, certPEM)}, + CaPEM: api.Certificate{Certificate: parseCertificate(t, rootPEM)}, CertChainPEM: []api.Certificate{ - {Certificate: parseCertificate(certPEM)}, - {Certificate: parseCertificate(rootPEM)}, + {Certificate: parseCertificate(t, certPEM)}, + {Certificate: parseCertificate(t, rootPEM)}, }, } tests := []struct { From 0898c6db972b2c823eb873b719d31eca96cff613 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 29 Feb 2024 20:26:27 +0100 Subject: [PATCH 13/16] Use UUIDv4 as automatically generated client request identifier --- ca/client.go | 15 +++++++++++++-- ca/client_test.go | 10 ++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/ca/client.go b/ca/client.go index 9e245cd7..b18efbaf 100644 --- a/ca/client.go +++ b/ca/client.go @@ -24,7 +24,6 @@ import ( "strings" "github.com/pkg/errors" - "github.com/rs/xid" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority" "github.com/smallstep/certificates/authority/provisioner" @@ -35,6 +34,7 @@ import ( "go.step.sm/crypto/jose" "go.step.sm/crypto/keyutil" "go.step.sm/crypto/pemutil" + "go.step.sm/crypto/randutil" "go.step.sm/crypto/x509util" "golang.org/x/net/http2" "google.golang.org/protobuf/encoding/protojson" @@ -105,6 +105,17 @@ func (c *uaClient) PostWithContext(ctx context.Context, u, contentType string, b // the CA client to the CA and back again. const requestIDHeader = "X-Request-Id" +// newRequestID generates a new random UUIDv4 request ID. If it fails, +// the request ID will be the empty string. +func newRequestID() string { + requestID, err := randutil.UUIDv4() + if err != nil { + return "" + } + + return requestID +} + // enforceRequestID checks if the X-Request-Id HTTP header is filled. If it's // empty, the context is searched for a request ID. If that's also empty, a new // request ID is generated. @@ -115,7 +126,7 @@ func enforceRequestID(r *http.Request) { // used before by the client (unless it's a retry for the same request)? requestID = reqID } else { - requestID = xid.New().String() + requestID = newRequestID() } r.Header.Set(requestIDHeader, requestID) } diff --git a/ca/client_test.go b/ca/client_test.go index 5fd11179..44d24c6e 100644 --- a/ca/client_test.go +++ b/ca/client_test.go @@ -17,6 +17,7 @@ import ( "testing" "time" + "github.com/google/uuid" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/api/read" "github.com/smallstep/certificates/api/render" @@ -1056,3 +1057,12 @@ func Test_enforceRequestID(t *testing.T) { }) } } + +func Test_newRequestID(t *testing.T) { + requestID := newRequestID() + u, err := uuid.Parse(requestID) + assert.NoError(t, err) + assert.Equal(t, uuid.Version(0x4), u.Version()) + assert.Equal(t, uuid.RFC4122, u.Variant()) + assert.Equal(t, requestID, u.String()) +} From 7fd524f70b82021df5e7d58f2a6d5fc483ecafe1 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 1 Mar 2024 01:04:50 +0100 Subject: [PATCH 14/16] Default to generating request IDs using UUIDv4 format in CA --- internal/requestid/requestid.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/requestid/requestid.go b/internal/requestid/requestid.go index 97f58f8c..7008d469 100644 --- a/internal/requestid/requestid.go +++ b/internal/requestid/requestid.go @@ -5,6 +5,7 @@ import ( "net/http" "github.com/rs/xid" + "go.step.sm/crypto/randutil" ) const ( @@ -61,9 +62,16 @@ func (h *Handler) Middleware(next http.Handler) http.Handler { return http.HandlerFunc(fn) } -// newRequestID creates a new request ID using github.com/rs/xid. +// newRequestID generates a new random UUIDv4 request ID. If UUIDv4 +// generation fails, it'll fallback to generating a random ID using +// github.com/rs/xid. func newRequestID() string { - return xid.New().String() + requestID, err := randutil.UUIDv4() + if err != nil { + requestID = xid.New().String() + } + + return requestID } type requestIDKey struct{} From d392c169fce826a32ab562102f3e1ccba1fb8abc Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 4 Mar 2024 12:00:08 +0100 Subject: [PATCH 15/16] Improve functional coverage of request ID integration test --- authority/provisioner/webhook_test.go | 33 +-- ca/client/requestid.go | 6 +- ca/provisioner_test.go | 8 +- ca/tls_options_test.go | 98 +++------ ca/tls_test.go | 103 ++++----- internal/requestid/requestid.go | 7 +- internal/requestid/requestid_test.go | 4 + internal/userid/userid.go | 6 +- logging/handler.go | 1 + monitoring/monitoring.go | 1 + test/e2e/requestid_test.go | 132 ------------ test/integration/requestid_test.go | 289 ++++++++++++++++++++++++++ 12 files changed, 402 insertions(+), 286 deletions(-) delete mode 100644 test/e2e/requestid_test.go create mode 100644 test/integration/requestid_test.go diff --git a/authority/provisioner/webhook_test.go b/authority/provisioner/webhook_test.go index 4c80796f..90583418 100644 --- a/authority/provisioner/webhook_test.go +++ b/authority/provisioner/webhook_test.go @@ -17,13 +17,15 @@ import ( "testing" "time" - "github.com/smallstep/certificates/internal/requestid" - "github.com/smallstep/certificates/webhook" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.step.sm/crypto/pemutil" "go.step.sm/crypto/x509util" "go.step.sm/linkedca" + + "github.com/smallstep/certificates/internal/requestid" + "github.com/smallstep/certificates/webhook" ) func TestWebhookController_isCertTypeOK(t *testing.T) { @@ -103,7 +105,8 @@ func TestWebhookController_isCertTypeOK(t *testing.T) { // withRequestID is a helper that calls into [requestid.NewContext] and returns // a new context with the requestID added. -func withRequestID(ctx context.Context, requestID string) context.Context { +func withRequestID(t *testing.T, ctx context.Context, requestID string) context.Context { + t.Helper() return requestid.NewContext(ctx, requestID) } @@ -138,7 +141,7 @@ func TestWebhookController_Enrich(t *testing.T) { webhooks: []*Webhook{{Name: "people", Kind: "ENRICHING"}}, TemplateData: x509util.TemplateData{}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: true, Data: map[string]any{"role": "bar"}}}, expectErr: false, @@ -153,7 +156,7 @@ func TestWebhookController_Enrich(t *testing.T) { }, TemplateData: x509util.TemplateData{}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{ {Allow: true, Data: map[string]any{"role": "bar"}}, @@ -177,7 +180,7 @@ func TestWebhookController_Enrich(t *testing.T) { TemplateData: x509util.TemplateData{}, certType: linkedca.Webhook_X509, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{ {Allow: true, Data: map[string]any{"role": "bar"}}, @@ -197,7 +200,7 @@ func TestWebhookController_Enrich(t *testing.T) { TemplateData: x509util.TemplateData{}, options: []webhook.RequestBodyOption{webhook.WithX5CCertificate(cert)}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: true, Data: map[string]any{"role": "bar"}}}, expectErr: false, @@ -220,7 +223,7 @@ func TestWebhookController_Enrich(t *testing.T) { webhooks: []*Webhook{{Name: "people", Kind: "ENRICHING"}}, TemplateData: x509util.TemplateData{}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: false}}, expectErr: true, @@ -235,7 +238,7 @@ func TestWebhookController_Enrich(t *testing.T) { PublicKey: []byte("bad"), })}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: false}}, expectErr: true, @@ -296,7 +299,7 @@ func TestWebhookController_Authorize(t *testing.T) { client: http.DefaultClient, webhooks: []*Webhook{{Name: "people", Kind: "AUTHORIZING"}}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: true}}, expectErr: false, @@ -307,7 +310,7 @@ func TestWebhookController_Authorize(t *testing.T) { webhooks: []*Webhook{{Name: "people", Kind: "AUTHORIZING", CertType: linkedca.Webhook_X509.String()}}, certType: linkedca.Webhook_SSH, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: false}}, expectErr: false, @@ -318,7 +321,7 @@ func TestWebhookController_Authorize(t *testing.T) { webhooks: []*Webhook{{Name: "people", Kind: "AUTHORIZING"}}, options: []webhook.RequestBodyOption{webhook.WithX5CCertificate(cert)}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: true}}, expectErr: false, @@ -339,7 +342,7 @@ func TestWebhookController_Authorize(t *testing.T) { client: http.DefaultClient, webhooks: []*Webhook{{Name: "people", Kind: "AUTHORIZING"}}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: false}}, expectErr: true, @@ -352,7 +355,7 @@ func TestWebhookController_Authorize(t *testing.T) { PublicKey: []byte("bad"), })}, }, - ctx: withRequestID(context.Background(), "reqID"), + ctx: withRequestID(t, context.Background(), "reqID"), req: &webhook.RequestBody{}, responses: []*webhook.ResponseBody{{Allow: false}}, expectErr: true, @@ -568,7 +571,7 @@ func TestWebhook_Do(t *testing.T) { ctx := context.Background() if tc.requestID != "" { - ctx = withRequestID(context.Background(), tc.requestID) + ctx = withRequestID(t, ctx, tc.requestID) } ctx, cancel := context.WithTimeout(ctx, time.Second*10) defer cancel() diff --git a/ca/client/requestid.go b/ca/client/requestid.go index 2bebb7e5..1fb785eb 100644 --- a/ca/client/requestid.go +++ b/ca/client/requestid.go @@ -2,17 +2,17 @@ package client import "context" -type requestIDKey struct{} +type contextKey struct{} // NewRequestIDContext returns a new context with the given request ID added to the // context. func NewRequestIDContext(ctx context.Context, requestID string) context.Context { - return context.WithValue(ctx, requestIDKey{}, requestID) + return context.WithValue(ctx, contextKey{}, requestID) } // RequestIDFromContext returns the request ID from the context if it exists. // and is not empty. func RequestIDFromContext(ctx context.Context) (string, bool) { - v, ok := ctx.Value(requestIDKey{}).(string) + v, ok := ctx.Value(contextKey{}).(string) return v, ok && v != "" } diff --git a/ca/provisioner_test.go b/ca/provisioner_test.go index 39193f3f..5a754f08 100644 --- a/ca/provisioner_test.go +++ b/ca/provisioner_test.go @@ -7,6 +7,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "go.step.sm/crypto/jose" "go.step.sm/crypto/pemutil" "go.step.sm/crypto/x509util" @@ -41,14 +43,12 @@ func getTestProvisioner(t *testing.T, caURL string) *Provisioner { } func TestNewProvisioner(t *testing.T) { - ca := startCATestServer() + ca := startCATestServer(t) defer ca.Close() want := getTestProvisioner(t, ca.URL) caBundle, err := os.ReadFile("testdata/secrets/root_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) type args struct { name string diff --git a/ca/tls_options_test.go b/ca/tls_options_test.go index c29947ad..4ac6ff85 100644 --- a/ca/tls_options_test.go +++ b/ca/tls_options_test.go @@ -10,6 +10,8 @@ import ( "sort" "testing" + "github.com/stretchr/testify/require" + "github.com/smallstep/certificates/api" ) @@ -196,23 +198,17 @@ func TestAddClientCA(t *testing.T) { //nolint:gosec // test tls config func TestAddRootsToRootCAs(t *testing.T) { - ca := startCATestServer() + ca := startCATestServer(t) defer ca.Close() client, err := NewClient(ca.URL, WithRootFile("testdata/secrets/root_ca.crt")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) clientFail, err := NewClient(ca.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) root, err := os.ReadFile("testdata/secrets/root_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) cert := parseCertificate(t, string(root)) pool := x509.NewCertPool() @@ -251,23 +247,17 @@ func TestAddRootsToRootCAs(t *testing.T) { //nolint:gosec // test tls config func TestAddRootsToClientCAs(t *testing.T) { - ca := startCATestServer() + ca := startCATestServer(t) defer ca.Close() client, err := NewClient(ca.URL, WithRootFile("testdata/secrets/root_ca.crt")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) clientFail, err := NewClient(ca.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) root, err := os.ReadFile("testdata/secrets/root_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) cert := parseCertificate(t, string(root)) pool := x509.NewCertPool() @@ -306,28 +296,20 @@ func TestAddRootsToClientCAs(t *testing.T) { //nolint:gosec // test tls config func TestAddFederationToRootCAs(t *testing.T) { - ca := startCATestServer() + ca := startCATestServer(t) defer ca.Close() client, err := NewClient(ca.URL, WithRootFile("testdata/secrets/root_ca.crt")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) clientFail, err := NewClient(ca.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) root, err := os.ReadFile("testdata/secrets/root_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) federated, err := os.ReadFile("testdata/secrets/federated_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) crt1 := parseCertificate(t, string(root)) crt2 := parseCertificate(t, string(federated)) @@ -371,28 +353,20 @@ func TestAddFederationToRootCAs(t *testing.T) { //nolint:gosec // test tls config func TestAddFederationToClientCAs(t *testing.T) { - ca := startCATestServer() + ca := startCATestServer(t) defer ca.Close() client, err := NewClient(ca.URL, WithRootFile("testdata/secrets/root_ca.crt")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) clientFail, err := NewClient(ca.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) root, err := os.ReadFile("testdata/secrets/root_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) federated, err := os.ReadFile("testdata/secrets/federated_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) crt1 := parseCertificate(t, string(root)) crt2 := parseCertificate(t, string(federated)) @@ -436,23 +410,17 @@ func TestAddFederationToClientCAs(t *testing.T) { //nolint:gosec // test tls config func TestAddRootsToCAs(t *testing.T) { - ca := startCATestServer() + ca := startCATestServer(t) defer ca.Close() client, err := NewClient(ca.URL, WithRootFile("testdata/secrets/root_ca.crt")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) clientFail, err := NewClient(ca.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) root, err := os.ReadFile("testdata/secrets/root_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) cert := parseCertificate(t, string(root)) pool := x509.NewCertPool() @@ -491,28 +459,20 @@ func TestAddRootsToCAs(t *testing.T) { //nolint:gosec // test tls config func TestAddFederationToCAs(t *testing.T) { - ca := startCATestServer() + ca := startCATestServer(t) defer ca.Close() client, err := NewClient(ca.URL, WithRootFile("testdata/secrets/root_ca.crt")) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) clientFail, err := NewClient(ca.URL, WithTransport(http.DefaultTransport)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) root, err := os.ReadFile("testdata/secrets/root_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) federated, err := os.ReadFile("testdata/secrets/federated_ca.crt") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) crt1 := parseCertificate(t, string(root)) crt2 := parseCertificate(t, string(federated)) diff --git a/ca/tls_test.go b/ca/tls_test.go index a19685ce..d1ce11ea 100644 --- a/ca/tls_test.go +++ b/ca/tls_test.go @@ -17,27 +17,28 @@ import ( "testing" "time" - "github.com/smallstep/certificates/api" - "github.com/smallstep/certificates/authority" + "github.com/stretchr/testify/require" + "go.step.sm/crypto/jose" "go.step.sm/crypto/randutil" + + "github.com/smallstep/certificates/api" + "github.com/smallstep/certificates/authority" ) -func generateOTT(subject string) string { +func generateOTT(t *testing.T, subject string) string { + t.Helper() now := time.Now() jwk, err := jose.ReadKey("testdata/secrets/ott_mariano_priv.jwk", jose.WithPassword([]byte("password"))) - if err != nil { - panic(err) - } + require.NoError(t, err) + opts := new(jose.SignerOptions).WithType("JWT").WithHeader("kid", jwk.KeyID) sig, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.ES256, Key: jwk.Key}, opts) - if err != nil { - panic(err) - } + require.NoError(t, err) + id, err := randutil.ASCII(64) - if err != nil { - panic(err) - } + require.NoError(t, err) + cl := struct { jose.Claims SANS []string `json:"sans"` @@ -53,9 +54,8 @@ func generateOTT(subject string) string { SANS: []string{subject}, } raw, err := jose.Signed(sig).Claims(cl).CompactSerialize() - if err != nil { - panic(err) - } + require.NoError(t, err) + return raw } @@ -72,32 +72,28 @@ func startTestServer(baseContext context.Context, tlsConfig *tls.Config, handler return srv } -func startCATestServer() *httptest.Server { +func startCATestServer(t *testing.T) *httptest.Server { config, err := authority.LoadConfiguration("testdata/ca.json") - if err != nil { - panic(err) - } + require.NoError(t, err) ca, err := New(config) - if err != nil { - panic(err) - } + require.NoError(t, err) // Use a httptest.Server instead baseContext := buildContext(ca.auth, nil, nil, nil) srv := startTestServer(baseContext, ca.srv.TLSConfig, ca.srv.Handler) return srv } -func sign(domain string) (*Client, *api.SignResponse, crypto.PrivateKey) { - srv := startCATestServer() +func sign(t *testing.T, domain string) (*Client, *api.SignResponse, crypto.PrivateKey) { + t.Helper() + srv := startCATestServer(t) defer srv.Close() - return signDuration(srv, domain, 0) + return signDuration(t, srv, domain, 0) } -func signDuration(srv *httptest.Server, domain string, duration time.Duration) (*Client, *api.SignResponse, crypto.PrivateKey) { - req, pk, err := CreateSignRequest(generateOTT(domain)) - if err != nil { - panic(err) - } +func signDuration(t *testing.T, srv *httptest.Server, domain string, duration time.Duration) (*Client, *api.SignResponse, crypto.PrivateKey) { + t.Helper() + req, pk, err := CreateSignRequest(generateOTT(t, domain)) + require.NoError(t, err) if duration > 0 { req.NotBefore = api.NewTimeDuration(time.Now()) @@ -105,13 +101,11 @@ func signDuration(srv *httptest.Server, domain string, duration time.Duration) ( } client, err := NewClient(srv.URL, WithRootFile("testdata/secrets/root_ca.crt")) - if err != nil { - panic(err) - } + require.NoError(t, err) + sr, err := client.Sign(req) - if err != nil { - panic(err) - } + require.NoError(t, err) + return client, sr, pk } @@ -145,7 +139,7 @@ func serverHandler(t *testing.T, clientDomain string) http.Handler { func TestClient_GetServerTLSConfig_http(t *testing.T) { clientDomain := "test.domain" - client, sr, pk := sign("127.0.0.1") + client, sr, pk := sign(t, "127.0.0.1") // Create mTLS server ctx, cancel := context.WithCancel(context.Background()) @@ -212,7 +206,7 @@ func TestClient_GetServerTLSConfig_http(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - client, sr, pk := sign(clientDomain) + client, sr, pk := sign(t, clientDomain) cli := tt.getClient(t, client, sr, pk) if cli == nil { return @@ -246,19 +240,18 @@ func TestClient_GetServerTLSConfig_renew(t *testing.T) { defer reset() // Start CA - ca := startCATestServer() + ca := startCATestServer(t) defer ca.Close() clientDomain := "test.domain" - client, sr, pk := signDuration(ca, "127.0.0.1", 5*time.Second) + client, sr, pk := signDuration(t, ca, "127.0.0.1", 5*time.Second) // Start mTLS server ctx, cancel := context.WithCancel(context.Background()) defer cancel() tlsConfig, err := client.GetServerTLSConfig(ctx, sr, pk) - if err != nil { - t.Fatalf("Client.GetServerTLSConfig() error = %v", err) - } + require.NoError(t, err) + srvMTLS := startTestServer(context.Background(), tlsConfig, serverHandler(t, clientDomain)) defer srvMTLS.Close() @@ -266,30 +259,26 @@ func TestClient_GetServerTLSConfig_renew(t *testing.T) { ctx, cancel = context.WithCancel(context.Background()) defer cancel() tlsConfig, err = client.GetServerTLSConfig(ctx, sr, pk, VerifyClientCertIfGiven()) - if err != nil { - t.Fatalf("Client.GetServerTLSConfig() error = %v", err) - } + require.NoError(t, err) + srvTLS := startTestServer(context.Background(), tlsConfig, serverHandler(t, clientDomain)) defer srvTLS.Close() // Transport - client, sr, pk = signDuration(ca, clientDomain, 5*time.Second) + client, sr, pk = signDuration(t, ca, clientDomain, 5*time.Second) tr1, err := client.Transport(context.Background(), sr, pk) - if err != nil { - t.Fatalf("Client.Transport() error = %v", err) - } + require.NoError(t, err) + // Transport with tlsConfig - client, sr, pk = signDuration(ca, clientDomain, 5*time.Second) + client, sr, pk = signDuration(t, ca, clientDomain, 5*time.Second) tlsConfig, err = client.GetClientTLSConfig(context.Background(), sr, pk) - if err != nil { - t.Fatalf("Client.GetClientTLSConfig() error = %v", err) - } + require.NoError(t, err) + tr2 := getDefaultTransport(tlsConfig) // No client cert root, err := RootCertificate(sr) - if err != nil { - t.Fatalf("RootCertificate() error = %v", err) - } + require.NoError(t, err) + tlsConfig = getDefaultTLSConfig(sr) tlsConfig.RootCAs = x509.NewCertPool() tlsConfig.RootCAs.AddCert(root) diff --git a/internal/requestid/requestid.go b/internal/requestid/requestid.go index 7008d469..ace08f16 100644 --- a/internal/requestid/requestid.go +++ b/internal/requestid/requestid.go @@ -5,6 +5,7 @@ import ( "net/http" "github.com/rs/xid" + "go.step.sm/crypto/randutil" ) @@ -74,17 +75,17 @@ func newRequestID() string { return requestID } -type requestIDKey struct{} +type contextKey struct{} // NewContext returns a new context with the given request ID added to the // context. func NewContext(ctx context.Context, requestID string) context.Context { - return context.WithValue(ctx, requestIDKey{}, requestID) + return context.WithValue(ctx, contextKey{}, requestID) } // FromContext returns the request ID from the context if it exists and // is not the empty value. func FromContext(ctx context.Context) (string, bool) { - v, ok := ctx.Value(requestIDKey{}).(string) + v, ok := ctx.Value(contextKey{}).(string) return v, ok && v != "" } diff --git a/internal/requestid/requestid_test.go b/internal/requestid/requestid_test.go index 4d0e872d..84a9021f 100644 --- a/internal/requestid/requestid_test.go +++ b/internal/requestid/requestid_test.go @@ -19,11 +19,15 @@ func newRequest(t *testing.T) *http.Request { func Test_Middleware(t *testing.T) { requestWithID := newRequest(t) requestWithID.Header.Set("X-Request-Id", "reqID") + requestWithoutID := newRequest(t) + requestWithEmptyHeader := newRequest(t) requestWithEmptyHeader.Header.Set("X-Request-Id", "") + requestWithSmallstepID := newRequest(t) requestWithSmallstepID.Header.Set("X-Smallstep-Id", "smallstepID") + tests := []struct { name string traceHeader string diff --git a/internal/userid/userid.go b/internal/userid/userid.go index bab4908f..48087da8 100644 --- a/internal/userid/userid.go +++ b/internal/userid/userid.go @@ -2,19 +2,19 @@ package userid import "context" -type userIDKey struct{} +type contextKey struct{} // NewContext returns a new context with the given user ID added to the // context. // TODO(hs): this doesn't seem to be used / set currently; implement // when/where it makes sense. func NewContext(ctx context.Context, userID string) context.Context { - return context.WithValue(ctx, userIDKey{}, userID) + return context.WithValue(ctx, contextKey{}, userID) } // FromContext returns the user ID from the context if it exists // and is not empty. func FromContext(ctx context.Context) (string, bool) { - v, ok := ctx.Value(userIDKey{}).(string) + v, ok := ctx.Value(contextKey{}).(string) return v, ok && v != "" } diff --git a/logging/handler.go b/logging/handler.go index a29383b2..06fc56d3 100644 --- a/logging/handler.go +++ b/logging/handler.go @@ -9,6 +9,7 @@ import ( "time" "github.com/sirupsen/logrus" + "github.com/smallstep/certificates/internal/requestid" "github.com/smallstep/certificates/internal/userid" ) diff --git a/monitoring/monitoring.go b/monitoring/monitoring.go index 7c88ab3b..2ca2ef54 100644 --- a/monitoring/monitoring.go +++ b/monitoring/monitoring.go @@ -9,6 +9,7 @@ import ( "github.com/newrelic/go-agent/v3/newrelic" "github.com/pkg/errors" + "github.com/smallstep/certificates/internal/requestid" "github.com/smallstep/certificates/logging" ) diff --git a/test/e2e/requestid_test.go b/test/e2e/requestid_test.go deleted file mode 100644 index d2f968c3..00000000 --- a/test/e2e/requestid_test.go +++ /dev/null @@ -1,132 +0,0 @@ -package e2e - -import ( - "context" - "encoding/json" - "fmt" - "net" - "path/filepath" - "sync" - "testing" - - "github.com/smallstep/certificates/authority/config" - "github.com/smallstep/certificates/ca" - "github.com/smallstep/certificates/ca/client" - "github.com/smallstep/certificates/errs" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "go.step.sm/crypto/minica" - "go.step.sm/crypto/pemutil" -) - -// reservePort "reserves" a TCP port by opening a listener on a random -// port and immediately closing it. The port can then be assumed to be -// available for running a server on. -func reservePort(t *testing.T) (host, port string) { - t.Helper() - l, err := net.Listen("tcp", ":0") - require.NoError(t, err) - - address := l.Addr().String() - err = l.Close() - require.NoError(t, err) - - host, port, err = net.SplitHostPort(address) - require.NoError(t, err) - - return -} - -func Test_reflectRequestID(t *testing.T) { - dir := t.TempDir() - m, err := minica.New(minica.WithName("Step E2E")) - require.NoError(t, err) - - rootFilepath := filepath.Join(dir, "root.crt") - _, err = pemutil.Serialize(m.Root, pemutil.WithFilename(rootFilepath)) - require.NoError(t, err) - - intermediateCertFilepath := filepath.Join(dir, "intermediate.crt") - _, err = pemutil.Serialize(m.Intermediate, pemutil.WithFilename(intermediateCertFilepath)) - require.NoError(t, err) - - intermediateKeyFilepath := filepath.Join(dir, "intermediate.key") - _, err = pemutil.Serialize(m.Signer, pemutil.WithFilename(intermediateKeyFilepath)) - require.NoError(t, err) - - // get a random address to listen on and connect to; currently no nicer way to get one before starting the server - // TODO(hs): find/implement a nicer way to expose the CA URL, similar to how e.g. httptest.Server exposes it? - host, port := reservePort(t) - - cfg := &config.Config{ - Root: []string{rootFilepath}, - IntermediateCert: intermediateCertFilepath, - IntermediateKey: intermediateKeyFilepath, - Address: net.JoinHostPort(host, port), // reuse the address that was just "reserved" - DNSNames: []string{"127.0.0.1", "[::1]", "localhost"}, - AuthorityConfig: &config.AuthConfig{ - AuthorityID: "stepca-test", - DeploymentType: "standalone-test", - }, - Logger: json.RawMessage(`{"format": "text"}`), - } - c, err := ca.New(cfg) - require.NoError(t, err) - - // instantiate a client for the CA running at the random address - caClient, err := ca.NewClient( - fmt.Sprintf("https://localhost:%s", port), - ca.WithRootFile(rootFilepath), - ) - require.NoError(t, err) - - var wg sync.WaitGroup - wg.Add(1) - - go func() { - defer wg.Done() - err = c.Run() - require.Error(t, err) // expect error when server is stopped - }() - - // require OK health response as the baseline - ctx := context.Background() - healthResponse, err := caClient.HealthWithContext(ctx) - require.NoError(t, err) - if assert.NotNil(t, healthResponse) { - require.Equal(t, "ok", healthResponse.Status) - } - - // expect an error when retrieving an invalid root - rootResponse, err := caClient.RootWithContext(ctx, "invalid") - if assert.Error(t, err) { - apiErr := &errs.Error{} - if assert.ErrorAs(t, err, &apiErr) { - assert.Equal(t, 404, apiErr.StatusCode()) - assert.Equal(t, "The requested resource could not be found. Please see the certificate authority logs for more info.", apiErr.Err.Error()) - assert.NotEmpty(t, apiErr.RequestID) - - // TODO: include the below error in the JSON? It's currently only output to the CA logs. Also see https://github.com/smallstep/certificates/pull/759 - //assert.Equal(t, "/root/invalid was not found: certificate with fingerprint invalid was not found", apiErr.Msg) - } - } - assert.Nil(t, rootResponse) - - // expect an error when retrieving an invalid root and provided request ID - rootResponse, err = caClient.RootWithContext(client.NewRequestIDContext(ctx, "reqID"), "invalid") - if assert.Error(t, err) { - apiErr := &errs.Error{} - if assert.ErrorAs(t, err, &apiErr) { - assert.Equal(t, 404, apiErr.StatusCode()) - assert.Equal(t, "The requested resource could not be found. Please see the certificate authority logs for more info.", apiErr.Err.Error()) - assert.Equal(t, "reqID", apiErr.RequestID) - } - } - assert.Nil(t, rootResponse) - - // done testing; stop and wait for the server to quit - err = c.Stop() - require.NoError(t, err) - - wg.Wait() -} diff --git a/test/integration/requestid_test.go b/test/integration/requestid_test.go new file mode 100644 index 00000000..f15db12f --- /dev/null +++ b/test/integration/requestid_test.go @@ -0,0 +1,289 @@ +package integration + +import ( + "context" + "crypto/tls" + "crypto/x509" + "encoding/json" + "fmt" + "net" + "net/http" + "net/http/httptest" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.step.sm/crypto/jose" + "go.step.sm/crypto/keyutil" + "go.step.sm/crypto/minica" + "go.step.sm/crypto/pemutil" + "go.step.sm/crypto/randutil" + "go.step.sm/crypto/x509util" + + "github.com/smallstep/certificates/api" + "github.com/smallstep/certificates/authority/config" + "github.com/smallstep/certificates/authority/provisioner" + "github.com/smallstep/certificates/ca" + "github.com/smallstep/certificates/ca/client" + "github.com/smallstep/certificates/errs" +) + +// reservePort "reserves" a TCP port by opening a listener on a random +// port and immediately closing it. The port can then be assumed to be +// available for running a server on. +func reservePort(t *testing.T) (host, port string) { + t.Helper() + l, err := net.Listen("tcp", ":0") + require.NoError(t, err) + + address := l.Addr().String() + err = l.Close() + require.NoError(t, err) + + host, port, err = net.SplitHostPort(address) + require.NoError(t, err) + + return +} + +func Test_reflectRequestID(t *testing.T) { + dir := t.TempDir() + m, err := minica.New(minica.WithName("Step E2E")) + require.NoError(t, err) + + rootFilepath := filepath.Join(dir, "root.crt") + _, err = pemutil.Serialize(m.Root, pemutil.WithFilename(rootFilepath)) + require.NoError(t, err) + + intermediateCertFilepath := filepath.Join(dir, "intermediate.crt") + _, err = pemutil.Serialize(m.Intermediate, pemutil.WithFilename(intermediateCertFilepath)) + require.NoError(t, err) + + intermediateKeyFilepath := filepath.Join(dir, "intermediate.key") + _, err = pemutil.Serialize(m.Signer, pemutil.WithFilename(intermediateKeyFilepath)) + require.NoError(t, err) + + // get a random address to listen on and connect to; currently no nicer way to get one before starting the server + // TODO(hs): find/implement a nicer way to expose the CA URL, similar to how e.g. httptest.Server exposes it? + host, port := reservePort(t) + + authorizingSrv := newAuthorizingServer(t, m) + defer authorizingSrv.Close() + authorizingSrv.StartTLS() + + password := []byte("1234") + jwk, jwe, err := jose.GenerateDefaultKeyPair(password) + require.NoError(t, err) + encryptedKey, err := jwe.CompactSerialize() + require.NoError(t, err) + prov := &provisioner.JWK{ + ID: "jwk", + Name: "jwk", + Type: "JWK", + Key: jwk, + EncryptedKey: encryptedKey, + Claims: &config.GlobalProvisionerClaims, + Options: &provisioner.Options{ + Webhooks: []*provisioner.Webhook{ + { + ID: "webhook", + Name: "webhook-test", + URL: fmt.Sprintf("%s/authorize", authorizingSrv.URL), + Kind: "AUTHORIZING", + CertType: "X509", + }, + }, + }, + } + err = prov.Init(provisioner.Config{}) + require.NoError(t, err) + + cfg := &config.Config{ + Root: []string{rootFilepath}, + IntermediateCert: intermediateCertFilepath, + IntermediateKey: intermediateKeyFilepath, + Address: net.JoinHostPort(host, port), // reuse the address that was just "reserved" + DNSNames: []string{"127.0.0.1", "[::1]", "localhost"}, + AuthorityConfig: &config.AuthConfig{ + AuthorityID: "stepca-test", + DeploymentType: "standalone-test", + Provisioners: provisioner.List{prov}, + }, + Logger: json.RawMessage(`{"format": "text"}`), + } + c, err := ca.New(cfg) + require.NoError(t, err) + + // instantiate a client for the CA running at the random address + caClient, err := ca.NewClient( + fmt.Sprintf("https://localhost:%s", port), + ca.WithRootFile(rootFilepath), + ) + require.NoError(t, err) + + var wg sync.WaitGroup + wg.Add(1) + + go func() { + defer wg.Done() + err = c.Run() + require.ErrorIs(t, err, http.ErrServerClosed) + }() + + // require OK health response as the baseline + ctx := context.Background() + healthResponse, err := caClient.HealthWithContext(ctx) + require.NoError(t, err) + if assert.NotNil(t, healthResponse) { + require.Equal(t, "ok", healthResponse.Status) + } + + // expect an error when retrieving an invalid root + rootResponse, err := caClient.RootWithContext(ctx, "invalid") + var firstErr *errs.Error + if assert.ErrorAs(t, err, &firstErr) { + assert.Equal(t, 404, firstErr.StatusCode()) + assert.Equal(t, "The requested resource could not be found. Please see the certificate authority logs for more info.", firstErr.Err.Error()) + assert.NotEmpty(t, firstErr.RequestID) + + // TODO: include the below error in the JSON? It's currently only output to the CA logs. Also see https://github.com/smallstep/certificates/pull/759 + //assert.Equal(t, "/root/invalid was not found: certificate with fingerprint invalid was not found", apiErr.Msg) + } + assert.Nil(t, rootResponse) + + // expect an error when retrieving an invalid root and provided request ID + rootResponse, err = caClient.RootWithContext(client.NewRequestIDContext(ctx, "reqID"), "invalid") + var secondErr *errs.Error + if assert.ErrorAs(t, err, &secondErr) { + assert.Equal(t, 404, secondErr.StatusCode()) + assert.Equal(t, "The requested resource could not be found. Please see the certificate authority logs for more info.", secondErr.Err.Error()) + assert.Equal(t, "reqID", secondErr.RequestID) + } + assert.Nil(t, rootResponse) + + // prepare a Sign request + subject := "test" + decryptedJWK := decryptPrivateKey(t, jwe, password) + ott := generateOTT(t, decryptedJWK, subject) + + signer, err := keyutil.GenerateDefaultSigner() + require.NoError(t, err) + + csr, err := x509util.CreateCertificateRequest(subject, []string{subject}, signer) + require.NoError(t, err) + + // perform the Sign request using the OTT and CSR + signResponse, err := caClient.SignWithContext(client.NewRequestIDContext(ctx, "signRequestID"), &api.SignRequest{ + CsrPEM: api.CertificateRequest{CertificateRequest: csr}, + OTT: ott, + NotAfter: api.NewTimeDuration(time.Now().Add(1 * time.Hour)), + NotBefore: api.NewTimeDuration(time.Now().Add(-1 * time.Hour)), + }) + assert.NoError(t, err) + + // assert a certificate was returned for the subject "test" + if assert.NotNil(t, signResponse) { + assert.Len(t, signResponse.CertChainPEM, 2) + cert, err := x509.ParseCertificate(signResponse.CertChainPEM[0].Raw) + assert.NoError(t, err) + if assert.NotNil(t, cert) { + assert.Equal(t, "test", cert.Subject.CommonName) + assert.Contains(t, cert.DNSNames, "test") + } + } + + // done testing; stop and wait for the server to quit + err = c.Stop() + require.NoError(t, err) + + wg.Wait() +} + +func decryptPrivateKey(t *testing.T, jwe *jose.JSONWebEncryption, pass []byte) *jose.JSONWebKey { + t.Helper() + d, err := jwe.Decrypt(pass) + require.NoError(t, err) + + jwk := &jose.JSONWebKey{} + err = json.Unmarshal(d, jwk) + require.NoError(t, err) + + return jwk +} + +func generateOTT(t *testing.T, jwk *jose.JSONWebKey, subject string) string { + t.Helper() + now := time.Now() + + keyID, err := jose.Thumbprint(jwk) + require.NoError(t, err) + + opts := new(jose.SignerOptions).WithType("JWT").WithHeader("kid", keyID) + signer, err := jose.NewSigner(jose.SigningKey{Key: jwk.Key}, opts) + require.NoError(t, err) + + id, err := randutil.ASCII(64) + require.NoError(t, err) + + cl := struct { + jose.Claims + SANS []string `json:"sans"` + }{ + Claims: jose.Claims{ + ID: id, + Subject: subject, + Issuer: "jwk", + NotBefore: jose.NewNumericDate(now), + Expiry: jose.NewNumericDate(now.Add(time.Minute)), + Audience: []string{"https://127.0.0.1/1.0/sign"}, + }, + SANS: []string{subject}, + } + raw, err := jose.Signed(signer).Claims(cl).CompactSerialize() + require.NoError(t, err) + + return raw +} + +func newAuthorizingServer(t *testing.T, ca *minica.CA) *httptest.Server { + t.Helper() + + key, err := keyutil.GenerateDefaultSigner() + require.NoError(t, err) + + csr, err := x509util.CreateCertificateRequest("127.0.0.1", []string{"127.0.0.1"}, key) + require.NoError(t, err) + + crt, err := ca.SignCSR(csr) + require.NoError(t, err) + + srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if assert.Equal(t, "signRequestID", r.Header.Get("X-Request-Id")) { + json.NewEncoder(w).Encode(struct{ Allow bool }{Allow: true}) + w.WriteHeader(http.StatusOK) + return + } + + w.WriteHeader(http.StatusBadRequest) + })) + trustedRoots := x509.NewCertPool() + trustedRoots.AddCert(ca.Root) + + srv.TLS = &tls.Config{ + Certificates: []tls.Certificate{ + { + Certificate: [][]byte{crt.Raw, ca.Intermediate.Raw}, + PrivateKey: key, + Leaf: crt, + }, + }, + ClientCAs: trustedRoots, + ClientAuth: tls.RequireAndVerifyClientCert, + ServerName: "localhost", + } + + return srv +} From 2a47644d31458041b5e3f02ffc595d1ca10b6f6d Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 4 Mar 2024 12:01:25 +0100 Subject: [PATCH 16/16] Fix linting issue --- test/integration/requestid_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/requestid_test.go b/test/integration/requestid_test.go index f15db12f..54fd2eb0 100644 --- a/test/integration/requestid_test.go +++ b/test/integration/requestid_test.go @@ -248,7 +248,7 @@ func generateOTT(t *testing.T, jwk *jose.JSONWebKey, subject string) string { return raw } -func newAuthorizingServer(t *testing.T, ca *minica.CA) *httptest.Server { +func newAuthorizingServer(t *testing.T, mca *minica.CA) *httptest.Server { t.Helper() key, err := keyutil.GenerateDefaultSigner() @@ -257,7 +257,7 @@ func newAuthorizingServer(t *testing.T, ca *minica.CA) *httptest.Server { csr, err := x509util.CreateCertificateRequest("127.0.0.1", []string{"127.0.0.1"}, key) require.NoError(t, err) - crt, err := ca.SignCSR(csr) + crt, err := mca.SignCSR(csr) require.NoError(t, err) srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -270,12 +270,12 @@ func newAuthorizingServer(t *testing.T, ca *minica.CA) *httptest.Server { w.WriteHeader(http.StatusBadRequest) })) trustedRoots := x509.NewCertPool() - trustedRoots.AddCert(ca.Root) + trustedRoots.AddCert(mca.Root) srv.TLS = &tls.Config{ Certificates: []tls.Certificate{ { - Certificate: [][]byte{crt.Raw, ca.Intermediate.Raw}, + Certificate: [][]byte{crt.Raw, mca.Intermediate.Raw}, PrivateKey: key, Leaf: crt, },