Allow non-cert public keys #965

ecordell · 2016-09-21T19:46:36Z

This allows non-cert PEM encoded public keys to be loaded as delegation keys

docker-jenkins · 2016-09-21T19:48:40Z

Can one of the admins verify this patch?

endophage · 2016-09-21T22:06:39Z

tuf/utils/x509.go

+	if err != nil {
+		return "", fmt.Errorf("unable to parse pem encoded public key: %v", err)
+	}
+	_, ok := pub.(*ecdsa.PublicKey)


Use a type switch for clarity and ease of adding other key types in the future:

switch pub.(type) { case *ecdsa.PublicKey: return data.ECDSAKey, nil case *rsa.PublicKey: return data.RSAKey, nil }

Non-blocking: We seem to support ED25519 for ParsePEMPrivateKey, although I guess we concatenate the private and public parts. I don't think it violates any existing spec to PEM-encode the public key, although I know it's not not supported by openssl or other software. Would it make sense to either support that here, or remove support in ParsePEMPrivateKey?

I'm for supporting it here as well - PEM's just a transport format.

@endophage Also a good review note - not sure why I didn't do that originally.

endophage · 2016-09-21T22:07:45Z

tuf/utils/x509.go

 	}
+	return "", fmt.Errorf("unkown public key format")


I think we probably have a relevant error type for this somewhere.

I couldn't find one but I'm happy to switch it if you can point me to it.

In tuf/signed/errors.go we have ErrInvalidKeyType which seems appropriate. It's used to indicate it the key type is invalid/unrecognized in the context of key types Notary accepts, not necessarily that the key is universally invalid.

endophage · 2016-09-21T22:09:16Z

tuf/utils/x509.go

@@ -252,9 +252,31 @@ func ParsePEMPublicKey(pubKeyBytes []byte) (data.PublicKey, error) {
 			return nil, fmt.Errorf("invalid certificate: %v", err)
 		}
 		return CertToKey(cert), nil
+	case "PUBLIC KEY":
+		keyType, err := keyTypeForPublicKey(pemBlock.Bytes)


Would it make sense to move this entire case block into a new constructor for data.PublicKey objects that takes a []byte (in this case pemBlock.Bytes) and returns (PublicKey, error)?

That seems like a good refactor to me; I assume the same for ParsePEMPrivateKey?

I was going to bring this up elsewhere but it seems relevant here: I think it's confusing that if you pass in a cert to ParsePEMPublicKey you get back a data.PublicKey object containing the cert bytes in the Public field instead of the parsed out public key bytes.

I was planning to read through a bit more before suggesting a change, but personally I like the idea of a flow like: certs get loaded/verified -> translated to plain keys -> tuf only deals with plain keys.

This seems like a very clean separation to me; the only issue would be that you would lose the property that certs get verified right before they're used every time. Perhaps it would make sense, after loading the certs into memory, to keep around a map of parsedKeys -> originatingCerts so that at various well-defined points during verification, you can do something like reverifyCerts(currentlyReleventKeyList) to find the certs you're using and check them again.

Just a couple of thoughts I've had on making the flow of notary easier to understand. I've just rambled a bit too much for an issue comment, so I'll pull this out into an issue later today...

I agree it's a little messy and would be good to clean up. Let's discuss how to approach it in the issue you file.

cyli

I think there's a minor lint error. It'd also be awesome to add a test case for a PEM encoded file of a public key that is not supported (if we don't want to support ED25519, for example, that might be a good one. :)

Thanks for adding this!

cyli · 2016-09-21T22:20:35Z

trustpinning/certs_test.go

@@ -814,6 +814,52 @@ func generateRootKeyIDs(r *data.SignedRoot) {
 	}
 }

+func TestParsePEMPublicKey(t *testing.T) {
+	gun := "notary"
+	pass := func(keyName, alias string, createNew bool, attempts int) (passphrase string, giveup bool, err error) {


Non-blocking: I think you can use passphrase.ConstantRetriever("password") for this, unless you don't want the extra import.

cyli · 2016-09-21T22:32:01Z

tuf/utils/x509.go

+	if err != nil {
+		return "", fmt.Errorf("unable to parse pem encoded public key: %v", err)
+	}
+	_, ok := pub.(*ecdsa.PublicKey)


Non-blocking: We seem to support ED25519 for ParsePEMPrivateKey, although I guess we concatenate the private and public parts. I don't think it violates any existing spec to PEM-encode the public key, although I know it's not not supported by openssl or other software. Would it make sense to either support that here, or remove support in ParsePEMPrivateKey?

ecordell · 2016-09-26T15:57:17Z

Addressed review comments.

@endophage held off on public key refactoring because of #977

@cyli Didn't end up adding ed25519 support because the x509 library doesn't recognize it as a valid PEM public key type. If it's something that should be supported can always wrap the x509 lib and add ed25519 support.

cyli · 2016-09-26T17:49:01Z

jenkins, test this please

cyli

Didn't end up adding ed25519 support because the x509 library doesn't recognize it as a valid PEM public key type. If it's something that should be supported can always wrap the x509 lib and add ed25519 support.

I think we use ED25519ToPrivateKey to unmarshal the private key - we do our own weird thing where basically the bytes are the public key bytes + the private key bytes, which is how we unmarshal the private key. I think the same thing can be done to parse out the public bytes (not using the x509 lib). This is non-blocking though - it's not like we supported importing ED25519 keys before. :)

I'm ok with addressing #977 in a separate PR - this LGTM aside from the one extra function that can be removed.

cyli · 2016-09-26T18:03:19Z

trustpinning/certs_test.go

@@ -1007,12 +1003,79 @@ func generateExpiredTestingCertificate(rootKey data.PrivateKey, gun string) (*x5
 	return cryptoservice.GenerateCertificate(rootKey, gun, startTime, startTime.AddDate(1, 0, 0))
 }

+// Helper function for explicitly generating key IDs and unexported fields for equality testing
+func generateRootKeyIDs(r *data.SignedRoot) {


I'm not sure if this function is still used anywhere - should it be removed? Although you do bring up a good point; it would be good to useful to have equality test functions for some of our data structures that have unexported fields at some point.

cyli · 2016-09-26T22:59:56Z

jenkins, test this please

cyli

Thanks for the update! :) LGTM

cyli · 2016-09-26T23:10:09Z

(Hmm... you can't edit the text of your previous review status comment :P sorry for the spam!)

riyazdf

some small nits, but overall LGTM! Thank you @ecordell :)

riyazdf · 2016-09-27T16:56:37Z

trustpinning/certs_test.go

+
+	// unsupported key type
+	unsupportedPemBytes := pem.EncodeToMemory(&pem.Block{
+		Type:    "UNSUPORTED KEY",


nit typo: UNSUPPORTED

I think this case errors because pem.Decode fails, not because of the type-switch.

This case is great, but It would be awesome if we could add another test case with a valid type that ParsePEMPublicKey does not support, like CERTIFICATE REQUEST or PRIVATE KEY

Signed-off-by: Evan Cordell <cordell.evan@gmail.com>

cyli · 2016-09-30T17:57:54Z

jenkins, test this please

riyazdf · 2016-09-30T18:13:34Z

thanks @ecordell for updating this, glad to have this feature :)

GordonTheTurtle added status/0-triage dco/no labels Sep 21, 2016

ecordell force-pushed the non-cert branch from c6920c8 to 8ba0b06 Compare September 21, 2016 19:48

GordonTheTurtle removed the dco/no label Sep 21, 2016

endophage reviewed Sep 21, 2016

View reviewed changes

cyli reviewed Sep 21, 2016

View reviewed changes

ecordell mentioned this pull request Sep 23, 2016

Separate cert verification from TUF metadata handling #977

Closed

ecordell force-pushed the non-cert branch 3 times, most recently from bea9d25 to 3de4d9c Compare September 26, 2016 15:42

cyli suggested changes Sep 26, 2016

View reviewed changes

ecordell force-pushed the non-cert branch from 3de4d9c to f61a677 Compare September 26, 2016 22:03

cyli approved these changes Sep 26, 2016

View reviewed changes

riyazdf approved these changes Sep 27, 2016

View reviewed changes

Allow non-cert public keys

452fe50

Signed-off-by: Evan Cordell <cordell.evan@gmail.com>

ecordell force-pushed the non-cert branch from f61a677 to 452fe50 Compare September 29, 2016 21:52

riyazdf merged commit 0dba98d into notaryproject:master Sep 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow non-cert public keys #965

Allow non-cert public keys #965

ecordell commented Sep 21, 2016

docker-jenkins commented Sep 21, 2016

endophage Sep 21, 2016

cyli Sep 21, 2016

ecordell Sep 22, 2016

ecordell Sep 22, 2016

endophage Sep 21, 2016

ecordell Sep 26, 2016

endophage Sep 27, 2016

endophage Sep 21, 2016

ecordell Sep 22, 2016 •

edited

Loading

endophage Sep 22, 2016

ecordell Sep 26, 2016

cyli left a comment

cyli Sep 21, 2016 •

edited

Loading

cyli Sep 21, 2016

ecordell commented Sep 26, 2016

cyli commented Sep 26, 2016

cyli left a comment

cyli Sep 26, 2016

cyli commented Sep 26, 2016

cyli left a comment

cyli commented Sep 26, 2016

riyazdf left a comment

riyazdf Sep 27, 2016

riyazdf Sep 27, 2016

ecordell Sep 29, 2016

cyli commented Sep 30, 2016

riyazdf commented Sep 30, 2016

Allow non-cert public keys #965

Allow non-cert public keys #965

Conversation

ecordell commented Sep 21, 2016

docker-jenkins commented Sep 21, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli left a comment

Choose a reason for hiding this comment

cyli Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell commented Sep 26, 2016

cyli commented Sep 26, 2016

cyli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Sep 26, 2016

cyli left a comment

Choose a reason for hiding this comment

cyli commented Sep 26, 2016

riyazdf 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

cyli commented Sep 30, 2016

riyazdf commented Sep 30, 2016

ecordell Sep 22, 2016 •

edited

Loading

cyli Sep 21, 2016 •

edited

Loading