Look up PKCS#11 keys by key label, regardless of slot #321
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@bifurcation Looks like go vet failed; |
|
Actually a number of places where |
| @@ -158,31 +129,67 @@ func (ps *PKCS11Key) Destroy() { | |||
| } | |||
| } | |||
|
|
|||
| func (ps *PKCS11Key) openSession() (session pkcs11.SessionHandle, err error) { | |||
| // Look up the token that contains the desired private key | |||
| func (ps *PKCS11Key) openSession() (session pkcs11.SessionHandle, keyHandle pkcs11.ObjectHandle, err error) { | |||
There was a problem hiding this comment.
This function is now too long to used named returns, please use positional returns instead for clarity, per Go style.
|
Thanks, @kisom, forgot to remove some printfs from debugging. |
| // Find slot by description | ||
| slots, err := ps.module.GetSlotList(true) | ||
| if err != nil { | ||
| return | ||
| } | ||
| for _, slot := range slots { | ||
| slotInfo, err := ps.module.GetSlotInfo(slot) | ||
| // If ps.slotDescription is provided, we will only check matching slots | ||
| slotInfo, err2 := ps.module.GetSlotInfo(slot) |
There was a problem hiding this comment.
Shadowing, but now not necessary.
|
I'll be able to actually review this in a few hours. Thanks @jsha for the review, too. |
| // The name of the slot to be used, or "" to use any slot | ||
| slotDescription string | ||
|
|
||
| // The label of the token to be used, or "" to use any token |
There was a problem hiding this comment.
I'm fine with slotDescription being optional, but I think at least tokenLabel should be mandatory. Otherwise it's easy to accidentally leave out the token label, attempt to log into the wrong token, fail repeatedly, and lock that incorrect token.
jsha
added a commit
to jsha/cfssl
that referenced
this pull request
Sep 17, 2015
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.
|
@jsha Now I'm confused. Is this being pursued here or in your branch? Happy to pass this to you if you want to get it finished. |
|
@bifurcation: I wound up pulling this into a larger branch I was working on for concurrency problems. Sorry I neglected to let you know! I'll take it over from here, thanks for getting it this far. |
|
@jsha No problem. I consider the ball passed. |
jsha
added a commit
to jsha/cfssl
that referenced
this pull request
Sep 17, 2015
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.
jsha
added a commit
to jsha/cfssl
that referenced
this pull request
Sep 17, 2015
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.
Merged
jsha
added a commit
to letsencrypt/pkcs11key
that referenced
this pull request
Feb 22, 2016
Fixes cloudflare/cfssl#329 Fixes cloudflare/cfssl#325 Fixes cloudflare/cfssl#322 Incorporates cloudflare/cfssl#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/cfssl#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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In some PKCS#11 modules, slots are not distinguished by name, but it is possible to assign distinct names to private keys. This PR updates the PKCS#11 key object handle this situation better: