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.

Sign up for GitHub

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

Jump to bottom

Pkcs11 login #418

Merged
merged 4 commits into from
Apr 16, 2020
Merged

Pkcs11 login #418

merged 4 commits into from
Apr 16, 2020

Conversation

jenswi-linaro
Copy link
Contributor

No description provided.

@jenswi-linaro jenswi-linaro mentioned this pull request Apr 14, 2020
Merged
etienne-lms
etienne-lms reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "pkcs11: 1003: test C_InitToken()"

host/xtest/pkcs11_1000.c Outdated
}

ADBG_CASE_DEFINE(pkcs11, 1003, xtest_pkcs11_test_1003,
"PKCS11: Login to PKCS#11 token");
Copy link
Contributor

Choose a reason for hiding this comment

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

indent with one more space char.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix

host/xtest/pkcs11_1000.c Outdated
static CK_RV init_test_token(CK_SLOT_ID slot)
{
return C_InitToken(slot,
(CK_UTF8CHAR_PTR)test_token_so_pin,
Copy link
Contributor

Choose a reason for hiding this comment

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

cast not needed here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix

etienne-lms
etienne-lms reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

one more minor comment for same commit.

host/xtest/pkcs11_1000.c Outdated
@@ -425,6 +425,9 @@ ADBG_CASE_DEFINE(pkcs11, 1002, xtest_pkcs11_test_1002,
* These define the genuine PINs and label to be used with the test token.
*/
static CK_UTF8CHAR test_token_so_pin[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8 , 9, 10,};
Copy link
Contributor

Choose a reason for hiding this comment

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

add a space before }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

etienne-lms
etienne-lms reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "pkcs11: 1003: test C_Login() and C_Logout()"

host/xtest/pkcs11_1000.c Outdated
if (!ADBG_EXPECT_CK_OK(c, rv))
goto out;

/* Login user then so and reverse */
Copy link
Contributor

Choose a reason for hiding this comment

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

s/so/security officer/ or s/so/SO/

host/xtest/pkcs11_1000.c Outdated
@@ -639,6 +720,13 @@ static void xtest_pkcs11_test_1003(ADBG_Case_t *c)
!ADBG_EXPECT_NOT_NULL(c, ckfunc_list->C_InitPIN) ||
!ADBG_EXPECT_NOT_NULL(c, ckfunc_list->C_SetPIN))
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the test above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

host/xtest/pkcs11_1000.c
if (rv != CKR_OK)
goto out;


Copy link
Contributor

Choose a reason for hiding this comment

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

some space for further tests? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! :-)

etienne-lms
etienne-lms reviewed Apr 14, 2020
View reviewed changes
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

for commit "pkcs11: 1003: test C_SetPIN()"

host/xtest/pkcs11_1000.c
if (ut == user_type)
goto out_logout;
else
goto out_session;
Copy link
Contributor

Choose a reason for hiding this comment

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

Failing C_SetPIN() is not expected to log out the currently logged user. This should goto out_logout; in both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the case where user_type isn't CKU_SO or CKU_USER we're not logged in, that's why we're skipping the logout.

host/xtest/pkcs11_1000.c Outdated
return rv;
}


Copy link
Contributor

Choose a reason for hiding this comment

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

remove 1 empty line

host/xtest/pkcs11_1000.c Outdated
*/
rv = test_set_pin(c, slot, CKU_CONTEXT_SPECIFIC);
if (rv != CKR_OK)
goto out;
Copy link
Contributor

Choose a reason for hiding this comment

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

add an empty line below. Or maybe remove lines 839 and 840.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this redundant test.

@jenswi-linaro
Copy link
Contributor Author

Update

@etienne-lms etienne-lms mentioned this pull request Apr 15, 2020
Merged
Emantor
Emantor approved these changes Apr 16, 2020
View reviewed changes
Copy link

@Emantor Emantor left a comment

Choose a reason for hiding this comment

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

Acked-by: Rouven Czerwinski <[email protected]>

@jenswi-linaro
pkcs11: 1003: test C_InitToken()
a65279a 
Test C_InitToken() on uninitialized and already initialized token.

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
@jenswi-linaro
pkcs11: 1003: test C_InitPIN()
c1eea71 
Test C_InitPIN().

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
@jenswi-linaro
pkcs11: 1003: test C_Login() and C_Logout()
39badcc 
Test C_Login() and C_Logout().

Acked-by: Rouven Czerwinski <[email protected]>
Co-developed-by: Etienne Carriere <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
@jenswi-linaro
pkcs11: 1003: test C_SetPIN()
f4b02af 
Test C_SetPIN().

Acked-by: Rouven Czerwinski <[email protected]>
Signed-off-by: Jens Wiklander <[email protected]>
@jenswi-linaro
Copy link
Contributor Author

Squashed, rebased and tag applied.

@jenswi-linaro
Copy link
Contributor Author

Update

@jforissier jforissier merged commit 6ce1e69 into OP-TEE:master Apr 16, 2020
@jenswi-linaro jenswi-linaro deleted the pkcs11_login branch April 16, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etienne-lms etienne-lms etienne-lms left review comments

@Emantor Emantor Emantor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jenswi-linaro @Emantor @etienne-lms @jforissier