small patch to crypt.c

JD,

* Joshua D. Drake (jd@commandprompt.com) wrote:

In my quest to understand how all the logging etc works with
authentication I came across the area of crypt.c that checks for
valid_until but it seems like it has an extraneous check.

If I am wrong I apologize for the noise but wouldn't mind an explanation.

Alright, there probably aren't too many people out there running with
their clock set to pre-2000, but wouldn't this end up giving the wrong
result in those cases, as GetCurrentTimestamp() would end up returning a
negative value, which would make it less than vuntil's default of zero?

Perhaps we could change what vuntil is set to by default, but I think
it's probably better to keep things as-is; we should really be checking
for null cases explicitly in general.

Thanks,

Stephen

On 06/08/2013 08:47 PM, Stephen Frost wrote:

JD,

* Joshua D. Drake (jd@commandprompt.com) wrote:

In my quest to understand how all the logging etc works with
authentication I came across the area of crypt.c that checks for
valid_until but it seems like it has an extraneous check.

If I am wrong I apologize for the noise but wouldn't mind an explanation.

Alright, there probably aren't too many people out there running with
their clock set to pre-2000, but wouldn't this end up giving the wrong
result in those cases, as GetCurrentTimestamp() would end up returning a
negative value, which would make it less than vuntil's default of zero?

Perhaps we could change what vuntil is set to by default, but I think
it's probably better to keep things as-is; we should really be checking
for null cases explicitly in general.

Well I was more referring to the default is:

check if null, if true return ok
check if valuntil < today, if true return error
else return ok

To me we don't need the null check. However, when I tested it, without
the null check you can't login. So now I am curious about what is going on.

JD

Thanks,

Stephen

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Joshua D. Drake (jd@commandprompt.com) wrote:

Well I was more referring to the default is:

check if null, if true return ok
check if valuntil < today, if true return error
else return ok

To me we don't need the null check. However, when I tested it,
without the null check you can't login. So now I am curious about
what is going on.

Erm, but what is valuntil set to when it's null? I'd expect it to be
zero because it hasn't been changed since:

TimestampTz vuntil = 0;

Using your pseudo-code, you end up with:

check if 0 < today, if true return error
else return ok

Which is why you end up always getting an error when you get rid of the
explicit isnull check. Looking at it too quickly, I had assumed that
the test was inverted and that your patch worked most of the time but
didn't account for GetCurrentTimestamp() going negative.

Regardless, setting vuntil to some magic value that really means "it's
actually NULL", which is what you'd need to do in order to get rid of
that explicit check for null, doesn't strike me as a good idea. When a
value is null, we shouldn't be looking at the data at all.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

Regardless, setting vuntil to some magic value that really means "it's
actually NULL", which is what you'd need to do in order to get rid of
that explicit check for null, doesn't strike me as a good idea. When a
value is null, we shouldn't be looking at the data at all.

Even aside from that, the proposed change seems like a bad idea because
it introduces an unnecessary call of GetCurrentTimestamp() in the common
case where there's no valuntil limit. On some platforms that call is
pretty slow.

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

On 06/09/2013 09:28 AM, Tom Lane wrote:

Even aside from that, the proposed change seems like a bad idea because
it introduces an unnecessary call of GetCurrentTimestamp() in the common
case where there's no valuntil limit. On some platforms that call is
pretty slow.

And that would explain why we don't do something like this:

index f01d904..4935c9f 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -145,12 +145,10 @@ md5_crypt_verify(const Port *port, const char 
*role, char *client_pass)
                 /*
                  * Password OK, now check to be sure we are not past 
rolvaliduntil
                  */
-               if (isnull)
+               if (isnull || vuntil > GetCurrentTimestamp())
                         retval = STATUS_OK;
-               else if (vuntil < GetCurrentTimestamp())
-                       retval = STATUS_ERROR;
                 else
-                       retval = STATUS_OK;
+                       retval = STATUS_ERROR;
         }

Right. Ty for the feedback, I know it was just a little bit of code but
it just looked off and I appreciate the explanation.

JD

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers