initial insanity::pkix code

initial insanity::pkix code

[dkeeler, 2013-03-27 18:53:58]

https://codereview.appspot.com/7970044/diff/4007/lib/pkixbuild.cpp
File lib/pkixbuild.cpp (right):

https://codereview.appspot.com/7970044/diff/4007/lib/pkixbuild.cpp#newcode55
lib/pkixbuild.cpp:55: switch (SECOidTag tag = SECOID_FindOIDTag(&ext->id)) {
gcc-4.6.3 gives me "lib/pkixbuild.cpp:60:25: error: unused variable 'tag'
[-Werror=unused-variable]" here unless the declaration/assignment is separated
from the switch.

[Ryan Sleevi, 2013-04-05 02:21:36]

Ah, unfortunately it appears that upload.py is only taking the delta from the
previous patchset, rather than the traditional approach of taking the delta from
a fixed point.

If you're using upload.py (and it seems you are), the --rev option will let you
specify the base rev from where your branchpoint started (I'm guessing ~15
revisions before?), which will help show the full change in context.

[Brian Smith, 2013-04-11 19:07:59]

On 2013/04/05 02:21:36, Ryan Sleevi wrote:
> If you're using upload.py (and it seems you are), the --rev option will let
you
> specify the base rev from where your branchpoint started (I'm guessing ~15
> revisions before?), which will help show the full change in context.

I think you are saying that all 17 patches should have the same base revisition.
I read some (apparently bad) advice on the internet to do the opposite, and make
the base revision the previous revision.

I will update all the patches. Besides fixing this issue, I will also fix the
problem that keeler identified, split up patch 6 into at least two simpler
parts, fix the OCSP bug that is causing the test failure that I has previously
thought was a transient network problem. Also, based on the plan that cviecco
and I made for Firefox, we would like to split off the policy work (patches
15-17) into a separate bug.

Now is a good time to suggest any other reorganization of how the review is
done, if you have any suggestions.

[cviecco, 2013-04-29 21:16:19]

Looks good.

One general comment: should optional arguments come with default values?

https://codereview.appspot.com/7970044/diff/10001/test/pkixvfychain.cpp
File test/pkixvfychain.cpp (left):

https://codereview.appspot.com/7970044/diff/10001/test/pkixvfychain.cpp#oldco...
test/pkixvfychain.cpp:750: CERT_DestroyCertList(builtChain);
Why is this removed?

https://codereview.appspot.com/7970044/diff/10001/test/pkixvfychain.cpp
File test/pkixvfychain.cpp (right):

https://codereview.appspot.com/7970044/diff/10001/test/pkixvfychain.cpp#newco...
test/pkixvfychain.cpp:160: SECItem         item = {siBuffer, NULL, 0};
siBuffer? (this is cviecco's ignorance).

https://codereview.appspot.com/7970044/diff/10001/test/pkixvfychain.cpp#newco...
test/pkixvfychain.cpp:329: PRUint64 *revFlags = NULL;
nullptr?

https://codereview.appspot.com/7970044/diff/12001/lib/pkixkey.cpp
File lib/pkixkey.cpp (right):

https://codereview.appspot.com/7970044/diff/12001/lib/pkixkey.cpp#newcode11
lib/pkixkey.cpp:11: ExtractPublicKey(/*optional*/ const CERTCertificate *
endEntityCert,
1. Put a comment with what you want the function do do.

2.Style question: why put the optional params at the beggining and not at the
end with default values?

https://codereview.appspot.com/7970044/diff/12001/lib/pkixkey.cpp#newcode28
lib/pkixkey.cpp:28: endEntityCert = CERT_LIST_HEAD(chain)->cert;
you are assigning endEntityCert to the same value regardless of the condition
(!endEntityCert)

https://codereview.appspot.com/7970044/diff/12001/lib/pkixutil.h
File lib/pkixutil.h (right):

https://codereview.appspot.com/7970044/diff/12001/lib/pkixutil.h#newcode13
lib/pkixutil.h:13: enum Result { Success, FatalError, RecoverableError };
what makes a result fatal vs recoverable? Needs documentation next to the enum

https://codereview.appspot.com/7970044/diff/3007/include/insanity/pkixtypes.h
File include/insanity/pkixtypes.h (right):

https://codereview.appspot.com/7970044/diff/3007/include/insanity/pkixtypes.h...
include/insanity/pkixtypes.h:35: PRTime time,
make all input paramters consts?

https://codereview.appspot.com/7970044/diff/3007/include/insanity/pkixtypes.h...
include/insanity/pkixtypes.h:47: virtual SECStatus IsRevoked(EndEntityOrCA
endEntityOrCA,
Describe the behavior of this function:
When is it called?
What are the impications of each input parameter?

What would insanity do for the differnt reutrn values?

https://codereview.appspot.com/7970044/diff/3007/test/pkixvfychain.cpp
File test/pkixvfychain.cpp (right):

https://codereview.appspot.com/7970044/diff/3007/test/pkixvfychain.cpp#newcod...
test/pkixvfychain.cpp:423: public:
move object variables (in particular private ones) to before the constructor?

https://codereview.appspot.com/7970044/diff/3007/test/pkixvfychain.cpp#newcod...
test/pkixvfychain.cpp:467: if (trustedCertList) {
defined where?

https://codereview.appspot.com/7970044/diff/2002/include/insanity/pkix.h
File include/insanity/pkix.h (right):

https://codereview.appspot.com/7970044/diff/2002/include/insanity/pkix.h#newc...
include/insanity/pkix.h:11: CERTCertificate * cert,
const?

https://codereview.appspot.com/7970044/diff/2002/include/insanity/pkix.h#newc...
include/insanity/pkix.h:12: PRTime time,
const?

https://codereview.appspot.com/7970044/diff/2002/lib/pkixbuild.cpp
File lib/pkixbuild.cpp (right):

https://codereview.appspot.com/7970044/diff/2002/lib/pkixbuild.cpp#newcode9
lib/pkixbuild.cpp:9: // XXX: This check was copied exactly from
cert_HasUnknownCriticalExten
Thus we will need some tests for this (insanity test suite)

https://codereview.appspot.com/7970044/diff/2002/lib/pkixutil.h
File lib/pkixutil.h (right):

https://codereview.appspot.com/7970044/diff/2002/lib/pkixutil.h#newcode35
lib/pkixutil.h:35: struct BackCert
cviecco's ingnorance-> struct instead of class? I mean a struct with methods
seems like a class.

https://codereview.appspot.com/7970044/diff/19007/include/insanity/pkix.h
File include/insanity/pkix.h (right):

https://codereview.appspot.com/7970044/diff/19007/include/insanity/pkix.h#new...
include/insanity/pkix.h:14: /*optional*/ SECOidTag requiredEKUIfPresent,
OK I get the api

[cviecco, 2013-05-01 18:34:44]

PLArena are prefrerred, correct?

we should use int64_t or PRInt64 types instead of int64.

https://codereview.appspot.com/7970044/diff/7003/lib/pkixocsp.cpp
File lib/pkixocsp.cpp (right):

https://codereview.appspot.com/7970044/diff/7003/lib/pkixocsp.cpp#newcode98
lib/pkixocsp.cpp:98: PRArenaPool *arena;			/* local; not part of encoding */
s/PRArenaPool/PLArenaPool/g ?

Incompat found when building againstFF

https://codereview.appspot.com/7970044/diff/7003/lib/pkixocsp.cpp#newcode497
lib/pkixocsp.cpp:497: ocsp_CertRevokedAfter(ocspRevokedInfo *revokedInfo, int64
time);
s/int64/int64_t/?

[cviecco, 2013-05-06 20:20:58]

https://codereview.appspot.com/7970044/diff/19007/lib/pkixbuild.cpp
File lib/pkixbuild.cpp (right):

https://codereview.appspot.com/7970044/diff/19007/lib/pkixbuild.cpp#newcode40
lib/pkixbuild.cpp:40: case 37: out = &encodedExtendedKeyUsage; continue;
BUG!!!! is break not a continue.