Issue 258340043: Unit test for bug 11170222 PKCS#11 mechanism

Can't Edit
Can't Publish+Mail
Start Review

Created:
8 years, 8 months ago by ekr-rietveld

Modified:
8 years, 8 months ago

Reviewers:
mt, rbarnes

Visibility:
Public.

More Reviews

Description

Unit test for bug 11170222 PKCS#11 mechanism

Patch Set 2 : Updated per RLB's review #

Total comments: 4

Patch Set 3 : Revised patch set #

Total comments: 2

Created: 8 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+135 lines, -0 lines)			Patch
M	external_tests/ssl_gtest/manifest.mn	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	lib/pk11wrap/pk11mech.c	View	1 2	2 chunks	+4 lines, -0 lines	0 comments	Download
M	lib/softoken/pkcs11.c	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
M	lib/softoken/pkcs11c.c	View	1 2	1 chunk	+97 lines, -0 lines	1 comment	Download
M	lib/util/pkcs11n.h	View	1 2	2 chunks	+29 lines, -0 lines	1 comment	Download

Messages

Total messages: 13

Expand All Messages | Collapse All Messages

ekr-rietveld

Barnes, please find attached a first cut at a gtest (not really reviewed). Why don't ...

8 years, 8 months ago (2015-08-19 21:58:02 UTC) #1

rbarnes

This is pretty good. Bunch of nits, and it needs a test for the CKM_TLS_PRF ...

8 years, 8 months ago (2015-08-20 02:14:13 UTC) #2

ekr-rietveld

MT, PTAL. https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_prf_unittest.cc File external_tests/ssl_gtest/ssl_prf_unittest.cc (right): https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_prf_unittest.cc#newcode21 external_tests/ssl_gtest/ssl_prf_unittest.cc:21: 0x28,0x29,0x2a,0x2b,0x2c,0x2d,0x2e,0x2f On 2015/08/20 02:14:13, rbarnes wrote: > ...

8 years, 8 months ago (2015-08-20 15:58:00 UTC) #3

MT, PTAL.

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_prf_unittest.cc (right):

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:21:
0x28,0x29,0x2a,0x2b,0x2c,0x2d,0x2e,0x2f
On 2015/08/20 02:14:13, rbarnes wrote:
> Nit: Be consistent about spaces or no spaces.

Done.

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:29:
0xe8,0xe9,0xea,0xeb,0xec,0xed,0xee,0xef
On 2015/08/20 02:14:13, rbarnes wrote:
> Nit: Be consistent about spaces or no spaces.

Done.

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:71: void ComputeMs(SECItem* params,
const uint8_t* expected_output) {
On 2015/08/20 02:14:13, rbarnes wrote:
> Nit: ComputeEms?

I would prefer not since this way when we write a test for regular MS it works

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:79: CKF_SIGN | CKF_VERIFY);
On 2015/08/20 02:14:13, rbarnes wrote:
> Nit: Seems like you could remove some of these line breaks.

Done.

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:88: // TODO(ekr@rtfm.com): Free
msData?
On 2015/08/20 02:14:13, rbarnes wrote:
> Not necessary.  PK11_GetKeyData just returns a reference to the data in ms_,
so
> it gets freed by PK11_FreeSymKey in the destructor.

Acknowledged.

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:108: };
On 2015/08/20 02:14:13, rbarnes wrote:
> Nit: Put this with the other constants
> 
> Nit: Be consistent about spaces or no spaces.

Done.

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:116: static_cast<const unsigned
char *>(kPrfSeed)),
On 2015/08/20 02:14:13, rbarnes wrote:
> Might be worth macro-izing?  e.g., "#define CONST_TO_UCHAR_PTR"

Done

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:137: sizeof(kPrfSeed), &pms_version
On 2015/08/20 02:14:13, rbarnes wrote:
> Nit: line break

Done.

https://codereview.appspot.com/258340043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_prf_unittest.cc:152: }  // namespace nss_test
Done, though this was the wrong test vector as discussed on IM.

On 2015/08/20 02:14:13, rbarnes wrote:
> You should test the case where the hash algorithm is CKM_TLS_PRF as well. 
> Because we've made this mechanism take a unitary seed (in order to accommodate
> the session hash), you won't need to change any of the inputs -- the
> ClientRandom and ServerRandom are just implicitly the two halves of the seed
> value.
> 
> According to my python implementation [1] (which produced other vector as
well),
> the expected output for this case is:
> 
> const uint8_t kTlsPrfExpectedOutputEms[] = {
>   0x3e, 0x11, 0xf1, 0x74, 0xbf, 0xc5, 0x0a, 0x4a,
>   0x71, 0x35, 0x4a, 0x05, 0x54, 0x77, 0x9b, 0x33,
>   0xd9, 0x4f, 0x6c, 0x50, 0xb9, 0x30, 0x51, 0xc1,
>   0xca, 0x04, 0x7b, 0xcf, 0x73, 0x4b, 0xf2, 0xcb,
>   0x4d, 0xe8, 0x87, 0xe5, 0x75, 0x15, 0xa3, 0xed,
>   0xa3, 0x9e, 0xcb, 0x4f, 0xdc, 0x35, 0x54, 0x4b
> };
> 
> [1] https://pastebin.mozilla.org/8843282

I'm not sure about the coverage here. It's certainly good to cover happy path, but ...

8 years, 8 months ago (2015-08-20 21:04:31 UTC) #4

ekr-rietveld

I think at this point Richard should pick this up. My role was mostly gtestizing ...

8 years, 8 months ago (2015-08-20 21:08:23 UTC) #5

rbarnes

I've updated this patch, and sent it to EKR to upload, since AFAICT I can't. ...

8 years, 8 months ago (2015-08-21 20:11:53 UTC) #6

I've updated this patch, and sent it to EKR to upload, since AFAICT I can't. 
Comments below. (Mainly just agreeing with you.)

On 2015/08/20 21:04:31, mt wrote:
> I'm not sure about the coverage here.  It's certainly good to cover happy
path,
> but don't you want to test that the code handles errors correctly?

I've added test cases for the errors that can be caused through the PKCS#11
interface:

1. Output key size != 0, SSL3_MASTER_SECRET_LENGTH
2. not-DH && input key size != SSL3_PMS_LENGTH
3. CKM_TLS_PRF && seed length != MD5_LENGTH + SHA1_LENGTH
4. !CKM_TLS_PRF && seed length != hash output length


>
https://codereview.appspot.com/258340043/diff/20001/external_tests/ssl_gtest/...
> File external_tests/ssl_gtest/ssl_prf_unittest.cc (right):
> 
>
https://codereview.appspot.com/258340043/diff/20001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/ssl_prf_unittest.cc:16: static_cast<const unsigned
char
> *>(a))
> I know that you love #define, but I hate it. Isn't this equally good?
> 
> static unsigned char* toUcharPtr(const uint8_t* v) {
>   return const_cast<unsigned char*>(
>     static_cast<const unsigned char *>(v));
> }

Done.


>
https://codereview.appspot.com/258340043/diff/20001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/ssl_prf_unittest.cc:164: ASSERT_EQ(1,
> pms_version_.minor);
> I was told to use EXPECT_EQ here.  Failing this check doesn't need to mean
that
> the test ends: maybe there's more information to gain from later failures.

Done.


>
https://codereview.appspot.com/258340043/diff/20001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/ssl_prf_unittest.cc:204: ComputeMs(&params,
> kExpectedOutputEmsSha256);
> Assuming that you can always include a pointer to pms_version_ in the
> parameters, you could reduce the three complex statements to a single call for
> these four cases:
> 
> ComputeMs(CKM_SHA256, kPrfSeedSizeSha256, kExpectedOutputEmsSha256);

Done.  It's not quite that small, but almost.


> Also, I wonder if the value of key_mech_ could be used to populate the first
two
> parameters.  Unless you wanted to test how the code handled mismatches there,
> which it probably should.

It doesn't appear that the current TLS derivation functions test this, so I'm
concluding that this one doesn't need to either.  I've implemented this
inference.


>
https://codereview.appspot.com/258340043/diff/20001/external_tests/ssl_gtest/...
> external_tests/ssl_gtest/ssl_prf_unittest.cc:207: ASSERT_EQ(1,
> pms_version_.minor);
> As above.

Done.

LGTM https://codereview.appspot.com/258340043/diff/40001/lib/softoken/pkcs11c.c File lib/softoken/pkcs11c.c (right): https://codereview.appspot.com/258340043/diff/40001/lib/softoken/pkcs11c.c#newcode6148 lib/softoken/pkcs11c.c:6148: SECItem master = { siBuffer, NULL, 0 }; ...

8 years, 8 months ago (2015-08-21 22:09:09 UTC) #9

ekr-rietveld

MT, please advise when tree is open and we will land this.

8 years, 8 months ago (2015-08-21 22:10:15 UTC) #10

rbarnes

EKR, are you going to fix the last nits from MT? Sent from my iPhone. ...

8 years, 8 months ago (2015-08-22 00:39:36 UTC) #11

rbarnes

8 years, 8 months ago (2015-08-24 13:24:49 UTC) #13

Since mt's nits were in the base patch, not the test, I uploaded the new patch
to the bug.

https://bugzilla.mozilla.org/attachment.cgi?id=8651755&action=edit

I think both patches (base + test) are clear to land now.

> On Aug 23, 2015, at 6:07 PM, ekr-webrtc@rtfm.com wrote:
> 
> Why don't you fix and then I can upload for you.
> 
> https://codereview.appspot.com/258340043/

Expand All Messages | Collapse All Messages