Skip to content

Commit

Permalink
Remove GetSessionInfo Call
Browse files Browse the repository at this point in the history
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 <brett.t.logan@ibm.com>
  • Loading branch information
Brett Logan authored and mastersingh24 committed Sep 25, 2020
1 parent 4f1e340 commit 2d63281
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 63 deletions.
9 changes: 1 addition & 8 deletions bccsp/pkcs11/pkcs11.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
56 changes: 1 addition & 55 deletions bccsp/pkcs11/pkcs11_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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")
})
}

Expand Down

0 comments on commit 2d63281

Please sign in to comment.