Merge pull request #3708 from Azure/refactor/pki

refactor package pki to be unit testeable

Dockerfile.ci-rp

@@ -56,7 +56,7 @@ RUN go build -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=${A
 RUN go test ./test/e2e/... -tags e2e,codec.safe -c -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=${ARO_VERSION}" -o e2e.test
 
 # Additional tests
-RUN ARO_SKIP_PKI_TESTS=true go run gotest.tools/[email protected] --format pkgname --junitfile report.xml -- -coverprofile=cover.out ./...
+RUN go run gotest.tools/[email protected] --format pkgname --junitfile report.xml -- -coverprofile=cover.out ./...
 RUN hack/fips/validate-fips.sh ./aro
 
 ###############################################################################

env.example

@@ -3,6 +3,4 @@ export ARO_IMAGE=arointsvc.azurecr.io/aro:latest
 export NO_CACHE=false
 export AZURE_EXTENSION_DEV_SOURCES="$(pwd)/python"
 
-export ARO_SKIP_PKI_TESTS=true
-
 . secrets/env

pkg/util/azureclient/applens/applens_client.go

@@ -7,7 +7,6 @@ package applens
 
 import (
 	"context"
-	"crypto/x509"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -60,19 +59,33 @@ func NewClient(endpoint, issuerUrlTemplate, caName, scope string, cred azcore.To
 	return &Client{endpoint: endpoint, pipeline: *pipeline}, nil
 }
 
-func newPipeline(authPolicy []policy.Policy, options *ClientOptions, issuerUrlTemplate, caName string) (*runtime.Pipeline, error) {
-	var cp *x509.CertPool = nil
-	var err error = nil
-	if options == nil {
-		// if provided pki info fetch the correct cert pool
-		// otherwise use the default of nil
-		if issuerUrlTemplate != "" && caName != "" {
-			cp, err = pki.GetTlsCertPool(issuerUrlTemplate, caName)
-			if err != nil {
-				return nil, err
-			}
-		}
-		options = NewClientOptions(cp)
+func getClientOptions(initialOptions *ClientOptions, issuerUrlTemplate, caName string) (*ClientOptions, error) {
+	if initialOptions != nil {
+		return initialOptions, nil
+	}
+
+	if issuerUrlTemplate == "" || caName == "" {
+		return NewClientOptions(nil), nil
+	}
+
+	url := fmt.Sprintf(issuerUrlTemplate, caName)
+	rootCAs, err := pki.FetchDataFromGetIssuerPki(url)
+	if err != nil {
+		return nil, err
+	}
+
+	certPool, err := pki.BuildCertPoolForCaName(rootCAs)
+	if err != nil {
+		return nil, err
+	}
+
+	return NewClientOptions(certPool), nil
+}
+
+func newPipeline(authPolicy []policy.Policy, initialClientOptions *ClientOptions, issuerUrlTemplate, caName string) (*runtime.Pipeline, error) {
+	options, err := getClientOptions(initialClientOptions, issuerUrlTemplate, caName)
+	if err != nil {
+		return nil, err
 	}
 
 	runtimePipeline := runtime.NewPipeline(

pkg/util/pki/fetchdata.go

@@ -0,0 +1,32 @@
+package pki
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"encoding/json"
+	"io"
+	"net/http"
+)
+
+// https://aka.ms/getissuers
+// The v3 endpoint can be used to get ca certs
+// For example https://issuer.pki.azure.com/dsms/issuercertificates?getissuersv3&caName=ame
+// returns the ame certs
+func FetchDataFromGetIssuerPki(url string) (*RootCAs, error) {
+	response, err := http.Get(url)
+	if err != nil {
+		return nil, err
+	}
+
+	defer response.Body.Close()
+
+	body, err := io.ReadAll(response.Body)
+	if err != nil {
+		return nil, err
+	}
+
+	var rootCAs RootCAs
+	json.Unmarshal(body, &rootCAs)
+	return &rootCAs, nil
+}

pkg/util/pki/pki.go

@@ -5,17 +5,9 @@ package pki
 
 import (
 	"crypto/x509"
-	"encoding/json"
 	"errors"
-	"fmt"
-	"io"
-	"net/http"
-	"sync"
 )
 
-var caMap map[string]x509.CertPool = make(map[string]x509.CertPool)
-var mu sync.RWMutex
-
 type RootCAs struct {
 	RootsInfos []RootInfo `json:"RootsInfos"`
 }
@@ -42,76 +34,8 @@ type IntermediateInfo struct {
 	PEM              string `json:"PEM"`
 }
 
-// https://aka.ms/getissuers
-// The v3 endpoint can be used to get ca certs
-// For example https://issuer.pki.azure.com/dsms/issuercertificates?getissuersv3&caName=ame
-// returns the ame certs
-func FetchDataFromGetIssuerPki(url string) (*RootCAs, error) {
-	response, err := http.Get(url)
-
-	if err != nil {
-		return nil, err
-	}
-
-	defer response.Body.Close()
-
-	if err != nil {
-		return nil, err
-	}
-
-	// Read in certs from endpoint
-	body, err := io.ReadAll(response.Body)
-
-	if err != nil {
-		return nil, err
-	}
-
-	var rootCAs RootCAs
-	json.Unmarshal(body, &rootCAs)
-	return &rootCAs, nil
-}
-
-func GetTlsCertPool(urlTemplate, caName string) (*x509.CertPool, error) {
-	url := fmt.Sprintf(urlTemplate, caName)
-	caCertPool, ok := getCaCertPoolFromMap(url)
-	if ok {
-		return &caCertPool, nil
-	} else {
-		caCertPool, err := buildCertPoolForCaName(url)
-
-		if err != nil || caCertPool == nil {
-			return nil, err
-		}
-
-		setCaCertPoolInMap(url, *caCertPool)
-
-		return caCertPool, nil
-	}
-}
-
-func getCaCertPoolFromMap(key string) (x509.CertPool, bool) {
-	mu.RLock()
-	defer mu.RUnlock()
-	caCertPool, ok := caMap[key]
-	return caCertPool, ok
-}
-
-func setCaCertPoolInMap(key string, caCertPool x509.CertPool) {
-	mu.Lock()
-	defer mu.Unlock()
-	caMap[key] = caCertPool
-}
-
-func buildCertPoolForCaName(url string) (*x509.CertPool, error) {
-	data, err := FetchDataFromGetIssuerPki(url)
-
-	if err != nil {
-		return nil, err
-	}
-
-	// Create a CertPool
+func BuildCertPoolForCaName(data *RootCAs) (*x509.CertPool, error) {
 	caCertPool, err := x509.SystemCertPool()
-
 	if err != nil {
 		return nil, err
 	}