Hi,

On 03/24/2015 04:40 PM, Martin Basti wrote:
On 24/03/15 14:41, Milan Kubik wrote:
Hello,

thanks for the review.

On 03/24/2015 12:39 PM, Martin Basti wrote:
On 17/03/15 10:38, Milan Kubik wrote:
Hi,

On 03/16/2015 05:23 PM, Martin Basti wrote:
On 16/03/15 15:32, Milan Kubik wrote:
On 03/16/2015 12:03 PM, Milan Kubik wrote:
On 03/13/2015 02:59 PM, Milan Kubik wrote:
Hi,

this is a patch with port of [1] to pytest.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

Cheers,
Milan



Added few more asserts in methods where the test could fail and cause other errors.


New version of the patch after brief discussion with Martin Basti. Removed unnecessary variable assignments and separated a new test case.


Hello,

thank you for the patch.
I have a few nitpicks:
1)
You can remove this and use just hexlify(s)
+def str_to_hex(s):
+    return ''.join("{:02x}".format(ord(c)) for c in s)
done

2)
+ def test_find_secret_key(self, p11):
+ assert p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY, label=u"žžž-aest")

In tests before you tested the exact number of expected IDs returned by find_keys method, why not here?
Lack of attention.
Fixed the assert in `test_search_for_master_key` which does the same thing. Merged `test_find_secret_key` with `test_search_for_master_key` where it belongs.

Martin^2

Milan


Thank you for patches, just two nitpicks:

1)
Can you use the ipaplatform.paths constant? This is platform specific.
LIBSOFTHSM2_SO = "/usr/lib/pkcs11/libsofthsm2.so"
LIBSOFTHSM2_SO_64 = "/usr/lib64/pkcs11/libsofthsm2.so"

Respectively use just LIBSOFTHSM2_SO, on 64bit systems it is automatically mapped into LIBSOFTHSM2_SO_64

instead of:
+
+libsofthsm = "/usr/lib64/pkcs11/libsofthsm2.so"
+

Done.
2)
Can you please check if keys were really deleted?
+    def test_delete_key(self, p11):
Done.
--
Martin Basti

I also moved `test_search_for_master_key` right after `test_generate_master_key` and changed the assert message to a more specific one.

Cheers,
Milan
Please fix this:

1)
$ git am freeipa-mkubik-0001-5-ipatests-port-of-p11helper-test-from-github.patch
Applying: ipatests: port of p11helper test from github
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:228: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

fixed (TIL: vim doesn't show the last empty line)
2) Please respect PEP8 if it is possible
Mostly done, there are few instances of long variable names off by few characters.

3)
I'm still not sure with this:
assert len(master_key) == 0, "The master key should be deleted."

following example is more pythonic
assert not master_key, "The master key...."

Changed to the latter variant.
4)
Related to 3), should we test return value, if correct type was returned?
assert isinstance(master_key, list) and not master_key, "....."
I do not insist on this.

Otherwise it works as expected.
--
Martin Basti

Milan
>From 64308fc10192ed7892845dd17d5bcb42846d55c2 Mon Sep 17 00:00:00 2001
From: Milan Kubik <mku...@redhat.com>
Date: Thu, 12 Mar 2015 16:52:33 +0100
Subject: [PATCH] ipatests: port of p11helper test from github

Ported the github hosted [1] script to use pytest's abilities
and included it in ipatests/test_ipapython directory.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

https://fedorahosted.org/freeipa/ticket/4829
---
 ipatests/test_ipapython/test_ipap11helper.py | 271 +++++++++++++++++++++++++++
 make-lint                                    |   2 +-
 2 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 ipatests/test_ipapython/test_ipap11helper.py

diff --git a/ipatests/test_ipapython/test_ipap11helper.py b/ipatests/test_ipapython/test_ipap11helper.py
new file mode 100644
index 0000000000000000000000000000000000000000..56083c96aa935c36e83eacfc510afbe75c0c78ab
--- /dev/null
+++ b/ipatests/test_ipapython/test_ipap11helper.py
@@ -0,0 +1,271 @@
+# -*- coding: utf-8 -*-
+# Authors:
+#   Milan Kubik <mku...@redhat.com>
+#
+# Copyright (C) 2015  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+"""
+Test the `ipapython/ipap11helper/p11helper.c` module.
+"""
+
+
+from binascii import hexlify
+import os
+import os.path
+import logging
+import subprocess
+import tempfile
+
+import pytest
+from ipaplatform.paths import paths
+
+import _ipap11helper
+
+
+CONFIG_DATA = """
+# SoftHSM v2 configuration file
+directories.tokendir = %s/tokens
+objectstore.backend = file
+"""
+
+LIBSOFTHSM = paths.LIBSOFTHSM2_SO
+SOFTHSM2_UTIL = paths.SOFTHSM2_UTIL
+
+logging.basicConfig(level=logging.INFO)
+log = logging.getLogger('t')
+
+
+@pytest.fixture(scope="module")
+def p11(request):
+    token_path = tempfile.mkdtemp(prefix='pytest_', suffix='_pkcs11')
+    os.chdir(token_path)
+    os.mkdir('tokens')
+
+    with open('softhsm2.conf', 'w') as cfg:
+        cfg.write(CONFIG_DATA % token_path)
+    os.environ['SOFTHSM2_CONF'] = os.path.join(token_path, 'softhsm2.conf')
+    subprocess.check_call([SOFTHSM2_UTIL, '--init-token', '--slot', '0',
+                           '--label', 'test', '--pin', '1234', '--so-pin',
+                           '1234'])
+
+    try:
+        p11 = _ipap11helper.P11_Helper(0, "1234", LIBSOFTHSM)
+    except _ipap11helper.Error:
+        pytest.fail('Failed to initialize the helper object.', pytrace=False)
+
+    def fin():
+        try:
+            p11.finalize()
+        except _ipap11helper.Error:
+            pytest.fail('Failed to finalize the helper object.', pytrace=False)
+        finally:
+            del os.environ['SOFTHSM2_CONF']
+
+    request.addfinalizer(fin)
+
+    return p11
+
+
+class test_p11helper(object):
+    def test_generate_master_key(self, p11):
+        assert p11.generate_master_key(u"žžž-aest", "m", key_length=16,
+                                       cka_wrap=True, cka_unwrap=True)
+
+    def test_search_for_master_key(self, p11):
+        master_key = p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
+                                   label=u"žžž-aest", id="m")
+        assert len(master_key) == 1, "The master key should exist."
+
+    def test_generate_replica_key_pair(self, p11):
+        assert p11.generate_replica_key_pair(u"replica1", "id1",
+                                             pub_cka_wrap=True,
+                                             priv_cka_unwrap=True)
+
+    def test_find_key(self, p11):
+        rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                 label=u"replica1", cka_wrap=True)
+        assert len(rep1_pub) == 1, ("replica key pair has to contain "
+                                    "1 pub key instead of %s" % len(rep1_pub))
+
+        rep1_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY,
+                                  label=u"replica1", cka_unwrap=True)
+        assert len(rep1_priv) == 1, ("replica key pair has to contain 1 "
+                                     "private key instead of %s" %
+                                     len(rep1_priv))
+
+    def test_find_key_by_uri(self, p11):
+        rep1_pub = p11.find_keys(uri="pkcs11:object=replica1;objecttype=public")
+        assert len(rep1_pub) == 1, ("replica key pair has to contain 1 pub "
+                                    "key instead of %s" % len(rep1_pub))
+
+    def test_get_attribute_from_object(self, p11):
+        rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                 label=u"replica1", cka_wrap=True)[0]
+
+        iswrap = p11.get_attribute(rep1_pub, _ipap11helper.CKA_WRAP)
+        assert iswrap is True, "replica public key has to have CKA_WRAP = TRUE"
+
+    def test_generate_replica_keypair_with_extractable_private_key(self, p11):
+        assert p11.generate_replica_key_pair(u"replica2", "id2",
+                                             pub_cka_wrap=True,
+                                             priv_cka_unwrap=True,
+                                             priv_cka_extractable=True)
+
+    def test_find_key_on_nonexistent_key_pair(self, p11):
+        test_list = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                  label=u"replica666")
+        assert len(test_list) == 0, ("list should be empty because label "
+                                     "replica666 should not exist")
+
+    def test_export_import_of_public_key(self, p11):
+        rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                 label=u"replica1", cka_wrap=True)[0]
+        pub = p11.export_public_key(rep1_pub)
+
+        log.debug("Exported public key %s", hexlify(pub))
+        with open("public_key.asn1.der", "wb") as f:
+            f.write(pub)
+
+        rep1_pub_import = p11.import_public_key(u'replica1-import',
+                                                'replica1-import-id',
+                                                pub,
+                                                cka_wrap=True)
+        log.debug('imported replica 1 public key: %s', rep1_pub_import)
+
+        # test public key import
+        rep1_modulus_orig = p11.get_attribute(rep1_pub,
+                                              _ipap11helper.CKA_MODULUS)
+        rep1_modulus_import = p11.get_attribute(rep1_pub_import,
+                                                _ipap11helper.CKA_MODULUS)
+        log.debug('rep1_modulus_orig   = 0x%s', hexlify(rep1_modulus_orig))
+        log.debug('rep1_modulus_import = 0x%s', hexlify(rep1_modulus_import))
+        assert rep1_modulus_import == rep1_modulus_orig
+
+        rep1_pub_exp_orig = p11.get_attribute(rep1_pub,
+                                              _ipap11helper.CKA_PUBLIC_EXPONENT)
+        rep1_pub_exp_import = p11.get_attribute(rep1_pub_import,
+                                                _ipap11helper.CKA_PUBLIC_EXPONENT)
+        log.debug('rep1_pub_exp_orig   = 0x%s', hexlify(rep1_pub_exp_orig))
+        log.debug('rep1_pub_exp_import = 0x%s', hexlify(rep1_pub_exp_import))
+        assert rep1_pub_exp_import == rep1_pub_exp_orig
+
+    def test_wrap_unwrap_key_by_master_key_with_AES(self, p11):
+        master_key = p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
+                                   label=u"žžž-aest", id="m")[0]
+        rep2_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY,
+                                  label=u"replica2", cka_unwrap=True)[0]
+
+        log.debug("wrapping dnssec priv key by master key")
+        wrapped_priv = p11.export_wrapped_key(rep2_priv, master_key,
+                                              _ipap11helper.MECH_AES_KEY_WRAP_PAD)
+        assert wrapped_priv
+
+        log.debug("wrapped_dnssec priv key: %s", hexlify(wrapped_priv))
+        with open("wrapped_priv.der", "wb") as f:
+            f.write(wrapped_priv)
+
+        assert p11.import_wrapped_private_key(u'test_import_wrapped_priv',
+                                              '666',
+                                              wrapped_priv,
+                                              master_key,
+                                              _ipap11helper.MECH_AES_KEY_WRAP_PAD,
+                                              _ipap11helper.KEY_TYPE_RSA)
+
+    def test_wrap_unwrap_key_by_master_key_with_RSA_PKCS(self, p11):
+        master_key = p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
+                                   label=u"žžž-aest", id="m")[0]
+        rep2_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                 label=u"replica2", cka_wrap=True)[0]
+        rep2_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY,
+                                  label=u"replica2", cka_unwrap=True)[0]
+
+        wrapped = p11.export_wrapped_key(master_key,
+                                         rep2_pub,
+                                         _ipap11helper.MECH_RSA_PKCS)
+        assert wrapped
+
+        log.debug("wrapped key MECH_RSA_PKCS (secret master wrapped by pub key): "
+                  "%s", hexlify(wrapped))
+        assert p11.import_wrapped_secret_key(u'test_import_wrapped',
+                                             '555',
+                                             wrapped,
+                                             rep2_priv,
+                                             _ipap11helper.MECH_RSA_PKCS,
+                                             _ipap11helper.KEY_TYPE_AES)
+
+    def test_wrap_unwrap_by_master_key_with_RSA_PKCS_OAEP(self, p11):
+        master_key = p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
+                                   label=u"žžž-aest", id="m")[0]
+        rep2_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                 label=u"replica2", cka_wrap=True)[0]
+        rep2_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY,
+                                  label=u"replica2", cka_unwrap=True)[0]
+
+        wrapped = p11.export_wrapped_key(master_key,
+                                         rep2_pub,
+                                         _ipap11helper.MECH_RSA_PKCS_OAEP)
+        assert wrapped
+
+        log.debug("wrapped key MECH_RSA_PKCS_OAEP (secret master wrapped by "
+                  "pub key): %s", hexlify(wrapped))
+
+        assert p11.import_wrapped_secret_key(u'test_import_wrapped',
+                                             '444',
+                                             wrapped,
+                                             rep2_priv,
+                                             _ipap11helper.MECH_RSA_PKCS_OAEP,
+                                             _ipap11helper.KEY_TYPE_AES)
+
+    def test_set_attribute_on_object(self, p11):
+        rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                 label=u"replica1", cka_wrap=True)[0]
+        test_label = u"newlabelž"
+
+        p11.set_attribute(rep1_pub, _ipap11helper.CKA_LABEL, test_label)
+        assert p11.get_attribute(rep1_pub, _ipap11helper.CKA_LABEL) \
+            == test_label, "The labels do not match."
+
+    def test_do_not_generate_identical_master_keys(self, p11):
+        with pytest.raises(_ipap11helper.DuplicationError):
+            p11.generate_master_key(u"žžž-aest", "m", key_length=16)
+
+        master_key = p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
+                                   label=u"žžž-aest")
+        assert len(master_key) == 1, ("There shouldn't be multiple keys "
+                                      "with the same label.")
+
+    def test_delete_key(self, p11):
+        master_key = p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
+                                   label=u"žžž-aest", id="m")[0]
+        rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                 label=u"newlabelž", cka_wrap=True)[0]
+        rep2_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY,
+                                  label=u"replica2", cka_unwrap=True)[0]
+
+        for key in (rep1_pub, rep2_priv, master_key):
+            p11.delete_key(key)
+
+        master_key = p11.find_keys(_ipap11helper.KEY_CLASS_SECRET_KEY,
+                                   label=u"žžž-aest", id="m")
+        assert len(master_key) == 0, "The master key should be deleted."
+        rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY,
+                                 label=u"newlabelž", cka_wrap=True)
+        assert len(rep1_pub) == 0, ("The public key of replica1 pair should "
+                                    "be deleted.")
+        rep2_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY,
+                                  label=u"replica2", cka_unwrap=True)
+        assert len(rep2_priv) == 0, ("The private key of replica2 pair should"
+                                     " be deleted.")
diff --git a/make-lint b/make-lint
index bd0eb4d75c50c794dbd40444ab035df5a5153d6c..33adf38ff2d020ae64486c6d31c247f520aad302 100755
--- a/make-lint
+++ b/make-lint
@@ -57,7 +57,7 @@ class IPATypeChecker(TypeChecker):
         'urlparse.ResultMixin': ['scheme', 'netloc', 'path', 'query',
             'fragment', 'username', 'password', 'hostname', 'port'],
         'urlparse.ParseResult': ['params'],
-        'pytest': ['fixture', 'raises', 'skip', 'yield_fixture', 'mark'],
+        'pytest': ['fixture', 'raises', 'skip', 'yield_fixture', 'mark', 'fail'],
         'nose.tools': ['assert_equal', 'assert_raises'],
         'datetime.tzinfo': ['houroffset', 'minoffset', 'utcoffset', 'dst'],
 
-- 
1.9.3

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to