Bug 473860 - certwatch does not warn of expiring certificates
certwatch does not warn of expiring certificates
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: crypto-utils (Show other bugs)
10
All Linux
urgent Severity urgent
: ---
: ---
Assigned To: Joe Orton
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-30 23:48 EST by Andrew Jones
Modified: 2009-03-16 15:43 EDT (History)
5 users (show)

See Also:
Fixed In Version: 2.4.1-16
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-02-18 16:26:45 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
proposed fix to certwatch to detect expiring certificates (7.84 KB, patch)
2008-12-23 17:03 EST, Elio Maldonado Batiz
no flags Details | Diff
This version handles leap years. (3.24 KB, patch)
2008-12-24 16:23 EST, Elio Maldonado Batiz
no flags Details | Diff
Fixed white space. (5.25 KB, patch)
2008-12-24 16:35 EST, Elio Maldonado Batiz
no flags Details | Diff
certwatch 473860 fix plus genkey fix for F-10 (6.52 KB, patch)
2009-01-05 12:36 EST, Elio Maldonado Batiz
no flags Details | Diff
the ca which fails (1.83 KB, text/plain)
2009-01-20 03:47 EST, Levente Farkas
no flags Details
Fix for certificate expiry time calculations (11.97 KB, patch)
2009-01-21 13:05 EST, Elio Maldonado Batiz
no flags Details | Diff
Much simpler fix for PRTime diff calculation. (11.97 KB, patch)
2009-01-21 18:07 EST, Elio Maldonado Batiz
no flags Details | Diff
Fix with simpler difftime calculations for Fedora-10. (10.71 KB, patch)
2009-01-21 18:15 EST, Elio Maldonado Batiz
no flags Details | Diff
simple test to demostrate the off-by-one day problem with time-diff-days (693 bytes, text/plain)
2009-01-22 16:23 EST, Elio Maldonado Batiz
no flags Details
Fix for review. (10.79 KB, patch)
2009-01-26 13:05 EST, Elio Maldonado Batiz
no flags Details | Diff
Fix for review. With brief yet accurate comments :-) (10.80 KB, application/octet-stream)
2009-01-26 16:39 EST, Elio Maldonado Batiz
no flags Details

  None (edit)
Description Andrew Jones 2008-11-30 23:48:03 EST
Description of problem:
certwatch worked fine in Fedora 9, but in Fedora 10, it does not notice that a certificate is about to expire.


Version-Release number of selected component (if applicable):
2.4.1-2


How reproducible:
certwatch expiring-certificate.pem


Steps to Reproduce:
Here an example from my system:
[root@server certs]# date
Mon Dec  1 05:44:50 CET 2008
[root@server certs]# openssl x509 -in 43.pem -text
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 67 (0x43)
        Signature Algorithm: sha1WithRSAEncryption
        Issuer: C=DE, ST=Baden-Wuerttemberg, L=Mannheim, O=Andrew Jones, CN=Root CA/emailAddress=root@simultan.dyndns.org
        Validity
            Not Before: Dec  9 18:48:22 2006 GMT
            Not After : Dec  8 18:48:22 2008 GMT
        Subject: C=DE, ST=Baden-Wuerttemberg, L=Mannheim, O=Andrew Jones, CN=submit.pipasoft.com/emailAddress=root@simultan.dyndns.org
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
            RSA Public Key: (4096 bit)
                Modulus (4096 bit):
[...]
                Exponent: 65537 (0x10001)
        X509v3 extensions:
            X509v3 Basic Constraints: 
                CA:FALSE
            X509v3 Subject Key Identifier: 
                F8:C0:14:5F:B0:44:EC:6B:C4:7C:57:6B:89:7C:25:34:CF:D9:AF:9A
            X509v3 Authority Key Identifier: 
                keyid:D8:0E:6F:7F:03:61:CE:FE:3A:CA:E4:22:80:3C:9C:50:72:EC:3B:63
                DirName:/C=DE/ST=Baden-Wuerttemberg/L=Mannheim/O=Andrew Jones/CN=Root CA/emailAddress=root@simultan.dyndns.org
                serial:E8:2F:3D:92:20:7C:39:B0

            X509v3 Subject Alternative Name: 
                DNS:submit.pipasoft.com, DNS:simultan.dyndns.org
            X509v3 CRL Distribution Points: 
                URI:http://simultan.dyndns.org/PKI/ca.crl
                URI:http://server.pipasoft.com/PKI/ca.crl

    Signature Algorithm: sha1WithRSAEncryption
[...]
-----BEGIN CERTIFICATE-----
[...]
-----END CERTIFICATE-----
[root@server certs]# certwatch 43.pem 
[root@server certs]# echo $?
1
[root@server certs]# 
  
Actual results:
No warning, return code 1.


Expected results:
Warning mail to stdout, return code 0.
Comment 1 Elio Maldonado Batiz 2008-12-23 17:03:41 EST
Created attachment 327775 [details]
proposed fix to certwatch to detect expiring certificates

Replaces the faulty time arithmetic based on LL macros with PRTime. The warning function now uses the validity parameter to discriminate among various cases. For the secCertTimeValid case it further distinguishes among expired and about to expired certificates. days_from_today computes the days left until a certificate expires. Added a a conditionally compiled tracing function, activated via -DTRACE at compile time. Added certificates to the testing directory and cert watching test targets to the makefile.
Comment 2 Elio Maldonado Batiz 2008-12-24 16:23:10 EST
Created attachment 327827 [details]
This version handles leap years.

It passes RHEL's rhts-based suite.
Comment 3 Elio Maldonado Batiz 2008-12-24 16:35:01 EST
Created attachment 327828 [details]
Fixed white space.
Comment 4 Elio Maldonado Batiz 2009-01-05 12:36:05 EST
Created attachment 328217 [details]
certwatch 473860 fix plus genkey fix for F-10

This attachment consolidates certwatch 473680 fix with a genkey fix to server and ca key file name extension. brings to F-10 fixes already applied in the development branch.
Comment 5 Levente Farkas 2009-01-19 07:49:34 EST
can we wait a bugfix?
imho it's a high priority, urgent bug. simple currently certwatch is not usable!
Comment 6 Levente Farkas 2009-01-19 07:58:22 EST
the situation is even worst. the latest koji build crypto-utils-2.4.1-6.i386.rpm generate warning for my own root ca which is rather valid:
------------------------------------------------
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number: 0 (0x0)
        Signature Algorithm: sha1WithRSAEncryption
        Issuer: C=HU, L=Budapest, O=Lenux Bt., CN=Lenux Root CA/emailAddress=help@lenux.hu
        Validity
            Not Before: Jun  5 17:23:11 2006 GMT
            Not After : Jun  2 17:23:11 2016 GMT
        Subject: C=HU, L=Budapest, O=Lenux Bt., CN=Lenux Root CA/emailAddress=help@lenux.hu
...
------------------------------------------------
and i've got this error which is obviously not valid:
------------------------------------------------
/usr/bin/certwatch --address lfarkas@lfarkas.org lenux-ca.crt 
To: lfarkas@lfarkas.org
Subject: The certificate for Lenux Root CA will expire today

 ################# SSL Certificate Warning ################

  Certificate for hostname 'Lenux Root CA', in file (or by nickname):
     lenux-ca.crt

  The certificate needs to be renewed; this can be done
  using the 'genkey' program.

  Browsers will not be able to correctly connect to this
  web site using SSL until the certificate is renewed.

 ##########################################################
                                  Generated by certwatch(1)
------------------------------------------------
Comment 7 Elio Maldonado Batiz 2009-01-19 19:26:24 EST
(In reply to comment #6)
While attempting the fix I created several certificates with different expiration dates while testing the fix but maybe there is something I missed. Could you attach a copy of the cert in comment #6 to the bug? Thanks.
Comment 8 Levente Farkas 2009-01-20 03:47:25 EST
Created attachment 329440 [details]
the ca which fails

here is what you request.
Comment 9 Elio Maldonado Batiz 2009-01-21 13:05:54 EST
Created attachment 329626 [details]
Fix for certificate expiry time calculations

Submitting this patch for review. Contrary to what the NSPR headers suggest the PRTime is not quite the same as time_t as far as as time arithmetic goes so earlier attempts to compute time differences using the NSPR LL macros failed. I had to resort to recursively computing using exploded times.
Comment 10 Levente Farkas 2009-01-21 13:14:17 EST
are you _absolutely_ sure that this nss lib changes was a clever move? i'm just look int this patch and if you have to calculate leap_year yourself i'm absolutely sure this is a bad lib! come on! this must be in some common libs now eg. glibc:-) if it's true it's also and upstream bug!
anyway is there any koji build which i can test?
Comment 11 Elio Maldonado Batiz 2009-01-21 15:44:04 EST
Granted, I shouldn't be calculating leap_year if glibc or other library does it for me but more serious is having to do the time difference via exploded times to begin with. My first attempt to just cast the PRTime to time_t and take the difference didn't work. I then tried using the LL macros like this
static int diff_time_days_LL(PRTime aT, PRTime bT)  {
    int days;
    if (LL_EQ(aT, bT)) return 0;
    if (LL_CMP(aT, >, bT)) {
        PRTime diffTime;
    	LL_SUB(diffTime, aT, bT);
    	LL_L2I(days, diffTime);
    }
    return days;
}
but that doesn't work either. That's why I resorted to the current method.
I'll be glad to fire up a Koji build in Rawhide for you to try but let's wait a bit for more comments from upstream folks.
Comment 12 Levente Farkas 2009-01-21 17:19:42 EST
it's doesn't matter which library to use the only thing if nss use stupid structure for time then at least it's to provide supporting funcs too.
the real question here:
does nss really a good and even better library then openssl?
i don't know the answer and i also think 3,4 or 5 crypto libs are too luxury and it's be better to reduce it to only one, but from this simple task like porting certwatch to nss which has so much problem i become pessimistic.
Comment 13 Elio Maldonado Batiz 2009-01-21 17:38:38 EST
(In reply to comment #11) Forgot to divide by (3600 *24)
Comment 14 Wan-Teh Chang 2009-01-21 17:41:19 EST
It's no longer necessary to use the LL macros.
You can use PRTime as a regular integer type.

To convert the difference between two PRTime's
to days, just recall that the time unit of
PRTime is microseconds.  So you first divide
the difference by PR_USEC_PER_SEC (1000000UL)
to get the difference in seconds, and then
divide by 86400 to get the difference in days.

static int diff_time_days(PRTime aT, PRTime bT)
{
    PRInt64 secs = (aT - bT) / PR_USEC_PER_SEC;
    return secs / 86400L;
}

It's very simple, no need to deal with leap years.
Comment 15 Elio Maldonado Batiz 2009-01-21 18:02:57 EST
(In reply to comment #14) It's simple indeed and my tests are passing. Thank you. Good riddance to the exploding times version :-)
Comment 16 Elio Maldonado Batiz 2009-01-21 18:07:04 EST
Created attachment 329670 [details]
Much simpler fix for PRTime diff calculation.
Comment 17 Elio Maldonado Batiz 2009-01-21 18:10:12 EST
Comment on attachment 329670 [details]
Much simpler fix for PRTime diff calculation.

Obsoleting, looks like I attached the old patch
Comment 18 Elio Maldonado Batiz 2009-01-21 18:15:50 EST
Created attachment 329672 [details]
Fix with simpler difftime calculations for Fedora-10.
Comment 19 Levente Farkas 2009-01-22 03:59:03 EST
koji build?
Comment 20 Elio Maldonado Batiz 2009-01-22 12:57:40 EST
(In reply to comment #19)
A Koji build it should be avilable from Rawhide tagged crypto-utils-2_4_1-7.
This is nor ready for Fedora-10.  The expiry notifications are off by one day with the simpler calculation.  Expiration warnings are being issued a day earlier.
For example: generated a cert that expires in 30 days.
# Test 12 got: "To: root\nSubject: The certificate for www.example.com will expire in 29 days\n\n 
 .....
Generated by certwatch(1)\n\n" (certwatch.t at line 39)
#    Expected: ""
#  certwatch.t line 39 is: ok `$certwatch certw.30d`, '';
The error does not occur with the recursive calculation.
Comment 21 Elio Maldonado Batiz 2009-01-22 16:23:21 EST
Created attachment 329740 [details]
simple test to demostrate the off-by-one day problem with time-diff-days
Comment 22 Wan-Teh Chang 2009-01-22 17:32:18 EST
Elio,

You're in the best position to figure out the
off-by-one problem.  PRTime is very simple:
it is a 64-bit signed integer representing
the number of microseconds since the epoch
(1970-01-01 00:00:00 UTC):
https://developer.mozilla.org/en/PRTime

I suspect that the off-by-one problem is
caused by not ignoring the excess over a
multiple of days in each PRTime timestamp.
I base this on the fact that your original
code only uses the tm_year and tm_yday fields
of PRExplodedTime, ignoring tm_hour, tm_min,
tm_sec, and tm_usec.

You can try something like this:

static int diff_time_days(PRTime aT, PRTime bT)
{
    PRInt64 aDays = aT;
    PRInt64 bDays = bT;
    aDays /= PR_USEC_PER_SEC;
    aDays /= 86400L;
    bDays /= PR_USEC_PER_SEC;
    bDays /= 86400L;
    return aDays - bDays;
}
Comment 23 Elio Maldonado Batiz 2009-01-23 15:07:40 EST
Wan-teh,  My old recursive with exploded times solution actually worked because there are no division expect for a module op.  The off by one problem with the earlier PRint64 based version was that taking a difference and before the division cause a truncation.  Your current suggestion avoids it by dividing first. Before I saw this posting I had tried a somewhat simpler version of this divide-first method which worked and all my tests passed.  But not quite, that was on a 64-bit system. On a 32-bit system the 0 days-left test failed. Need to investigate, tweak, and retest some more. Thanks.
Comment 24 Elio Maldonado Batiz 2009-01-26 12:48:51 EST
It was faulty test and both this proposal and the simplified version of it work just fine.
Comment 25 Elio Maldonado Batiz 2009-01-26 13:05:48 EST
Created attachment 330011 [details]
Fix for review.

In this version diff_time_days performs the division first to prevent truncation errors. Submitting this one for review.
Comment 26 Wan-Teh Chang 2009-01-26 13:49:43 EST
/* Must divide before substracting to prevent truncation */

This comment isn't entirely accurate.  I think you
actually want to ensure truncation -- except that
you want truncation to occur before subtraction
because of the way you define the difference between
the timestamps in days.
Comment 27 Elio Maldonado Batiz 2009-01-26 15:09:55 EST
You are right, an inaccurate comment can be worst than none. How about?
/* Dividing before substracting to support the desired day granularity */
Trying to be brief here.
Comment 28 Wan-Teh Chang 2009-01-26 16:08:03 EST
That sounds good.
Comment 29 Elio Maldonado Batiz 2009-01-26 16:39:33 EST
Created attachment 330029 [details]
Fix for review. With brief yet accurate comments :-)
Comment 30 Elio Maldonado Batiz 2009-01-26 17:48:45 EST
Requesting Joe Orton's review.
Comment 31 Joe Orton 2009-01-29 12:22:33 EST
You shouldn't require my review before checking in, you wrote all this code!

But, patch in comment 29 looks fine to me.
Comment 32 Levente Farkas 2009-02-02 15:11:28 EST
koji build is working and i hope it's working for expired certs too:-)
you can close this bug. i'll reopen it if i find further bugs.
Comment 33 Elio Maldonado Batiz 2009-02-04 15:04:07 EST
submitted crypto-utils-2.4.1-10 to the updates-testing repository.
Comment 34 Fedora Update System 2009-02-04 21:16:43 EST
crypto-utils-2.4.1-10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update crypto-utils'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-1305
Comment 35 Fedora Update System 2009-02-26 21:53:53 EST
crypto-utils-2.4.1-16 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/crypto-utils-2.4.1-16
Comment 36 Fedora Update System 2009-03-16 15:43:43 EDT
crypto-utils-2.4.1-16 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.