131734 2014-04-16 08:40:58 -0700 [EFL][WK1] SSL Strict is set according to input parameter. 2014-04-17 18:11:30 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) All Linux RESOLVED FIXED P2 Normal --- 1 je_julie.kim webkit-unassigned bunhere cdumez commit-queue gyuyoung.kim gyuyoung.kim lucas.de.marchi sergio oldest_to_newest 1001143 0 je_julie.kim 2014-04-16 08:40:58 -0700 SSL-Strict is set regardless of input param from ewk_network_tls_certificate_check_set. It should be set with input param, 'checkCertificates'. 1001145 1 je_julie.kim 2014-04-16 08:43:15 -0700 For information, ewk_network_tls_certificate_check_set/get APIs were added at https://bugs.webkit.org/show_bug.cgi?id=74299. 1001148 2 229445 je_julie.kim 2014-04-16 08:50:28 -0700 Created attachment 229445 Patch 1001383 3 229445 gyuyoung.kim 2014-04-16 16:20:22 -0700 Comment on attachment 229445 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229445&action=review > Source/WebKit/efl/ChangeLog:10 > + Otherwise, SSLStrict is unset. Unneeded line break. > Source/WebKit/efl/ewk/ewk_network.cpp:56 > void ewk_network_tls_certificate_check_set(Eina_Bool checkCertificates) BTW, parameter name is different with public header. Public header has used *enable* EAPI void ewk_network_tls_certificate_check_set(Eina_Bool enable); > Source/WebKit/efl/ewk/ewk_network.cpp:58 > unsigned policy = WebCore::SoupNetworkSession::defaultSession().sslPolicy(); Now I understand this API behavior is to enable soup SSL policy if checkCertificates is enabled. However, it looks this API is getting current ssl policy value unnecessary because you set *policy* value only depends on *checkCertificates*. I think there are two choices. One is just to set the policy regardless of current policy value. The other is to enable only both when *checkCertificates* is enabled and *current policy value* is disabled. Below may be one of examples for first one. void ewk_network_tls_certificate_check_set(Eina_Bool checkCertificates) { if (checkCertificates) policy = WebCore::SoupNetworkSession::SSLStrict; else policy = ~WebCore::SoupNetworkSession::SSLStrict; WebCore::SoupNetworkSession::defaultSession().setSSLPolicy(policy); } 1001492 4 je_julie.kim 2014-04-16 22:33:05 -0700 (In reply to comment #3) > EAPI void ewk_network_tls_certificate_check_set(Eina_Bool enable); > > > Source/WebKit/efl/ewk/ewk_network.cpp:58 > > unsigned policy = WebCore::SoupNetworkSession::defaultSession().sslPolicy(); > > Now I understand this API behavior is to enable soup SSL policy if checkCertificates is enabled. However, it looks this API is getting current ssl policy value unnecessary because you set *policy* value only depends on *checkCertificates*. I think there are two choices. One is just to set the policy regardless of current policy value. The other is to enable only both when *checkCertificates* is enabled and *current policy value* is disabled. > > Below may be one of examples for first one. > > void ewk_network_tls_certificate_check_set(Eina_Bool checkCertificates) { > if (checkCertificates) > policy = WebCore::SoupNetworkSession::SSLStrict; > else > policy = ~WebCore::SoupNetworkSession::SSLStrict; > > WebCore::SoupNetworkSession::defaultSession().setSSLPolicy(policy); > } Thanks, Gyuyoung. SoupNetworkSession::SSLPolicy SoupNetworkSession::sslPolicy() includes two options, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE and SOUP_SESSION_SSL_STRICT. We can set both of them using setSSLPolicy. That's why I'm getting current value. Originally, ewk_network_tls_certificate_check_set is only related to SSL-Strict. As to the second suggestion, It means that if (checkCertificates && !(policy | WebCore::SoupNetworkSession::SSLStrict)) policy |= WebCore::SoupNetworkSession::SSLStrict; else if (!checkCertificates && (policy | WebCore::SoupNetworkSession::SSLStrict)) policy &= ~WebCore::SoupNetworkSession::SSLStrict; So, your suggestion is that? 1001493 5 gyuyoung.kim 2014-04-16 22:39:18 -0700 (In reply to comment #4) > (In reply to comment #3) > > EAPI void ewk_network_tls_certificate_check_set(Eina_Bool enable); > > > > > Source/WebKit/efl/ewk/ewk_network.cpp:58 > > > unsigned policy = WebCore::SoupNetworkSession::defaultSession().sslPolicy(); > > > > Now I understand this API behavior is to enable soup SSL policy if checkCertificates is enabled. However, it looks this API is getting current ssl policy value unnecessary because you set *policy* value only depends on *checkCertificates*. I think there are two choices. One is just to set the policy regardless of current policy value. The other is to enable only both when *checkCertificates* is enabled and *current policy value* is disabled. > > > > Below may be one of examples for first one. > > > > void ewk_network_tls_certificate_check_set(Eina_Bool checkCertificates) { > > if (checkCertificates) > > policy = WebCore::SoupNetworkSession::SSLStrict; > > else > > policy = ~WebCore::SoupNetworkSession::SSLStrict; > > > > WebCore::SoupNetworkSession::defaultSession().setSSLPolicy(policy); > > } > > Thanks, Gyuyoung. > SoupNetworkSession::SSLPolicy SoupNetworkSession::sslPolicy() includes two options, SOUP_SESSION_SSL_USE_SYSTEM_CA_FILE and SOUP_SESSION_SSL_STRICT. > We can set both of them using setSSLPolicy. That's why I'm getting current value. > Originally, ewk_network_tls_certificate_check_set is only related to SSL-Strict. > As to the second suggestion, It means that > if (checkCertificates && !(policy | WebCore::SoupNetworkSession::SSLStrict)) > policy |= WebCore::SoupNetworkSession::SSLStrict; > else if (!checkCertificates && (policy | WebCore::SoupNetworkSession::SSLStrict)) > policy &= ~WebCore::SoupNetworkSession::SSLStrict; > So, your suggestion is that? Looks like my second suggestion. My point is that we don't need to get current policy value in your uploaded patch, because your patch sets the policy value by using *checkCertificates* regardless current policy value. So, when the policy was already set to SSLStrict, we don't need to set it to SSLStrict again. 1001533 6 229539 je_julie.kim 2014-04-17 06:49:46 -0700 Created attachment 229539 Patch 1001538 7 je_julie.kim 2014-04-17 07:11:12 -0700 (In reply to comment #5) > Looks like my second suggestion. My point is that we don't need to get current policy value in your uploaded patch, because your patch sets the policy value by using *checkCertificates* regardless current policy value. So, when the policy was already set to SSLStrict, we don't need to set it to SSLStrict again. Hi Gyuyoung, Thank you for your comment. I updated patch. If you are available, please look into it. Thanks, 1001540 8 229539 gyuyoung.kim 2014-04-17 07:21:53 -0700 Comment on attachment 229539 Patch LGTM. 1001792 9 229539 commit-queue 2014-04-17 18:11:22 -0700 Comment on attachment 229539 Patch Clearing flags on attachment: 229539 Committed r167475: <http://trac.webkit.org/changeset/167475> 1001793 10 commit-queue 2014-04-17 18:11:30 -0700 All reviewed patches have been landed. Closing bug. 229445 2014-04-16 08:50:28 -0700 2014-04-17 06:49:33 -0700 Patch bug-131734-20140417005001.patch text/plain 1841 je_julie.kim U3VidmVyc2lvbiBSZXZpc2lvbjogMTY3MzQ2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Vm bC9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0L2VmbC9DaGFuZ2VMb2cKaW5kZXggM2M1N2U4NGMy YThlYTk5MjVmMDU3MmQ1NWUzNzY3MDA4OGIxNDEyZC4uNjgyM2MwMzQ0OGMxMmUyZjQ0NzFjMzFl NGM1OTZiNWYxNTMyYzRkOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9lZmwvQ2hhbmdlTG9n CisrKyBiL1NvdXJjZS9XZWJLaXQvZWZsL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE5IEBACiAyMDE0 LTA0LTE2ICBKZW9uZ2V1biBLaW0gIDxqZV9qdWxpZS5raW1Ac2Ftc3VuZy5jb20+CiAKKyAgICAg ICAgW0VGTF1bV0sxXSBTU0wgU3RyaWN0IGlzIHNldCBhY2NvcmRpbmcgdG8gaW5wdXQgcGFyYW1l dGVyLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTMx NzM0LgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRo ZSBwYXJhbSwgY2hlY2tDZXJ0aWZpY2F0ZXMsIHNob3VsZCBiZSB1c2VkIGZvciBzZXR0aW5nIFNT TC1TdHJpY3QuCisgICAgICAgIElmIGl0IGlzIHRydWUsIFNTTFN0cmljdCBpcyBzZXQuCisgICAg ICAgIE90aGVyd2lzZSwgU1NMU3RyaWN0IGlzIHVuc2V0LgorCisgICAgICAgICogZXdrL2V3a19u ZXR3b3JrLmNwcDoKKyAgICAgICAgKGV3a19uZXR3b3JrX3Rsc19jZXJ0aWZpY2F0ZV9jaGVja19z ZXQpOgorCisyMDE0LTA0LTE2ICBKZW9uZ2V1biBLaW0gIDxqZV9qdWxpZS5raW1Ac2Ftc3VuZy5j b20+CisKICAgICAgICAgW0VGTF0gQ2xlYW51cCB0aGUgYnVpbGQgZnJvbSB1bnVzZWQgcGFyYW1l dGVycyBpbiBXZWJLaXQtZWZsLgogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTMxNjc3LgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2VmbC9ld2sv ZXdrX25ldHdvcmsuY3BwIGIvU291cmNlL1dlYktpdC9lZmwvZXdrL2V3a19uZXR3b3JrLmNwcApp bmRleCBhN2MyMWY2ZjBkNGQwNDcxY2NiOWI3MjExOWZiYWQ5OTA4OTYyZDFiLi4wOWU2NWEyYjE5 MzMwZjYyNmM0MDBiNGUyYzMzMjkzNmU4NzQzNDI5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L2VmbC9ld2svZXdrX25ldHdvcmsuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvZWZsL2V3ay9ld2tf bmV0d29yay5jcHAKQEAgLTU2LDcgKzU2LDExIEBAIEVpbmFfQm9vbCBld2tfbmV0d29ya190bHNf Y2VydGlmaWNhdGVfY2hlY2tfZ2V0KCkKIHZvaWQgZXdrX25ldHdvcmtfdGxzX2NlcnRpZmljYXRl X2NoZWNrX3NldChFaW5hX0Jvb2wgY2hlY2tDZXJ0aWZpY2F0ZXMpCiB7CiAgICAgdW5zaWduZWQg cG9saWN5ID0gV2ViQ29yZTo6U291cE5ldHdvcmtTZXNzaW9uOjpkZWZhdWx0U2Vzc2lvbigpLnNz bFBvbGljeSgpOwotICAgIFdlYkNvcmU6OlNvdXBOZXR3b3JrU2Vzc2lvbjo6ZGVmYXVsdFNlc3Np b24oKS5zZXRTU0xQb2xpY3kocG9saWN5IHwgV2ViQ29yZTo6U291cE5ldHdvcmtTZXNzaW9uOjpT U0xTdHJpY3QpOworICAgIGlmIChjaGVja0NlcnRpZmljYXRlcykKKyAgICAgICAgcG9saWN5IHw9 IFdlYkNvcmU6OlNvdXBOZXR3b3JrU2Vzc2lvbjo6U1NMU3RyaWN0OworICAgIGVsc2UKKyAgICAg ICAgcG9saWN5ICY9IH5XZWJDb3JlOjpTb3VwTmV0d29ya1Nlc3Npb246OlNTTFN0cmljdDsKKyAg ICBXZWJDb3JlOjpTb3VwTmV0d29ya1Nlc3Npb246OmRlZmF1bHRTZXNzaW9uKCkuc2V0U1NMUG9s aWN5KHBvbGljeSk7CiB9CiAKIGNvbnN0IGNoYXIqIGV3a19uZXR3b3JrX3Rsc19jYV9jZXJ0aWZp Y2F0ZXNfcGF0aF9nZXQoKQo= 229539 2014-04-17 06:49:46 -0700 2014-04-17 18:11:22 -0700 Patch bug-131734-20140417224919.patch text/plain 2061 je_julie.kim U3VidmVyc2lvbiBSZXZpc2lvbjogMTY3MzQ2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Vm bC9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0L2VmbC9DaGFuZ2VMb2cKaW5kZXggM2M1N2U4NGMy YThlYTk5MjVmMDU3MmQ1NWUzNzY3MDA4OGIxNDEyZC4uNDk0NTU0ZWI1YjA5MjQ1YzI2NjljMWNm MGIwZGM0MGY5OTUyMzFmYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9lZmwvQ2hhbmdlTG9n CisrKyBiL1NvdXJjZS9XZWJLaXQvZWZsL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE4IEBACiAyMDE0 LTA0LTE2ICBKZW9uZ2V1biBLaW0gIDxqZV9qdWxpZS5raW1Ac2Ftc3VuZy5jb20+CiAKKyAgICAg ICAgW0VGTF1bV0sxXSBTU0wgU3RyaWN0IGlzIHNldCBhY2NvcmRpbmcgdG8gaW5wdXQgcGFyYW1l dGVyLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTMx NzM0LgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRo ZSBwYXJhbSwgJ2VuYWJsZScsIHNob3VsZCBiZSB1c2VkIGZvciBzZXR0aW5nIFNTTC1TdHJpY3Qu CisgICAgICAgIElmIGl0IGlzIHRydWUsIFNTTFN0cmljdCBpcyBzZXQuIE90aGVyd2lzZSwgU1NM U3RyaWN0IGlzIHVuc2V0LgorCisgICAgICAgICogZXdrL2V3a19uZXR3b3JrLmNwcDoKKyAgICAg ICAgKGV3a19uZXR3b3JrX3Rsc19jZXJ0aWZpY2F0ZV9jaGVja19zZXQpOgorCisyMDE0LTA0LTE2 ICBKZW9uZ2V1biBLaW0gIDxqZV9qdWxpZS5raW1Ac2Ftc3VuZy5jb20+CisKICAgICAgICAgW0VG TF0gQ2xlYW51cCB0aGUgYnVpbGQgZnJvbSB1bnVzZWQgcGFyYW1ldGVycyBpbiBXZWJLaXQtZWZs LgogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTMxNjc3 LgogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2VmbC9ld2svZXdrX25ldHdvcmsuY3BwIGIv U291cmNlL1dlYktpdC9lZmwvZXdrL2V3a19uZXR3b3JrLmNwcAppbmRleCBhN2MyMWY2ZjBkNGQw NDcxY2NiOWI3MjExOWZiYWQ5OTA4OTYyZDFiLi45ZTFiMWVjNjIzZTY3Y2M2OTc5ODMyMzhmMGUw NjIzMzEyYWU1M2FhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2VmbC9ld2svZXdrX25ldHdv cmsuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvZWZsL2V3ay9ld2tfbmV0d29yay5jcHAKQEAgLTUz LDEwICs1MywxNCBAQCBFaW5hX0Jvb2wgZXdrX25ldHdvcmtfdGxzX2NlcnRpZmljYXRlX2NoZWNr X2dldCgpCiAgICAgcmV0dXJuIHBvbGljeSAmIFdlYkNvcmU6OlNvdXBOZXR3b3JrU2Vzc2lvbjo6 U1NMU3RyaWN0OwogfQogCi12b2lkIGV3a19uZXR3b3JrX3Rsc19jZXJ0aWZpY2F0ZV9jaGVja19z ZXQoRWluYV9Cb29sIGNoZWNrQ2VydGlmaWNhdGVzKQordm9pZCBld2tfbmV0d29ya190bHNfY2Vy dGlmaWNhdGVfY2hlY2tfc2V0KEVpbmFfQm9vbCBlbmFibGUpCiB7CiAgICAgdW5zaWduZWQgcG9s aWN5ID0gV2ViQ29yZTo6U291cE5ldHdvcmtTZXNzaW9uOjpkZWZhdWx0U2Vzc2lvbigpLnNzbFBv bGljeSgpOwotICAgIFdlYkNvcmU6OlNvdXBOZXR3b3JrU2Vzc2lvbjo6ZGVmYXVsdFNlc3Npb24o KS5zZXRTU0xQb2xpY3kocG9saWN5IHwgV2ViQ29yZTo6U291cE5ldHdvcmtTZXNzaW9uOjpTU0xT dHJpY3QpOworICAgIGlmIChlbmFibGUgJiYgIShwb2xpY3kgfCBXZWJDb3JlOjpTb3VwTmV0d29y a1Nlc3Npb246OlNTTFN0cmljdCkpCisgICAgICAgIHBvbGljeSB8PSBXZWJDb3JlOjpTb3VwTmV0 d29ya1Nlc3Npb246OlNTTFN0cmljdDsKKyAgICBlbHNlIGlmICghZW5hYmxlICYmIChwb2xpY3kg fCBXZWJDb3JlOjpTb3VwTmV0d29ya1Nlc3Npb246OlNTTFN0cmljdCkpCisgICAgICAgIHBvbGlj eSAmPSB+V2ViQ29yZTo6U291cE5ldHdvcmtTZXNzaW9uOjpTU0xTdHJpY3Q7CisgICAgV2ViQ29y ZTo6U291cE5ldHdvcmtTZXNzaW9uOjpkZWZhdWx0U2Vzc2lvbigpLnNldFNTTFBvbGljeShwb2xp Y3kpOwogfQogCiBjb25zdCBjaGFyKiBld2tfbmV0d29ya190bHNfY2FfY2VydGlmaWNhdGVzX3Bh dGhfZ2V0KCkK