3.59 KB, patch
Brian Ryner (not reading): superreview+
|Details | Diff | Splinter Review|
779 bytes, patch
|Details | Diff | Splinter Review|
The PSM file mozilla/security/manager/ssl/src/nsCrypto.cpp includes the private NSS header file crmfi.h. We should find out whether PSM really needs to include crmfi.h. If so, we should eliminate that need, which may require fixing NSS.
I'll try building without that include and we'll see what happens.
I tried. When you remove the include, compilcation fails with an error: /mozilla/security/manager/ssl/src/nsCrypto.cpp: In function `char * nsCreateReqFromKeyPairs(struct nsKeyPairInfo *, PRInt32, char *, char *, char *, class nsNSSCertificate *)': /mozilla/security/manager/ssl/src/nsCrypto.cpp:1347: aggregate `struct CRMFCertReqMessages messages' has incomplete type and cannot be initialized
You should be able to use the CRMF_EncodeCertReqMessages function to avoid the need for 'CRMFCertReqMessages messages'. You should be able to replace the SEC_ASN1EncodeItem call on 'messages' by two CRMF_EncodeCertReqMessages calls on 'certReqMsgs'. You would use the first call to get the length of the encoded output, allocate a SECItem with a buffer that size, and use the second call to write the encoded output in the SECItem. There may be other solutions.
Comment on attachment 112357 [details] [diff] [review] Proposed patch Bob, Kai, could you review this patch? Kai, could you test the code that's affected?
I forgot to mention that this patch is against Mozilla 1.3a.
Thanks for the patch. I will review and test it, but note that I'm not a superreviewer, we will need to ask one of the general superreviewers.
Comment on attachment 112357 [details] [diff] [review] Proposed patch r=kaie Patch looks fine to me. I've tested it and it compiles on Linux. However, I'm not sure which functionality I'd have to access that uses this portion of the code. Wan-Teh, do you think compiling on Linux is sufficient, or should I test on Windows and Mac, too?
Javi, I have a patch that modifies the nsCreateReqFromKeyPairs function in nsCrypto.cpp (http://lxr.mozilla.org/security/ident?i=nsCreateReqFromKeyPairs), which is used by nsCrypto::GenerateCRMFRequest. Do you know what tests we can perform to exercise that code path?
My LXR searches showed that nsCrypto::GenerateCRMFRequest is not being used by the Mozilla client. Kai, could you confirm this? This would mean we can't test the modified code using the Mozilla client.
Oops, indeed. While the interface is defined in nsIDOMCrypto.idl, I can't find any callers either, neither in JS or C++.
To test this, any page that calls crypto.generateCRMFRequest should do. I believe John Unruh's web page has such sites set up. Failing that, you could just get another cert from the Corporate CA.
Kai, could you test this patch for me? Thanks.
Wan-Teh, did you see my comment 9, where I say I have already tested that your patch compiles? Or did you mean that I should test the functionality?
John, do you know which of your test pages access crypto.generateCRMFRequest ?
Kai, I'd like you to test the functionality. Thanks.
Getting a dual key uses crypto.generateCRMFRequest https://pki.mcom.com:1025/ca/DirUserEnrollDual1024.html View the source code at that page.
http://testca.netscape.com/ will also test crypto.generateCRMFRequest
The patch seems to work for me. With the patch applied, I was successfully able to obtain certificates from both sites.
Comment on attachment 112357 [details] [diff] [review] Proposed patch Kai, thank you for testing this patch. Brian, could you review this patch? PSM is including two private NSS header files. One of them (genname.h) is just unnecessary. The other (crmfi.h) is for the definition of an opaque structure (CRMFCertReqMessages). I found that the need for the definition for that opaque type can be eliminated by using a public function (CRMF_EncodeCertReqMessages).
Comment on attachment 112357 [details] [diff] [review] Proposed patch Requesting mozilla 1.3 approval. Asa, please see comment 21 for a description of the patch. The risk of this patch is low and is confined to certificate requests. Kai has tested the patch against our test certificate authority.
Comment on attachment 112357 [details] [diff] [review] Proposed patch a=asa (on behalf of drivers) for checkin to 1.3
Patch checked in. Checking in Makefile.in; /cvsroot/mozilla/security/manager/ssl/src/Makefile.in,v <-- Makefile.in new revision: 1.54; previous revision: 1.53 done Checking in nsCrypto.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsCrypto.cpp,v <-- nsCrypto.cpp new revision: 1.31; previous revision: 1.30 done Checking in nsNSSIOLayer.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v <-- nsNSSIOLayer. cpp new revision: 1.87; previous revision: 1.86 done
Have you guys tried this on an HP-UX build? I ran into problems on hp 10.20 using aCC A.01.21 (I am trying on 11 with a newer aCC as I speak). When I build security/managers/ssl/src/nsCrypto.cpp I get the following: Error 744: "nsCrypto.cpp", line 884 # The operand of a static_cast must be a pointer to or lvalue of a complete class; the actual type was CRMFEncryptedKeyStr *. static_cast< void* >(encrKey) ); ^^^^^^^ If I go back and include crmfi.h in nsCrypto.cpp and return the -I for the nss private header directory it builds. What I think is happening is that aCC needs to know exactly what CRMFEncryptedKey is. Right now all it knows is that it is a "struct CRMFEncryptedKeyStr" but the definition of such is not included.
Jim, Could you ask on HP's cxx-dev mailing list what's the right way to cast a pointer to an incomplete C struct to a void * pointer in C++? I think we can replace the static_cast by either a C-style cast or reinterpret_cast, but I'd like to hear from a C++ expert what's the right way to do this cast in C++.
Verified per wtc's comment.
Created attachment 116404 [details] [diff] [review] Do not use static_cast to cast a pointer to an incomplete type to a void * pointer aCC on HP-UX does not allow using static_cast to cast a pointer to an incomplete type to a void * pointer. With the removal of the crmfi.h inclusion, CRMFEncryptedKey becomes an incomplete type. Turns out that any data pointer can be implicitly converted to a void * pointer by the compiler, so no cast is needed.
Comment on attachment 116404 [details] [diff] [review] Do not use static_cast to cast a pointer to an incomplete type to a void * pointer r=kaie
Comment on attachment 116404 [details] [diff] [review] Do not use static_cast to cast a pointer to an incomplete type to a void * pointer Patch checked into the trunk (mozilla 1.4alpha). Jim, could you verify it on HP-UX?
VERIFIED on hpux... thanks wtc