Implement DecodeChains function that accepts multiple key bags

[marsskop]

Discussed in https://github.com/SSLMate/go-pkcs12/discussions/61

According to the PKCS#12 v1.1. standard, a PKCS#12 file may have multiple key bags.

Use case: an application uses a PKCS#12 keystore to connect to two other apps in the cluster, one asks for one key, another for another (there is mTLS). During the deployment of an application, keystores are meant to be read and generated.

API proposal (by @AGWA):

type Chain struct {
        FriendlyName string
        PrivateKey   crypto.PrivateKey
        Leaf         *x509.Certificate
        CACerts      []*x509.Certificate
}

func DecodeChains(pfxData []byte, password string) ([]Chain, error)

Details:

• FriendlyName attribute is extracted from attributes of each bag via convertAttributes
• for every private key, find the corresponding certificate, and then build a chain by recursively finding the issuer; FriendlyNames should match; build a chain via x509.Certificate.Verify(opts) perhaps?

This resolves https://github.com/SSLMate/go-pkcs12/issues/54, and the use case of https://github.com/SSLMate/go-pkcs12/issues/49.

[marsskop]

After taking a look at the code, I have some stuff to discuss.

• current implementation of convertAttribute errors upon attributes with unknown OIDs. I am not sure this is a good decision in the current implementation when OID is compared to a short list of OIDs. Additionally, this function errors on the test data in TestPBES2_AES256CBC on an attribute localMachineKeySet with OID 1.3.6.1.4.1.311.17.2 (attribute definition retrieved via https://lapo.it/asn1js/).
• current implementation of Decode uses DecodeChain to get the private key and the certificate. I assume they should have the same friendlyName attribute (be part of the same entry). But in the "Windows Azure Tools" test data in TestPfx private key has friendlyName "{B4A4FEB0-A18A-44BB-B5F2-491EF152BA16}" and cert has friendlyName "Paul's Playground-10-2-2014-credentials" that do not match; is this considered a malformed chain, and thus Decode should error?

[AGWA]

DecodeChains should not use convertAttribute, which is a helper function for the deprecated ToPEM. Instead let's just write a new function that extracts only the friendly name and ignores all other attributes.

Decode and DecodeChain don't currently care about friendly name at all, and this should not change.

We can't use x509.Certificate.Verify because we don't know what root the chain should end at. To test if a certificate issued another, the following should suffice:

func issuedBy(subject, issuer *x509.Certificate) bool {
        return bytes.Equal(subject.RawIssuer, issuer.RawSubject) && issuer.CheckSignature(subject.SignatureAlgorithm, subject.RawTBSCertificate, subject.Signature) == nil 
}

If more than one certificate matches, we should probably add them both to CACerts and recur on both. This will work well with TLS 1.3, where the certificate list is just an unordered collection of certs which help the peer build a chain.

It's an open question whether friendly names should match when picking certificates. This doesn't seem to be specified anywhere, and we've got several examples of PKCS#12 files where the friendly names do not match.

[marsskop]

So the FriendlyName in the Chain is the friendly name of the private key, and thus friendly names of the certificates are to be discarded?

Indeed, in "Windows Azure Tools" test data private key matches the certificate but friendly names do not match (and for some certificates, e.g. created and imported via openssl and keytool, friendly name is outright subject DN in the keystore).

[AGWA]

So the FriendlyName in the Chain is the friendly name of the private key, and thus friendly names of the certificates are to be discarded?

Yeah, I think this is the best option.

[nitram509]

Hi @AGWA @marsskop ,

I'm working on another implementation, depending on this great go-pkcs2 lib FYI: I refer to https://github.com/pavlo-v-chernykh/keystore-go/pull/55

About the proposed #63 PR ... may I share my concerns (me using the library) and kindly asking for your perspective?

I did prepare an example key chain using Keystore Explorer tool (see image below)

Using the code from the #63 PR, I implemented some simple tests (shown below). As a user of go-pkcs12, I want to consume any p12 file, without knowing upfront (or guessing) if a chain is included in the file. Or in other words, I do expect a single functions like LoadStore(), which takes care of the internals. My concern is that the three (DecodeChain, DecodeChains, DecodeTrustStore) functions are confusing for the users/developers of the lib. What's your view?

Screenshot, example signed certificate chains

Go test code, demoing DecodeChains and DecodeTrustStore

package pkcs12

import (
	"fmt"
	"testing"
)
import _ "embed"

//go:embed example_signed_certificates_chain.p12
var exampleSignedCertificatesChain []byte

func Test_DecodeChains(t *testing.T) {
	certs, err := DecodeChains(exampleSignedCertificatesChain, "password")
	if err != nil { t.Fatal(err) } // no error, all fine
	println(fmt.Sprintf("%v", certs)) // prints a list of three certificates, as expected
}

func Test_DecodeTrustStore(t *testing.T) {
	certs, err := DecodeTrustStore(exampleSignedCertificatesChain, "password")
	if err != nil { t.Fatal(err) } // raises error: pkcs12: expected exactly 1 items in the authenticated safe, but this file has 2
	println(fmt.Sprintf("%v", certs))
}

This is the example file, encoded as base64 file (you need to decode before using it)

example_signed_certificates_chain.p12.base64.txt

PS: @marsskop may I propose to add these test cases, and maybe some better assertions to your PR as well?

[nitram509]

Hi @AGWA @marsskop, I further tested #63 and found a bug when decoding chains, there were duplicate ca certificates in the chain. I created another PR #65 and fixed the bug. May I ask for some review / comments or feedback please?

[marsskop]

Hi @nitram509

• the rework of static tests in https://github.com/SSLMate/go-pkcs12/pull/65/commits/32f4b4b94001ab544839454b89767d035db32684 looks good!
• the logic of the reworked DecodeChains function looks good, too; you got rid of the recursion which was the source of duplicate certificates. I do not see any immediate issues.
• but the tests, alas, do not pass and break on Test_DecodeChains_with_private_key:

$ go test                
--- FAIL: Test_DecodeChains_with_private_key (0.03s)
    --- FAIL: Test_DecodeChains_with_private_key/example_signed_certificates_chain.p12 (0.03s)
        pkcs12_dec_test.go:108: unexpected private key friendly name, got 'example-server (example-intermediate-ca)', want 'example-ca'
        pkcs12_dec_test.go:119: unexpected # of caCerts: got 2, want 0
        pkcs12_dec_test.go:108: unexpected private key friendly name, got 'example-ca', want 'example-intermediate-ca (example-ca)'
        pkcs12_dec_test.go:119: unexpected # of caCerts: got 0, want 1
        pkcs12_dec_test.go:108: unexpected private key friendly name, got 'example-intermediate-ca (example-ca)', want 'example-server (example-intermediate-ca)'
        pkcs12_dec_test.go:119: unexpected # of caCerts: got 1, want 2
FAIL
exit status 1
FAIL    software.sslmate.com/src/go-pkcs12      0.634s

[nitram509]

Hi @marsskop , thank you for your feedback, and thanks for catching the broken test. The test failed, because the result slice of DecodeChains() was baked from a hashmap, which has unpredictable results ... hence I did add a sort.Slice() function, to make the result and thus the test stable.

Would you consider to merge the PR?

[marsskop]

Awesome, thanks @nitram509 ! Tests pass now.

$ go test                                                          
PASS
ok      software.sslmate.com/src/go-pkcs12      0.842s

I approve all changes. I will certainly merge this into my repo. (and probably close my PR to leave only #65 ?) @AGWA, can you please take a look, review and merge if all good? Thanks.