Repository navigation
Close the session when login fails in Token.open() - #247
WiktorB2004 wants to merge 1 commit into
Conversation
Token.open() opens a session with C_OpenSession and then authenticates with C_Login. If authentication failed (wrong PIN, locked PIN, user already logged in, or protected authentication requested from a module that does not support it), the exception propagated without closing the session, and since the caller never received a Session object, the handle stayed open until C_Finalize. Close it before re-raising. The C_CloseSession return value is ignored on purpose, so that it cannot mask the original error.
MatthiasValvekens
left a comment
MatthiasValvekens
left a comment
There was a problem hiding this comment.
Good catch; I'd factor the fix a bit differently, though.
| pin_data = pin | ||
| pin_length = <CK_ULONG> len(pin) | ||
|
|
||
| except BaseException: |
There was a problem hiding this comment.
Not a big fan of catch-all except clauses where all the possible error cases are basically directly spelled out in the try: block.
I'd hoist the check that leads to ArgumentsBad to before the C_OpenSession invocation. Then you can simply branch on the return value of C_Login to decide whether to close the session and throw an error, or to wrap the session with Session.make(...).
| with self.token.open(rw=True, so_pin=TOKEN_SO_PIN) as session: | ||
| self.assertIsInstance(session, pkcs11.Session) | ||
|
|
||
| @Only.softhsm2 # We don't have credentials to do this for other platforms |
There was a problem hiding this comment.
Limiting this to @Only.softhsm2 is fine, but that comment is neither here nor there. Whether there are credentials or not is a property of the environment, not of the test code itself.
I'd just drop the comment.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #247 +/- ##
==========================================
+ Coverage 91.52% 92.29% +0.77%
==========================================
Files 15 15
Lines 2713 2790 +77
Branches 27 27
==========================================
+ Hits 2483 2575 +92
+ Misses 228 213 -15
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #246
Token.open()opens a session withC_OpenSessionand then authenticates withC_Login. When authenticationraised (wrong or locked PIN, user already logged in, or protected authentication requested from a module without
CKF_PROTECTED_AUTHENTICATION_PATH), the exception propagated without closing the session. The caller never gets aSessionobject, so the handle stayed open untilC_Finalize.This wraps the authentication block in
try ... except BaseException:, callsC_CloseSessionand re-raises. TheC_CloseSessionreturn value is ignored on purpose so that it cannot mask the original error.BaseExceptionalso covers
KeyboardInterruptduring the (GIL-released) login.Tests (
tests/test_sessions.py, SoftHSMv2 only, like the neighbouring login tests):test_failed_login_does_not_leak_session: a failed user login must not leave a read-only session behind. Detectedwithout ctypes: a leftover read-only session makes a following security officer login fail with
SessionReadOnlyExists.test_unsupported_protected_auth_does_not_leak_session: same for theArgumentsBadpath.Without the fix both tests fail, and the leaked session also makes later tests in the module fail (
test_init_pin,test_open_session_and_login_so). With the fix the full suite passes on SoftHSM2 2.6.1 (Debian 13, Python 3.13):147 passed, 18 skipped. I did not run opencryptoki, Windows or macOS locally.