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

session: provide chromium cert verification result to setCertificateVerifiy callback #7955

Merged
merged 8 commits into from Feb 8, 2017

Conversation

Projects
None yet
3 participants
@deepak1556
Member

deepak1556 commented Nov 13, 2016

Fixes #7891

Not sure if i have deprecated the old method the right way, would like some help with it.

/cc @kevinsawicki

@gregnolle

This comment has been minimized.

Show comment
Hide comment
@gregnolle

gregnolle Nov 21, 2016

Contributor

I just tried to build this (after merging with master) and get this compile error:

FAILED: ../../vendor/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/atom/browser/net/electron_lib.atom_cert_verifier.o.d '-DATOM_PRODUCT_NAME="Electron"' '-DATOM_PROJECT_NAME="electron"' -DNODE_WANT_INTERNALS=1 -DNODE_SHARED_MODE -DSK_SUPPORT_LEGACY_GETTOPDEVICE -DGLIB_DISABLE_DEPRECATION_WARNINGS -DV8_USE_EXTERNAL_STARTUP_DATA -DENABLE_PLUGINS -DENABLE_PEPPER_CDMS -DUSE_PROPRIETARY_CODECS -DENABLE_WEBRTC -DNDEBUG -DSK_SUPPORT_LEGACY_BITMAP_CONFIG -DSK_SUPPORT_LEGACY_DEVICE_VIRTUAL_ISOPAQUE -DSK_SUPPORT_LEGACY_N32_NAME -DSK_SUPPORT_LEGACY_SETCONFIG -DSK_IGNORE_ETC1_SUPPORT -DSK_IGNORE_GPU_DITHER -DDISABLE_NACL -DUSE_OPENSSL -DWEBRTC_MAC -I../.. -I../../chromium_src -I../../vendor/brightray -I../../vendor/native_mate -Igen -I../../vendor/node/src -I../../vendor/node/deps/http_parser -I../../vendor/node/deps/uv/include -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/v8/include -I../../vendor/node/deps/cares/include -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/WebKit/Source -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/libyuv/include -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/components/cdm -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/widevine -I../../vendor -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/gpu -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/skia/config -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/skia/include/core -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/skia/include/config -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/icu/source/common -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/mojo/src -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/khronos -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/WebKit -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/static_library/gen -I../../vendor/crashpad -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -fasm-blocks -mpascal-strings -O2 -gdwarf-2 -Werror -mmacosx-version-min=10.9 -arch x86_64 -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wno-deprecated-declarations -Wno-undefined-var-template -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -F/Users/greg/Projects/electron/external_binaries -std=c++11 -stdlib=libc++ -fno-rtti -fno-exceptions -fcolor-diagnostics -fno-inline -fno-omit-frame-pointer -fno-builtin -fno-optimize-sibling-calls  -c ../../atom/browser/net/atom_cert_verifier.cc -o obj/atom/browser/net/electron_lib.atom_cert_verifier.o
../../atom/browser/net/atom_cert_verifier.cc:47:9: error: field 'cert_verifier_' will be initialized after field 'error_' [-Werror,-Wreorder]
        cert_verifier_(cert_verifier),
        ^
1 error generated.
[325/482] CXX obj/atom/browser/net/electron_lib.atom_network_delegate.o
ninja: build stopped: subcommand failed.

Any ideas?

Contributor

gregnolle commented Nov 21, 2016

I just tried to build this (after merging with master) and get this compile error:

FAILED: ../../vendor/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/atom/browser/net/electron_lib.atom_cert_verifier.o.d '-DATOM_PRODUCT_NAME="Electron"' '-DATOM_PROJECT_NAME="electron"' -DNODE_WANT_INTERNALS=1 -DNODE_SHARED_MODE -DSK_SUPPORT_LEGACY_GETTOPDEVICE -DGLIB_DISABLE_DEPRECATION_WARNINGS -DV8_USE_EXTERNAL_STARTUP_DATA -DENABLE_PLUGINS -DENABLE_PEPPER_CDMS -DUSE_PROPRIETARY_CODECS -DENABLE_WEBRTC -DNDEBUG -DSK_SUPPORT_LEGACY_BITMAP_CONFIG -DSK_SUPPORT_LEGACY_DEVICE_VIRTUAL_ISOPAQUE -DSK_SUPPORT_LEGACY_N32_NAME -DSK_SUPPORT_LEGACY_SETCONFIG -DSK_IGNORE_ETC1_SUPPORT -DSK_IGNORE_GPU_DITHER -DDISABLE_NACL -DUSE_OPENSSL -DWEBRTC_MAC -I../.. -I../../chromium_src -I../../vendor/brightray -I../../vendor/native_mate -Igen -I../../vendor/node/src -I../../vendor/node/deps/http_parser -I../../vendor/node/deps/uv/include -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/v8/include -I../../vendor/node/deps/cares/include -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/WebKit/Source -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/libyuv/include -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/components/cdm -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/widevine -I../../vendor -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/gpu -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/skia/config -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/skia/include/core -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/skia/include/config -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/icu/source/common -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/mojo/src -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/khronos -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/src/third_party/WebKit -I/Users/greg/Projects/electron/vendor/brightray/vendor/download/libchromiumcontent/static_library/gen -I../../vendor/crashpad -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -fasm-blocks -mpascal-strings -O2 -gdwarf-2 -Werror -mmacosx-version-min=10.9 -arch x86_64 -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -Wno-deprecated-declarations -Wno-undefined-var-template -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -F/Users/greg/Projects/electron/external_binaries -std=c++11 -stdlib=libc++ -fno-rtti -fno-exceptions -fcolor-diagnostics -fno-inline -fno-omit-frame-pointer -fno-builtin -fno-optimize-sibling-calls  -c ../../atom/browser/net/atom_cert_verifier.cc -o obj/atom/browser/net/electron_lib.atom_cert_verifier.o
../../atom/browser/net/atom_cert_verifier.cc:47:9: error: field 'cert_verifier_' will be initialized after field 'error_' [-Werror,-Wreorder]
        cert_verifier_(cert_verifier),
        ^
1 error generated.
[325/482] CXX obj/atom/browser/net/electron_lib.atom_network_delegate.o
ninja: build stopped: subcommand failed.

Any ideas?

@deepak1556

This comment has been minimized.

Show comment
Hide comment
@deepak1556

deepak1556 Nov 22, 2016

Member

@gregnolle should work now.

Member

deepak1556 commented Nov 22, 2016

@gregnolle should work now.

@gregnolle

This comment has been minimized.

Show comment
Hide comment
@gregnolle

gregnolle Nov 23, 2016

Contributor

That did the trick, thanks!

Contributor

gregnolle commented Nov 23, 2016

That did the trick, thanks!

@gregnolle

This comment has been minimized.

Show comment
Hide comment
@gregnolle

gregnolle Dec 2, 2016

Contributor

Any news on when this might be merged? It's working well for me.

Contributor

gregnolle commented Dec 2, 2016

Any news on when this might be merged? It's working well for me.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Dec 2, 2016

Contributor

Any news on when this might be merged? It's working well for me.

No news right now, still needs to be reviewed.

Contributor

kevinsawicki commented Dec 2, 2016

Any news on when this might be merged? It's working well for me.

No news right now, still needs to be reviewed.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 8, 2017

Contributor

Apologies for the long delay in reviewing this. I've rebased this branch on top of master.

@deepak1556 I pushed a change I'd like to get your thoughts on. It switches the first argument to the proc to be a request object that contains the hostname, cert, and verification result in it. This way we can add future request values without breaking the signature and instead just add new properties to the object. Can you take a look at the last commit and let me know if you are okay with it?

Contributor

kevinsawicki commented Feb 8, 2017

Apologies for the long delay in reviewing this. I've rebased this branch on top of master.

@deepak1556 I pushed a change I'd like to get your thoughts on. It switches the first argument to the proc to be a request object that contains the hostname, cert, and verification result in it. This way we can add future request values without breaking the signature and instead just add new properties to the object. Can you take a look at the last commit and let me know if you are okay with it?

@kevinsawicki kevinsawicki changed the title from session: provide chromium cert verification result to setCertificateVerifiy calback to session: provide chromium cert verification result to setCertificateVerifiy callback Feb 8, 2017

@kevinsawicki kevinsawicki merged commit bad130f into electron:master Feb 8, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 8, 2017

Contributor

Thanks for this @deepak1556 👍 🚢 :shipit:

Contributor

kevinsawicki commented Feb 8, 2017

Thanks for this @deepak1556 👍 🚢 :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment