[systemd-devel] [PATCH 1/3] Add more password agent information

[David Härdeman]

On Tue, Mar 25, 2014 at 01:03:48AM +0100, Lennart Poettering wrote:
>On Wed, 12.02.14 23:55, David Härdeman (david at hardeman.nu) wrote:
>> Add an (optional) "Id" key in the password agent .ask files. The Id is
>> supposed to be a simple string in "<subsystem>:<target>" form which
>> is used to provide more information on what the requested passphrase
>> is to be used for (which e.g. allows an agent to only react to cryptsetup
>> requests).
>
>I wonder if this is related to the "keyhandler" stuff Benjamin Sans has
>asked for.

I think Benjamin and I have basically both come up with the same
solution (though I haven't changed the option from "keyscript=" to
"keyhandler=" since that would break backwards compatibility...which is
partly the point of the whole exercise)...

Bejamin's approach does not seem to solve the binary key part of the
puzzle either...(passing binary keys from the keyscript, as opposed to
passphrases).

My proposed approach is provided in more detail in the corresponding
Debian bug report, see:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=618862#44

So yes, I think they're related...as in they are independent
implementations of the same thing :)

>
>http://lists.freedesktop.org/archives/systemd-devel/2014-March/017869.html
>
>Benjamin, can you comment?
>
>> -        r = ask_password_auto(text, "drive-harddisk", until, accept_cached, passwords);
>> +	if (asprintf(&id, "cryptsetup:%s", name) < 0)
>> +		return log_oom();
>> +
>> +        r = ask_password_auto(text, "drive-harddisk", id, until,
>> accept_cached, passwords);
>
>Hmm, no tabs please...
>
>Also, please use strappend() for cases like this, where we just want to
>concatenate two strings.
>
>That said, I wodner, if we should escape the second part of the string,
>just to be sure. Using cescape() here would suffice?

And risk breaking backward compatibility? (I was too lazy to check
exactly what cescape() handles right now)...

The "name" (second part of the string) comes from /etc/crypttab, which
is writeable by root only on any sane system? (otherwise it'd be dead
easy to insert a keyscript in between which does some keylogging).

>
>>          if (r < 0) {
>>                  log_error("Failed to query password: %s", strerror(-r));
>>                  return r;
>> @@ -281,7 +285,10 @@ static int get_password(const char *name, usec_t until, bool accept_cached, char
>>                  if (asprintf(&text, "Please enter passphrase for disk %s! (verification)", name) < 0)
>>                          return log_oom();
>>  
>> -                r = ask_password_auto(text, "drive-harddisk", until, false, &passwords2);
>> +		if (asprintf(&id, "cryptsetup-verification:%s", name) < 0)
>> +			return log_oom();
>> +
>
>Similar here.
>
>Otherwise this looks good to go, but I'd like to see a comment by
>Benjamin, to see if this would work for him, too!
>
>Lennart
>
>-- 
>Lennart Poettering, Red Hat
>

-- 
David Härdeman