Bug 70404 – [EFL] Implement port layer of keygen element.

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

[Dongwoo Joshua Im (dwim)]

Keygen element is new feature of HTML5, and supported on most of port of WebKit already.
But this element is not supported on EFL port.

Keygen element is one of children of form element, make a private/public key pair, 
and return the public key which is encoded as base64 when the form is submitted.
(http://www.w3.org/TR/html5/the-button-element.html#the-keygen-element)

[Dongwoo Joshua Im (dwim)]

Created attachment 111579 [details]
Patch

I use OpenSSL APIs for create and encode RSA key pair.

[Gyuyoung Kim]

Comment on attachment 111579 [details]
Patch

Attachment 111579 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10142341

[Gyuyoung Kim]

EFL EWS should install ssl library first in order to build this patch. Could you tell me what library need to be installed ?

[Ryuan Choi]

View in context: https://bugs.webkit.org/attachment.cgi?id=111579&action=review

> Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:29
> +#include "NotImplemented.h"

you can remove this.

> Source/cmake/FindSSL.cmake:11
> +libfind_pkg_check_modules(SSL_PKGCONF vconf)

vconf is not what FindSSL want.

[Leandro Pereira]

Comment on attachment 111579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111579&action=review

Informal r- for the reasons below.

> Source/WebCore/platform/efl/SSLKeyGeneratorEfl.cpp:84
> +    if (!(RSA_check_key(rsa) == 1))
> +        return SSLErrorHandler(rsa, pkey, spki);

if (RSA_check_key(rsa) != 1) ...

> Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:35
> +static SSLPrivateKeyStoreEfl* s_SSLPrivateKeyStoreEfl = 0;
> +static Mutex s_SSLPrivateKeyStoreMutex;

These can be declared inside SSLPrivateKeyStoreEfl::getInstance() below, using the DECLARE_STATIC_LOCAL() macro.

> Source/cmake/FindSSL.cmake:15
> +  NAMES openssl/ssl.h

It would be nice if a GNUTLS implementation were also provided, so that if GNUTLS is being used by the networking backend (by libcurl or libsoup), there's no need to link to both OpenSSL and GNUTLS libraries.

> Source/cmake/OptionsEfl.cmake:39
> +FIND_PACKAGE(SSL REQUIRED)

I'd rather not mark this as required, but condition it to a build-time option.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #3)
> EFL EWS should install ssl library first in order to build this patch. Could you tell me what library need to be installed ?

I've used libssl-dev which version is 0.9.8s.

[Gyuyoung Kim]

(In reply to comment #6)
> (In reply to comment #3)
> > EFL EWS should install ssl library first in order to build this patch. Could you tell me what library need to be installed ?
> 
> I've used libssl-dev which version is 0.9.8s.

I installed libssl-dev 0.9.8 both efl build bot and ews.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #3)
> > > EFL EWS should install ssl library first in order to build this patch. Could you tell me what library need to be installed ?
> > 
> > I've used libssl-dev which version is 0.9.8s.
> 
> I installed libssl-dev 0.9.8 both efl build bot and ews.

Thanks!

[Dongwoo Joshua Im (dwim)]

(In reply to comment #5)
> (From update of attachment 111579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111579&action=review

> > Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:35
> > +static SSLPrivateKeyStoreEfl* s_SSLPrivateKeyStoreEfl = 0;
> > +static Mutex s_SSLPrivateKeyStoreMutex;
> 
> These can be declared inside SSLPrivateKeyStoreEfl::getInstance() below, using the DECLARE_STATIC_LOCAL() macro.

I can't find "DECLARE_STATIC_LOCAL()" macro in WebKit source code.
Do you mean I need to define that kind of macro and use it?

 
> > Source/cmake/FindSSL.cmake:15
> > +  NAMES openssl/ssl.h
> 
> It would be nice if a GNUTLS implementation were also provided, so that if GNUTLS is being used by the networking backend (by libcurl or libsoup), there's no need to link to both OpenSSL and GNUTLS libraries.

As I know, OpenSSL is the most popular and powerful open-source library which implements SSL.
So, I think if we can use that, if there is no limitation to add new library, it's better to use OpenSSL rather than other library.

I think that's why Chrome implements keygen handler with OpenSSL.
(As I know, Chromium offers 4 kinds of keygen handlers, which are 'win', 'mac', 'nss', and 'OpenSSL'.
'win' and 'mac' is platform specific, 'nss' is library from mozilla.)

What do you think?


 
> > Source/cmake/OptionsEfl.cmake:39
> > +FIND_PACKAGE(SSL REQUIRED)
> 
> I'd rather not mark this as required, but condition it to a build-time option.

There is no option to enable/disable keygen. Keygen is always enabled.
So, I think SSL library is required, if we use OpenSSL for keygen.

You have a lot more experience and knowledge about this than me, please let me know if I miss something.


Thanks for your comments.

[Ryuan Choi]

(In reply to comment #9)
> (In reply to comment #5)
> > (From update of attachment 111579 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=111579&action=review
> 
> > > Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:35
> > > +static SSLPrivateKeyStoreEfl* s_SSLPrivateKeyStoreEfl = 0;
> > > +static Mutex s_SSLPrivateKeyStoreMutex;
> > 
> > These can be declared inside SSLPrivateKeyStoreEfl::getInstance() below, using the DECLARE_STATIC_LOCAL() macro.
> 
> I can't find "DECLARE_STATIC_LOCAL()" macro in WebKit source code.
> Do you mean I need to define that kind of macro and use it?

You can reference history and sample in http://trac.webkit.org/changeset/91171

> 
> 
> > > Source/cmake/FindSSL.cmake:15
> > > +  NAMES openssl/ssl.h
> > 
> > It would be nice if a GNUTLS implementation were also provided, so that if GNUTLS is being used by the networking backend (by libcurl or libsoup), there's no need to link to both OpenSSL and GNUTLS libraries.
> 
> As I know, OpenSSL is the most popular and powerful open-source library which implements SSL.
> So, I think if we can use that, if there is no limitation to add new library, it's better to use OpenSSL rather than other library.
> 
> I think that's why Chrome implements keygen handler with OpenSSL.
> (As I know, Chromium offers 4 kinds of keygen handlers, which are 'win', 'mac', 'nss', and 'OpenSSL'.
> 'win' and 'mac' is platform specific, 'nss' is library from mozilla.)
> 
> What do you think?
> 
> 
> 
> > > Source/cmake/OptionsEfl.cmake:39
> > > +FIND_PACKAGE(SSL REQUIRED)
> > 
> > I'd rather not mark this as required, but condition it to a build-time option.
> 
> There is no option to enable/disable keygen. Keygen is always enabled.
> So, I think SSL library is required, if we use OpenSSL for keygen.
> 
> You have a lot more experience and knowledge about this than me, please let me know if I miss something.
> 
> 
> Thanks for your comments.

I also think that it's not good to provide openssl only.
However, this bug doesn't need to cover gnutls.

IMO, it's better to disable this feature when user doesn't have openssl, for now.

Leandro, how do you think about this?

[Leandro Pereira]

(In reply to comment #10)
> 
> I also think that it's not good to provide openssl only.
> However, this bug doesn't need to cover gnutls.
> 
> IMO, it's better to disable this feature when user doesn't have openssl, for now.
> 
> Leandro, how do you think about this?
>

Sounds like a plan.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > (From update of attachment 111579 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=111579&action=review
> > 
> > > > Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:35
> > > > +static SSLPrivateKeyStoreEfl* s_SSLPrivateKeyStoreEfl = 0;
> > > > +static Mutex s_SSLPrivateKeyStoreMutex;
> > > 
> > > These can be declared inside SSLPrivateKeyStoreEfl::getInstance() below, using the DECLARE_STATIC_LOCAL() macro.
> > 
> > I can't find "DECLARE_STATIC_LOCAL()" macro in WebKit source code.
> > Do you mean I need to define that kind of macro and use it?
> 
> You can reference history and sample in http://trac.webkit.org/changeset/91171

I use the static variables because I use singletone pattern here.

If I use the "DEFINE_STATIC_LOCAL" macro,
source code might look like this.

SSLPrivateKeyStoreEfl* SSLPrivateKeyStoreEfl::getInstance()
{
    DEFINE_STATIC_LOCAL(SSLPrivateKeyStoreEfl, s_SSLPrivateKeyStoreEfl, ());
    return &s_SSLPrivateKeyStoreEfl;
}

I don't understand deeply about DEFINE_STATIC_LOCAL, 
is it secure enough for singtone??

Or, do I need to make it as critical section?

[Dongwoo Joshua Im (dwim)]

(In reply to comment #11)
> (In reply to comment #10)
> > 
> > I also think that it's not good to provide openssl only.
> > However, this bug doesn't need to cover gnutls.
> > 
> > IMO, it's better to disable this feature when user doesn't have openssl, for now.
> > 
> > Leandro, how do you think about this?
> >
> 
> Sounds like a plan.

Ok.
Then, I'll revise this patch to disable keygen feature if the system doesn't support OpenSSL.

And, I'll try to prepare another patch which use gnutils.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #12)
> I use the static variables because I use singletone pattern here.
> 
> If I use the "DEFINE_STATIC_LOCAL" macro,
> source code might look like this.
> 
> SSLPrivateKeyStoreEfl* SSLPrivateKeyStoreEfl::getInstance()
> {
>     DEFINE_STATIC_LOCAL(SSLPrivateKeyStoreEfl, s_SSLPrivateKeyStoreEfl, ());
>     return &s_SSLPrivateKeyStoreEfl;
> }
> 
> I don't understand deeply about DEFINE_STATIC_LOCAL, 
> is it secure enough for singtone??
> 
> Or, do I need to make it as critical section?

If you're in doubt, it's worth taking a look at the macro in Source/JavaScriptCore/wtf/StdLibExtras.h -- the code is very simple.

[Soo-Hyun Choi]

(In reply to comment #14)
> (In reply to comment #12)
> > I use the static variables because I use singletone pattern here.
> > 
> > If I use the "DEFINE_STATIC_LOCAL" macro,
> > source code might look like this.
> > 
> > SSLPrivateKeyStoreEfl* SSLPrivateKeyStoreEfl::getInstance()
> > {
> >     DEFINE_STATIC_LOCAL(SSLPrivateKeyStoreEfl, s_SSLPrivateKeyStoreEfl, ());
> >     return &s_SSLPrivateKeyStoreEfl;
> > }
> > 
> > I don't understand deeply about DEFINE_STATIC_LOCAL, 
> > is it secure enough for singtone??
> > 
> > Or, do I need to make it as critical section?
> 
> If you're in doubt, it's worth taking a look at the macro in Source/JavaScriptCore/wtf/StdLibExtras.h -- the code is very simple.

Having looked at the code mentioned in the above, it doesn't seem to guarantee the singleton pattern when used as is - i.e., it may instantiate another object if you simply use the "DEFINE_STATIC_LOCAL", in which Dongwoo have tried to avoid from in the first place.

IMHO, I'd rather suggest you to write a separate macro that guarantees the singleton pattern, if you have to use macro function instead of using in-function "static local". Alternatively, you might be able to use 

My question to the previous reviewers is that if it is mandatory to use the defined set of macros all the times, or if there is a room to use a customized macro or more specifically "static local" inside a function when necessary?

Kind regards,
Soo-Hyun

[Soo-Hyun Choi]

Some part of my earlier comment has chopped off for some reason. 

(snipped)

Alternatively, you might be able to use in-line "static local" with the mutex lock - not sure though if there is a WebKit policy about the "static local" usage.

(snipped)

[Dongwoo Joshua Im (dwim)]

(In reply to comment #14)
> (In reply to comment #12)
> > I use the static variables because I use singletone pattern here.
> > 
> > If I use the "DEFINE_STATIC_LOCAL" macro,
> > source code might look like this.
> > 
> > SSLPrivateKeyStoreEfl& SSLPrivateKeyStoreEfl::getInstance()
> > {
> >     DEFINE_STATIC_LOCAL(SSLPrivateKeyStoreEfl, s_SSLPrivateKeyStoreEfl, ());
> >     return s_SSLPrivateKeyStoreEfl;
> > }
> > 
> > I don't understand deeply about DEFINE_STATIC_LOCAL, 
> > is it secure enough for singtone??
> > 
> > Or, do I need to make it as critical section?
> 
> If you're in doubt, it's worth taking a look at the macro in Source/JavaScriptCore/wtf/StdLibExtras.h -- the code is very simple.

IMO, I can't use this macro for singletone pattern which I use for KeyStore.
I want only one instance of KeyStore and use it, but this macro will create another instance whenever it is called if I use this macro in the "getInstance" function.

I really think I need to use singletone pattern for the KeyStore without changing WebCore.
(Singletone pattern is also used in Chrome.)


So, I have a question.
Is it prohibited to use static variable without the macro in WebKit?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 111579 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111579&action=review

It looks like part of the code is missing -- who is calling fetchPrivateKey(), for example? And if you use a singleton as you do in getInstance(), who's going to destroy your object?

> ChangeLog:13
> +        * Source/cmake/FindSSL.cmake: OpenSSL is required for keygen element on EFL port.

CMake already has FindOpenSSL.cmake, there's no need for this file.

> Source/WebCore/platform/efl/SSLKeyGeneratorEfl.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY

You guys should check this, as this is not Apple's copyright. This is the 2-clause BSD license, so you should probably follow this template: http://www.opensource.org/licenses/BSD-2-Clause

> Source/WebCore/platform/efl/SSLKeyGeneratorEfl.cpp:41
> +    ASSERT(supportedKeySizes.isEmpty());

This will only be triggered in debug mode, it might be safer to actually resize the vector.

> Source/WebCore/platform/efl/SSLKeyGeneratorEfl.cpp:105
> +    return result;

Don't you need to free spki and rsa in the successful case too?

> Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:29
> +#include "NotImplemented.h"

Not needed.

>>>>> Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:35
>>>>> +static Mutex s_SSLPrivateKeyStoreMutex;
>>>> 
>>>> These can be declared inside SSLPrivateKeyStoreEfl::getInstance() below, using the DECLARE_STATIC_LOCAL() macro.
>>> 
>>> I can't find "DECLARE_STATIC_LOCAL()" macro in WebKit source code.
>>> Do you mean I need to define that kind of macro and use it?
>> 
>> You can reference history and sample in http://trac.webkit.org/changeset/91171
> 
> I use the static variables because I use singletone pattern here.
> 
> If I use the "DEFINE_STATIC_LOCAL" macro,
> source code might look like this.
> 
> SSLPrivateKeyStoreEfl* SSLPrivateKeyStoreEfl::getInstance()
> {
>     DEFINE_STATIC_LOCAL(SSLPrivateKeyStoreEfl, s_SSLPrivateKeyStoreEfl, ());
>     return &s_SSLPrivateKeyStoreEfl;
> }
> 
> I don't understand deeply about DEFINE_STATIC_LOCAL, 
> is it secure enough for singtone??
> 
> Or, do I need to make it as critical section?

It will have the same effect. If you expand the macro, the difference is that you declare the variable to be static in the scope of the method.

> Source/WebCore/platform/efl/SSLPrivateKeyStoreEfl.cpp:66
> +    return (EVP_PKEY*)m_privateKeyMap.get(url.host());

Use a C++ cast here.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #18)
> (From update of attachment 111579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111579&action=review
> 
> It looks like part of the code is missing -- who is calling fetchPrivateKey(), for example? 

No one calls fetchPrivateKey() function now, because the specification of keygen is not define who use and how to use the private key.
But I've made the function because the function is definitely needed to use the private key.


> And if you use a singleton as you do in getInstance(), who's going to destroy your object?

I know this is the reason why you've recommended me to use the macro.

I'm still looking for how to use singletone pattern in WebKit.
I need to use the design pattern, and I have not found that pattern in WebKit yet.


> CMake already has FindOpenSSL.cmake, there's no need for this file.

Is it? As I know, there is no "FindOpenSSL.cmake" file under Source/cmake/.
Could please let me know where the file is?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #19)
> (In reply to comment #18)
> > It looks like part of the code is missing -- who is calling fetchPrivateKey(), for example? 
> 
> No one calls fetchPrivateKey() function now, because the specification of keygen is not define who use and how to use the private key.
> But I've made the function because the function is definitely needed to use the private key.

Could you give some use cases at least (which parts of WebCore or WebKit would need it, for example)? I'm asking because depending on the uses it might be better to store the data using another data structure (and the private key is being stored in plain memory, but I don't know what the other ports do in this regard, so I may be being too paranoid on this one).

> > And if you use a singleton as you do in getInstance(), who's going to destroy your object?
> 
> I know this is the reason why you've recommended me to use the macro.

This is not the reason why I've mentioned that macro. Even if you use your code without the DEFINE_STATIC_LOCAL change, you are creating an object on the heap but never deleting it, so the destructor will not be called.

> I'm still looking for how to use singletone pattern in WebKit.
> I need to use the design pattern, and I have not found that pattern in WebKit yet.

If by singleton you simply mean preventing users from having more than 1 instance of the class, then both your original patch and DEFINE_STATIC_LOCAL do that.
 
> > CMake already has FindOpenSSL.cmake, there's no need for this file.
> 
> Is it? As I know, there is no "FindOpenSSL.cmake" file under Source/cmake/.
> Could please let me know where the file is?

I mean CMake itself.

[Dongwoo Joshua Im (dwim)]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > It looks like part of the code is missing -- who is calling fetchPrivateKey(), for example? 
> > 
> > No one calls fetchPrivateKey() function now, because the specification of keygen is not define who use and how to use the private key.
> > But I've made the function because the function is definitely needed to use the private key.
> 
> Could you give some use cases at least (which parts of WebCore or WebKit would need it, for example)? I'm asking because depending on the uses it might be better to store the data using another data structure (and the private key is being stored in plain memory, but I don't know what the other ports do in this regard, so I may be being too paranoid on this one).
> 

When server sent a message which is encrypted by the public key, 
user agent should decrypt this encrypted message using the private key which is stored.

Chrome port use std::vector to store the private keys.
It looks Win port does not store the private keys, IMHO.

There is no guarantee that server will store and use the public key permanently,
(because specification doesn't specify that)
so user agent doesn't need to store the private key in disk rather than in memory.

[Gyuyoung Kim]

Any update ? If there is no discussion anymore, I would like to close this bug.

[Dongwoo Joshua Im (dwim)]

I will start this again. ;)

I can use layouttest now, (we don't have it at that time.)
so, I think I can make and verify reasonable patch.