x/crypto/pkcs12: SSL certificate decode validation error

[tipycalFlow]

This is in continuation with #13855

I'm trying to use the x/crypto/pkcs12 package's Decode method to read this p12 file (public-certificate + private-key pair) with password googler to further generate a TLS certificate but get the following error:
pkcs12: expected exactly two safe bags in the PFX PDU

Here's how I'm reading the certificate file: p12, err := ioutil.ReadFile("filename")
and I'm passing the read binary data to Decode(p12, "password")

I logged the number of safe bags returned from getSafeContents before line 219: if len(bags) != 2 { and it returned 4, which is why Decode is failing. Now I'm using go1.6beta1 darwin/amd64 on OS X Yosemite version 10.10.5 (14F1509) and I generate the certificate by exporting both certificate and private key to a .p12 as shown below:

I suspect the validation at line 219: if len(bags) != 2 { might be either incomplete or maybe I'm missing something. I've attached the certificate in question here with password googler for testing...

[ianlancetaylor]

CC @agl

[nathany]

@tipycalFlow If you comment out that validation and log.Println(bag.Id) within the bags loop, you can see what types of keys/certs the p12 contains. Certs end with 3 and keys end with 2 (see the oid* vars at the top of safebags.go).

Could we consider changing Decode to return slices of certs/keys or an x509.CertPool? What is the API stability policy for x/crypto?

For a project I'm working on, I would like to include an intermediate certificate in the .p12 (2 certs, one private key). I'd prefer not to use ToPEM to Marshal keys if it's unnecessary.

/cc @paulmey @AGWA

[nathany]

This issue isn't tied to the release cycle, but I'm planning to wait until after Go 1.6 is released and things settle down.

Adding a DecodeAll function should do the trick, while leaving the existing API intact. DecodeAll would possibly return a slice of x509 certificates and a slice of keys.

func DecodeAll(pfxData []byte, password string) (privateKeys []interface{}, certificates []*x509.Certificate, err error) {

Right now the keys or interface{} because they may be RSA or another format. I don't know that there is any better option, but I'd like to look into that.

I'd also like to include an example of using this method with TLS, which isn't immediately obvious.

P.S. I think there is a bug in the current implementation where it continues to decode bags after an "pkcs12: expected exactly one certificate bag" error. There should be a return statement and tests around this. In reality this is unlikely to be triggered thanks to an earlier check. The p12 would need to be contain exactly two certs and no keys.
https://github.com/golang/crypto/blob/master/pkcs12/pkcs12.go#L228

[zhengying]

Same error in 1.7. 1
any update?

[nathany]

@zhengying Sorry, I haven't worked on this yet. The x/crypto library isn't tied to a Go release, so once this issue is closed you will be able to just update x/crypto.

If anyone else wants to contribute a CL (change list), please don't wait for me.

NOTE: Another consideration is whether to contribute to x/crypto at all, or just patch an independent repository. See Azure/go-pkcs12#28.

[rautammi]

I am sorry to bring up this old issue again. Is there a decision about DecodeAll function and x/crypto/pkcs12? If DecodeAll won't be added into x/crypto/pkcs12, which fork one should use? https://github.com/lotus-wu/go-pkcs12 ?

My use case is to decode certificate chain from p12 for use with the crypto/tls. I don't need encoding of p12.

[adamdecaf]

I ran into a similar problem (#23499) with the other != 2 check in this code.

[ereOn]

For the record, I just submitted a PR that adds the DecodeAll function, as suggested by @nathany.

[gopherbot]

Change https://golang.org/cl/105876 mentions this issue: pkcs12: add a DecodeAll method

[roelandxyz]

Why is this code review still open? There is a remark about the commit message but I don't understand if that is a question or just an edit?
I have a .P12 with multiple certificates that I would like to decode.

[gopherbot]

Change https://golang.org/cl/160257 mentions this issue: Revert "pkcs12: add a DecodeAll method"

[gopherbot]

Change https://golang.org/cl/160258 mentions this issue: pkcs12: add a note suggesting ToPEM for multiple certificates/keys

[FiloSottile]

As explained in https://go-review.googlesource.com/c/crypto/+/105876/12#message-0dad31af2b487e895ee6926ded82f85ac81c74f8 I reverted this PR.

If you need to extract multiple certificates/keys, you can use ToPEM which will give you all the contents of the file.

If you do want to add a helper for that operation, I suggest sending a PR to software.sslmate.com/src/go-pkcs12 (https://github.com/SSLMate/go-pkcs12).

[AGWA]

Maintainer of software.sslmate.com/src/go-pkcs12 here. I'd be happy to review a PR for DecodeAll over at https://github.com/SSLMate/go-pkcs12