Merge pull request #703 from erikgb/refactor-additional-formats-2

Move additional formats handling from source to target

Author: cert-manager-prow[bot]

pkg/bundle/bundle.go

@@ -115,7 +115,7 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (statusP
 	statusPatch = &trustapi.BundleStatus{
 		DefaultCAPackageVersion: bundle.Status.DefaultCAPackageVersion,
 	}
-	resolvedBundle, err := b.bundleBuilder.BuildBundle(ctx, bundle.Spec.Sources, bundle.Spec.Target.AdditionalFormats)
+	resolvedBundle, err := b.bundleBuilder.BuildBundle(ctx, bundle.Spec.Sources)
 
 	if err != nil {
 		var reason, message string

pkg/bundle/internal/source/source.go

@@ -28,7 +28,6 @@ import (
 
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
 	"github.com/cert-manager/trust-manager/pkg/bundle/controller"
-	"github.com/cert-manager/trust-manager/pkg/bundle/internal/truststore"
 	"github.com/cert-manager/trust-manager/pkg/fspkg"
 	"github.com/cert-manager/trust-manager/pkg/util"
 )
@@ -43,38 +42,11 @@ type InvalidSecretError struct{ error }
 // certificate data from concatenating all the sources together, binary data for any additional formats and
 // any metadata from the sources which needs to be exposed on the Bundle resource's status field.
 type BundleData struct {
-	Data string
-
-	BinaryData map[string][]byte
+	CertPool *util.CertPool
 
 	DefaultCAPackageStringID string
 }
 
-func (b *BundleData) populate(pool *util.CertPool, formats *trustapi.AdditionalFormats) error {
-	b.Data = pool.PEM()
-
-	if formats != nil {
-		b.BinaryData = make(map[string][]byte)
-
-		if formats.JKS != nil {
-			encoded, err := truststore.NewJKSEncoder(*formats.JKS.Password).Encode(pool)
-			if err != nil {
-				return fmt.Errorf("failed to encode JKS: %w", err)
-			}
-			b.BinaryData[formats.JKS.Key] = encoded
-		}
-
-		if formats.PKCS12 != nil {
-			encoded, err := truststore.NewPKCS12Encoder(*formats.PKCS12.Password, formats.PKCS12.Profile).Encode(pool)
-			if err != nil {
-				return fmt.Errorf("failed to encode PKCS12: %w", err)
-			}
-			b.BinaryData[formats.PKCS12.Key] = encoded
-		}
-	}
-	return nil
-}
-
 type BundleBuilder struct {
 	client.Reader
 
@@ -87,9 +59,9 @@ type BundleBuilder struct {
 
 // BuildBundle retrieves and concatenates all source bundle data for this Bundle object.
 // Each source data is validated and pruned to ensure that all certificates within are valid.
-func (b *BundleBuilder) BuildBundle(ctx context.Context, sources []trustapi.BundleSource, formats *trustapi.AdditionalFormats) (BundleData, error) {
+func (b *BundleBuilder) BuildBundle(ctx context.Context, sources []trustapi.BundleSource) (BundleData, error) {
 	var resolvedBundle BundleData
-	certPool := util.NewCertPool(
+	resolvedBundle.CertPool = util.NewCertPool(
 		util.WithFilteredExpiredCerts(b.FilterExpiredCerts),
 		util.WithLogger(logf.FromContext(ctx).WithName("cert-pool")),
 	)
@@ -120,20 +92,16 @@ func (b *BundleBuilder) BuildBundle(ctx context.Context, sources []trustapi.Bund
 			panic(fmt.Sprintf("don't know how to process source: %+v", source))
 		}
 
-		if err := certSource.addToCertPool(ctx, certPool); err != nil {
+		if err := certSource.addToCertPool(ctx, resolvedBundle.CertPool); err != nil {
 			return BundleData{}, err
 		}
 	}
 
 	// NB: empty bundles are not valid, so check and return an error if one somehow snuck through.
-	if certPool.Size() == 0 {
+	if resolvedBundle.CertPool.Size() == 0 {
 		return BundleData{}, NotFoundError{fmt.Errorf("couldn't find any valid certificates in bundle")}
 	}
 
-	if err := resolvedBundle.populate(certPool, formats); err != nil {
-		return BundleData{}, err
-	}
-
 	return resolvedBundle, nil
 }
 

pkg/bundle/internal/source/source_test.go

@@ -17,20 +17,16 @@ limitations under the License.
 package source
 
 import (
-	"bytes"
-	"encoding/pem"
 	"errors"
 	"strings"
 	"testing"
 
-	jks "github.com/pavlo-v-chernykh/keystore-go/v4"
 	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"software.sslmate.com/src/go-pkcs12"
 
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
 	"github.com/cert-manager/trust-manager/pkg/bundle/controller"
@@ -40,26 +36,15 @@ import (
 	"github.com/cert-manager/trust-manager/test/dummy"
 )
 
-const (
-	jksKey    = "trust.jks"
-	pkcs12Key = "trust.p12"
-	data      = dummy.TestCertificate1
-)
-
 func Test_BuildBundle(t *testing.T) {
 	tests := map[string]struct {
 		sources                     []trustapi.BundleSource
 		filterExpired               bool
-		formats                     *trustapi.AdditionalFormats
 		objects                     []runtime.Object
 		expData                     string
 		expError                    bool
 		expNotFoundError            bool
 		expInvalidSecretSourceError bool
-		bool
-		expJKS      bool
-		expPKCS12   bool
-		expPassword *string
 	}{
 		"if no sources defined, should return NotFoundError": {
 			expError:         true,
@@ -333,85 +318,6 @@ func Test_BuildBundle(t *testing.T) {
 			expError:         false,
 			expNotFoundError: false,
 		},
-
-		"if has JKS target, return binaryData with encoded JKS": {
-			sources: []trustapi.BundleSource{
-				{ConfigMap: &trustapi.SourceObjectKeySelector{Name: "configmap", Key: "key"}},
-			},
-			formats: &trustapi.AdditionalFormats{
-				JKS: &trustapi.JKS{
-					KeySelector: trustapi.KeySelector{
-						Key: jksKey,
-					},
-					Password: ptr.To(trustapi.DefaultJKSPassword),
-				},
-			},
-			objects: []runtime.Object{&corev1.ConfigMap{
-				ObjectMeta: metav1.ObjectMeta{Name: "configmap"},
-				Data:       map[string]string{"key": dummy.TestCertificate1},
-			}},
-			expData: dummy.JoinCerts(dummy.TestCertificate1),
-			expJKS:  true,
-		},
-		"if has JKS target with arbitrary password, return binaryData with encoded JKS": {
-			sources: []trustapi.BundleSource{
-				{ConfigMap: &trustapi.SourceObjectKeySelector{Name: "configmap", Key: "key"}},
-			},
-			formats: &trustapi.AdditionalFormats{
-				JKS: &trustapi.JKS{
-					KeySelector: trustapi.KeySelector{
-						Key: jksKey,
-					},
-					Password: ptr.To("testPasswd123"),
-				},
-			},
-			objects: []runtime.Object{&corev1.ConfigMap{
-				ObjectMeta: metav1.ObjectMeta{Name: "configmap"},
-				Data:       map[string]string{"key": dummy.TestCertificate1},
-			}},
-			expData:     dummy.JoinCerts(dummy.TestCertificate1),
-			expJKS:      true,
-			expPassword: ptr.To("testPasswd123"),
-		},
-		"if has PKCS12 target, return binaryData with encoded PKCS12": {
-			sources: []trustapi.BundleSource{
-				{ConfigMap: &trustapi.SourceObjectKeySelector{Name: "configmap", Key: "key"}},
-			},
-			formats: &trustapi.AdditionalFormats{
-				PKCS12: &trustapi.PKCS12{
-					KeySelector: trustapi.KeySelector{
-						Key: pkcs12Key,
-					},
-					Password: ptr.To(trustapi.DefaultPKCS12Password),
-				},
-			},
-			objects: []runtime.Object{&corev1.ConfigMap{
-				ObjectMeta: metav1.ObjectMeta{Name: "configmap"},
-				Data:       map[string]string{"key": dummy.TestCertificate1},
-			}},
-			expData:   dummy.JoinCerts(dummy.TestCertificate1),
-			expPKCS12: true,
-		},
-		"if has PKCS12 target with arbitrary password, return binaryData with encoded PKCS12": {
-			sources: []trustapi.BundleSource{
-				{ConfigMap: &trustapi.SourceObjectKeySelector{Name: "configmap", Key: "key"}},
-			},
-			formats: &trustapi.AdditionalFormats{
-				PKCS12: &trustapi.PKCS12{
-					KeySelector: trustapi.KeySelector{
-						Key: pkcs12Key,
-					},
-					Password: ptr.To("testPasswd123"),
-				},
-			},
-			objects: []runtime.Object{&corev1.ConfigMap{
-				ObjectMeta: metav1.ObjectMeta{Name: "configmap"},
-				Data:       map[string]string{"key": dummy.TestCertificate1},
-			}},
-			expData:     dummy.JoinCerts(dummy.TestCertificate1),
-			expPKCS12:   true,
-			expPassword: ptr.To("testPasswd123"),
-		},
 	}
 
 	for name, tt := range tests {
@@ -433,24 +339,7 @@ func Test_BuildBundle(t *testing.T) {
 				Options: controller.Options{FilterExpiredCerts: tt.filterExpired},
 			}
 
-			// for corresponding store if arbitrary password is expected then set it instead of default one
-			var password string
-			if tt.expJKS {
-				if tt.expPassword != nil {
-					password = *tt.expPassword
-				} else {
-					password = trustapi.DefaultJKSPassword
-				}
-			}
-			if tt.expPKCS12 {
-				if tt.expPassword != nil {
-					password = *tt.expPassword
-				} else {
-					password = trustapi.DefaultPKCS12Password
-				}
-			}
-
-			resolvedBundle, err := b.BuildBundle(t.Context(), tt.sources, tt.formats)
+			resolvedBundle, err := b.BuildBundle(t.Context(), tt.sources)
 
 			if (err != nil) != tt.expError {
 				t.Errorf("unexpected error, exp=%t got=%v", tt.expError, err)
@@ -462,45 +351,9 @@ func Test_BuildBundle(t *testing.T) {
 				t.Errorf("unexpected InvalidSecretError, exp=%t got=%v", tt.expInvalidSecretSourceError, err)
 			}
 
-			if resolvedBundle.Data != tt.expData {
-				t.Errorf("unexpected data, exp=%q got=%q", tt.expData, resolvedBundle.Data)
-			}
-
-			binData, jksExists := resolvedBundle.BinaryData[jksKey]
-			assert.Equal(t, tt.expJKS, jksExists)
-
-			if tt.expJKS {
-				reader := bytes.NewReader(binData)
-
-				ks := jks.New()
-
-				err := ks.Load(reader, []byte(password))
-				assert.Nil(t, err)
-
-				entryNames := ks.Aliases()
-
-				assert.Len(t, entryNames, 1)
-				assert.True(t, ks.IsTrustedCertificateEntry(entryNames[0]))
-
-				// Safe to ignore errors here, we've tested that it's present and a TrustedCertificateEntry
-				cert, _ := ks.GetTrustedCertificateEntry(entryNames[0])
-
-				// Only one certificate block for this test, so we can safely ignore the `remaining` byte array
-				p, _ := pem.Decode([]byte(data))
-				assert.Equal(t, p.Bytes, cert.Certificate.Content)
-			}
-
-			binData, pkcs12Exists := resolvedBundle.BinaryData[pkcs12Key]
-			assert.Equal(t, tt.expPKCS12, pkcs12Exists)
-
-			if tt.expPKCS12 {
-				cas, err := pkcs12.DecodeTrustStore(binData, password)
-				assert.Nil(t, err)
-				assert.Len(t, cas, 1)
-
-				// Only one certificate block for this test, so we can safely ignore the `remaining` byte array
-				p, _ := pem.Decode([]byte(data))
-				assert.Equal(t, p.Bytes, cas[0].Raw)
+			data := resolvedBundle.CertPool.PEM()
+			if data != tt.expData {
+				t.Errorf("unexpected data, exp=%q got=%q", tt.expData, data)
 			}
 		})
 	}