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

libckteec: Add EDDSA attribute serialization #324

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

varder
Copy link

@varder varder commented Sep 29, 2022

The PKCS#11 Specification:
https://docs.oasis-open.org/pkcs11/pkcs11-spec/v3.1/cs01/pkcs11-spec-v3.1-cs01.pdf

The pull request should be merged with the following pull requests:
OP-TEE/optee_os#5559
OP-TEE/optee_test#618

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.

LGTM aside these few comments.

@@ -492,6 +495,15 @@ struct CK_GCM_PARAMS {
CK_ULONG ulTagBits;
};

/* EDDSA */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: /* EdDSA (RFC 8032) */ as that's the reference we find in the litterature?

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -230,6 +230,7 @@ typedef CK_KEY_TYPE *CK_KEY_TYPE_PTR;
#define CKK_DH 0x002
#define CKK_ECDSA 0x003
#define CKK_EC 0x003
#define CKK_EC_EDWARDS 0x040
Copy link
Contributor

Choose a reason for hiding this comment

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

to move at the end (numerical order)

I propose you mention that this is from pkcs11 v3.1-cs01:
`#define CKK_EC_EDWARDS 0x040 /* from PKCS#11 v3.1-cs01 */

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@@ -350,6 +352,7 @@ typedef CK_MECHANISM_TYPE *CK_MECHANISM_TYPE_PTR;
#define CKM_ECDH1_COFACTOR_DERIVE 0x01051
#define CKM_ECMQV_DERIVE 0x01052
#define CKM_ECDH_AES_KEY_WRAP 0x01053
#define CKM_EDDSA 0x01057
Copy link
Contributor

Choose a reason for hiding this comment

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

 #define CKM_ECMQV_DERIVE		0x01052
 #define CKM_ECDH_AES_KEY_WRAP		0x01053
 #define CKM_RSA_AES_KEY_WRAP		0x01054
+#define CKM_EC_EDWARDS_KEY_PAIR_GEN	0x01055
+#define CKM_EDDSA			0x01057
 #define CKM_AES_KEY_GEN			0x01080
 #define CKM_AES_ECB			0x01081

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@@ -1276,6 +1276,7 @@ enum pkcs11_mechanism_id {
PKCS11_CKM_ECDSA_SHA512 = 0x01046,
PKCS11_CKM_ECDH1_DERIVE = 0x01050,
PKCS11_CKM_ECDH1_COFACTOR_DERIVE = 0x01051,
PKCS11_CKM_EDDSA = 0x01057,
Copy link
Contributor

Choose a reason for hiding this comment

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

move below

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@varder varder force-pushed the eddsa branch 2 times, most recently from aba80ff to be84913 Compare October 3, 2022 14:45
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.

Reviewed-by: Etienne Carriere <[email protected]> with 2 minor comments addressed.

@@ -351,6 +352,8 @@ typedef CK_MECHANISM_TYPE *CK_MECHANISM_TYPE_PTR;
#define CKM_ECMQV_DERIVE 0x01052
#define CKM_ECDH_AES_KEY_WRAP 0x01053
#define CKM_RSA_AES_KEY_WRAP 0x01054
#define CKM_EC_EDWARDS_KEY_PAIR_GEN 0x01055
#define CKM_EDDSA 0x01057
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking: remove 1 or 2 tabulations

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@@ -1279,6 +1279,7 @@ enum pkcs11_mechanism_id {
PKCS11_CKM_ECMQV_DERIVE = 0x01052,
PKCS11_CKM_ECDH_AES_KEY_WRAP = 0x01053,
PKCS11_CKM_RSA_AES_KEY_WRAP = 0x01054,
PKCS11_CKM_EDDSA = 0x01057,
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 tab

Copy link
Author

Choose a reason for hiding this comment

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

fixed

The PKCS#11 Specification:
https://docs.oasis-open.org/pkcs11/pkcs11-spec/v3.1/cs01/
pkcs11-spec-v3.1-cs01.pdf
6.3.16 EC mechanism parameters

Signed-off-by: Valerii Chubar <[email protected]>
Reviewed-by: Etienne Carriere <[email protected]>
@jforissier jforissier merged commit 140bf46 into OP-TEE:master Oct 6, 2022
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.

3 participants