Skip to content

Commit

Permalink
Drain session pool before creating new sessions
Browse files Browse the repository at this point in the history 
If a bad session was retrieved from the session pool, a new session
would be immediately created instead of attempting to retrieve another
session from the pool.

This change attempts to uses pooled sessions before creating new ones.

Signed-off-by: Matthew Sykes <[email protected]>
Signed-off-by: Gari Singh <[email protected]>
  • Loading branch information
@mastersingh24 @ale-linux
mastersingh24 authored and ale-linux committed Sep 7, 2020
1 parent d0c5065 commit d626146
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 73 deletions.
103 changes: 37 additions & 66 deletions bccsp/pkcs11/pkcs11.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"time"

"github.com/miekg/pkcs11"
"github.com/pkg/errors"
"go.uber.org/zap/zapcore"
)

Expand Down Expand Up @@ -55,43 +56,33 @@ func loadLib(lib, pin, label string) (*pkcs11.Ctx, uint, *pkcs11.SessionHandle,
return nil, slot, nil, fmt.Errorf("Could not find token with label %s", label)
}

session := createSession(ctx, slot, pin)

logger.Debugf("Created new pkcs11 session %+v on slot %d\n", session, slot)

if pin == "" {
return nil, slot, nil, fmt.Errorf("No PIN set")
}
err = ctx.Login(session, pkcs11.CKU_USER, pin)
session, err := createSession(ctx, slot, pin)
if err != nil {
if err != pkcs11.Error(pkcs11.CKR_USER_ALREADY_LOGGED_IN) {
return nil, slot, nil, fmt.Errorf("Login failed [%s]", err)
}
return nil, slot, nil, err
}

return ctx, slot, &session, nil
}

func (csp *impl) getSession() (session pkcs11.SessionHandle) {
select {
case session = <-csp.sessions:
_, err := csp.ctx.GetSessionInfo(session)
if err != nil {
func (csp *impl) getSession() (session pkcs11.SessionHandle, err error) {
for {
select {
case session = <-csp.sessions:
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.ctx.CloseSession(session)
session = createSession(csp.ctx, csp.slot, csp.pin)
} else {
logger.Debugf("Reusing existing pkcs11 session %+v on slot %d\n", session, csp.slot)
}

default:
// cache is empty (or completely in use), create a new session
session = createSession(csp.ctx, csp.slot, csp.pin)
default:
// cache is empty (or completely in use), create a new session
return createSession(csp.ctx, csp.slot, csp.pin)
}
}
return session
}

func createSession(ctx *pkcs11.Ctx, slot uint, pin string) pkcs11.SessionHandle {
func createSession(ctx *pkcs11.Ctx, slot uint, pin string) (pkcs11.SessionHandle, error) {
var s pkcs11.SessionHandle
var err error
// attempt 10 times to open a session with a 100ms delay after each attempt
Expand All @@ -105,18 +96,15 @@ func createSession(ctx *pkcs11.Ctx, slot uint, pin string) pkcs11.SessionHandle
time.Sleep(100 * time.Millisecond)
}
if err != nil {
logger.Panicf("OpenSession failed [%s]", err)
return 0, errors.Wrap(err, "OpenSession failed")
}
logger.Debugf("Created new pkcs11 session %+v on slot %d\n", s, slot)
session := s

err = ctx.Login(session, pkcs11.CKU_USER, pin)
if err != nil {
if err != pkcs11.Error(pkcs11.CKR_USER_ALREADY_LOGGED_IN) {
logger.Errorf("Login failed [%s]", err)
}
if err != nil && err != pkcs11.Error(pkcs11.CKR_USER_ALREADY_LOGGED_IN) {
return session, errors.Wrap(err, "Login failed")
}
return session
return session, nil
}

func (csp *impl) returnSession(session pkcs11.SessionHandle) {
Expand All @@ -133,7 +121,10 @@ func (csp *impl) returnSession(session pkcs11.SessionHandle) {
// This function can probably be adapted for both EC and RSA keys.
func (csp *impl) getECKey(ski []byte) (pubKey *ecdsa.PublicKey, isPriv bool, err error) {
p11lib := csp.ctx
session := csp.getSession()
session, err := csp.getSession()
if err != nil {
return nil, false, err
}
defer csp.returnSession(session)
isPriv = true
_, err = findKeyPairFromSKI(p11lib, session, ski, csp.altId, privateKeyFlag)
Expand Down Expand Up @@ -224,7 +215,10 @@ func oidFromNamedCurve(curve elliptic.Curve) (asn1.ObjectIdentifier, bool) {

func (csp *impl) generateECKey(curve asn1.ObjectIdentifier, ephemeral bool) (ski []byte, pubKey *ecdsa.PublicKey, err error) {
p11lib := csp.ctx
session := csp.getSession()
session, err := csp.getSession()
if err != nil {
return nil, nil, err
}
defer csp.returnSession(session)

keylabel := ""
Expand Down Expand Up @@ -351,7 +345,10 @@ func (csp *impl) generateECKey(curve asn1.ObjectIdentifier, ephemeral bool) (ski

func (csp *impl) signP11ECDSA(ski []byte, msg []byte) (R, S *big.Int, err error) {
p11lib := csp.ctx
session := csp.getSession()
session, err := csp.getSession()
if err != nil {
return nil, nil, err
}
defer csp.returnSession(session)

privateKey, err := findKeyPairFromSKI(p11lib, session, ski, csp.altId, privateKeyFlag)
Expand Down Expand Up @@ -381,7 +378,10 @@ func (csp *impl) signP11ECDSA(ski []byte, msg []byte) (R, S *big.Int, err error)

func (csp *impl) verifyP11ECDSA(ski []byte, msg []byte, R, S *big.Int, byteSize int) (bool, error) {
p11lib := csp.ctx
session := csp.getSession()
session, err := csp.getSession()
if err != nil {
return false, err
}
defer csp.returnSession(session)

logger.Debugf("Verify ECDSA\n")
Expand Down Expand Up @@ -564,35 +564,6 @@ func listAttrs(p11lib *pkcs11.Ctx, session pkcs11.SessionHandle, obj pkcs11.Obje
}
}

func (csp *impl) getSecretValue(ski []byte) []byte {
p11lib := csp.ctx
session := csp.getSession()
defer csp.returnSession(session)

keyHandle, err := findKeyPairFromSKI(p11lib, session, ski, csp.altId, privateKeyFlag)
if err != nil {
logger.Warningf("P11: findKeyPairFromSKI [%s]\n", err)
}
var privKey []byte
template := []*pkcs11.Attribute{
pkcs11.NewAttribute(pkcs11.CKA_VALUE, privKey),
}

// certain errors are tolerated, if value is missing
attr, err := p11lib.GetAttributeValue(session, *keyHandle, template)
if err != nil {
logger.Warningf("P11: get(attrlist) [%s]\n", err)
}

for _, a := range attr {
// Would be friendlier if the bindings provided a way convert Attribute hex to string
logger.Debugf("ListAttr: type %d/0x%x, length %d\n%s", a.Type, a.Type, len(a.Value), hex.Dump(a.Value))
return a.Value
}
logger.Warningf("No Key Value found: %v", err)
return nil
}

var (
bigone = new(big.Int).SetInt64(1)
idCtr = new(big.Int)
Expand Down
19 changes: 12 additions & 7 deletions bccsp/pkcs11/pkcs11_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/hyperledger/fabric/bccsp"
"github.com/miekg/pkcs11"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestKeyGenFailures(t *testing.T) {
Expand Down Expand Up @@ -50,7 +51,7 @@ func TestLoadLib(t *testing.T) {
// Test for no pin
_, _, _, err = loadLib(lib, "", label)
assert.Error(t, err)
assert.Contains(t, err.Error(), "No PIN set")
assert.Contains(t, err.Error(), "Login failed: pkcs11")
}

func TestOIDFromNamedCurve(t *testing.T) {
Expand Down Expand Up @@ -105,7 +106,8 @@ func TestAlternateLabelGeneration(t *testing.T) {
testBCCSP, err := New(opts, currentKS)

p11lib := testBCCSP.(*impl).ctx
session := testBCCSP.(*impl).getSession()
session, err := testBCCSP.(*impl).getSession()
require.NoError(t, err)
defer testBCCSP.(*impl).returnSession(session)

// Passing fake SKI to ensure that the look up fails if the uniqueAltId is not used
Expand Down Expand Up @@ -163,7 +165,9 @@ func TestNamedCurveFromOID(t *testing.T) {
func TestPKCS11GetSession(t *testing.T) {
var sessions []pkcs11.SessionHandle
for i := 0; i < 3*sessionCacheSize; i++ {
sessions = append(sessions, currentBCCSP.(*impl).getSession())
session, err := currentBCCSP.(*impl).getSession()
require.NoError(t, err)
sessions = append(sessions, session)
}

// Return all sessions, should leave sessionCacheSize cached
Expand All @@ -178,13 +182,14 @@ func TestPKCS11GetSession(t *testing.T) {

// Should be able to get sessionCacheSize cached sessions
for i := 0; i < sessionCacheSize; i++ {
sessions = append(sessions, currentBCCSP.(*impl).getSession())
session, err := currentBCCSP.(*impl).getSession()
require.NoError(t, err)
sessions = append(sessions, session)
}

// This one should fail
assert.Panics(t, func() {
currentBCCSP.(*impl).getSession()
}, "Should not been able to create another session")
_, err := currentBCCSP.(*impl).getSession()
assert.EqualError(t, err, "OpenSession failed: pkcs11: 0x3: CKR_SLOT_ID_INVALID")

// Cleanup
for _, session := range sessions {
Expand Down

0 comments on commit d626146

Please sign in to comment.