[Libvir] PATCH: 6/10: remote driver auth callback API

[Daniel P. Berrange]

On Mon, Dec 03, 2007 at 11:53:04PM +0100, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange at redhat.com> wrote:
> > This patch implements internal driver API for authentication callbacks
> > in the remote driver. It is basically a bunch of code to bridge from
> > the libvirt public API for auth/credentials and the SASL equivalent
> > API. The libvirt API is very close in style to the SASL API so it is
> > a fairly mechanical mapping.
> 
> I have to start by admitting I've never used or even looked at
> policykit before.
> 
> > diff -r 98599cfde033 src/libvirt.c
> > --- a/src/libvirt.c	Wed Nov 28 23:01:08 2007 -0500
> > +++ b/src/libvirt.c	Wed Nov 28 23:29:58 2007 -0500
> > @@ -62,6 +62,78 @@ static int initialized = 0;
> >  #define DEBUG0
> >  #define DEBUG(fs,...)
> >  #endif /* !ENABLE_DEBUG */
> > +
> > +static int virConnectAuthCallbackDefault(virConnectCredentialPtr cred,
> > +                                         unsigned int ncred,
> > +                                         void *cbdata ATTRIBUTE_UNUSED) {
> > +    int i;
> > +
> > +    for (i = 0 ; i < ncred ; i++) {
> > +        char buf[1024];
> > +        char *bufptr = buf;
> > +
> > +        printf("%s:", cred[i].prompt);
> > +        fflush(stdout);
> 
> If printf or fflush fails, this probably return -1.

Good point.

> 
> > +        switch (cred[i].type) {
> > +        case VIR_CRED_USERNAME:
> > +        case VIR_CRED_AUTHNAME:
> > +        case VIR_CRED_ECHOPROMPT:
> > +        case VIR_CRED_REALM:
> > +            if (!fgets(buf, sizeof(buf), stdin)) {
> > +                return -1;
> > +            }
> 
> A consistency nit: you might want to make EOF be treated the same as
> an empty name.  Currently typing EOF to fgets (which then returns NULL)
> makes this code return -1, while entering an empty line doesn't.
> At least with passwords, I confirmed that cvs login treats ^D like
> the empty string.
> 
> On the other hand, an empty name probably makes no sense in many
> applications.

Actually an empty name does make sense for things like REALM/AUTHNAME
as it potentially means use the default value. Its upto underlying SASL
impl whether is likes emptry names or not, so we should be treating EOF
as same as "".

> > +            if (buf[strlen(buf)-1] == '\n')
> > +                buf[strlen(buf)-1] = '\0';
> > +            break;
> > +
> > +        case VIR_CRED_PASSPHRASE:
> > +        case VIR_CRED_NOECHOPROMPT:
> > +            bufptr = getpass("");
> 
> If getpass fails (it'd return NULL), return -1.
> Otherwise, the following strdup would segfault.

Yep, will do.

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=|