Improve session handling #3
Conversation
This adds the following functionality: - Ensure we never open more sessions than the max capacity we specify - Timeout instead of (potentially) blocking forever waiting on a session - Close idle sessions
The new test included in these changes will hang using the upstream crypto11 package - see ThalesGroup/crypto11#3.
| @@ -124,7 +125,7 @@ func GenerateDSAKeyPairOnSession(session pkcs11.SessionHandle, slot uint, id []b | |||
| pkcs11.NewAttribute(pkcs11.CKA_ID, id), | |||
| } | |||
| mech := []*pkcs11.Mechanism{pkcs11.NewMechanism(pkcs11.CKM_DSA_KEY_PAIR_GEN, nil)} | |||
| pubHandle, privHandle, err := libHandle.GenerateKeyPair(session, | |||
| pubHandle, privHandle, err := libHandle.GenerateKeyPair(session.Handle, | |||
There was a problem hiding this comment.
Is there a logic behind passing PKCS11Session to some functions and SessionHandle to others?
There was a problem hiding this comment.
All of the functions in crypto11 should operate on *PKCS11Session, whereas all of the functions in pkcs11 accept a pkcs11.SessionHandle.
| var err error | ||
| var attributes []*pkcs11.Attribute | ||
| var pub ecdsa.PublicKey | ||
| template := []*pkcs11.Attribute{ | ||
| pkcs11.NewAttribute(pkcs11.CKA_ECDSA_PARAMS, nil), | ||
| pkcs11.NewAttribute(pkcs11.CKA_EC_POINT, nil), | ||
| } | ||
| if attributes, err = libHandle.GetAttributeValue(session, pubHandle, template); err != nil { | ||
| if attributes, err = libHandle.GetAttributeValue(session.Handle, pubHandle, template); err != nil { |
There was a problem hiding this comment.
Adding to the previous comment, I think having two different "handles" (one libHandle and one session.Handle) is a little confusing.
There was a problem hiding this comment.
That's the nature of pkcs11 - there are many handles. I'm just maintaining the existing conventions, see PKCS11Object for another example:
// PKCS11Object contains a reference to a loaded PKCS#11 object.
type PKCS11Object struct {
// The PKCS#11 object handle.
Handle pkcs11.ObjectHandle
// The PKCS#11 slot number.
//
// This is used internally to find a session handle that can
// access this object.
Slot uint
}| sessionPoolMutex.Lock() | ||
| defer sessionPoolMutex.Unlock() |
There was a problem hiding this comment.
Is there a benefit to putting this in the end?
There was a problem hiding this comment.
There is a very minor (negligible here) performance impact of using defer, so for simple functions without any early returns I've developed the habit of not using it.
| maxSessions = DefaultMaxSessions | ||
| } | ||
| if token.MaxRwSessionCount > 0 && uint(maxSessions) > token.MaxRwSessionCount { | ||
| return nil, fmt.Errorf("crypto11: provided max sessions value (%d) exceeds max value the token supports (%d)", maxSessions, token.MaxRwSessionCount) |
There was a problem hiding this comment.
Use log.Fatalf instead since log is already imported.
There was a problem hiding this comment.
I'm merely returning an error here, not trying to make the application exit (which is what log.Fatal will do). Handling the error is up to the calling application.
There was a problem hiding this comment.
I understand, but if you look above, this same function is logging errors in addition to returning errors. I think it's a good idea given that this function handles hardware. What do you think?
There was a problem hiding this comment.
Current behavior (i.e.without this PR) is a bit inconsistent; I've created #4 as a placeholder for doing a cleanup at some later date.
| return f(session) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), newSessionTimeout) | ||
| defer cancel() |
There was a problem hiding this comment.
Is there a probability that this would cut off a session that is active?
There was a problem hiding this comment.
Good question, but no, the context is only used as a timeout inside Get.
There was a problem hiding this comment.
Would still like to test this in practice when we can.
|
Confirmed that also works with the Thales nShield HSM. |
|
Thanks for the PR! |
The new test included in these changes will hang using the upstream crypto11 package - see ThalesGroup/crypto11#3.
The new test included in these changes will hang using the upstream crypto11 package - see ThalesGroup/crypto11#3.
This adds the following functionality:
These changes were tested using SoftHSM2.