Closed Bug 1111392 Opened 6 years ago Closed 6 years ago

Add tests for malformed name constraints when there are no names of the constrained type

Categories

(Core :: Security: PSM, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

No description provided.
Attachment #8536227 - Flags: review?(dkeeler)
Severity: major → minor
OS: Windows 8.1 → All
Hardware: x86_64 → All
Comment on attachment 8536227 [details] [diff] [review]
more-name-constraint-tests.patch

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

LGTM. Just a couple of questions.

::: security/pkix/test/gtest/pkixnames_tests.cpp
@@ +1754,5 @@
> +  // Name constraints for unsupported types of names are ignored when there are
> +  // are no names of that type to constrain.
> +  { ByteString(), NO_SAN,
> +    GeneralSubtree(DNSName("!")),
> +    Success, Success

Isn't this the same as the first new test?

@@ +1758,5 @@
> +    Success, Success
> +  },
> +
> +  /////////////////////////////////////////////////////////////////////////////
> +  // Name constraints for unsupported types of names guarantee always result in

s/guarantee // ?

@@ +1760,5 @@
> +
> +  /////////////////////////////////////////////////////////////////////////////
> +  // Name constraints for unsupported types of names guarantee always result in
> +  // failure when there are names of that type to constrain.
> +

Are there supposed to be more tests here, or is this just additional documentation for existing tests?
Attachment #8536227 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #1)
> Isn't this the same as the first new test?

Yes. I removed the redundant copy. Thanks for noticing that.

> > +  /////////////////////////////////////////////////////////////////////////////
> > +  // Name constraints for unsupported types of names guarantee always result in
> 
> s/guarantee // ?

Oops! I overlooked this comment. I will push a fix for this typo.

> > +  /////////////////////////////////////////////////////////////////////////////
> > +  // Name constraints for unsupported types of names guarantee always result in
> > +  // failure when there are names of that type to constrain.
> > +
> 
> Are there supposed to be more tests here, or is this just additional
> documentation for existing tests?

I decided to split those tests into a separate patch because they aren't relevant to the RFC 822 name constraint issue like this one is. I forgot to remove that comment, but I've removed the comment. Thanks for noticing this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/64e991d62eeb
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #2)
> > > +  /////////////////////////////////////////////////////////////////////////////
> > > +  // Name constraints for unsupported types of names guarantee always result in
> > 
> > s/guarantee // ?
> 
> Oops! I overlooked this comment. I will push a fix for this typo.

Actually, I'll just incorporate that typo fix into the not-yet-pushed patch that will add that comment!
https://hg.mozilla.org/mozilla-central/rev/64e991d62eeb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.