Closed Bug 217024 Opened 17 years ago Closed 15 years ago

NSS should provide a function for comparing validity

Categories

(NSS :: Libraries, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

Attachments

(4 files, 3 obsolete files)

This function, which compares the validity dates of two certs, is useful for
applications, and should be exported from nss3 .
Attached patch export CERT_IsNewer (obsolete) — Splinter Review
Attachment #130241 - Flags: review?(wtc)
The logic in CERT_IsNewer looks reasonable.  It is
hardcoded to use the current time (PR_Now()) to
determine if the certs have expired.  Not sure if
we really need the flexibility to ask whether Cert A
is newer than Cert B at a time instant other than
the current time.

Nelson, Bob, any objection to exporting this function?
CERT_IsNewer should either take a time as wtc meantions, or it should remove the
time based checks. 

Currently this function is only used by the Stan wrapper code.
(pki3hack.c -> used in nss3certificate_isNewerThan(), which is used in the
'virtual function' isNewerThan() which is called in pkibase.c pkistore.c and
tdcache.c

bob
Looking at the code, the CERT_IsNewer test that uses a time as part of its input
is somewhat questionable .

It is used in certain cases with overlapping periods. The following is a text
representation of certs validity periods, on a horizontal time axis.

AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA

       BBBBBBBBBBBBBBB

              t1              t2

According to the current algorithm :
At time t1 :
B is newer than A

At time t2 :
A is newer than B

It is certainly non-intuitive that the "newness" change as a function of the
time the test is done.

Most likely the application would be comparing unrelated certs in the above case.
But this edge case can still happen for legitimate purposes, for example if a
cert was revoked, and the CA changed its policy to a shorter period, so the
newer cert expires sooner.

I think the biggest problem here is that we need to define of "newness".

All we have in cert A and cert B is a validity period, with NotBefore and
NotAfter times. There is no issuing date, so we don't know when each cert was
actually issued. If that's the question that CERT_IsNewer is trying to answer,
then we should delete the function, because that's one nobody can answer.

I have been trying to formulate a definition of "newness" that would match what
we have in the code in this function, but so far I can't. Only the first block
of code makes sense - the one that says basically "cert A is newer than cert B
if it is valid from a date later than that of cert B, and expires after cert B".
Unfortunately there are two conditions in that sentence, and they aren't always
both true, hence this discussion.

The other problem is with the prototype of the function. If the same
CERTCertificate is passed in as a and b, which one is newer ? There is no right
answer for that case.

Looking at the existing usages of this function that Bob mentions :
1) in pkibase.c, it is used to choose the "best" certificate at a particular
time and for a particular usage. A time is passed in to this function.
However, the caller only compares 2 valid certs or 2 invalid certs, but never a
valid cert against an invalid cert, so the time shouldn't be a factor for
CERT_IsNewer, ie. the determination will not be made because one cert is expired
and the other is not

2) in tdcache.c and pkistore.c, CERT_IsNewer is used to sort a subject list . 
The caller doesn't pass a time, so the result should probably not be a function
of time. Using the current time is arbitrary. I'm not sure if it is ever correct.
Hmm, The algorithm that Julian described makes perfect sense to me. Absent an
'issue date', the 'valid date' is the best indication of whether the cert is
newer or not.

The confusion comes when you and the requirement of 'valid' cert. It looks like
currently the function returns the newest 'valid' cert, which makes since for a
simple loop. It may be more confusing as a separately exported function.

I also think the usage in the subject list needs to be fixed. It's clear the
subject list needs to sort by time.

Probably the best solution is to do a strictly 'isNewer' test and a separate is
valid test, much the way the tdcache code works today.

bob
Target Milestone: --- → 3.9
Priority: -- → P2
Target Milestone: 3.9 → 3.10
Going back to this old issue, I agree with Bob that it is confusing for an
exported function to return the newest "valid" cert at the current time.

Having validity handled as a separate test is preferable. Applications that need
to select a certificate to use should first filter for valid certs, before
trying to find the most curent.

The bottom line is that we should not export this function as it is currently
implemented and named.

I suggest we implement a different function,
CERT_CompareCertValidityTimes(CERTCertificate* a, CERTCertificate* b)

It would not depend on the current time, and would return 1 if a is newer, 0 if
the validity periods of and b are identical, and -1 if b is newer.
Severity: normal → enhancement
Sounds like a useful to export function to me...

bob
Summary: CERT_IsNewer should be exported → NSS should provide a function for comparing validity
Attachment #130241 - Flags: review?(wchang0222)
I have been working on this to try to clean this long standing open bug and
implement the new function I suggested.

The main issue is to have this function perform a comparison operation that will
be useful to applications, and to clearly state what that comparison operation
is by properly naming it.

As I already commented, I want it to return one of 3 main values - a is newer, a
= b, and b is newer (or later, if you want to be strict about it). I will
probably add another value for invalid cases (eg. validity won't decode,
notBefore > notAfter in one of the certs). These should be enum's to clearly
explain what they mean.

I'm thinking that the comparison function should always choose the cert that
extends the latest into the future if possible. Ie. if notAfter differs between
the two certs, then the one with the latest notAfter wins, and that takes care
of 6 of the 9 comparison cases.

If however the 2 notAfter are equal, this leaves a few possibilities.
1) easiest case - notBefore is also equal - then choose neither, return equal
2) notBefore differs .

Then, the function could choose either the cert with the longest validity, ie.
with the oldest notBefore ; or with the newest notBefore, because, well, it's
newer ... I don't know which one is more useful to applications. But either way,
I think we may need a function name to explain it, or if we keep my fairly
non-descriptive function name I suggested from comment #6, we would need good
names for the enum values .

Another option is simply to have additional return values for the ambiguous
cases. I just don't know if the applications need or want this much precision. I
would tend to probably say yes.

Patch forthcoming which will probably explain the dilemma much better than this
comment ...

I know the enum def needs to go in certt.h, and the enum names probably need
work.
What I'm mostly concerned about here is the comparison algorithm, though.
Comments welcome.
Attachment #130241 - Attachment is obsolete: true
Attachment #152086 - Flags: review?(rrelyea)
Target Milestone: 3.10 → 3.11
Comment on attachment 152086 [details] [diff] [review]
new function to compare validity. needs work

Just reviewing the logic:

1) Sanity checks: OK, but it might be better to also check for equality (make
the checks != ValidityChooseB)unless we think it's valid to issue certs for a
zero length validity period. 

2) Your if statement can be reduced to:

if (afterStatus != ValidityEqual) {
    return afterStatus;
}

return beforeStatus.

I believe this is your intention (return the cert that is valid the latest, if
both certs expire on the same day, return the the one that was issued later).

The simplest code in this case would be:

return afterStatus == ValidityEqual ? beforeStatus : afterStatus;
QA Contact: bishakhabanerjee → jason.m.reid
Comment on attachment 152086 [details] [diff] [review]
new function to compare validity. needs work

logic review already given in comment.
Attachment #152086 - Flags: review?(rrelyea)
Bob,

1) I haven't seen any spec that said notBefore and notAfter must be different.
If you have, I will be happy to add a sanity check for that.

2) I will use your first suggestion for the code simplification .
Attached patch update (obsolete) — Splinter Review
Attachment #152086 - Attachment is obsolete: true
Attachment #188369 - Flags: review?(rrelyea)
Comment on attachment 188369 [details] [diff] [review]
update

r+ looks good, 

only minor issue, if 'ValidityUndetermined' is considered an error, we should
set a PORT_Error for the 'sanity check' case (the failed DER_xxx case should
have the error set by DER_DecodeTimeChoice().

bob
Attachment #188369 - Flags: review?(rrelyea) → review+
Bob,

Thanks for the review. I set the error code in the sanity check case to
SEC_ERROR_INVALID_TIME . I also added the declarations in the header and
nss.def .

I checked this patch in to the tip for 3.11 .

Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.74; previous revision: 1.73
done
Checking in certdb/certdb.h;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.h,v  <--  certdb.h
new revision: 1.17; previous revision: 1.16
done
Checking in certdb/certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.31; previous revision: 1.30
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.150; previous revision: 1.149
done
Attachment #188369 - Attachment is obsolete: true
Marking fixed.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 188644 [details] [diff] [review]
patch as checked in

The new enumeration type does not follow our naming
convention.

>+typedef enum CERT_CompareValidityStatusType
>+{
>+    ValidityUndetermined = 0,
>+    ValidityChooseB = 1,
>+    ValidityEqual = 2,
>+    ValidityChooseA = 3,
>+} CERT_CompareValidityStatus;

We don't use _ in data type names.

The enumeration constants should begin with one of
our prefixes, probably "cert".
Comment on attachment 188644 [details] [diff] [review]
patch as checked in

Please also add comments to document the new function
and the new enumeration type.
I agree with comments 17 and 18.  NSS's convention is that 
PREFIX_Suffix  is used exclusively for function names.
PREFIXSuffix   (no underscore) is used for names of structs, unions, enums,
and all typedefs.  This new enum name and typedef should be fixed to conform
to that convention.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I checked in this patch which addresses the concerns over naming and
documentation. I also fixed a build failure on AIX by removing a comma on the
ValidityChooseA enum definition.

Checking in certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.75; previous revision: 1.74
done
Checking in certdb.h;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.h,v  <--  certdb.h
new revision: 1.18; previous revision: 1.17
done
Checking in certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.32; previous revision: 1.31
done
Marking fixed.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Comment on attachment 188739 [details] [diff] [review]
incremental patch

>+static CERTCompareValidityStatus GetNewestTime(PRTime a, PRTime b)

This function makes the code hard to understand.  Instead of the
follwoing:

    if (certValidityChooseA == GetNewestTime(notBeforeA, notAfterA) ||
	certValidityChooseA == GetNewestTime(notBeforeB, notAfterB)) {
	PORT_SetError(SEC_ERROR_INVALID_TIME);
	return certValidityUndetermined;
    }

it's much clearer to say:

    if (LL_CMP(notBeforeA,>,notAfterA) || LL_CMP(notBeforeB,>,notAfterB)) {
	PORT_SetError(SEC_ERROR_INVALID_TIME);
	return certValidityUndetermined;
    }

Instead of:

    afterStatus = GetNewestTime(notAfterA, notAfterB);
    if (afterStatus != certValidityEqual) {
	/* one cert validity goes farthest into the future, select it */
	return afterStatus;
    }

say:

    if (LL_CMP(notAfterA,!=,notAfterB)) {
	/* one cert validity goes farther into the future, select it */
	return LL_CMP(notAfterA,<,notAfterB) ? ... : ...;
    }

If you don't want to make these changes, at least change "Newest" to
"Newer" and "farthest" to "farther" because only two things are being
compared.

>+typedef enum CERTCompareValidityStatusEnum
>+    certValidityUndetermined = 0,
>+    certValidityChooseB = 1,
>+    certValidityEqual = 2,
>+    certValidityChooseA = 3
>+} CERTCompareValidityStatus;

Each enumeration constant should be documented.

certValidityNewer and certValidityOlder may be better...

But certValidityChooseA and certValidityChooseB imply we should choose
the newer cert.  In any case the documentation could be improved to
spell these out.
Wan-Teh,

It would have been great if you would have made the remarks on the
GetNewestTime function yesterday. With 2 review comments, I thought you that
was your complete review. I'm fine with LL_CMP changes and included them in
this patch.

Regarding the enum names (certValidityChooseA, certValidityChooseB), the reason
I didn't spell out "newerA" or "newerB", or that I didn't call the function
something like the existing "CERT_IsNewer" is that there is nothing obvious
about the selection algorithm that can be described in one word like "newer",
and you may notice notice the algorithm took 2 years to agree on . It is
conceivable to me that a different selection algorithm may be written at a
later point, for which this same enum definition could be reused. So, I would
rather not change the enum value names to something too specific. But I have
added some comments about each value.

You are right in that there should be some more documentation about the
selection algorithm, but IMO that would belong with the function. That's where
I added the comments about how the selection is made.
Attachment #188745 - Flags: review?(wtchang)
Reopening bug due to comment #22 . Patch is pending Wan-Teh's review.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 188745 [details] [diff] [review]
Wan-Teh's latest suggestions, and doc update

r=wtc.	Thanks a lot for the nice comments that
document the function and the enum constants.

Please make the following changes before you check this
in. I marked these suggestions with priorities P1-P3:
P1=most important, P3=least important.	The only P1 is
item #3; at least fix that.

1. P3: change "farthest" to "farther" and "latest" to "later"
and "newest" (two instances) to "newer" because we are comparing
only two things.

2. P3: This assertion in the CERT_CompareValidityTimes function
may be deleted because it's obvious it has to be true in the new
code:

>+    if (LL_CMP(notAfterA,!=,notAfterB)) {
>         /* one cert validity goes farthest into the future, select it */
>+        return LL_CMP(notAfterA,<,notAfterB) ?
>+            certValidityChooseB : certValidityChooseA;
>     }
>     /* the two certs have the same expiration date */
>     PORT_Assert(LL_CMP(notAfterA, == , notAfterB));

3. P1: at the end of the CERT_CompareValidityTimes function, we have

>     /* choose cert with the latest start date */
>+    return LL_CMP(notBeforeA,<,notBeforeB) ?
>+        certValidityChooseB : certValidityChooseA;
> }

This is not complete, because now this function never returns
certValidityEqual.  You can fix this by adding the following
before that return statement.

    if (LL_CMP(notBeforeA,==,notBeforeB)) {
	return certValidityEqual;
    }

or use if-else-if-else like the removed GetNewestTime function.

4. P2: In certt.h, we have

>+ * This is used with as return status in functions that compare the validity

Remove the "with" in "used with as".

5. P3: In certt.h, we have

>+    certValidityEqual = 2,        /* both certs have the same validity  */

Seems better to add "period" after "validity" in the comment.
Attachment #188745 - Flags: review?(wtchang) → review+
Attachment #192883 - Flags: review?(wtchang)
Comment on attachment 192883 [details] [diff] [review]
Fixes from latest feedback from Wan-Teh

r=wtc.
Attachment #192883 - Flags: review?(wtchang) → review+
Thanks, Wan-Teh. I checked this on the tip.

Checking in certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.76; previous revision: 1.75
done
Checking in certdb/certdb.h;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.h,v  <--  certdb.h
new revision: 1.19; previous revision: 1.18
done
Checking in certdb/certt.h;
/cvsroot/mozilla/security/nss/lib/certdb/certt.h,v  <--  certt.h
new revision: 1.33; previous revision: 1.32
done
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.