Closed Bug 986150 Opened 10 years ago Closed 10 years ago

mozilla::pkix: test der::OptionalBoolean

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(2 files, 2 obsolete files)

The patch in bug 985021 introduces mozilla::pkix::der::OptionalBoolean to handle cases of BOOLEAN DEFAULT FALSE in encodings (i.e. if the boolean is present and has value false, it is an error). There are more places where this would be useful. We should also cover it in our tests.
Turns out, there's only one place we decode BOOLEAN at all, and it already uses OptionalBoolean, so all that's needed are some tests.
Summary: mozilla::pkix: use and test der::OptionalBoolean in all the places (introduced in bug 985021) → mozilla::pkix: test der::OptionalBoolean
Attached patch test (obsolete) — Splinter Review
Any other tests related to this you think I should add?
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8421955 - Flags: review?(mmc)
Comment on attachment 8421955 [details] [diff] [review]
test

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

For readability, I would restructure so that are

- 1 test with the only valid encoding first (with ASSERT_EQ(Success, ...) << "Should accept the only valid encoding for OptionalBoolean"
- 1 test with multiple invalid encodings (true boolean and integer), each encoding with allowInvalid = true followed by allowInvalid = false with appropriate ASSERT_EQ output
- 1 test with the AtEnd stuff

That way it's clear that you're testing allowInvalid orthogonally. Actually I'm not sure if the AtEnd stuff adds much.

Seems reasonable to me, though.

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +149,5 @@
> +                       sizeof DER_OPTIONAL_BOOLEAN_PRESENT_TRUE));
> +
> +  bool value = false;
> +  ASSERT_EQ(Success, OptionalBoolean(input, false, value));
> +  ASSERT_EQ(true, value);

Always use ASSERT_TRUE(value) when expecting a boolean.

@@ +167,5 @@
> +            input.Init(DER_OPTIONAL_BOOLEAN_PRESENT_FALSE,
> +                       sizeof DER_OPTIONAL_BOOLEAN_PRESENT_FALSE));
> +
> +  bool value = false;
> +  ASSERT_EQ(Failure, OptionalBoolean(input, false, value));

I would find this test easier to read if combined with the next test. It took my eyes a while to distinguish the input param is being tested.

Also, it is nice when doing these to give a string to the assert when it fails. That way it is more self documenting. From http://mxr.mozilla.org/mozilla-central/source/testing/gtest/gtest/include/gtest/gtest.h#1844

ASSERT_EQ(Failure, OptionalBoolean(input, false, value)) <<
  "Should disallow invalid encoding";
ASSERT_EQ(Failure, OptionalBoolean(input, true, value)) <<
  "Should allow invalid encoding";

@@ +193,5 @@
> +
> +TEST_F(pkixder_universal_types_tests, OptionalBooleanNotPresentNotAtEnd)
> +{
> +  const uint8_t DER_INTEGER_05[] = {
> +    0x02,                       // INTEGER

I was initially confused why this is INTEGER when the test is for OptionalBoolean. But probably I don't understand OptionalBoolean very well.

@@ +202,5 @@
> +  Input input;
> +  ASSERT_EQ(Success, input.Init(DER_INTEGER_05, sizeof DER_INTEGER_05));
> +
> +  bool value = true;
> +  ASSERT_EQ(Success, OptionalBoolean(input, true, value));

ASSERT_EQ(Success, OptionalBoolean(input, true, value)) << "Should allow invalid encoding for Integer DER (or something)";

@@ +203,5 @@
> +  ASSERT_EQ(Success, input.Init(DER_INTEGER_05, sizeof DER_INTEGER_05));
> +
> +  bool value = true;
> +  ASSERT_EQ(Success, OptionalBoolean(input, true, value));
> +  ASSERT_EQ(false, value);

ASSERT_FALSE(value);
Attachment #8421955 - Flags: review?(mmc) → review-
Attached patch test v2 (obsolete) — Splinter Review
Thanks for the review - let me know if this is the kind of thing you were looking for.
Attachment #8421955 - Attachment is obsolete: true
Attachment #8424993 - Flags: review?(mmc)
I separated out the patch to fix the comments in the other tests (the DER tags were often mislabeled).
Attachment #8424994 - Flags: review?(mmc)
Attachment #8424994 - Flags: review?(mmc) → review+
Comment on attachment 8424993 [details] [diff] [review]
test v2

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

::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ +159,5 @@
> +  };
> +
> +  Input input1;
> +  ASSERT_EQ(Success, input1.Init(DER_OPTIONAL_BOOLEAN_PRESENT_TRUE,
> +                                sizeof DER_OPTIONAL_BOOLEAN_PRESENT_TRUE));

alignment on sizeof

@@ +203,5 @@
> +  ASSERT_EQ(Success, input1.Init(DER_OPTIONAL_BOOLEAN_PRESENT_FALSE,
> +                                 sizeof DER_OPTIONAL_BOOLEAN_PRESENT_FALSE));
> +  bool value;
> +  // If the second parameter to OptionalBoolean is false, invalid encodings
> +  // that include the field even when it is the DEFAULT FALSE are rejected.

This comment is fine. However, you may find it more readable to omit the comment and do

bool allowInvalid = false;
ASSERT_EQ(Failure, OptionalBoolean(blah, allowInvalid, value))
allowInvalid = true;
ASSERT_EQ(Success, OptionalBoolean(blah, allowInvalid, value))

Either way is OK.
Attachment #8424993 - Flags: review?(mmc) → review+
https://hg.mozilla.org/mozilla-central/rev/1214948e42bb
https://hg.mozilla.org/mozilla-central/rev/2ae2a011788b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.