Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add in --duplex flag to run HTTP and GRPC servers on the same port #931

Merged
merged 7 commits into from
Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 114 additions & 3 deletions cmd/app/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,22 @@ import (
"net/http"
"os"
"path/filepath"
"strconv"
"strings"
"time"

"chainguard.dev/go-grpc-kit/pkg/duplex"
"github.com/goadesign/goa/grpc/middleware"
ctclient "github.com/google/certificate-transparency-go/client"
"github.com/google/certificate-transparency-go/jsonclient"
grpcmw "github.com/grpc-ecosystem/go-grpc-middleware"
grpc_zap "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap"
grpc_recovery "github.com/grpc-ecosystem/go-grpc-middleware/recovery"
grpc_prometheus "github.com/grpc-ecosystem/go-grpc-prometheus"
"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"
"github.com/sigstore/fulcio/pkg/ca"
certauth "github.com/sigstore/fulcio/pkg/ca"
"github.com/sigstore/fulcio/pkg/ca/ephemeralca"
"github.com/sigstore/fulcio/pkg/ca/fileca"
Expand All @@ -39,12 +48,18 @@ import (
"github.com/sigstore/fulcio/pkg/ca/pkcs11ca"
"github.com/sigstore/fulcio/pkg/ca/tinkca"
"github.com/sigstore/fulcio/pkg/config"
"github.com/sigstore/fulcio/pkg/generated/protobuf"
"github.com/sigstore/fulcio/pkg/generated/protobuf/legacy"
"github.com/sigstore/fulcio/pkg/log"
"github.com/sigstore/fulcio/pkg/server"
"github.com/sigstore/sigstore/pkg/cryptoutils"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"github.com/spf13/viper"
"go.uber.org/zap"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/protobuf/proto"
)

const serveCmdEnvPrefix = "FULCIO_SERVE"
Expand Down Expand Up @@ -81,10 +96,11 @@ func newServeCmd() *cobra.Command {
cmd.Flags().String("tink-keyset-path", "", "Path to KMS-encrypted keyset for Tink-backed CA")
cmd.Flags().String("host", "0.0.0.0", "The host on which to serve requests for HTTP; --http-host is alias")
cmd.Flags().String("port", "8080", "The port on which to serve requests for HTTP; --http-port is alias")
cmd.Flags().String("grpc-host", "0.0.0.0", "The host on which to serve requests for GRPC")
cmd.Flags().String("grpc-port", "8081", "The port on which to serve requests for GRPC")
cmd.Flags().String("grpc-host", "0.0.0.0", "[DEPRECATED] The host on which to serve requests for GRPC")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer we don't deprecate these flags, because there are no plans to cut a new major release of Fulcio anytime soon, and ideally we don't mark something as deprecated right after we cut 1.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we eventually plan to remove them (which I think makes the most sense) then it's best to deprecate them sooner. IIUC we'll still maintain support for at least a few releases or a few months (whatever the policy comes up with suggests).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a great look to deprecate functionality right after we've introduced it as stable. I know it's a tiny part of the service, but I think we shouldn't begin deprecation of features right after a 1.0 release until we start thinking about a 2.0 release, and right now, I don't see any features in the pipeline to need a 2.0 release.

cmd.Flags().String("grpc-port", "8081", "[DEPRECATED] The port on which to serve requests for GRPC")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other issue is by deprecating these, we remove the ability to expose the service only on gRPC, which is removing functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case for GRPC only? My guess is there's a pretty small subset of people running their own Fulcio to begin with, and I'm not sure how likely it is they'd specifically want to run only GRPC. It's just a hunch, but I'm guessing removing this functionality probably wouldn't have much of an effect on users 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might affect those who run their own infrastructure.

cmd.Flags().String("metrics-port", "2112", "The port on which to serve prometheus metrics endpoint")
cmd.Flags().Duration("read-header-timeout", 10*time.Second, "The time allowed to read the headers of the requests in seconds")
cmd.Flags().Bool("duplex", false, "experimental: serve HTTP and GRPC on the same port instead of on two separate ports")
priyawadhwa marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about not adding any flags, but changing the behavior such that when the HTTP and gRPC ports match, we use duplex?

This has a few benefits:

  • No deprecations needed
  • No breaking changes, since it would not have been possible to set the port values to be the same previously
  • No additional flags, which I always see as a win

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it would work, but as a user I don't find it very intuitive. As a user I'd expect that setting --port and --grpc-port to the same value would cause an error. I'd still prefer to deprecate the grpc flags and have one --port flag to expose everything on, mostly because I think longterm it'll be cleaner, easier for users to understand, and because I don't see a use case for GRPC-only functionality.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the addition of this change as fixing the error of setting the ports to be the same, rather than adding new functionality. I also would prefer to not use duplex as a flag name since that's exposing implementation details.


// convert "http-host" flag to "host" and "http-port" flag to be "port"
cmd.Flags().SetNormalizeFunc(func(f *pflag.FlagSet, name string) pflag.NormalizedName {
Expand Down Expand Up @@ -116,6 +132,7 @@ func (la logAdaptor) Printf(s string, args ...interface{}) {
}

func runServeCmd(cmd *cobra.Command, args []string) {
ctx := cmd.Context()
// If a config file is provided, modify the viper config to locate and read it
if err := checkServeCmdConfigFile(); err != nil {
log.Logger.Fatal(err)
Expand Down Expand Up @@ -254,6 +271,25 @@ func runServeCmd(cmd *cobra.Command, args []string) {
}
}

if viper.GetBool("duplex") {
p := viper.GetString("port")
priyawadhwa marked this conversation as resolved.
Show resolved Hide resolved
port, err := strconv.Atoi(p)
if err != nil {
log.Logger.Fatal("%s is not a valid port", p)
}
mp := viper.GetString("metrics-port")
priyawadhwa marked this conversation as resolved.
Show resolved Hide resolved
metricsPort, err := strconv.Atoi(mp)
if err != nil {
log.Logger.Fatal("%s is not a valid port", mp)
}
if err := StartDuplexServer(ctx, cfg, ctClient, baseca, port, metricsPort); err != nil {
priyawadhwa marked this conversation as resolved.
Show resolved Hide resolved
log.Logger.Fatal(err)
}
priyawadhwa marked this conversation as resolved.
Show resolved Hide resolved
} else {
log.Logger.Warnln("Hosting HTTP and GRPC servers on different ports will soon be deprecated, please set the --duplex flag to serve both on a single port." +
"The single port can be set with the --port flag.")
}

httpServerEndpoint := fmt.Sprintf("%v:%v", viper.GetString("http-host"), viper.GetString("http-port"))

reg := prometheus.NewRegistry()
Expand All @@ -271,7 +307,7 @@ func runServeCmd(cmd *cobra.Command, args []string) {
}
legacyGRPCServer.startUnixListener()

httpServer := createHTTPServer(context.Background(), httpServerEndpoint, grpcServer, legacyGRPCServer)
httpServer := createHTTPServer(ctx, httpServerEndpoint, grpcServer, legacyGRPCServer)
httpServer.startListener()

readHeaderTimeout := viper.GetDuration("read-header-timeout")
Expand Down Expand Up @@ -313,3 +349,78 @@ func checkServeCmdConfigFile() error {
}
return nil
}

func StartDuplexServer(ctx context.Context, cfg *config.FulcioConfig, ctClient *ctclient.LogClient, baseca ca.CertificateAuthority, port, metricsPort int) error {
haydentherapper marked this conversation as resolved.
Show resolved Hide resolved
logger, opts := log.SetupGRPCLogging()

d := duplex.New(
port,
grpc.WithTransportCredentials(insecure.NewCredentials()),
runtime.WithMetadata(extractOIDCTokenFromAuthHeader),
grpc.UnaryInterceptor(grpcmw.ChainUnaryServer(
grpc_recovery.UnaryServerInterceptor(grpc_recovery.WithRecoveryHandlerContext(panicRecoveryHandler)), // recovers from per-transaction panics elegantly, so put it first
middleware.UnaryRequestID(middleware.UseXRequestIDMetadataOption(true), middleware.XRequestMetadataLimitOption(128)),
grpc_zap.UnaryServerInterceptor(logger, opts...),
PassFulcioConfigThruContext(cfg),
grpc_prometheus.UnaryServerInterceptor,
)),
grpc.MaxRecvMsgSize(int(maxMsgSize)),
runtime.WithForwardResponseOption(HTTPResponseModifier),
)

// GRPC server
grpcCAServer := server.NewGRPCCAServer(ctClient, baseca)
protobuf.RegisterCAServer(d.Server, grpcCAServer)
if err := d.RegisterHandler(ctx, protobuf.RegisterCAHandlerFromEndpoint); err != nil {
return fmt.Errorf("registering grpc ca handler: %w", err)
}

// Legacy server
legacyGRPCCAServer := server.NewLegacyGRPCCAServer(grpcCAServer)
legacy.RegisterCAServer(d.Server, legacyGRPCCAServer)
if err := d.RegisterHandler(ctx, legacy.RegisterCAHandlerFromEndpoint); err != nil {
return fmt.Errorf("registering legacy grpc ca handler: %w", err)
}

// Prometheus
reg := prometheus.NewRegistry()
grpcMetrics := grpc_prometheus.DefaultServerMetrics
grpcMetrics.EnableHandlingTimeHistogram()
reg.MustRegister(grpcMetrics, server.MetricLatency, server.RequestsCount)
grpc_prometheus.Register(d.Server)

// Register prometheus handle.
d.RegisterListenAndServeMetrics(metricsPort, false)

if err := d.ListenAndServe(ctx); err != nil {
return fmt.Errorf("duplex server: %w", err)
}
priyawadhwa marked this conversation as resolved.
Show resolved Hide resolved
return nil
}

// The fulcio HTTP legacy client requires a 201 response status code to pass
// However, even though the legacy client sets the 201 code, GRPC will automatically
// change it to 200, causing the cert request to fail
// GRPC recommends controlling HTTP status codes with this response modifier
// https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_your_gateway/#controlling-http-response-status-codes
// This is required to use fulcio with duplex.
func HTTPResponseModifier(ctx context.Context, w http.ResponseWriter, p proto.Message) error {
priyawadhwa marked this conversation as resolved.
Show resolved Hide resolved
md, ok := runtime.ServerMetadataFromContext(ctx)
if !ok {
return nil
}

// set http status code
if vals := md.HeaderMD.Get(server.HTTPResponseCodeMetadataKey); len(vals) > 0 {
code, err := strconv.Atoi(vals[0])
if err != nil {
return err
}
// delete the headers to not expose any grpc-metadata in http response
delete(md.HeaderMD, server.HTTPResponseCodeMetadataKey)
delete(w.Header(), "Grpc-Metadata-X-Http-Code")
w.WriteHeader(code)
}

return nil
}
112 changes: 112 additions & 0 deletions cmd/app/serve_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
// Copyright 2021 The Sigstore Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

package app

import (
"context"
"fmt"
"io"
"log"
"net/http"
"net/url"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/sigstore/fulcio/pkg/api"
"github.com/sigstore/fulcio/pkg/ca/ephemeralca"
"github.com/sigstore/fulcio/pkg/config"
"github.com/sigstore/fulcio/pkg/generated/protobuf"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
)

func TestDuplex(t *testing.T) {
// Start a server with duplex on port 8089
ctx := context.Background()
ca, err := ephemeralca.NewEphemeralCA()
if err != nil {
t.Fatal(err)
}
port := 8089
serverURL, err := url.Parse(fmt.Sprintf("http://localhost:%d", port))
if err != nil {
t.Fatal(err)
}
metricsPort := 2114

go func() {
if err := StartDuplexServer(ctx, config.DefaultConfig, nil, ca, port, metricsPort); err != nil {
log.Fatalf("error starting duplex server: %v", err)
}
}()

var rootCert string
t.Run("http", func(t *testing.T) {
// Make sure we can grab the rootcert with the v1 endpoint
legacyClient := api.NewClient(serverURL)
resp, err := legacyClient.RootCert()
if err != nil {
t.Fatal(err)
}
rootCert = string(resp.ChainPEM)
})

var grpcRootCert string
t.Run("grpc", func(t *testing.T) {
// Grab the rootcert with the v2 endpoint
conn, err := grpc.Dial(fmt.Sprintf("127.0.0.1:%d", port), grpc.WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
t.Fatal(err)
}
grpcClient := protobuf.NewCAClient(conn)
tb, err := grpcClient.GetTrustBundle(ctx, &protobuf.GetTrustBundleRequest{})
if err != nil {
t.Fatalf("error getting trust bundle: %v", err)
}
if len(tb.Chains) != 1 {
t.Fatalf("didn't get expected length certificate chain: %v", tb.Chains)
}
if len(tb.Chains[0].Certificates) != 1 {
t.Fatalf("didn't get expected length certs: %v", tb.Chains)
}
grpcRootCert = strings.TrimSuffix(tb.Chains[0].Certificates[0], "\n")
})

t.Run("compare root certs", func(t *testing.T) {
if d := cmp.Diff(rootCert, grpcRootCert); d != "" {
t.Fatal(d)
}
})

t.Run("prometheus", func(t *testing.T) {
// make sure there are metrics on the metrics port
url := fmt.Sprintf("http://localhost:%d/metrics", metricsPort)
resp, err := http.Get(url)
if err != nil {
t.Fatal(err)
}
contents, err := io.ReadAll(resp.Body)
if err != nil {
t.Fatal(err)
}
// make sure there's something about hitting the GetTrustBundle in there
// this just confirms some metrics are being printed
if !strings.Contains(string(contents), "GetTrustBundle") {
t.Fatalf("didn't get expected metrics output: %s", string(contents))
}
})
}
27 changes: 25 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module github.com/sigstore/fulcio
go 1.18

require (
chainguard.dev/go-grpc-kit v0.11.0
cloud.google.com/go/security v1.10.0
github.com/PaesslerAG/jsonpath v0.1.1
github.com/ThalesIgnite/crypto11 v1.2.5
Expand All @@ -15,7 +16,7 @@ require (
github.com/google/go-cmp v0.5.9
github.com/google/tink/go v1.7.0
github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.1-0.20210315223345-82c243799c99
github.com/grpc-ecosystem/grpc-gateway/v2 v2.14.0
github.com/hashicorp/golang-lru v0.5.4
github.com/magiconair/properties v1.8.7
Expand Down Expand Up @@ -76,16 +77,21 @@ require (
github.com/aws/aws-sdk-go-v2/service/sts v1.17.5 // indirect
github.com/aws/smithy-go v1.13.4 // indirect
github.com/beorn7/perks v1.0.1 // indirect
github.com/blendle/zapdriver v1.3.1 // indirect
github.com/cenkalti/backoff/v3 v3.2.2 // indirect
github.com/cenkalti/backoff/v4 v4.1.3 // indirect
github.com/cespare/xxhash/v2 v2.1.2 // indirect
github.com/common-nighthawk/go-figure v0.0.0-20210622060536-734e95fb86be // indirect
github.com/dimchansky/utfbom v1.1.1 // indirect
github.com/fatih/color v1.13.0 // indirect
github.com/go-logr/logr v1.2.0 // indirect
github.com/go-logr/logr v1.2.3 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang-jwt/jwt/v4 v4.4.2 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/snappy v0.0.4 // indirect
github.com/google/go-containerregistry v0.12.1 // indirect
github.com/google/gofuzz v1.2.0 // indirect
github.com/google/uuid v1.3.0 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.2.0 // indirect
github.com/googleapis/gax-go/v2 v2.7.0 // indirect
Expand All @@ -110,6 +116,7 @@ require (
github.com/inconshreveable/mousetrap v1.0.1 // indirect
github.com/jellydator/ttlcache/v2 v2.11.1 // indirect
github.com/jmespath/go-jmespath v0.4.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/letsencrypt/boulder v0.0.0-20221109233200-85aa52084eaf // indirect
github.com/mattn/go-colorable v0.1.13 // indirect
github.com/mattn/go-isatty v0.0.16 // indirect
Expand All @@ -120,6 +127,8 @@ require (
github.com/mitchellh/go-testing-interface v1.14.1 // indirect
github.com/mitchellh/mapstructure v1.5.0 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/oklog/run v1.1.0 // indirect
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/pelletier/go-toml v1.9.5 // indirect
Expand All @@ -138,6 +147,13 @@ require (
github.com/theupdateframework/go-tuf v0.5.2-0.20220930112810-3890c1e7ace4 // indirect
github.com/titanous/rocacheck v0.0.0-20171023193734-afe73141d399 // indirect
go.opencensus.io v0.24.0 // indirect
go.opentelemetry.io/otel v1.11.1 // indirect
go.opentelemetry.io/otel/exporters/otlp/internal/retry v1.11.1 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.11.1 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.11.1 // indirect
go.opentelemetry.io/otel/sdk v1.11.1 // indirect
go.opentelemetry.io/otel/trace v1.11.1 // indirect
go.opentelemetry.io/proto/otlp v0.19.0 // indirect
go.uber.org/atomic v1.10.0 // indirect
go.uber.org/multierr v1.8.0 // indirect
goa.design/goa v2.2.5+incompatible // indirect
Expand All @@ -150,7 +166,14 @@ require (
golang.org/x/text v0.5.0 // indirect
golang.org/x/time v0.2.0 // indirect
google.golang.org/appengine v1.6.7 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/ini.v1 v1.67.0 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
k8s.io/api v0.25.3 // indirect
k8s.io/apimachinery v0.25.3 // indirect
k8s.io/klog/v2 v2.80.1 // indirect
k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed // indirect
knative.dev/pkg v0.0.0-20221010143036-21d3b47e2efe // indirect
sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect
)
Loading