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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Feb 22, 2019 at 02:27:26PM +0100, Philippe Mathieu-Daudé wrote:
> 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" <berrange@redhat.com>
> >>>
> >>> 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 <berrange@redhat.com>
> >>> ---
> > 
> >>> +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).

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

Thanks, I will squash this in:

diff --git a/authz/pamacct.c b/authz/pamacct.c
index 8fe4c8ee11..5038358cdc 100644
--- a/authz/pamacct.c
+++ b/authz/pamacct.c
@@ -47,15 +47,14 @@ static bool qauthz_pam_is_allowed(QAuthZ *authz,
     }
 
     ret = pam_acct_mgmt(pamh, PAM_SILENT);
+    pam_end(pamh, ret);
     if (ret != PAM_SUCCESS) {
         error_setg(errp, "Unable to authorize user '%s': %s",
                    identity, pam_strerror(pamh, ret));
-        goto cleanup;
+        return false;
     }
 
- cleanup:
-    pam_end(pamh, ret);
-    return ret == PAM_SUCCESS;
+    return true;
 }
 
 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|