From 15bc6a4a67734d6213acef1b4796c9d5d82c335d Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 31 Mar 2022 14:48:52 -0400 Subject: [PATCH 1/3] Add more details to FIPS comments Signed-off-by: Monis Khan --- Dockerfile | 2 +- hack/Dockerfile_fips | 42 ++++++++++++++++---- internal/crypto/ptls/fips_strict.go | 11 +++-- test/integration/securetls_fips_test.go | 7 ++-- test/testlib/securetls.go | 2 +- test/testlib/securetls_preference_fips.go | 4 +- test/testlib/securetls_preference_nonfips.go | 4 +- 7 files changed, 47 insertions(+), 25 deletions(-) diff --git a/Dockerfile b/Dockerfile index 5588ff77..ecbc9220 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -# syntax = docker/dockerfile:1.0-experimental +# syntax=docker/dockerfile:1 # Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 diff --git a/hack/Dockerfile_fips b/hack/Dockerfile_fips index 1b1bf858..1dfbc243 100644 --- a/hack/Dockerfile_fips +++ b/hack/Dockerfile_fips @@ -1,10 +1,15 @@ -# syntax = docker/dockerfile:1.0-experimental +# syntax=docker/dockerfile:1 -# Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. +# Copyright 2022 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 # this dockerfile is used to produce a binary of Pinniped that uses -# only fips-allowable ciphers. +# only fips-allowable ciphers. Note that this is provided only as +# an example. Pinniped has no official support for fips and using +# a version built from this dockerfile may have unforseen consquences. +# Please do not create issues in regards to problems encountered by +# using this dockerfile. Using this dockerfile does not convey +# any type of fips certification. # use go-boringcrypto rather than main go FROM us-docker.pkg.dev/google.com/api-project-999119582588/go-boringcrypto/golang:1.17.8b7 as build-env @@ -13,12 +18,33 @@ WORKDIR /work COPY . . ARG GOPROXY -# Build the executable binary (CGO_ENABLED=1 is required for go boring) -# Pass in GOCACHE (build cache) and GOMODCACHE (module cache) so they -# can be re-used between image builds. +# Build the executable binary (CGO_ENABLED=1 is required for go boring). +# Even though we need cgo to call the boring crypto C functions, these +# functions are statically linked into the binary. We also want to statically +# link any libc bits hence we pass "-linkmode=external -extldflags -static" +# to the ldflags directive. We do not pass "-s" to ldflags because we do +# not want to strip symbols - those are used to verify if we compiled correctly. +# We do not pass in GOCACHE (build cache) and GOMODCACHE (module cache) +# because there have been bugs in the Go compiler caching when using cgo +# (it will sometimes use cached artifiacts when it should not). Since we +# use gcc as the C compiler, the following warning is emitted: +# /boring/boringssl/build/../crypto/bio/socket_helper.c:55: warning: +# Using 'getaddrinfo' in statically linked applications requires at +# runtime the shared libraries from the glibc version used for linking +# This is referring to the code in +# https://github.com/google/boringssl/blob/af34f6460f0bf99dc267818f02b2936f60a30de7/crypto/bio/socket_helper.c#L55 +# which calls the getaddrinfo function. This function, even when statically linked, +# uses dlopen to dynamically fetch networking config. It is safe for us to ignore +# this warning because the go boring cypto code does not create netowrking connections: +# https://github.com/golang/go/blob/9d6ab825f6fe125f7ce630e103b887e580403802/src/crypto/internal/boring/goboringcrypto.h +# The osusergo and netgo tags are used to make sure that the Go implementations of these +# standard library packages are used instead of the libc based versions. +# We want to have no reliance on any C code other than the boring crypto bits. +# Setting GOOS=linux GOARCH=amd64 is a hard requirment for boring crypto: +# https://github.com/golang/go/blob/9d6ab825f6fe125f7ce630e103b887e580403802/misc/boring/README.md?plain=1#L95 +# Thus trying to compile the pinniped CLI with boring crypto is meaningless +# since we would not be able to ship windows and macOS binaries. RUN \ - --mount=type=cache,target=/cache/gocache \ - --mount=type=cache,target=/cache/gomodcache \ mkdir out && \ export CGO_ENABLED=1 GOOS=linux GOARCH=amd64 && \ go build -tags fips_strict,osusergo,netgo -v -trimpath -ldflags "$(hack/get-ldflags.sh) -w -linkmode=external -extldflags -static" -o /usr/local/bin/pinniped-concierge-kube-cert-agent ./cmd/pinniped-concierge-kube-cert-agent/... && \ diff --git a/internal/crypto/ptls/fips_strict.go b/internal/crypto/ptls/fips_strict.go index e35a30a5..cebcebb4 100644 --- a/internal/crypto/ptls/fips_strict.go +++ b/internal/crypto/ptls/fips_strict.go @@ -10,12 +10,13 @@ package ptls import ( - "C" "crypto/tls" - _ "crypto/tls/fipsonly" // restricts all TLS configuration to FIPS-approved settings. "crypto/x509" "runtime" + "C" // explicitly import cgo so that runtime/cgo gets linked into the kube-cert-agent + _ "crypto/tls/fipsonly" // restricts all TLS configuration to FIPS-approved settings. + "go.pinniped.dev/internal/plog" ) @@ -24,10 +25,7 @@ const secureServingOptionsMinTLSVersion = "VersionTLS12" const SecureTLSConfigMinTLSVersion = tls.VersionTLS12 func init() { - go func() { - version := runtime.Version() - plog.Debug("using boringcrypto in fips only mode.", "go version", version) - }() + plog.Debug("using boring crypto in fips only mode", "go version", runtime.Version()) } func Default(rootCAs *x509.CertPool) *tls.Config { @@ -46,6 +44,7 @@ func Default(rootCAs *x509.CertPool) *tls.Config { // This is all of the fips-approved ciphers. // The list is hard-coded for convenience of testing. + // This is kept in sync with the boring crypto compiler via TestFIPSCipherSuites. CipherSuites: []uint16{ tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, diff --git a/test/integration/securetls_fips_test.go b/test/integration/securetls_fips_test.go index aed0c399..cc7a4789 100644 --- a/test/integration/securetls_fips_test.go +++ b/test/integration/securetls_fips_test.go @@ -138,10 +138,11 @@ func TestSecureTLSSupervisor(t *testing.T) { // does not run in parallel because require.Contains(t, stdout, testlib.GetExpectedCiphers(defaultECDSAOnly), "stdout:\n%s", stdout) } -// this test ensures that if the list of default fips cipher -// suites changes, we will know. +// TestFIPSCipherSuites_Parallel ensures that if the list of default fips cipher suites changes, +// we will know. This is an integration test because we do not support build tags on unit tests. func TestFIPSCipherSuites_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t) + server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // use the default fips config which contains a hard coded list of cipher suites // that should be equal to the default list of fips cipher suites. @@ -157,7 +158,7 @@ func TestFIPSCipherSuites_Parallel(t *testing.T) { // and therefore uses goboring's default fips ciphers. defaultConfig := &tls.Config{ RootCAs: pool, - NextProtos: ptls.Default(nil).NextProtos, + NextProtos: ptls.Default(nil).NextProtos, // we do not care about field for this test, so just make it match } transport := http.Transport{ TLSClientConfig: defaultConfig, diff --git a/test/testlib/securetls.go b/test/testlib/securetls.go index 3777c1aa..b7f45b92 100644 --- a/test/testlib/securetls.go +++ b/test/testlib/securetls.go @@ -87,7 +87,7 @@ func GetExpectedCiphers(config *tls.Config) string { } s.WriteString("\n") } - tls12Bit = fmt.Sprintf(tls12Base, s.String(), getCipherSuitePreference()) + tls12Bit = fmt.Sprintf(tls12Base, s.String(), cipherSuitePreference) } skip13 := config.MaxVersion == tls.VersionTLS12 diff --git a/test/testlib/securetls_preference_fips.go b/test/testlib/securetls_preference_fips.go index 3da1f5df..e61fc205 100644 --- a/test/testlib/securetls_preference_fips.go +++ b/test/testlib/securetls_preference_fips.go @@ -10,6 +10,4 @@ package testlib // incorrectly shown as 'client' in some cases. // in fips-only mode, it correctly shows the cipher preference // as 'server', while in non-fips mode it shows as 'client'. -func getCipherSuitePreference() string { - return "server" -} +const cipherSuitePreference = "server" diff --git a/test/testlib/securetls_preference_nonfips.go b/test/testlib/securetls_preference_nonfips.go index 86bf0edc..002d329d 100644 --- a/test/testlib/securetls_preference_nonfips.go +++ b/test/testlib/securetls_preference_nonfips.go @@ -10,6 +10,4 @@ package testlib // incorrectly shown as 'client' in some cases. // in fips-only mode, it correctly shows the cipher preference // as 'server', while in non-fips mode it shows as 'client'. -func getCipherSuitePreference() string { - return "client" -} +const cipherSuitePreference = "client" From 3f0753ec5a354cbefa0863adde198d20bcb86e3c Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 31 Mar 2022 15:31:20 -0400 Subject: [PATCH 2/3] Remove duplication in secure TLS tests Signed-off-by: Monis Khan --- internal/testutil/tlsserver/tlsserver.go | 10 +- test/integration/securetls_fips_test.go | 139 +---------------------- test/integration/securetls_test.go | 13 ++- 3 files changed, 11 insertions(+), 151 deletions(-) diff --git a/internal/testutil/tlsserver/tlsserver.go b/internal/testutil/tlsserver/tlsserver.go index 21238993..1585284d 100644 --- a/internal/testutil/tlsserver/tlsserver.go +++ b/internal/testutil/tlsserver/tlsserver.go @@ -69,14 +69,6 @@ func RecordTLSHello(server *httptest.Server) { func AssertTLS(t *testing.T, r *http.Request, tlsConfigFunc ptls.ConfigFunc) { t.Helper() - tlsConfig := tlsConfigFunc(nil) - - AssertTLSConfig(t, r, tlsConfig) -} - -func AssertTLSConfig(t *testing.T, r *http.Request, tlsConfig *tls.Config) { - t.Helper() - m, ok := getCtxMap(r.Context()) require.True(t, ok) @@ -86,6 +78,8 @@ func AssertTLSConfig(t *testing.T, r *http.Request, tlsConfig *tls.Config) { info, ok := h.(*tls.ClientHelloInfo) require.True(t, ok) + tlsConfig := tlsConfigFunc(nil) + supportedVersions := []uint16{tlsConfig.MinVersion} ciphers := tlsConfig.CipherSuites diff --git a/test/integration/securetls_fips_test.go b/test/integration/securetls_fips_test.go index cc7a4789..19cfea78 100644 --- a/test/integration/securetls_fips_test.go +++ b/test/integration/securetls_fips_test.go @@ -7,12 +7,8 @@ package integration import ( - "context" "crypto/tls" - "encoding/base64" - "fmt" "net/http" - "strings" "testing" "github.com/stretchr/testify/require" @@ -23,121 +19,6 @@ import ( "go.pinniped.dev/test/testlib" ) -// This test mirrors securetls_test.go, but adapted for fips mode. -// e.g. checks for only TLS 1.2 ciphers and checks for the -// list of fips-approved ciphers above. -// TLS checks safe to run in parallel with serial tests, see main_test.go. -func TestSecureTLSPinnipedCLIToKAS_Parallel(t *testing.T) { - _ = testlib.IntegrationEnv(t) - t.Log("testing FIPs tls config") - - server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // pinniped CLI uses ptls.Secure when talking to KAS, - // although the distinction doesn't matter much in FIPs mode because - // each of the configs is a wrapper for the same base FIPs config. - secure := ptls.Secure(nil) - tlsserver.AssertTLSConfig(t, r, secure) - w.Header().Set("content-type", "application/json") - fmt.Fprint(w, `{"kind":"TokenCredentialRequest","apiVersion":"login.concierge.pinniped.dev/v1alpha1",`+ - `"status":{"credential":{"token":"some-fancy-token"}}}`) - }), tlsserver.RecordTLSHello) - - ca := tlsserver.TLSTestServerCA(server) - - pinnipedExe := testlib.PinnipedCLIPath(t) - - stdout, stderr := runPinnipedCLI(t, nil, pinnipedExe, "login", "static", - "--token", "does-not-matter", - "--concierge-authenticator-type", "webhook", - "--concierge-authenticator-name", "does-not-matter", - "--concierge-ca-bundle-data", base64.StdEncoding.EncodeToString(ca), - "--concierge-endpoint", server.URL, - "--enable-concierge", - "--credential-cache", "", - ) - - require.Empty(t, stderr) - require.Equal(t, `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1",`+ - `"spec":{"interactive":false},"status":{"expirationTimestamp":null,"token":"some-fancy-token"}} -`, stdout) -} - -// TLS checks safe to run in parallel with serial tests, see main_test.go. -func TestSecureTLSPinnipedCLIToSupervisor_Parallel(t *testing.T) { - _ = testlib.IntegrationEnv(t) - - server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // pinniped CLI uses ptls.Default when talking to supervisor, - // although the distinction doesn't matter much in FIPs mode because - // each of the configs is a wrapper for the same base FIPs config. - defaultTLS := ptls.Default(nil) - tlsserver.AssertTLSConfig(t, r, defaultTLS) - w.Header().Set("content-type", "application/json") - fmt.Fprint(w, `{"issuer":"https://not-a-good-issuer"}`) - }), tlsserver.RecordTLSHello) - - ca := tlsserver.TLSTestServerCA(server) - - pinnipedExe := testlib.PinnipedCLIPath(t) - - stdout, stderr := runPinnipedCLI(&fakeT{T: t}, nil, pinnipedExe, "login", "oidc", - "--ca-bundle-data", base64.StdEncoding.EncodeToString(ca), - "--issuer", server.URL, - "--credential-cache", "", - "--upstream-identity-provider-flow", "cli_password", - "--upstream-identity-provider-name", "does-not-matter", - "--upstream-identity-provider-type", "oidc", - ) - - require.Equal(t, `Error: could not complete Pinniped login: could not perform OIDC discovery for "`+ - server.URL+`": oidc: issuer did not match the issuer returned by provider, expected "`+ - server.URL+`" got "https://not-a-good-issuer" -`, stderr) - require.Empty(t, stdout) -} - -// TLS checks safe to run in parallel with serial tests, see main_test.go. -func TestSecureTLSConciergeAggregatedAPI_Parallel(t *testing.T) { - env := testlib.IntegrationEnv(t) - - cancelCtx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - - startKubectlPortForward(cancelCtx, t, "10446", "443", env.ConciergeAppName+"-api", env.ConciergeNamespace) - - stdout, stderr := testlib.RunNmapSSLEnum(t, "127.0.0.1", 10446) - - require.Empty(t, stderr) - secure := ptls.Secure(nil) - require.Contains(t, stdout, testlib.GetExpectedCiphers(secure), "stdout:\n%s", stdout) -} - -func TestSecureTLSSupervisor(t *testing.T) { // does not run in parallel because of the createSupervisorDefaultTLSCertificateSecretIfNeeded call - env := testlib.IntegrationEnv(t) - - ctx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - - startKubectlPortForward(ctx, t, "10447", "443", env.SupervisorAppName+"-nodeport", env.SupervisorNamespace) - - stdout, stderr := testlib.RunNmapSSLEnum(t, "127.0.0.1", 10447) - - // supervisor's cert is ECDSA - defaultECDSAOnly := ptls.Default(nil) - ciphers := make([]uint16, 0, len(defaultECDSAOnly.CipherSuites)/3) - for _, id := range defaultECDSAOnly.CipherSuites { - id := id - if !strings.Contains(tls.CipherSuiteName(id), "_ECDSA_") { - continue - } - ciphers = append(ciphers, id) - } - defaultECDSAOnly.CipherSuites = ciphers - - require.Empty(t, stderr) - require.Contains(t, stdout, testlib.GetExpectedCiphers(defaultECDSAOnly), "stdout:\n%s", stdout) -} - // TestFIPSCipherSuites_Parallel ensures that if the list of default fips cipher suites changes, // we will know. This is an integration test because we do not support build tags on unit tests. func TestFIPSCipherSuites_Parallel(t *testing.T) { @@ -146,9 +27,8 @@ func TestFIPSCipherSuites_Parallel(t *testing.T) { server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // use the default fips config which contains a hard coded list of cipher suites // that should be equal to the default list of fips cipher suites. - defaultTLS := ptls.Default(nil) // assert that the client hello response has the same tls config as this test server. - tlsserver.AssertTLSConfig(t, r, defaultTLS) + tlsserver.AssertTLS(t, r, ptls.Default) }), tlsserver.RecordTLSHello) ca := tlsserver.TLSTestServerCA(server) @@ -173,20 +53,3 @@ func TestFIPSCipherSuites_Parallel(t *testing.T) { require.NoError(t, err) require.Equal(t, http.StatusOK, response.StatusCode) } - -type fakeT struct { - *testing.T -} - -func (t *fakeT) FailNow() { - t.Errorf("fakeT ignored FailNow") -} - -func (t *fakeT) Errorf(format string, args ...interface{}) { - t.Cleanup(func() { - if !t.Failed() { - return - } - t.Logf("reporting previously ignored errors since main test failed:\n"+format, args...) - }) -} diff --git a/test/integration/securetls_test.go b/test/integration/securetls_test.go index 05ef2586..e4324692 100644 --- a/test/integration/securetls_test.go +++ b/test/integration/securetls_test.go @@ -1,9 +1,6 @@ // Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -//go:build !fips_strict -// +build !fips_strict - package integration import ( @@ -27,7 +24,10 @@ func TestSecureTLSPinnipedCLIToKAS_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t) server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tlsserver.AssertTLS(t, r, ptls.Secure) // pinniped CLI uses ptls.Secure when talking to KAS + // pinniped CLI uses ptls.Secure when talking to KAS + // in FIPs mode the distinction doesn't matter much because + // each of the configs is a wrapper for the same base FIPs config + tlsserver.AssertTLS(t, r, ptls.Secure) w.Header().Set("content-type", "application/json") fmt.Fprint(w, `{"kind":"TokenCredentialRequest","apiVersion":"login.concierge.pinniped.dev/v1alpha1",`+ `"status":{"credential":{"token":"some-fancy-token"}}}`) @@ -58,7 +58,10 @@ func TestSecureTLSPinnipedCLIToSupervisor_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t) server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - tlsserver.AssertTLS(t, r, ptls.Default) // pinniped CLI uses ptls.Default when talking to supervisor + // pinniped CLI uses ptls.Default when talking to supervisor + // in FIPs mode the distinction doesn't matter much because + // each of the configs is a wrapper for the same base FIPs config + tlsserver.AssertTLS(t, r, ptls.Default) w.Header().Set("content-type", "application/json") fmt.Fprint(w, `{"issuer":"https://not-a-good-issuer"}`) }), tlsserver.RecordTLSHello) From 07066e020d52914a60a91b6ee5f67d7a5375f82f Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 31 Mar 2022 17:07:47 -0400 Subject: [PATCH 3/3] Explicitly set defaultServing ciphers in FIPS mode This is a no-op today, but could change in the future when we add support for FIPS in non-strict mode. Signed-off-by: Monis Khan --- internal/crypto/ptls/fips_strict.go | 6 ++++++ internal/crypto/ptls/ptls.go | 5 ----- internal/crypto/ptls/secure.go | 7 +++++++ 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/internal/crypto/ptls/fips_strict.go b/internal/crypto/ptls/fips_strict.go index cebcebb4..a6a3c65d 100644 --- a/internal/crypto/ptls/fips_strict.go +++ b/internal/crypto/ptls/fips_strict.go @@ -17,6 +17,8 @@ import ( "C" // explicitly import cgo so that runtime/cgo gets linked into the kube-cert-agent _ "crypto/tls/fipsonly" // restricts all TLS configuration to FIPS-approved settings. + "k8s.io/apiserver/pkg/server/options" + "go.pinniped.dev/internal/plog" ) @@ -63,3 +65,7 @@ func Secure(rootCAs *x509.CertPool) *tls.Config { func DefaultLDAP(rootCAs *x509.CertPool) *tls.Config { return Default(rootCAs) } + +func secureServing(opts *options.SecureServingOptionsWithLoopback) { + defaultServing(opts) +} diff --git a/internal/crypto/ptls/ptls.go b/internal/crypto/ptls/ptls.go index 3eb45d41..ab331e38 100644 --- a/internal/crypto/ptls/ptls.go +++ b/internal/crypto/ptls/ptls.go @@ -82,11 +82,6 @@ func defaultServing(opts *options.SecureServingOptionsWithLoopback) { opts.MinTLSVersion = defaultServingOptionsMinTLSVersion } -func secureServing(opts *options.SecureServingOptionsWithLoopback) { - opts.MinTLSVersion = secureServingOptionsMinTLSVersion - opts.CipherSuites = nil -} - func secureClient(opts *options.RecommendedOptions, f RestConfigFunc) error { inClusterClient, inClusterConfig, err := f(nil) if err != nil { diff --git a/internal/crypto/ptls/secure.go b/internal/crypto/ptls/secure.go index 9f07b633..ddea0816 100644 --- a/internal/crypto/ptls/secure.go +++ b/internal/crypto/ptls/secure.go @@ -9,6 +9,8 @@ package ptls import ( "crypto/tls" "crypto/x509" + + "k8s.io/apiserver/pkg/server/options" ) // secureServingOptionsMinTLSVersion is the minimum tls version in the format @@ -42,3 +44,8 @@ func Secure(rootCAs *x509.CertPool) *tls.Config { } return c } + +func secureServing(opts *options.SecureServingOptionsWithLoopback) { + opts.MinTLSVersion = secureServingOptionsMinTLSVersion + opts.CipherSuites = nil +}