Bug 131734 – [EFL][WK1] SSL Strict is set according to input parameter.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Jeongeun Kim]

SSL-Strict is set regardless of input param from ewk_network_tls_certificate_check_set.
It should be set with input param, 'checkCertificates'.

[Jeongeun Kim]

For information,
ewk_network_tls_certificate_check_set/get APIs were added at 
https://bugs.webkit.org/show_bug.cgi?id=74299.

[Jeongeun Kim]

Created attachment 229445 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 229445 [details]
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);
}

[Jeongeun Kim]

(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?

[Gyuyoung Kim]

(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.

[Jeongeun Kim]

Created attachment 229539 [details]
Patch

[Jeongeun Kim]

(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,

[Gyuyoung Kim]

Comment on attachment 229539 [details]
Patch

LGTM.

[WebKit Commit Bot]

Comment on attachment 229539 [details]
Patch

Clearing flags on attachment: 229539

Committed r167475: <http://trac.webkit.org/changeset/167475>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.