Pkcs#11 services through OP-TEE SKS TA #121

etienne-lms · 2018-05-22T15:05:59Z

This changes introduces an implementation of the Pkcs#11 Cryptoki library that interface a specific trusted application in the secure side.

This P-R implements the basics for the client library. The effective implementation of the services will come later.

This P-R does not introduce the TA API itself that is being reviewed through OP-TEE/optee_os#2342. The client lib will integrate the TA API once its is approved and merged.

jenswi-linaro · 2018-05-23T06:06:12Z

libsks/include/pkcs11.h

@@ -0,0 +1,265 @@
+/* Copyright (c) OASIS Open 2016. All Rights Reserved./


SPDX header please

Looks like the OASIS IPR Policy is not registered to SPDX.
Tricky...

I think i will go another way.
The opencryptoki project includes a package (opencryptoki) that proposes a pkcs11 cryptoki header under the creative commons license, here.

The header file reflects a slightly older pkcs11 and must be updated with more recent IDs and few crypto parameter structures. From license view, it is more suitable for a project like the OP-TEE. @jenswi-linaro, @jforissier, what do you think?

[It is the "Common Public License, version 1.0 (CPL-1.0)", SPDX identifier "CPL-1.0". Nothing to do with the Creative Commons licenses AFAICT.]

I think we may have a problem with sections 2.a and 3.a.iv of https://spdx.org/licenses/CPL-1.0.html (source code).

yes, CPL-1.0, thanks for fixing. From wikipedia, CPL-1.0 is not a good choice.

CPL lacks compatibility with both versions of the GPL because it has a "choice of law" section in section 7, which restricts legal disputes to a certain court. Another source of incompatibility is the differing copyleft requirements. [2]

Rewrite from scratch based on the specs?

jbech-linaro

Rewrite from scratch based on the specs?

If we cannot include it as it is, then I think that rewrite is the only option we have. I just wonder, wouldn't you end up with almost the same kind of defines etc in the end.

jbech-linaro · 2018-05-25T07:41:36Z

libsks/include/pkcs11.h

+ */
+#include "pkcs11t.h"
+
+#define __PASTE(x,y)      x##y


Rename to __JOIN (or __CONCATENATE) might better describe what this actually does?

etienne-lms · 2018-05-25T09:17:28Z

If we cannot include it as it is, then I think that rewrite is the only option we have. I just wonder, wouldn't you end up with almost the same kind of defines etc in the end.

Yes, same IDs, labels, structures and prototypes...
I have started, it's a bit painful... will restrict to what is currently supported/planned for SKS.

etienne-lms · 2018-05-25T15:04:47Z

Pushed proposal for a new pkcs11.h header file implementation.

Implement pkcs11.h header file that partially covers the PKCS#11 specification. Resources initially planned to be supported are defined. The header will need to be updated with remaining PKCS#11 definition when related support will be implemented. Signed-off-by: Etienne Carriere <[email protected]>

Define the few platform macros expected by the cryptolib header files. Initial source file: the API functions from pkcs11_api.c. Build from Makefile or from CMake. Signed-off-by: Etienne Carriere <[email protected]>

LOG_ERROR(), LOG_INFO, LOG_DEBUG() currently all calling printf(). ASSERT() exits process on error. Signed-off-by: Etienne Carriere <[email protected]>

PKCS#11 specifies library must be initialized prior being used but for 2 APIs: C_Initialize and C_GetFunctionList. Signed-off-by: Etienne Carriere <[email protected]>

Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms · 2018-06-14T12:45:24Z

Comments addressed and squashed.

jforissier

Not sure several commits are justified instead of one single "Add stubbed liboptee_cryptoki"?

jforissier · 2018-06-14T13:04:38Z

libsks/src/local_utils.h

+#include <stdio.h>
+#include <stdlib.h>
+
+#define ASSERT(cond) \


Why not use assert() from <assert.h> directly?

true. thanks.

jforissier · 2018-06-14T13:06:16Z

libsks/src/pkcs11_api.c

+/*
+ * Copyright (c) 2017, Linaro Limited
+ *
+ * SPDX-License-Identifier: BSD-2-Clause


// SPDX-License-Identifier: BSD-2-Clause on first line? (should we follow the same rules as in optee_os?)

not mandated but ok.

jforissier · 2018-06-14T13:17:39Z

libsks/src/pkcs11_api.c

+	do { \
+		if (!ptr) \
+			return CKR_ARGUMENTS_BAD; \
+	} while (0)


Bad idea as per https://www.kernel.org/doc/html/v4.10/process/coding-style.html#macros-enums-and-rtl "Macros that affect control flow".

ok, i will go another way. thanks to point out.

jforissier · 2018-06-14T13:20:56Z

Makefile

@@ -129,3 +137,6 @@ copy_export: build
 	cp -a ${O}/libteec/libteec.a $(DESTDIR)$(LIBDIR)
 	cp ${O}/tee-supplicant/tee-supplicant $(DESTDIR)$(BINDIR)
 	cp public/*.h $(DESTDIR)$(INCLUDEDIR)
+	cp libsks/include/*.h $(DESTDIR)$(INCLUDEDIR)
+	cp -a ${O}/libsks/liboptee_cryptoki.so* $(DESTDIR)$(LIBDIR)


The fact that the library is not called libsks, or rather, that the targets build-libsks and clean-libsks are not called build-liboptee_cryptoki and clean-liboptee_cryptoki, is mildly disturbing. It diverges from libteec.

true. better be cleaned now. yet it find name libsks a bit short for a quite specific library (optee/pkcs11).

etienne-lms · 2018-06-15T06:09:06Z

Not sure several commits are justified instead of one single "Add stubbed liboptee_cryptoki"?

True. I have split initial work into smaller change too easy review. Maybe too much...
I can squash in one if you feel it is better and safer.

Rename library optee_cryptoki into library sks. Signed-off-by: Etienne Carriere <[email protected]>

Fix CMake script for header files to no install the CMakeLists.txt file. Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms · 2018-06-15T10:08:42Z

all comments not addressed yet. in progress...

Signed-off-by: Etienne Carriere <[email protected]>

Do not use BIT. Some components as xtest build with an util.h from optee_os that defines macro BIT() whereas other components can build libsks (and libteec, ...) without relynig on a prebuilt optee_os that provides such util.h. As a consequence, use (1U << pos) rather than BIT(pos) in the code. Signed-off-by: Etienne Carriere <[email protected]>

Remove ASSERT(). Will use the generic assert() from assert.h when needed. Fix notice. Signed-off-by: Etienne Carriere <[email protected]>

fix notice Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms · 2018-06-15T10:25:11Z

Comments addressed, but the one about squashing all the commits into a single one.

Signed-off-by: Etienne Carriere <[email protected]>

jforissier · 2018-06-15T10:57:11Z

libsks/src/pkcs11_api.c

+
+static int inited;
+
+static int sanity_lib_init(void)


Remove. This function sounds like it initializes the library, but actually it does not (C_Initialize() does).

jforissier · 2018-06-15T10:59:46Z

libsks/src/pkcs11_api.c

+	return inited == 0;
+}
+
+static int sanity_nonnull_ptr(void *ptr)


jforissier · 2018-06-15T11:00:37Z

libsks/src/pkcs11_api.c

+{
+	(void)res;
+
+	if (sanity_lib_init())


if (!inited)

What about thread safety? Is the API supposed to be thread-safe?

Yes but I would rather postpone this issue to later...

jforissier · 2018-06-15T11:00:52Z

libsks/src/pkcs11_api.c

+CK_RV C_GetFunctionList(CK_FUNCTION_LIST_PTR_PTR ppFunctionList)
+{
+	/* Note: no need to call sanity_lib_init() here */
+	if (sanity_nonnull_ptr(ppFunctionList))


if (!ppFunctionList)

Drop sanity_lib_init()/sanity_nonnull_ptr() in favor to inline instructions. Signed-off-by: Etienne Carriere <[email protected]>

jbech-linaro · 2018-07-02T10:40:53Z

libsks/src/pkcs11_api.c

+	/* Argument currently unused */
+	(void)args;
+
+	if (lib_inited)


inited? What about ...initiated? Some comment for the other places further down.

jbech-linaro · 2018-07-02T10:52:31Z

Besides a minor suggestion, I think this looks good. Then we have the golden question, whether to merge something in the middle of being developed or not. It would be nice to live up to "submit early and often" and get pieces of the complete implementation merged, one by one ... @OP-TEE/linaro thoughts?

jforissier · 2018-07-02T11:08:50Z

@jbech-linaro generally speaking, my opinion is we should "submit early and often" but into a separate branch, until the code actually provides a real service or at least can be somewhat tested. If I understand correctly, it is the case for this PKCS#11 stuff already?
Anyway, I don't feel strongly about it, and I will not object to merging code into master as long as it has been sufficiently reviewed.

etienne-lms · 2018-07-03T07:57:09Z

@jforissier, yes this P-R comes with related P-R in optee_os and optee_test and do provide some test materials. All these P-R rely on configuration directive CFG_SECURE_KEY_SERVICES to build related code in each component. The currently proposed build sequence (OP-TEE/build#261) is independent of this P-R.
Anyway, i strongly agree with "I will not object to merging code into master as long as it has been sufficiently reviewed.".

jforissier · 2018-07-03T08:16:57Z

@etienne-lms all good then, thanks. I will take all this as soon as 3.2.0 is out, which may be in a couple of days or so (I'd like to better understand OP-TEE/optee_os#2414 and OP-TEE/optee_os#2437).

etienne-lms · 2018-07-03T08:33:01Z

@jforissier, fine with me.

etienne-lms · 2019-01-11T09:42:20Z

Superseded by #138.

etienne-lms force-pushed the sks-merge branch from 97e8f5f to 7032ae6 Compare May 22, 2018 15:09

etienne-lms mentioned this pull request May 22, 2018

Secure key services: step1: TA basics and API OP-TEE/optee_os#2342

Closed

jenswi-linaro reviewed May 23, 2018

View reviewed changes

jbech-linaro reviewed May 25, 2018

View reviewed changes

etienne-lms added 5 commits June 14, 2018 13:29

libsks: fully stubbed cryptoki API

c25b9d3

Define the few platform macros expected by the cryptolib header files. Initial source file: the API functions from pkcs11_api.c. Build from Makefile or from CMake. Signed-off-by: Etienne Carriere <[email protected]>

libsks: local utilities

ace2315

LOG_ERROR(), LOG_INFO, LOG_DEBUG() currently all calling printf(). ASSERT() exits process on error. Signed-off-by: Etienne Carriere <[email protected]>

libsks: sanity on APIs regarding library initialization

0f0f5c3

PKCS#11 specifies library must be initialized prior being used but for 2 APIs: C_Initialize and C_GetFunctionList. Signed-off-by: Etienne Carriere <[email protected]>

libsks: implement C_GetFunctionList

7581c02

Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms force-pushed the sks-merge branch from 76902a3 to 7581c02 Compare June 14, 2018 11:31

jforissier reviewed Jun 14, 2018

View reviewed changes

etienne-lms added 2 commits June 15, 2018 11:58

[review] libsks: fully stubbed cryptoki API

88a31ba

Rename library optee_cryptoki into library sks. Signed-off-by: Etienne Carriere <[email protected]>

[review] libsks: fully stubbed cryptoki API

81652bc

Fix CMake script for header files to no install the CMakeLists.txt file. Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms added 4 commits June 15, 2018 12:13

[review] libsks: sanity on APIs regarding library initialization

f7b1a58

Signed-off-by: Etienne Carriere <[email protected]>

[review] libsks: local utilities

6a1e58c

Remove ASSERT(). Will use the generic assert() from assert.h when needed. Fix notice. Signed-off-by: Etienne Carriere <[email protected]>

[review] libsks: fully stubbed cryptoki API

ab57c5d

fix notice Signed-off-by: Etienne Carriere <[email protected]>

[review] libsks: sanity on APIs regarding library initialization

8ef18e0

Signed-off-by: Etienne Carriere <[email protected]>

jforissier reviewed Jun 15, 2018

View reviewed changes

[review] libsks: sanity on APIs regarding library initialization

e6b7507

Drop sanity_lib_init()/sanity_nonnull_ptr() in favor to inline instructions. Signed-off-by: Etienne Carriere <[email protected]>

etienne-lms mentioned this pull request Jun 15, 2018

xtest: regression 41xx target Secure Key Services tests OP-TEE/optee_test#281

Merged

jbech-linaro reviewed Jul 2, 2018

View reviewed changes

etienne-lms mentioned this pull request Jan 11, 2019

[RFC] libsks for Pkcs#11 services through OP-TEE SKS TA #138

Merged

etienne-lms closed this Jan 11, 2019

etienne-lms deleted the sks-merge branch October 30, 2020 18:20

		@@ -0,0 +1,265 @@
		/* Copyright (c) OASIS Open 2016. All Rights Reserved./

Pkcs#11 services through OP-TEE SKS TA #121

Pkcs#11 services through OP-TEE SKS TA #121

Conversation

etienne-lms commented May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbech-linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented May 25, 2018

etienne-lms commented May 25, 2018

etienne-lms commented Jun 14, 2018

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms commented Jun 15, 2018

etienne-lms commented Jun 15, 2018

etienne-lms commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbech-linaro commented Jul 2, 2018

jforissier commented Jul 2, 2018

etienne-lms commented Jul 3, 2018

jforissier commented Jul 3, 2018

etienne-lms commented Jul 3, 2018

etienne-lms commented Jan 11, 2019

etienne-lms commented May 22, 2018 •

edited

Loading

jforissier Jun 14, 2018 •

edited

Loading