Limit PKCS11 session creation by EvanYan1024 · Pull Request #49 · hyperledger/fabric-lib-go

EvanYan1024 · 2026-05-14T03:18:06Z

Summary

Bound PKCS#11 session creation by the configured session cache size so callers wait for a returned session instead of opening sessions without limit.
Add PKCS11_SESSION_CACHE_SIZE environment override with warning + fallback on invalid values.
Raise the default session cache size to 30 and update PKCS#11 session tests to verify blocking behaviour at the limit.

Testing

go test ./...
go test ./bccsp/pkcs11

Note: go test -tags pkcs11 ./bccsp/pkcs11 requires a local PKCS#11 library configuration and was not runnable in this environment without PKCS11_LIB.

pfi79

Thank you for the error you found. But it seems to me that the fix is not ready yet.

EvanYan1024 · 2026-05-15T04:12:19Z

@pfi79 Pushed 692eef6 addressing all the review feedback. Quick summary:

Design changes

golang.org/x/sync/semaphore (pkcs11.go:53): replaced sessionSlots chan struct{} with *semaphore.Weighted. Slot accounting uses Acquire/Release.
env var removed (pkcs11.go:145): added a public PKCS11Opts.SessionCacheSize field with json / yaml / mapstructure tags, so the bound flows through the existing BCCSP factory configuration alongside Library / Label / Pin / etc. The internal sessionCacheSize is now strictly the resolved value used by New() (private; not user-facing).
createBoundedSession reuse (pkcs11.go:365): removed entirely. initialize now calls csp.getSession() directly so slot accounting stays symmetric.
doc comments (pkcs11.go:370): added multi-paragraph doc on getSession covering acquire-up-front, returnSession releases on cache, closeSession releases for known sessions, and why cached sessions don't hold a slot.
nil check (pkcs11.go:353): sessSem == nil represents the unbounded mode (SessionCacheSize < 0). The field doc comment in conf.go describes this.
known flag in closeSession (pkcs11.go:446): kept with an inline comment. If OpenSession succeeds but the subsequent Login fails, createSession closes the bare session before registering it in csp.sessions; the known check prevents a double sessSem.Release(1) in that path.

Test

Red test (pkcs11_test.go:1): TestPKCS11GetSession and TestSessionHandleCaching already serve this role — they fill the cache, spawn an extra getSession in a goroutine, and assert it blocks until a session is returned. Without the bound those goroutines fall straight through to createSession and the t.Fatalf(\"getSession should block...\") would fire. Happy to add an explicit comment on those tests if you'd like it more obvious.

Fabric integration

fabric build with this fork (pkcs11.go:1): verified on a fabric branch (`test-pkcs11-session-bound`) with `replace github.com/hyperledger/fabric-lib-go => github.com/EvanYan1024/fabric-lib-go@692eef6`. The fabric `verify-build` workflow passes end-to-end, including the PKCS#11 paths:
- ✅ Basic Checks · ✅ Unit Tests
- ✅ Integration Tests (gateway idemix pkcs11 configtx configtxlator)
- ✅ all other integration test groups (smartbft / ledger / raft / pvtdatapurge / lifecycle / sbe nwo msp / pvtdata / discovery gossip devmode pluggable)
Run: https://github.com/EvanYan1024/fabric/actions/runs/25898693262

pfi79 · 2026-05-15T07:45:00Z

@pfi79 Pushed 692eef6 addressing all the review feedback. Quick summary:

Design changes

golang.org/x/sync/semaphore (pkcs11.go:53): replaced sessionSlots chan struct{} with *semaphore.Weighted. Slot accounting uses Acquire/Release.

env var removed (pkcs11.go:145): added a public PKCS11Opts.SessionCacheSize field with json / yaml / mapstructure tags, so the bound flows through the existing BCCSP factory configuration alongside Library / Label / Pin / etc. The internal sessionCacheSize is now strictly the resolved value used by New() (private; not user-facing).

createBoundedSession reuse (pkcs11.go:365): removed entirely. initialize now calls csp.getSession() directly so slot accounting stays symmetric.

doc comments (pkcs11.go:370): added multi-paragraph doc on getSession covering acquire-up-front, returnSession releases on cache, closeSession releases for known sessions, and why cached sessions don't hold a slot.

nil check (pkcs11.go:353): sessSem == nil represents the unbounded mode (SessionCacheSize < 0). The field doc comment in conf.go describes this.

known flag in closeSession (pkcs11.go:446): kept with an inline comment. If OpenSession succeeds but the subsequent Login fails, createSession closes the bare session before registering it in csp.sessions; the known check prevents a double sessSem.Release(1) in that path.

Test

Red test (pkcs11_test.go:1): TestPKCS11GetSession and TestSessionHandleCaching already serve this role — they fill the cache, spawn an extra getSession in a goroutine, and assert it blocks until a session is returned. Without the bound those goroutines fall straight through to createSession and the t.Fatalf(\"getSession should block...\") would fire. Happy to add an explicit comment on those tests if you'd like it more obvious.

Fabric integration

fabric build with this fork (pkcs11.go:1): verified on a fabric branch (test-pkcs11-session-bound) with replace github.com/hyperledger/fabric-lib-go => github.com/EvanYan1024/fabric-lib-go@692eef6. The fabric verify-build workflow passes end-to-end, including the PKCS#11 paths:

✅ Basic Checks · ✅ Unit Tests

✅ Integration Tests (gateway idemix pkcs11 configtx configtxlator)

✅ all other integration test groups (smartbft / ledger / raft / pvtdatapurge / lifecycle / sbe nwo msp / pvtdata / discovery gossip devmode pluggable)

Run: https://github.com/EvanYan1024/fabric/actions/runs/25898693262

This is such a bad change that I have only one question: why are you needed? You need to leave just me and your AI.

EvanYan1024 · 2026-05-15T08:11:10Z

@pfi79 Pushed 692eef6 addressing all the review feedback. Quick summary:
Design changes

golang.org/x/sync/semaphore (pkcs11.go:53): replaced sessionSlots chan struct{} with *semaphore.Weighted. Slot accounting uses Acquire/Release.

env var removed (pkcs11.go:145): added a public PKCS11Opts.SessionCacheSize field with json / yaml / mapstructure tags, so the bound flows through the existing BCCSP factory configuration alongside Library / Label / Pin / etc. The internal sessionCacheSize is now strictly the resolved value used by New() (private; not user-facing).

createBoundedSession reuse (pkcs11.go:365): removed entirely. initialize now calls csp.getSession() directly so slot accounting stays symmetric.

doc comments (pkcs11.go:370): added multi-paragraph doc on getSession covering acquire-up-front, returnSession releases on cache, closeSession releases for known sessions, and why cached sessions don't hold a slot.

nil check (pkcs11.go:353): sessSem == nil represents the unbounded mode (SessionCacheSize < 0). The field doc comment in conf.go describes this.

known flag in closeSession (pkcs11.go:446): kept with an inline comment. If OpenSession succeeds but the subsequent Login fails, createSession closes the bare session before registering it in csp.sessions; the known check prevents a double sessSem.Release(1) in that path.

Test

Red test (pkcs11_test.go:1): TestPKCS11GetSession and TestSessionHandleCaching already serve this role — they fill the cache, spawn an extra getSession in a goroutine, and assert it blocks until a session is returned. Without the bound those goroutines fall straight through to createSession and the t.Fatalf(\"getSession should block...\") would fire. Happy to add an explicit comment on those tests if you'd like it more obvious.

Fabric integration

fabric build with this fork (pkcs11.go:1): verified on a fabric branch (test-pkcs11-session-bound) with replace github.com/hyperledger/fabric-lib-go => github.com/EvanYan1024/fabric-lib-go@692eef6. The fabric verify-build workflow passes end-to-end, including the PKCS#11 paths:

✅ Basic Checks · ✅ Unit Tests

✅ Integration Tests (gateway idemix pkcs11 configtx configtxlator)

✅ all other integration test groups (smartbft / ledger / raft / pvtdatapurge / lifecycle / sbe nwo msp / pvtdata / discovery gossip devmode pluggable)

Run: https://github.com/EvanYan1024/fabric/actions/runs/25898693262

This is such a bad change that I have only one question: why are you needed? You need to leave just me and your AI.

I'm sorry, I did use AI to solve the problem I encountered. You don't have to accept my pull request, but that's the issue, and you can use better ways to resolve it.

pfi79 · 2026-05-15T08:16:24Z

I'm sorry, I did use AI to solve the problem I encountered. You don't have to accept my pull request, but that's the issue, and you can use better ways to resolve it.

Let's take it one step at a time. I need a test that would show that there really is a problem. That is, it seems to me that you have a sound grain in the problem you have found, but I need to make sure. To do this, you need to artificially recreate the situation.

Can you do it?

…n creation This test is added on top of upstream/main without the fix from hyperledger#49, so running it here demonstrates that unbounded session creation is observable today: the test spawns 25 concurrent getSession callers with sessionCacheSize=5, tracks the peak number of open PKCS#11 sessions (len(csp.sessions)) over the run, and asserts that it never exceeds the configured cache size. On this branch (no fix) the test fails with peak == 25 because every caller whose arrival finds an empty sessPool falls through to createSession() and opens a new PKCS#11 session unconditionally. Under high sign concurrency that translates to CKR_SESSION_COUNT on OpenSession and subsequent CKR_DEVICE_ERROR on operations (see hyperledger#50). Apply hyperledger#49 on top of this commit and the same test passes with peak == 5. Signed-off-by: Evan <evanyan@sign.global>

…n creation This test is added on top of upstream/main without the fix from hyperledger#49, so running it here demonstrates that unbounded session creation is observable today: the test spawns 5 * defaultSessionCacheSize concurrent getSession callers, tracks the peak number of open PKCS#11 sessions (len(csp.sessions)) over the run, and asserts that it never exceeds defaultSessionCacheSize. On this branch (no fix) the test fails with peak equal to the caller count because every caller whose arrival finds an empty sessPool falls through to createSession() and opens a new PKCS#11 session unconditionally. Under high sign concurrency that translates to CKR_SESSION_COUNT on OpenSession and subsequent CKR_DEVICE_ERROR on operations (see hyperledger#50). Apply hyperledger#49 on top of this commit and the same test passes because getSession then acquires a semaphore.Weighted slot before opening a new session, capping the peak at defaultSessionCacheSize. Signed-off-by: Evan <evanyan@sign.global>

EvanYan1024 · 2026-05-15T08:57:59Z

Let's take it one step at a time. I need a test that would show that there really is a problem. That is, it seems to me that you have a sound grain in the problem you have found, but I need to make sure. To do this, you need to artificially recreate the situation.

Can you do it?

I added a test that reproduces the problem to my branch; you can take a look. Thanks
EvanYan1024@1c53337

pfi79 · 2026-05-15T09:10:45Z

Let's take it one step at a time. I need a test that would show that there really is a problem. That is, it seems to me that you have a sound grain in the problem you have found, but I need to make sure. To do this, you need to artificially recreate the situation.
Can you do it?

I added a test that reproduces the problem to my branch; you can take a look. Thanks EvanYan1024@1c53337

I took your test and ran it at my place. Yes, he fell, but I didn't see any of the mistakes you described.

When the number of concurrent signers exceeds what the HSM / PKCS#11 token can support:

- The PKCS#11 library returns CKR_SESSION_COUNT from C_OpenSession, surfaced as OpenSession failed, retrying [pkcs11: 0xB1: CKR_SESSION_COUNT].
- Tokens may then return CKR_DEVICE_ERROR from C_Sign because session state on the device becomes exhausted/inconsistent.

EvanYan1024 · 2026-05-15T09:16:45Z

Let's take it one step at a time. I need a test that would show that there really is a problem. That is, it seems to me that you have a sound grain in the problem you have found, but I need to make sure. To do this, you need to artificially recreate the situation.
Can you do it?

I added a test that reproduces the problem to my branch; you can take a look. Thanks EvanYan1024@1c53337

I took your test and ran it at my place. Yes, he fell, but I didn't see any of the mistakes you described.
When the number of concurrent signers exceeds what the HSM / PKCS#11 token can support:

- The PKCS#11 library returns CKR_SESSION_COUNT from C_OpenSession, surfaced as OpenSession failed, retrying [pkcs11: 0xB1: CKR_SESSION_COUNT].
- Tokens may then return CKR_DEVICE_ERROR from C_Sign because session state on the device becomes exhausted/inconsistent.

Thanks for running it. The CKR_SESSION_COUNT / CKR_DEVICE_ERROR errors in issue #50 come from a production deployment that exposes a remote KMS via a PKCS#11 shim, where
the underlying KMS has a low per-process session quota. SoftHSM2 has no hard session cap by default (the free build's MAX_SESSION_COUNT is effectively unbounded), so
opening 50 concurrent sessions against SoftHSM works just fine — the load just sits there.

What the test demonstrates is the BCCSP-layer cause: getSession opens a new PKCS#11 session for every concurrent caller whose arrival finds an empty sessPool,
completely independent of sessionCacheSize. The HSM-side CKR_SESSION_COUNT is the downstream effect — it surfaces only on tokens with a finite session quota:

Most commercial HSMs cap per-slot sessions in the low hundreds.
Remote-KMS-backed PKCS#11 shims (our deployment) cap much lower.
SoftHSM doesn't cap at all, so it doesn't surface the failure.

The peak 50 in the test (vs sessionCacheSize=10) is the precise quantity that, on a quota-bounded token, becomes CKR_SESSION_COUNT followed by cascading sign
failures. The fix puts the bound inside BCCSP so the underlying token never sees more than sessionCacheSize concurrent OpenSession calls, regardless of how strict the
HSM's own quota is.

pfi79 · 2026-05-15T09:20:40Z

The pipeline is red, there's a mistake in your code somewhere.

EvanYan1024 · 2026-05-15T09:29:33Z

The pipeline is red, there's a mistake in your code somewhere.

fixed

The PKCS#11 BCCSP previously fell back to OpenSession unbounded whenever the session cache was empty, allowing the number of concurrently checked out sessions to grow with the number of concurrent callers. Under high sign concurrency this surfaces from HSM/PKCS#11 libraries as CKR_SESSION_COUNT on OpenSession and CKR_DEVICE_ERROR on subsequent operations (see issue hyperledger#50). Gate concurrent OpenSession calls behind a semaphore.Weighted whose weight equals sessionCacheSize, so the number of outstanding sessions is bounded. Cached sessions in sessPool intentionally do not occupy a slot: getSession reacquires its own slot when pulling one out, and returnSession releases the slot when caching, so a caller blocked on the bound is unblocked the moment another session is returned or closed. Expose the bound via a new public PKCS11Opts.SessionCacheSize field (with json/yaml/mapstructure tags) so it can be configured through the existing BCCSP factory configuration alongside Library/Label/Pin etc., with no environment variable required. A value of 0 selects the default (30); a negative value disables the bound and preserves the original unbounded code path for callers that want it. Update closeSession to release the slot only for sessions that were previously registered, guarding against double-release when an open succeeds but the subsequent Login fails and the partial session is closed before being tracked. Tests in pkcs11_test.go (TestPKCS11GetSession, TestSessionHandleCaching) exercise the bound: they fill the cache, spawn an extra getSession in a goroutine, and assert it blocks until a session is returned. Without this change those tests would fail because the goroutine would proceed straight through to createSession instead of blocking. Signed-off-by: Evan <evanyan@sign.global>

pfi79 · 2026-05-18T06:03:35Z

 	default:
-		// have plenty of sessions in cache, dropping
 		csp.closeSession(session)


When you limited the number of slots and made them equal to the channel capacity, can we get in here? It seems to me that if we got here, we have very big problems.

You're right — with sessSem and sessPool both bounded by cacheSize,
the default branch is unreachable.

Invariant: every returnSession() call comes from a path that previously
acquired a slot via getSession() → sem.Acquire(1), and the Release
happens after the push. With A = acquired-but-not-returned and
L = cached, the semaphore enforces A + L <= cacheSize. The caller
about to push contributes A >= 1, so L <= cacheSize-1 and the send
never blocks.

Simplified to a direct send in 65b4b8f:

func (csp *Provider) returnSession(session pkcs11.SessionHandle) {
csp.sessPool <- session
csp.sessSem.Release(1)
}

TestPKCS11GetSession at pkcs11_test.go:668-673 already exercises
the worst case (all N in-flight sessions returned simultaneously)
and asserts len(sessPool) == cacheSize, which would fail if any
return path dropped a session via the old default branch.

sessPool and sessSem now share cacheSize as capacity, so the channel send in returnSession can never block: the caller still holds an outstanding sem.Acquire(1) from its prior getSession(), which bounds acquired-but-not-returned sessions A >= 1, leaving cached L <= cacheSize-1. The select+default branch was therefore unreachable; remove it so the control flow reflects the actual invariant. Signed-off-by: Evan <evanyan@sign.global>

EvanYan1024 requested a review from a team as a code owner May 14, 2026 03:18

EvanYan1024 force-pushed the limit-pkcs11-sessions branch from 46082c7 to b33d2e5 Compare May 14, 2026 03:35

EvanYan1024 mentioned this pull request May 14, 2026

pkcs11: getSession() can open unbounded HSM sessions under concurrent load #50

Closed

EvanYan1024 force-pushed the limit-pkcs11-sessions branch from b33d2e5 to 692b67a Compare May 14, 2026 05:11

pfi79 reviewed May 14, 2026

View reviewed changes

EvanYan1024 force-pushed the limit-pkcs11-sessions branch 2 times, most recently from 7d9868e to 692eef6 Compare May 15, 2026 02:26

EvanYan1024 force-pushed the limit-pkcs11-sessions branch from 692eef6 to 62e0181 Compare May 15, 2026 06:27

EvanYan1024 force-pushed the limit-pkcs11-sessions branch from 62e0181 to c4b0938 Compare May 15, 2026 08:38

EvanYan1024 force-pushed the limit-pkcs11-sessions branch from c4b0938 to fe00338 Compare May 15, 2026 08:48

EvanYan1024 force-pushed the limit-pkcs11-sessions branch from fe00338 to 7a31c71 Compare May 15, 2026 09:25

pfi79 reviewed May 15, 2026

View reviewed changes

Comment thread bccsp/pkcs11/conf.go Outdated

EvanYan1024 force-pushed the limit-pkcs11-sessions branch 2 times, most recently from 2c04058 to 38b7958 Compare May 15, 2026 12:29

pfi79 reviewed May 15, 2026

View reviewed changes

Comment thread bccsp/pkcs11/pkcs11.go Outdated

pfi79 reviewed May 15, 2026

View reviewed changes

Comment thread bccsp/pkcs11/pkcs11.go Outdated

EvanYan1024 force-pushed the limit-pkcs11-sessions branch from 38b7958 to 730a932 Compare May 18, 2026 01:45

pfi79 reviewed May 18, 2026

View reviewed changes

pfi79 approved these changes May 18, 2026

View reviewed changes

pfi79 merged commit fd7325d into hyperledger:main May 18, 2026
4 checks passed

Uh oh!

Conversation

EvanYan1024 commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

pfi79 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EvanYan1024 commented May 15, 2026

Uh oh!

pfi79 commented May 15, 2026

Uh oh!

EvanYan1024 commented May 15, 2026

Uh oh!

pfi79 commented May 15, 2026

Uh oh!

EvanYan1024 commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfi79 commented May 15, 2026

Uh oh!

EvanYan1024 commented May 15, 2026

Uh oh!

pfi79 commented May 15, 2026

Uh oh!

EvanYan1024 commented May 15, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pfi79 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

EvanYan1024 May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

EvanYan1024 commented May 14, 2026 •

edited

Loading

EvanYan1024 commented May 15, 2026 •

edited

Loading