[Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Daniel P. Berrange]

The following changes since commit 0f2fa73ba0ca19ebdaccf0d1785583d6601411b6:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-19 11:23:20 +0100)

are available in the git repository at:

  git://github.com/berrange/qemu tags/pull-qcrypto-2016-09-19-1

for you to fetch changes up to 7b1f229a860fa98258a345923dd35df1097838b2:

  crypto: add trace points for TLS cert verification (2016-09-19 12:22:22 +0100)

----------------------------------------------------------------
Merge qcrypto 2016/09/19 v1

----------------------------------------------------------------
Daniel P. Berrange (8):
      crypto: use uint64_t for pbkdf iteration count parameters
      crypto: make PBKDF iterations configurable for LUKS format
      crypto: clear out buffer after timing pbkdf algorithm
      crypto: use correct derived key size when timing pbkdf
      crypto: remove bogus /= 2 for pbkdf iterations
      crypto: increase default pbkdf2 time for luks to 2 seconds
      crypto: support more hash algorithms for pbkdf
      crypto: add trace points for TLS cert verification

 block/crypto.c            |  6 ++++++
 crypto/block-luks.c       | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------
 crypto/pbkdf-gcrypt.c     | 21 +++++++++++++++++++--
 crypto/pbkdf-nettle.c     | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 crypto/pbkdf-stub.c       |  2 +-
 crypto/pbkdf.c            | 35 +++++++++++++++++++----------------
 crypto/tlssession.c       | 10 ++++++++--
 crypto/trace-events       |  1 +
 include/crypto/pbkdf.h    | 16 ++++++++++------
 qapi/crypto.json          |  6 +++++-
 tests/test-crypto-pbkdf.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 11 files changed, 238 insertions(+), 62 deletions(-)

-- 
2.7.4

[Qemu-devel] [PULL v1 1/8] crypto: use uint64_t for pbkdf iteration count parameters

[Daniel P. Berrange]

The qcrypto_pbkdf_count_iters method uses a 64 bit int
but then checks its value against INT32_MAX before
returning it. This bounds check is premature, because
the calling code may well scale the iteration count
by some value. It is thus better to return a 64-bit
integer and let the caller do range checking.

For consistency the qcrypto_pbkdf method is also changed
to accept a 64bit int, though this is somewhat academic
since nettle is limited to taking an 'int' while gcrypt
is limited to taking a 'long int'.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/block-luks.c    | 52 ++++++++++++++++++++++++++++++--------------------
 crypto/pbkdf-gcrypt.c  |  9 ++++++++-
 crypto/pbkdf-nettle.c  |  8 +++++++-
 crypto/pbkdf-stub.c    |  2 +-
 crypto/pbkdf.c         | 16 +++++-----------
 include/crypto/pbkdf.h | 10 +++++-----
 6 files changed, 57 insertions(+), 40 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index aba4455..bc086ac 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -917,6 +917,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
     const char *hash_alg;
     char *cipher_mode_spec = NULL;
     QCryptoCipherAlgorithm ivcipheralg = 0;
+    uint64_t iters;
 
     memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts));
     if (!luks_opts.has_cipher_alg) {
@@ -1064,12 +1065,11 @@ qcrypto_block_luks_create(QCryptoBlock *block,
     /* Determine how many iterations we need to hash the master
      * key, in order to have 1 second of compute time used
      */
-    luks->header.master_key_iterations =
-        qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
-                                   masterkey, luks->header.key_bytes,
-                                   luks->header.master_key_salt,
-                                   QCRYPTO_BLOCK_LUKS_SALT_LEN,
-                                   &local_err);
+    iters = qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
+                                       masterkey, luks->header.key_bytes,
+                                       luks->header.master_key_salt,
+                                       QCRYPTO_BLOCK_LUKS_SALT_LEN,
+                                       &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error;
@@ -1079,11 +1079,15 @@ qcrypto_block_luks_create(QCryptoBlock *block,
      * explanation why they chose /= 8... Probably so that
      * if all 8 keyslots are active we only spend 1 second
      * in total time to check all keys */
-    luks->header.master_key_iterations /= 8;
-    luks->header.master_key_iterations = MAX(
-        luks->header.master_key_iterations,
-        QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS);
-
+    iters /= 8;
+    if (iters > UINT32_MAX) {
+        error_setg_errno(errp, ERANGE,
+                         "PBKDF iterations %llu larger than %u",
+                         (unsigned long long)iters, UINT32_MAX);
+        goto error;
+    }
+    iters = MAX(iters, QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS);
+    luks->header.master_key_iterations = iters;
 
     /* Hash the master key, saving the result in the LUKS
      * header. This hash is used when opening the encrypted
@@ -1131,22 +1135,28 @@ qcrypto_block_luks_create(QCryptoBlock *block,
     /* Again we determine how many iterations are required to
      * hash the user password while consuming 1 second of compute
      * time */
-    luks->header.key_slots[0].iterations =
-        qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
-                                   (uint8_t *)password, strlen(password),
-                                   luks->header.key_slots[0].salt,
-                                   QCRYPTO_BLOCK_LUKS_SALT_LEN,
-                                   &local_err);
+    iters = qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
+                                       (uint8_t *)password, strlen(password),
+                                       luks->header.key_slots[0].salt,
+                                       QCRYPTO_BLOCK_LUKS_SALT_LEN,
+                                       &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error;
     }
     /* Why /= 2 ?  That matches cryptsetup, but there's no
      * explanation why they chose /= 2... */
-    luks->header.key_slots[0].iterations /= 2;
-    luks->header.key_slots[0].iterations = MAX(
-        luks->header.key_slots[0].iterations,
-        QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS);
+    iters /= 2;
+
+    if (iters > UINT32_MAX) {
+        error_setg_errno(errp, ERANGE,
+                         "PBKDF iterations %llu larger than %u",
+                         (unsigned long long)iters, UINT32_MAX);
+        goto error;
+    }
+
+    luks->header.key_slots[0].iterations =
+        MAX(iters, QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS);
 
 
     /* Generate a key that we'll use to encrypt the master
diff --git a/crypto/pbkdf-gcrypt.c b/crypto/pbkdf-gcrypt.c
index 34af3a9..7713031 100644
--- a/crypto/pbkdf-gcrypt.c
+++ b/crypto/pbkdf-gcrypt.c
@@ -38,7 +38,7 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)
 int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
                    const uint8_t *key, size_t nkey,
                    const uint8_t *salt, size_t nsalt,
-                   unsigned int iterations,
+                   uint64_t iterations,
                    uint8_t *out, size_t nout,
                    Error **errp)
 {
@@ -49,6 +49,13 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
     };
     int ret;
 
+    if (iterations > ULONG_MAX) {
+        error_setg_errno(errp, ERANGE,
+                         "PBKDF iterations %llu must be less than %ul",
+                         (long long unsigned)iterations, ULONG_MAX);
+        return -1;
+    }
+
     if (hash >= G_N_ELEMENTS(hash_map) ||
         hash_map[hash] == GCRY_MD_NONE) {
         error_setg(errp, "Unexpected hash algorithm %d", hash);
diff --git a/crypto/pbkdf-nettle.c b/crypto/pbkdf-nettle.c
index d681a60..db81517 100644
--- a/crypto/pbkdf-nettle.c
+++ b/crypto/pbkdf-nettle.c
@@ -38,10 +38,16 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)
 int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
                    const uint8_t *key, size_t nkey,
                    const uint8_t *salt, size_t nsalt,
-                   unsigned int iterations,
+                   uint64_t iterations,
                    uint8_t *out, size_t nout,
                    Error **errp)
 {
+    if (iterations > UINT_MAX) {
+        error_setg_errno(errp, ERANGE,
+                         "PBKDF iterations %llu must be less than %u",
+                         (long long unsigned)iterations, UINT_MAX);
+        return -1;
+    }
     switch (hash) {
     case QCRYPTO_HASH_ALG_SHA1:
         pbkdf2_hmac_sha1(nkey, key,
diff --git a/crypto/pbkdf-stub.c b/crypto/pbkdf-stub.c
index 266a505..a15044d 100644
--- a/crypto/pbkdf-stub.c
+++ b/crypto/pbkdf-stub.c
@@ -32,7 +32,7 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash G_GNUC_UNUSED,
                    size_t nkey G_GNUC_UNUSED,
                    const uint8_t *salt G_GNUC_UNUSED,
                    size_t nsalt G_GNUC_UNUSED,
-                   unsigned int iterations G_GNUC_UNUSED,
+                   uint64_t iterations G_GNUC_UNUSED,
                    uint8_t *out G_GNUC_UNUSED,
                    size_t nout G_GNUC_UNUSED,
                    Error **errp)
diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
index 695cc35..929458b 100644
--- a/crypto/pbkdf.c
+++ b/crypto/pbkdf.c
@@ -62,13 +62,13 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms,
 #endif
 }
 
-int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
-                               const uint8_t *key, size_t nkey,
-                               const uint8_t *salt, size_t nsalt,
-                               Error **errp)
+uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
+                                    const uint8_t *key, size_t nkey,
+                                    const uint8_t *salt, size_t nsalt,
+                                    Error **errp)
 {
     uint8_t out[32];
-    long long int iterations = (1 << 15);
+    uint64_t iterations = (1 << 15);
     unsigned long long delta_ms, start_ms, end_ms;
 
     while (1) {
@@ -100,11 +100,5 @@ int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
 
     iterations = iterations * 1000 / delta_ms;
 
-    if (iterations > INT32_MAX) {
-        error_setg(errp, "Iterations %lld too large for a 32-bit int",
-                   iterations);
-        return -1;
-    }
-
     return iterations;
 }
diff --git a/include/crypto/pbkdf.h b/include/crypto/pbkdf.h
index e9e4cec..6f4ac85 100644
--- a/include/crypto/pbkdf.h
+++ b/include/crypto/pbkdf.h
@@ -122,7 +122,7 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash);
 int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
                    const uint8_t *key, size_t nkey,
                    const uint8_t *salt, size_t nsalt,
-                   unsigned int iterations,
+                   uint64_t iterations,
                    uint8_t *out, size_t nout,
                    Error **errp);
 
@@ -144,9 +144,9 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
  *
  * Returns: number of iterations in 1 second, -1 on error
  */
-int qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
-                               const uint8_t *key, size_t nkey,
-                               const uint8_t *salt, size_t nsalt,
-                               Error **errp);
+uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
+                                    const uint8_t *key, size_t nkey,
+                                    const uint8_t *salt, size_t nsalt,
+                                    Error **errp);
 
 #endif /* QCRYPTO_PBKDF_H */
-- 
2.7.4

[Qemu-devel] [PULL v1 2/8] crypto: make PBKDF iterations configurable for LUKS format

[Daniel P. Berrange]

As protection against bruteforcing passphrases, the PBKDF
algorithm is tuned by counting the number of iterations
needed to produce 1 second of running time. If the machine
that the image will be used on is much faster than the
machine where the image is created, it can be desirable
to raise the number of iterations. This change adds a new
'iter-time' property that allows the user to choose the
iteration wallclock time.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/crypto.c      |  6 ++++++
 crypto/block-luks.c | 24 ++++++++++++++++++++++++
 qapi/crypto.json    |  6 +++++-
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index 7f61e12..7aa7eb5 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -33,6 +33,7 @@
 #define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg"
 #define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg"
 #define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
+#define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
 
 typedef struct BlockCrypto BlockCrypto;
 
@@ -183,6 +184,11 @@ static QemuOptsList block_crypto_create_opts_luks = {
             .type = QEMU_OPT_STRING,
             .help = "Name of encryption hash algorithm",
         },
+        {
+            .name = BLOCK_CRYPTO_OPT_LUKS_ITER_TIME,
+            .type = QEMU_OPT_NUMBER,
+            .help = "Time to spend in PBKDF in milliseconds",
+        },
         { /* end of list */ }
     },
 };
diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index bc086ac..91a4172 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -920,6 +920,9 @@ qcrypto_block_luks_create(QCryptoBlock *block,
     uint64_t iters;
 
     memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts));
+    if (!luks_opts.has_iter_time) {
+        luks_opts.iter_time = 1000;
+    }
     if (!luks_opts.has_cipher_alg) {
         luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256;
     }
@@ -1075,6 +1078,16 @@ qcrypto_block_luks_create(QCryptoBlock *block,
         goto error;
     }
 
+    if (iters > (ULLONG_MAX / luks_opts.iter_time)) {
+        error_setg_errno(errp, ERANGE,
+                         "PBKDF iterations %llu too large to scale",
+                         (unsigned long long)iters);
+        goto error;
+    }
+
+    /* iter_time was in millis, but count_iters reported for secs */
+    iters = iters * luks_opts.iter_time / 1000;
+
     /* Why /= 8 ?  That matches cryptsetup, but there's no
      * explanation why they chose /= 8... Probably so that
      * if all 8 keyslots are active we only spend 1 second
@@ -1144,6 +1157,17 @@ qcrypto_block_luks_create(QCryptoBlock *block,
         error_propagate(errp, local_err);
         goto error;
     }
+
+    if (iters > (ULLONG_MAX / luks_opts.iter_time)) {
+        error_setg_errno(errp, ERANGE,
+                         "PBKDF iterations %llu too large to scale",
+                         (unsigned long long)iters);
+        goto error;
+    }
+
+    /* iter_time was in millis, but count_iters reported for secs */
+    iters = iters * luks_opts.iter_time / 1000;
+
     /* Why /= 2 ?  That matches cryptsetup, but there's no
      * explanation why they chose /= 2... */
     iters /= 2;
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 34d2583..2b6118f 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -185,6 +185,9 @@
 #                  Currently defaults to 'sha256'
 # @hash-alg: #optional the master key hash algorithm
 #            Currently defaults to 'sha256'
+# @iter-time: #optional number of milliseconds to spend in
+#             PBKDF passphrase processing. Currently defaults
+#             to 1000. (since 2.8)
 # Since: 2.6
 ##
 { 'struct': 'QCryptoBlockCreateOptionsLUKS',
@@ -193,7 +196,8 @@
             '*cipher-mode': 'QCryptoCipherMode',
             '*ivgen-alg': 'QCryptoIVGenAlgorithm',
             '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
-            '*hash-alg': 'QCryptoHashAlgorithm'}}
+            '*hash-alg': 'QCryptoHashAlgorithm',
+            '*iter-time': 'int'}}
 
 
 ##
-- 
2.7.4

[Qemu-devel] [PULL v1 3/8] crypto: clear out buffer after timing pbkdf algorithm

[Daniel P. Berrange]

The 'out' buffer will hold a key derived from master
password, so it is best practice to clear this buffer
when no longer required.

At this time, the code isn't worrying about locking
buffers into RAM to prevent swapping sensitive data
to disk.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/pbkdf.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
index 929458b..e391505 100644
--- a/crypto/pbkdf.c
+++ b/crypto/pbkdf.c
@@ -67,13 +67,14 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
                                     const uint8_t *salt, size_t nsalt,
                                     Error **errp)
 {
+    uint64_t ret = -1;
     uint8_t out[32];
     uint64_t iterations = (1 << 15);
     unsigned long long delta_ms, start_ms, end_ms;
 
     while (1) {
         if (qcrypto_pbkdf2_get_thread_cpu(&start_ms, errp) < 0) {
-            return -1;
+            goto cleanup;
         }
         if (qcrypto_pbkdf2(hash,
                            key, nkey,
@@ -81,10 +82,10 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
                            iterations,
                            out, sizeof(out),
                            errp) < 0) {
-            return -1;
+            goto cleanup;
         }
         if (qcrypto_pbkdf2_get_thread_cpu(&end_ms, errp) < 0) {
-            return -1;
+            goto cleanup;
         }
 
         delta_ms = end_ms - start_ms;
@@ -100,5 +101,9 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
 
     iterations = iterations * 1000 / delta_ms;
 
-    return iterations;
+    ret = iterations;
+
+ cleanup:
+    memset(out, 0, sizeof(out));
+    return ret;
 }
-- 
2.7.4

[Qemu-devel] [PULL v1 4/8] crypto: use correct derived key size when timing pbkdf

[Daniel P. Berrange]

Currently when timing the pbkdf algorithm a fixed key
size of 32 bytes is used. This results in inaccurate
timings for certain hashes depending on their digest
size. For example when using sha1 with aes-256, this
causes us to measure time for the master key digest
doing 2 sha1 operations per iteration, instead of 1.

Instead we should pass in the desired key size to the
timing routine that matches the key size that will be
used for real later.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/block-luks.c       |  2 ++
 crypto/pbkdf.c            | 10 +++++++---
 include/crypto/pbkdf.h    |  6 +++++-
 tests/test-crypto-pbkdf.c |  1 +
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 91a4172..9269aaf 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1072,6 +1072,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
                                        masterkey, luks->header.key_bytes,
                                        luks->header.master_key_salt,
                                        QCRYPTO_BLOCK_LUKS_SALT_LEN,
+                                       QCRYPTO_BLOCK_LUKS_DIGEST_LEN,
                                        &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -1152,6 +1153,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
                                        (uint8_t *)password, strlen(password),
                                        luks->header.key_slots[0].salt,
                                        QCRYPTO_BLOCK_LUKS_SALT_LEN,
+                                       luks->header.key_bytes,
                                        &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/crypto/pbkdf.c b/crypto/pbkdf.c
index e391505..f22e71d 100644
--- a/crypto/pbkdf.c
+++ b/crypto/pbkdf.c
@@ -65,13 +65,16 @@ static int qcrypto_pbkdf2_get_thread_cpu(unsigned long long *val_ms,
 uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
                                     const uint8_t *key, size_t nkey,
                                     const uint8_t *salt, size_t nsalt,
+                                    size_t nout,
                                     Error **errp)
 {
     uint64_t ret = -1;
-    uint8_t out[32];
+    uint8_t *out;
     uint64_t iterations = (1 << 15);
     unsigned long long delta_ms, start_ms, end_ms;
 
+    out = g_new(uint8_t, nout);
+
     while (1) {
         if (qcrypto_pbkdf2_get_thread_cpu(&start_ms, errp) < 0) {
             goto cleanup;
@@ -80,7 +83,7 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
                            key, nkey,
                            salt, nsalt,
                            iterations,
-                           out, sizeof(out),
+                           out, nout,
                            errp) < 0) {
             goto cleanup;
         }
@@ -104,6 +107,7 @@ uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
     ret = iterations;
 
  cleanup:
-    memset(out, 0, sizeof(out));
+    memset(out, 0, nout);
+    g_free(out);
     return ret;
 }
diff --git a/include/crypto/pbkdf.h b/include/crypto/pbkdf.h
index 6f4ac85..ef209b3 100644
--- a/include/crypto/pbkdf.h
+++ b/include/crypto/pbkdf.h
@@ -133,6 +133,7 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
  * @nkey: the length of @key in bytes
  * @salt: a random salt
  * @nsalt: length of @salt in bytes
+ * @nout: size of desired derived key
  * @errp: pointer to a NULL-initialized error object
  *
  * Time the PBKDF2 algorithm to determine how many
@@ -140,13 +141,16 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
  * key from a user password provided in @key in 1
  * second of compute time. The result of this can
  * be used as a the @iterations parameter of a later
- * call to qcrypto_pbkdf2().
+ * call to qcrypto_pbkdf2(). The value of @nout should
+ * match that value that will later be provided with
+ * a call to qcrypto_pbkdf2().
  *
  * Returns: number of iterations in 1 second, -1 on error
  */
 uint64_t qcrypto_pbkdf2_count_iters(QCryptoHashAlgorithm hash,
                                     const uint8_t *key, size_t nkey,
                                     const uint8_t *salt, size_t nsalt,
+                                    size_t nout,
                                     Error **errp);
 
 #endif /* QCRYPTO_PBKDF_H */
diff --git a/tests/test-crypto-pbkdf.c b/tests/test-crypto-pbkdf.c
index 8ceceb1..a651dc5 100644
--- a/tests/test-crypto-pbkdf.c
+++ b/tests/test-crypto-pbkdf.c
@@ -358,6 +358,7 @@ static void test_pbkdf_timing(void)
     iters = qcrypto_pbkdf2_count_iters(QCRYPTO_HASH_ALG_SHA256,
                                        key, sizeof(key),
                                        salt, sizeof(salt),
+                                       32,
                                        &error_abort);
 
     g_assert(iters >= (1 << 15));
-- 
2.7.4

[Qemu-devel] [PULL v1 5/8] crypto: remove bogus /= 2 for pbkdf iterations

[Daniel P. Berrange]

When calculating iterations for pbkdf of the key slot
data, we had a /= 2, which was copied from identical
code in cryptsetup. It was always unclear & undocumented
why cryptsetup had this division and it was recently
removed there, too.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/block-luks.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 9269aaf..3ab3250 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -1170,10 +1170,6 @@ qcrypto_block_luks_create(QCryptoBlock *block,
     /* iter_time was in millis, but count_iters reported for secs */
     iters = iters * luks_opts.iter_time / 1000;
 
-    /* Why /= 2 ?  That matches cryptsetup, but there's no
-     * explanation why they chose /= 2... */
-    iters /= 2;
-
     if (iters > UINT32_MAX) {
         error_setg_errno(errp, ERANGE,
                          "PBKDF iterations %llu larger than %u",
-- 
2.7.4

[Qemu-devel] [PULL v1 6/8] crypto: increase default pbkdf2 time for luks to 2 seconds

[Daniel P. Berrange]

cryptsetup recently increased the default pbkdf2 time to 2 seconds
to partially mitigate improvements in hardware performance wrt
brute-forcing the pbkdf algorithm. This updates QEMU defaults to
match.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/block-luks.c | 2 +-
 qapi/crypto.json    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/block-luks.c b/crypto/block-luks.c
index 3ab3250..a848232 100644
--- a/crypto/block-luks.c
+++ b/crypto/block-luks.c
@@ -921,7 +921,7 @@ qcrypto_block_luks_create(QCryptoBlock *block,
 
     memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts));
     if (!luks_opts.has_iter_time) {
-        luks_opts.iter_time = 1000;
+        luks_opts.iter_time = 2000;
     }
     if (!luks_opts.has_cipher_alg) {
         luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256;
diff --git a/qapi/crypto.json b/qapi/crypto.json
index 2b6118f..6933b13 100644
--- a/qapi/crypto.json
+++ b/qapi/crypto.json
@@ -187,7 +187,7 @@
 #            Currently defaults to 'sha256'
 # @iter-time: #optional number of milliseconds to spend in
 #             PBKDF passphrase processing. Currently defaults
-#             to 1000. (since 2.8)
+#             to 2000. (since 2.8)
 # Since: 2.6
 ##
 { 'struct': 'QCryptoBlockCreateOptionsLUKS',
-- 
2.7.4

[Qemu-devel] [PULL v1 7/8] crypto: support more hash algorithms for pbkdf

[Daniel P. Berrange]

Currently pbkdf is only supported with SHA1 and SHA256. Expand
this to support all algorithms known to QEMU.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/pbkdf-gcrypt.c     | 12 ++++++++-
 crypto/pbkdf-nettle.c     | 63 ++++++++++++++++++++++++++++++++++++++++-------
 tests/test-crypto-pbkdf.c | 53 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 117 insertions(+), 11 deletions(-)

diff --git a/crypto/pbkdf-gcrypt.c b/crypto/pbkdf-gcrypt.c
index 7713031..693adc6 100644
--- a/crypto/pbkdf-gcrypt.c
+++ b/crypto/pbkdf-gcrypt.c
@@ -28,7 +28,11 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)
     switch (hash) {
     case QCRYPTO_HASH_ALG_MD5:
     case QCRYPTO_HASH_ALG_SHA1:
+    case QCRYPTO_HASH_ALG_SHA224:
     case QCRYPTO_HASH_ALG_SHA256:
+    case QCRYPTO_HASH_ALG_SHA384:
+    case QCRYPTO_HASH_ALG_SHA512:
+    case QCRYPTO_HASH_ALG_RIPEMD160:
         return true;
     default:
         return false;
@@ -45,7 +49,11 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
     static const int hash_map[QCRYPTO_HASH_ALG__MAX] = {
         [QCRYPTO_HASH_ALG_MD5] = GCRY_MD_MD5,
         [QCRYPTO_HASH_ALG_SHA1] = GCRY_MD_SHA1,
+        [QCRYPTO_HASH_ALG_SHA224] = GCRY_MD_SHA224,
         [QCRYPTO_HASH_ALG_SHA256] = GCRY_MD_SHA256,
+        [QCRYPTO_HASH_ALG_SHA384] = GCRY_MD_SHA384,
+        [QCRYPTO_HASH_ALG_SHA512] = GCRY_MD_SHA512,
+        [QCRYPTO_HASH_ALG_RIPEMD160] = GCRY_MD_RMD160,
     };
     int ret;
 
@@ -58,7 +66,9 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
 
     if (hash >= G_N_ELEMENTS(hash_map) ||
         hash_map[hash] == GCRY_MD_NONE) {
-        error_setg(errp, "Unexpected hash algorithm %d", hash);
+        error_setg_errno(errp, ENOSYS,
+                         "PBKDF does not support hash algorithm %s",
+                         QCryptoHashAlgorithm_lookup[hash]);
         return -1;
     }
 
diff --git a/crypto/pbkdf-nettle.c b/crypto/pbkdf-nettle.c
index db81517..6fb2671 100644
--- a/crypto/pbkdf-nettle.c
+++ b/crypto/pbkdf-nettle.c
@@ -20,6 +20,7 @@
 
 #include "qemu/osdep.h"
 #include <nettle/pbkdf2.h>
+#include <nettle/hmac.h>
 #include "qapi/error.h"
 #include "crypto/pbkdf.h"
 
@@ -28,7 +29,11 @@ bool qcrypto_pbkdf2_supports(QCryptoHashAlgorithm hash)
 {
     switch (hash) {
     case QCRYPTO_HASH_ALG_SHA1:
+    case QCRYPTO_HASH_ALG_SHA224:
     case QCRYPTO_HASH_ALG_SHA256:
+    case QCRYPTO_HASH_ALG_SHA384:
+    case QCRYPTO_HASH_ALG_SHA512:
+    case QCRYPTO_HASH_ALG_RIPEMD160:
         return true;
     default:
         return false;
@@ -42,30 +47,70 @@ int qcrypto_pbkdf2(QCryptoHashAlgorithm hash,
                    uint8_t *out, size_t nout,
                    Error **errp)
 {
+    union {
+        struct hmac_md5_ctx md5;
+        struct hmac_sha1_ctx sha1;
+        struct hmac_sha224_ctx sha224;
+        struct hmac_sha256_ctx sha256;
+        struct hmac_sha384_ctx sha384;
+        struct hmac_sha512_ctx sha512;
+        struct hmac_ripemd160_ctx ripemd160;
+    } ctx;
+
     if (iterations > UINT_MAX) {
         error_setg_errno(errp, ERANGE,
                          "PBKDF iterations %llu must be less than %u",
                          (long long unsigned)iterations, UINT_MAX);
         return -1;
     }
+
     switch (hash) {
+    case QCRYPTO_HASH_ALG_MD5:
+        hmac_md5_set_key(&ctx.md5, nkey, key);
+        PBKDF2(&ctx.md5, hmac_md5_update, hmac_md5_digest,
+               MD5_DIGEST_SIZE, iterations, nsalt, salt, nout, out);
+        break;
+
     case QCRYPTO_HASH_ALG_SHA1:
-        pbkdf2_hmac_sha1(nkey, key,
-                         iterations,
-                         nsalt, salt,
-                         nout, out);
+        hmac_sha1_set_key(&ctx.sha1, nkey, key);
+        PBKDF2(&ctx.sha1, hmac_sha1_update, hmac_sha1_digest,
+               SHA1_DIGEST_SIZE, iterations, nsalt, salt, nout, out);
+        break;
+
+    case QCRYPTO_HASH_ALG_SHA224:
+        hmac_sha224_set_key(&ctx.sha224, nkey, key);
+        PBKDF2(&ctx.sha224, hmac_sha224_update, hmac_sha224_digest,
+               SHA224_DIGEST_SIZE, iterations, nsalt, salt, nout, out);
         break;
 
     case QCRYPTO_HASH_ALG_SHA256:
-        pbkdf2_hmac_sha256(nkey, key,
-                           iterations,
-                           nsalt, salt,
-                           nout, out);
+        hmac_sha256_set_key(&ctx.sha256, nkey, key);
+        PBKDF2(&ctx.sha256, hmac_sha256_update, hmac_sha256_digest,
+               SHA256_DIGEST_SIZE, iterations, nsalt, salt, nout, out);
+        break;
+
+    case QCRYPTO_HASH_ALG_SHA384:
+        hmac_sha384_set_key(&ctx.sha384, nkey, key);
+        PBKDF2(&ctx.sha384, hmac_sha384_update, hmac_sha384_digest,
+               SHA384_DIGEST_SIZE, iterations, nsalt, salt, nout, out);
+        break;
+
+    case QCRYPTO_HASH_ALG_SHA512:
+        hmac_sha512_set_key(&ctx.sha512, nkey, key);
+        PBKDF2(&ctx.sha512, hmac_sha512_update, hmac_sha512_digest,
+               SHA512_DIGEST_SIZE, iterations, nsalt, salt, nout, out);
+        break;
+
+    case QCRYPTO_HASH_ALG_RIPEMD160:
+        hmac_ripemd160_set_key(&ctx.ripemd160, nkey, key);
+        PBKDF2(&ctx.ripemd160, hmac_ripemd160_update, hmac_ripemd160_digest,
+               RIPEMD160_DIGEST_SIZE, iterations, nsalt, salt, nout, out);
         break;
 
     default:
         error_setg_errno(errp, ENOSYS,
-                         "PBKDF does not support hash algorithm %d", hash);
+                         "PBKDF does not support hash algorithm %s",
+                         QCryptoHashAlgorithm_lookup[hash]);
         return -1;
     }
     return 0;
diff --git a/tests/test-crypto-pbkdf.c b/tests/test-crypto-pbkdf.c
index a651dc5..d937aff 100644
--- a/tests/test-crypto-pbkdf.c
+++ b/tests/test-crypto-pbkdf.c
@@ -261,7 +261,6 @@ static QCryptoPbkdfTestData test_data[] = {
                "\xcc\x4a\x5e\x6d\xca\x04\xec\x58",
         .nout = 32
     },
-#if 0
     {
         .path = "/crypto/pbkdf/nonrfc/sha512/iter1200",
         .hash = QCRYPTO_HASH_ALG_SHA512,
@@ -280,6 +279,58 @@ static QCryptoPbkdfTestData test_data[] = {
         .nout = 32
     },
     {
+        .path = "/crypto/pbkdf/nonrfc/sha224/iter1200",
+        .hash = QCRYPTO_HASH_ALG_SHA224,
+        .iterations = 1200,
+        .key = "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
+        .nkey = 129,
+        .salt = "pass phrase exceeds block size",
+        .nsalt = 30,
+        .out = "\x13\x3b\x88\x0c\x0e\x52\xa2\x41"
+               "\x49\x33\x35\xa6\xc3\x83\xae\x23"
+               "\xf6\x77\x43\x9e\x5b\x30\x92\x3e"
+               "\x4a\x3a\xaa\x24\x69\x3c\xed\x20",
+        .nout = 32
+    },
+    {
+        .path = "/crypto/pbkdf/nonrfc/sha384/iter1200",
+        .hash = QCRYPTO_HASH_ALG_SHA384,
+        .iterations = 1200,
+        .key = "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
+        .nkey = 129,
+        .salt = "pass phrase exceeds block size",
+        .nsalt = 30,
+        .out = "\xfe\xe3\xe1\x84\xc9\x25\x3e\x10"
+               "\x47\xc8\x7d\x53\xc6\xa5\xe3\x77"
+               "\x29\x41\x76\xbd\x4b\xe3\x9b\xac"
+               "\x05\x6c\x11\xdd\x17\xc5\x93\x80",
+        .nout = 32
+    },
+    {
+        .path = "/crypto/pbkdf/nonrfc/ripemd160/iter1200",
+        .hash = QCRYPTO_HASH_ALG_RIPEMD160,
+        .iterations = 1200,
+        .key = "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX"
+               "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
+        .nkey = 129,
+        .salt = "pass phrase exceeds block size",
+        .nsalt = 30,
+        .out = "\xd6\xcb\xd8\xa7\xdb\x0c\xa2\x2a"
+               "\x23\x5e\x47\xaf\xdb\xda\xa8\xef"
+               "\xe4\x01\x0d\x6f\xb5\x33\xc8\xbd"
+               "\xce\xbf\x91\x14\x8b\x5c\x48\x41",
+        .nout = 32
+    },
+#if 0
+    {
         .path = "/crypto/pbkdf/nonrfc/whirlpool/iter1200",
         .hash = QCRYPTO_HASH_ALG_WHIRLPOOL,
         .iterations = 1200,
-- 
2.7.4

[Qemu-devel] [PULL v1 8/8] crypto: add trace points for TLS cert verification

[Daniel P. Berrange]

It is very useful to know about TLS cert verification
status when debugging, so add a trace point for it.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/tlssession.c | 10 ++++++++--
 crypto/trace-events |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 2de42c6..96a02de 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -351,16 +351,22 @@ qcrypto_tls_session_check_credentials(QCryptoTLSSession *session,
 {
     if (object_dynamic_cast(OBJECT(session->creds),
                             TYPE_QCRYPTO_TLS_CREDS_ANON)) {
+        trace_qcrypto_tls_session_check_creds(session, "nop");
         return 0;
     } else if (object_dynamic_cast(OBJECT(session->creds),
                             TYPE_QCRYPTO_TLS_CREDS_X509)) {
         if (session->creds->verifyPeer) {
-            return qcrypto_tls_session_check_certificate(session,
-                                                         errp);
+            int ret = qcrypto_tls_session_check_certificate(session,
+                                                            errp);
+            trace_qcrypto_tls_session_check_creds(session,
+                                                  ret == 0 ? "pass" : "fail");
+            return ret;
         } else {
+            trace_qcrypto_tls_session_check_creds(session, "skip");
             return 0;
         }
     } else {
+        trace_qcrypto_tls_session_check_creds(session, "error");
         error_setg(errp, "Unexpected credential type %s",
                    object_get_typename(OBJECT(session->creds)));
         return -1;
diff --git a/crypto/trace-events b/crypto/trace-events
index 8181843..dc6ddd3 100644
--- a/crypto/trace-events
+++ b/crypto/trace-events
@@ -17,3 +17,4 @@ qcrypto_tls_creds_x509_load_cert_list(void *creds, const char *file) "TLS creds
 
 # crypto/tlssession.c
 qcrypto_tls_session_new(void *session, void *creds, const char *hostname, const char *aclname, int endpoint) "TLS session new session=%p creds=%p hostname=%s aclname=%s endpoint=%d"
+qcrypto_tls_session_check_creds(void *session, const char *status) "TLS session check creds session=%p status=%s"
-- 
2.7.4

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19
Message-id: 1474285452-6166-1-git-send-email-berrange@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
bdd8574 crypto: add trace points for TLS cert verification
a5218f2 crypto: support more hash algorithms for pbkdf
4483f4e crypto: increase default pbkdf2 time for luks to 2 seconds
c75443a crypto: remove bogus /= 2 for pbkdf iterations
61e0f1a crypto: use correct derived key size when timing pbkdf
616fc7b crypto: clear out buffer after timing pbkdf algorithm
d4a7ad1 crypto: make PBKDF iterations configurable for LUKS format
f56a5e3 crypto: use uint64_t for pbkdf iteration count parameters

=== OUTPUT BEGIN ===
Checking PATCH 1/8: crypto: use uint64_t for pbkdf iteration count parameters...
Checking PATCH 2/8: crypto: make PBKDF iterations configurable for LUKS format...
Checking PATCH 3/8: crypto: clear out buffer after timing pbkdf algorithm...
Checking PATCH 4/8: crypto: use correct derived key size when timing pbkdf...
Checking PATCH 5/8: crypto: remove bogus /= 2 for pbkdf iterations...
Checking PATCH 6/8: crypto: increase default pbkdf2 time for luks to 2 seconds...
Checking PATCH 7/8: crypto: support more hash algorithms for pbkdf...
ERROR: if this code is redundant consider removing it
#222: FILE: tests/test-crypto-pbkdf.c:332:
+#if 0

total: 1 errors, 0 warnings, 194 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 8/8: crypto: add trace points for TLS cert verification...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Daniel P. Berrange]

On Mon, Sep 19, 2016 at 05:33:57AM -0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
> Hi,
> 
> Your series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Subject: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19
> Message-id: 1474285452-6166-1-git-send-email-berrange@redhat.com
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> Switched to a new branch 'test'
> bdd8574 crypto: add trace points for TLS cert verification
> a5218f2 crypto: support more hash algorithms for pbkdf
> 4483f4e crypto: increase default pbkdf2 time for luks to 2 seconds
> c75443a crypto: remove bogus /= 2 for pbkdf iterations
> 61e0f1a crypto: use correct derived key size when timing pbkdf
> 616fc7b crypto: clear out buffer after timing pbkdf algorithm
> d4a7ad1 crypto: make PBKDF iterations configurable for LUKS format
> f56a5e3 crypto: use uint64_t for pbkdf iteration count parameters
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/8: crypto: use uint64_t for pbkdf iteration count parameters...
> Checking PATCH 2/8: crypto: make PBKDF iterations configurable for LUKS format...
> Checking PATCH 3/8: crypto: clear out buffer after timing pbkdf algorithm...
> Checking PATCH 4/8: crypto: use correct derived key size when timing pbkdf...
> Checking PATCH 5/8: crypto: remove bogus /= 2 for pbkdf iterations...
> Checking PATCH 6/8: crypto: increase default pbkdf2 time for luks to 2 seconds...
> Checking PATCH 7/8: crypto: support more hash algorithms for pbkdf...
> ERROR: if this code is redundant consider removing it
> #222: FILE: tests/test-crypto-pbkdf.c:332:
> +#if 0
> 
> total: 1 errors, 0 warnings, 194 lines checked

Peter, FWIW, I know about this style check error and I'm intentionally
ignoring it as I consider it a false positive. IMHO we should probably
downgrade that style check to a WARNING, not an ERROR. The message itself
is indicating that maintainers have discretion to ignore it and thus it
shouldn't be an error.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Peter Maydell]

On 19 September 2016 at 13:47, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Mon, Sep 19, 2016 at 05:33:57AM -0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
>> Hi,
>>
>> Your series seems to have some coding style problems. See output below for
>> more information:
>>
>> Type: series
>> Subject: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19
>> Message-id: 1474285452-6166-1-git-send-email-berrange@redhat.com
>>
>> === TEST SCRIPT BEGIN ===
>> #!/bin/bash
>>
>> BASE=base
>> n=1
>> total=$(git log --oneline $BASE.. | wc -l)
>> failed=0
>>
>> # Useful git options
>> git config --local diff.renamelimit 0
>> git config --local diff.renames True
>>
>> commits="$(git log --format=%H --reverse $BASE..)"
>> for c in $commits; do
>>     echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
>>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>>         failed=1
>>         echo
>>     fi
>>     n=$((n+1))
>> done
>>
>> exit $failed
>> === TEST SCRIPT END ===
>>
>> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>> Switched to a new branch 'test'
>> bdd8574 crypto: add trace points for TLS cert verification
>> a5218f2 crypto: support more hash algorithms for pbkdf
>> 4483f4e crypto: increase default pbkdf2 time for luks to 2 seconds
>> c75443a crypto: remove bogus /= 2 for pbkdf iterations
>> 61e0f1a crypto: use correct derived key size when timing pbkdf
>> 616fc7b crypto: clear out buffer after timing pbkdf algorithm
>> d4a7ad1 crypto: make PBKDF iterations configurable for LUKS format
>> f56a5e3 crypto: use uint64_t for pbkdf iteration count parameters
>>
>> === OUTPUT BEGIN ===
>> Checking PATCH 1/8: crypto: use uint64_t for pbkdf iteration count parameters...
>> Checking PATCH 2/8: crypto: make PBKDF iterations configurable for LUKS format...
>> Checking PATCH 3/8: crypto: clear out buffer after timing pbkdf algorithm...
>> Checking PATCH 4/8: crypto: use correct derived key size when timing pbkdf...
>> Checking PATCH 5/8: crypto: remove bogus /= 2 for pbkdf iterations...
>> Checking PATCH 6/8: crypto: increase default pbkdf2 time for luks to 2 seconds...
>> Checking PATCH 7/8: crypto: support more hash algorithms for pbkdf...
>> ERROR: if this code is redundant consider removing it
>> #222: FILE: tests/test-crypto-pbkdf.c:332:
>> +#if 0
>>
>> total: 1 errors, 0 warnings, 194 lines checked
>
> Peter, FWIW, I know about this style check error and I'm intentionally
> ignoring it as I consider it a false positive. IMHO we should probably
> downgrade that style check to a WARNING, not an ERROR. The message itself
> is indicating that maintainers have discretion to ignore it and thus it
> shouldn't be an error.

Well, I don't in general look at patchew complaints on pull
requests (we should probably make it stop sending them) on
the basis that the maintainer should have already done that
before accepting the patches. But in general I think that
"#if 0" should be an error because there's not really any
good reason for it. For instance in this case there's no
explanation anywhere in the file of why these particular
test cases are disabled or in what circumstances they might
ever in future be enabled. If there's a case for the code being
possibly enabled at compile time locally or in the future then
#if SOMETHING (like the #ifdef DEBUG checks) with some comment
explaining the situation; if there isn't then the code doesn't
need to be there at all.

(This doesn't mean I'm rejecting this pull request.)

thanks
-- PMM

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Daniel P. Berrange]

On Mon, Sep 19, 2016 at 02:01:59PM +0100, Peter Maydell wrote:
> On 19 September 2016 at 13:47, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On Mon, Sep 19, 2016 at 05:33:57AM -0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
> >> Hi,
> >>
> >> Your series seems to have some coding style problems. See output below for
> >> more information:
> >>
> >> Type: series
> >> Subject: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19
> >> Message-id: 1474285452-6166-1-git-send-email-berrange@redhat.com
> >>
> >> === TEST SCRIPT BEGIN ===
> >> #!/bin/bash
> >>
> >> BASE=base
> >> n=1
> >> total=$(git log --oneline $BASE.. | wc -l)
> >> failed=0
> >>
> >> # Useful git options
> >> git config --local diff.renamelimit 0
> >> git config --local diff.renames True
> >>
> >> commits="$(git log --format=%H --reverse $BASE..)"
> >> for c in $commits; do
> >>     echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
> >>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
> >>         failed=1
> >>         echo
> >>     fi
> >>     n=$((n+1))
> >> done
> >>
> >> exit $failed
> >> === TEST SCRIPT END ===
> >>
> >> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> >> Switched to a new branch 'test'
> >> bdd8574 crypto: add trace points for TLS cert verification
> >> a5218f2 crypto: support more hash algorithms for pbkdf
> >> 4483f4e crypto: increase default pbkdf2 time for luks to 2 seconds
> >> c75443a crypto: remove bogus /= 2 for pbkdf iterations
> >> 61e0f1a crypto: use correct derived key size when timing pbkdf
> >> 616fc7b crypto: clear out buffer after timing pbkdf algorithm
> >> d4a7ad1 crypto: make PBKDF iterations configurable for LUKS format
> >> f56a5e3 crypto: use uint64_t for pbkdf iteration count parameters
> >>
> >> === OUTPUT BEGIN ===
> >> Checking PATCH 1/8: crypto: use uint64_t for pbkdf iteration count parameters...
> >> Checking PATCH 2/8: crypto: make PBKDF iterations configurable for LUKS format...
> >> Checking PATCH 3/8: crypto: clear out buffer after timing pbkdf algorithm...
> >> Checking PATCH 4/8: crypto: use correct derived key size when timing pbkdf...
> >> Checking PATCH 5/8: crypto: remove bogus /= 2 for pbkdf iterations...
> >> Checking PATCH 6/8: crypto: increase default pbkdf2 time for luks to 2 seconds...
> >> Checking PATCH 7/8: crypto: support more hash algorithms for pbkdf...
> >> ERROR: if this code is redundant consider removing it
> >> #222: FILE: tests/test-crypto-pbkdf.c:332:
> >> +#if 0
> >>
> >> total: 1 errors, 0 warnings, 194 lines checked
> >
> > Peter, FWIW, I know about this style check error and I'm intentionally
> > ignoring it as I consider it a false positive. IMHO we should probably
> > downgrade that style check to a WARNING, not an ERROR. The message itself
> > is indicating that maintainers have discretion to ignore it and thus it
> > shouldn't be an error.
> 
> Well, I don't in general look at patchew complaints on pull
> requests (we should probably make it stop sending them) on
> the basis that the maintainer should have already done that
> before accepting the patches. But in general I think that
> "#if 0" should be an error because there's not really any
> good reason for it. For instance in this case there's no
> explanation anywhere in the file of why these particular
> test cases are disabled or in what circumstances they might
> ever in future be enabled. If there's a case for the code being
> possibly enabled at compile time locally or in the future then
> #if SOMETHING (like the #ifdef DEBUG checks) with some comment
> explaining the situation; if there isn't then the code doesn't
> need to be there at all.

The data in the test file is a conversion of test data from
cryptsetup. Some are disabled since we don't support the
particular hash algorithms yet, but I've been enabling more,
as in this patch series. IMHO the '#if 0' is appropriate as
this is a marker for future todo items, and if I had deleted
the code as suggested, then whoever adds the extra algorithms
in the future will have to go and find the original test data,
and do a data conversion of it again which is just a waste of
their time.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Fam Zheng]

On Mon, 09/19 14:01, Peter Maydell wrote:
> On 19 September 2016 at 13:47, Daniel P. Berrange <berrange@redhat.com> wrote:
> > On Mon, Sep 19, 2016 at 05:33:57AM -0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com wrote:
> >> Hi,
> >>
> >> Your series seems to have some coding style problems. See output below for
> >> more information:
> >>
> >> Type: series
> >> Subject: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19
> >> Message-id: 1474285452-6166-1-git-send-email-berrange@redhat.com
> >>
> >> === TEST SCRIPT BEGIN ===
> >> #!/bin/bash
> >>
> >> BASE=base
> >> n=1
> >> total=$(git log --oneline $BASE.. | wc -l)
> >> failed=0
> >>
> >> # Useful git options
> >> git config --local diff.renamelimit 0
> >> git config --local diff.renames True
> >>
> >> commits="$(git log --format=%H --reverse $BASE..)"
> >> for c in $commits; do
> >>     echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
> >>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
> >>         failed=1
> >>         echo
> >>     fi
> >>     n=$((n+1))
> >> done
> >>
> >> exit $failed
> >> === TEST SCRIPT END ===
> >>
> >> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> >> Switched to a new branch 'test'
> >> bdd8574 crypto: add trace points for TLS cert verification
> >> a5218f2 crypto: support more hash algorithms for pbkdf
> >> 4483f4e crypto: increase default pbkdf2 time for luks to 2 seconds
> >> c75443a crypto: remove bogus /= 2 for pbkdf iterations
> >> 61e0f1a crypto: use correct derived key size when timing pbkdf
> >> 616fc7b crypto: clear out buffer after timing pbkdf algorithm
> >> d4a7ad1 crypto: make PBKDF iterations configurable for LUKS format
> >> f56a5e3 crypto: use uint64_t for pbkdf iteration count parameters
> >>
> >> === OUTPUT BEGIN ===
> >> Checking PATCH 1/8: crypto: use uint64_t for pbkdf iteration count parameters...
> >> Checking PATCH 2/8: crypto: make PBKDF iterations configurable for LUKS format...
> >> Checking PATCH 3/8: crypto: clear out buffer after timing pbkdf algorithm...
> >> Checking PATCH 4/8: crypto: use correct derived key size when timing pbkdf...
> >> Checking PATCH 5/8: crypto: remove bogus /= 2 for pbkdf iterations...
> >> Checking PATCH 6/8: crypto: increase default pbkdf2 time for luks to 2 seconds...
> >> Checking PATCH 7/8: crypto: support more hash algorithms for pbkdf...
> >> ERROR: if this code is redundant consider removing it
> >> #222: FILE: tests/test-crypto-pbkdf.c:332:
> >> +#if 0
> >>
> >> total: 1 errors, 0 warnings, 194 lines checked
> >
> > Peter, FWIW, I know about this style check error and I'm intentionally
> > ignoring it as I consider it a false positive. IMHO we should probably
> > downgrade that style check to a WARNING, not an ERROR. The message itself
> > is indicating that maintainers have discretion to ignore it and thus it
> > shouldn't be an error.
> 
> Well, I don't in general look at patchew complaints on pull
> requests (we should probably make it stop sending them) on
> the basis that the maintainer should have already done that
> before accepting the patches.

Please keep in mind if a series was based on a maintainer tree or another
pending series, it would fail to apply it thus skip the testing and stay quiet;
pull requests on the contrary can be applied to master reliably. Therefore, I'm
not sure about stoping that.

Fam

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Peter Maydell]

On 19 September 2016 at 14:10, Daniel P. Berrange <berrange@redhat.com> wrote:
> On Mon, Sep 19, 2016 at 02:01:59PM +0100, Peter Maydell wrote:
>> But in general I think that
>> "#if 0" should be an error because there's not really any
>> good reason for it. For instance in this case there's no
>> explanation anywhere in the file of why these particular
>> test cases are disabled or in what circumstances they might
>> ever in future be enabled. If there's a case for the code being
>> possibly enabled at compile time locally or in the future then
>> #if SOMETHING (like the #ifdef DEBUG checks) with some comment
>> explaining the situation; if there isn't then the code doesn't
>> need to be there at all.
>
> The data in the test file is a conversion of test data from
> cryptsetup. Some are disabled since we don't support the
> particular hash algorithms yet, but I've been enabling more,
> as in this patch series. IMHO the '#if 0' is appropriate as
> this is a marker for future todo items, and if I had deleted
> the code as suggested, then whoever adds the extra algorithms
> in the future will have to go and find the original test data,
> and do a data conversion of it again which is just a waste of
> their time.

That sounds like it should fall under "#if SOMETHING plus a
comment about why it's there", then.

thanks
-- PMM

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Peter Maydell]

On 19 September 2016 at 12:44, Daniel P. Berrange <berrange@redhat.com> wrote:
> The following changes since commit 0f2fa73ba0ca19ebdaccf0d1785583d6601411b6:
>
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-19 11:23:20 +0100)
>
> are available in the git repository at:
>
>   git://github.com/berrange/qemu tags/pull-qcrypto-2016-09-19-1
>
> for you to fetch changes up to 7b1f229a860fa98258a345923dd35df1097838b2:
>
>   crypto: add trace points for TLS cert verification (2016-09-19 12:22:22 +0100)
>
> ----------------------------------------------------------------
> Merge qcrypto 2016/09/19 v1
>
> ----------------------------------------------------------------
> Daniel P. Berrange (8):
>       crypto: use uint64_t for pbkdf iteration count parameters
>       crypto: make PBKDF iterations configurable for LUKS format
>       crypto: clear out buffer after timing pbkdf algorithm
>       crypto: use correct derived key size when timing pbkdf
>       crypto: remove bogus /= 2 for pbkdf iterations
>       crypto: increase default pbkdf2 time for luks to 2 seconds
>       crypto: support more hash algorithms for pbkdf
>       crypto: add trace points for TLS cert verification

Fails to build with format string error, I'm afraid:

/home/petmay01/qemu/crypto/pbkdf-gcrypt.c: In function 'qcrypto_pbkdf2':
/home/petmay01/qemu/crypto/pbkdf-gcrypt.c:61:9: error: format '%u'
expects argument of type 'unsigned int', but argument 8 has type 'long
unsigned int' [-Werror=format=]
         error_setg_errno(errp, ERANGE,
         ^

(32-bit and 64-bit ARM.)

thanks
-- PMM

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Daniel P. Berrange]

On Mon, Sep 19, 2016 at 03:36:38PM +0100, Peter Maydell wrote:
> On 19 September 2016 at 12:44, Daniel P. Berrange <berrange@redhat.com> wrote:
> > The following changes since commit 0f2fa73ba0ca19ebdaccf0d1785583d6601411b6:
> >
> >   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-19 11:23:20 +0100)
> >
> > are available in the git repository at:
> >
> >   git://github.com/berrange/qemu tags/pull-qcrypto-2016-09-19-1
> >
> > for you to fetch changes up to 7b1f229a860fa98258a345923dd35df1097838b2:
> >
> >   crypto: add trace points for TLS cert verification (2016-09-19 12:22:22 +0100)
> >
> > ----------------------------------------------------------------
> > Merge qcrypto 2016/09/19 v1
> >
> > ----------------------------------------------------------------
> > Daniel P. Berrange (8):
> >       crypto: use uint64_t for pbkdf iteration count parameters
> >       crypto: make PBKDF iterations configurable for LUKS format
> >       crypto: clear out buffer after timing pbkdf algorithm
> >       crypto: use correct derived key size when timing pbkdf
> >       crypto: remove bogus /= 2 for pbkdf iterations
> >       crypto: increase default pbkdf2 time for luks to 2 seconds
> >       crypto: support more hash algorithms for pbkdf
> >       crypto: add trace points for TLS cert verification
> 
> Fails to build with format string error, I'm afraid:
> 
> /home/petmay01/qemu/crypto/pbkdf-gcrypt.c: In function 'qcrypto_pbkdf2':
> /home/petmay01/qemu/crypto/pbkdf-gcrypt.c:61:9: error: format '%u'
> expects argument of type 'unsigned int', but argument 8 has type 'long
> unsigned int' [-Werror=format=]
>          error_setg_errno(errp, ERANGE,
>          ^
> 
> (32-bit and 64-bit ARM.)

Opps, will send a v2 shortly.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PULL v1 0/8] Merge qcrypto 2016/09/19

[Daniel P. Berrange]

On Mon, Sep 19, 2016 at 03:50:00PM +0100, Daniel P. Berrange wrote:
> On Mon, Sep 19, 2016 at 03:36:38PM +0100, Peter Maydell wrote:
> > On 19 September 2016 at 12:44, Daniel P. Berrange <berrange@redhat.com> wrote:
> > > The following changes since commit 0f2fa73ba0ca19ebdaccf0d1785583d6601411b6:
> > >
> > >   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-19 11:23:20 +0100)
> > >
> > > are available in the git repository at:
> > >
> > >   git://github.com/berrange/qemu tags/pull-qcrypto-2016-09-19-1
> > >
> > > for you to fetch changes up to 7b1f229a860fa98258a345923dd35df1097838b2:
> > >
> > >   crypto: add trace points for TLS cert verification (2016-09-19 12:22:22 +0100)
> > >
> > > ----------------------------------------------------------------
> > > Merge qcrypto 2016/09/19 v1
> > >
> > > ----------------------------------------------------------------
> > > Daniel P. Berrange (8):
> > >       crypto: use uint64_t for pbkdf iteration count parameters
> > >       crypto: make PBKDF iterations configurable for LUKS format
> > >       crypto: clear out buffer after timing pbkdf algorithm
> > >       crypto: use correct derived key size when timing pbkdf
> > >       crypto: remove bogus /= 2 for pbkdf iterations
> > >       crypto: increase default pbkdf2 time for luks to 2 seconds
> > >       crypto: support more hash algorithms for pbkdf
> > >       crypto: add trace points for TLS cert verification
> > 
> > Fails to build with format string error, I'm afraid:
> > 
> > /home/petmay01/qemu/crypto/pbkdf-gcrypt.c: In function 'qcrypto_pbkdf2':
> > /home/petmay01/qemu/crypto/pbkdf-gcrypt.c:61:9: error: format '%u'
> > expects argument of type 'unsigned int', but argument 8 has type 'long
> > unsigned int' [-Werror=format=]
> >          error_setg_errno(errp, ERANGE,
> >          ^
> > 
> > (32-bit and 64-bit ARM.)
> 
> Opps, will send a v2 shortly.

Turned out to be a trivial typo  - '%ul' insead of '%lu' in the format
string, so fixed in the v2.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|