DO NOT REVIEW: Certificate Transparency: Extend SCT verify result enum.

DO NOT REVIEW: Certificate Transparency: Extend SCT verify result enum.

(a better description will come soon)

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

[Eran Messeri, 2016-08-03 16:59:40]

Description was changed from

==========
DO NOT REVIEW: Certificate Transparency: Extend SCT verify result enum.

(a better description will come soon)

BUG=
==========

to

==========
DO NOT REVIEW: Certificate Transparency: Extend SCT verify result enum.

(a better description will come soon)

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Ryan Sleevi, 2016-08-03 19:19:39]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2016-08-03 19:19:40]

There are a number of ways you could design this, and it's not clear why you
chose particular designs.

For example, one approach would have been to change the public API (to provide
more fidelity), but then leave it as a further task for consumers to decide how
to handle it. That is, INVALID_BECAUSE_X and INVALID_BECAUSE_Y can be exposed
from the SCTVerifyStatus API, but then coalesced into the existing INVALID
pattern at higher layers. It's unclear why you chose to change all at once, or
whether that's the right thing, but I suspect you had reasonings for it? This
might be an example of trying to find the right size of the CL and the right
end-state. It sounded like the expectation was there'd be additional discussion
re: integration with those bits?

I realize you published this as DO NOT REVIEW, but mostly, I'm trying to
understand your reasoning and approach to the problem, to see if there's the
opportunity for guidance or streamlining to make it easier for future CLs :)

https://codereview.chromium.org/2208073002/diff/20001/net/cert/ct_verify_resu...
File net/cert/ct_verify_result.h (right):

https://codereview.chromium.org/2208073002/diff/20001/net/cert/ct_verify_resu...
net/cert/ct_verify_result.h:25: // then switch to using that.
Why? :)

https://codereview.chromium.org/2208073002/diff/20001/net/cert/ct_verify_resu...
net/cert/ct_verify_result.h:42: SCTAndStatusList invalid_scts;
From a design standpoint, could you explain more your reasoning for this change?

One approach could have been to leave it as an SCTList (that is, not surface the
additional details beyond this API point yet)

Another could have been to combine all three vectors into a single vector, since
you're now tracking SCTVerifyStatus with the SCT.

But it's unclear why you did a little of A and a little of B

https://codereview.chromium.org/2208073002/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2208073002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:90131: +  <int value="2"
label="SCT_STATUS_INVALID_SIGNATURE"/>
This does the thing I mentioned on the thread that I didn't think we should do
:)

Which is, it changes the semantic meaning of this value (that is,
INVALID_SIGNATURE is a sub-category of INVALID)

The extent of implications here are primarily because it makes it difficult to
compare ver X to Y, because in version X, it means one thing, and version Y, it
means something different.

[Eran Messeri, 2016-08-03 21:24:06]

On 2016/08/03 at 19:19:40, rsleevi wrote:
> There are a number of ways you could design this, and it's not clear why you
chose particular designs.
> 
> For example, one approach would have been to change the public API (to provide
more fidelity), but then leave it as a further task for consumers to decide how
to handle it. That is, INVALID_BECAUSE_X and INVALID_BECAUSE_Y can be exposed
from the SCTVerifyStatus API, but then coalesced into the existing INVALID
pattern at higher layers. It's unclear why you chose to change all at once, or
whether that's the right thing, but I suspect you had reasonings for it? This
might be an example of trying to find the right size of the CL and the right
end-state. It sounded like the expectation was there'd be additional discussion
re: integration with those bits?
> 
> I realize you published this as DO NOT REVIEW, but mostly, I'm trying to
understand your reasoning and approach to the problem, to see if there's the
opportunity for guidance or streamlining to make it easier for future CLs :)
> 
>
https://codereview.chromium.org/2208073002/diff/20001/net/cert/ct_verify_resu...
> File net/cert/ct_verify_result.h (right):
> 
>
https://codereview.chromium.org/2208073002/diff/20001/net/cert/ct_verify_resu...
> net/cert/ct_verify_result.h:25: // then switch to using that.
> Why? :)
> 
>
https://codereview.chromium.org/2208073002/diff/20001/net/cert/ct_verify_resu...
> net/cert/ct_verify_result.h:42: SCTAndStatusList invalid_scts;
> From a design standpoint, could you explain more your reasoning for this
change?
> 
> One approach could have been to leave it as an SCTList (that is, not surface
the additional details beyond this API point yet)
> 
> Another could have been to combine all three vectors into a single vector,
since you're now tracking SCTVerifyStatus with the SCT.
> 
> But it's unclear why you did a little of A and a little of B
> 
>
https://codereview.chromium.org/2208073002/diff/20001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2208073002/diff/20001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:90131: +  <int value="2"
label="SCT_STATUS_INVALID_SIGNATURE"/>
> This does the thing I mentioned on the thread that I didn't think we should do
:)
> 
> Which is, it changes the semantic meaning of this value (that is,
INVALID_SIGNATURE is a sub-category of INVALID)
> 
> The extent of implications here are primarily because it makes it difficult to
compare ver X to Y, because in version X, it means one thing, and version Y, it
means something different.

Apologies Ryan, I should have made it clear this CL is not meant to be reviewed
- I've simply exported my exploratory attempt at making this change so that I'd
have a base for discussion with estark (and the reason the CL description is
missing is that I've explained out-of-band what's in the CL to Emily).

I agree that the incomplete approach of having SCT status list for invalid
statuses, but not for valid/unknown logs statuses is not desirable. That
conclusion was captured in the bug
(https://bugs.chromium.org/p/chromium/issues/detail?id=634006).

[Ryan Sleevi, 2016-08-03 21:26:02]

On 2016/08/03 21:24:06, Eran Messeri wrote:
> Apologies Ryan, I should have made it clear this CL is not meant to be
reviewed
> - I've simply exported my exploratory attempt at making this change so that
I'd
> have a base for discussion with estark (and the reason the CL description is
> missing is that I've explained out-of-band what's in the CL to Emily).
> 
> I agree that the incomplete approach of having SCT status list for invalid
> statuses, but not for valid/unknown logs statuses is not desirable. That
> conclusion was captured in the bug
> (https://bugs.chromium.org/p/chromium/issues/detail?id=634006).

From the original message:

"I realize you published this as DO NOT REVIEW, but mostly, I'm trying to
understand your reasoning and approach to the problem, to see if there's the
opportunity for guidance or streamlining to make it easier for future CLs :)"