8000 Allow vetoing successful SSL certificate verification by gregnolle · Pull Request #7914 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allow vetoing successful SSL certificate verification #7914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Allow vetoing successful SSL certificate verification #7914

wants to merge 2 commits into from

Conversation

gregnolle
Copy link
Contributor

First go at resolving #7891.

/cc @deepak1556: Thanks for your guidance in the issue report. Would you mind taking a look at this and see if you can see any issues please? I removed the call into the CT delegate that I see you added in #7651 since I don't want to bypass CT checking. I'm not clear exactly when CT checking happens though (before or after the CertVerifier::Verify call?) and perhaps there should still be a way of bypassing it?

I thought about maybe changing the interpretation of the callback value to:

  • true - resets the verify_result, bypass CT and send back OK result
  • false - send back ERR_FAILED result regardless of default cert verifier's result
  • null - send back whatever result the default cert verifier sent and let CT checking happen normally; this assumes that CT checking happens afterwards

What do you think?

@deepak1556 deepak1556 self-assigned this Nov 10, 2016
@deepak1556
Copy link
Member

Thanks for picking this up.

I'm not clear exactly when CT checking happens though (before or after the CertVerifier::Verify call?) and perhaps there should still be a way of bypassing it?

It will happen after the verify call, you can look at this state machine https://cs.chromium.org/chromium/src/net/socket/ssl_client_socket_impl.cc?sq=package:chromium&l=1312.

I thought about maybe changing the interpretation of the callback value to:

  • true - resets the verify_result, bypass CT and send back OK result
  • false - send back ERR_FAILED result regardless of default cert verifier's result
  • null - send back whatever result the default cert verifier sent and let CT checking happen normally; this assumes that CT checking happens afterwards

This looks good, but instead of null we can just expect a second boolean argument whether to default to chromium verification result. This argument will be false by default to maintain backwards compatibility.

I have some style changes to this PR locally, will push them today.

@deepak1556
Copy link
Member

@gregnolle have made the changes https://github.com/deepak1556/atom-shell/commits/veto-ssl-verify , if it looks good I can either push to your branch given the permission or create a new pull request. Thanks!

@gregnolle
Copy link
Contributor Author

@deepak1556 thanks very much for helping with this. I was hoping to find time during the week to work on it again, but didn't get the chance. Your implementation looks much better though. I'm happy to close off my pull request and you can do one for your branch.

@gregnolle gregnolle closed this Nov 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@deepak1556 deepak1556

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gregnolle @deepak1556
0