fix: Only reload PKCS11 library if key is not found#3795
fix: Only reload PKCS11 library if key is not found#3795Bravo555 merged 2 commits intothin-edge:mainfrom
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
Robot Results
|
84176fb to
921c94e
Compare
didier-wenzek
left a comment
There was a problem hiding this comment.
A critical and no so obvious piece of code is duplicated. Otherwise, the code looks good.
| // This branch runs only when a client calls TedgeP11Client::with_ready_check. | ||
| // It essentially does nothing, but when the server is not yet running and is triggered | ||
| // by a systemd socket, at a point client makes the request systemd starts the server, | ||
| // Cryptoki object is initialized before TedgeP11Server is constructed (in main) and at | ||
| // the point we return the response here, the PKCS11 library is loaded and ready to | ||
| // serve the subsequent request. |
There was a problem hiding this comment.
I feel this comment a bit difficult to understand. The content is here (reading twice, I got it), but having several independent sentences might help.
Here is my proposal:
The Ping/Pong request does nothing per-se, but can be used by a client to start the service. Indeed, systemd being configured to start the service when a request is received on the associated socket, a Ping/Pong request triggers a service start and ensures the PKCS11 library is loaded and ready to serve signing requests. In practice, this only occurs with a client calls TedgeP11Client::with_ready_check.
There was a problem hiding this comment.
Indeed, the comment describes what happens, but not why it happens, assuming the context of a problem that a reader will have no notion of. Used your comment and changed the beginning a bit to add a bit more context.
Fixed in c4bf1ad.
| } | ||
|
|
||
| Err(e) => return Err(e), | ||
| }; |
There was a problem hiding this comment.
This piece of code is critical and tricky, and must therefore not be duplicated to choose_scheme and sign.
=> Introduce a get_signing_key(self, uri) method.
| // dropping the signing key should drop the session | ||
| drop(signing_key); |
There was a problem hiding this comment.
The term should makes me wonder. Is the session actually dropped?
There was a problem hiding this comment.
Indeed, the comment sounds unsure. However, I'm sure the session must be dropped here. The signing_key is a Pkcs11Session which has session: Arc<Mutex<Session>, but after construction signing_key is immediately dropped without us cloning the session anywhere, so it must be dropped in entirety.
I have some in-progress refactoring that removes this Mutex, will be more clearer then. For now, giving this comment a little self-confidence in 931ae6b
The requested change has been addressed
Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
TedgeP11Client::with_ready_check used to make a ChooseScheme request that it immediately discarded only to make sure the library is loaded. That made unnecessary operation and generated output that was also unnecessary and made the log harder to read. Now with_ready_check uses a separate Ping request to which the server responds by only ensuring the library is loaded and doing nothing else. Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>
c4bf1ad to
77d7f9a
Compare
TODO
Optional follow-up
Proposed changes
Reload PKCS11 library only if we can't find any key and re-enable previously disabled test suite.
This PR re-enables the slot fix disabled by #3785, but defers the reloading of the library only when we don't find any key. This has the advantage that the first request won't suffer from the Nitrokey slowness and we'll be able to complete normally if the key is present and is configured properly.
Note that the first request that we make to ensure the library is loaded is a new
Pingrequest and responded to by aPongresponse, which is essentially a no-op but in systemd socket deployments it causes the server to start if it isn't already running. The library is loaded during server startup, with subsequentPingrequests NOT triggering the reload of the library.The library is only reloaded when handling
ChooseSchemeandSignrequests, and only when the key cannot be found. Other errors like a wrong PIN, objects not having expected attributes, or other generic PKCS11 errors do not trigger a reload.Types of changes
Paste Link to the issue
#3766
Checklist
just prepare-devonce)just formatas mentioned in CODING_GUIDELINESjust checkas mentioned in CODING_GUIDELINESFurther comments