From 2d63281adc0bdc76409d92ad90f015350ecbc870 Mon Sep 17 00:00:00 2001 From: Brett Logan Date: Thu, 24 Sep 2020 13:46:57 -0400 Subject: [PATCH] Remove GetSessionInfo Call The GetSessionInfo call was added to the getSession function which retrieves a session from the pool. The call was used to validate the state of the session before using it to ensure it was still healthy. Unfortunately the call is exceedingly expensive with preliminary tests showing nearly a second to perform the single operation. With a transaction require requiring several signings it is possible for it to take several seconds for a transaction just to retrieve all of it signatures. This change reverts the call and in future implementations we can look into other ways of verifying if a session has become stale, perhaps by analyzing failed transaction error messages and ejecting the session on transaction failures. Signed-off-by: Brett Logan --- bccsp/pkcs11/pkcs11.go | 9 +----- bccsp/pkcs11/pkcs11_test.go | 56 +------------------------------------ 2 files changed, 2 insertions(+), 63 deletions(-) diff --git a/bccsp/pkcs11/pkcs11.go b/bccsp/pkcs11/pkcs11.go index b5b9f0a4eb4..350772f0f4b 100644 --- a/bccsp/pkcs11/pkcs11.go +++ b/bccsp/pkcs11/pkcs11.go @@ -67,14 +67,7 @@ func (csp *impl) getSession() (session pkcs11.SessionHandle, err error) { for { select { case session = <-csp.sessPool: - if _, err = csp.ctx.GetSessionInfo(session); err == nil { - logger.Debugf("Reusing existing pkcs11 session %d on slot %d\n", session, csp.slot) - return session, nil - } - - logger.Warningf("Get session info failed [%s], closing existing session and getting a new session\n", err) - csp.closeSession(session) - + return default: // cache is empty (or completely in use), create a new session return csp.createSession() diff --git a/bccsp/pkcs11/pkcs11_test.go b/bccsp/pkcs11/pkcs11_test.go index 29a5bd44ce7..812d79f42be 100644 --- a/bccsp/pkcs11/pkcs11_test.go +++ b/bccsp/pkcs11/pkcs11_test.go @@ -182,38 +182,14 @@ func TestPKCS11GetSession(t *testing.T) { currentBCCSP.returnSession(session) } - // Lets break OpenSession, so non-cached session cannot be opened - oldSlot := currentBCCSP.slot - currentBCCSP.slot = ^uint(0) - // Should be able to get sessionCacheSize cached sessions + sessions = nil for i := 0; i < sessionCacheSize; i++ { session, err := currentBCCSP.getSession() require.NoError(t, err) sessions = append(sessions, session) } - _, err := currentBCCSP.getSession() - require.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID") - - // Load cache with bad sessions - for i := 0; i < sessionCacheSize; i++ { - currentBCCSP.returnSession(pkcs11.SessionHandle(^uint(0))) - } - - // Fix OpenSession so non-cached sessions can be opened - currentBCCSP.slot = oldSlot - - // Request a session, return, and re-acquire. The pool should be emptied - // before creating a new session so when returned, it should be the only - // session in the cache. - sess, err := currentBCCSP.getSession() - require.NoError(t, err) - currentBCCSP.returnSession(sess) - sess2, err := currentBCCSP.getSession() - require.NoError(t, err) - require.Equal(t, sess, sess2, "expected to get back the same session") - // Cleanup for _, session := range sessions { currentBCCSP.returnSession(session) @@ -274,11 +250,6 @@ func TestSessionHandleCaching(t *testing.T) { pi.returnSession(sess2) require.Empty(t, pi.sessions, "expected sessions to be empty") require.Empty(t, pi.handleCache, "expected handles to be cleared") - - pi.slot = ^uint(0) // break OpenSession - _, err = pi.getSession() - require.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID") - require.Empty(t, pi.sessions, "expected sessions to be empty") }) t.Run("SessionCacheEnabled", func(t *testing.T) { @@ -327,31 +298,6 @@ func TestSessionHandleCaching(t *testing.T) { require.Len(t, pi.sessions, 1, "expected one open session (sess1)") require.Len(t, pi.sessPool, 0, "sessionPool should be empty") require.Len(t, pi.handleCache, 2, "expected two handles in handle cache") - - pi.slot = ^uint(0) // break OpenSession - _, err = pi.getSession() - require.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID") - require.Len(t, pi.sessions, 1, "expected one active session (sess1)") - require.Len(t, pi.sessPool, 0, "sessionPool should be empty") - require.Len(t, pi.handleCache, 2, "expected two handles in handle cache") - - // Return a busted session that should be cached - pi.returnSession(pkcs11.SessionHandle(^uint(0))) - require.Len(t, pi.sessions, 1, "expected one active session (sess1)") - require.Len(t, pi.sessPool, 1, "sessionPool should contain busted session") - require.Len(t, pi.handleCache, 2, "expected two handles in handle cache") - - // Return sess1 that should be discarded - pi.returnSession(sess1) - require.Len(t, pi.sessions, 0, "expected sess1 to be removed") - require.Len(t, pi.sessPool, 1, "sessionPool should contain busted session") - require.Empty(t, pi.handleCache, "expected handles to be purged on removal of last tracked session") - - // Try to get broken session from cache - _, err = pi.getSession() - require.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID") - require.Empty(t, pi.sessions, "expected sessions to be empty") - require.Len(t, pi.sessPool, 0, "sessionPool should be empty") }) }