Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Add verifiers for known certificate transparency logs #249

Merged
merged 2 commits into from
Sep 21, 2016

Conversation

kevinsawicki
Copy link
Contributor

@kevinsawicki kevinsawicki commented Sep 21, 2016

This pull request implements option 3 from #248 to fix the net::ERR_INSECURE_RESPONSE error that are happening in Electron 1.4.0 for certain requests.

/cc @deepak1556 @sleevi do you think this is right approach until Brightray can use URLRequestContextBuilder directly?

Closes #248
Refs electron/electron#7221
Upstream pull request electron/electron#7292

cert_transparency_verifier_.reset(new net::MultiLogCTVerifier());
auto multi_log_ct_verifier = new net::MultiLogCTVerifier();
multi_log_ct_verifier->AddLogs(net::ct::CreateLogVerifiersForKnownLogs());
cert_transparency_verifier_.reset(multi_log_ct_verifier);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C++ Style wise, I'd encourage a pattern more like

cert_transparency_verifier_.reset(new net::MultiLogCTVerifier());
cert_transparency_verifier_->AddLogs(net::ct::CreateLogVerifiersForKnownLogs());

This is preferred in general because should AddLogs throw an exception (which, admittedly, Chromium disables, but I'm speaking to the general C++ rule), then you'd end up with a memory leak, but if you assign to the unique_ptr<> first, then if AddLogs were to throw, you'd still end up with a graceful cleanup of memory.

But this is totally the Right Thing to do until you use a URLRequestContextBuilder, so looks good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sleevi thanks for the feedback, I've pushed up a new commit that incorporates it 👍

@kevinsawicki kevinsawicki changed the title Add verifiers for known logs Add verifiers for known certificate transparency logs Sep 21, 2016
@deepak1556
Copy link
Contributor

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A default CTPolicyEnforcer is initialized without populating the CTLogVerifier
3 participants