-----Original Message----- From: linux-nfs-owner@xxxxxxxxxxxxxxx [mailto:linux-nfs-owner@xxxxxxxxxxxxxxx] On Behalf Of Timo Aaltonen Sent: Wednesday, May 05, 2010 6:16 PM To: Kevin Coffman Cc: J. Bruce Fields; linux-nfs@xxxxxxxxxxxxxxx Subject: Re: [PATCH] Check for AD style machine principal name On Wed, 5 May 2010, Kevin Coffman wrote: > On Wed, May 5, 2010 at 12:53 PM, Timo Aaltonen <timo.aaltonen@xxxxxxxx> wrote: >> >> CC:ing the "new" list. >> >> On Tue, 27 Apr 2010, J. Bruce Fields wrote: >> >>> On Mon, Apr 26, 2010 at 05:00:59PM +0300, timo.aaltonen@xxxxxxxx wrote: >>>> >>>> @@ -737,6 +737,26 @@ gssd_search_krb5_keytab(krb5_context context, >>>> krb5_keytab kt, >>>> } >>>> >>>> /* >>>> + * Convert the hostname to machine principal name as created >>>> + * by MS Active Directory. >>>> +*/ >>>> + >>>> +static char * >>>> +hostname_to_adprinc(char *name) >>>> +{ >>>> + int i = 0; >>>> + char *buf = strdup(name); >>> >>> We should handle the possible NULL return. >> >> How about not calling hostname_to_adprinc from find_keytab_entry unless >> myhostname is != NULL? > > I think he meant that strdup() may fail, and return NULL. That has to > be handled here, and a possible resulting NULL return from > host_to_adprinc() should also be handled below. Ahh.. ok so how about this: static char * hostname_to_adprinc(char *name) { int i = 0; int len = strlen(name); char *buf; if ((buf = malloc(len+1))) { > Doesn't this need to be "+2" since you are adding a '$' and a '\0' to the end of the string, neither of which is included in the strlen() calculation? strcpy(buf, name); while(i < len) { buf[i] = toupper(buf[i]); i++; } > Instead of copying name to buf and then converting the characters in buf to uppercase, why not copy and convert in one loop? buf[i++] = '$'; buf[i] = 0; return buf; } return NULL; } and then adding 'if (adprinc != NULL) { ... }' to the while loop in find_keytab_entry. That passes my tests. >>>> @@ -812,6 +836,35 @@ find_keytab_entry(krb5_context context, krb5_keytab >>>> kt, const char *hostname, >>>> break; >>>> if (strcmp(realm, default_realm) == 0) >>>> tried_default = 1; >>>> + /* First try the Active Directory style machine principal >>>> */ >>>> + code = krb5_build_principal_ext(context, &princ, >>>> + strlen(realm), >>>> + realm, >>>> + strlen(adprinc), >>>> + adprinc, >>>> + NULL); >>>> + if (code) { >>>> + k5err = gssd_k5_err_msg(context, code); >>>> + printerr(1, "%s while building principal for " >>>> + "'%s@%s'\n", k5err, >>>> + adprinc, realm); >>>> + continue; >>> >>> Is there an infinite loop here? >> >> Not to my knowledge, it's basically the same chunk of code copied from a few >> lines below, modified to suit the adprinc. > > The chunk you copied was within a for() loop. You are calling > continue within the while(1) loop. Ok, so 'continue' was extra, whoops.. -- Timo Aaltonen Systems Specialist IT Services, Aalto University -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
