feat: support p12 certificate format #415

wangtiga · 2023-10-02T02:18:27Z

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 #331

ghost · 2023-10-02T02:19:08Z

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

Legend

dragonsinth · 2023-10-09T13:01:52Z

I have some questions....

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

wangtiga · 2023-10-13T14:09:15Z

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 · 2023-10-13T16:01:46Z

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?

jhump

Some feedback. Note that even if the feedback were addressed, it's still up to @dragonsinth to decide whether to accept the change or not.

jhump · 2023-10-13T16:48:48Z

grpcurl.go

-		certificate, err := tls.LoadX509KeyPair(clientCertFile, clientKeyFile)
+		var certificate tls.Certificate
+		var err error
+		if strings.EqualFold(clientCertType, "p12") {


For one, we never check that the alternative is "pem". So we accept a garbage cert type, as long as the cert file is actually pem-encoded, which is non-intuitive.

Also (1) why EqualFold? We can just make it case-sensitive. (2) Why string comparisons everywhere? Maybe introduce a new type CertType int to represent this, instead of it being stringly typed.

jhump · 2023-10-13T16:50:24Z

grpcurl.go

@@ -527,12 +529,18 @@ func ClientTransportCredentials(insecureSkipVerify bool, cacertFile, clientCertF
 // given properties. If cacertFile is blank, only standard trusted certs are used to
 // verify the server certs. If clientCertFile is blank, the client will not use a client
 // certificate. If clientCertFile is not blank then clientKeyFile must not be blank.
-func ClientTLSConfig(insecureSkipVerify bool, cacertFile, clientCertFile, clientKeyFile string) (*tls.Config, error) {
+func ClientTLSConfig(insecureSkipVerify bool, cacertFile, clientCertFile, clientKeyFile, clientCertType, clientPass string) (*tls.Config, error) {


This is not a backwards-compatible change. This is exported API, and any other callers of this would be broken by the signature change. This kind of capability would have to be exposed via a new function.

Thank you for your suggestions. I am currently trying to implement auto-detection to DER and ENG formats, and I will work on addressing the issues you pointed out during this process.

jhump · 2023-10-13T16:56:06Z

cmd/grpcurl/grpcurl.go

+	if (*key == "") != (*cert == "") && !strings.EqualFold(*certType, "p12") {
 		fail(nil, "The -cert and -key arguments must be used together and both be present.")
 	}


This is insufficient. With "p12", it still doesn't make sense to provide a -key argument but not -cert, since the actual kay+cert is loaded from the -cert argument. Also, if they do specify both as different files, it should probably be an error with "p12", since the key argument is erroneous (and the current implementation would ignore it).

Also, nowhere is validating the the -cert-type flag be "p12" or "pem" as indicated in the help for the flag. So this allows users to set a garbage type, in which case the meaning is unclear. It also misleadingly lets them enter "der" or "eng", but then expects the file to actually be "pem".

dragonsinth · 2023-11-02T16:26:48Z

grpcurl.go

+	for _, block := range pemBlocks {
+		pemBytes = append(pemBytes, pem.EncodeToMemory(block)...)
+	}
+	certificate, err := tls.X509KeyPair(pemBytes, pemBytes)


pemBytes, pemBytes seems strange... shouldn't there be separate entries for the key and cert?

The file format PKCS12 will contain both the CERTIFICATE and PRIVATE KEY data, while the tls.X509KeyPair function internally identifies the CERTIFICATE and PRIVATE KEY from the PEM data.

dragonsinth · 2023-11-02T16:34:42Z

.gitignore

@@ -2,3 +2,4 @@ dist/
 .idea/
 VERSION
 .tmp/
+*.swp


what's this?

"swp" files are temporary files generated by the vim.

dragonsinth · 2023-11-02T16:36:57Z

@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 · 2023-11-09T15:45:00Z

@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 · 2023-11-11T14:45:42Z

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.

dragonsinth · 2024-01-09T15:25:19Z

internal/certigo/lib/certs.go

+	".jceks": CertKeyFormatJCEKS,
+	".jks":   CertKeyFormatJCEKS, // Only partially supported
+	".der":   CertKeyFormatDER,
+}


This file looks to have a lot of content copied from square/certigo/lib/certs.go -- why is so much code needing to be copied instead of finding a way to use the lib directly?

The main purpose is to add the functionality of auto-detecting file formats. In the square/certigo/lib package, the formatForFile function is private, so a new function lib.GuessFormatForFile has been added. Additionally, there are some minor adjustments, such as introducing the CertificateKeyFormat type to identify file formats and reducing direct string comparisons operations.

dragonsinth · 2024-01-09T15:38:40Z

internal/certigo/lib/cert_key_format.go

+
+func (f CertificateKeyFormat) IsNone() bool {
+	return f == CertKeyFormatNONE
+}


This seems misleading.. these are called CertificateKeyFormat, but as far as I can tell the only supported key format is PEM, whereas the cert formats are what vary.

Both JCEKS and PKCS12 formats include both key and certificate files.

The key file can also be in DER format, but its usage frequency is low, so it is currently not supported.

I am not using the JCEKS format at the moment and haven't tested it, so the code for it is included but not made available for public use.

jhump reviewed Oct 13, 2023

View reviewed changes

wangtiga force-pushed the dev-ws branch from a8adcc1 to a445f06 Compare October 16, 2023 16:18

dragonsinth reviewed Nov 2, 2023

View reviewed changes

wangtiga force-pushed the dev-ws branch 2 times, most recently from 1730e03 to 3dad42b Compare November 11, 2023 14:41

wangtiga added 7 commits December 10, 2023 00:37

support p12 certificate format, add param -cert-type and -pass

95fbe59

guess cert format

8060fff

tidy code not finished yet

6f4f86d

add unit test for cert PKCS12 DER PEM

0ade6c1

add testfile tls/testcert.pem tls/testkey.pem

a0139c0

tidy code

99e57ce

tidy code

f5e7213

wangtiga force-pushed the dev-ws branch from 28b9138 to f5e7213 Compare December 9, 2023 16:49

wangtiga added 2 commits December 10, 2023 08:59

tidy code

bba2985

build success in cimg/go:1.20

559de63

dragonsinth reviewed Jan 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support p12 certificate format #415

feat: support p12 certificate format #415

wangtiga commented Oct 2, 2023 •

edited

Loading

ghost commented Oct 2, 2023

Legend

dragonsinth commented Oct 9, 2023

wangtiga commented Oct 13, 2023

dragonsinth commented Oct 13, 2023

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

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

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

jhump left a comment

jhump Oct 13, 2023

jhump Oct 13, 2023

wangtiga Nov 9, 2023

jhump Oct 13, 2023

dragonsinth Nov 2, 2023

wangtiga Nov 11, 2023 •

edited

Loading

dragonsinth Nov 2, 2023

wangtiga Nov 9, 2023

dragonsinth commented Nov 2, 2023

wangtiga commented Nov 9, 2023

wangtiga commented Nov 11, 2023

dragonsinth Jan 9, 2024

wangtiga Jan 18, 2024

dragonsinth Jan 9, 2024

wangtiga Jan 18, 2024

@@ @@ -2,3 +2,4 @@ dist/ @@
               .idea/
               VERSION
               .tmp/
+              *.swp

feat: support p12 certificate format #415

Are you sure you want to change the base?

feat: support p12 certificate format #415

Conversation

wangtiga commented Oct 2, 2023 • edited Loading

ghost commented Oct 2, 2023

Legend

dragonsinth commented Oct 9, 2023

wangtiga commented Oct 13, 2023

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

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

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

dragonsinth commented Oct 13, 2023

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

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

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

jhump left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangtiga Nov 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dragonsinth commented Nov 2, 2023

wangtiga commented Nov 9, 2023

wangtiga commented Nov 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangtiga commented Oct 2, 2023 •

edited

Loading

wangtiga Nov 11, 2023 •

edited

Loading