Re: [Qemu-devel] [PATCH v8 10/11] authz: add QAuthZPAM object type for authorizing using PAM

[Philippe Mathieu-Daudé]

Hi Daniel,

On 2/22/19 1:24 PM, Daniel P. Berrangé wrote:
> On Fri, Feb 22, 2019 at 01:34:12AM +0100, Philippe Mathieu-Daudé wrote:
>> Hi Daniel,
>>
>> On 2/15/19 4:57 PM, Daniel P. Berrangé wrote:
>>> From: "Daniel P. Berrange" <address@hidden>
>>>
>>> Add an authorization backend that talks to PAM to check whether the user
>>> identity is allowed. This only uses the PAM account validation facility,
>>> which is essentially just a check to see if the provided username is 
>>> permitted
>>> access. It doesn't use the authentication or session parts of PAM, since
>>> that's dealt with by the relevant part of QEMU (eg VNC server).
>>>
>>> Consider starting QEMU with a VNC server and telling it to use TLS with
>>> x509 client certificates and configuring it to use an PAM to validate
>>> the x509 distinguished name. In this example we're telling it to use PAM
>>> for the QAuthZ impl with a service name of "qemu-vnc"
>>>
>>>  $ qemu-system-x86_64 \
>>>      -object tls-creds-x509,id=tls0,dir=/home/berrange/security/qemutls,\
>>>              endpoint=server,verify-peer=yes \
>>>      -object authz-pam,id=authz0,service=qemu-vnc \
>>>      -vnc :1,tls-creds=tls0,tls-authz=authz0
>>>
>>> This requires an /etc/pam/qemu-vnc file to be created with the auth
>>> rules. A very simple file based whitelist can be setup using
>>>
>>>   $ cat > /etc/pam/qemu-vnc <<EOF
>>>   account         requisite       pam_listfile.so item=user sense=allow 
>>> file=/etc/qemu/vnc.allow
>>>   EOF
>>>
>>> The /etc/qemu/vnc.allow file simply contains one username per line. Any
>>> username not in the file is denied. The usernames in this example are
>>> the x509 distinguished name from the client's x509 cert.
>>>
>>>   $ cat > /etc/qemu/vnc.allow <<EOF
>>>   CN=laptop.berrange.com,O=Berrange Home,L=London,ST=London,C=GB
>>>   EOF
>>>
>>> More interesting would be to configure PAM to use an LDAP backend, so
>>> that the QEMU authorization check data can be centralized instead of
>>> requiring each compute host to have file maintained.
>>>
>>> The main limitation with this PAM module is that the rules apply to all
>>> QEMU instances on the host. Setting up different rules per VM, would
>>> require creating a separate PAM service name & config file for every
>>> guest. An alternative approach for the future might be to not pass in
>>> the plain username to PAM, but instead combine the VM name or UUID with
>>> the username. This requires further consideration though.
>>>
>>> Signed-off-by: Daniel P. Berrange <address@hidden>
>>> ---
> 
>>> +static bool qauthz_pam_is_allowed(QAuthZ *authz,
>>> +                                  const char *identity,
>>> +                                  Error **errp)
>>> +{
>>> +    QAuthZPAM *pauthz = QAUTHZ_PAM(authz);
>>> +    const struct pam_conv pam_conversation = { 0 };
>>> +    pam_handle_t *pamh = NULL;
>>> +    int ret;
>>> +
>>> +    trace_qauthz_pam_check(authz, identity, pauthz->service);
>>> +    ret = pam_start(pauthz->service,
>>> +                    identity,
>>> +                    &pam_conversation,
>>> +                    &pamh);
>>> +    if (ret != PAM_SUCCESS) {
>>> +        error_setg(errp, "Unable to start PAM transaction: %s",
>>> +                   pam_strerror(NULL, ret));

"In an error case is the content of pamh undefined."
So it is safer to use NULL here indeed.

>>> +        return false;
>>> +    }
>>> +
>>> +    ret = pam_acct_mgmt(pamh, PAM_SILENT);
>>> +    if (ret != PAM_SUCCESS) {
>>> +        error_setg(errp, "Unable to authorize user '%s': %s",
>>> +                   identity, pam_strerror(pamh, ret));
>>> +        goto cleanup;
>>> +    }
>>> +
>>> + cleanup:
>>> +    pam_end(pamh, ret);
>>> +    return ret == PAM_SUCCESS;

Hmm I find this fragile.

A 'cleanup' label means (to me) you expect someone to eventually add
more code around, and I'm worried someone add a pam_smth() call after
pam_acct_mgmt(), that sets ret to PAM_SUCCESS.

It looks safer to me to simply not use any label here (for the current
code, if it is extended, we'll see).

>>> +}
>>
>> I still need to digest this function (reading more about PAM).
> 
> FWIW there's reasonably good manpages 'pam(3)' and 'pam(8)' are
> starting points.

Easier inverted, first 'pam(8)' then 'pam(3)' ;)

Here I realize last time I checked 'pam(3)' was 17 years ago...

>>> @@ -2864,6 +2870,33 @@ else
>>>  fi
>>>  
>>>  
>>> +##########################################
>>> +# PAM probe
>>> +
>>> +if test "x$auth_pam" != "no"; then
>>
>> Either check "x$auth_pam" != "xno", or "$auth_pam" != "no" (the latter
>> seems to follow the style of this file).
>>
>> Currently this condition is always true, so the script always calls
>> compile_prog. And if an user has PAM locally installed, it is not
>> possible to not use it.
> 
> Opps, yes, did I say I hate shell :-)
> 
>>
>>> +    cat > $TMPC <<EOF
>>> +#include <security/pam_appl.h>
>>> +#include <stdio.h>
>>> +int main(void) {
>>> +   const char *service_name = "qemu";
>>> +   const char *user = "frank";
>>> +   const struct pam_conv *pam_conv = NULL;
>>> +   pam_handle_t *pamh = NULL;
>>> +   pam_start(service_name, user, pam_conv, &pamh);
>>> +   return 0;
>>> +}
>>> +EOF
>>> +    if compile_prog "" "-lpam" ; then
>>> +   auth_pam=yes
>>> +    else
>>> +   if test "$auth_pam" = "yes"; then
>>> +       feature_not_found "PAM" "Install PAM development package"
>>> +   else
>>> +       auth_pam=no
>>> +   fi
>>> +    fi
> 
> I notice some indentation damage here now due to tabs that I'll also
> fix.

OK.

If you agree on removing the 'cleanup' label in qauthz_pam_is_allowed(),
for the whole patch:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

Regards,

Phil.

> 
> Regards,
> Daniel
>