From bb8f8bdbca25047997ab333000c0d7279d4b3421 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Mon, 30 Mar 2026 14:10:49 -0700 Subject: [PATCH] Replace util/certutil with azidentity.ParseCertificates (#4) Address review feedback: use azidentity.ParseCertificates instead of the custom util/certutil package for certificate parsing. The azidentity package already handles PEM and PKCS#12 formats natively. - Remove util/certutil package entirely - Use azidentity.ParseCertificates in newCertificateTokenCredential - Revert golang.org/x/crypto to indirect dependency Agent-Logs-Url: https://github.com/bragi92/prometheus/sessions/efc3d6c5-9927-4d5b-8aa9-afe94b659c6e Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: bragi92 <28612268+bragi92@users.noreply.github.com> --- go.mod | 2 +- storage/remote/azuread/azuread.go | 36 +++--- util/certutil/certutil.go | 204 ------------------------------ util/certutil/certutil_test.go | 55 -------- 4 files changed, 22 insertions(+), 275 deletions(-) delete mode 100644 util/certutil/certutil.go delete mode 100644 util/certutil/certutil_test.go diff --git a/go.mod b/go.mod index 96c97a9a47..89d468e874 100644 --- a/go.mod +++ b/go.mod @@ -239,7 +239,7 @@ require ( go.opentelemetry.io/collector/pipeline v1.51.0 // indirect go.opentelemetry.io/proto/otlp v1.9.0 // indirect go.uber.org/zap v1.27.1 // indirect - golang.org/x/crypto v0.47.0 + golang.org/x/crypto v0.47.0 // indirect golang.org/x/exp v0.0.0-20260112195511-716be5621a96 // indirect golang.org/x/mod v0.32.0 // indirect golang.org/x/net v0.49.0 // indirect diff --git a/storage/remote/azuread/azuread.go b/storage/remote/azuread/azuread.go index 15267486ef..ff8ff6c50f 100644 --- a/storage/remote/azuread/azuread.go +++ b/storage/remote/azuread/azuread.go @@ -17,6 +17,7 @@ import ( "context" "errors" "net/http" + "os" "strings" "sync" "time" @@ -27,8 +28,6 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/google/uuid" "github.com/grafana/regexp" - - "github.com/prometheus/prometheus/util/certutil" ) // Clouds. @@ -434,21 +433,28 @@ func newSDKTokenCredential(clientOpts *azcore.ClientOptions, sdkConfig *SDKConfi // newCertificateTokenCredential returns new certificate-based token credential. func newCertificateTokenCredential(clientOpts *azcore.ClientOptions, certConfig *CertificateConfig) (azcore.TokenCredential, error) { - // Use certutil to parse the certificate files - certData, err := certutil.ParseCertificateFiles( - certConfig.CertificatePath, - certConfig.CertificateKeyPath, - certConfig.CertificatePassword, - ) + certData, err := os.ReadFile(certConfig.CertificatePath) if err != nil { - return nil, err + return nil, errors.New("failed to read certificate file " + certConfig.CertificatePath + ": " + err.Error()) } - if len(certData.Certificates) == 0 { - return nil, errors.New("no certificates found in certificate file") + // If a separate key file is provided, append it to the cert data so ParseCertificates can find the private key. + if certConfig.CertificateKeyPath != "" { + keyData, err := os.ReadFile(certConfig.CertificateKeyPath) + if err != nil { + return nil, errors.New("failed to read private key file " + certConfig.CertificateKeyPath + ": " + err.Error()) + } + certData = append(append(certData, '\n'), keyData...) } - if certData.PrivateKey == nil { - return nil, errors.New("no private key found") + + var password []byte + if certConfig.CertificatePassword != "" { + password = []byte(certConfig.CertificatePassword) + } + + certs, key, err := azidentity.ParseCertificates(certData, password) + if err != nil { + return nil, errors.New("failed to parse certificate data: " + err.Error()) } opts := &azidentity.ClientCertificateCredentialOptions{ @@ -459,8 +465,8 @@ func newCertificateTokenCredential(clientOpts *azcore.ClientOptions, certConfig return azidentity.NewClientCertificateCredential( certConfig.TenantID, certConfig.ClientID, - certData.Certificates, - certData.PrivateKey, + certs, + key, opts, ) } diff --git a/util/certutil/certutil.go b/util/certutil/certutil.go deleted file mode 100644 index e428ff4424..0000000000 --- a/util/certutil/certutil.go +++ /dev/null @@ -1,204 +0,0 @@ -// Copyright The Prometheus Authors -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package certutil provides utilities for loading and parsing X.509 certificates -// and private keys from various formats (PEM, PKCS12/PFX). -package certutil - -import ( - "crypto/x509" - "encoding/pem" - "errors" - "os" - - "golang.org/x/crypto/pkcs12" -) - -// CertificateData represents parsed certificate data including certificates and private key. -type CertificateData struct { - // Certificates is a slice of X.509 certificates (leaf certificate first, followed by any intermediates) - Certificates []*x509.Certificate - // PrivateKey is the private key associated with the leaf certificate - PrivateKey any -} - -// ParseCertificateFiles loads and parses certificate(s) and private key from files. -// It supports both PEM and PKCS12/PFX formats. -// -// Parameters: -// - certPath: Path to the certificate file (required) -// - keyPath: Path to a separate private key file (optional, only used for PEM format) -// - password: Password for PKCS12/PFX files (optional, only used for PKCS12 format) -// -// Returns the parsed certificate data or an error. -func ParseCertificateFiles(certPath, keyPath, password string) (*CertificateData, error) { - // Read certificate file - certData, err := os.ReadFile(certPath) - if err != nil { - return nil, errors.New("failed to read certificate file " + certPath + ": " + err.Error()) - } - - // Detect format: check if PEM format first - if isPEMFormat(certData) { - return parsePEMData(certData, keyPath, certPath) - } - - // Must be PKCS12/PFX format - return parsePKCS12Data(certData, password, certPath) -} - -// isPEMFormat checks if data is in PEM format. -func isPEMFormat(data []byte) bool { - block, _ := pem.Decode(data) - return block != nil -} - -// parsePEMData parses PEM-encoded certificate and private key data. -func parsePEMData(certData []byte, keyPath, certPath string) (*CertificateData, error) { - var certs []*x509.Certificate - var privateKey any - - // Parse certificates and keys from certificate file - rest := certData - for { - var block *pem.Block - block, rest = pem.Decode(rest) - if block == nil { - break - } - - switch block.Type { - case "CERTIFICATE": - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse certificate from " + certPath + ": " + err.Error()) - } - certs = append(certs, cert) - case "PRIVATE KEY": - if privateKey == nil { // Only take the first private key - var err error - privateKey, err = x509.ParsePKCS8PrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse PKCS8 private key from " + certPath + ": " + err.Error()) - } - } - case "RSA PRIVATE KEY": - if privateKey == nil { // Only take the first private key - var err error - privateKey, err = x509.ParsePKCS1PrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse RSA private key from " + certPath + ": " + err.Error()) - } - } - case "EC PRIVATE KEY": - if privateKey == nil { // Only take the first private key - var err error - privateKey, err = x509.ParseECPrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse EC private key from " + certPath + ": " + err.Error()) - } - } - } - } - - // If no private key found in main file and separate key file is provided, read it - if privateKey == nil && keyPath != "" { - keyData, err := os.ReadFile(keyPath) - if err != nil { - return nil, errors.New("failed to read private key file " + keyPath + ": " + err.Error()) - } - - block, _ := pem.Decode(keyData) - if block == nil { - return nil, errors.New("failed to decode PEM private key from " + keyPath) - } - - switch block.Type { - case "PRIVATE KEY": - privateKey, err = x509.ParsePKCS8PrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse PKCS8 private key from " + keyPath + ": " + err.Error()) - } - case "RSA PRIVATE KEY": - privateKey, err = x509.ParsePKCS1PrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse RSA private key from " + keyPath + ": " + err.Error()) - } - case "EC PRIVATE KEY": - privateKey, err = x509.ParseECPrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse EC private key from " + keyPath + ": " + err.Error()) - } - default: - return nil, errors.New("unsupported private key type in " + keyPath + ": " + block.Type) - } - } - - return &CertificateData{ - Certificates: certs, - PrivateKey: privateKey, - }, nil -} - -// parsePKCS12Data parses PKCS12/PFX-encoded certificate and private key data. -func parsePKCS12Data(certData []byte, password, certPath string) (*CertificateData, error) { - // Convert PKCS12 to PEM blocks - pemBlocks, err := pkcs12.ToPEM(certData, password) - if err != nil { - return nil, errors.New("failed to parse PFX certificate " + certPath + ": " + err.Error()) - } - - var certs []*x509.Certificate - var privateKey any - - // Parse PEM blocks to extract certificates and private key - for _, block := range pemBlocks { - switch block.Type { - case "CERTIFICATE": - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse certificate from PFX " + certPath + ": " + err.Error()) - } - certs = append(certs, cert) - case "PRIVATE KEY": - // PKCS8 private key - if privateKey == nil { // Only take the first private key - privateKey, err = x509.ParsePKCS8PrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse PKCS8 private key from PFX " + certPath + ": " + err.Error()) - } - } - case "RSA PRIVATE KEY": - // PKCS1 RSA private key - if privateKey == nil { // Only take the first private key - privateKey, err = x509.ParsePKCS1PrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse RSA private key from PFX " + certPath + ": " + err.Error()) - } - } - case "EC PRIVATE KEY": - // EC private key - if privateKey == nil { // Only take the first private key - privateKey, err = x509.ParseECPrivateKey(block.Bytes) - if err != nil { - return nil, errors.New("failed to parse EC private key from PFX " + certPath + ": " + err.Error()) - } - } - } - } - - return &CertificateData{ - Certificates: certs, - PrivateKey: privateKey, - }, nil -} diff --git a/util/certutil/certutil_test.go b/util/certutil/certutil_test.go deleted file mode 100644 index 59c7da465b..0000000000 --- a/util/certutil/certutil_test.go +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright The Prometheus Authors -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package certutil - -import ( - "testing" -) - -// TestIsPEMFormat tests the PEM format detection. -func TestIsPEMFormat(t *testing.T) { - tests := []struct { - name string - data []byte - expected bool - }{ - { - name: "valid PEM", - data: []byte(`-----BEGIN CERTIFICATE----- -MIIBkTCB+wIJAKHHCgVZU6pfMA0GCSqGSIb3DQEBCwUAMA0xCzAJBgNVBAYTAlVT -MB4XDTE5MDEwMTAwMDAwMFoXDTIwMDEwMTAwMDAwMFowDTELMAkGA1UEBhMCVVMw ------END CERTIFICATE-----`), - expected: true, - }, - { - name: "not PEM", - data: []byte("random binary data"), - expected: false, - }, - { - name: "empty", - data: []byte(""), - expected: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := isPEMFormat(tt.data) - if result != tt.expected { - t.Errorf("isPEMFormat() = %v, want %v", result, tt.expected) - } - }) - } -}