Importing RSA and EC keypairs #1050

venafi-iw · 2021-11-14T06:40:14Z

Summary

This PR provides support for importing RSA and EC keypairs that were generated using 3rd party utilities such as OpenSSL. Imported keypairs are converted to Cosign keypairs.

Ticket Link

This PR is in relation to #549

Release Notes

Added support for importing EC and RSA keypairs generated using 3rd party utilities such as OpenSSL
New command line option import-key-pair
added instructions on how to import keypairs

dlorenc · 2021-11-14T13:17:06Z

Looks like DCO is missing! You'll need to use the git commit -s flag

Signed-off-by: Ivan Wallis <[email protected]>

SantiagoTorres

Overall, it looks in a good direction, but I wonder how this fits the broader codebase (I think there's a lot of possibly repeated code).

I also feel there are a couple of minor fix/tweaks that may be making the code slightly harder to review. I wonder if we could split those into a smaller PR...

cmd/cosign/cli/importkeypair/import_key_pair.go

SantiagoTorres · 2021-11-15T17:09:45Z

cmd/cosign/cli/importkeypair/import_key_pair.go

+	}
+	fmt.Fprintln(os.Stderr)
+	fmt.Fprint(os.Stderr, "Enter password for private key again: ")
+	pw2, err := term.ReadPassword(0)


I'm not entirely fond of pw2 when this should be a confirmation. I'm not sure what the golang guidelines are for variable naming, but maybe something more explicit would make the code more readable.

Ok, I can update the variable to something more explcit such as confirmpw

small ping on this comment

cmd/cosign/cli/importkeypair/import_key_pair.go

cmd/cosign/cli/importkeypair/import_key_pair_test.go

pkg/cosign/keys.go

cmd/cosign/cli/import_key_pair.go

Signed-off-by: Ivan Wallis <[email protected]>

asraa

Thanks! looking really good so far :D appreciate all the changes

asraa · 2021-11-16T15:11:33Z

pkg/cosign/keys.go

+
+}
+
+func MarshallKeyPair(keypair Key, pf PassFunc) (*Keys, error) {


nit: spelling MarshalKeyPair

could you share this code with GenerateKeyPair? and given that it's only called within this function, maybe it should be made private.

made changes.

pkg/cosign/keys.go

test/import_test.sh

Signed-off-by: Ivan Wallis <[email protected]>

asraa

Sorry for the delay, taking a review! Could you check on the test failures as well?

?   	github.com/sigstore/cosign/pkg/blob	[no test files]
--- FAIL: TestImportPrivateKey (0.12s)
    keys_test.go:158: unexpected error importing key: parsing error
FAIL

Also, running and committing the change from go run -tags pivkey,pkcs11key,cgo ./cmd/help/ will fix the doc gen CI check

cmd/cosign/cli/importkeypair/import_key_pair.go

pkg/cosign/keys.go

pkg/cosign/keys_test.go

test/e2e_test.go

test/import_test.sh

Signed-off-by: Ivan Wallis <[email protected]>

asraa

Thanks! Could you just please address the lint comments and update the branch and this would LGTM!

pkg/cosign/keys.go

venafi-iw · 2021-12-05T00:27:50Z

#1050 (comment)

Your code suggestion doesn't compile.

Signed-off-by: Ivan Wallis <[email protected]>

asraa · 2021-12-06T14:28:03Z

Your code suggestion doesn't compile.

Was hoping you would take away the gist of what I was saying. I pulled your branch and completed it for you:

diff --git a/pkg/cosign/keys.go b/pkg/cosign/keys.go
index 98f92f9..d592b0e 100644
--- a/pkg/cosign/keys.go
+++ b/pkg/cosign/keys.go
@@ -71,20 +71,20 @@ func ImportKeyPair(keyPath string, pf PassFunc) (*KeysBytes, error) {
                return nil, errors.New("invalid pem block")
        }
 
+       var pk crypto.Signer
        switch p.Type {
        case RSAPrivateKeyPemType:
-               pk, err := x509.ParsePKCS1PrivateKey(p.Bytes)
+               pk, err = x509.ParsePKCS1PrivateKey(p.Bytes)
                if err != nil {
                        return nil, fmt.Errorf("parsing error")
                }
-               return marshalKeyPair(Keys{pk, pk.Public()}, pf)
        default:
-               pk, err := x509.ParseECPrivateKey(p.Bytes)
+               pk, err = x509.ParseECPrivateKey(p.Bytes)
                if err != nil {
                        return nil, fmt.Errorf("parsing error")
                }
-               return marshalKeyPair(Keys{pk, pk.Public()}, pf)
        }
+       return marshalKeyPair(Keys{pk, pk.Public()}, pf)
 }
 
 func marshalKeyPair(keypair Keys, pf PassFunc) (*KeysBytes, error) {

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw · 2021-12-06T18:05:07Z

ImportKeyPair function updated.

pkg/cosign/keys_test.go

Signed-off-by: Ivan Wallis <[email protected]>

pkg/cosign/keys_test.go

Signed-off-by: Ivan Wallis <[email protected]>

… rsa-import

asraa · 2021-12-07T18:25:36Z

cc @lukehinds @dlorenc do you want to give this a merge? LGTM just want a final pass in case I'm missing something cosign-style-y

lukehinds · 2021-12-07T19:19:30Z

cc @lukehinds @dlorenc do you want to give this a merge? LGTM just want a final pass in case I'm missing something cosign-style-y

looks goof from a cursory view, but there are a quite a few CI failures.

dlorenc · 2021-12-14T13:53:54Z

Yeah looks like a merge conflict somewhere, otherwise should be good to go.

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw · 2021-12-28T19:43:19Z

@dlorenc Made some code updates and looks good for next steps.

dlorenc · 2021-12-31T19:58:26Z

This looks good to me, there's one more conflict though :(

Once that gets fixed i'll merge right away so you don't keep getting hit!

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw · 2022-01-01T01:42:42Z

Conflict resolved

dlorenc · 2022-01-01T13:38:49Z

Thanks for dealing with all the delays and merge conflicts!

cpanato · 2022-01-08T11:02:55Z

IMPORT.md

+### Sign a container with imported keypair
+
+```shell
+$ cosign sign --key import import-cosign.key dlorenc/demo


this import is needed?

or should be just cosign sign --key import-cosign.key dlorenc/demo ?

documentation needs to be updated as it should be:

cosign sign --key import-cosign.key dlorenc/demo

#1290

venafi-iw added 3 commits November 14, 2021 14:58

initial cmd structure

3b79cee

Signed-off-by: Ivan Wallis <[email protected]>

import rsa and ec keypairs into cosign

28e8b78

Signed-off-by: Ivan Wallis <[email protected]>

added IMPORT.md for instructions

c55d69e

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw force-pushed the rsa-import branch from 8db9819 to c55d69e Compare November 14, 2021 22:58

whitespace fix

e899afd

Signed-off-by: Ivan Wallis <[email protected]>

SantiagoTorres requested changes Nov 15, 2021

View reviewed changes

asraa reviewed Nov 15, 2021

View reviewed changes

cmd/cosign/cli/import_key_pair.go Outdated Show resolved Hide resolved

PR sigstore#1050 fixes

34b5bec

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw requested a review from asraa November 16, 2021 06:40

asraa suggested changes Nov 16, 2021

View reviewed changes

fixes and added more tests

db13da2

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw requested a review from asraa November 17, 2021 06:15

asraa reviewed Nov 18, 2021

View reviewed changes

asraa suggested changes Nov 18, 2021

View reviewed changes

venafi-iw mentioned this pull request Nov 19, 2021

Centralize password prompt logic for key generation/import functionality #1078

Closed

fixes and test updates

ddb2235

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw requested a review from asraa November 19, 2021 01:24

SantiagoTorres approved these changes Nov 30, 2021

View reviewed changes

asraa reviewed Dec 4, 2021

View reviewed changes

pkg/cosign/keys.go Outdated Show resolved Hide resolved

lint updates

b2f2056

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw requested a review from asraa December 5, 2021 00:43

importkeypair update

4f21d45

Signed-off-by: Ivan Wallis <[email protected]>

asraa reviewed Dec 6, 2021

View reviewed changes

pkg/cosign/keys_test.go Outdated Show resolved Hide resolved

venafi-iw and others added 2 commits December 6, 2021 10:26

TestImportPrivateKey update

1e0f967

Signed-off-by: Ivan Wallis <[email protected]>

Merge branch 'main' into rsa-import

07ede86

venafi-iw requested a review from asraa December 6, 2021 18:41

asraa suggested changes Dec 6, 2021

View reviewed changes

pkg/cosign/keys_test.go Outdated Show resolved Hide resolved

pkg/cosign/keys_test.go Outdated Show resolved Hide resolved

asraa reviewed Dec 6, 2021

View reviewed changes

pkg/cosign/keys_test.go Show resolved Hide resolved

venafi-iw added 2 commits December 6, 2021 16:25

TestImportPrivateKey updates

86c3ada

Signed-off-by: Ivan Wallis <[email protected]>

Merge branch 'rsa-import' of https://github.com/venafi-iw/cosign into…

8cde326

… rsa-import

venafi-iw requested a review from asraa December 7, 2021 00:30

asraa approved these changes Dec 7, 2021

View reviewed changes

asraa self-assigned this Dec 7, 2021

venafi-iw and others added 3 commits December 28, 2021 10:37

Merge branch 'sigstore:main' into rsa-import

f38a372

linting fixes

2dfbb28

Signed-off-by: Ivan Wallis <[email protected]>

E2E fix

660b394

Signed-off-by: Ivan Wallis <[email protected]>

venafi-iw and others added 2 commits December 31, 2021 12:56

conflict fix

a37d984

Signed-off-by: Ivan Wallis <[email protected]>

Merge branch 'main' into rsa-import

6533f33

dlorenc approved these changes Jan 1, 2022

View reviewed changes

dlorenc merged commit 9a27e1f into sigstore:main Jan 1, 2022

github-actions bot added this to the v1.5.0 milestone Jan 1, 2022

cpanato reviewed Jan 8, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Importing RSA and EC keypairs #1050

Importing RSA and EC keypairs #1050

venafi-iw commented Nov 14, 2021

dlorenc commented Nov 14, 2021

SantiagoTorres left a comment

SantiagoTorres Nov 15, 2021

venafi-iw Nov 15, 2021

asraa Nov 22, 2021

asraa left a comment

asraa Nov 16, 2021

asraa Nov 16, 2021

venafi-iw Nov 17, 2021

asraa left a comment

asraa left a comment

venafi-iw commented Dec 5, 2021

asraa commented Dec 6, 2021

venafi-iw commented Dec 6, 2021

asraa commented Dec 7, 2021

lukehinds commented Dec 7, 2021

dlorenc commented Dec 14, 2021

venafi-iw commented Dec 28, 2021

dlorenc commented Dec 31, 2021

venafi-iw commented Jan 1, 2022

dlorenc commented Jan 1, 2022

cpanato Jan 8, 2022 •

edited

Loading

venafi-iw Jan 8, 2022 •

edited

Loading


		}

		func MarshallKeyPair(keypair Key, pf PassFunc) (*Keys, error) {

Importing RSA and EC keypairs #1050

Importing RSA and EC keypairs #1050

Conversation

venafi-iw commented Nov 14, 2021

Summary

Ticket Link

Release Notes

dlorenc commented Nov 14, 2021

SantiagoTorres left a comment

Choose a reason for hiding this comment

SantiagoTorres Nov 15, 2021

Choose a reason for hiding this comment

venafi-iw Nov 15, 2021

Choose a reason for hiding this comment

asraa Nov 22, 2021

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

asraa Nov 16, 2021

Choose a reason for hiding this comment

asraa Nov 16, 2021

Choose a reason for hiding this comment

venafi-iw Nov 17, 2021

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

venafi-iw commented Dec 5, 2021

asraa commented Dec 6, 2021

venafi-iw commented Dec 6, 2021

asraa commented Dec 7, 2021

lukehinds commented Dec 7, 2021

dlorenc commented Dec 14, 2021

venafi-iw commented Dec 28, 2021

dlorenc commented Dec 31, 2021

venafi-iw commented Jan 1, 2022

dlorenc commented Jan 1, 2022

cpanato Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

venafi-iw Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

cpanato Jan 8, 2022 •

edited

Loading

venafi-iw Jan 8, 2022 •

edited

Loading