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.

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

Expose whole certificate chain to verify proc #7917

Merged
merged 9 commits into from Nov 16, 2016

Conversation

Projects
None yet
5 participants
@gregnolle
Contributor

gregnolle commented Nov 9, 2016

The existing Certificate structure only contains the certificate at the end of the chain. This PR exposes the full chain of issuers, and also adds full details of subject principal (beyond just the common name).

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Nov 9, 2016

Contributor

Would you mind updating the existing specs to assert these new fields?

Contributor

kevinsawicki commented Nov 9, 2016

Would you mind updating the existing specs to assert these new fields?

@@ -0,0 +1,8 @@
# CertificatePrincipal Object

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Nov 10, 2016

Member

/cc @zeke Needs to end up in seeds.js

@MarshallOfSound

MarshallOfSound Nov 10, 2016

Member

/cc @zeke Needs to end up in seeds.js

This comment has been minimized.

@zeke

zeke Nov 10, 2016

Member

Ack. Thanks for pointing this out.

@zeke

zeke Nov 10, 2016

Member

Ack. Thanks for pointing this out.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Nov 10, 2016

Member

Keen for #6931 so we can stop caring about adding these every time 😄

@MarshallOfSound

MarshallOfSound Nov 10, 2016

Member

Keen for #6931 so we can stop caring about adding these every time 😄

@zeke zeke referenced this pull request Nov 10, 2016

Closed

add CertificatePrincipal #72

@gregnolle

This comment has been minimized.

Show comment
Hide comment
@gregnolle

gregnolle Nov 12, 2016

Contributor

I wasn't sure how far to go with the assertions since I see there a number of the existing fields don't have them in the specs. I've covered the same fields that are in the new structures though.

Contributor

gregnolle commented Nov 12, 2016

I wasn't sure how far to go with the assertions since I see there a number of the existing fields don't have them in the specs. I've covered the same fields that are in the new structures though.

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Nov 12, 2016

Member

@gregnolle it looks like there are few linting errors:

  /home/travis/build/electron/electron/spec/api-app-spec.js:211:31: Extra semicolon.
  /home/travis/build/electron/electron/spec/api-app-spec.js:213:32: Extra semicolon.
  /home/travis/build/electron/electron/spec/api-session-spec.js:557:35: Extra semicolon.
  /home/travis/build/electron/electron/spec/api-session-spec.js:559:36: Extra semicolon.
Member

zeke commented Nov 12, 2016

@gregnolle it looks like there are few linting errors:

  /home/travis/build/electron/electron/spec/api-app-spec.js:211:31: Extra semicolon.
  /home/travis/build/electron/electron/spec/api-app-spec.js:213:32: Extra semicolon.
  /home/travis/build/electron/electron/spec/api-session-spec.js:557:35: Extra semicolon.
  /home/travis/build/electron/electron/spec/api-session-spec.js:559:36: Extra semicolon.
@gregnolle

This comment has been minimized.

Show comment
Hide comment
@gregnolle

gregnolle Nov 15, 2016

Contributor

I don't think the Travis build failure is due to anything on my branch (seems to be failing early on with an APT issue). Is there a way to trigger a rebuild?

Contributor

gregnolle commented Nov 15, 2016

I don't think the Travis build failure is due to anything on my branch (seems to be failing early on with an APT issue). Is there a way to trigger a rebuild?

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke Nov 15, 2016

Member

restarted travis

Member

zeke commented Nov 15, 2016

restarted travis

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Nov 16, 2016

Contributor

@gregnolle thanks for this, rebased it and tweaked a few of the asserts.

The issueCert was null on the select-client-certificate event so I removed the assert for it to get the build passing. It was available on the setCertificateVerifyProc callback though.

Going to merge this in 👍 🚢

Contributor

kevinsawicki commented Nov 16, 2016

@gregnolle thanks for this, rebased it and tweaked a few of the asserts.

The issueCert was null on the select-client-certificate event so I removed the assert for it to get the build passing. It was available on the setCertificateVerifyProc callback though.

Going to merge this in 👍 🚢

@kevinsawicki kevinsawicki merged commit 9624bc1 into electron:master Nov 16, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment