Revamp pkcs11key #330

jsha · 2015-09-17T22:46:45Z

Fixes #329
Fixes #325
Fixes #322
Incorporates #321

pkcs11key used to log in and log out for each Sign request. This created a
performance problem, because logins are slow. It also created a concurrency
problem.

Logged in / logged out state is a property of a PKCS#11 application, not a
session. So it was possible for two Sign operations on separate threads to log
in at the same time, and for one Sign operation to finish and call Logout before
the other Sign operation called SignInit. This would result in a
CKR_OBJECT_HANDLE_INVALID error.

To fix both of these issues, I made the session a persistent property of a
PKCS11Key, and protected it with a mutex. Now each PKCS11Key maintains a single
session throughout its lifetime, increasing signing performance.

Also, some of the field names for PKCS11Key were incorrect, and there was no
provision to select by token label, only slot label. In general, token label is
more unique, and the better choice. This change incorporates the elements of
#321 fixing that. That is, it renames
Token to SlotDescription, Label to PrivateKeyLabel, and adds a new field
TokenLabel. Note for people updating configs: In your configs, don't rename
Token to TokenLabel. The value you currently have in Token is actually a
SlotDescription, and should be named such. You should add a field named
TokenLabel, whose value you can find with pkcs11-tool --list-slots.

I also changed where the PKCS11Key Config object is defined, to make it more
straightforward to share between different components, and added object label
support to pkcs11uri, so private keys can be selected by label.

Fixes cloudflare#329 Fixes cloudflare#325 Fixes cloudflare#322 Incorporates cloudflare#321 pkcs11key used to log in and log out for each Sign request. This created a performance problem, because logins are slow. It also created a concurrency problem. Logged in / logged out state is a property of a PKCS#11 application, not a session. So it was possible for two Sign operations on separate threads to log in at the same time, and for one Sign operation to finish and call Logout before the other Sign operation called SignInit. This would result in a CKR_OBJECT_HANDLE_INVALID error. To fix both of these issues, I made the session a persistent property of a PKCS11Key, and protected it with a mutex. Now each PKCS11Key maintains a single session throughout its lifetime, increasing signing performance. Also, some of the field names for PKCS11Key were incorrect, and there was no provision to select by token label, only slot label. In general, token label is more unique, and the better choice. This change incorporates the elements of cloudflare#321 fixing that. That is, it renames Token to SlotDescription, Label to PrivateKeyLabel, and adds a new field TokenLabel. Note for people updating configs: In your configs, don't rename Token to TokenLabel. The value you currently have in Token is actually a SlotDescription, and should be named such. You should add a name field TokenLabel, whose value you can find with pkcs11-tool --list-slots. I also changed where the PKCS11Key Config object is defined, to make it more straightforward to share between different components, and added object label support to pkcs11uri, so private keys can be selected by label.

jmhodges · 2015-09-17T23:16:18Z

crypto/pkcs11key/pkcs11key.go

-			// Login
-			if err = ps.module.Login(session, pkcs11.CKU_USER, ps.pin); err != nil {
-				return session, err
+			if slotInfo.SlotDescription != ps.slotDescription {


Isn't slot description just some made up stuff from the manufacturer and not ever guaranteed to be unique? This seems like an unnecessary complication for something that won't be useful for folks. They'll just think they need to have it when they don't.

jcjones · 2015-09-18T01:07:33Z

crypto/pkcs11key/pkcs11key.go

+		// Note: Logged-in status is application-wide, not per session. But in
+		// practice it appears to be okay to login to a token multiple times with the same
+		// credentials.
+		if err = ps.module.Login(session, pkcs11.CKU_USER, ps.pin); err != nil {


I've done this same thing before with other PKCS11 modules and it's worked fine.

jcjones · 2015-09-18T01:15:12Z

I agree with the decision to remove SlotDescription, for what it's worth.

I'm only qualified to review the changes to crypto/pkcs11key/pkcs11key.go, but it looks like you gents have done a good job on it. The new design's mutex layout should even work alright for Encrypt operations, should that ever become useful to CFSSL.

And update the benchmark test to use it. In the process, introduce a flag allowing selection of the session count independently of GOMAXPROCS.

kisom · 2015-09-18T23:01:41Z

crypto/pkcs11key/key_pool.go

+			for j := 0; j < i; j++ {
+				signers[j].Destroy()
+			}
+			return nil, fmt.Errorf("Problem making PKCS11Key: %s", err)


As a style thing, this error should be "pkcs11key: problem making PKCS11Key".

kisom · 2015-09-18T23:30:21Z

@jsha It would be useful to have some PKCS11 docs in the top-level docs dir. Is that something that could be included with this PR? It's not strictly necessary, but I think it would be useful for people to get started.

jsha · 2015-09-18T23:41:04Z

@kisom: Sure, will do.

jsha · 2015-09-19T00:06:54Z

Okay, added docs.

kisom · 2015-09-19T00:12:02Z

Tested locally with a yubikey neo. LGTM.

If the private key has a CKA_ALWAYS_AUTHENTICATE attribute, it means we have to Login before each sign attempt, and that would be terribly slow. Note: This means that a default-setup Yubikey Neo in PIV mode will be rejected. I'm planning to see if it's possible to set the attribute to false.

jsha · 2015-09-19T03:01:22Z

@kisom: Thanks! This works great with my SoftHSM instance. I had some trouble with my Yubikey, which I tracked to the CKA_ALWAYS_AUTHENTICATE attribute on my private key object. Can you run this command and tell me if the Private Key Object has Access: always authenticate ?

pkcs11-tool --module /usr/lib/x86_64-linux-gnu/opensc-pkcs11.so  --login --list-objects    --pin <pin>

The problem I was having was that each session would succeed at signing once, and then fail on the second sign attempt. I can see now why the original version of this code did a login and logout on each sign attempt! It appears to be necessary for Yubikeys.

I just pushed an update to this branch that checks for the CKA_ALWAYS_AUTHENTICATE attribute and fails if it's present. However, this probably breaks usage with most Yubikeys. Try running the pkcs11_bench_test against your Yubikey and let me know what the result is?

My next steps is to see if I can clear the CKA_ALWAYS_AUTHENTICATE attribute, in which case we wouldn't need any special casing for Yubikeys. If I can't clear the attribute, and support for Yubikeys is important, we'll have to add some special case code that will do a logout/login after each signature when CKA_ALWAYS_AUTHENTICATE is present. I'd rather avoid the special casing because it will be messy: we would also have to make sure we disallow creation of multiple pkcs11key.Key's, because their behavior would be undefined. So let me know if you consider Yubikey support optional and it'll save me some work!

jsha · 2015-09-19T03:16:04Z

Actually, I went ahead and implemented the login/logout when CKA_ALWAYS_AUTHENTICATE is true, so Yubikeys should work properly.

kisom · 2015-09-19T05:25:03Z

@jsha I'll have some time to do more extensive testing over the weekend. Thanks again for this PR!

We need to take a global lock when doing the logout/login and when doing the sign. Moving those calls to the top of Sign allows us to do that.

jsha · 2015-09-20T21:46:37Z

@kisom: Thanks! Let me know what you find.

kisom · 2015-09-21T03:16:12Z

After jumping through a number of hoops figuring out how to stand up a new CA on a Neo-N, and a much more pleasant (in relative PKCS11 terms) time with SoftHSM, this LGTM.

Revamp pkcs11key

jsha · 2015-09-21T03:23:50Z

Excellent, thanks for the review & merge!

This pulls in the pkcs11key change from cloudflare/cfssl#330, and updates the Boulder code to match. Note: This change overwrites the local changes to our vendored CFSSL made in letsencrypt#784. That's intentional: The upstream changes in cloudflare/cfssl#330 accomplish the same thing, more cleanly.

jsha added 3 commits September 17, 2015 15:35

Add new file config.go

3147e42

Fix lint problem.

801fb2c

jmhodges reviewed Sep 17, 2015
View reviewed changes

jsha added 2 commits September 17, 2015 17:33

Move comment.

d4271e5

Remove slotdescription, per review.

28666b2

jcjones reviewed Sep 18, 2015
View reviewed changes

jsha mentioned this pull request Sep 18, 2015

backport cfssl pkcs11 API change to their repo letsencrypt/boulder#786

Closed

Measure total elapsed time and throughput in pkcs11_bench_test.go

eb5f3e9

jsha mentioned this pull request Sep 18, 2015

Fix slot ID types and use slot list from token letsencrypt/boulder#787

Closed

jsha added 3 commits September 18, 2015 15:22

Add PKCS11KeyPool

5af0909

And update the benchmark test to use it. In the process, introduce a flag allowing selection of the session count independently of GOMAXPROCS.

Fix lint issue.

6840a24

Rename PKCS11KeyPool to Pool.

e06d229

kisom reviewed Sep 18, 2015
View reviewed changes

jsha added 2 commits September 18, 2015 16:19

Rename PKCS11Key -> Key

743829b

Tweak comment.

f473667

Fix lint error.

1ea4489

Add PKCS#11 documentation.

2658002

jsha mentioned this pull request Sep 19, 2015

Change how CFSSL uses PKCS#11 keys? #335

Closed

Do login / logout to support Yubikey NEO.

9c9b8c6

jsha added 2 commits September 19, 2015 08:56

Move logout/login to top of pkcs11key.Sign

103af70

We need to take a global lock when doing the logout/login and when doing the sign. Moving those calls to the top of Sign allows us to do that.

Warn about slowness in benchmark.

4975c51

kisom added a commit that referenced this pull request Sep 21, 2015

Merge pull request #330 from jsha/pkcs11-rebooted

fed4546

Revamp pkcs11key

kisom merged commit fed4546 into cloudflare:master Sep 21, 2015

This was referenced Sep 21, 2015

Update cfssl to latest master. letsencrypt/boulder#821

Merged

CA give CKR_ATTRIBUTE_TYPE_INVALID error in production letsencrypt/boulder#849

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp pkcs11key #330

Revamp pkcs11key #330

jsha commented Sep 17, 2015

jmhodges Sep 17, 2015

jcjones Sep 18, 2015

jcjones commented Sep 18, 2015

kisom Sep 18, 2015

kisom commented Sep 18, 2015

jsha commented Sep 18, 2015

jsha commented Sep 19, 2015

kisom commented Sep 19, 2015

jsha commented Sep 19, 2015

jsha commented Sep 19, 2015

kisom commented Sep 19, 2015

jsha commented Sep 20, 2015

kisom commented Sep 21, 2015

jsha commented Sep 21, 2015

Revamp pkcs11key #330

Revamp pkcs11key #330

Conversation

jsha commented Sep 17, 2015

jmhodges Sep 17, 2015

Choose a reason for hiding this comment

jcjones Sep 18, 2015

Choose a reason for hiding this comment

jcjones commented Sep 18, 2015

kisom Sep 18, 2015

Choose a reason for hiding this comment

kisom commented Sep 18, 2015

jsha commented Sep 18, 2015

jsha commented Sep 19, 2015

kisom commented Sep 19, 2015

jsha commented Sep 19, 2015

jsha commented Sep 19, 2015

kisom commented Sep 19, 2015

jsha commented Sep 20, 2015

kisom commented Sep 21, 2015

jsha commented Sep 21, 2015