Re: [systemd-devel] [systemd-commits] src/cryptsetup
Hi, On 01/12/2014 01:12, Lennart Poettering wrote : On Mon, 24.11.14 19:25, Quentin Lefebvre (qlefebvre_...@yahoo.com) wrote: Le 24/11/2014 19:17, Zbigniew Jędrzejewski-Szmek a écrit : On Mon, Nov 24, 2014 at 07:03:27PM +0100, Quentin Lefebvre wrote: Le 24/11/2014 19:01, Zbigniew Jędrzejewski-Szmek a écrit : On Mon, Nov 24, 2014 at 06:44:25PM +0100, Quentin Lefebvre wrote: Hi, I tested your patch and actually it doesn't solve the bug. For example, if hash=sha512 is provided in /etc/crypttab, the firstif (!streq(arg_hash, plain)) is true, and the +} else if (!key_file) is not reached. This is be design. My patch is quite different from your patch, which I tried to make clear in the description. If you specify hash=sha512, then you get hash=sha512. Yes, and this is the problem. cryptsetup ignores the hash, so that we should obtain hash=NULL for it to work. Systemd is not going to work around a bug in a different package. Specifying a hash in the configuration if you don't want a hash is an error, please just fix it there. I understand your point. Still you have a cryptsetup tool in systemd, so I would expect it behaves as the true cryptsetup program. The problem here is compatibility, you do something with cryptsetup and then your system fails to boot because of a different behaviour of systemd. Note that compatibiltiy is really a problem with crypttab anyway, as there were multiple implementations of the code that reads it around: at least the one from DEbian and the one from Fedora, both of which had very different feature sets and even different syntaxes. With systemd's crypttab support we try to provide a decent amount of compatibility with both, to the level that it makes sense. Since we cannot reach 100% compat anyway, this explicitly means no bug-for-bug compatibility however. Hence I really think we should do the correct thing, rather than the traditional thing here, given that this is not the most common usecase anyway, Hope that makes sense, OK. I also read Zbigniew's answer on the bug report. I understand your point, which makes sense. I guess we'll have to document these different behaviors in Debian, so that users don't get too confused... But anyway, plain dm-crypt devices, even if they're not the most common usecase, should be handled in the long-term, as it is still a useful, and quite different setup compared to LUKS devices. Thanks for your replies and great work! Best regards, Quentin ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [systemd-commits] src/cryptsetup
Hi, On 24/11/2014 19:17, Zbigniew Jędrzejewski-Szmek wrote : On Mon, Nov 24, 2014 at 07:03:27PM +0100, Quentin Lefebvre wrote: On 24/11/2014 19:01, Zbigniew Jędrzejewski-Szmek wrote : On Mon, Nov 24, 2014 at 06:44:25PM +0100, Quentin Lefebvre wrote: Hi, I tested your patch and actually it doesn't solve the bug. For example, if hash=sha512 is provided in /etc/crypttab, the firstif (!streq(arg_hash, plain)) is true, and the +} else if (!key_file) is not reached. This is be design. My patch is quite different from your patch, which I tried to make clear in the description. If you specify hash=sha512, then you get hash=sha512. Yes, and this is the problem. cryptsetup ignores the hash, so that we should obtain hash=NULL for it to work. Systemd is not going to work around a bug in a different package. Specifying a hash in the configuration if you don't want a hash is an error, please just fix it there. As I mention it in the bugreport (https://bugs.freedesktop.org/show_bug.cgi?id=52630), this is not exactly a cryptsetup bug, but rather the intended and documented way it works. Please see the NOTES ON PASSPHRASE PROCESSING FOR PLAIN MODE section, where it is clearly stated that hash processing is only used on *passphrases*. So, I'm afraid commit http://cgit.freedesktop.org/systemd/systemd/commit/?id=8a52210c93 doesn't make the job it should. Actually it doesn't solve a bug that definitely seems related to systemd, and it kind of breaks the previous logic of the code. To be clear, when a hash algorithm is provided along with a key file for plain mode encryption, systemd-cryptsetup should, IMHO, ignore the hash algorithm as cryptsetup does. Please don't get angry at me for insisting like this. I don't want to declare a futile war against anybody. I'm just a systemd user who wants the best from the software he uses. And I'm sure you're doing your best here. Best regards, Quentin ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] This patch solves the bug 52630 described here: https://bugs.freedesktop.org/show_bug.cgi?id=52630 .
On 24/11/2014 15:21, Zbigniew Jędrzejewski-Szmek wrote : On Wed, Nov 19, 2014 at 02:22:02PM +0100, Quentin Lefebvre wrote: For plain dm-crypt devices, the behavior of cryptsetup package is to ignore the hash algorithm when a key file is provided. With this patch, systemd-cryptsetup now behaves as cryptsetup, so that old plain dm-crypt devices created with cryptsetup can be mounted at boot time by systemd, with no modification of /etc/crypttab. As I said on the bug tracker, it seems wrong to ignore the hash if it is explicitly configured by the user. Yes, I agree on that. I pushed a change to default to no hash if the keyfile is specified instead. Thanks, I think it was the default choice for compatibility. Best, Quentin ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [systemd-commits] src/cryptsetup
Hi,
I tested your patch and actually it doesn't solve the bug.
For example, if hash=sha512 is provided in /etc/crypttab, the first
if (!streq(arg_hash, plain))
is true, and the
+} else if (!key_file)
is not reached.
So I suggest rewriting the patch, or applying my original patch, that is
maybe less elegant, but has both advantages to work and be easily readable.
Best regards,
Quentin
On 24/11/2014 15:14, Zbigniew Jędrzejewski-Szmek wrote :
src/cryptsetup/cryptsetup.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
New commits:
commit 8a52210c9392887a31fdb2845f65b4c5869e8e66
Author: Zbigniew Jędrzejewski-Szmek zbys...@in.waw.pl
Date: Mon Nov 24 09:11:12 2014 -0500
cryptsetup: default to no hash when keyfile is specified
For plain dm-crypt devices, the behavior of cryptsetup package is to
ignore the hash algorithm when a key file is provided. It seems wrong
to ignore a hash when it is explicitly specified, but we should default
to no hash if the keyfile is specified.
https://bugs.freedesktop.org/show_bug.cgi?id=52630
diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index 94570eb..b9e67fa 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -400,7 +400,9 @@ static int attach_luks_or_plain(struct crypt_device *cd,
/* plain isn't a real hash type. it just means use no
hash */
if (!streq(arg_hash, plain))
params.hash = arg_hash;
-} else
+} else if (!key_file)
+/* for CRYPT_PLAIN, the behaviour of cryptsetup
+ * package is to not hash when a key file is provided
*/
params.hash = ripemd160;
if (arg_cipher) {
___
systemd-commits mailing list
systemd-comm...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-commits
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] This patch solves the bug 52630 described here: https://bugs.freedesktop.org/show_bug.cgi?id=52630 .
Hi, I reopened the bug after testing the patch. See: https://bugs.freedesktop.org/show_bug.cgi?id=52630 http://lists.freedesktop.org/archives/systemd-devel/2014-November/025490.html (sorry for the poor formatting of the last email). Best, Quentin Le 24/11/2014 15:21, Zbigniew Jędrzejewski-Szmek a écrit : On Wed, Nov 19, 2014 at 02:22:02PM +0100, Quentin Lefebvre wrote: For plain dm-crypt devices, the behavior of cryptsetup package is to ignore the hash algorithm when a key file is provided. With this patch, systemd-cryptsetup now behaves as cryptsetup, so that old plain dm-crypt devices created with cryptsetup can be mounted at boot time by systemd, with no modification of /etc/crypttab. [replying here too for the sake of ml archives] As I said on the bug tracker, it seems wrong to ignore the hash if it is explicitly configured by the user. I pushed a change to default to no hash if the keyfile is specified instead. Zbyszek ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [systemd-commits] src/cryptsetup
Le 24/11/2014 19:01, Zbigniew Jędrzejewski-Szmek a écrit : On Mon, Nov 24, 2014 at 06:44:25PM +0100, Quentin Lefebvre wrote: Hi, I tested your patch and actually it doesn't solve the bug. For example, if hash=sha512 is provided in /etc/crypttab, the firstif (!streq(arg_hash, plain)) is true, and the +} else if (!key_file) is not reached. This is be design. My patch is quite different from your patch, which I tried to make clear in the description. If you specify hash=sha512, then you get hash=sha512. Yes, and this is the problem. cryptsetup ignores the hash, so that we should obtain hash=NULL for it to work. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [systemd-commits] src/cryptsetup
Le 24/11/2014 19:17, Zbigniew Jędrzejewski-Szmek a écrit : On Mon, Nov 24, 2014 at 07:03:27PM +0100, Quentin Lefebvre wrote: Le 24/11/2014 19:01, Zbigniew Jędrzejewski-Szmek a écrit : On Mon, Nov 24, 2014 at 06:44:25PM +0100, Quentin Lefebvre wrote: Hi, I tested your patch and actually it doesn't solve the bug. For example, if hash=sha512 is provided in /etc/crypttab, the firstif (!streq(arg_hash, plain)) is true, and the +} else if (!key_file) is not reached. This is be design. My patch is quite different from your patch, which I tried to make clear in the description. If you specify hash=sha512, then you get hash=sha512. Yes, and this is the problem. cryptsetup ignores the hash, so that we should obtain hash=NULL for it to work. Systemd is not going to work around a bug in a different package. Specifying a hash in the configuration if you don't want a hash is an error, please just fix it there. I understand your point. Still you have a cryptsetup tool in systemd, so I would expect it behaves as the true cryptsetup program. The problem here is compatibility, you do something with cryptsetup and then your system fails to boot because of a different behaviour of systemd. But it's up to you, that may just get users and installers into trouble. Best regards, Quentin PS: Actually, the good practice is to have a key file obtained from /dev/random, with the correct key size, so I'm not sure hashing the key file matters. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] This patch solves the bug 52630 described here: https://bugs.freedesktop.org/show_bug.cgi?id=52630 .
Hi,
Is there any chance that someone can validate this fix? I tested the
patch against systemd-215 present in Debian testing, but can't test it
with the current version.
However, the patch is very simple and should work with the latest version.
Also, the bug involved is pretty important for cryptsetup / dm-crypt
users, so it would be nice to validate the patch.
Sorry to insist.
Best regards,
Quentin
Le 18/11/2014 15:54, qlefebvre_...@yahoo.com a écrit :
From: Quentin Lefebvre qlefebvre_...@yahoo.com
For plain dm-crypt devices, the behavior of cryptsetup package is to ignore the
hash algorithm when a key file is provided.
With this patch, systemd-cryptsetup now behaves as cryptsetup, so that old
plain dm-crypt devices created with cryptsetup can be mounted at boot time by
systemd, with no modification of /etc/crypttab.
---
src/cryptsetup/cryptsetup.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index 94570eb..88626da 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -403,6 +403,11 @@ static int attach_luks_or_plain(struct crypt_device *cd,
} else
params.hash = ripemd160;
+/* for CRYPT_PLAIN, the behavior of cryptsetup package
+ * is to ignore the hash algorithm when a key file is provided
*/
+if (key_file)
+params.hash = NULL;
+
if (arg_cipher) {
size_t l;
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
