2010-05-05 17:23:57

by Timo Aaltonen

[permalink] [raw]
Subject: Re: [PATCH] Check for AD style machine principal name


CC:ing the "new" list.

On Tue, 27 Apr 2010, J. Bruce Fields wrote:

> On Mon, Apr 26, 2010 at 05:00:59PM +0300, [email protected] 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?

>> + int len = strlen(name);
>> + while(i < len) {
>> + buf[i] = toupper(buf[i]);
>> + i++;
>> + }
>> + buf[i++] = '$';
>> + buf[i] = 0;
>
> This is past the end of the buffer. Maybe you want buf to be
> strlen(name)+1?

Ok, so:
char *buf = malloc(sizeof(name)+1);
buf = strdup(name);
.
.
works, and should be ok?

>> @@ -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.


--
Timo Aaltonen
Systems Specialist
IT Services, Aalto University