Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On Fri, Apr 8, 2016 at 1:38 PM, Magnus Hagander wrote: > On Tue, Mar 29, 2016 at 11:24 PM, Christian Ullrich > wrote: > >> * Magnus Hagander wrote: >> >> On Tue, Mar 29, 2016 at 5:09 PM, David Steele >>> wrote: >>> >> >> It seems like this patch should be set "ready for committer". Can one of the reviewers do that if appropriate? >>> >>> I'll pick it up to do that as well as committing it. >>> >> >> Ah, good news! >> >> I hope it's not coming too late, but I have a final update removing a >> rather pointless palloc() return value check. No changes otherwise. > > > Small notes: > > * I think it's wrong to have the docs say "leave this at the default to > maintain compatibility" in the reference section - if anything, that's for > release notes. And it's the default behaviour. So I just removed that one > > * Made some other wordsmithing on the SGML. > > * This looks strange to me: > if (!res || p == NULL) > > it's correct logically, the style just looks weird. But maybe it's a good > idea to keep it to make it clear that res is a bool and p is a pointer. I'm > on the fence. > > * it also needed a pgindent, in particular a couple of return STATUS_ERROR > were indented in a way that made them look like they were almost in the > wrong place, and some minor style changes. But that's all mechanical. > > Other than those minor things it looks good to me, so I'm going to push > the current version with those once I'm back on reliable wifi. > > And now committed. Thanks! -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On Tue, Mar 29, 2016 at 11:24 PM, Christian Ullrich wrote: > * Magnus Hagander wrote: > > On Tue, Mar 29, 2016 at 5:09 PM, David Steele wrote: >> > > It seems like this patch should be set "ready for committer". Can one of >>> the reviewers do that if appropriate? >>> >> >> I'll pick it up to do that as well as committing it. >> > > Ah, good news! > > I hope it's not coming too late, but I have a final update removing a > rather pointless palloc() return value check. No changes otherwise. Small notes: * I think it's wrong to have the docs say "leave this at the default to maintain compatibility" in the reference section - if anything, that's for release notes. And it's the default behaviour. So I just removed that one * Made some other wordsmithing on the SGML. * This looks strange to me: if (!res || p == NULL) it's correct logically, the style just looks weird. But maybe it's a good idea to keep it to make it clear that res is a bool and p is a pointer. I'm on the fence. * it also needed a pgindent, in particular a couple of return STATUS_ERROR were indented in a way that made them look like they were almost in the wrong place, and some minor style changes. But that's all mechanical. Other than those minor things it looks good to me, so I'm going to push the current version with those once I'm back on reliable wifi. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On Apr 8, 2016 1:13 AM, "Tom Lane" wrote: > > Magnus Hagander writes: > > On Apr 7, 2016 9:14 PM, "Christian Ullrich" wrote: > >> Magnus, do you intend to commit the patch before the feature freeze? > > > It's on my list of things to work on this weekend, yeah. > > But the stated feature freeze deadline is tomorrow (Friday), not the > weekend or later. > > To the extent that this can be called a bug fix, it might be exempt > from feature freeze, but I'm not on the RMT so I'm not going to make > that call. > Oh, dang, I had put it down as Sunday in my calendar :S I'll have to see what dish the travel-gods hand out today, and try to get it done before. /Magnus
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
Magnus Hagander writes: > On Apr 7, 2016 9:14 PM, "Christian Ullrich" wrote: >> Magnus, do you intend to commit the patch before the feature freeze? > It's on my list of things to work on this weekend, yeah. But the stated feature freeze deadline is tomorrow (Friday), not the weekend or later. To the extent that this can be called a bug fix, it might be exempt from feature freeze, but I'm not on the RMT so I'm not going to make that call. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On Apr 7, 2016 9:14 PM, "Christian Ullrich" wrote: > > * Magnus Hagander wrote: > >> On Tue, Mar 29, 2016 at 5:09 PM, David Steele wrote: > > >>> It seems like this patch should be set "ready for committer". Can one of >>> the reviewers do that if appropriate? > > >> I'll pick it up to do that as well as committing it. > > > Magnus, do you intend to commit the patch before the feature freeze? > It's on my list of things to work on this weekend, yeah. /Magnus
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* Magnus Hagander wrote: On Tue, Mar 29, 2016 at 5:09 PM, David Steele wrote: It seems like this patch should be set "ready for committer". Can one of the reviewers do that if appropriate? I'll pick it up to do that as well as committing it. Magnus, do you intend to commit the patch before the feature freeze? -- Christian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
Alvaro Herrera writes: > Tom Lane wrote: >> Anyway, as things stand, elog(ERROR) will abort the session safely but >> you won't necessarily get the kind of logging you want, so expected >> auth-failure cases should try to go the STATUS_ERROR route. > In other words, the use of palloc() and friends (psprintf in the patch) > should be acceptable here. Sure, no problem with that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
Tom Lane wrote: > Alvaro Herrera writes: > > So, it seems that ClientAuthentication() expects to raise ereport(FATAL) > > in case of authentication failures. But what's the code path that > > causes that to happen if a ereport(ERROR) happens in there? Because all > > that code is pretty careful to not do ereport(ERROR) directly and > > instead return STATUS_ERROR which makes ClientAuthentication do the > > FATAL report. If this doesn't matter, then isn't this whole code overly > > complicated for no reason? > > The reason why elog(ERROR) will become a FATAL is that no outer setjmp > has been executed yet, so elog.c will realize it has noplace to longjmp > to. Ah, I was looking callers up-stack and found nothing. That should have cued me that that was happening :-) > Anyway, as things stand, elog(ERROR) will abort the session safely but > you won't necessarily get the kind of logging you want, so expected > auth-failure cases should try to go the STATUS_ERROR route. In other words, the use of palloc() and friends (psprintf in the patch) should be acceptable here. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
Alvaro Herrera writes: > So, it seems that ClientAuthentication() expects to raise ereport(FATAL) > in case of authentication failures. But what's the code path that > causes that to happen if a ereport(ERROR) happens in there? Because all > that code is pretty careful to not do ereport(ERROR) directly and > instead return STATUS_ERROR which makes ClientAuthentication do the > FATAL report. If this doesn't matter, then isn't this whole code overly > complicated for no reason? The reason why elog(ERROR) will become a FATAL is that no outer setjmp has been executed yet, so elog.c will realize it has noplace to longjmp to. Whether it's overcomplicated I dunno. I think the idea behind returning STATUS_ERROR is to allow a centralized reporting site to decorate the errors with additional info, as indeed auth_fail does. Certainly that could be done another way (errcontext?), but that's the way we've got. Anyway, as things stand, elog(ERROR) will abort the session safely but you won't necessarily get the kind of logging you want, so expected auth-failure cases should try to go the STATUS_ERROR route. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
So, it seems that ClientAuthentication() expects to raise ereport(FATAL) in case of authentication failures. But what's the code path that causes that to happen if a ereport(ERROR) happens in there? Because all that code is pretty careful to not do ereport(ERROR) directly and instead return STATUS_ERROR which makes ClientAuthentication do the FATAL report. If this doesn't matter, then isn't this whole code overly complicated for no reason? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* Magnus Hagander wrote: On Tue, Mar 29, 2016 at 5:09 PM, David Steele wrote: It seems like this patch should be set "ready for committer". Can one of the reviewers do that if appropriate? I'll pick it up to do that as well as committing it. Ah, good news! I hope it's not coming too late, but I have a final update removing a rather pointless palloc() return value check. No changes otherwise. -- Christian diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml new file mode 100644 index 3b2935c..f7d7b5a *** a/doc/src/sgml/client-auth.sgml --- b/doc/src/sgml/client-auth.sgml *** omicron bryanh *** 1097,1102 --- 1097,1140 + compat_realm + + + If set to 1, the domain's SAM-compatible name (also known as the + NetBIOS name) is used for the include_realm + option. This is the default. If set to 0, the true realm name from + the Kerberos user principal name is used. Leave this option + disabled to maintain compatibility with existing + pg_ident.conf files. + + + Do not enable this option unless your server runs under a domain + account (this includes virtual service accounts on a domain member + system) and all clients authenticating through SSPI are also using + domain accounts, or authentication will fail. + + + + + + upn_username + + + If this option is enabled along with compat_realm, + the user name from the Kerberos UPN is used for authentication. If + it is disabled (the default), the SAM-compatible user name is used. + By default, these two names are identical for new user accounts. + + + Note that libpq uses the SAM-compatible name if no + explicit user name is specified. If you use + libpq (e.g. through the ODBC driver), you should + leave this option disabled. + + + + + map diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c new file mode 100644 index 899da71..cedebf1 *** a/src/backend/libpq/auth.c --- b/src/backend/libpq/auth.c *** typedef SECURITY_STATUS *** 155,160 --- 155,165 (WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) ( PCtxtHandle, void **); static intpg_SSPI_recvauth(Port *port); + static intpg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +char *domainname, +size_t domainnamesize, +bool update_accountname); #endif /* *** pg_SSPI_recvauth(Port *port) *** 1265,1270 --- 1270,1284 free(tokenuser); + if (!port->hba->compat_realm) + { + int status = pg_SSPI_make_upn(accountname, sizeof(accountname), + domainname, sizeof(domainname), + port->hba->upn_username); + if (status != STATUS_OK) + return status; + } + /* * Compare realm/domain if requested. In SSPI, always compare case * insensitive. *** pg_SSPI_recvauth(Port *port) *** 1300,1305 --- 1314,1412 else return check_usermap(port->hba->usermap, port->user_name, accountname, true); } + + /* + * Replaces the domainname with the Kerberos realm name, + * and optionally the accountname with the Kerberos user name. + */ + static intpg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +char *domainname, +size_t domainnamesize, +bool update_accountname) + { + char *samname; + char *upname = NULL; + char *p = NULL; + ULONG upnamesize = 0; + size_t upnamerealmsize; + BOOLEAN res; + + /* +* 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. +*/ + + samname = psprintf("%s\\%s", domainname, accountname); + res = TranslateName(samname, NameSamCompatible, NameUserPrincipal, +
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On Tue, Mar 29, 2016 at 5:09 PM, David Steele wrote: > On 3/24/16 5:22 PM, Alvaro Herrera wrote: > >> Christian Ullrich wrote: >> >> To be honest, I'm not sure what can and cannot be done in auth code. I >>> took inspiration from the existing SSPI code and nearly every error >>> check in pg_SSPI_recvauth() ends up doing ereport(ERROR) already, >>> directly or via pg_SSPI_error(). If this could cause serious trouble, >>> someone would have noticed yet. >>> >> >> I think the problem is whether the report is sent to the client or not, >> but I may be confusing with something else (COMMERROR reports?). >> >> What *could* happen, anyway? Can ereport(ERROR) in a backend make the >>> postmaster panic badly enough to force a shared memory reset? >>> >> >> Probably not, since it's running in a backend already at that point, not >> in postmaster. >> > > It seems like this patch should be set "ready for committer". Can one of > the reviewers do that if appropriate? > I'll pick it up to do that as well as committing it. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On 3/24/16 5:22 PM, Alvaro Herrera wrote: Christian Ullrich wrote: To be honest, I'm not sure what can and cannot be done in auth code. I took inspiration from the existing SSPI code and nearly every error check in pg_SSPI_recvauth() ends up doing ereport(ERROR) already, directly or via pg_SSPI_error(). If this could cause serious trouble, someone would have noticed yet. I think the problem is whether the report is sent to the client or not, but I may be confusing with something else (COMMERROR reports?). What *could* happen, anyway? Can ereport(ERROR) in a backend make the postmaster panic badly enough to force a shared memory reset? Probably not, since it's running in a backend already at that point, not in postmaster. It seems like this patch should be set "ready for committer". Can one of the reviewers do that if appropriate? Thanks, -- -David da...@pgmasters.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
Christian Ullrich wrote: > To be honest, I'm not sure what can and cannot be done in auth code. I > took inspiration from the existing SSPI code and nearly every error > check in pg_SSPI_recvauth() ends up doing ereport(ERROR) already, > directly or via pg_SSPI_error(). If this could cause serious trouble, > someone would have noticed yet. I think the problem is whether the report is sent to the client or not, but I may be confusing with something else (COMMERROR reports?). > What *could* happen, anyway? Can ereport(ERROR) in a backend make the > postmaster panic badly enough to force a shared memory reset? Probably not, since it's running in a backend already at that point, not in postmaster. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* From: Alvaro Herrera [mailto:alvhe...@2ndquadrant.com] > Christian Ullrich wrote: > > * Christian Ullrich wrote: > > > > >* From: Magnus Hagander [mailto:mag...@hagander.net] > > > >>Code uses a mix of malloc() and palloc() (through sprintf). Is there > > >>a reason for that? > > > > > >I wasn't sure which to prefer, so I looked around in auth.c, and > > >other than RADIUS, everything seems to use malloc() (although the > > >sample size is not too great). Should I use palloc() instead? > > > > The single instance of malloc() has been replaced with palloc(). > > I'm wary of palloc() in this code actually ... if the allocation fails, > I'm not sure it's okay to use ereport(ERROR) which is what would happen > with palloc. With the malloc code, you report the problem with > elog(LOG) and then return STATUS_ERROR which lets the calling code > handle the failure in a different way. I didn't actually review your > new code, but I recall this from previous readings of auth code; so if > you're going to use palloc(), you better audit what happens on OOM. > > For the same reason, using psprintf is probably not acceptable either. To be honest, I'm not sure what can and cannot be done in auth code. I took inspiration from the existing SSPI code and nearly every error check in pg_SSPI_recvauth() ends up doing ereport(ERROR) already, directly or via pg_SSPI_error(). If this could cause serious trouble, someone would have noticed yet. What *could* happen, anyway? Can ereport(ERROR) in a backend make the postmaster panic badly enough to force a shared memory reset? -- Christian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
Christian Ullrich wrote: > * Christian Ullrich wrote: > > >* From: Magnus Hagander [mailto:mag...@hagander.net] > >>Code uses a mix of malloc() and palloc() (through sprintf). Is there a > >>reason for that? > > > >I wasn't sure which to prefer, so I looked around in auth.c, and other than > >RADIUS, everything seems to use malloc() (although the sample size is not > >too great). Should I use palloc() instead? > > The single instance of malloc() has been replaced with palloc(). I'm wary of palloc() in this code actually ... if the allocation fails, I'm not sure it's okay to use ereport(ERROR) which is what would happen with palloc. With the malloc code, you report the problem with elog(LOG) and then return STATUS_ERROR which lets the calling code handle the failure in a different way. I didn't actually review your new code, but I recall this from previous readings of auth code; so if you're going to use palloc(), you better audit what happens on OOM. For the same reason, using psprintf is probably not acceptable either. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
Christian Ullrich writes: > Updated patch attached. Okay, I am happy now. Thanks! signature.asc Description: PGP signature
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* From: Christian Ullrich > * From: Robbie Harwood [mailto:rharw...@redhat.com] > > > Christian Ullrich writes: > > > + /* 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. > > Because it's copied *into* domainname right there on the last line. > > That said, sizeof(domainname) is MAXPGPATH, which is 1024, so there is > absolutely no chance that the realm could be longer -- it would need an > AD forest at least 16 domains deep. Oh, sorry, I misunderstood the question. Yes, it's due to convenience, but a) it *is* rather convenient given the plentiful buffer I get, and b) doing it differently involves char** inout parameters and potential trouble with pointer aliasing in the caller, both things I'd rather avoid. -- Christian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On 2016-03-24 16:35, Christian Ullrich wrote: * From: Robbie Harwood [mailto:rharw...@redhat.com] Christian Ullrich writes: 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". I would not, because retval is local to that last if, but you are right, status does not need to be in function scope. Moved declaration. + /* 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). True. Will fix. Reformatted. + upname = (char*)palloc(upnamesize); I don't believe this cast is typically included. Left over from when this was malloc() before Magnus' first look at it. Removed. Updated patch attached. -- Christian diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml new file mode 100644 index 3b2935c..f7d7b5a *** a/doc/src/sgml/client-auth.sgml --- b/doc/src/sgml/client-auth.sgml *** omicron bryanh *** 1097,1102 --- 1097,1140 + compat_realm + + + If set to 1, the domain's SAM-compatible name (also known as the + NetBIOS name) is used for the include_realm + option. This is the default. If set to 0, the true realm name from + the Kerberos user principal name is used. Leave this option + disabled to maintain compatibility with existing + pg_ident.conf files. + + + Do not enable this option unless your server runs under a domain + account (this includes virtual service accounts on a domain member + system) and all clients authenticating through SSPI are also using + domain accounts, or authentication will fail. + + + + + + upn_username + + + If this option is enabled along with compat_realm, + the user name from the Kerberos UPN is used for authentication. If + it is disabled (the default), the SAM-compatible user name is used. + By default, these two names are identical for new user accounts. + + + Note that libpq uses the SAM-compatible name if no + explicit user name is specified. If you use + libpq (e.g. through the ODBC driver), you should + leave this option disabled. + + + + + map diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c new file mode 100644 index 7f1ae8c..6830764 *** a/src/backend/libpq/auth.c --- b/src/backend/libpq/auth.c *** typedef SECURITY_STATUS *** 155,160 --- 155,165 (WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) ( PCtxtHandle, void **); static intpg_SSPI_recvauth(Port *port); + static intpg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +char *domainname, +size_t domainnamesize, +bool update_accountname); #endif /* *** pg_SSPI_recvauth(Port *port) *** 1263,1268 --- 1268,1282 free(tokenuser); + if (!port->hba->compat_realm) + { + int status = pg_SSPI_make_upn(accountname, sizeof(accountname), + domainname, sizeof(domainname), + port->hba->upn_username); + if (status != STATUS_OK) + return status; + } + /* * Compare realm/domain if requested. In SSPI, always compare case * insensitive. *** pg_SSPI_recvauth(Port *port) *** 1298,1303 --- 1312,1419 else return check_usermap(port->hba->usermap, port->user_name, accountname, true); } + + /* + * Replaces the domainname with the Kerberos realm name, + * and optionally the accountname with the Kerberos user name. + */ + static intpg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* From: Robbie Harwood [mailto:rharw...@redhat.com] > Christian Ullrich 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. Thanks for the review. > 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". I would not, because retval is local to that last if, but you are right, status does not need to be in function scope. > > + 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? sizeof(array) != sizeof(pointer). > > + /* 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). True. Will fix. > > + upname = (char*)palloc(upnamesize); > > I don't believe this cast is typically included. Left over from when this was malloc() before Magnus' first look at it. > > + /* 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. Because it's copied *into* domainname right there on the last line. That said, sizeof(domainname) is MAXPGPATH, which is 1024, so there is absolutely no chance that the realm could be longer -- it would need an AD forest at least 16 domains deep. > > + /* 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. Yup. -- Christian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On Thu, Mar 24, 2016 at 11:07 AM, Robbie Harwood wrote: > Christian Ullrich 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? Well, suppose there is another caller to that function in the future which wants to use arguments of different lengths. This actually seems pretty sensible to me - concern about the length of the buffer is isolated in the caller, without any spooky action at a distance. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
Christian Ullrich 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
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* Christian Ullrich wrote: * From: Magnus Hagander [mailto:mag...@hagander.net] I don't like the name "real_realm" as a parameter name. I'm wondering if it might be better to reverse the meaning, and call it sspi_netbios_realm (and then change the default to on, to be backwards compatible). What about "compat_realm" instead? "sspi_netbios_realm" is a bit long. Also, the domain short name is not really a realm name, and this would feel like implying that it is. I changed it to "compat_realm". Code uses a mix of malloc() and palloc() (through sprintf). Is there a reason for that? I wasn't sure which to prefer, so I looked around in auth.c, and other than RADIUS, everything seems to use malloc() (although the sample size is not too great). Should I use palloc() instead? The single instance of malloc() has been replaced with palloc(). Another thing: This business of getting a SID, turning it into a user name/domain pair, then using that to get the UPN (which probably involves converting it to the SID again somewhere in between) does not look very good to me. I'll have to see if it works, but what do you think about briefly impersonating the client, just long enough to get the SAM and UPN names? I did not pursue this further; it involves quite a bit more code including several more functions from secur32.dll. These might be a problem on MinGW according to the comment in auth.c regarding QuerySecurityContextToken() (if that is still accurate, because my mingw\lib\libsecur32.a apparently has the export). Updated patch attached. -- Christian diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml new file mode 100644 index 3b2935c..f7d7b5a *** a/doc/src/sgml/client-auth.sgml --- b/doc/src/sgml/client-auth.sgml *** omicron bryanh *** 1097,1102 --- 1097,1140 + compat_realm + + + If set to 1, the domain's SAM-compatible name (also known as the + NetBIOS name) is used for the include_realm + option. This is the default. If set to 0, the true realm name from + the Kerberos user principal name is used. Leave this option + disabled to maintain compatibility with existing + pg_ident.conf files. + + + Do not enable this option unless your server runs under a domain + account (this includes virtual service accounts on a domain member + system) and all clients authenticating through SSPI are also using + domain accounts, or authentication will fail. + + + + + + upn_username + + + If this option is enabled along with compat_realm, + the user name from the Kerberos UPN is used for authentication. If + it is disabled (the default), the SAM-compatible user name is used. + By default, these two names are identical for new user accounts. + + + Note that libpq uses the SAM-compatible name if no + explicit user name is specified. If you use + libpq (e.g. through the ODBC driver), you should + leave this option disabled. + + + + + map diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c new file mode 100644 index 7f1ae8c..f31b06f *** a/src/backend/libpq/auth.c --- b/src/backend/libpq/auth.c *** typedef SECURITY_STATUS *** 155,160 --- 155,165 (WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) ( PCtxtHandle, void **); static intpg_SSPI_recvauth(Port *port); + static intpg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +char *domainname, +size_t domainnamesize, +bool update_accountname); #endif /* *** static int *** 1026,1031 --- 1031,1037 pg_SSPI_recvauth(Port *port) { int mtype; + int status; StringInfoData buf; SECURITY_STATUS r; CredHandle sspicred; *** pg_SSPI_recvauth(Port *port) *** 1263,1268 --- 1269,1283 free(tokenuser); + if (!port->hba->compat_realm) + { + status = pg_SSPI_make_upn(accountname, sizeof(accountname), + domainname, sizeof(domainname), + port->hba->upn_username); + if (status != STATUS_OK) + return status; + } +
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* From: Magnus Hagander [mailto:mag...@hagander.net] > I took a quick look at this one, and have some initial thoughts. > > I don't like the name "real_realm" as a parameter name. I'm wondering if > it might be better to reverse the meaning, and call it sspi_netbios_realm > (and then change the default to on, to be backwards compatible). What about "compat_realm" instead? "sspi_netbios_realm" is a bit long. Also, the domain short name is not really a realm name, and this would feel like implying that it is. > How does the real_realm thing work if you connect with a local account? > Hostname, or kerberos principal for the host? It fails. There is no UPN with a local account, so the conversion does not happen, and I thought it would be best from a security POV to let the logon fail rather than inventing a reason why it should succeed. > Code uses a mix of malloc() and palloc() (through sprintf). Is there a > reason for that? I wasn't sure which to prefer, so I looked around in auth.c, and other than RADIUS, everything seems to use malloc() (although the sample size is not too great). Should I use palloc() instead? > Looking at the docs: > > + Note that libpq uses the SAM-compatible name if > no > + explicit user name is specified. If you use > + libpq (e.g. through the ODBC driver), you should > + leave this option disabled. > > What's the actual usecase where it makes sense to change it? Seems that's > the more reasonable thing to document, with a reference to active > directory specifically (or is there also such a compatible name for local > accounts?) In an environment where sAMAccountName and userPrincipalName are different, it might be preferable to have something to map in pg_ident.conf that is actually a valid user name (UPN, in this case), rather than a mixture of both forms that isn't valid for either. Also, since I already have the UPN, adding the option is basically free and feels more useful than always throwing away half the information. Another thing: This business of getting a SID, turning it into a user name/domain pair, then using that to get the UPN (which probably involves converting it to the SID again somewhere in between) does not look very good to me. I'll have to see if it works, but what do you think about briefly impersonating the client, just long enough to get the SAM and UPN names? -- Christian -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
On Fri, Jan 15, 2016 at 9:46 PM, Christian Ullrich wrote: > * Christian Ullrich wrote: > > * Christian Ullrich wrote: >> >> * Christian Ullrich wrote: >>> >>> > According to the release notes, the default for the "include_realm" >>> > option in SSPI authentication was changed from off to on in 9.5 for >>> >> > > improved security. However, the authenticated user name, with the >> > > option enabled, includes the NetBIOS domain name, *not* the Kerberos >> >>> > realm name: >>> >> >> Below is a patch to correct this behavior. I suspect it has some >>> serious compatibility issues, so I would appreciate feedback. >>> >> >> Updated patch, sorry. The first one worked by accident only. >> > > Another update. This time even the documentation builds. > > One thing I'm fairly sure I need advice on is error handling and/or error > codes. Right now I use ERROR_INVALID_ROLE_SPECIFICATION just about > everywhere (because the surrounding SSPI code does as well), and that is > probably not the best choice in some places. I took a quick look at this one, and have some initial thoughts. I don't like the name "real_realm" as a parameter name. I'm wondering if it might be better to reverse the meaning, and call it sspi_netbios_realm (and then change the default to on, to be backwards compatible). How does the real_realm thing work if you connect with a local account? Hostname, or kerberos principal for the host? Code uses a mix of malloc() and palloc() (through sprintf). Is there a reason for that? Looking at the docs: + Note that libpq uses the SAM-compatible name if no + explicit user name is specified. If you use + libpq (e.g. through the ODBC driver), you should + leave this option disabled. What's the actual usecase where it makes sense to change it? Seems that's the more reasonable thing to document, with a reference to active directory specifically (or is there also such a compatible name for local accounts?) -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* Christian Ullrich wrote: * Christian Ullrich wrote: * Christian Ullrich wrote: > According to the release notes, the default for the "include_realm" > option in SSPI authentication was changed from off to on in 9.5 for > > improved security. However, the authenticated user name, with the > > option enabled, includes the NetBIOS domain name, *not* the Kerberos > realm name: Below is a patch to correct this behavior. I suspect it has some serious compatibility issues, so I would appreciate feedback. Updated patch, sorry. The first one worked by accident only. Another update. This time even the documentation builds. One thing I'm fairly sure I need advice on is error handling and/or error codes. Right now I use ERROR_INVALID_ROLE_SPECIFICATION just about everywhere (because the surrounding SSPI code does as well), and that is probably not the best choice in some places. -- Christian diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml index 3b2935c..cb4fe5e 100644 --- a/doc/src/sgml/client-auth.sgml +++ b/doc/src/sgml/client-auth.sgml @@ -1097,6 +1097,41 @@ omicron bryanh guest1 + real_realm + + +If set to 0, the domain's SAM-compatible name (also known as the +NetBIOS name) is used for the include_realm +option. This is the default. If set to 1, the true realm name from +the Kerberos user principal name is used. Leave this option +disabled to maintain compatibility with existing +pg_ident.conf files. + + +Do not enable this option unless your server runs under a domain +account (this includes virtual service accounts on a domain member +system) and all clients authenticating through SSPI are also using +domain accounts, or authentication will fail. + + + + + + upn_username + + +If this option is enabled along with real_realm, +the user name from the Kerberos UPN is used for authentication. If +it is disabled (the default), the SAM-compatible user name is used. +Note that libpq uses the SAM-compatible name if no +explicit user name is specified. If you use +libpq (e.g. through the ODBC driver), you should +leave this option disabled. + + + + + map diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 57c2f48..0a0f5e3 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -155,6 +155,11 @@ typedef SECURITY_STATUS (WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) ( PCtxtHandle, void **); static int pg_SSPI_recvauth(Port *port); +static int pg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +char *domainname, +size_t domainnamesize, +bool update_accountname); #endif /* @@ -1026,6 +1031,7 @@ static int pg_SSPI_recvauth(Port *port) { int mtype; + int status; StringInfoData buf; SECURITY_STATUS r; CredHandle sspicred; @@ -1263,6 +1269,15 @@ pg_SSPI_recvauth(Port *port) free(tokenuser); + if (port->hba->real_realm) + { + status = pg_SSPI_make_upn(accountname, sizeof(accountname), + domainname, sizeof(domainname), + port->hba->upn_username); + if (status != STATUS_OK) + return status; + } + /* * Compare realm/domain if requested. In SSPI, always compare case * insensitive. @@ -1298,6 +1313,102 @@ pg_SSPI_recvauth(Port *port) else return check_usermap(port->hba->usermap, port->user_name, accountname, true); } + +static int pg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +char *domainname, +size_t domainnamesize, +bool update_accountname) +{ + char *samname; + char *upname = NULL; + char *p = NULL; + ULONG upnamesize = 0; + size_t upnamerealmsize; + BOOLEAN res; + + /* Build SAM name (DOMAIN\\user), then translate to UPN + (user@kerberos.realm). The realm name is returned in + low
Re: [HACKERS] BUG #13854: SSPI authentication failure: wrong realm name used
* Christian Ullrich wrote: * Christian Ullrich wrote: > According to the release notes, the default for the "include_realm" > option in SSPI authentication was changed from off to on in 9.5 for > > improved security. However, the authenticated user name, with the > > option enabled, includes the NetBIOS domain name, *not* the Kerberos > realm name: Below is a patch to correct this behavior. I suspect it has some serious compatibility issues, so I would appreciate feedback. Updated patch, sorry. The first one worked by accident only. -- Christian diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml new file mode 100644 index 3b2935c..7236459 *** a/doc/src/sgml/client-auth.sgml --- b/doc/src/sgml/client-auth.sgml *** omicron bryanh *** 1097,1102 --- 1097,1132 + real_realm + + + If set to 0, the domain's SAM-compatible name (also known as the + NetBIOS name) is used for the include_realm + option. This is the default. If set to 1, the true realm name from + the Kerberos user principal name is used. If you used the + include_realm option, you can leave this option + disabled to maintain compatibility with existing + pg_ident.conf files. + + + + + + upn_username + + + If this option is enabled along with real_realm, + the user name from the Kerberos UPN is used for authentication. If + it is disabled (the default), the SAM-compatible user name is used. + Note that libpq uses the SAM-compatible name if no + explicit user name is specified. If you use + libpq (e.g. through the ODBC driver), you should + leave this option disabled. + + + + + map diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c new file mode 100644 index 0131bfd..0f28f54 *** a/src/backend/libpq/auth.c --- b/src/backend/libpq/auth.c *** typedef SECURITY_STATUS *** 155,160 --- 155,165 (WINAPI * QUERY_SECURITY_CONTEXT_TOKEN_FN) ( PCtxtHandle, void **); static intpg_SSPI_recvauth(Port *port); + static intpg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +char *domainname, +size_t domainnamesize, +bool update_accountname); #endif /* *** static int *** 1026,1031 --- 1031,1037 pg_SSPI_recvauth(Port *port) { int mtype; + int status; StringInfoData buf; SECURITY_STATUS r; CredHandle sspicred; *** pg_SSPI_recvauth(Port *port) *** 1261,1266 --- 1267,1281 free(tokenuser); + if (port->hba->real_realm) { + status = pg_SSPI_make_upn(accountname, sizeof(accountname), + domainname, sizeof(domainname), + port->hba->upn_username); + if (status != STATUS_OK) { + return status; + } + } + /* * Compare realm/domain if requested. In SSPI, always compare case * insensitive. *** pg_SSPI_recvauth(Port *port) *** 1296,1301 --- 1311,1407 else return check_usermap(port->hba->usermap, port->user_name, accountname, true); } + + static intpg_SSPI_make_upn(char *accountname, +size_t accountnamesize, +char *domainname, +size_t domainnamesize, +bool update_accountname) + { + char *samname; + char *upname = NULL; + char *p = NULL; + ULONG upnamesize = 0; + size_t upnamerealmsize; + BOOLEAN res; + + /* 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. */ + + samname = psprintf("%s\\%s", domainname, accountname); + res = TranslateName(samname, NameSamCompatible, NameUserPrincipal, + NULL, &upnamesize); + + if ((!res && GetLastError() != ERROR_INSUFFICIENT_B