pkcs11: Add TEE Identity based authentication support by vesajaaskelainen · Pull Request #236 · OP-TEE/optee_client

vesajaaskelainen · 2020-11-22T16:30:41Z

Please see OP-TEE/optee_os#4222 for testing and usage instructions.

etienne-lms

Only minor comments.
For both commits, prefer start log header line with libckteec: and add () to function labels. Also add a description line between header line and S-o-b tag in commit "C_Login: Allow NULL_PTR PIN entry.". i.e.

libckteec: Allow NULL_PTR PIN entry in C_Login()

Allow C_Login() to transport a null PIN reference as when protected
authentication path is used.

Signed-off-by: Vesa Jääskeläinen <[email protected]>

vesajaaskelainen · 2020-11-23T18:25:02Z

Only minor comments.
For both commits, prefer start log header line with libckteec: and add () to function labels. Also add a description line between header line and S-o-b tag in commit "C_Login: Allow NULL_PTR PIN entry.". i.e.
libckteec: Allow NULL_PTR PIN entry in C_Login()

Allow C_Login() to transport a null PIN reference as when protected
authentication path is used.

Signed-off-by: Vesa Jääskeläinen <[email protected]>

OK

vesajaaskelainen · 2020-11-23T19:26:07Z

Only minor comments.
For both commits, prefer start log header line with libckteec: and add () to function labels. Also add a description line between header line and S-o-b tag in commit "C_Login: Allow NULL_PTR PIN entry.". i.e.
libckteec: Allow NULL_PTR PIN entry in C_Login()

Allow C_Login() to transport a null PIN reference as when protected
authentication path is used.

Signed-off-by: Vesa Jääskeläinen <[email protected]>
OK

Updated commit messages.

vesajaaskelainen · 2020-12-12T14:54:50Z

@etienne-lms I believe I have addressed all your comments.

Also tested (and fixed) PIN == NULL_PTR cases for methods that use PIN as input.

vesajaaskelainen · 2020-12-12T14:56:54Z

I was thinking about could xtest be used here somehow but seems rather challenging as there is no API to reset the token back to initial state without performing clearing secure storage and reboot 😞. Also switching to different identities and somehow having common identities can be a bit of problem.

etienne-lms · 2020-12-13T08:23:35Z

Maybe a debug command to reset a token even without the SO credentials?
The TA supports multi tokens, a token could be reserved for debug purpose, identified from its token label.

vesajaaskelainen · 2020-12-14T06:22:17Z

Maybe a debug command to reset a token even without the SO credentials?
The TA supports multi tokens, a token could be reserved for debug purpose, identified from its token label.

If it would have some magic name then perhaps that could enable reset token feature. This magic name would just be something that normally would not be used.

What I would like to have is that we can run xtest (and other tests) on to be released TA binaries to verify that everything is still looking good and if automated tests pass then mark release gate for those TA's as tests has passed. Eg. so that the binary would be intact and no need to compile it differently.

etienne-lms · 2020-12-14T07:28:53Z

Currently xtest lists the slots and picks the last last to run its tests.
We could have xtest to check the token label is "Test token for OP-TEE xtest " so we can safely scratch all even without rights.

etienne-lms

@ruchi393, does this change look ok to you?
@vesajaaskelainen, LGTM (with checkpatch report addressed) but I wait for OP-TEE/optee_os#4222 before posting my R-b tag.
I think this is a nice feature, thanks for sharing.

etienne-lms · 2020-12-14T13:22:14Z

+			if ((errno == ERANGE) || (tmpconv > (gid_t)-1) ||
+			    (login_gid_env + strlen(login_gid_env) != endp)) {


CI/checkpatch issue report:

4e56bdb libckteec: Use environment variables to determine OP-TEE TA login CHECK: Unnecessary parentheses around 'errno == ERANGE' #68: FILE: libckteec/src/invoke_ta.c:267: + if ((errno == ERANGE) || (tmpconv > (gid_t)-1) || + (login_gid_env + strlen(login_gid_env) != endp)) { total: 0 errors, 0 warnings, 1 checks, 63 lines checked

I.e.:

if (errno == ERANGE || tmpconv > (gid_t)-1 || (login_gid_env + strlen(login_gid_env) != endp)) { ... }

$ CHECKPATCH=../linux/scripts/checkpatch.pl ../optee_os/scripts/checkpatch.sh 0eaddfc570b557c39148ff2e7faccaba8e07d55a Checking commit(s): 0eaddfc libckteec: Use environment variables to determine OP-TEE TA login total: 0 errors, 0 warnings, 110 lines checked "[PATCH] libckteec: Use environment variables to determine OP-TEE TA" has no obvious style problems and is ready for submission.

Where as linux == v5.10-rc1.

Then wondering the difference:

$ cp ../optee_os/scripts/checkpatch.sh scripts/ $ CHECKPATCH=../linux/scripts/checkpatch.pl scripts/checkpatch.sh 0eaddfc570b557c39148ff2e7faccaba8e07d55a Checking commit(s): 0eaddfc libckteec: Use environment variables to determine OP-TEE TA login CHECK: Unnecessary parentheses around 'errno == ERANGE' #121: FILE: libckteec/src/invoke_ta.c:267: + if ((errno == ERANGE) || (tmpconv > (gid_t)-1) || + (login_gid_env + strlen(login_gid_env) != endp)) { total: 0 errors, 0 warnings, 1 checks, 110 lines checked

Shall we copy the checkpatch.sh also to this repo?

vesajaaskelainen · 2021-01-06T18:56:58Z

Just rebased on top of master.

etienne-lms

Reviewed-by: Etienne Carriere <[email protected]> with comment on 1st commit.

It would be nice we have something in optee_test/host/xtest/pkcts11_1000.c to test a bit this but it may tricky to test invalid identities.

Add initialization for variables as required by coding convention. Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Vesa Jääskeläinen <[email protected]>

C_Initialize() needs to know what kind of OP-TEE TA login is needed. CKTEEC_LOGIN_TYPE is used to specify login type: - "public" -> TEEC_LOGIN_PUBLIC - "user" -> TEEC_LOGIN_USER - "group" -> TEEC_LOGIN_GROUP When login type is "group" then CKTEEC_LOGIN_GID must be numerical group id. Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Vesa Jääskeläinen <[email protected]>

Allow C_InitToken() to transport a null PIN reference as when protected authentication path is used. Specified in: PKCS OP-TEE#11 Cryptographic Token Interface Base Specification Version 2.40 Plus Errata 01 C_InitPIN Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Vesa Jääskeläinen <[email protected]>

Allow C_Login() to transport a null PIN reference as when protected authentication path is used. Specified in: PKCS OP-TEE#11 Cryptographic Token Interface Base Specification Version 2.40 Plus Errata 01 C_Login Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Vesa Jääskeläinen <[email protected]>

Allow C_InitPIN() to transport a null PIN reference as when protected authentication path is used. Specified in: PKCS OP-TEE#11 Cryptographic Token Interface Base Specification Version 2.40 Plus Errata 01 C_InitPIN Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Vesa Jääskeläinen <[email protected]>

Allow C_SetPIN() to transport a null PIN reference as when protected authentication path is used. Specified in: PKCS OP-TEE#11 Cryptographic Token Interface Base Specification Version 2.40 Plus Errata 01 C_SetPIN Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Vesa Jääskeläinen <[email protected]>

vesajaaskelainen · 2021-01-07T12:52:09Z

Reviewed-by: Etienne Carriere <[email protected]> with comment on 1st commit.

Thanks for the review!

Fixed and applied tags.

vesajaaskelainen · 2021-01-07T12:55:27Z

It would be nice we have something in optee_test/host/xtest/pkcts11_1000.c to test a bit this but it may tricky to test invalid identities.

I believe we need to improve the testing support for tokens in overall first and then we can try to add tests also for these. This is kinda integration testing not unit testing level problem as we would need to navigate to different identities in a way that people can still run xtest successfully.

I would prefer that we do these as separate things.

etienne-lms reviewed Nov 23, 2020

View reviewed changes

Comment thread libckteec/src/invoke_ta.c

vesajaaskelainen force-pushed the token-acl branch from a8ab0b4 to f8cb453 Compare November 23, 2020 19:24

etienne-lms reviewed Nov 24, 2020

View reviewed changes

Comment thread libckteec/src/invoke_ta.c

Comment thread libckteec/src/invoke_ta.c Outdated

Comment thread libckteec/src/invoke_ta.c Outdated

vesajaaskelainen force-pushed the token-acl branch from f8cb453 to bbd669d Compare December 12, 2020 14:38

etienne-lms reviewed Dec 14, 2020

View reviewed changes

etienne-lms mentioned this pull request Dec 16, 2020

pkcs11: Add TEE Identity based authentication support OP-TEE/optee_os#4222

Merged

vesajaaskelainen force-pushed the token-acl branch 2 times, most recently from bd1f882 to 8e8b284 Compare December 20, 2020 19:53

vesajaaskelainen force-pushed the token-acl branch from 8e8b284 to 20a08f8 Compare January 6, 2021 18:55

etienne-lms reviewed Jan 7, 2021

View reviewed changes

Comment thread libckteec/src/invoke_ta.c Outdated

vesajaaskelainen added 6 commits January 7, 2021 14:34

libckteec: invoke_ta: initialize un-initialized variables

4c1f267

Add initialization for variables as required by coding convention. Reviewed-by: Etienne Carriere <[email protected]> Signed-off-by: Vesa Jääskeläinen <[email protected]>

vesajaaskelainen force-pushed the token-acl branch from 20a08f8 to 8b96990 Compare January 7, 2021 12:49

jforissier merged commit 487e341 into OP-TEE:master Jan 7, 2021

ricardosalveti mentioned this pull request Sep 5, 2022

Initial support for rootfs disk encryption foundriesio/meta-lmp#732

Merged

4 tasks

		if ((errno == ERANGE) \|\| (tmpconv > (gid_t)-1) \|\|
		(login_gid_env + strlen(login_gid_env) != endp)) {

Conversation

vesajaaskelainen commented Nov 22, 2020

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vesajaaskelainen commented Nov 23, 2020

Uh oh!

vesajaaskelainen commented Nov 23, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vesajaaskelainen commented Dec 12, 2020

Uh oh!

vesajaaskelainen commented Dec 12, 2020

Uh oh!

etienne-lms commented Dec 13, 2020

Uh oh!

vesajaaskelainen commented Dec 14, 2020

Uh oh!

etienne-lms commented Dec 14, 2020

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

etienne-lms Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

vesajaaskelainen Dec 20, 2020

Choose a reason for hiding this comment

Uh oh!

vesajaaskelainen commented Jan 6, 2021

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vesajaaskelainen commented Jan 7, 2021

Uh oh!

vesajaaskelainen commented Jan 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants