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
