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 prelimary 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.
Temporarily we will revert the call and in future
implementations we can look into other ways of verifying
if a session has become stale.

Signed-off-by: Brett Logan <brett.t.logan@ibm.com>
  • Loading branch information
Brett Logan authored and mastersingh24 committed Sep 25, 2020
1 parent bf5917f commit 1eec843
Show file tree
Hide file tree
Showing 2 changed files with 1 addition 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 @@ -303,14 +303,7 @@ func (csp *Provider) 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
55 changes: 0 additions & 55 deletions bccsp/pkcs11/pkcs11_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -534,10 +534,6 @@ func TestPKCS11GetSession(t *testing.T) {
csp.returnSession(session)
}

// Lets break OpenSession, so non-cached session cannot be opened
oldSlot := csp.slot
csp.slot = ^uint(0)

// Should be able to get sessionCacheSize cached sessions
sessions = nil
for i := 0; i < sessionCacheSize; i++ {
Expand All @@ -546,27 +542,6 @@ func TestPKCS11GetSession(t *testing.T) {
sessions = append(sessions, session)
}

_, err := csp.getSession()
require.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID")

// Load cache with bad sessions
for i := 0; i < sessionCacheSize; i++ {
csp.returnSession(pkcs11.SessionHandle(^uint(0)))
}

// Fix OpenSession so non-cached sessions can be opened
csp.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 := csp.getSession()
require.NoError(t, err)
csp.returnSession(sess)
sess2, err := csp.getSession()
require.NoError(t, err)
require.Equal(t, sess, sess2, "expected to get back the same session")

// Cleanup
for _, session := range sessions {
csp.returnSession(session)
Expand Down Expand Up @@ -621,11 +596,6 @@ func TestSessionHandleCaching(t *testing.T) {
csp.returnSession(sess2)
require.Empty(t, csp.sessions, "expected sessions to be empty")
require.Empty(t, csp.handleCache, "expected handles to be cleared")

csp.slot = ^uint(0) // break OpenSession
_, err = csp.getSession()
require.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID")
require.Empty(t, csp.sessions, "expected sessions to be empty")
})

t.Run("SessionCacheEnabled", func(t *testing.T) {
Expand Down Expand Up @@ -673,31 +643,6 @@ func TestSessionHandleCaching(t *testing.T) {
require.Len(t, csp.sessions, 1, "expected one open session (sess1)")
require.Len(t, csp.sessPool, 0, "sessionPool should be empty")
require.Len(t, csp.handleCache, 2, "expected two handles in handle cache")

csp.slot = ^uint(0) // break OpenSession
_, err = csp.getSession()
require.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID")
require.Len(t, csp.sessions, 1, "expected one active session (sess1)")
require.Len(t, csp.sessPool, 0, "sessionPool should be empty")
require.Len(t, csp.handleCache, 2, "expected two handles in handle cache")

// Return a busted session that should be cached
csp.returnSession(pkcs11.SessionHandle(^uint(0)))
require.Len(t, csp.sessions, 1, "expected one active session (sess1)")
require.Len(t, csp.sessPool, 1, "sessionPool should contain busted session")
require.Len(t, csp.handleCache, 2, "expected two handles in handle cache")

// Return sess1 that should be discarded
csp.returnSession(sess1)
require.Len(t, csp.sessions, 0, "expected sess1 to be removed")
require.Len(t, csp.sessPool, 1, "sessionPool should contain busted session")
require.Empty(t, csp.handleCache, "expected handles to be purged on removal of last tracked session")

// Try to get broken session from cache
_, err = csp.getSession()
require.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID")
require.Empty(t, csp.sessions, "expected sessions to be empty")
require.Len(t, csp.sessPool, 0, "sessionPool should be empty")
})
}

Expand Down

0 comments on commit 1eec843

Please sign in to comment.