Merge pull request #1106 from enj/enj/i/fips_followup

Add more details to FIPS comments

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

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)
+# Build the executable binary (CGO_ENABLED=1 is required for go boring).
-# Pass in GOCACHE (build cache) and GOMODCACHE (module cache) so they
+# Even though we need cgo to call the boring crypto C functions, these
-# can be re-used between image builds.
+# 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/... && \

internal/crypto/ptls/fips_strict.go

@@ -10,12 +10,15 @@
 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.
+
+	"k8s.io/apiserver/pkg/server/options"
+
 	"go.pinniped.dev/internal/plog"
 )
 
@@ -24,10 +27,7 @@ const secureServingOptionsMinTLSVersion = "VersionTLS12"
 const SecureTLSConfigMinTLSVersion = tls.VersionTLS12
 
 func init() {
-	go func() {
+	plog.Debug("using boring crypto in fips only mode", "go version", runtime.Version())
-		version := runtime.Version()
-		plog.Debug("using boringcrypto in fips only mode.", "go version", version)
-	}()
 }
 
 func Default(rootCAs *x509.CertPool) *tls.Config {
@@ -46,6 +46,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,
@@ -64,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)
+}

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 {

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
+}

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
 

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,131 +19,16 @@ import (
 	"go.pinniped.dev/test/testlib"
 )
 
-// This test mirrors securetls_test.go, but adapted for fips mode.
+// TestFIPSCipherSuites_Parallel ensures that if the list of default fips cipher suites changes,
-// e.g. checks for only TLS 1.2 ciphers and checks for the
+// we will know.  This is an integration test because we do not support build tags on unit tests.
-// 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)
-}
-
-// this test ensures that if the list of default fips cipher
-// suites changes, we will know.
 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.
-		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)
@@ -157,7 +38,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,
@@ -172,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...)
-	})
-}

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)

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

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 {
+const cipherSuitePreference = "server"
-	return "server"
-}

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 {
+const cipherSuitePreference = "client"
-	return "client"
-}