Thread: BUG #13854: SSPI authentication failure: wrong realm name used

The following bug has been logged on the website:

Bug reference:      13854
Logged by:          Christian Ullrich
Email address:      chris@chrullrich.net
PostgreSQL version: 9.5.0
Operating system:   Windows
Description:

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, now
includes the NetBIOS domain name, *not* the Kerberos realm name:

[chul@itdb 2016-01-08 00:31:56 CET] ([unknown]) LOG:  provided user name
(chul) and authenticated user name (chul@LOCAL-DOM) do not match
[chul@itdb 2016-01-08 00:31:56 CET] ([unknown]) FATAL:  SSPI authentication
failed for user "chul"
[chul@itdb 2016-01-08 00:31:56 CET] ([unknown]) DETAIL:  Connection matched
pg_hba.conf line 101: "host    all             all
192.168.0.1/32       sspi"

"LOCAL-DOM" is the domain short name/NetBIOS name. The realm name is
(typically, and in  this case) the domain DNS name in uppercase. The string
used for the realm name is retrieved from the LookupAccountSid() function,
which will always return the short name:

(Python 3.5 on a Windows 10 client in the same domain):
>>> from win32security import LookupAccountName, LookupAccountSid
>>> sid = LookupAccountName(None, "chul")[0]
>>> LookupAccountSid(None, sid)
('chul', 'LOCAL-DOM', 1)

Login is successful if I add "include_realm=0 krb_realm=LOCAL-DOM" to
pg_hba.conf. If I use the actual Kerberos realm name instead, I simply get
"SSPI authentication failed" with no further information in the log.

I am aware of the option of using pg_ident.conf to map authenticated user
names with realm to bare database role names, but I would have to put the
wrong realm name string in there as well, so it is not a fix.

A possible fix might be to convert the user name/domain name retrieved from
LookupAccountSid() using TranslateName()/IADsNameTranslate to get the
Kerberos UPN, which includes the actual realm name. There may be
compatibility issues with that, because the first part of the UPN need not
equal sAMAccountName (the logon user name). Apparently [1] you can also get
an explicit mapping (look up dnsRoot by nETBIOSName) from AD, but whether
that is the correct approach, I don't know.

My distribution is from
<http://get.enterprisedb.com/postgresql/postgresql-9.5.0-1-windows-x64-binaries.zip>.

[1] https://stackoverflow.com/questions/12606466

--
Christian

chris@chrullrich.net writes:
> 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, now
> includes the NetBIOS domain name, *not* the Kerberos realm name:

Is this new breakage, or did include_realm=1 fail in the same way for
your configuration in prior releases?

            regards, tom lane

KiBGcm9tOiBUb20gTGFuZSBbbWFpbHRvOnRnbEBzc3MucGdoLnBhLnVzXQ0KDQo+IGNocmlzQGNo
cnVsbHJpY2gubmV0IHdyaXRlczoNCg0KPiA+IEFjY29yZGluZyB0byB0aGUgcmVsZWFzZSBub3Rl
cywgdGhlIGRlZmF1bHQgZm9yIHRoZSAiaW5jbHVkZV9yZWFsbSINCj4gPiBvcHRpb24gaW4gU1NQ
SSBhdXRoZW50aWNhdGlvbiB3YXMgY2hhbmdlZCBmcm9tIG9mZiB0byBvbiBpbiA5LjUgZm9yDQo+
ID4gaW1wcm92ZWQgc2VjdXJpdHkuIEhvd2V2ZXIsIHRoZSBhdXRoZW50aWNhdGVkIHVzZXIgbmFt
ZSwgd2l0aCB0aGUNCj4gPiBvcHRpb24gZW5hYmxlZCwgbm93IGluY2x1ZGVzIHRoZSBOZXRCSU9T
IGRvbWFpbiBuYW1lLCAqbm90KiB0aGUNCj4gPiBLZXJiZXJvcyByZWFsbSBuYW1lOg0KDQo+IElz
IHRoaXMgbmV3IGJyZWFrYWdlLCBvciBkaWQgaW5jbHVkZV9yZWFsbT0xIGZhaWwgaW4gdGhlIHNh
bWUgd2F5IGZvcg0KPiB5b3VyIGNvbmZpZ3VyYXRpb24gaW4gcHJpb3IgcmVsZWFzZXM/DQoNCnMv
bm93IGluY2x1ZGVzL2luY2x1ZGVzLw0KDQpJIGRpZCBub3QgdXNlIHRoYXQgb3B0aW9uIGJlZm9y
ZSwgdGhlIHNhbWUgYXMgZXZlcnlvbmUgZWxzZSwgYnV0IEkgY2hlY2tlZA0KOS40LjUganVzdCBu
b3cgYW5kIGl0IGZhaWxzIGluIHRoZSBzYW1lIHdheSB0aGVyZS4gVGhlIGNvZGUgaW4gYXV0aC5j
IGhhcw0Kbm90IGNoYW5nZWQgc2lnbmlmaWNhbnRseSBzaW5jZSBpdCB3YXMgaW50cm9kdWNlZCwg
c28gSSBhc3N1bWUgdGhhdCBpdA0KaGFzIGJlaGF2ZWQgbGlrZSB0aGlzIGZyb20gdGhlIHN0YXJ0
Lg0KDQotLSANCkNocmlzdGlhbg0KDQo=

* 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:

This has been true ever since the include_realm option was added in
2009 (so presumably nobody has ever used it).

Below is a patch to correct this behavior. I suspect it has some
serious compatibility issues, so I would appreciate feedback.

The patch adds:

- a pg_hba.conf option "real_realm" (defaults to off) that uses the
   "real" realm name from the Kerberos UPN instead of the domain short
   name. It does this by converting the SAM-compatible user name that is
   the result of the SSPI authentication to its UPN equivalent, using
   domain controller knowledge in the form of the TranslateName()
   function. The option defaults to off for backward compatibility with
   existing pg_ident.conf files.

- a pg_hba.conf option "upn_username" (defaults to off) that uses
   the user name from the UPN (the part before the "@") instead of the
   SAM-compatible user name from the authentication result. It only
   applies if real_realm is enabled. This is a separate option to keep
   compatibility with libpq, which uses the SAM-compatible user name if
   none is specified, either from the USERNAME environment variable or
   from the GetUserName() function, not sure which. The SAM-compatible
   user name (AD attribute sAMAccountName) and the UPN user name
   (userPrincipalName) can be different, and there is even UI for that
   in the default AD user management tool, so I would expect that there
   are environments where this is true.

   To see the effect of this option, create an AD user where the two
   names are different and look at the "authentication failed" message
   in the server log.

- documentation for the above.

I will add the patch to the open CF shortly.


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 ----
        </varlistentry>

        <varlistentry>
+       <term><literal>real_realm</literal></term>
+       <listitem>
+        <para>
+         If set to 0, the domain's SAM-compatible name (also known as the
+         NetBIOS name) is used for the <literal>include_realm</literal>
+         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
+         <literal>include_realm</literal> option, you can leave this option
+         disabled to maintain compatibility with existing
+         <filename>pg_ident.conf</filename> files.
+        </para>
+       </listitem>
+      </varlistentry>
+
+      <varlistentry>
+       <term><literal>upn_username</literal></term>
+       <listitem>
+        <para>
+         If this option is enabled along with <literal>real_realm</literal>,
+         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 <application>libpq</> uses the SAM-compatible name if no
+         explicit user name is specified. If you use
+         <application>libpq</> (e.g. through the ODBC driver), you should
+         leave this option disabled.
+        </para>
+       </listitem>
+      </varlistentry>
+
+      <varlistentry>
         <term><literal>map</literal></term>
         <listitem>
          <para>
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
new file mode 100644
index 0131bfd..5d84c19
*** 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 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

   /*----------------------------------------------------------------
*************** 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) != STATUS_OK;
+         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 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
+        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_BUFFER) || upnamesize == 0) {
+         pfree(samname);
+         ereport(LOG,
+                 (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
+                  errmsg("could not translate name")));
+                  return STATUS_ERROR;
+     }
+
+     /* upnamesize includes the NUL. */
+     upname = (char*)malloc(upnamesize);
+
+     if (!upname) {
+         pfree(samname);
+         ereport(LOG,
+                 (errcode(ERRCODE_OUT_OF_MEMORY),
+                  errmsg("out of memory")));
+         return STATUS_ERROR;
+     }
+
+     res = TranslateName(samname, NameSamCompatible, NameUserPrincipal,
+                         upname, &upnamesize);
+
+     pfree(samname);
+     if (res) {
+         p = strrchr(upname, '@');
+     }
+
+     if (!res || p == NULL) {
+         ereport(LOG,
+                 (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
+                  errmsg("could not translate name")));
+                  return STATUS_ERROR;
+     }
+
+     /* Length of realm name after the '@', including the NUL. */
+     upnamerealmsize = upnamesize - (p - upname + 1);
+
+     /* Replace domainname with realm name. */
+     if (upnamerealmsize > domainnamesize) {
+         free(upname);
+         ereport(LOG,
+                 (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
+                  errmsg("realm name too long")));
+                  return STATUS_ERROR;
+     }
+
+     /* Length is now safe. */
+     strcpy(domainname, p+1);
+
+     /* Replace account name as well (in case UPN != SAM)? */
+     if (update_accountname) {
+         if ((p - upname + 1) > accountnamesize) {
+             free(upname);
+             ereport(LOG,
+                     (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
+                      errmsg("translated account name too long")));
+                      return STATUS_ERROR;
+         }
+
+         *p = 0;
+         strcpy(accountname, upname);
+     }
+
+     free(upname);
+     return STATUS_OK;
+ }
+
+
   #endif   /* ENABLE_SSPI */


diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
new file mode 100644
index 28f9fb5..f8defab
*** a/src/backend/libpq/hba.c
--- b/src/backend/libpq/hba.c
*************** parse_hba_line(List *line, int line_num,
*** 1287,1292 ****
--- 1287,1302 ----
           parsedline->auth_method == uaSSPI)
           parsedline->include_realm = true;

+     /*
+      * For SSPI, include_realm defaults to the SAM-compatible domain
+      * (aka NetBIOS name) and user names instead of the Kerberos
+      * principal name for compatibility.
+      */
+     if (parsedline->auth_method == uaSSPI) {
+         parsedline->real_realm = false;
+         parsedline->upn_username = false;
+     }
+
       /* Parse remaining arguments */
       while ((field = lnext(field)) != NULL)
       {
*************** parse_hba_auth_opt(char *name, char *val
*** 1570,1575 ****
--- 1580,1603 ----
           else
               hbaline->include_realm = false;
       }
+     else if (strcmp(name, "real_realm") == 0)
+     {
+         if (hbaline->auth_method != uaSSPI)
+             INVALID_AUTH_OPTION("real_realm", gettext_noop("sspi"));
+         if (strcmp(val, "1") == 0)
+             hbaline->real_realm = true;
+         else
+             hbaline->real_realm = false;
+     }
+     else if (strcmp(name, "upn_username") == 0)
+     {
+         if (hbaline->auth_method != uaSSPI)
+             INVALID_AUTH_OPTION("upn_username", gettext_noop("sspi"));
+         if (strcmp(val, "1") == 0)
+             hbaline->upn_username = true;
+         else
+             hbaline->upn_username = false;
+     }
       else if (strcmp(name, "radiusserver") == 0)
       {
           struct addrinfo *gai_result;
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
new file mode 100644
index 68a953a..9e4ad8e
*** a/src/include/libpq/hba.h
--- b/src/include/libpq/hba.h
*************** typedef struct HbaLine
*** 77,82 ****
--- 77,84 ----
       bool        clientcert;
       char       *krb_realm;
       bool        include_realm;
+     bool        real_realm;
+     bool        upn_username;
       char       *radiusserver;
       char       *radiussecret;
       char       *radiusidentifier;

--
Christian Ullrich

* From: Magnus Hagander [mailto:magnus@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 <application>libpq</> uses the SAM-compatible name if
> no
> +         explicit user name is specified. If you use
> +         <application>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

* Christian Ullrich wrote:

> * From: Magnus Hagander [mailto:magnus@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

Christian Ullrich <chris@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

On Thu, Mar 24, 2016 at 11:07 AM, Robbie Harwood <rharwood@redhat.com> wrote:
> Christian Ullrich <chris@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?

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

* From: Robbie Harwood [mailto:rharwood@redhat.com]

> Christian Ullrich <chris@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.

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

On 2016-03-24 16:35, Christian Ullrich wrote:

> * From: Robbie Harwood [mailto:rharwood@redhat.com]
>
>> Christian Ullrich <chris@chrullrich.net> 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

* From: Christian Ullrich

> * From: Robbie Harwood [mailto:rharwood@redhat.com]
>
> > Christian Ullrich <chris@chrullrich.net> 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

Christian Ullrich <chris@chrullrich.net> writes:

> Updated patch attached.

Okay, I am happy now.  Thanks!

Christian Ullrich wrote:
> * Christian Ullrich wrote:
> 
> >* From: Magnus Hagander [mailto:magnus@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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

* From: Alvaro Herrera [mailto:alvherre@2ndquadrant.com]

> Christian Ullrich wrote:

> > * Christian Ullrich wrote:
> >
> > >* From: Magnus Hagander [mailto:magnus@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
nearlyevery error check in pg_SSPI_recvauth() ends up doing ereport(ERROR) already, directly or via pg_SSPI_error(). If
thiscould 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
memoryreset?
 

-- 
Christian

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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
david@pgmasters.net

* Magnus Hagander wrote:

> On Tue, Mar 29, 2016 at 5:09 PM, David Steele <david@pgmasters.net> 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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> 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

Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> 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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> 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

* Magnus Hagander wrote:

> On Tue, Mar 29, 2016 at 5:09 PM, David Steele <david@pgmasters.net> 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

<p dir="ltr"><br /> On Apr 7, 2016 9:14 PM, "Christian Ullrich" <<a
href="mailto:chris@chrullrich.net">chris@chrullrich.net</a>>wrote:<br /> ><br /> > * Magnus Hagander wrote:<br
/>><br /> >> On Tue, Mar 29, 2016 at 5:09 PM, David Steele <<a
href="mailto:david@pgmasters.net">david@pgmasters.net</a>>wrote:<br /> ><br /> ><br /> >>> It seems
likethis patch should be set "ready for committer".  Can one of<br /> >>> the reviewers do that if
appropriate?<br/> ><br /> ><br /> >> I'll pick it up to do that as well as committing it.<br /> ><br />
><br/> > Magnus, do you intend to commit the patch before the feature freeze?<br /> ><br /><p dir="ltr">It's
onmy list of things to work on this weekend, yeah. <p dir="ltr">/Magnus  

Magnus Hagander <magnus@hagander.net> writes:
> On Apr 7, 2016 9:14 PM, "Christian Ullrich" <chris@chrullrich.net> 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

<p dir="ltr"><br /> On Apr 8, 2016 1:13 AM, "Tom Lane" <<a href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>
wrote:<br/> ><br /> > Magnus Hagander <<a href="mailto:magnus@hagander.net">magnus@hagander.net</a>>
writes:<br/> > > On Apr 7, 2016 9:14 PM, "Christian Ullrich" <<a
href="mailto:chris@chrullrich.net">chris@chrullrich.net</a>>wrote:<br /> > >> Magnus, do you intend to
committhe patch before the feature freeze?<br /> ><br /> > > It's on my list of things to work on this
weekend,yeah.<br /> ><br /> > But the stated feature freeze deadline is tomorrow (Friday), not the<br /> >
weekendor later.<br /> ><br /> > To the extent that this can be called a bug fix, it might be exempt<br /> >
fromfeature freeze, but I'm not on the RMT so I'm not going to make<br /> > that call.<br /> ><p dir="ltr">Oh,
dang,I had put it down as Sunday in my calendar :S<p dir="ltr">I'll have to see what dish the travel-gods hand out
today,and try to get it done before. <p dir="ltr">/Magnus <br />