Add more details to FIPS comments

Signed-off-by: Monis Khan <mok@vmware.com>

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)
-# 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/... && \

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,

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,

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