From 8458cdb3bb79a65fcecf96600f66d3ec4ba75cc4 Mon Sep 17 00:00:00 2001 From: Ajay Gupta Date: Fri, 28 Jul 2023 17:37:08 -0400 Subject: [PATCH 1/6] RolesAnywhere-4667: Skip over certificates that can't be parsed when iterating over certificate stores --- aws_signing_helper/darwin_cert_store_signer.go | 11 ++++++++--- aws_signing_helper/windows_cert_store_signer.go | 8 ++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/aws_signing_helper/darwin_cert_store_signer.go b/aws_signing_helper/darwin_cert_store_signer.go index 7c9641d..a5288b1 100644 --- a/aws_signing_helper/darwin_cert_store_signer.go +++ b/aws_signing_helper/darwin_cert_store_signer.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "io" + "log" "os" "unsafe" ) @@ -73,6 +74,7 @@ func GetMatchingCertsAndIdentity(certIdentifier CertIdentifier) (C.SecIdentityRe var certContainers []CertificateContainer var certRef C.SecCertificateRef var identRef C.SecIdentityRef + var isMatch bool for _, curIdentRef := range identRefs { curCertRef, err := getCertRef(C.SecIdentityRef(curIdentRef)) if err != nil { @@ -80,12 +82,13 @@ func GetMatchingCertsAndIdentity(certIdentifier CertIdentifier) (C.SecIdentityRe } curCert, err := getCert(curCertRef) if err != nil { - return 0, 0, nil, errors.New("unable to get cert") + log.Println("unable to parse certificate - skipping") + goto nextIteration } // Find whether there is a matching certificate - certMatches := certMatches(certIdentifier, *curCert) - if certMatches { + isMatch = certMatches(certIdentifier, *curCert) + if isMatch { certContainers = append(certContainers, CertificateContainer{curCert, ""}) // Assign to certRef and identRef at most once in the loop // Both values are only useful if there is exactly one match in the certificate store @@ -95,6 +98,8 @@ func GetMatchingCertsAndIdentity(certIdentifier CertIdentifier) (C.SecIdentityRe identRef = C.SecIdentityRef(curIdentRef) } } + + nextIteration: } if Debug { diff --git a/aws_signing_helper/windows_cert_store_signer.go b/aws_signing_helper/windows_cert_store_signer.go index 6fb24ee..d083d09 100644 --- a/aws_signing_helper/windows_cert_store_signer.go +++ b/aws_signing_helper/windows_cert_store_signer.go @@ -122,6 +122,7 @@ func GetMatchingCertsAndChain(certIdentifier CertIdentifier) (store windows.Hand paramsPtr = unsafe.Pointer(¶ms) var curCertCtx *windows.CertContext + var curCert *x509.Certificate for { // Previous chainCtx should be freed here if it isn't nil chainCtx, err = windows.CertFindChainInStore(store, encoding, flags, findType, paramsPtr, chainCtx) @@ -155,11 +156,12 @@ func GetMatchingCertsAndChain(certIdentifier CertIdentifier) (store windows.Hand curCertCtx = chainElts[j].CertContext x509CertChain[j], err = exportCertContext(curCertCtx) if err != nil { - goto fail + log.Println("unable to parse certificate - skipping") + goto nextIteration } } - curCert := x509CertChain[0] + curCert = x509CertChain[0] if certMatches(certIdentifier, *curCert) { certContainers = append(certContainers, CertificateContainer{curCert, ""}) @@ -175,6 +177,8 @@ func GetMatchingCertsAndChain(certIdentifier CertIdentifier) (store windows.Hand windows.CertDuplicateCertificateContext(certCtx) } } + + nextIteration: } if Debug { From 365d1960e78f6378247a684bd75de7a61908e355 Mon Sep 17 00:00:00 2001 From: Ajay Gupta Date: Fri, 28 Jul 2023 17:43:17 -0400 Subject: [PATCH 2/6] Import log in windows_cert_store_signer.go --- aws_signing_helper/windows_cert_store_signer.go | 1 + 1 file changed, 1 insertion(+) diff --git a/aws_signing_helper/windows_cert_store_signer.go b/aws_signing_helper/windows_cert_store_signer.go index d083d09..b904ea6 100644 --- a/aws_signing_helper/windows_cert_store_signer.go +++ b/aws_signing_helper/windows_cert_store_signer.go @@ -40,6 +40,7 @@ import ( "fmt" "golang.org/x/sys/windows" "io" + "log" "os" "strconv" "strings" From 23ca8259941cb226c3075ed8f1f0dfc0c5abed5f Mon Sep 17 00:00:00 2001 From: Ajay Gupta Date: Sat, 29 Jul 2023 14:23:13 -0400 Subject: [PATCH 3/6] Modify logging in certificate store integrations Log to stderr as opposed to stdout when certificates can't be parsed and are skipped Only log if the debug option is specified --- aws_signing_helper/darwin_cert_store_signer.go | 5 +++-- aws_signing_helper/windows_cert_store_signer.go | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/aws_signing_helper/darwin_cert_store_signer.go b/aws_signing_helper/darwin_cert_store_signer.go index a5288b1..1b16e46 100644 --- a/aws_signing_helper/darwin_cert_store_signer.go +++ b/aws_signing_helper/darwin_cert_store_signer.go @@ -22,7 +22,6 @@ import ( "errors" "fmt" "io" - "log" "os" "unsafe" ) @@ -82,7 +81,9 @@ func GetMatchingCertsAndIdentity(certIdentifier CertIdentifier) (C.SecIdentityRe } curCert, err := getCert(curCertRef) if err != nil { - log.Println("unable to parse certificate - skipping") + if Debug { + fmt.Fprintf(os.Stderr, "unable to parse certificate - skipping") + } goto nextIteration } diff --git a/aws_signing_helper/windows_cert_store_signer.go b/aws_signing_helper/windows_cert_store_signer.go index b904ea6..3feba7f 100644 --- a/aws_signing_helper/windows_cert_store_signer.go +++ b/aws_signing_helper/windows_cert_store_signer.go @@ -40,7 +40,6 @@ import ( "fmt" "golang.org/x/sys/windows" "io" - "log" "os" "strconv" "strings" @@ -157,7 +156,9 @@ func GetMatchingCertsAndChain(certIdentifier CertIdentifier) (store windows.Hand curCertCtx = chainElts[j].CertContext x509CertChain[j], err = exportCertContext(curCertCtx) if err != nil { - log.Println("unable to parse certificate - skipping") + if Debug { + fmt.Fprintf(os.Stderr, "unable to parse certificate - skipping") + } goto nextIteration } } From 3c838d89259a6cf2a22be81b4544b8b2bec6acfc Mon Sep 17 00:00:00 2001 From: Ajay Gupta Date: Mon, 31 Jul 2023 10:32:03 -0400 Subject: [PATCH 4/6] Add newlines to the ends of the newly added debugging print statements --- aws_signing_helper/darwin_cert_store_signer.go | 2 +- aws_signing_helper/windows_cert_store_signer.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/aws_signing_helper/darwin_cert_store_signer.go b/aws_signing_helper/darwin_cert_store_signer.go index 1b16e46..fc80d49 100644 --- a/aws_signing_helper/darwin_cert_store_signer.go +++ b/aws_signing_helper/darwin_cert_store_signer.go @@ -82,7 +82,7 @@ func GetMatchingCertsAndIdentity(certIdentifier CertIdentifier) (C.SecIdentityRe curCert, err := getCert(curCertRef) if err != nil { if Debug { - fmt.Fprintf(os.Stderr, "unable to parse certificate - skipping") + fmt.Fprintf(os.Stderr, "unable to parse certificate - skipping\n") } goto nextIteration } diff --git a/aws_signing_helper/windows_cert_store_signer.go b/aws_signing_helper/windows_cert_store_signer.go index 3feba7f..345f2f4 100644 --- a/aws_signing_helper/windows_cert_store_signer.go +++ b/aws_signing_helper/windows_cert_store_signer.go @@ -157,7 +157,7 @@ func GetMatchingCertsAndChain(certIdentifier CertIdentifier) (store windows.Hand x509CertChain[j], err = exportCertContext(curCertCtx) if err != nil { if Debug { - fmt.Fprintf(os.Stderr, "unable to parse certificate - skipping") + fmt.Fprintf(os.Stderr, "unable to parse certificate - skipping\n") } goto nextIteration } From 732874229ab5df7571d613d4097b0e0b471e639f Mon Sep 17 00:00:00 2001 From: Ajay Gupta Date: Fri, 4 Aug 2023 10:27:49 -0400 Subject: [PATCH 5/6] Improve logging when debug flag is specified --- aws_signing_helper/darwin_cert_store_signer.go | 2 +- aws_signing_helper/windows_cert_store_signer.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/aws_signing_helper/darwin_cert_store_signer.go b/aws_signing_helper/darwin_cert_store_signer.go index fc80d49..4b7e853 100644 --- a/aws_signing_helper/darwin_cert_store_signer.go +++ b/aws_signing_helper/darwin_cert_store_signer.go @@ -82,7 +82,7 @@ func GetMatchingCertsAndIdentity(certIdentifier CertIdentifier) (C.SecIdentityRe curCert, err := getCert(curCertRef) if err != nil { if Debug { - fmt.Fprintf(os.Stderr, "unable to parse certificate - skipping\n") + fmt.Fprintf(os.Stderr, "unable to parse certificate with error (%s) - skipping\n", err) } goto nextIteration } diff --git a/aws_signing_helper/windows_cert_store_signer.go b/aws_signing_helper/windows_cert_store_signer.go index 345f2f4..b0c5850 100644 --- a/aws_signing_helper/windows_cert_store_signer.go +++ b/aws_signing_helper/windows_cert_store_signer.go @@ -157,7 +157,7 @@ func GetMatchingCertsAndChain(certIdentifier CertIdentifier) (store windows.Hand x509CertChain[j], err = exportCertContext(curCertCtx) if err != nil { if Debug { - fmt.Fprintf(os.Stderr, "unable to parse certificate - skipping\n") + fmt.Fprintf(os.Stderr, "unable to parse certificate with error (%s) - skipping\n", err) } goto nextIteration } From 485eea3dceb7402341aa88592ac3ac6c256e450c Mon Sep 17 00:00:00 2001 From: Ajay Gupta Date: Fri, 4 Aug 2023 14:13:32 -0400 Subject: [PATCH 6/6] Remove function that obfuscates the actual error in darwin_cert_store_signer.go --- aws_signing_helper/darwin_cert_store_signer.go | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/aws_signing_helper/darwin_cert_store_signer.go b/aws_signing_helper/darwin_cert_store_signer.go index 4b7e853..a83735c 100644 --- a/aws_signing_helper/darwin_cert_store_signer.go +++ b/aws_signing_helper/darwin_cert_store_signer.go @@ -79,7 +79,7 @@ func GetMatchingCertsAndIdentity(certIdentifier CertIdentifier) (C.SecIdentityRe if err != nil { return 0, 0, nil, errors.New("unable to get cert ref") } - curCert, err := getCert(curCertRef) + curCert, err := exportCertRef(curCertRef) if err != nil { if Debug { fmt.Fprintf(os.Stderr, "unable to parse certificate with error (%s) - skipping\n", err) @@ -159,16 +159,6 @@ func GetCertStoreSigner(certIdentifier CertIdentifier) (signer Signer, signingAl return &DarwinCertStoreSigner{identRef, keyRef, certRef, cert, nil}, signingAlgorithm, nil } -// Gets a pointer to the certificate from a certificate reference -func getCert(certRef C.SecCertificateRef) (*x509.Certificate, error) { - cert, err := exportCertRef(certRef) - if err != nil { - return nil, errors.New("unable to export certificate reference to x509.Certificate") - } - - return cert, nil -} - // Gets the certificate associated with this DarwinCertStoreSigner func (signer *DarwinCertStoreSigner) Certificate() (*x509.Certificate, error) { if signer.cert != nil { @@ -180,7 +170,7 @@ func (signer *DarwinCertStoreSigner) Certificate() (*x509.Certificate, error) { return nil, err } - cert, err := getCert(certRef) + cert, err := exportCertRef(certRef) if err != nil { return nil, err }