363987 - crlutil does not change thisUpdate date when creating a modified CRL

Status: RESOLVED FIXED

(NSS :: Tools, defect, P1)

Description

[Alexei Volkov]

The date should be reset to PR_Now. User should use "update <date>" crl generation script command to set a specific thisUpdate date.

crl generation tests needed to be modified to explicitly use "update" command to Zulu and local time differences.

Comment 1

[Alexei Volkov]

Attachment: reset time of modified crl

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Alexei, this patch (attachment 248767 [details] [diff] [review]) patches two files.  The name of one of
the two files begins with ../ which really confuses bugzilla's attachment.cgi 
patch diffing tool.  That makes it difficult for me to review this patch.
So please submit a new version of your patch where the file names all start
in mozilla/security and don't have any ../ in them.  Thanks.

I need to understand some more about crlutil in order to properly review this.
I hope to ask you some questions about it, face to face.

I'd also like you to explain the remark about Zulu and local time differences
and its impact on this issue. 

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 248767 [details] [diff] [review]
reset time of modified crl

r= only because of this:
>Index: ../cmd/crlutil/crlutil.c
which prevents me from doing more review.
Out of curiosity, how did you create that?
Did you do "cvs diff -u ../cmd/crlutil/crlutil.c" ?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #2) where I wrote:
> I need to understand some more about crlutil in order to properly review this.

I got most of my answers in the crlutil documentation page at 
   http://www.mozilla.org/projects/security/pki/nss/tools/crlutil.html
Good documentation!  

Comment 5

[Alexei Volkov]

> >Index: ../cmd/crlutil/crlutil.c
> which prevents me from doing more review.
> Out of curiosity, how did you create that?
> Did you do "cvs diff -u ../cmd/crlutil/crlutil.c" ?

Yes, this is exactly what I used. I always use relative names
when do diffs.
Apparently cvs can not resolve to full name relatively to cvs ws
if ".." are used in a file/dir path.

Comment 6

[Alexei Volkov]

Attachment: corrected diff

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 249022 [details] [diff] [review]
corrected diff

r=nelson

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 249022 [details] [diff] [review]
corrected diff

Asking Neil for second review for the branch.

Comment 9

[Neil Williams]

Comment on attachment 249022 [details] [diff] [review]
corrected diff

Looks good. You might want to add a note about your face-to-face with Nelson so we know what the Zulu comment referred to.

Comment 10

[Alexei Volkov]

/cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v  <--  crlutil.c
new revision: 1.29; previous revision: 1.28
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.37; previous revision: 1.36

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

The fix was only commmitted on the trunk, not the branch, so this bug
is not yet resolved/fixed for the target release.

Comment 12

[Alexei Volkov]

Committed on the 3.11 branch:

/cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v  <--  crlutil.c
new revision: 1.26.24.2; previous revision: 1.26.24.1
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.28.2.7; previous revision: 1.28.2.6