Issue #44: Option to remove False flags from the C_UnwrapKey template. #45

aalba6675 · 2019-02-26T16:54:51Z

Addresses #44.

Option to control the set of flags included in the template sent with C_UnwrapKey: flags that are False are removed.

Needed for some HSMs that don't accept all the flags even if they are set to False.

danni

Good catch, but I think this is slightly the wrong solution. I think it would be better suited on one of the earlier calls that sets up the default template. Or maybe we should just not pass False in to any default template.

aalba6675 · 2019-02-27T00:46:55Z

New squashed commit - for C_UnwrapKey template I only added True flags to template_; this has worked with the Gemalto SafeNet that I have access to.

I'm not familiar with Cython, so unsure where template_ in the method body comes from: tried your second suggestion and update the dict only with MechanismFlags that are True.

danni

I would take this one step further, into a function that took capabilities as an argument and returned a dictionary, of only the True values. And then use it everywhere we mix capabilities into the template.

aalba6675 · 2019-02-27T06:20:18Z

Added a common function to convert to dict, and mix this function into the methods.

I see a problem with this commit: it breaks test_sessions.py:SessionTests::test_generate_key because
I cannot force ENCRYPT to be False using capabilities, I would have to use explicit template. (Softhsm2 uses CKA_ENCRYPT=True as default for secret keys)

Maybe capabilities can be an int or (int, include_mask)-tuple so that the flag is returned in the dict even if False, what do you think?

# with this commit when generating the test key I would have to use
# template = {Attribute.ENCRYPT:  False}
self.assertNotIsInstance(key, pkcs11.EncryptMixin)

If I change the test as below I can get the same test passes as master:

# Create another key with no capabilities
            key = session.generate_key(pkcs11.KeyType.AES, 128,
                                       label='MY KEY',
                                       id=b'\1\2\3\4',
                                       template={pkcs11.Attribute.ENCRYPT: False})
           # capabilities=0 doesn't work here :-(
            self.assertIsInstance(key, pkcs11.Object)
            self.assertIsInstance(key, pkcs11.SecretKey)
            self.assertNotIsInstance(key, pkcs11.EncryptMixin)

            self.assertEqual(key.label, 'MY KEY')

aalba6675 · 2019-02-27T07:25:14Z

Added a commit to fix a test.
On Python 3.7/Fedora 29 I still have one test failure, but this failure is also in master so probably not related to this PR.

danni · 2019-02-28T23:23:41Z

tests/test_sessions.py

@@ -53,7 +53,7 @@ def test_generate_key(self):
            key = session.generate_key(pkcs11.KeyType.AES, 128,
                                       label='MY KEY',
                                       id=b'\1\2\3\4',
-                                       capabilities=0)
+                                       template={pkcs11.Attribute.ENCRYPT: False})


Hmm.. does this break an assumption of the library, if you're not set the capability is that false or default?

Maybe we're thinking about this all wrong, and you get the default OR the capabilities you've set only, without merging.

So capabilities=None -> default set of True's, no falses. capabilities is not None Trues for set capabilities. No falses. That way this test won't have to be changed. And for anything not specified you get the HSM's default, which should be sensible.

For this test we don't know the HSM defaults so we explicitly need to set ENCRYPT to False, this is achieved by the current behaviour which set all flags to False.

Not sure what you mean by "So capabilities=None -> default set of True's, no falses. capabilities is not None Trues for set capabilities. No falses." Won't we still have to force the flag to False somehow?

I'm not sure what I mean either. I think, I've been thinking about this all wrong.

We've got 3 states: true, false and left-out (default). In this test we wanted to pass False so we unset the capability. That is intuitive to me.

This has worked for everything except for HSMs that don't accept the Attribute at all. There have been a few times where it could have been useful to remove a value out of the template. Perhaps what we need is a new constant DEFAULT, which is filtered out of the final merged template.

Something like this:

template={ pkcs11.Attribute.UNWANTED_ATTRIBUTE: pkcs11.DEFAULT }

We could then filter out all DEFAULT values from the remaining template. I'm happy to code this up if you're not keen.

So I have tried to code this up - there is no change to the tests, capabilities=N does what it does before and sets the library default flags.

If the user really does not want the attr sent to the HSM, the library can be overridden with the semantics you suggest:

# my HSM does not want ENCRYPT VERIFY WRAP on private keys but # for unwrap the library sets all 7 flags to either True/False, so... obj.unwrap_key( .... template={Attribute.ENCRYPT: pkcs11.DEFAULT, Attribute.VERIFY: pkcs11.DEFAULT, Attribute.WRAP: pkcs11.DEFAULT})

danni · 2019-03-03T07:00:35Z

pkcs11/constants.py

+    Wraps DEFAULT so we don't accidentally coincide
+    with a desired value of 0xFFFFFFFF
+    """
+    DEFAULT = 0xFFFFFFFF


Use the sentinel object pattern. IntEnum is especially dangerous because 0xffffffff might be a valid value for something (indeed I think it is for mantissa).

DEFAULT = object() if value is DEFAULT: ....

danni · 2019-03-03T07:08:02Z

pkcs11/_pkcs11.pyx

@@ -293,6 +293,18 @@ class SearchIter:
            self.session._operation_lock.release()


+def _finalize_template(template_, template):


I like to avoid functions that mutate their inputs. I would do this like:

def merge_templates(default_template, *user_templates): template = default_template.copy() for user_template in user_templates: if user_template is not None: template.update(user_template) return { key: value for key, value in template.items() if value is not DEFAULT }

It's also a good opportunity to name things what they are.

danni · 2019-03-03T07:16:45Z

pkcs11/_pkcs11.pyx

@@ -348,7 +360,7 @@ class Session(types.Session):
            Attribute.TOKEN: store,
            Attribute.PRIME_BITS: param_length,
        }
-        template_.update(template or {})
+        _finalize_template(template_, template)


You conceivably could then remove all of the weird underscore confusion with something like

attrs = AttributeList(merge_templates({ default: template, goes: here, }, template))

Which I think would make the code a lot more readable.

You could also try making this a @classmethod on AttributeList (e.g. from_template). Doing this means you would have to deal with Cython's binding nonsense, see the tests for examples.

aalba6675 · 2019-03-03T10:04:35Z

Using the non-mutating function recommendation as the MVP path.

danni · 2019-03-03T21:58:35Z

pkcs11/constants.py

@@ -11,6 +11,9 @@
    from aenum import IntEnum, IntFlag, unique


+# sentinel value to remove attributes from template
+DEFAULT = object()


Add a docstring for this so that it turns up in the documentation :)

danni · 2019-03-03T21:59:31Z

Looks good. If you can make sure DEFAULT is documented this is good to merge.

and use the HSM default values Addresses pyauth#44

aalba6675 · 2019-03-04T01:27:36Z

Great - done! Thanks.

danni · 2019-03-04T02:28:23Z

Thanks for this. I'll do a release as soon as I've sorted out the Windows unit testing.

aalba6675 force-pushed the fix-issue44 branch from 214a38d to 511ee1f Compare February 26, 2019 17:04

danni requested changes Feb 26, 2019

View reviewed changes

aalba6675 force-pushed the fix-issue44 branch from 511ee1f to 0a849d4 Compare February 27, 2019 00:41

danni reviewed Feb 27, 2019

View reviewed changes

aalba6675 force-pushed the fix-issue44 branch from 0a849d4 to 1b8a83c Compare February 27, 2019 06:17

danni reviewed Feb 28, 2019

View reviewed changes

aalba6675 force-pushed the fix-issue44 branch 2 times, most recently from 4b72364 to 4f0f1e7 Compare March 3, 2019 02:01

danni reviewed Mar 3, 2019

View reviewed changes

danni mentioned this pull request Mar 3, 2019

CloudHSM Support #14

Open

aalba6675 force-pushed the fix-issue44 branch 3 times, most recently from 5eba1b7 to 0bde0a5 Compare March 3, 2019 09:56

danni reviewed Mar 3, 2019

View reviewed changes

add a pseudo-flag to remove an attribute from the library templates

e6107cb

and use the HSM default values Addresses pyauth#44

aalba6675 force-pushed the fix-issue44 branch from 0bde0a5 to e6107cb Compare March 4, 2019 01:27

danni approved these changes Mar 4, 2019

View reviewed changes

danni merged commit bc372de into pyauth:master Mar 4, 2019

aalba6675 deleted the fix-issue44 branch March 4, 2019 04:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #44: Option to remove False flags from the C_UnwrapKey template. #45

Issue #44: Option to remove False flags from the C_UnwrapKey template. #45

aalba6675 commented Feb 26, 2019

danni left a comment

aalba6675 commented Feb 27, 2019

danni left a comment

aalba6675 commented Feb 27, 2019 •

edited

Loading

aalba6675 commented Feb 27, 2019 •

edited

Loading

danni Feb 28, 2019

aalba6675 Mar 1, 2019 •

edited

Loading

danni Mar 2, 2019

aalba6675 Mar 3, 2019 •

edited

Loading

danni Mar 3, 2019

danni Mar 3, 2019

danni Mar 3, 2019

aalba6675 commented Mar 3, 2019

danni Mar 3, 2019

danni commented Mar 3, 2019

aalba6675 commented Mar 4, 2019

danni commented Mar 4, 2019

		@@ -293,6 +293,18 @@ class SearchIter:
		self.session._operation_lock.release()


		def _finalize_template(template_, template):

Issue #44: Option to remove False flags from the C_UnwrapKey template. #45

Issue #44: Option to remove False flags from the C_UnwrapKey template. #45

Conversation

aalba6675 commented Feb 26, 2019

danni left a comment

Choose a reason for hiding this comment

aalba6675 commented Feb 27, 2019

danni left a comment

Choose a reason for hiding this comment

aalba6675 commented Feb 27, 2019 • edited Loading

aalba6675 commented Feb 27, 2019 • edited Loading

danni Feb 28, 2019

Choose a reason for hiding this comment

aalba6675 Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

danni Mar 2, 2019

Choose a reason for hiding this comment

aalba6675 Mar 3, 2019 • edited Loading

Choose a reason for hiding this comment

danni Mar 3, 2019

Choose a reason for hiding this comment

danni Mar 3, 2019

Choose a reason for hiding this comment

danni Mar 3, 2019

Choose a reason for hiding this comment

aalba6675 commented Mar 3, 2019

danni Mar 3, 2019

Choose a reason for hiding this comment

danni commented Mar 3, 2019

aalba6675 commented Mar 4, 2019

danni commented Mar 4, 2019

aalba6675 commented Feb 27, 2019 •

edited

Loading

aalba6675 commented Feb 27, 2019 •

edited

Loading

aalba6675 Mar 1, 2019 •

edited

Loading

aalba6675 Mar 3, 2019 •

edited

Loading