Make it easy to create test certificates in GTest tests

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

No description provided.
The idea here is to create a DER-encoded certificate with a reference to its private key, without ever instantiating a CERTCertificate object for it. This way, we don't have to worry about polluting the CERTCertificate caches that NSS has and thus we don't need to do cert generation and signing in a separate process.
Attachment #8416612 - Flags: review?(dkeeler)
Comment on attachment 8416612 [details] [diff] [review]
test-certs.patch

Review of attachment 8416612 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +31,3 @@
>  #include "secder.h"
>  
> +using namespace std;

I think this is a little too broad according to the style guidelines:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces

using namespace std::strcpy;

etc

@@ +305,4 @@
>  }
>  
> +static SECItem*
> +Boolean(PLArenaPool* arena, bool value)

I'm confused by these, since there is Boolean in pkixder.h with a different sig.

@@ +314,5 @@
> +  }
> +  result->data[0] = der::BOOLEAN;
> +  result->data[1] = 1; // length
> +  result->data[2] = value ? 0xff : 0x00;
> +  return result;  

pink in splinter diff

@@ +376,5 @@
> +
> +  if (encoding == GeneralizedTime) {
> +    derTime->data[i++] = '0' + (exploded.tm_year / 1000);
> +    derTime->data[i++] = '0' + ((exploded.tm_year % 1000) / 100);
> +  } else if (exploded.tm_year < 1950 || exploded.tm_year >= 2050) {

Is it normal to include no reference for hardcoded constants?
Comment on attachment 8416612 [details] [diff] [review]
test-certs.patch

Review of attachment 8416612 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking at this Monica.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +31,3 @@
>  #include "secder.h"
>  
> +using namespace std;

I don't think that's what the style guide is saying. I think the part of the style guide you are referring to is about "using" for nested namespaces. Otherwise, please quote which part of the style guide you think I'm not following. (Note: this isn't a header file.)

@@ +305,4 @@
>  }
>  
> +static SECItem*
> +Boolean(PLArenaPool* arena, bool value)

We could name all of these "EncodeXXX" instead of "XXX" if that would be clearer. Personally, I don't think it would be much clearer but also I don't care much either way.

@@ +314,5 @@
> +  }
> +  result->data[0] = der::BOOLEAN;
> +  result->data[1] = 1; // length
> +  result->data[2] = value ? 0xff : 0x00;
> +  return result;  

Thanks.

@@ +376,5 @@
> +
> +  if (encoding == GeneralizedTime) {
> +    derTime->data[i++] = '0' + (exploded.tm_year / 1000);
> +    derTime->data[i++] = '0' + ((exploded.tm_year % 1000) / 100);
> +  } else if (exploded.tm_year < 1950 || exploded.tm_year >= 2050) {

I will include a reference to http://tools.ietf.org/html/rfc5280#section-4.1.2.5 which covers the whole syntax.
You are right, I was misreading the style-guide about namespaces.

Re: EncodeBoolean vs Boolean, this was really just a drive-by glance from someone who barely knows the code -- but to my eyes EncodeBoolean is clearer because it's obviously related but different from the version in pkixder.h, since pkixder.h::Boolean appears in other unittests in the same module.

Thanks for working on the bug!
Comment on attachment 8416612 [details] [diff] [review]
test-certs.patch

Review of attachment 8416612 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Just a few nits and questions. r=me with those addressed.

::: security/pkix/include/pkix/pkix.h
@@ +115,5 @@
>                                      CERTCertificate* issuerCert,
>                                      PRTime time,
>                                      const SECItem* encodedResponse,
> +                 /* optional out */ PRTime* thisUpdate = nullptr,
> +                 /* optional out */ PRTime* validThrough = nullptr);

Is this change related?

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +40,5 @@
> +
> +inline void
> +deleteCharArray(char* chars)
> +{
> +  delete [] chars;

nit: I've always seen this as "delete[]"

@@ +69,5 @@
> +  {
> +    FILE* rawFile;
> +    errno_t error = fopen_s(&rawFile, path.get(), mode);
> +    if (error) {
> +    // TODO: map error to NSPR error code

nit: indent comment

@@ +82,5 @@
> +    // TODO: map errno to NSPR error code
> +    PR_SetError(PR_FILE_NOT_FOUND_ERROR, errno);
> +  }
> +#endif
> +  return file.release();

is release the same as forget?

@@ +314,5 @@
> +  }
> +  result->data[0] = der::BOOLEAN;
> +  result->data[1] = 1; // length
> +  result->data[2] = value ? 0xff : 0x00;
> +  return result;  

nit: trailing spaces

@@ +318,5 @@
> +  return result;  
> +}
> +
> +static SECItem*
> +Integer(PLArenaPool* arena, long value)

any particular reason we don't want to specify the size of this type here?

@@ +376,5 @@
> +
> +  if (encoding == GeneralizedTime) {
> +    derTime->data[i++] = '0' + (exploded.tm_year / 1000);
> +    derTime->data[i++] = '0' + ((exploded.tm_year % 1000) / 100);
> +  } else if (exploded.tm_year < 1950 || exploded.tm_year >= 2050) {

I feel like this check should go earlier in the function (in particular, before the allocation (not that it really matters, since it's in the arena anyway)).

@@ +392,5 @@
> +  derTime->data[i++] = '0' + (exploded.tm_hour % 10);
> +  derTime->data[i++] = '0' + (exploded.tm_min / 10);
> +  derTime->data[i++] = '0' + (exploded.tm_min % 10);
> +  derTime->data[i++] = '0' + (exploded.tm_sec / 10);
> +  derTime->data[i++] = '0' + (exploded.tm_sec % 10);  

nit: trailing space

@@ +410,5 @@
> +{
> +  PRExplodedTime exploded;
> +  PR_ExplodeTime(time, PR_GMTParameters, &exploded);
> +  return PRTimeToEncodedTime(arena, time,
> +    (exploded.tm_year >= 1950 && exploded.tm_year < 2050) ? UTCTime

so, UTCTime/GeneralizedTime is chosen based on the year?

@@ +420,5 @@
> +           SECKEYPrivateKey* privKey, SECOidTag hashAlg,
> +           bool corrupt, /*optional*/ SECItem const* const* certs)
> +{
> +  PR_ASSERT(arena);
> +  PR_ASSERT(privKey);

also assert tbsData, privKey?

@@ +545,5 @@
> +    return nullptr;
> +  }
> +
> +  // This allows us to more easily debug the generated output, by creating a
> +  // file in the directory given by MOZILLA_PKIX_TEST_LOG_DIR for each 

nit: trailing space

@@ +547,5 @@
> +
> +  // This allows us to more easily debug the generated output, by creating a
> +  // file in the directory given by MOZILLA_PKIX_TEST_LOG_DIR for each 
> +  // NOT THREAD-SAFE!!!
> +  const char* logPath = getenv("MOZILLA_PKIX_TEST_LOG_DIR");

not PR_GetEnv?

@@ +580,5 @@
> +  params.keySizeInBits = 2048;
> +  params.pe = 3;
> +  SECKEYPublicKey* publicKeyTemp = nullptr;
> +  privateKey = PK11_GenerateKeyPair(slot.get(), CKM_RSA_PKCS_KEY_PAIR_GEN,
> +  				      &params, &publicKeyTemp, false, true,

nit: indentation

@@ +748,5 @@
> +
> +  // SubjectPublicKeyInfo  ::=  SEQUENCE  {
> +  //       algorithm            AlgorithmIdentifier,
> +  //       subjectPublicKey     BIT STRING  }
> +  // TODO(bug 984608): remove the const_cast once the signature of

Looks like this has been fixed?

::: security/pkix/test/lib/pkixtestutil.h
@@ +31,5 @@
> +
> +inline void fclose_void(FILE* file) { (void) fclose(file); }
> +
> +inline void
> +SECITEM_FreeItem_True(SECItem* item)

let's be consistent with whether or not these functions are all on one line and how they're named (uppercase True vs. lowercase true doesn't match the pattern with fclose_void (although that refers to the return value, so it's not quite analogous anyway)).

@@ +71,5 @@
> +                                  const char* issuerASCII, PRTime notBefore,
> +                                  PRTime notAfter, const char* subjectASCII,
> +                     /*optional*/ SECItem const* const* extensions,
> +                     /*optional*/ SECKEYPrivateKey* issuerPrivateKey,
> +                                  SECOidTag signatureHashAlg,

I'm a little confused here - is signatureHashAlg not the same as signature?

@@ +74,5 @@
> +                     /*optional*/ SECKEYPrivateKey* issuerPrivateKey,
> +                                  SECOidTag signatureHashAlg,
> +                          /*out*/ ScopedSECKEYPrivateKey& privateKey);
> +
> +enum ExtensionCriticality { NonCritical = 0, Critical = 1 };

nit: maybe "NotCritical"?
Attachment #8416612 - Flags: review?(dkeeler) → review+
Comment on attachment 8416612 [details] [diff] [review]
test-certs.patch

Review of attachment 8416612 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the reviews.

I decided not to rename things to "EncodeXXX" from "XXX" because the existing convention in the file is to omit the "Encode" prefix and also I don't think there is any real ambiguity given that the two sets of functions are in different namespaces and given that this file doesn't do any DER decoding, only encoding.

::: security/pkix/include/pkix/pkix.h
@@ +115,5 @@
>                                      CERTCertificate* issuerCert,
>                                      PRTime time,
>                                      const SECItem* encodedResponse,
> +                 /* optional out */ PRTime* thisUpdate = nullptr,
> +                 /* optional out */ PRTime* validThrough = nullptr);

Moved this change to the changeset it was intended for.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +40,5 @@
> +
> +inline void
> +deleteCharArray(char* chars)
> +{
> +  delete [] chars;

Changed to "delete[]".

@@ +69,5 @@
> +  {
> +    FILE* rawFile;
> +    errno_t error = fopen_s(&rawFile, path.get(), mode);
> +    if (error) {
> +    // TODO: map error to NSPR error code

Thanks.

@@ +82,5 @@
> +    // TODO: map errno to NSPR error code
> +    PR_SetError(PR_FILE_NOT_FOUND_ERROR, errno);
> +  }
> +#endif
> +  return file.release();

Yes, see http://www.cplusplus.com/reference/memory/unique_ptr/release/.

@@ +318,5 @@
> +  return result;  
> +}
> +
> +static SECItem*
> +Integer(PLArenaPool* arena, long value)

I don't see why using a sized type would help things much, since there isn't a type with range 0-127 anyway.

@@ +376,5 @@
> +
> +  if (encoding == GeneralizedTime) {
> +    derTime->data[i++] = '0' + (exploded.tm_year / 1000);
> +    derTime->data[i++] = '0' + ((exploded.tm_year % 1000) / 100);
> +  } else if (exploded.tm_year < 1950 || exploded.tm_year >= 2050) {

OK, I moved the check.

@@ +410,5 @@
> +{
> +  PRExplodedTime exploded;
> +  PR_ExplodeTime(time, PR_GMTParameters, &exploded);
> +  return PRTimeToEncodedTime(arena, time,
> +    (exploded.tm_year >= 1950 && exploded.tm_year < 2050) ? UTCTime

I added this comment:

// http://tools.ietf.org/html/rfc5280#section-4.1.2.5: "CAs conforming to this
// profile MUST always encode certificate validity dates through the year 2049
// as UTCTime; certificate validity dates in 2050 or later MUST be encoded as
// GeneralizedTime." (This is a special case of the rule that we must always
// use the shortest possible encoding.)

@@ +420,5 @@
> +           SECKEYPrivateKey* privKey, SECOidTag hashAlg,
> +           bool corrupt, /*optional*/ SECItem const* const* certs)
> +{
> +  PR_ASSERT(arena);
> +  PR_ASSERT(privKey);

I changed this to:

  PR_ASSERT(arena);
  PR_ASSERT(tbsData);
  PR_ASSERT(privKey);
  if (!arena || !tbsData || !privKey) {
    PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
    return nullptr;
  }

@@ +545,5 @@
> +    return nullptr;
> +  }
> +
> +  // This allows us to more easily debug the generated output, by creating a
> +  // file in the directory given by MOZILLA_PKIX_TEST_LOG_DIR for each 

Thanks. Fixed.

@@ +547,5 @@
> +
> +  // This allows us to more easily debug the generated output, by creating a
> +  // file in the directory given by MOZILLA_PKIX_TEST_LOG_DIR for each 
> +  // NOT THREAD-SAFE!!!
> +  const char* logPath = getenv("MOZILLA_PKIX_TEST_LOG_DIR");

I think it does not matter for this test code and better to avoid NSPR if we can.

@@ +580,5 @@
> +  params.keySizeInBits = 2048;
> +  params.pe = 3;
> +  SECKEYPublicKey* publicKeyTemp = nullptr;
> +  privateKey = PK11_GenerateKeyPair(slot.get(), CKM_RSA_PKCS_KEY_PAIR_GEN,
> +  				      &params, &publicKeyTemp, false, true,

Fixed.

@@ +748,5 @@
> +
> +  // SubjectPublicKeyInfo  ::=  SEQUENCE  {
> +  //       algorithm            AlgorithmIdentifier,
> +  //       subjectPublicKey     BIT STRING  }
> +  // TODO(bug 984608): remove the const_cast once the signature of

Removed the comment.

::: security/pkix/test/lib/pkixtestutil.h
@@ +31,5 @@
> +
> +inline void fclose_void(FILE* file) { (void) fclose(file); }
> +
> +inline void
> +SECITEM_FreeItem_True(SECItem* item)

Changed to:

inline void
fclose_void(FILE* file) {
  (void) fclose(file);
}

inline void
SECITEM_FreeItem_true(SECItem* item)
{
  SECITEM_FreeItem(item, true);
}

@@ +71,5 @@
> +                                  const char* issuerASCII, PRTime notBefore,
> +                                  PRTime notAfter, const char* subjectASCII,
> +                     /*optional*/ SECItem const* const* extensions,
> +                     /*optional*/ SECKEYPrivateKey* issuerPrivateKey,
> +                                  SECOidTag signatureHashAlg,

They are *suppoosed* to be the same in a *valid* cert, but they may be different in an invalid cert.

@@ +74,5 @@
> +                     /*optional*/ SECKEYPrivateKey* issuerPrivateKey,
> +                                  SECOidTag signatureHashAlg,
> +                          /*out*/ ScopedSECKEYPrivateKey& privateKey);
> +
> +enum ExtensionCriticality { NonCritical = 0, Critical = 1 };

Renamed that. Also made this MOZILLA_PKIX_ENUM_CLASS.
Depends on: 1005667
https://hg.mozilla.org/mozilla-central/rev/2a5624d51bd3
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.