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" <berra...@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 <berra...@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é <phi...@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 :|
