Added support for parsing certificates with RSAES-OAEP public keys #470

DenisKarch · 2019-03-08T19:28:56Z

This will currently return an *rsa.PublicKey throwing away the hashing
and masking algorithms.
We are working with go upstream to find a more permanent solution.

This will currently return an *rsa.PublicKey throwing away the hashing and masking algorithms. We are working with go upstream to find a more permanent solution.

ericchiang · 2019-03-08T22:58:45Z

x509/x509.go

+			N: p.N,
+		}
+		return pub, nil
+	case RSAESOAEP:


nit, instead of a different case, combine this with the case above and use an if statement.

case RSA, RSAESOAEP: // ... if algo == RSAESOAEP { // param parsing } // ...

ericchiang · 2019-03-08T23:00:00Z

x509/x509_test.go

@@ -1119,6 +1119,35 @@ func TestRSAPSSSelfSigned(t *testing.T) {
 	}
 }

+// Valid EKCert with RSAES-OAEP Public Key
+var oaepCertDER = `3082056e30820456a003020102020464f5bda5300d06092a864886f70d01010505003077310b3009060355040613024445310f300d060355040813065361786f6e793121301f060355040a1318496e66696e656f6e20546563686e6f6c6f67696573204147310c300a060355040b130341494d312630240603550403131d4946582054504d20454b20496e7465726d656469617465204341203230301e170d3135303932313231323932385a170d3235303932313231323932385a300030820137302206092a864886f70d0101073015a213301106092a864886f70d0101090404544350410382010f003082010a02820101008e7983105063a3f1c2e77d7c8b4f411db9c76f11366ccbb11757001fa51805bbbf68b6dccd9ad28a82c6a831e1591f06181c2e328c261cf9a14ff1704dc3261cc16da751760141969330a75b3fc5ae185ac76d636a97a339838bd042aa7c5999f63f946c6987b0e8be8f5908d08563f62bc6ee9510e36752bd3b2e7b55281bc92a8dcc8ba8a30d6aaa7580b672d83802449d34bfea0434edea4dbc3a9b201f3de0bf5ab2ca96a5254f4e76a58adc8d3bc385be94e93e0052e4066f238a9c1195eaacda02a6dc78393063340054e3ce99754a8770c8efcca45ad8fc999f7aaa67a8a83960141e5f4b892d3af333f09b6d13e60900e0ea8bd3b5eea30f4f2b889b0203010001a38202623082025e30550603551d110101ff044b3049a447304531163014060567810502010c0b69643a343934363538303031173015060567810502020c0c534c42393636307878312e3231123010060567810502030c0769643a30343238300c0603551d130101ff040230003081bc0603551d200101ff0481b13081ae3081ab060b6086480186f84501072f0130819b303906082b06010505070201162d687474703a2f2f7777772e766572697369676e2e636f6d2f7265706f7369746f72792f696e6465782e68746d6c305e06082b0601050507020230521e5000540043005000410020005400720075007300740065006400200050006c006100740066006f0072006d0020004d006f00640075006c006500200045006e0064006f007200730065006d0065006e00743081a10603551d2304819930819680148ffd47880e239a3a3a20de13edf101e882a9d21da17ba4793077310b3009060355040613024445310f300d060355040813065361786f6e793121301f060355040a1318496e66696e656f6e20546563686e6f6c6f67696573204147310c300a060355040b130341494d312630240603550403131d4946582054504d20454b20496e7465726d6564696174652043412032308201053081930603551d0904818b308188303a06035504343133300b300906052b0e03021a05003024302206092a864886f70d0101073015a213301106092a864886f70d010109040454435041301606056781050210310d300b0c03312e32020102020103303206056781050212312930270101ffa0030a0101a1030a0100a2030a0100a310300e1603332e310a01040a01010101ff0101ff300d06092a864886f70d010105050003820101003b25d3958cf08b2c32cefb40570f845e25fa98fd38a47be8c16e4ef663d8f057824ff550052b4a338e353b08bfe70364cda07961d3f4fb5cbf548497389a5ab59c3469d14b6f5bfdc27db3dd0cbd21ac818a64a14032cd433a2bffb939b5f54555c2b6892b5f6479e3c38bd4b30283d2e8ee57ecad779dae90bd9d6052ad6bf3efbb30f639e03b0a239f539a476b129540bc9806e2dc7011d265dca8a95301aaba872b4e176bdb670e65b750b0afdc2ca97b3f0bef55862ae135bd86b1d3a28d4eedfe5ee445a3f1d65bbad0adae49999e613eabb133166461cf47146de078a7d7f550940a1a475ecc834ee784a6a6e42fa4ad7869ef8d2b2251c830efb38496`


nit: consider PEM encoding this so it's not just a super long string. https://golang.org/pkg/encoding/pem/

Also, explain where this cert comes from, and why we care about parsing it.

I couldn't find any easy way to convert the cert from DER to PEM (probably because of the RSAES-OAEP public key).

PEM is just base64'ing the DER data. Here's an example https://play.golang.org/p/-ADspaRm91-

Let me know if you have any other questions :)

codecov · 2019-03-09T00:13:19Z

Codecov Report

Merging #470 into master will increase coverage by 0.03%.
The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
+ Coverage   66.64%   66.67%   +0.03%     
==========================================
  Files          88       88              
  Lines       10700    10713      +13     
==========================================
+ Hits         7131     7143      +12     
  Misses       2962     2962              
- Partials      607      608       +1

Impacted Files	Coverage Δ
x509/x509.go	`71.52% <73.33%> (-0.03%)`	⬇️
fixchain/logger.go	`80.68% <0%> (+3.4%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12c6b17...8caa4d6. Read the comment docs.

daviddrysdale

Seems reasonable, just a bunch of minor things.

x509/x509.go

daviddrysdale · 2019-03-11T09:40:45Z

x509/x509.go

+	// 2.1.  One-way Hash Functions
+	OIDSHA1 = asn1.ObjectIdentifier{1, 3, 14, 3, 2, 26}
+	// 2.2.  Mask Generation Functions
+	OIDMFGSHA1 = asn1.ObjectIdentifier{1, 2, 840, 113549, 1, 1, 8}


This value already exists as oidMGF1 (and there's a typo in the name)

daviddrysdale · 2019-03-11T09:40:58Z

x509/x509.go

+// Various identifiers
+var (
+	// 2.1.  One-way Hash Functions
+	OIDSHA1 = asn1.ObjectIdentifier{1, 3, 14, 3, 2, 26}


This one could probably live next to the definitions of oidSHA256, oidSHA384 and oidSHA512 further down (which are also from RFC 4055 s2.1)

daviddrysdale · 2019-03-11T09:55:59Z

x509/x509.go

+	}
+	mgf1SHA1Identifier = pkix.AlgorithmIdentifier{
+		Algorithm:  OIDMFGSHA1,
+		Parameters: asn1.RawValue{0, 6, false, []byte{43, 14, 3, 2, 26}, []byte{6, 5, 43, 14, 3, 2, 26}},


Is this value for the Parameters right? It looks like it's the id-sha1 OID, but RFC 4055 s2.2 says the parameters should be sha1Identifier (which is an AlgorithmIdentifier that's a SEQUENCE of that OID plus NULL parameters). Or am I reading the spec wrong?

daviddrysdale · 2019-03-11T09:57:14Z

x509/x509.go

+	}
+	pSpecifiedEmptyIdentifier = pkix.AlgorithmIdentifier{
+		Algorithm:  OIDpSpecified,
+		Parameters: asn1.RawValue{0, 4, false, []byte{}, []byte{4, 0}},


Be nice to add a comment here (as well as converting the numbers to asn1 constants) describing what the parameters are supposed to be.

x509/x509_test.go

daviddrysdale · 2019-03-11T10:03:50Z

x509/x509.go

@@ -238,6 +244,37 @@ const (
 	SHA512WithRSAPSS
 )

+// RFC 4055
+// Various identifiers


Do these constants need to be exported?

x509/x509.go

Fix to mgf1SHA1Identifier (only had sha1 oid in params but needed sha1Identifier) Style changes and comments

ericchiang · 2019-03-11T20:46:10Z

x509/x509_test.go

@@ -1119,6 +1119,39 @@ func TestRSAPSSSelfSigned(t *testing.T) {
 	}
 }

+// Valid EKCert (from a TPM)  with RSAES-OAEP Public Key.


Which hardware vendor is this? Can you include that information in your comment?

daviddrysdale

Just a couple of nits

daviddrysdale · 2019-03-12T13:08:59Z

x509/x509.go

@@ -1264,14 +1329,14 @@ func parsePublicKey(algo PublicKeyAlgorithm, keyData *publicKeyInfo, nfe *NonFat
 		if len(rest) != 0 {
 			return nil, errors.New("x509: trailing data after RSA public key")
 		}
-


nit: you've removed a blank line here (sorry to be pedantic, but any unnecessary changes make upstream merges harder)

daviddrysdale · 2019-03-12T13:09:30Z

x509/x509.go

 			nfe.AddError(errors.New("x509: RSA key missing NULL parameters"))
 		}
+		if algo == RSAESOAEP {
+			// We only parse the parameters to ensure it is a valid encoding, we throw out the actually values


nit: add a spec ref here, s/actually/actual/

x509/x509.go

Replaced DER cert with PEM Added TODO (linked to the open issue in golang/go) Format and name changes (corrected typos)

crypto/x509 now enforces that rsa public keys must have NULL parameters. The old fix no longer solves the issue and instead will silently fail when parsing the key, ultimately causing a null pointer dereference at (pubkey := cert.PublicKey.(*rsa.PublicKey)). Currently working with crypto/x509 to add support for RSAES-OAEP keys golang/go#30416 For certificate-transparency-go have accepted a temporary fix to /x509 google/certificate-transparency-go#470 so we will be building against them for the time being.

crypto/x509 now enforces that rsa public keys must have NULL parameters. The old fix no longer solves the issue and instead will silently fail when parsing the key, ultimately causing a null pointer dereference at (pubkey := cert.PublicKey.(*rsa.PublicKey)). Currently working with crypto/x509 to add support for RSAES-OAEP keys golang/go#30416 certificate-transparency-go have accepted a temporary fix to /x509 google/certificate-transparency-go#470 so we will be building against them for the time being.

Added support for parsing certificates with RSAES-OAEP public keys

80edb96

This will currently return an *rsa.PublicKey throwing away the hashing and masking algorithms. We are working with go upstream to find a more permanent solution.

googlebot added the cla: yes label Mar 8, 2019

ericchiang reviewed Mar 8, 2019

View reviewed changes

RJPercival requested review from daviddrysdale and RJPercival March 8, 2019 23:58

daviddrysdale reviewed Mar 11, 2019

View reviewed changes

Changes for PR

e433068

Fix to mgf1SHA1Identifier (only had sha1 oid in params but needed sha1Identifier) Style changes and comments

DenisKarch force-pushed the rsaes-oaep branch from eb06d37 to e433068 Compare March 11, 2019 20:21

ericchiang reviewed Mar 11, 2019

View reviewed changes

daviddrysdale approved these changes Mar 12, 2019

View reviewed changes

Changes for PR

8caa4d6

Replaced DER cert with PEM Added TODO (linked to the open issue in golang/go) Format and name changes (corrected typos)

DenisKarch force-pushed the rsaes-oaep branch from fb1f3ce to 8caa4d6 Compare March 12, 2019 16:23

daviddrysdale merged commit 653f849 into google:master Mar 12, 2019

DenisKarch deleted the rsaes-oaep branch March 12, 2019 17:24

ericchiang mentioned this pull request Mar 12, 2019

Fix to GenerateChallenge google/go-tspi#24

Merged

Added support for parsing certificates with RSAES-OAEP public keys #470

Added support for parsing certificates with RSAES-OAEP public keys #470

Uh oh!

Conversation

DenisKarch commented Mar 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

daviddrysdale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daviddrysdale left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 9, 2019 •

edited

Loading