letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

wfe2: don't pass through client-initiated cancellation (#6608) 

And clean up the code and tests that were used for cancellation
pass-through.

Fixes #6603
This commit is contained in:
Jacob Hoffman-Andrews 2023-01-26 17:26:15 -08:00 committed by GitHub
parent a7dc34f127
commit c23e59ba59
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 23 additions and 272 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
cmd/boulder-sa/main.go
View File

@ -111,7 +111,7 @@ func main() {
start, stop, err := bgrpc.NewServer(c.SA.GRPC).Add(
&sapb.StorageAuthorityReadOnly_ServiceDesc, saroi).Add(
&sapb.StorageAuthority_ServiceDesc, sai).Build(
tls, scope, clk, bgrpc.NoCancelInterceptor)
tls, scope, clk)
cmd.FailOnError(err, "Unable to setup SA gRPC server")
go cmd.CatchSignals(logger, stop)

8
cmd/boulder-wfe2/main.go
View File

@ -280,23 +280,23 @@ func setupWFE(c Config, scope prometheus.Registerer, clk clock.Clock) (rapb.Regi
tlsConfig, err := c.WFE.TLS.Load()
cmd.FailOnError(err, "TLS config")
raConn, err := bgrpc.ClientSetup(c.WFE.RAService, tlsConfig, scope, clk, bgrpc.CancelTo408Interceptor)
raConn, err := bgrpc.ClientSetup(c.WFE.RAService, tlsConfig, scope, clk)
cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to RA")
rac := rapb.NewRegistrationAuthorityClient(raConn)
saConn, err := bgrpc.ClientSetup(c.WFE.SAService, tlsConfig, scope, clk, bgrpc.CancelTo408Interceptor)
saConn, err := bgrpc.ClientSetup(c.WFE.SAService, tlsConfig, scope, clk)
cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to SA")
sac := sapb.NewStorageAuthorityReadOnlyClient(saConn)
var rns noncepb.NonceServiceClient
npm := map[string]noncepb.NonceServiceClient{}
if c.WFE.GetNonceService != nil {
rnsConn, err := bgrpc.ClientSetup(c.WFE.GetNonceService, tlsConfig, scope, clk, bgrpc.CancelTo408Interceptor)
rnsConn, err := bgrpc.ClientSetup(c.WFE.GetNonceService, tlsConfig, scope, clk)
cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to get nonce service")
rns = noncepb.NewNonceServiceClient(rnsConn)
for prefix, serviceConfig := range c.WFE.RedeemNonceServices {
serviceConfig := serviceConfig
conn, err := bgrpc.ClientSetup(&serviceConfig, tlsConfig, scope, clk, bgrpc.CancelTo408Interceptor)
conn, err := bgrpc.ClientSetup(&serviceConfig, tlsConfig, scope, clk)
cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to redeem nonce service")
npm[prefix] = noncepb.NewNonceServiceClient(conn)
}

6
grpc/client.go
View File

@ -26,7 +26,7 @@ import (
// a client certificate and validates the the server certificate based
// on the provided *tls.Config.
// It dials the remote service and returns a grpc.ClientConn if successful.
func ClientSetup(c *cmd.GRPCClientConfig, tlsConfig *tls.Config, statsRegistry prometheus.Registerer, clk clock.Clock, interceptors ...grpc.UnaryClientInterceptor) (*grpc.ClientConn, error) {
func ClientSetup(c *cmd.GRPCClientConfig, tlsConfig *tls.Config, statsRegistry prometheus.Registerer, clk clock.Clock) (*grpc.ClientConn, error) {
if c == nil {
return nil, errors.New("nil gRPC client config provided: JSON config is probably missing a fooService section")
}
@ -41,11 +41,11 @@ func ClientSetup(c *cmd.GRPCClientConfig, tlsConfig *tls.Config, statsRegistry p
cmi := clientMetadataInterceptor{c.Timeout.Duration, metrics, clk}
unaryInterceptors := append(interceptors, []grpc.UnaryClientInterceptor{
unaryInterceptors := []grpc.UnaryClientInterceptor{
cmi.Unary,
cmi.metrics.grpcMetrics.UnaryClientInterceptor(),
hnygrpc.UnaryClientInterceptor(),
}...)
}
streamInterceptors := []grpc.StreamClientInterceptor{
cmi.Stream,

3
grpc/client_test.go
View File

@ -8,7 +8,6 @@ import (
"github.com/letsencrypt/boulder/cmd"
"github.com/letsencrypt/boulder/metrics"
"github.com/letsencrypt/boulder/test"
"google.golang.org/grpc"
_ "google.golang.org/grpc/health"
)
@ -30,7 +29,7 @@ func TestClientSetup(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
client, err := ClientSetup(tt.cfg, &tls.Config{}, metrics.NoopRegisterer, clock.NewFake(), []grpc.UnaryClientInterceptor{}...)
client, err := ClientSetup(tt.cfg, &tls.Config{}, metrics.NoopRegisterer, clock.NewFake())
if tt.wantErr {
test.AssertError(t, err, "expected error, got nil")
} else {

33
grpc/interceptors.go
View File

@ -18,7 +18,6 @@ import (
"github.com/letsencrypt/boulder/cmd"
berrors "github.com/letsencrypt/boulder/errors"
"github.com/letsencrypt/boulder/probs"
)
const (
@ -54,24 +53,6 @@ type clientInterceptor interface {
Stream(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, streamer grpc.Streamer, opts ...grpc.CallOption) (grpc.ClientStream, error)
}
// NoCancelInterceptor is a gRPC interceptor that creates a new context,
// separate from the original context, that has the same deadline but does
// not propagate cancellation. This is used by SA.
//
// Because this interceptor throws away annotations on the context, it
// breaks tracing for events that get the modified context. To minimize that
// impact, this interceptor should always be last.
func NoCancelInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
cancel := func() {}
if deadline, ok := ctx.Deadline(); ok {
ctx, cancel = context.WithDeadline(context.Background(), deadline)
} else {
ctx = context.Background()
}
defer cancel()
return handler(ctx, req)
}
// serverMetadataInterceptor is a gRPC interceptor that adds Prometheus
// metrics to requests handled by a gRPC server, and wraps Boulder-specific
// errors for transmission in a grpc/metadata trailer (see bcodes.go).
@ -430,20 +411,6 @@ func (cmi *clientMetadataInterceptor) Stream(
var _ clientInterceptor = (*clientMetadataInterceptor)(nil)
// CancelTo408Interceptor calls the underlying invoker, checks to see if the
// resulting error was a gRPC Canceled error (because this client cancelled
// the request, likely because the ACME client itself canceled the HTTP
// request), and converts that into a Problem which can be "returned" to the
// (now missing) client, and into our logs. This should be the outermost client
// interceptor, and should only be enabled in the WFEs.
func CancelTo408Interceptor(ctx context.Context, fullMethod string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error {
err := invoker(ctx, fullMethod, req, reply, cc, opts...)
if err != nil && status.Code(err) == codes.Canceled {
return probs.Canceled(err.Error())
}
return err
}
// deadlineDetails is an error type that we use in place of gRPC's
// DeadlineExceeded errors in order to add more detail for debugging.
type deadlineDetails struct {

37
grpc/interceptors_test.go
View File

@ -8,7 +8,6 @@ import (
"fmt"
"log"
"net"
"net/http"
"strconv"
"strings"
"sync"
@ -28,7 +27,6 @@ import (
"github.com/letsencrypt/boulder/grpc/test_proto"
"github.com/letsencrypt/boulder/metrics"
"github.com/letsencrypt/boulder/probs"
"github.com/letsencrypt/boulder/test"
)
@ -90,19 +88,6 @@ func TestClientInterceptor(t *testing.T) {
test.AssertError(t, err, "ci.intercept didn't fail when handler returned a error")
}
func TestCancelTo408Interceptor(t *testing.T) {
err := CancelTo408Interceptor(context.Background(), "-service-test", nil, nil, nil, testInvoker)
test.AssertNotError(t, err, "CancelTo408Interceptor returned an error when it shouldn't")
err = CancelTo408Interceptor(context.Background(), "-service-requesterCanceledTest", nil, nil, nil, testInvoker)
test.AssertError(t, err, "CancelTo408Interceptor didn't return an error when it should")
var probDetails *probs.ProblemDetails
test.AssertErrorWraps(t, err, &probDetails)
test.AssertEquals(t, probDetails.Type, probs.MalformedProblem)
test.AssertEquals(t, probDetails.HTTPStatus, http.StatusRequestTimeout)
}
// TestFailFastFalse sends a gRPC request to a backend that is
// unavailable, and ensures that the request doesn't error out until the
// timeout is reached, i.e. that FailFast is set to false.
@ -378,28 +363,6 @@ func TestInFlightRPCStat(t *testing.T) {
test.AssertMetricWithLabelsEquals(t, ci.metrics.inFlightRPCs, labels, 0)
}
func TestNoCancelInterceptor(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
ctx, cancel2 := context.WithDeadline(ctx, time.Now().Add(time.Second))
handler := func(ctx context.Context, req interface{}) (interface{}, error) {
select {
case <-ctx.Done():
return nil, errors.New("oh no canceled")
case <-time.After(50 * time.Millisecond):
}
return nil, nil
}
go func() {
time.Sleep(10 * time.Millisecond)
cancel()
cancel2()
}()
_, err := NoCancelInterceptor(ctx, nil, nil, handler)
if err != nil {
t.Error(err)
}
}
func TestServiceAuthChecker(t *testing.T) {
ac := authInterceptor{
map[string]map[string]struct{}{

6
grpc/server.go
View File

@ -67,7 +67,7 @@ func (sb *serverBuilder) Add(desc *grpc.ServiceDesc, impl any) *serverBuilder {
// all of the services added to the builder. It also exposes a health check
// service. It returns two functions, start() and stop(), which should be used
// to start and gracefully stop the server.
func (sb *serverBuilder) Build(tlsConfig *tls.Config, statsRegistry prometheus.Registerer, clk clock.Clock, interceptors ...grpc.UnaryServerInterceptor) (func() error, func(), error) {
func (sb *serverBuilder) Build(tlsConfig *tls.Config, statsRegistry prometheus.Registerer, clk clock.Clock) (func() error, func(), error) {
// Add the health service to all servers.
healthSrv := health.NewServer()
sb = sb.Add(&healthpb.Health_ServiceDesc, healthSrv)
@ -127,12 +127,12 @@ func (sb *serverBuilder) Build(tlsConfig *tls.Config, statsRegistry prometheus.R
mi := newServerMetadataInterceptor(metrics, clk)
unaryInterceptors := append([]grpc.UnaryServerInterceptor{
unaryInterceptors := []grpc.UnaryServerInterceptor{
mi.metrics.grpcMetrics.UnaryServerInterceptor(),
ai.Unary,
mi.Unary,
hnygrpc.UnaryServerInterceptor(),
}, interceptors...)
}
streamInterceptors := []grpc.StreamServerInterceptor{
mi.metrics.grpcMetrics.StreamServerInterceptor(),

6
ocsp/responder/responder.go
View File

@ -182,7 +182,11 @@ func (rs Responder) sampledError(format string, a ...interface{}) {
// strings of repeated '/' into a single '/', which will break the base64
// encoding.
func (rs Responder) ServeHTTP(response http.ResponseWriter, request *http.Request) {
ctx := request.Context()
// We specifically ignore request.Context() because we would prefer for clients
// to not be able to cancel our operations in arbitrary places. Instead we
// start a new context, and apply timeouts in our various RPCs.
// TODO(go1.22?): Use context.Detach()
ctx := context.Background()
if rs.timeout != 0 {
var cancel func()

7
web/context.go
View File

@ -127,7 +127,12 @@ func (th *TopHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
Origin: r.Header.Get("Origin"),
Extra: make(map[string]interface{}),
}
ctx := r.Context()
// We specifically override the default r.Context() because we would prefer
// for clients to not be able to cancel our operations in arbitrary places.
// Instead we start a new context, and apply timeouts in our various RPCs.
// TODO(go1.22?): Use context.Detach()
ctx := context.Background()
r = r.WithContext(ctx)
beeline.AddFieldToTrace(ctx, "real_ip", logEvent.RealIP)
beeline.AddFieldToTrace(ctx, "method", logEvent.Method)
beeline.AddFieldToTrace(ctx, "user_agent", logEvent.UserAgent)

51
wfe2/verify_test.go
View File

@ -16,7 +16,6 @@ import (
corepb "github.com/letsencrypt/boulder/core/proto"
bgrpc "github.com/letsencrypt/boulder/grpc"
"github.com/letsencrypt/boulder/mocks"
noncepb "github.com/letsencrypt/boulder/nonce/proto"
"github.com/letsencrypt/boulder/probs"
sapb "github.com/letsencrypt/boulder/sa/proto"
"github.com/letsencrypt/boulder/test"
@ -24,7 +23,6 @@ import (
"github.com/prometheus/client_golang/prometheus"
"google.golang.org/grpc"
"google.golang.org/protobuf/types/known/emptypb"
"gopkg.in/go-jose/go-jose.v2"
)
@ -1649,52 +1647,3 @@ func TestMatchJWSURLs(t *testing.T) {
})
}
}
type alwaysCancelNonceService struct{}
func (acns alwaysCancelNonceService) Redeem(ctx context.Context, msg *noncepb.NonceMessage, opts ...grpc.CallOption) (*noncepb.ValidMessage, error) {
return nil, probs.Canceled("user canceled request")
}
func (acns alwaysCancelNonceService) Nonce(ctx context.Context, in *emptypb.Empty, opts ...grpc.CallOption) (*noncepb.NonceMessage, error) {
return nil, probs.Canceled("user canceled request")
}
// Test that cancellation of the nonce lookup will result in a 408, via the
// CancelTo408Interceptor in grpc/interceptors.go.
func TestNoncePassThrough408Problem(t *testing.T) {
wfe, _, signer := setupWFE(t)
jws, _, _ := signer.byKeyID(1234, nil, "http://example.com/", "request-body")
for k := range wfe.noncePrefixMap {
wfe.noncePrefixMap[k] = alwaysCancelNonceService{}
}
wfe.remoteNonceService = alwaysCancelNonceService{}
prob := wfe.validNonce(context.Background(), jws)
fmt.Printf("%s", prob)
test.AssertNotNil(t, prob, "expected failure")
test.AssertEquals(t, prob.HTTPStatus, http.StatusRequestTimeout)
}
type alwaysCancelAccountGetter struct{}
// GetRegistration implements AccountGetter
func (alwaysCancelAccountGetter) GetRegistration(ctx context.Context, regID *sapb.RegistrationID, opts ...grpc.CallOption) (*corepb.Registration, error) {
return nil, probs.Canceled("user canceled request")
}
// Test that cancellation of the account lookup will result in a 408, via the
// CancelTo408Interceptor in grpc/interceptors.go.
func TestAccountLookupPassThrough408Problem(t *testing.T) {
wfe, _, signer := setupWFE(t)
wfe.accountGetter = alwaysCancelAccountGetter{}
jws, _, jwsBody := signer.byKeyID(1234, nil, "http://example.com/", "request-body")
req := makePostRequestWithPath("test-path", jwsBody)
_, _, prob := wfe.lookupJWK(jws, context.Background(), req, newRequestEvent())
test.AssertNotNil(t, prob, "expected failure")
test.AssertEquals(t, prob.HTTPStatus, http.StatusRequestTimeout)
}

136
wfe2/wfe_test.go
View File

@ -420,23 +420,6 @@ func addHeadIfGet(s []string) []string {
return s
}
func TestGetNonceCancellationBecomes408(t *testing.T) {
wfe, _, _ := setupWFE(t)
mux := http.NewServeMux()
rw := httptest.NewRecorder()
for k := range wfe.noncePrefixMap {
wfe.noncePrefixMap[k] = alwaysCancelNonceService{}
}
wfe.remoteNonceService = alwaysCancelNonceService{}
wfe.HandleFunc(mux, "/foo", func(context.Context, *web.RequestEvent, http.ResponseWriter, *http.Request) {
}, "POST")
req, err := http.NewRequest("POST", "/foo", nil)
test.AssertNotError(t, err, "creating request")
mux.ServeHTTP(rw, req)
test.AssertEquals(t, rw.Code, 408)
}
func TestHandleFunc(t *testing.T) {
wfe, _, _ := setupWFE(t)
var mux *http.ServeMux
@ -1192,54 +1175,6 @@ func TestChallenge(t *testing.T) {
}
}
type mockSAGetCertificateCanceled struct {
*mocks.StorageAuthorityReadOnly
}
func (mockSAGetCertificateCanceled) GetCertificate(context.Context, *sapb.Serial, ...grpc.CallOption) (*corepb.Certificate, error) {
return nil, probs.Canceled("canceled!")
}
// When SA.GetCertificate is canceled, return 408.
func TestGetCertificateCanceled(t *testing.T) {
wfe, _, _ := setupWFE(t)
wfe.sa = mockSAGetCertificateCanceled{}
responseWriter := httptest.NewRecorder()
req, err := http.NewRequest("GET", "333333333333333333333333333333333333", nil)
test.AssertNotError(t, err, "creating request")
wfe.Certificate(ctx, newRequestEvent(), responseWriter, req)
test.AssertEquals(t, responseWriter.Code, http.StatusRequestTimeout)
}
type mockSAGetAuthorization2Canceled struct {
*mocks.StorageAuthorityReadOnly
}
func (mockSAGetAuthorization2Canceled) GetAuthorization2(context.Context, *sapb.AuthorizationID2, ...grpc.CallOption) (*corepb.Authorization, error) {
return nil, probs.Canceled("canceled!")
}
// When SA.GetAuthorization2 is canceled during a Challenge POST or Authorization GET, return 408.
func TestGetAuthorization2Canceled408(t *testing.T) {
wfe, _, signer := setupWFE(t)
wfe.sa = mockSAGetAuthorization2Canceled{}
post := func(path string) *http.Request {
signedURL := fmt.Sprintf("http://localhost/%s", path)
_, _, jwsBody := signer.byKeyID(1, nil, signedURL, `{}`)
return makePostRequestWithPath(path, jwsBody)
}
responseWriter := httptest.NewRecorder()
wfe.Challenge(ctx, newRequestEvent(), responseWriter, post("1/-ZfxEw"))
test.AssertEquals(t, responseWriter.Code, http.StatusRequestTimeout)
req, err := http.NewRequest("GET", "3", nil)
test.AssertNotError(t, err, "creating request")
responseWriter = httptest.NewRecorder()
wfe.Authorization(ctx, newRequestEvent(), responseWriter, req)
test.AssertEquals(t, responseWriter.Code, http.StatusRequestTimeout)
}
// MockRAPerformValidationError is a mock RA that just returns an error on
// PerformValidation.
type MockRAPerformValidationError struct {
@ -2883,30 +2818,6 @@ func TestKeyRolloverMismatchedJWSURLs(t *testing.T) {
}`)
}
type mockSAGetOrderCanceled struct {
sapb.StorageAuthorityReadOnlyClient
}
func (sa mockSAGetOrderCanceled) GetOrder(_ context.Context, req *sapb.OrderRequest, _ ...grpc.CallOption) (*corepb.Order, error) {
return nil, probs.Canceled("canceled!")
}
func TestGetOrderCanceled408(t *testing.T) {
wfe, _, signer := setupWFE(t)
wfe.sa = mockSAGetOrderCanceled{}
responseWriter := httptest.NewRecorder()
req, err := http.NewRequest("GET", "123/456", nil)
test.AssertNotError(t, err, "creating request")
wfe.GetOrder(ctx, newRequestEvent(), responseWriter, req)
test.AssertEquals(t, responseWriter.Code, http.StatusRequestTimeout)
_, _, jwsBody := signer.byKeyID(1, nil, "http://localhost/123/456", "{}")
postReq := makePostRequestWithPath("123/456", jwsBody)
responseWriter = httptest.NewRecorder()
wfe.FinalizeOrder(ctx, newRequestEvent(), responseWriter, postReq)
test.AssertEquals(t, responseWriter.Code, http.StatusRequestTimeout)
}
func TestGetOrder(t *testing.T) {
wfe, _, signer := setupWFE(t)
@ -3266,29 +3177,6 @@ func TestNewAccountWhenGetRegByKeyFails(t *testing.T) {
}
}
type mockSAGetRegByKeyCanceled struct {
sapb.StorageAuthorityReadOnlyClient
}
func (sa *mockSAGetRegByKeyCanceled) GetRegistrationByKey(_ context.Context, req *sapb.JSONWebKey, _ ...grpc.CallOption) (*corepb.Registration, error) {
return nil, probs.Canceled("canceled!")
}
// When SA.GetRegistrationByKey is canceled (i.e. by the client), NewAccount should
// return 408.
func TestNewAccountWhenGetRegByKeyCanceled(t *testing.T) {
wfe, _, signer := setupWFE(t)
wfe.sa = &mockSAGetRegByKeyCanceled{wfe.sa}
key := loadKey(t, []byte(testE2KeyPrivatePEM))
_, ok := key.(*ecdsa.PrivateKey)
test.Assert(t, ok, "Couldn't load ECDSA key")
payload := `{"contact":["mailto:person@mail.com"],"agreement":"` + agreementURL + `"}`
responseWriter := httptest.NewRecorder()
_, _, body := signer.embeddedJWK(key, "http://localhost/new-account", payload)
wfe.NewAccount(ctx, newRequestEvent(), responseWriter, makePostRequestWithPath("/new-account", body))
test.AssertEquals(t, responseWriter.Code, http.StatusRequestTimeout)
}
type mockSAGetRegByKeyNotFound struct {
sapb.StorageAuthorityReadOnlyClient
}
@ -3330,30 +3218,6 @@ func TestNewAccountWhenGetRegByKeyNotFound(t *testing.T) {
}`)
}
// mockRANewRegCanceled is a mock RA that always returns a `berrors.MissingSCTsError` from `FinalizeOrder`
type mockRANewRegCanceled struct {
MockRegistrationAuthority
}
func (ra *mockRANewRegCanceled) NewRegistration(context.Context, *corepb.Registration, ...grpc.CallOption) (*corepb.Registration, error) {
return nil, probs.Canceled("canceled!")
}
// When RA.GetRegistrationByKey is canceled (i.e. by the client), NewAccount should
// return 408.
func TestNewAccountWhenRANewRegCanceled(t *testing.T) {
wfe, _, signer := setupWFE(t)
wfe.ra = &mockRANewRegCanceled{}
key := loadKey(t, []byte(testE2KeyPrivatePEM))
_, ok := key.(*ecdsa.PrivateKey)
test.Assert(t, ok, "Couldn't load ECDSA key")
payload := `{"contact":["mailto:person@mail.com"],"termsOfServiceAgreed":true}`
responseWriter := httptest.NewRecorder()
_, _, body := signer.embeddedJWK(key, "http://localhost/new-account", payload)
wfe.NewAccount(ctx, newRequestEvent(), responseWriter, makePostRequestWithPath("/new-account", body))
test.AssertEquals(t, responseWriter.Code, http.StatusRequestTimeout)
}
func TestPrepAuthzForDisplay(t *testing.T) {
wfe, _, _ := setupWFE(t)