Bug 473860
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.
Created attachment 327827 [details]
This version handles leap years.
It passes RHEL's rhts-based suite.
Created attachment 327828 [details]
Fixed white space.
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.
can we wait a bugfix? imho it's a high priority, urgent bug. simple currently certwatch is not usable! 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
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
...
------------------------------------------------
and i've got this error which is obviously not valid:
------------------------------------------------
/usr/bin/certwatch --address lfarkas lenux-ca.crt
To: lfarkas
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)
------------------------------------------------
(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. Created attachment 329440 [details]
the ca which fails
here is what you request.
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.
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? 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.
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. (In reply to comment #11) Forgot to divide by (3600 *24) 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.
(In reply to comment #14) It's simple indeed and my tests are passing. Thank you. Good riddance to the exploding times version :-) Created attachment 329670 [details]
Much simpler fix for PRTime diff calculation.
Comment on attachment 329670 [details]
Much simpler fix for PRTime diff calculation.
Obsoleting, looks like I attached the old patch
Created attachment 329672 [details]
Fix with simpler difftime calculations for Fedora-10.
koji build? (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. Created attachment 329740 [details]
simple test to demostrate the off-by-one day problem with time-diff-days
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; } 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. It was faulty test and both this proposal and the simplified version of it work just fine. 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.
/* 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. 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. That sounds good. Created attachment 330029 [details]
Fix for review. With brief yet accurate comments :-)
Requesting Joe Orton's review. You shouldn't require my review before checking in, you wrote all this code! But, patch in comment 29 looks fine to me. 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. submitted crypto-utils-2.4.1-10 to the updates-testing repository. 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 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 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. |
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.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.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.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.