feat: support p12 certificate format

wangtiga commented 1 year ago

This commit adds support p12 certificate format.

add three param -cacert-format -cert-format -pass

$ grpcurl -insecure -import-path ./protos -proto ./protos/apiService.proto -cacert ca.pem -cert client.pfx -cert-format P12 -pass pfxpassword -key client.key -d '{"msg":"hi"}' 127.0.0.1:50053 apiService.SayHello
{}

$ grpcurl -insecure -import-path ./protos -proto ./protos/apiService.proto -cacert ca.der -cacert-format DER -cert client.der -cert-format DER -key client.key -d '{"msg":"hi"}' 127.0.0.1:50053 apiService.SayHello
{}

$ grpcurl -insecure -import-path ./protos -proto ./protos/apiService.proto -cacert ca.der -cacert-format '' -cert client.der -cert-format '' -key client.key -d '{"msg":"hi"}' 127.0.0.1:50053 apiService.SayHello
{}

$ grpcurl -insecure -import-path ./protos -proto ./protos/apiService.proto -cacert ca.pem -cert client.pem -key client.key -d '{"msg":"hi"}' 127.0.0.1:50053 apiService.SayHello
{}

This should close https://github.com/fullstorydev/grpcurl/issues/331

ghost commented 1 year ago

👇 Click on the image for a new way to code review

#### [![Review these changes using an interactive CodeSee Map](https://s3.us-east-2.amazonaws.com/maps.codesee.io/images/github/fullstorydev/grpcurl/415/bf40d8d0/170757ed78a4cb620b71c4c2112c64e376ca16e3.svg)](https://app.codesee.io/r/reviews?pr=415&src=https%3A%2F%2Fgithub.com%2Ffullstorydev%2Fgrpcurl) #### Legend CodeSee Map legend

dragonsinth commented 1 year ago

I have some questions.... 1) If we want to support a new type of cert (p12), what would you call the old type? 2) Is password applicable to any other types of certs, or is this p12 specific? 3) Could the cert type be auto detected without an explicit command line option?

wangtiga commented 1 year ago

If we want to support a new type of cert (p12), what would you call the old type?

Previously, grpcurl only supported certificates and private keys in PEM format, which can be referenced in tls.LoadX509KeyPair() https://pkg.go.dev/crypto/tls#LoadX509KeyPair.
The P12 format is a format that packages the certificate and private key files into one file, as described in rfc7292 https://datatracker.ietf.org/doc/html/rfc7292#section-3.

Is password applicable to any other types of certs, or is this p12 specific?

Passwords may also be applicable to PEM format certificates, as described in rfc7468 on the Textual Encoding of PKCS #8 Encrypted Private Key Info (https://datatracker.ietf.org/doc/html/rfc7468#section-11).

Could the cert type be auto detected without an explicit command line option?

In addition to the currently supported PEM and P12 certificate formats, there are actually other formats such as DER and ENG.
While it is possible to implement auto-detection for just PEM and P12 formats, considering the possibility of future compatibility with more certificate formats, it is better to have users explicitly specify the certificate format. This can be seen in the example of curl which allows users to specify the certificate type using the --cert-type <type> (TLS) Tells curl what type the provided client certificate is using. PEM, DER, ENG and P12 are recognized types. option https://curl.se/docs/manpage.html.

dragonsinth commented 1 year ago

If we want to support a new type of cert (p12), what would you call the old type?

Previously, grpcurl only supported certificates and private keys in PEM format, which can be referenced in tls.LoadX509KeyPair() https://pkg.go.dev/crypto/tls#LoadX509KeyPair.

The P12 format is a format that packages the certificate and private key files into one file, as described in rfc7292 https://datatracker.ietf.org/doc/html/rfc7292#section-3.

Gotcha. So if this PR merged, we'd have code written to support PEM and P12 but nothing else.

Is password applicable to any other types of certs, or is this p12 specific?

Passwords may also be applicable to PEM format certificates, as described in rfc7468 on the Textual Encoding of PKCS #8 Encrypted Private Key Info (https://datatracker.ietf.org/doc/html/rfc7468#section-11).

Seems like the password option should also apply to PEM then?

Could the cert type be auto detected without an explicit command line option?

In addition to the currently supported PEM and P12 certificate formats, there are actually other formats such as DER and ENG.

While it is possible to implement auto-detection for just PEM and P12 formats, considering the possibility of future compatibility with more certificate formats, it is better to have users explicitly specify the certificate format. This can be seen in the example of curl which allows users to specify the certificate type using the --cert-type <type> (TLS) Tells curl what type the provided client certificate is using. PEM, DER, ENG and P12 are recognized types. option https://curl.se/docs/manpage.html.

I get it, but I worry about option shock. If we added logic to auto-detect the cert type, is there any reason to think we couldn't extend such auto-detection to DER and ENG in the future?

dragonsinth commented 1 year ago

@wangtiga I pushed a commit demonstrating PEM decoding in a test; could you do something similar for p12? Generate a bogus key and cert with a password and write a simple test calling loadClientCertP12 to demonstrate that it works?

wangtiga commented 1 year ago

@wangtiga I pushed a commit demonstrating PEM decoding in a test; could you do something similar for p12? Generate a bogus key and cert with a password and write a simple test calling loadClientCertP12 to demonstrate that it works?

Okay, I'm currently trying auto-detection to DER and ENG formats. Once it's done, I will add unit tests for the relevant formats.

wangtiga commented 1 year ago

Currently, -cacert-format and -cert-format support three types of certificate formats: PEM, DER, and PKCS12, while key only supports the PEM format.
The -pass parameter is only allowed when using the -cert-format flag with PKCS12. However, in reality, PEM-formatted private keys can also have password protection, but this feature is not currently supported.
While we have added the -cacert-format '' and -cert-format '' options for auto-detection, the scope of automatic detection is limited. If we were to identify file contents, the current code would not be able to differentiate between PKCS12 and DER format certificate files. Specifically, a DER format file such as tls/client.der would be incorrectly identified as PKCS12, and a PEM format file such as tls/client.guess would not be recognized.
I have also added some unit tests for different certificate formats. I am unsure if this approach is appropriate, so I would appreciate any guidance or feedback.

fullstorydev / grpcurl