Certificate Transparency: Add Venafi log

Certificate Transparency: Add Venafi log

- Adds Venafi log to known CT logs.
- Expands log key size to accommodate large Venafi public key.
- Calculates number of known logs, rather than hard-coding.

R=rsleevi@chromium.org
BUG=499446
Committed: https://crrev.com/2d76d6f84658353d00d6c9a1a4f164df8478ada6
Cr-Commit-Position: refs/heads/master@{#349170}

[Ryan Sleevi, 2015-09-14 21:15:25]

No good deed goes unpunished. :)

Thanks for putting this together, but changes since the giant jump in key size
makes me a little sad as we try to trim the Chrome fat everywhere we can...

https://codereview.chromium.org/1338393002/diff/1/net/cert/ct_known_logs_stat...
File net/cert/ct_known_logs_static.h (right):

https://codereview.chromium.org/1338393002/diff/1/net/cert/ct_known_logs_stat...
net/cert/ct_known_logs_static.h:10: const char log_key[295];
I'm wondering whether or not this string should be externalized. The result is
that you're allocating 295 bytes now for every log, but that's kinda gross.

That is, something like

const char kAviatorKey[] = {...};
const char kVenafiKey[] = {...};

struct CTLogInfo {
  const char* const log_key;
  size_t log_key_length;
  const char* const log_name;
  const char* const log_url;
};

const CTLogInfo kCTLogList[] = {
  { kAviatorKey, sizeof(kAviatorKey), ... },
   ...,
  { kVenafiKey, sizeof(kVenafiKey), ... },
};



(We should also consider renaming this from .h to something like -inc.cc /
-inc.h, to make it clear it's not meant to be a generic header, so that we don't
get into multi-translation unit size bloat from having the same constant in
multiple TUs)

https://codereview.chromium.org/1338393002/diff/1/net/cert/ct_known_logs_stat...
net/cert/ct_known_logs_static.h:93: const size_t kNumKnownCTLogs =
sizeof(kCTLogList) / sizeof(CTLogInfo);
I've mentioned to Eran this wasn't needed. :) Chrome has a macro (arraysize)
that handles this. 

This constant is not used anywhere in Chrome (see
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_known_...
), so you could just as well remove it. I thought he kept it - and its form -
for potential other consumers, in which case, it seems to make no difference
about hardcoding or not, since it's all generated. Hardcoding makes it ever so
slightly less work for the compiler :P

[Rob Percival, 2015-09-15 14:44:08]

On 2015/09/14 21:15:25, Ryan Sleevi wrote:
> No good deed goes unpunished. :)
> 
> Thanks for putting this together, but changes since the giant jump in key size
> makes me a little sad as we try to trim the Chrome fat everywhere we can...
> 
>
https://codereview.chromium.org/1338393002/diff/1/net/cert/ct_known_logs_stat...
> File net/cert/ct_known_logs_static.h (right):
> 
>
https://codereview.chromium.org/1338393002/diff/1/net/cert/ct_known_logs_stat...
> net/cert/ct_known_logs_static.h:10: const char log_key[295];
> I'm wondering whether or not this string should be externalized. The result is
> that you're allocating 295 bytes now for every log, but that's kinda gross.
> 
> That is, something like
> 
> const char kAviatorKey[] = {...};
> const char kVenafiKey[] = {...};
> 
> struct CTLogInfo {
>   const char* const log_key;
>   size_t log_key_length;
>   const char* const log_name;
>   const char* const log_url;
> };
> 
> const CTLogInfo kCTLogList[] = {
>   { kAviatorKey, sizeof(kAviatorKey), ... },
>    ...,
>   { kVenafiKey, sizeof(kVenafiKey), ... },
> };
> 
> 
> 
> (We should also consider renaming this from .h to something like -inc.cc /
> -inc.h, to make it clear it's not meant to be a generic header, so that we
don't
> get into multi-translation unit size bloat from having the same constant in
> multiple TUs)
> 
>
https://codereview.chromium.org/1338393002/diff/1/net/cert/ct_known_logs_stat...
> net/cert/ct_known_logs_static.h:93: const size_t kNumKnownCTLogs =
> sizeof(kCTLogList) / sizeof(CTLogInfo);
> I've mentioned to Eran this wasn't needed. :) Chrome has a macro (arraysize)
> that handles this. 
> 
> This constant is not used anywhere in Chrome (see
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/ct_known_...
> ), so you could just as well remove it. I thought he kept it - and its form -
> for potential other consumers, in which case, it seems to make no difference
> about hardcoding or not, since it's all generated. Hardcoding makes it ever so
> slightly less work for the compiler :P

Thanks for reviewing this so quickly Ryan. Since the file is generated, the
simplest solution was to have the script explicitly specify the key length when
initializing kCTLogList, which avoided the need for a separate list of key
variables. What do you think? I've left the hard-coded length in there for now.

As for renaming it to "-inc.h"/"-inc.cc", wouldn't there be a linker error if
anyone attempted to include this into more than one TU? That should prevent
unintentional bloating of multiple TUs.

[Ryan Sleevi, 2015-09-15 17:59:13]

On 2015/09/15 14:44:08, Rob Percival wrote:
> Thanks for reviewing this so quickly Ryan. Since the file is generated, the
> simplest solution was to have the script explicitly specify the key length
when
> initializing kCTLogList, which avoided the need for a separate list of key
> variables. What do you think? I've left the hard-coded length in there for
now.

Works for me.

> As for renaming it to "-inc.h"/"-inc.cc", wouldn't there be a linker error if
> anyone attempted to include this into more than one TU? That should prevent
> unintentional bloating of multiple TUs.

Nope. This doesn't necessarily run afoul of the ODR rule because const implies
static linkage. So you'd end up with multiple (internal to a TU) copies. The way
to resolve that is to explicitly make the linkage external.

[Ryan Sleevi, 2015-09-15 18:03:54]

LGTM to this CL modulo one nit (that affects whether the data ends up in rodata
or not); we can follow-up separately on the ODR issue, since you've resolved the
immediate size concerns.

https://codereview.chromium.org/1338393002/diff/20001/net/cert/ct_known_logs_...
File net/cert/ct_known_logs_static.h (right):

https://codereview.chromium.org/1338393002/diff/20001/net/cert/ct_known_logs_...
net/cert/ct_known_logs_static.h:10: const char* log_key;
const char* const

[Rob Percival, 2015-09-16 16:25:28]

Nit fixed.

[Ryan Sleevi, 2015-09-16 16:30:02]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-09-16 16:30:02]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1338393002/#ps40001
(title: "Store CT log info in rodata (nit fix)")

[commit-bot: I haz the power, 2015-09-16 16:30:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1338393002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1338393002/40001

[Eran Messeri, 2015-09-16 16:49:14]

The CQ bit was checked by eranm@chromium.org

[Eran Messeri, 2015-09-16 16:49:15]

The CQ bit was unchecked by eranm@chromium.org

[Rob Percival, 2015-09-16 17:53:56]

The CQ bit was checked by robpercival@chromium.org

[commit-bot: I haz the power, 2015-09-16 17:54:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1338393002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1338393002/40001

[commit-bot: I haz the power, 2015-09-16 18:36:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-16 18:38:03]

Patchset 3 (id:??) landed as
https://crrev.com/2d76d6f84658353d00d6c9a1a4f164df8478ada6
Cr-Commit-Position: refs/heads/master@{#349170}

[commit-bot: I haz the power, 2015-09-23 12:58:22]

Patchset 3 (id:??) landed as
https://crrev.com/2d76d6f84658353d00d6c9a1a4f164df8478ada6
Cr-Commit-Position: refs/heads/master@{#349170}