Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Feature/pkcs11 token #461

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

MPeisl
Copy link
Member

@MPeisl MPeisl commented Apr 18, 2024

Support for generic PKCS#11 tokens:

  • scd/pkcs11-lib/*: contains headers and helper libraries
  • scd/pkcs11-test/*: unit-tests for p11token module
  • scd/p11token: actual scd_token implementation for PKCS#11-API

TODO: integration in daemon
TODO: integration testing
TODO before merging: squash commits

@MPeisl MPeisl force-pushed the feature/pkcs11-token branch from b99a779 to cb36751 Compare April 18, 2024 15:02
if (smartcard->token_type == CONTAINER_TOKEN_TYPE_PKCS11) {
if (NULL == container_get_pkcs11_module(smartcard->container)) {
ERROR("PKCS#11 module missing in container config. Abort creation of container");
mem_free0(smartcard);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return NULL;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

smartcard->success_cb = NULL;
smartcard->err_cb = NULL;
smartcard->err_cbdata = NULL;

smartcard->sock = sock_unix_create_and_connect(SOCK_SEQPACKET, scd_sock_path);
if (smartcard->sock < 0) {
mem_free0(smartcard);
mem_free0(smartcard); //? potential mem_leak for usbtoken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes smartcard->token_serial may be allocated here. Thus should be freed, same counts for smartcard->pkcs11_module. May instead of return NULL in error cases. We introduce a jump mark err:
and do cleanup there

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

scd/p11token.h Outdated
#include "pkcs11-lib/pkcs11.h"
#include <stddef.h>

// todo: move to c-file -> hide implementation details to user
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment

scd/p11token.h Outdated
#define P11TOKEN_H

#include <stdbool.h>
#include "pkcs11-lib/pkcs11.h"
Copy link
Member

@quitschbo quitschbo Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this header is not used here, -> move this to p11token.c

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, header was only needed for struct p11token which is now defined in p11token.c. Will be moved.

scd/p11token.c Outdated
CK_UTF8CHAR label[32];
unsigned int wrong_unlock_attempts;
CK_FUNCTION_LIST_PTR ctx;
CK_SESSION_HANDLE_PTR sh;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use the "GNU" typedefs of pkcs11.h? would be much better to read. (in the whole p11token.c)
e.g.:
ck_function_list_t *ctx;
ck_session_handle_t *sh;

scd/scd.c Outdated
@@ -481,13 +483,17 @@ scd_token_new(const DaemonToToken *msg)

create_data.type = scd_proto_to_tokentype(msg);

// ? why no switch statement here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes a "switch" would look nicer here for me, too :)

new_token->reset_auth = int_reset_auth_p11;
new_token->get_atr = int_get_atr_p11;
new_token->send_apdu = int_send_apdu_p11;
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a followup refactoring, we should consider to introduce a struct for all those functions and hide
the token specific functions in the corresponding p11token.c/usbtoken.c files (analogues to the compartment_module_t)

@MPeisl MPeisl force-pushed the feature/pkcs11-token branch from 14e15bd to 20f9bb3 Compare April 25, 2024 06:34
@MPeisl MPeisl force-pushed the feature/pkcs11-token branch 2 times, most recently from eb35e2b to 2c49d9f Compare June 21, 2024 12:04
#include <stdio.h>
#include <string.h>

#include "libscdl.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get rid of this wrapper lib, we do not need win32 compat.
directly call dlopen, dlsym and dlclose in the following.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@MPeisl MPeisl force-pushed the feature/pkcs11-token branch from 2c49d9f to 28e753e Compare December 3, 2024 09:35
@MPeisl MPeisl force-pushed the feature/pkcs11-token branch 2 times, most recently from 5e8f238 to f8c7d67 Compare February 24, 2025 16:59
@MPeisl MPeisl changed the base branch from kirkstone to main February 24, 2025 17:01
@MPeisl MPeisl force-pushed the feature/pkcs11-token branch from f8c7d67 to 5e8f238 Compare February 24, 2025 17:14
* pkcs11.h: Definitions of PKCS#11-API
  source: https://github.com/p11-glue/p11-kit/blob/master/common/pkcs11.h
* libpkcs11 + libscdl: Helper libraries which enable dynamic PKCS#11-module
  loading
  source: https://github.com/OpenSC/OpenSC/tree/master/src/common

Signed-off-by: Maximilian Peisl <[email protected]>
Add support for PKCS#11-based tokens to scd.

Signed-off-by: Maximilian Peisl <[email protected]>
Add unit tests for PKCS#11-token backend.

Signed-off-by: Maximilian Peisl <[email protected]>
Introduces new PKCS11 token-type which is available
in the scd.

Signed-off-by: Maximilian Peisl <[email protected]>
* Switched to GNU style pkcs11-header definitions
* several code enhancments in p11token: improved variable and function
  naming, fixed several potential memory leaks
* p11test: check for null-ptr in wrap/unwrap unit test

Signed-off-by: Maximilian Peisl <[email protected]>
Replaced if-else chain with more readable switch-statement.

Signed-off-by: Maximilian Peisl <[email protected]>
Fixed some memory-leaks and streamlined error-handling and cleanup by
introducing error-jump label.

Signed-off-by: Maximilian Peisl <[email protected]>
Added documentation for container_get_pkcs11_module.

Signed-off-by: Maximilian Peisl <[email protected]>
Removed libscdl: wrapping of dl* functions unnecessary in our usage
context as we only target linux systems.

Signed-off-by: Maximilian Peisl <[email protected]>
@MPeisl MPeisl force-pushed the feature/pkcs11-token branch from 643aa00 to 3a80617 Compare February 25, 2025 10:23
Signed-off-by: Maximilian Peisl <[email protected]>

Adjustments to meet upstream changes

Signed-off-by: Maximilian Peisl <[email protected]>
@MPeisl MPeisl force-pushed the feature/pkcs11-token branch from 3a80617 to da880e5 Compare February 25, 2025 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants