abstracting pkcs11 package and adding another vendor besides yubikey #1352

florianeichin · 2018-05-08T09:25:45Z

Issue Description

The only hardware that notary supports as trustmanager is Yubikey. We would like to extend notary to support other devices that work according to PKCS#11 -- and for our case add support for devices that are used through the opencryptoki library. Our suggestion is to split this up into two steps:

Create an abstraction for pkcs11 (and move Yubikey in there).
add opencryptoki support

So essentially this results in a file structure like the following:

├─trustmanager 
│     ├─pkcs11
│     │   ├─yubikey
│     │   └─ opencryptoki

Other PKCS#11 exploiters can hook in at the level below PKCS#11. To keep enablement of other PKCS#11 trustmanagers simple in general, we think it would make sense, to abstract some of the yubikey files and code pieces to reuse them.
The abstraction could go that far, that just the following yubikeystore functions have to stay hardware specific, everything else could be reused by opencryptoki and other pkcs11 implementations:

getECDSAKey(),
addECDSAKey(),
yubiRemoveKey(),
Sign(),
hardwareListKey(),
listObjects(),
SetupHSMEnv()

PR Description

This PR addresses #1289

First commit abstracts the yubikey package into a hardwareindependent pcks11 package and a hardwarespecific yubikey package.
Second commit adds opencryptoki as another hardware implementation.

docker-jenkins · 2018-05-08T09:25:47Z

Can one of the admins verify this patch?

justincormack · 2018-05-15T12:40:43Z

trustmanager/pkcs11/common/hardwareprivatekey.go

+	hpk.libLoader = loader
+}
+
+// CryptoSigner returns a crypto.Signer tha wraps the HardwarePrivateKey. Needed for


fixed that and pushed the fix

justincormack · 2018-05-16T17:49:26Z

If you rebase this it should fix CI now...

endophage · 2018-05-16T23:23:13Z

client/client.go

@@ -69,7 +70,7 @@ func NewFileCachedRepository(baseDir string, gun data.GUN, baseURL string, rt ht
 	if err != nil {
 		return nil, err
 	}
-
+	pkcs11.Setup()


could/should this be done in an init() function in the pkcs11 package?

You are absolutely right. A non_pkcs11 package should not contain any relation to a pkcs11 package it. I added an init() function in the pkcs11 package.
The init() just calls Setup(), because Docker for example uses another entrypoint into notary while building/pushing. It uses client/repo(_pkcs11).go as entry. If not calling Setup() there, the whole pkcs11 package would not be imported just pkcs11/common.
So init of pkcs11 would not be called.
Therefore in repo_pcks11.go the call for Setup() remains.

endophage · 2018-05-16T23:30:41Z

trustmanager/pkcs11/common/hardwareprivatekey.go

+}
+
+// hardwareSigner wraps a HardwarePrivateKey and implements the crypto.Signer interface
+type hardwareSigner struct {


What's the need for the extra wrapper? crypto.Signer is only implemented by virtue of the embedded HardwarePrivateKey?

I actualy don't know in that case. Took this from the original yubikey implementation. Seems to be implemented like this since the first yubikey enhanced release. So I don't know what was the intention to do it like that. Didn't want to touch things like this, to keep this restructuring as simple as possible.

endophage · 2018-05-16T23:31:15Z

trustmanager/pkcs11/common/hardwareprivatekey.go

+)
+
+const (
+	// SigAttempts defines maximum attempts to sign an image before aborting


Notary is not Docker/container specific, shouldn't reference "image"

changed "image" to "artifact"

endophage · 2018-05-16T23:33:51Z

trustmanager/pkcs11/common/hardwarestore.go

+		SlotID: slot,
+		KeyID:  keyID,
+	}
+	//err = hardwareKeyStore.AddECDSAKey(ctx, session, privKey, slot, s.PassRetriever, role)


remove commented out line.

removed the line

endophage · 2018-05-16T23:35:06Z

trustmanager/pkcs11/common/universal.go

+)
+
+// SetKeyStore sets up the to be used keystore
+func SetKeyStore(ks HardwareSpecificStore) {


This appears to be restricting us to only being able to access a single hardware device at a time. Is that correct? If yes, how can we not incur that limitation.

yes this is right, depending on which libs are found (opencryptoki and yubikey right now) a hardware device is chosen. Yet it would choose one of the libs and performing every crypto operation with it. What would be your suggestion for breaking down this limitation? And what do you mean with "at a time"? You would like to have a selection, which hardware should bes used? Didn't think about it (and it's design implications) too much yet, but with further development there could be a mechanism that chooses one if just one is present and asks which to use if more than one is found.

Ideally we would instantiate a key store for every recognized hardware device attached to the system. You might run into cases where say, 2 yubikeys were attached, one having the root or targets key, one having your delegation key.

I don't really know, if and how this is supported by the yubikey library. As we never tell them which usb/device to use, I don't think that the yubicopkcs11 lib supports choosing a device. I think this is a bigger piece of work, that needs an own story and pr.

resolves notaryproject#1289 Signed-off-by: Florian Eichin <florian.eichin@gmail.com>

florianeichin · 2018-10-16T07:39:22Z

As I'm leaving the team, developing the code leading to this PR, https://github.com/jschintag will continue development

florianeichin force-pushed the opencryptoki branch 6 times, most recently from cc73875 to e537e75 Compare May 9, 2018 09:40

justincormack reviewed May 15, 2018

View reviewed changes

florianeichin force-pushed the opencryptoki branch 3 times, most recently from 1d82cbf to 317691c Compare May 16, 2018 07:44

endophage reviewed May 16, 2018

View reviewed changes

refactored trustmanager to create abstraction

dad6be0

resolves notaryproject#1289 Signed-off-by: Florian Eichin <florian.eichin@gmail.com>

florianeichin force-pushed the opencryptoki branch 2 times, most recently from a7e8427 to 3f8397b Compare May 17, 2018 06:27

added opencryptoki as another trustmanager

94a7b8d

resolves notaryproject#1289 Signed-off-by: Florian Eichin <florian.eichin@gmail.com>

florianeichin force-pushed the opencryptoki branch from 3f8397b to 94a7b8d Compare May 17, 2018 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

abstracting pkcs11 package and adding another vendor besides yubikey #1352

abstracting pkcs11 package and adding another vendor besides yubikey #1352

florianeichin commented May 8, 2018 •

edited

Loading

docker-jenkins commented May 8, 2018

justincormack May 15, 2018

florianeichin May 16, 2018

justincormack commented May 16, 2018

endophage May 16, 2018

florianeichin May 17, 2018

endophage May 16, 2018

florianeichin May 17, 2018

endophage May 16, 2018

florianeichin May 17, 2018

endophage May 16, 2018

florianeichin May 17, 2018

endophage May 16, 2018

florianeichin May 17, 2018

endophage May 18, 2018

florianeichin May 22, 2018

florianeichin commented Oct 16, 2018

abstracting pkcs11 package and adding another vendor besides yubikey #1352

Are you sure you want to change the base?

abstracting pkcs11 package and adding another vendor besides yubikey #1352

Conversation

florianeichin commented May 8, 2018 • edited Loading

Issue Description

PR Description

docker-jenkins commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justincormack commented May 16, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florianeichin commented Oct 16, 2018

florianeichin commented May 8, 2018 •

edited

Loading