-
Notifications
You must be signed in to change notification settings - Fork 240
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
Conversation
There was a problem hiding this 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.
libckteec/include/pkcs11.h
Outdated
@@ -492,6 +495,15 @@ struct CK_GCM_PARAMS { | |||
CK_ULONG ulTagBits; | |||
}; | |||
|
|||
/* EDDSA */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
libckteec/include/pkcs11.h
Outdated
@@ -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 |
There was a problem hiding this comment.
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 */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
libckteec/include/pkcs11.h
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
libckteec/include/pkcs11_ta.h
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
aba80ff
to
be84913
Compare
There was a problem hiding this 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.
libckteec/include/pkcs11.h
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
libckteec/include/pkcs11_ta.h
Outdated
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove 1 tab
There was a problem hiding this comment.
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]>
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