Support for PKCS#8 #1130

umayr · 2017-04-04T17:04:53Z

Migrate Private Encrypted Key to PKCS#8 format. Since PKCS#8 format doesn't support headers so the key is being wrapped into another ASN1 structure as DER format with Role and GUN values. So, the
key being saved to keystore will not be directly supported with anything else like openssl.

EDIT
After David's comment:

The one comment I do have is I figured we'd just have the plaintext ASN.1 wrapping the encrypted ASN.1. I see there's an additional PEM wrapper around all of it. Looking at the code, I actually think the PEM outermost wrapper makes sense, it keeps a lot of the parsing simpler as we can guarantee we're always dealing with PEM, then look at what's in the Type and Headers to determine how to handle the data of the PEM block. However, it also makes the middle plaintext ASN.1 unnecessary. We should just keep the GUN and Role in the PEM headers, and have the PEM data be the encrypted ASN.1.

I removed the additional asn1 wrapper. Now it works like this:

[rsa/ecdsa/ed25519] --> [pkcs8] --> [pem with headers]

I have used this library for PKCS#8. It required crypto.PrivateKey type to convert the key to PKCS#8. Since we were already marshalling keys to byte array while creating data.PrivateKey so I hard forked the library into notary utils and modified it in such a way that now it takes data.PrivateKey type and return PKCS#8 key in byte array. Moreover, library didn't support ED25519 so I have added support for that as well using asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11591, 15, 1} OID.

When Notary is compiled with FIPS flag, its going to give an error of unsupported key in case of Private Encrypted Key, it will only support PKCS#8. Meanwhile, when its built without FIPS flag, it supports both PKCS#8 and PKCS#1 previously created collections but only create PKCS#8 keys if you initialize a new collection.

docker-jenkins · 2017-04-04T17:04:55Z

Can one of the admins verify this patch?

ecordell · 2017-04-06T12:05:42Z

Thanks @umayr, this is looking good to me.

I think we should consider removing the [custom format in asn1 to support role and gun] step entirely, and remove those headers from the unencrypted format as well. IMO, it is the responsibility of the TUF metadata, not the key serialization format, to specify what a key should be used for. @docker/core-notary-maintainers @docker/external-notary-maintainers, any thoughts?

Also the tests need to be passing :) We should be careful to verify in the tests that any key that could be used by notary before this change can still be used.

I have a couple of small nits but I'll hold off on those for now.

endophage · 2017-04-06T17:19:22Z

@ecordell the purpose of having the GUN/role in the (previously) PEM headers was for notary key list to give the user a hint about what the key was used for. That's particularly important for root keys where you wouldn't want to accidentally re-use a root key for a less well secured role. We also use it to filter to only root keys when initializing a new repo (and we still need to do the work to allow a user to select which root key to use from those already available).

It may eventually make sense to also remove the GUN, I don't have a strong opinion on that for now but I think it's reasonable for this PR to replicate existing functionality, then we can look at removing GUNs in a follow up. This PR is a nice size, let's not force it to be larger :-)

The one comment I do have is I figured we'd just have the plaintext ASN.1 wrapping the encrypted ASN.1. I see there's an additional PEM wrapper around all of it. Looking at the code, I actually think the PEM outermost wrapper makes sense, it keeps a lot of the parsing simpler as we can guarantee we're always dealing with PEM, then look at what's in the Type and Headers to determine how to handle the data of the PEM block. However, it also makes the middle plaintext ASN.1 unnecessary. We should just keep the GUN and Role in the PEM headers, and have the PEM data be the encrypted ASN.1.

umayr · 2017-04-13T15:01:45Z

@endophage You're absolutely correct about additional asn1 wrapper. I have removed it, and updated the PR. Now it simply uses PEM headers to hold role and gun information.

umayr · 2017-05-02T10:46:10Z

Is there any update on this @endophage?

endophage · 2017-05-02T17:02:43Z

@umayr we need it to pass codecov. If you install the codecov browser plugin you'll be able to see exactly which lines are missing coverage.

endophage · 2017-05-04T15:41:20Z

Jenkins test this please

endophage · 2017-05-08T17:46:30Z

Apologies, our Jenkins slave is having issues contacting the master, hence it's showing as not completing. Judging by other PRs though, the yubikey tests add 3.18% to the coverage so this will still need a little more testing to pass.

GordonTheTurtle · 2017-05-10T07:35:13Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "feat/pkcs8" [email protected]:umayr/notary.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353363416
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

riyazdf · 2017-05-12T20:37:37Z

Jenkins test this please

endophage · 2017-06-15T15:50:51Z

tuf/utils/x509.go

 	if role != "" {
-		encryptedPEMBlock.Headers["role"] = role.String()
+		headers["role"] = string(role)


role.String()

endophage · 2017-06-15T15:51:09Z

tuf/utils/x509.go

 	if gun != "" {
-		encryptedPEMBlock.Headers["gun"] = gun.String()
+		headers["gun"] = string(gun)


gun.String()

endophage · 2017-06-15T15:55:48Z

cryptoservice/crypto_service.go

@@ -143,7 +144,17 @@ func CheckRootKeyIsEncrypted(pemBytes []byte) error {
 		return ErrNoValidPrivateKey
 	}

-	if !x509.IsEncryptedPEMBlock(block) {
+	if notary.FIPSEnabled() {


Seems like this could be simplified a bit:

if block.Type == "ENCRYPTED PRIVATE KEY" { return nil } if !notary.FIPSEnabled() && x509.IsEncryptedPEMBlock(block) { return nil } return ErrRootKeyNotEncrypted

endophage · 2017-06-15T16:16:17Z

tuf/utils/pkcs8.go

+
+// ParsePKCS8ToTufKey requires PKCS#8 key in DER format and returns data.PrivateKey
+// Second argument is optional and only provided in case of encrypted keys.
+func ParsePKCS8ToTufKey(der []byte, v ...[]byte) (data.PrivateKey, error) {


If we only ever expect v to have a length of 1 can we remove the ... and change line 185 to password := v

Its not here because it expects more than one byte arrays. Basically, variadic params are being used as optional params to keep things a bit simpler. like:

// no password ParsePKCSToTufKey(der) // with password ParsePKCSToTufKey(der, []byte("foo"))

Yeah, I see what you're doing, just for something as critical as a password, I kind of like the fact a caller would have to pass nil explicitly. It's a "yes, I know I'm not setting a password" kind of statement.

Alright. I will update this.

endophage · 2017-06-15T16:17:09Z

tuf/utils/pkcs8.go

+
+// ConvertTUFKeyToPKCS8 converts a private key (data.Private) to PKCS#8 and returns in DER format
+// Additional argument is provided in case of an encrypted PKCS#8 key.
+func ConvertTUFKeyToPKCS8(priv data.PrivateKey, v ...[]byte) ([]byte, error) {


Same comment on ...[]byte as above. If we only expect it to be length 1 can we remove the ...

Same reason as above.

endophage · 2017-06-23T21:48:52Z

tuf/utils/pkcs8.go

+	}
+
+	// Use the password provided to decrypt the private key
+	password := v[0]


This will error if v == [][]byte{} (because an empty non-nil slice isn't caught by line 180)

It'll happen in this case: https://play.golang.org/p/-DDT6Yuvxt

Edit: updated with right link

endophage · 2017-06-23T21:50:03Z

tuf/utils/pkcs8.go

+	if v == nil {
+		return convertTUFKeyToPKCS8(priv)
+	}
+	password := string(v[0])


Same comment about v[0] erroring if v == []byte{}

endophage

LGTM!

@ecordell @cyli @riyazdf can we get another pair of eyes to approve or comment please!

riyazdf

this is really close - a few comments and we should be able to wrap this up.

Also, could you squash the commits down to a few logical checkpoints? Whatever you think is best should be fine.

riyazdf · 2017-07-13T19:09:29Z

tuf/utils/x509.go

+		if notary.FIPSEnabled() {
+			return "", "", fmt.Errorf("%s not supported in FIPS mode", block.Type)
+		}
+	case "PRIVATE KEY", "ENCRYPTED PRIVATE KEY":


can we have an explicit continue in this case to make it more readable?

continue is for loops, its a switch case. I don't think continue would work here.

I had to confirm, there isn't. Its a noop case.

ah - right. Ok that's fine. A comment (preferably in the case itself) explaining why it's a no-op would be helpful, if you don't mind.

Definitely, I'll add a comment.

Done. I couldn't come up with anything else, its pretty much self explanatory.

riyazdf · 2017-07-13T19:12:30Z

tuf/utils/x509_test.go

+
+	var privKey data.PrivateKey
+	var err error
+	if password == nil {


nit: we don't need this conditional or the variable definitions above.
We should also check that the block isn't nil.

This can just be:

block, _ := pem.Decode(b) require.NotNil(block) privKey, err := ParsePKCS8ToTufKey(block.Bytes, password) ...

Yes. You're right. I'll remove this block.

riyazdf · 2017-07-13T19:13:43Z

tuf/utils/x509_test.go

+}
+
+func TestExtractPrivateKeyAttributes(t *testing.T) {
+	if os.Getenv(t.Name()) != "1" {


for my own understanding: what does this block do? If we don't have a defined environment we defer the function call below?

Its a tricky one. Basically this is a test we try to emulate both environments i.e. one with GOFIPS=1 and the other when the flag is not set. The workflow is somewhat like this:

checks if os.Getenv(t.Name()) is set to 1, initially it would be true, so we defer a function that keeps the value of environment variable in the closure and unsets it. So, rest of the test gets no value GOFIPS whether the tests are run with it being set to 1 or not.

checks all workflows related to both PKCS#1 and PKCS#8 for no value of GOFIPS.

spawns a new process that executes this particular test in an emulated environment which has two environment variable, GOFIPS=1 and t.Name()=1.

checks again if os.Getenv(t.Name()) is set to 1, this time its false so it does nothing.

checks all workflows related to both PKCS#1 and PKCS#8 for GOFIPS=1.

spawned process gets done, deferred closure gets called which sets the value of GOFIPS to whatever it was before the test.

I used this technique couple of time to ensure all workflows whether tests run with GOFIPS=1 or not. Let me know if it requires anything else.

We should keep checks for the env variable consistent across tests and code. I think the code checks for any non-empty value of GOFIPS so can we mimic the check here?

If I understand you correctly, you're saying instead of doing this:

if os.Getenv(t.Name()) != "1" {

I should do this:

if os.Getenv(t.Name()) != "" {

If that's the case, I agree. I'll update this.

I agree with @riyazdf that this block, and the bit at the end where the test runs itself again, is a little confusing to read.

If I understand this thread correctly, it looks like the extra env checking is to make sure that we can run this test both with and without the FIPS flag on, whether or not it's set in environment?

If my understanding is correct, would it be possible to factor this out into 2 tests: TestExtractPrivateKeyAttributesFIPSOn and TestExtractPrivateKeyAttributesNoFIPS or something? Then you won't have to do the extra notary.FIPSEnabled() to determine which behavior is correct.

Each one can call the preserveEnv function, which will both save the previous environment variable and set it to a new desired value?

Similarly with the test below.

riyazdf · 2017-07-13T19:20:40Z

tuf/utils/pkcs8.go

+	iter := 2048
+	salt := make([]byte, 8)
+	iv := make([]byte, 16)
+	rand.Reader.Read(salt)


for this Read and below: should we check the error return?

Alright. Makes sense, I'll add checks.

cyli

Sorry for the slowness - thanks for working on this @umayr. I only have a couple of minor comments.

cyli · 2017-07-13T18:01:46Z

fips.go

+// FIPSEnabled returns true if environment variable `GOFIPS` has been set to enable
+// FIPS mode
+func FIPSEnabled() bool {
+	return os.Getenv(FIPSEnvVar) != ""


Non-blocking: Should this check for negative values, like 0, or "false"?

Ah, I personally think it should. But I tried to replicate what @FiloSottile did in his library. Basically, 0 or "false" are true values as of now in GCS. If you strongly feel it should be updated, then I'll update in here as well as send a PR there.

Maybe my UX compass is wrong but I'm a big fan of "unset" is false, and "set to anything" is true.

@endophage: fair point - this is fine as is, though we should add documentation (followup PR)

cyli · 2017-07-13T18:54:47Z

tuf/utils/pkcs8.go

@@ -0,0 +1,299 @@
+package utils


Maybe we can add a link to the project (https://github.com/youmark/pkcs8) and a comment that this isn't a straight up copy but modified? (to explain why we didn't just vendor?)

cyli · 2017-07-13T19:20:45Z

tuf/utils/pkcs8.go

+}
+
+func parsePKCS8ToTufKey(der []byte) (data.PrivateKey, error) {
+	var key struct {


Would it make sense to just move this struct definition out of the function? (e.g. include a top level private var with comments, copied from https://github.com/golang/go/blob/964639cc338db650ccadeafb7424bc8ebb2c0f6c/src/crypto/x509/pkcs8.go#L14, so someone going through this code knows where this is from?

cyli · 2017-07-13T21:14:37Z

tuf/utils/pkcs8_test.go

+	testEDKey, err := GenerateED25519Key(rand.Reader)
+	require.NoError(t, err)
+
+	testConvertKeyToPKCS8(t, testRSAKey)


Actually, would it make sense to convert these to a table type test with expectations, etc? The reason I ask is it'd also be nice to test that the function fails with the right error if the wrong password is provided, and also if a PKCS1 key is passed instead.

It'd also be nice to run ParsePKCS8ToTufKey on a key that is not encrypted, as well as just unmarshaling it as an asn1 structure.

cyli · 2017-07-13T23:09:03Z

tuf/utils/pkcs8.go

+
+		key, err := parsePKCS8ToTufKey(encryptedKey)
+		if err != nil {
+			return nil, errors.New("pkcs8: incorrect password")


Non-blocking: I wonder if it makes sense to return x509.IncorrectPasswordError instead? On the other hand, maybe it makes sense to distinguish between attempting to decrypt a PKCS8 key vs a PKCS1 key.

There are no usages of x509.IncorrectPasswordError in notary that why I went with custom error messages as other errors in the original package were the same. Moreover, I do agree with you that it would be better to keep the error different to distinguish between PKCS#8 and PKCS#1.

umayr · 2017-07-17T18:42:19Z

@cyli I have fixed most of the problems you found so far. I had to rewrite all the tests for PKCS#8, as you said it could have been more simplified. Moreover, I removed the functionality that was causing tests to rerun in an emulated environment for the sake of sane readability. I left a few comments there as well. Let me know if there's anything else.

@riyazdf I might have to rebase it again once all feedback is incorporated, so along with that, I'll do some squashing. There are way to many unnecessary commits in this branch that require clean up.

endophage · 2017-07-19T17:10:55Z

@cyli @riyazdf are you guys good with the final changes?

endophage · 2017-07-19T17:12:05Z

p.s. this is a larger and important change and we're over our 80% testing requirement. I'm OK to let the -0.16% pass here to get this wrapped up.

riyazdf

final changes LGTM - -0.16% cov change also ok with me

cyli · 2017-07-19T18:45:38Z

tuf/utils/pkcs8.go

+		mode := cipher.NewCBCDecrypter(block, iv)
+		mode.CryptBlocks(encryptedKey, encryptedKey)
+
+		key, err := parsePKCS8ToTufKey(encryptedKey)


I just wanted to double-check - in the swarmkit version of the PKCS8 decrypting code, an extra bit of padding is removed before the der can be parsed: https://github.com/docker/swarmkit/pull/2246/files

Is that also necessary here? I didn't understand what the difference between that implementation and this implementation was, so I just wanted to make sure we weren't missing something else.

Clearly, the keys can be encoded and decoded to PKCS#8 and work just fine. It unmarshals into a go ASN.1 data structure and can be used as a key just fine.

The only reason I ask about the padding is because if there is extra padding in the DER bytes or other extra bytes, it may be possible it could affect the key ID, since the DER bytes are serialized as part of the key definition, which is used to generate the key ID. I just wanted to be careful here in case at some point if pkcs8.go is contributed upstream to CFSSL or golang, and processing the padding changes, our key IDs may change.

Is that also necessary here? I didn't understand what the difference between that implementation and this implementation was, so I just wanted to make sure we weren't missing something else.

The padding is due to PKCS#5 algorithm, in swarmkit, unencrypted keys are being saved to file system, while in notary we don't do that. All keys are saved as encrypted PEM files, so remove padding doesn't matter as ASN1's unmarshal function discards the padding.

The only reason I ask about the padding is because if there is extra padding in the DER bytes or other extra bytes, it may be possible it could affect the key ID, since the DER bytes are serialized as part of the key definition, which is used to generate the key ID.

That's a valid concern but as far as I'm aware, key ID is derived from public key bytes which remain same in both cases (padding or no padding).

I wrote following snippet to verify if key ID remains same in both case i.e. with or without padding.

// 2048 bit rsa key generated with openssl rsa := []byte(`-----BEGIN RSA PRIVATE KEY----- MIIEowIBAAKCAQEA5Tw9iGIe13B4iEXjFkpqOoBEOIt4FuMu5YtTkqVP89qbBzbV gUvcqSH5bH8gr0ARPrIiYL03lh0oSbbKkcsBZm6wzXBBArTnjsStYrnwUEjbhCRy wdDKB8Wnb9a10Hx9O0R1sPmFUNIyTHgLF0AhnqzuElxwX+s04CRO0YNzgRdKCyXP YR0+0Plokyt3bJWxcYtDb82WYmWLg6wOqXfIhxx9u8/VigZib8SV8n/2jSyufue/ T5AmKH+d2DJqaM+9/fKcxxXyALAGWqLImHvJesg+CJ0YN4YQtK6XOBIwOlMI2dq9 8DD2q8j73ek88+1wwtiCrolyrCE9rG82Qp6uKwIDAQABAoIBAQDiSy2TTQmVM/pI zHT4tE1ZovW1vDi4n1zUTU4bHgZnfA4+eOsb0H/slxVa/cSOPHjJo6A8T45oKjBz VHd0pSqkq/DtXTtQKLXOQmnHZDbPipzpPfHrUumd8ojv5s1lGv+lWle0dR4eQuJU isyoDy8hLSV4CrkEd3SjMDR+r31+mFi+HBeXTfgTloGk3KGpS1iK5eb1N3tqny9E pKkyVd0jVdcnG7Hmb4e8AEZ03oyY3gPp7H6+HyBi2gOG/UkDqTgcfpBEL+X6j6GK TaLqJes3rfhwHM6QJPYG4rHEgkZbGYflC6zRlMhxFj7YHqBQR0C0SJcDJTUdenUv 1+gkO+rBAoGBAPLV+A+c7dd3REFAz5xnWdagCjDy/hknaSfminZGinpc5SDKF5pW rox4ncRfQdegBMGFYRC4qtP+qFnTiwXIIov/dCydB/fG6wSHEtWMNEqoElgg7GKE /GfyyQa+EKCISSqiGZJdtWs5oway0NJmsYj+mey12OW2O5pMxCBOVnS9AoGBAPGp hq3eDrzIeggYh9TK31lyw8N2Y/vYVhgRazwtXoIjjoh7oejzVFxktk3uI12rnkSA yhsohUiIQq4XtRmZi4/cySiUatCvAA9+eZRuB2cncsF6E8wnRSnVlXDrQR1dhKYZ CMy+EPOqP1iKzPqW9Dub0KHkEKgMFuJL1Ocuq8EHAoGAT5B965eucjeYvIyguY0/ aUwcqrcjPFimYrVtzp0ESF1hkaHFpAMcw9fNUYcoip7akBHEoPbwue8pd+0REv7l GtvDU87Lyop5fI1JRYDfrUpuYEPBT/JkxSZOaZAi4IZm+roMCOH9TDmhSdOTCi0t pEZPZriabYGM+9yFMlSYjT0CgYAE9phfhcoDspJfDDHt+uSvb535q7rN91l9UQ5E xCHGxngs8W0X3beD1kWwQA3V5Kpxebus5x4lqTbFYJNFnVOydrRi8vlmo9F6f/x7 Qe9A/RxsQNebP8s52rshAJPryHnq92So39Z1Sq/WX7NqCiOl/cNBL+/vo5/sseYD gX/hjQKBgChdkqRzFgYc4cR4FikhaHyRqDvdC8L/yHHV/uxRHPauj/zJdbsWFyty 0z19DkplcRT8mvbtUblkkqCsuuqx67uRI98OaUlxtaw2t0+98eUQcCw8HXhCbq3w 8+3QLnkDOYfWtS1wxVDw5MfamMqFjEVNEZV31Yir+4H7oelwrcfF -----END RSA PRIVATE KEY-----`) // Encrypted PKCS#8 of above key pkcs8 := []byte(`-----BEGIN ENCRYPTED PRIVATE KEY----- MIIFHzBJBgkqhkiG9w0BBQ0wPDAbBgkqhkiG9w0BBQwwDgQILOsAINm0ttYCAggA MB0GCWCGSAFlAwQBKgQQQrDl7iEE0vt0bouRL6ykpwSCBND8XKxFdIB2O8of4BMx UBWVIzBr4FW6goxoZxJGn4ms4YLfZ4xOpqQadLh6aT8TABEc86DQG5Vnbx9ATFmD tb8aZHGy391lBNTmIWT4Qf9+YNVCqCbNpR75hd1T3oCvWk0xnCFz3p0QSgX4qLBQ PxtyTHQoNgAigdSyz1b3sJEWKsgjFJkYNHVT2RQa6PopDf2GByTCb5A78yX2y+1o PeH9NQISwbluCGyOVi8UXP76nYTY5V7Iwg1XT5CQGE3ESYOBNiveNEZS9Lm18B3j 82FGewghIKGDlb87eAWsVKV6R7sHWGKGA+X/NjOFZSutJqI0FmyDDCQ9U3kULD0d DgRxipiwVGdWp6GAOrqf9GcnW2gzBqhfS1zldUfeT5wnZWZmGSjJyOKAjCCuVeSc RY0qx+wAiT01DHSDiytTqzxj5T47i406MFxOXC34+Urm801wh5uqShG9Vb4XveIH zBcn44cZl3FSaXgGa2GcD6Il3jcKtIOyV7i1t1QU+V7RTtp9JkbvJK8D6Iir4av4 IiUS5ob1zVcXYTr71aLvGBOVafyIh0tVEUHlNQpJqs8qH+QMCJ88Frbq5rbim6iR XnRShExZDQFoveQOn/Y2F+jxy8vSbfWfKlOAdVqFzFmgvICQhjWBUrOcDlErp71m 1WtND0c4nbRyUT3qocNrAKXe7Xm6fFYgEcWr+YJNyPk7W9oIE0HpxPKVXZZ6giKY nMhQuylcI3YuJPcB3psnsUm6AM6pKXgWCp8AJZx+z1qGIR+5FoU5EU1oONS6PcIT ngAMVW8+wDE4oDsUmTV3RLY/oEwdTLgc5ABY/jrh7Pl3PvqyItXWP4uK3I4nGxIP 6xY5DgmfKoxfwakoOcG5QA5Sa9NqH29HguX2fg9U24dLq0uH/xa+3P9NfC7JOHEv NSXA2vY3gBO9F/00ZNxqvtpXLEJKeV/K4UO+l8PpnSwLBihBQgwrqEaE2njZq/vY kIogIbJGHbGMV5kmHC6O1zh713b8Eapdzrbhpcy8N8+GkymQBvpaCdZAV7EsCKf2 GlD4R1m5bYhR5i+sv0iJyC/p1y9WVoXupXDkYpgjV59U1woY2GR+6J03wka8t9Nw jT7QsIEVl504ZfaOQuY2GrQwIbUzVrqiYtW+6c0wPiZCaxsLiGtGu0ctaoYJ2BJp B/2uP8RT6wPKcAmHHnDps1aci7MtCjDpzomYsrJ0lWbleJrMI+gn8FReJYyCLTUC wlNl+NPvJCchqwR4U1Xgq0xsxqNpS9enojORxWrEp9DwDM2TPdltwyxWQ/vEszVM 0ScrgDgkDgOFkmz8joO6Q6rTcmervmgUggkWI+8NR8zdC4MQBUCHuOCgwi5K6+8Y VtseiYJBJmINlahUsh44eAFWLvQoWG347bAdC06vtoOSfaqpO1r0oCp/B4/qOlDF lFsO20AHshjrLG4d1Nvpws6VdlvfdmPaw8ie05CXg+ynJdSyVc5qVWkEfx9ZuJ35 97O9afWMpeSmMtVKzXN7Wib8/QIYlmcMf+KPvWzJD0F61XqKjzgoC2i6hSLjwwBy N/3KiRDgdkjwJBvSdUWtocko+X3ndGTOEWaprAsWLZn0srXueG7CEtTbHYv43isJ Eug1Iq9j39eKLfzRNKVw0IRZlg== -----END ENCRYPTED PRIVATE KEY-----`) // simply try to parse it with our version pkcs8 package key, _ := ParsePEMPrivateKey(pkcs8, "foobar") fmt.Println(key.ID()) // 759c8aeaad87972d7b7650ce3064156318347cfc869986292399132b259c6037 // parse the rsa key to *rsa.PrivateKey block, _ := pem.Decode(rsa) rsaKey, _ := x509.ParsePKCS1PrivateKey(block.Bytes) key, _ = RSAToPrivateKey(rsaKey) fmt.Println(key.ID()) // 759c8aeaad87972d7b7650ce3064156318347cfc869986292399132b259c6037 // convert above rsa key to encrypted PKCS#8 buf, _ := ConvertTUFKeyToPKCS8(key, []byte("foobar")) key, _ = ParsePEMPrivateKey(pem.EncodeToMemory(&pem.Block{ Bytes: buf, Type: "ENCRYPTED PRIVATE KEY", }), "foobar") fmt.Println(key.ID()) // 759c8aeaad87972d7b7650ce3064156318347cfc869986292399132b259c6037

Let me know if there is any other concern regarding this.

@umayr: got it, thanks for the explanation on both points.

It would be good to make a comment to the code about how ASN1 unmarshaling behaves or to explicitly remove the padding (I think I would prefer to make it explicit).

@cyli @endophage this LGTM after that point is addressed, WDYT?

Yep, agree, thanks for the detailed explanation @umayr!

cyli · 2017-07-19T18:46:51Z

Other than the question I had about padding, this LGTM. Thanks for working on this @umayr!

endophage · 2017-07-20T20:49:55Z

@cyli @riyazdf thanks for reviewing! I'll wait on the answer about the padding before merging.

Hard forked package pkcs8 package from https://github.com/youmark/pkcs8 package. It has been further modified based on the requirements of Notary. For converting keys into PKCS#8 format, original package expected *crypto.PrivateKey interface, which then type inferred to either *rsa.PrivateKey or *ecdsa.PrivateKey depending on the need and later converted to ASN.1 DER encoded form, this whole process was superfluous here as keys are already being kept in ASN.1 DER format wrapped in data.PrivateKey structure. With these changes, package has became tightly coupled with notary as most of the method signatures have been updated. Moreover support for ED25519 keys has been added as well. Signed-off-by: Umayr Shahid <[email protected]>

Signed-off-by: Umayr Shahid <[email protected]>

There are two modes in which notary can run with. For FIPS mode, that could be switched on by setting an environment variable `GOFIPS`, only PKCS#8 keys are supported, any other type of key will throw an error. In Non-FIPS mode, private encrypted keys are supported as well, however all new keys that get generated, will be PKCS#8. Signed-off-by: Umayr Shahid <[email protected]>

cyli · 2017-07-25T20:51:48Z

Thanks again @umayr for all your patience with working through this PR!

GordonTheTurtle added the dco/no label Apr 4, 2017

umayr force-pushed the feat/pkcs8 branch from 6f92779 to ee02b39 Compare April 6, 2017 12:05

GordonTheTurtle removed the dco/no label Apr 6, 2017

umayr force-pushed the feat/pkcs8 branch from e877c7f to f899e98 Compare April 10, 2017 07:14

alyyousuf7 force-pushed the feat/pkcs8 branch from c330536 to da6488d Compare April 10, 2017 11:55

umayr changed the title ~~[WIP] Support for PKCS#8~~ Support for PKCS#8 Apr 13, 2017

umayr force-pushed the feat/pkcs8 branch from f386161 to 669a145 Compare April 27, 2017 12:52

alyyousuf7 force-pushed the feat/pkcs8 branch 2 times, most recently from a73eee2 to 5c4e43b Compare May 5, 2017 18:57

GordonTheTurtle added the dco/no label May 10, 2017

alyyousuf7 force-pushed the feat/pkcs8 branch from 5629a55 to fe2b80f Compare May 10, 2017 07:42

GordonTheTurtle removed the dco/no label May 10, 2017

alyyousuf7 force-pushed the feat/pkcs8 branch 2 times, most recently from 2072a25 to f791dd9 Compare May 10, 2017 12:41

umayr force-pushed the feat/pkcs8 branch from f791dd9 to 847a5b8 Compare May 11, 2017 11:41

GordonTheTurtle added the dco/no label May 11, 2017

umayr force-pushed the feat/pkcs8 branch from 9b170f0 to fed6266 Compare May 11, 2017 12:56

GordonTheTurtle added the dco/no label May 11, 2017

umayr force-pushed the feat/pkcs8 branch from fed6266 to 937c6ba Compare May 11, 2017 14:14

GordonTheTurtle removed the dco/no label May 11, 2017

endophage reviewed Jun 15, 2017

View reviewed changes

endophage reviewed Jun 23, 2017

View reviewed changes

endophage approved these changes Jul 13, 2017

View reviewed changes

cyli mentioned this pull request Jul 13, 2017

Migrate the openssl cert generation bits in regenerateTestingCerts.sh to use cfssl #1168

Open

riyazdf reviewed Jul 13, 2017

View reviewed changes

cyli reviewed Jul 13, 2017

View reviewed changes

umayr force-pushed the feat/pkcs8 branch 2 times, most recently from f67189c to 7bf996d Compare July 19, 2017 15:44

riyazdf approved these changes Jul 19, 2017

View reviewed changes

cyli reviewed Jul 19, 2017

View reviewed changes

umayr added 4 commits July 25, 2017 10:17

Update TUF utility functions to handle PKCS#8 keys

3939e19

Signed-off-by: Umayr Shahid <[email protected]>

Add function to check if running in FIPS mode

09f0e03

Signed-off-by: Umayr Shahid <[email protected]>

umayr force-pushed the feat/pkcs8 branch from 7bf996d to c9c91a3 Compare July 25, 2017 05:18

cyli merged commit 4995826 into notaryproject:master Jul 25, 2017

riyazdf mentioned this pull request Jul 25, 2017

pkcs8: add comment about padding during unmarshal #1201

Merged

cyli mentioned this pull request Mar 3, 2018

Unable To Publish Images Using Delegation Keys w/ Docker Content Trust - Del. Passphrase Not Accepted #1281

Open

Support for PKCS#8 #1130

Support for PKCS#8 #1130

Conversation

umayr commented Apr 4, 2017 • edited Loading

docker-jenkins commented Apr 4, 2017

ecordell commented Apr 6, 2017

endophage commented Apr 6, 2017

umayr commented Apr 13, 2017

umayr commented May 2, 2017

endophage commented May 2, 2017

endophage commented May 4, 2017

endophage commented May 8, 2017

GordonTheTurtle commented May 10, 2017

riyazdf commented May 12, 2017

endophage Jun 15, 2017 • edited Loading

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

Choose a reason for hiding this comment

endophage Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

endophage Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

endophage left a comment

Choose a reason for hiding this comment

riyazdf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umayr Jul 13, 2017 • edited Loading

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

umayr Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli Jul 13, 2017 • 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

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

Choose a reason for hiding this comment

umayr commented Jul 17, 2017

endophage commented Jul 19, 2017

endophage commented Jul 19, 2017

riyazdf left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli Jul 19, 2017 • edited Loading

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 Jul 19, 2017

endophage commented Jul 20, 2017

cyli commented Jul 25, 2017

umayr commented Apr 4, 2017 •

edited

Loading

endophage Jun 15, 2017 •

edited

Loading

endophage Jun 23, 2017 •

edited

Loading

endophage Jun 23, 2017 •

edited

Loading

umayr Jul 13, 2017 •

edited

Loading

umayr Jul 13, 2017 •

edited

Loading

cyli Jul 13, 2017 •

edited

Loading

riyazdf left a comment •

edited

Loading

cyli Jul 19, 2017 •

edited

Loading