[PATCH 1/2] crypt: Prevent asking for password multiple times if non-default crypt name is used.

[PATCH 1/2] crypt: Prevent asking for password multiple times if non-default crypt name is used.

[Colin Guthrie]

If a non-default device mapper name is used for an encrypted partion is
used, (i.e. not luks-$UUID) due to parsing of /etc/crypttab, then the
short-circuits put in place to prevent asking the password twice do not
work.

This would not normally be an issue as the settled job itself should be
removed after it has run and thus cannot be run again. Sadly, due to
the corresponding udev rule using ACTION="add|changed", and the fact
that trying to unlock the device (whether successful or not) seems to
trigger a changed event, it means the settled job is recreated with
each itteration thus causing the whole loop to run again.

It is this situation that the short-circuit exits would normally come
into play but sadly do not work when non-standard names are used.

By the time the /tmp/cryptroot-asked-$2 file is written near the end of
the script, the value of $2 has already been lost due to the argument
parsing code's use of 'shift'. So while on systems where the default
name is used are protected by checking /dev/mapper/xxxx, the
/tmp/cryptroot-asked-$2 file didn't help on systems where this was not
used due to this bug.

So this commit shuffles things around somewhat such that:

 1. The /dev/mapper/xxxx device is checked *after* resolving $2 (which
    contains the default name) to whatever /etc/crypttab specifies.
 2. The cryptroot-asked-xxxx file also uses the translated name both
    for the initial check and to flag when it's written.

As a separate fix, it might make sense to change the udev rule to only
act on add events rather than add|change events, but I'm not sure of the
ramifications of such a change and there may be cases where the add
event is missed and thus the change event needs to be included.
---
 modules.d/90crypt/cryptroot-ask.sh | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/modules.d/90crypt/cryptroot-ask.sh b/modules.d/90crypt/cryptroot-ask.sh
index 9665e48..cf13069 100755
--- a/modules.d/90crypt/cryptroot-ask.sh
+++ b/modules.d/90crypt/cryptroot-ask.sh
@@ -8,20 +8,6 @@ NEWROOT=${NEWROOT:-"/sysroot"}
 # do not ask, if we already have root
 [ -f $NEWROOT/proc ] && exit 0
 
-# check if destination already exists
-[ -b /dev/mapper/$2 ] && exit 0
-
-# we already asked for this device
-[ -f /tmp/cryptroot-asked-$2 ] && exit 0
-
-# load dm_crypt if it is not already loaded
-[ -d /sys/module/dm_crypt ] || modprobe dm_crypt
-
-. /lib/dracut-crypt-lib.sh
-
-# default luksname - luks-UUID
-luksname=$2
-
 # fallback to passphrase
 ask_passphrase=1
 
@@ -32,6 +18,9 @@ else
     device="$1"
 fi
 
+# default luksname - luks-UUID
+luksname=$2
+
 # number of tries
 numtries=${3:-10}
 
@@ -63,6 +52,17 @@ if [ -f /etc/crypttab ] && getargbool 1 rd.luks.crypttab -d -n rd_NO_CRYPTTAB; t
     unset name dev
 fi
 
+# check if destination already exists
+[ -b /dev/mapper/$luksname ] && exit 0
+
+# we already asked for this device
+[ -f /tmp/cryptroot-asked-$luksname ] && exit 0
+
+# load dm_crypt if it is not already loaded
+[ -d /sys/module/dm_crypt ] || modprobe dm_crypt
+
+. /lib/dracut-crypt-lib.sh
+
 #
 # Open LUKS device
 #
@@ -157,7 +157,7 @@ fi
 unset device luksname luksfile
 
 # mark device as asked
->> /tmp/cryptroot-asked-$2
+>> /tmp/cryptroot-asked-$luksname
 
 need_shutdown
 udevsettle
-- 
1.8.4.5

[PATCH 2/2] crypt: Only write udev rules to unlock when the device is added

[Colin Guthrie]

It seems that when the device is unlocked, it will trigger a change
event which in turn re-writes the settled job which was diligently
removed after it was run.

Due to the previous fix, this isn't crazy important as the triggered
job should exit, but avoiding this in the first place is still
desirable.
---

Note, I'm not sure if this is wise or not. The previous patch alone
fixes the issue, so this is just an efficiency saving. It might be
that this is needed - e.g. if you have any encrypted volume on top
of some other mapped device such as RAID, so please only apply this
patch if you are sure it will not cause regressions!!



 modules.d/90crypt/parse-crypt.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/modules.d/90crypt/parse-crypt.sh b/modules.d/90crypt/parse-crypt.sh
index a6b5252..cfa6ac9 100755
--- a/modules.d/90crypt/parse-crypt.sh
+++ b/modules.d/90crypt/parse-crypt.sh
@@ -10,7 +10,7 @@ if ! getargbool 1 rd.luks -d -n rd_NO_LUKS; then
 else
     {
         echo 'SUBSYSTEM!="block", GOTO="luks_end"'
-        echo 'ACTION!="add|change", GOTO="luks_end"'
+        echo 'ACTION!="add", GOTO="luks_end"'
     } > /etc/udev/rules.d/70-luks.rules.new
 
     LUKS=$(getargs rd.luks.uuid -d rd_LUKS_UUID)
-- 
1.8.4.5

[PATCH v2] crypt: Prevent asking for password multiple times if non-default crypt name is used.

[Colin Guthrie]

If a non-default device mapper name is used for an encrypted partion is
used, (i.e. not luks-$UUID) due to parsing of /etc/crypttab, then the
short-circuits put in place to prevent asking the password twice do not
work.

This would not normally be an issue as the settled job itself should be
removed after it has run and thus cannot be run again. Sadly, due to
the corresponding udev rule using ACTION="add|changed", and the fact
that trying to unlock the device (whether successful or not) seems to
trigger a changed event, it means the settled job is recreated with
each itteration thus causing the whole loop to run again.

It is this situation that the short-circuit exits would normally come
into play but sadly do not work when non-standard names are used.

By the time the /tmp/cryptroot-asked-$2 file is written near the end of
the script, the value of $2 has already been lost due to the argument
parsing code's use of 'shift'. So while on systems where the default
name is used are protected by checking /dev/mapper/xxxx, the
/tmp/cryptroot-asked-$2 file didn't help on systems where this was not
used due to this bug.

So this commit shuffles things around somewhat such that:

 1. The /dev/mapper/xxxx device is checked *after* resolving $2 (which
    contains the default name) to whatever /etc/crypttab specifies.
 2. The cryptroot-asked-xxxx file also uses the translated name both
    for the initial check and to flag when it's written.

As a separate fix, it might make sense to change the udev rule to only
act on add events rather than add|change events, but I'm not sure of the
ramifications of such a change and there may be cases where the add
event is missed and thus the change event needs to be included.
---

 v2: Fix issue where getargbool was not defined due to deferred loading
     of dracut-crypt-lib.sh

 modules.d/90crypt/cryptroot-ask.sh | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/modules.d/90crypt/cryptroot-ask.sh b/modules.d/90crypt/cryptroot-ask.sh
index 9665e48..18b2d38 100755
--- a/modules.d/90crypt/cryptroot-ask.sh
+++ b/modules.d/90crypt/cryptroot-ask.sh
@@ -8,22 +8,7 @@ NEWROOT=${NEWROOT:-"/sysroot"}
 # do not ask, if we already have root
 [ -f $NEWROOT/proc ] && exit 0
 
-# check if destination already exists
-[ -b /dev/mapper/$2 ] && exit 0
-
-# we already asked for this device
-[ -f /tmp/cryptroot-asked-$2 ] && exit 0
-
-# load dm_crypt if it is not already loaded
-[ -d /sys/module/dm_crypt ] || modprobe dm_crypt
-
-. /lib/dracut-crypt-lib.sh
-
-# default luksname - luks-UUID
-luksname=$2
-
-# fallback to passphrase
-ask_passphrase=1
+. /lib/dracut-lib.sh
 
 # if device name is /dev/dm-X, convert to /dev/mapper/name
 if [ "${1##/dev/dm-}" != "$1" ]; then
@@ -32,6 +17,9 @@ else
     device="$1"
 fi
 
+# default luksname - luks-UUID
+luksname=$2
+
 # number of tries
 numtries=${3:-10}
 
@@ -63,6 +51,17 @@ if [ -f /etc/crypttab ] && getargbool 1 rd.luks.crypttab -d -n rd_NO_CRYPTTAB; t
     unset name dev
 fi
 
+# check if destination already exists
+[ -b /dev/mapper/$luksname ] && exit 0
+
+# we already asked for this device
+[ -f /tmp/cryptroot-asked-$luksname ] && exit 0
+
+# load dm_crypt if it is not already loaded
+[ -d /sys/module/dm_crypt ] || modprobe dm_crypt
+
+. /lib/dracut-crypt-lib.sh
+
 #
 # Open LUKS device
 #
@@ -112,6 +111,9 @@ fi
 
 unset allowdiscards
 
+# fallback to passphrase
+ask_passphrase=1
+
 if [ -n "$luksfile" -a "$luksfile" != "none" -a -e "$luksfile" ]; then
     if cryptsetup --key-file "$luksfile" $cryptsetupopts luksOpen "$device" "$luksname"; then
         ask_passphrase=0
@@ -157,7 +159,7 @@ fi
 unset device luksname luksfile
 
 # mark device as asked
->> /tmp/cryptroot-asked-$2
+>> /tmp/cryptroot-asked-$luksname
 
 need_shutdown
 udevsettle
-- 
1.8.4.5

Re: [PATCH v2] crypt: Prevent asking for password multiple times if non-default crypt name is used.

[James Lee]

On 12/27/2013 08:00 AM, Colin Guthrie wrote:
> If a non-default device mapper name is used for an encrypted partion is
> used, (i.e. not luks-$UUID) due to parsing of /etc/crypttab, then the
> short-circuits put in place to prevent asking the password twice do not
> work.
> 
> This would not normally be an issue as the settled job itself should be
> removed after it has run and thus cannot be run again. Sadly, due to
> the corresponding udev rule using ACTION="add|changed", and the fact
> that trying to unlock the device (whether successful or not) seems to
> trigger a changed event, it means the settled job is recreated with
> each itteration thus causing the whole loop to run again.
> 
> It is this situation that the short-circuit exits would normally come
> into play but sadly do not work when non-standard names are used.
> 
> By the time the /tmp/cryptroot-asked-$2 file is written near the end of
> the script, the value of $2 has already been lost due to the argument
> parsing code's use of 'shift'. So while on systems where the default
> name is used are protected by checking /dev/mapper/xxxx, the
> /tmp/cryptroot-asked-$2 file didn't help on systems where this was not
> used due to this bug.
> 
> So this commit shuffles things around somewhat such that:
> 
>  1. The /dev/mapper/xxxx device is checked *after* resolving $2 (which
>     contains the default name) to whatever /etc/crypttab specifies.
>  2. The cryptroot-asked-xxxx file also uses the translated name both
>     for the initial check and to flag when it's written.
> 
> As a separate fix, it might make sense to change the udev rule to only
> act on add events rather than add|change events, but I'm not sure of the
> ramifications of such a change and there may be cases where the add
> event is missed and thus the change event needs to be included.
> ---
> 
>  v2: Fix issue where getargbool was not defined due to deferred loading
>      of dracut-crypt-lib.sh

I believe I've seen the situation this patch is trying to fix.  I will
test these patches with my fairly complex crypt setup [1] this weekend.

James

[1]
https://thestaticvoid.com/post/2013/10/26/how-i-do-encrypted-mirrored-zfs-root-on-linux/

Re: [PATCH v2] crypt: Prevent asking for password multiple times if non-default crypt name is used.

[James Lee]

On 01/10/2014 11:55 AM, James Lee wrote:
> I believe I've seen the situation this patch is trying to fix.  I will
> test these patches with my fairly complex crypt setup [1] this weekend.
> 
> James
> 
> [1]
> https://thestaticvoid.com/post/2013/10/26/how-i-do-encrypted-mirrored-zfs-root-on-linux/

Both patches (this one and the udev rule one) work great on all the
systems I tried them on.  Solves the problem I was seeing with being
asked for the passphrase multiple times.

Thanks!

James

Re: [PATCH 2/2] crypt: Only write udev rules to unlock when the device is added

[James Lee]

On 12/26/2013 04:26 PM, Colin Guthrie wrote:
> It seems that when the device is unlocked, it will trigger a change
> event which in turn re-writes the settled job which was diligently
> removed after it was run.
> 
> Due to the previous fix, this isn't crazy important as the triggered
> job should exit, but avoiding this in the first place is still
> desirable.
> ---
> 
> Note, I'm not sure if this is wise or not. The previous patch alone
> fixes the issue, so this is just an efficiency saving. It might be
> that this is needed - e.g. if you have any encrypted volume on top
> of some other mapped device such as RAID, so please only apply this
> patch if you are sure it will not cause regressions!!

Actually, after some more testing with this particular patch, I'm not so
sure about it.  I have encrypted devices that depend on other encrypted
devices.  With this patch applied I sometimes get prompted for
passphrases out of order.

I'll try to do some more digging and figure out why.

Thanks,

James

Re: [PATCH v2] crypt: Prevent asking for password multiple times if non-default crypt name is used.

[Colin Guthrie]

'Twas brillig, and James Lee at 13/01/14 15:30 did gyre and gimble:
> On 01/10/2014 11:55 AM, James Lee wrote:
>> I believe I've seen the situation this patch is trying to fix.  I will
>> test these patches with my fairly complex crypt setup [1] this weekend.
>>
>> James
>>
>> [1]
>> https://thestaticvoid.com/post/2013/10/26/how-i-do-encrypted-mirrored-zfs-root-on-linux/
> 
> Both patches (this one and the udev rule one) work great on all the
> systems I tried them on.  Solves the problem I was seeing with being
> asked for the passphrase multiple times.

Cool, thanks for testing!

Harald, care to commit at least one of them? I'm pretty sure the first
patch (v2) is safe on it's own. I'm not 100% sure about the udev bit as
although it solves the problem, it might have some unintended side effects.

Col

-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/

Re: [PATCH v2] crypt: Prevent asking for password multiple times if non-default crypt name is used.

[Harald Hoyer]

On 12/27/2013 02:00 PM, Colin Guthrie wrote:
> If a non-default device mapper name is used for an encrypted partion is
> used, (i.e. not luks-$UUID) due to parsing of /etc/crypttab, then the
> short-circuits put in place to prevent asking the password twice do not
> work.
> 
> This would not normally be an issue as the settled job itself should be
> removed after it has run and thus cannot be run again. Sadly, due to
> the corresponding udev rule using ACTION="add|changed", and the fact
> that trying to unlock the device (whether successful or not) seems to
> trigger a changed event, it means the settled job is recreated with
> each itteration thus causing the whole loop to run again.
> 
> It is this situation that the short-circuit exits would normally come
> into play but sadly do not work when non-standard names are used.
> 
> By the time the /tmp/cryptroot-asked-$2 file is written near the end of
> the script, the value of $2 has already been lost due to the argument
> parsing code's use of 'shift'. So while on systems where the default
> name is used are protected by checking /dev/mapper/xxxx, the
> /tmp/cryptroot-asked-$2 file didn't help on systems where this was not
> used due to this bug.
> 
> So this commit shuffles things around somewhat such that:
> 
>  1. The /dev/mapper/xxxx device is checked *after* resolving $2 (which
>     contains the default name) to whatever /etc/crypttab specifies.
>  2. The cryptroot-asked-xxxx file also uses the translated name both
>     for the initial check and to flag when it's written.
> 
> As a separate fix, it might make sense to change the udev rule to only
> act on add events rather than add|change events, but I'm not sure of the
> ramifications of such a change and there may be cases where the add
> event is missed and thus the change event needs to be included.
> ---
> 
>  v2: Fix issue where getargbool was not defined due to deferred loading
>      of dracut-crypt-lib.sh
> 
>  modules.d/90crypt/cryptroot-ask.sh | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 

Thanks! Pushed.

Re: [PATCH 2/2] crypt: Only write udev rules to unlock when the device is added

[Harald Hoyer]

On 12/26/2013 10:26 PM, Colin Guthrie wrote:
> It seems that when the device is unlocked, it will trigger a change
> event which in turn re-writes the settled job which was diligently
> removed after it was run.
> 
> Due to the previous fix, this isn't crazy important as the triggered
> job should exit, but avoiding this in the first place is still
> desirable.
> ---
> 
> Note, I'm not sure if this is wise or not. The previous patch alone
> fixes the issue, so this is just an efficiency saving. It might be
> that this is needed - e.g. if you have any encrypted volume on top
> of some other mapped device such as RAID, so please only apply this
> patch if you are sure it will not cause regressions!!
> 
> 
> 
>  modules.d/90crypt/parse-crypt.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/modules.d/90crypt/parse-crypt.sh b/modules.d/90crypt/parse-crypt.sh
> index a6b5252..cfa6ac9 100755
> --- a/modules.d/90crypt/parse-crypt.sh
> +++ b/modules.d/90crypt/parse-crypt.sh
> @@ -10,7 +10,7 @@ if ! getargbool 1 rd.luks -d -n rd_NO_LUKS; then
>  else
>      {
>          echo 'SUBSYSTEM!="block", GOTO="luks_end"'
> -        echo 'ACTION!="add|change", GOTO="luks_end"'
> +        echo 'ACTION!="add", GOTO="luks_end"'
>      } > /etc/udev/rules.d/70-luks.rules.new
>  
>      LUKS=$(getargs rd.luks.uuid -d rd_LUKS_UUID)
> 

Hmm, dm-devices get only active on "change" :-/ So, if a crypt device is on
another dm-device, the rule would not trigger anymore.

Re: [PATCH v2] crypt: Prevent asking for password multiple times if non-default crypt name is used.

[Colin Guthrie]

'Twas brillig, and Harald Hoyer at 28/01/14 14:38 did gyre and gimble:
> On 12/27/2013 02:00 PM, Colin Guthrie wrote:
>> If a non-default device mapper name is used for an encrypted partion is
>> used, (i.e. not luks-$UUID) due to parsing of /etc/crypttab, then the
>> short-circuits put in place to prevent asking the password twice do not
>> work.
>>
>> This would not normally be an issue as the settled job itself should be
>> removed after it has run and thus cannot be run again. Sadly, due to
>> the corresponding udev rule using ACTION="add|changed", and the fact
>> that trying to unlock the device (whether successful or not) seems to
>> trigger a changed event, it means the settled job is recreated with
>> each itteration thus causing the whole loop to run again.
>>
>> It is this situation that the short-circuit exits would normally come
>> into play but sadly do not work when non-standard names are used.
>>
>> By the time the /tmp/cryptroot-asked-$2 file is written near the end of
>> the script, the value of $2 has already been lost due to the argument
>> parsing code's use of 'shift'. So while on systems where the default
>> name is used are protected by checking /dev/mapper/xxxx, the
>> /tmp/cryptroot-asked-$2 file didn't help on systems where this was not
>> used due to this bug.
>>
>> So this commit shuffles things around somewhat such that:
>>
>>  1. The /dev/mapper/xxxx device is checked *after* resolving $2 (which
>>     contains the default name) to whatever /etc/crypttab specifies.
>>  2. The cryptroot-asked-xxxx file also uses the translated name both
>>     for the initial check and to flag when it's written.
>>
>> As a separate fix, it might make sense to change the udev rule to only
>> act on add events rather than add|change events, but I'm not sure of the
>> ramifications of such a change and there may be cases where the add
>> event is missed and thus the change event needs to be included.
>> ---
>>
>>  v2: Fix issue where getargbool was not defined due to deferred loading
>>      of dracut-crypt-lib.sh
>>
>>  modules.d/90crypt/cryptroot-ask.sh | 36 +++++++++++++++++++-----------------
>>  1 file changed, 19 insertions(+), 17 deletions(-)
>>
> 
> Thanks! Pushed.

Excellent, thanks. Your timing is good. I was going to pester you about
this on Thursday. I can chalk this one of my list now :p

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/

Re: [PATCH 2/2] crypt: Only write udev rules to unlock when the device is added

[Colin Guthrie]

'Twas brillig, and Harald Hoyer at 28/01/14 14:39 did gyre and gimble:
> On 12/26/2013 10:26 PM, Colin Guthrie wrote:
>> It seems that when the device is unlocked, it will trigger a change
>> event which in turn re-writes the settled job which was diligently
>> removed after it was run.
>>
>> Due to the previous fix, this isn't crazy important as the triggered
>> job should exit, but avoiding this in the first place is still
>> desirable.
>> ---
>>
>> Note, I'm not sure if this is wise or not. The previous patch alone
>> fixes the issue, so this is just an efficiency saving. It might be
>> that this is needed - e.g. if you have any encrypted volume on top
>> of some other mapped device such as RAID, so please only apply this
>> patch if you are sure it will not cause regressions!!
>>
>>
>>
>>  modules.d/90crypt/parse-crypt.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/modules.d/90crypt/parse-crypt.sh b/modules.d/90crypt/parse-crypt.sh
>> index a6b5252..cfa6ac9 100755
>> --- a/modules.d/90crypt/parse-crypt.sh
>> +++ b/modules.d/90crypt/parse-crypt.sh
>> @@ -10,7 +10,7 @@ if ! getargbool 1 rd.luks -d -n rd_NO_LUKS; then
>>  else
>>      {
>>          echo 'SUBSYSTEM!="block", GOTO="luks_end"'
>> -        echo 'ACTION!="add|change", GOTO="luks_end"'
>> +        echo 'ACTION!="add", GOTO="luks_end"'
>>      } > /etc/udev/rules.d/70-luks.rules.new
>>  
>>      LUKS=$(getargs rd.luks.uuid -d rd_LUKS_UUID)
>>
> 
> Hmm, dm-devices get only active on "change" :-/ So, if a crypt device is on
> another dm-device, the rule would not trigger anymore.


Cool, I was unsure about this one as mentioned in the message, so thanks
for clarifying. It did solve things in a test case I had but I did
wonder if it would cause regressions elsewhere!

Thanks!

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited http://www.tribalogic.net/
Open Source:
  Mageia Contributor http://www.mageia.org/
  PulseAudio Hacker http://www.pulseaudio.org/
  Trac Hacker http://trac.edgewall.org/