Christian Ullrich <ch...@chrullrich.net> writes: > Updated patch attached.
I unfortunately don't have windows machines to test this on, but I thought it might be helpful to review this anyway since I'm touching code in the same general area (GSSAPI). And as far as I can tell, you don't break anything there; master continues to behave as expected. Some comments inline: > pg_SSPI_recvauth(Port *port) > { > int mtype; > + int status; The section of this function for include_realm checking already uses an int status return code (retval). I would expect to see them share a variable rather than have both "retval" and "status". > + status = pg_SSPI_make_upn(accountname, sizeof(accountname), > + domainname, > sizeof(domainname), > + > port->hba->upn_username); This is the only place I see that this function is called. That being the case, why bother with the sizes of parameters? Why doesn't pg_SSPI_make_upn() just call sizeof() by itself rather than taking those as arguments? > + /* Build SAM name (DOMAIN\\user), then translate to UPN > + (user@kerberos.realm). The realm name is returned in > + lower case, but that is fine because in SSPI auth, > + string comparisons are always case-insensitive. */ Since we're already considering changing things: this is not the comment style for this file (though it is otherwise a good comment). > + upname = (char*)palloc(upnamesize); I don't believe this cast is typically included. > + /* Replace domainname with realm name. */ > + if (upnamerealmsize > domainnamesize) > + { > + pfree(upname); > + ereport(LOG, > + (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION), > + errmsg("realm name too long"))); > + return STATUS_ERROR; > + } > + > + /* Length is now safe. */ > + strcpy(domainname, p+1); Is this an actual fail state or something born out of convenience? A naive reading of this code doesn't explain why it's forbidden for the upn realm to be longer than the domain name. > + /* Replace account name as well (in case UPN != SAM)? */ > + if (update_accountname) > + { > + if ((p - upname + 1) > accountnamesize) > + { > + pfree(upname); > + ereport(LOG, > + > (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION), > + errmsg("translated account name too > long"))); > + return STATUS_ERROR; > + } > + > + *p = 0; > + strcpy(accountname, upname); Same as above. Minus the few small things above, this looks good to me, assuming it resolves the issue. --Robbie
signature.asc
Description: PGP signature