Thread: small patch to crypt.c

Hello,

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.

index f01d904..8d809b2 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -145,9 +145,7 @@ 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)
-                       retval = STATUS_OK;
-               else if (vuntil < GetCurrentTimestamp())
+               if (vuntil < GetCurrentTimestamp())                        retval = STATUS_ERROR;                else
                    retval = STATUS_OK;
 

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
>

* 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

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