Add AES Key Derivation Support (ECB,CBC Encrypt) #97

aXedge-arm · 2020-10-09T14:38:52Z

Additional Information:

Added AES Key Derivation mechanisms described in
PKCS11 v2.4.0 Section 2.15:

CKM_AES_ECB_ENCRYPT_DATA
CKM_AES_CBC_ENCRYPT_DATA

Sanity Testing:

Build Validation:
python setup.py build_ext --inplace
AES Testing:
export PKCS11_MODULE=XXX
export PKCS11_TOKEN_LABEL=XXX
export PKCS11_TOKEN_PIN=XXX
export PKCS11_TOKEN_SO_PIN=XXX
pytest ./tests/test_aes.py

Signed-off-by: Akshitij Malik

Additional Information: 1. Added AES Key Derivation mechanisms described in PKCS11 v2.4.0 Section 2.15: - CKM_AES_ECB_ENCRYPT_DATA - CKM_AES_CBC_ENCRYPT_DATA Sanity Testing: 1. Build Validation: python setup.py build_ext --inplace 2. AES Testing: export PKCS11_MODULE=XXX export PKCS11_TOKEN_LABEL=XXX export PKCS11_TOKEN_PIN=XXX export PKCS11_TOKEN_SO_PIN=XXX pytest ./tests/test_aes.py Signed-off-by: Akshitij Malik

danni

Needs tests.

danni · 2020-10-11T22:20:55Z

pkcs11/_pkcs11.pyx

+            self.param = aes_cbc_params = \
+                    <CK_AES_CBC_ENCRYPT_DATA_PARAMS *> PyMem_Malloc(paramlen)
+            (iv, data) = param
+            aes_cbc_params.iv = [<CK_BYTE> x for x in iv[:16]]


Should be able to express this without a comprehension.

danni · 2020-10-11T22:30:39Z

pkcs11/_pkcs11.pyx

        cdef CK_RSA_PKCS_PSS_PARAMS *pss_params
        cdef CK_ECDH1_DERIVE_PARAMS *ecdh1_params
+        cdef CK_KEY_DERIVATION_STRING_DATA *aes_ecb_params
+        cdef CK_AES_CBC_ENCRYPT_DATA_PARAMS *aes_cbc_params


This is getting to be a good indication that the mechanism types above should be implemented as subclasses of MechanismWithParam (which should be renamed), mechanism_param keyword should be removed and this class allowed as a parameter to mechanism.

This would allow mechanism=RSA_PKCS, mechanism=Mechanism(AES_CBC, param=iv) or mechanism=mechanisms.RSA_PKCS_OAEP(params=...) with proper typing and a much better point for implementing extensions beyond this package.

That doesn't have to be done now, just a note.

So shall this be added as a feature request, or do you want to take this up now?

If one has been created, I could add its link here in order to complete the tracking of this suggestion.

If you wanted to give refactoring it a go, I'd be keen. I've opened #98 to track it.

hi Danni, if it's okay with you, I'll attempt that once these changes have been merged.

Additional Information: 1. Added AES Key Derivation mechanisms described in PKCS11 v2.4.0 Section 2.15: - CKM_AES_ECB_ENCRYPT_DATA - CKM_AES_CBC_ENCRYPT_DATA 2. Incorporated code-review comments: - directly sliced the IV data to extract mechanism_params for CBC_ENCRYPT, - added Unit Tests for ECB_ENCRYPT - test_derive_ecb_encrypt - added Unit Tests for CBC_ENCRYPT - test_derive_cbc_encrypt Sanity Testing: 1. Build Validation: python setup.py build_ext --inplace 2. AES Testing: export PKCS11_MODULE=XXX export PKCS11_TOKEN_LABEL=XXX export PKCS11_TOKEN_PIN=XXX export PKCS11_TOKEN_SO_PIN=XXX pytest ./tests/test_aes.py Signed-off-by: Akshitij Malik

danni · 2020-10-20T22:40:46Z

tests/test_aes.py


 from . import TestCase, requires, FIXME

+from parameterized import parameterized


Missing from dev-requirements.{in,txt}

danni · 2020-10-20T22:41:22Z

tests/test_aes.py

+                pkcs11.exceptions.FunctionFailed) as e:
+            derived_key = None
+
+        if test_type.startswith("NEGATIVE"):


Split these into two separate tests.

Although, now I think about it, do we honestly need to parameterize so many tests? It feels here like we're actually testing the PKCS#11 library, not the wrapper.

Essentially these tests validate what "should" work with PKCS11.
I agree that they might be doing more than just verify our changes, but I felt that these tests might also serve as reference points of what works vs what shall not work. this might be useful for any other newbie developers (like me).
I would still remove them, if you like.
in the meantime, I would upload the test split across key_derivation-only and key_derivation+data_enc/dec
I await your feedback :-)

I'm okay to keep the parameterised, I just don't like branches in tests. It means you effectively now need to be coverage testing your tests to know if you got it all.

Consider passing in func which can either be assertIsNone or assertIsNotNone and not having a branch at all. func might have to be a string that calls getattr(self, func)(...), but that will still fail hard if the function doesn't exist.

danni · 2020-10-20T22:41:40Z

tests/test_aes.py

+                IndexError) as e:
+            derived_key = None
+
+        if test_type.startswith("NEGATIVE"):


Split these into two separate tests.

Additional Information: 1. Added AES Key Derivation mechanisms described in PKCS11 v2.4.0 Section 2.15: - CKM_AES_ECB_ENCRYPT_DATA - CKM_AES_CBC_ENCRYPT_DATA 2. Incorporated code-review comments: - directly sliced the IV data to extract mechanism_params for CBC_ENCRYPT, - added Unit Tests for ECB_ENCRYPT - test_derive_ecb_encrypt - added Unit Tests for CBC_ENCRYPT - test_derive_cbc_encrypt - updated dev-requirements - split the Unit Tests into 2 phases: - key-derivation tests - data encrypyion/decryption tests Sanity Testing: 1. Build Validation: python setup.py build_ext --inplace 2. AES Testing: export PKCS11_MODULE=XXX export PKCS11_TOKEN_LABEL=XXX export PKCS11_TOKEN_PIN=XXX export PKCS11_TOKEN_SO_PIN=XXX pytest ./tests/test_aes.py Signed-off-by: Akshitij Malik

Additional Information: 1. Added AES Key Derivation mechanisms described in PKCS11 v2.4.0 Section 2.15: - CKM_AES_ECB_ENCRYPT_DATA - CKM_AES_CBC_ENCRYPT_DATA 2. Incorporated code-review comments: - directly sliced the IV data to extract mechanism_params for CBC_ENCRYPT, - added Unit Tests for ECB_ENCRYPT - test_derive_ecb_encrypt - added Unit Tests for CBC_ENCRYPT - test_derive_cbc_encrypt - updated dev-requirements - split the Unit Tests into 2 phases: - key-derivation tests - data encrypyion/decryption tests - replaced f-strings with python3.5 compatible format strings Sanity Testing: 1. Build Validation: python setup.py build_ext --inplace 2. AES Testing: export PKCS11_MODULE=XXX export PKCS11_TOKEN_LABEL=XXX export PKCS11_TOKEN_PIN=XXX export PKCS11_TOKEN_SO_PIN=XXX pytest ./tests/test_aes.py Signed-off-by: Akshitij Malik

Additional Information: 1. Added AES Key Derivation mechanisms described in PKCS11 v2.4.0 Section 2.15: - CKM_AES_ECB_ENCRYPT_DATA - CKM_AES_CBC_ENCRYPT_DATA 2. Incorporated code-review comments: - directly sliced the IV data to extract mechanism_params for CBC_ENCRYPT, - added Unit Tests for ECB_ENCRYPT - test_derive_ecb_encrypt - added Unit Tests for CBC_ENCRYPT - test_derive_cbc_encrypt - updated dev-requirements - split the Unit Tests into 2 phases: - key-derivation tests - data encrypyion/decryption tests - replaced f-strings with python3.5 compatible format strings - negative test rework: - prevent branching by passing assertIsNotNone and assertIsNone as assertion functions for Positive & Negative test cases. Sanity Testing: 1. Build Validation: python setup.py build_ext --inplace 2. AES Testing: export PKCS11_MODULE=XXX export PKCS11_TOKEN_LABEL=XXX export PKCS11_TOKEN_PIN=XXX export PKCS11_TOKEN_SO_PIN=XXX pytest ./tests/test_aes.py Signed-off-by: Akshitij Malik

danni · 2020-10-29T01:04:51Z

Thanks for this 👍

danni reviewed Oct 11, 2020

View reviewed changes

danni mentioned this pull request Oct 13, 2020

Refactor MechanismWithParam #98

Open

danni reviewed Oct 20, 2020

View reviewed changes

Akshitij Malik added 3 commits October 26, 2020 11:05

danni approved these changes Oct 29, 2020

View reviewed changes

danni merged commit c148a2f into pyauth:master Oct 29, 2020


		from . import TestCase, requires, FIXME

		from parameterized import parameterized

Add AES Key Derivation Support (ECB,CBC Encrypt) #97

Add AES Key Derivation Support (ECB,CBC Encrypt) #97

Uh oh!

Conversation

aXedge-arm commented Oct 9, 2020

Uh oh!

danni left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danni commented Oct 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants