1154399 - Clean up pkixder.h

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

1. Remove unnecessary templates.
2. Replace old code patterns with new ones.
3. Move code that doesn't need to be inline to pkixder.cpp where practical.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 1: De-templatize and un-inline IntegralValue

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 2: Simplify and un-inline OptionalVersion

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 3: Simplify OptionalExtensions

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Part 4: Simplify certificate parsing in OCSP responses

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f783e1d81dd

I've noticed that OS X builds are often timing out on tryserver lately, so I'm just ignoring those.

Comment 6

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8592370 [details] [diff] [review]
Part 3: Simplify OptionalExtensions

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

This looks great.

Comment 7

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8592372 [details] [diff] [review]
Part 4: Simplify certificate parsing in OCSP responses

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

LGTM.

::: security/pkix/lib/pkixocsp.cpp
@@ +410,3 @@
>    NonOwningDERArray certs;
>    if (!input.AtEnd()) {
> +    rv = der::Nested(input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0,

If I'm reading this correctly, we now don't check that we've consumed all of input (whereas I believe ExpectTagAndGetValueAtEnd did). (Ah, but I see that BasicResponse is called from within a call to Nested, which does check that we consume all of this input by the time we return, so I think this is ok.)

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #7)
> If I'm reading this correctly, we now don't check that we've consumed all of
> input (whereas I believe ExpectTagAndGetValueAtEnd did). (Ah, but I see that
> BasicResponse is called from within a call to Nested, which does check that
> we consume all of this input by the time we return, so I think this is ok.)

Yes, exactly. I admit this isn't as clear as I'd like but this is a move towards making things more consistent which should ultimately provide that clarity.

Thanks for the super-fast reviews!

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/438fa1ccf5fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/58fd93740f87
https://hg.mozilla.org/integration/mozilla-inbound/rev/37975b5017c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b71382534453

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/438fa1ccf5fc
https://hg.mozilla.org/mozilla-central/rev/58fd93740f87
https://hg.mozilla.org/mozilla-central/rev/37975b5017c4
https://hg.mozilla.org/mozilla-central/rev/b71382534453