Bug 30360 – [Chromium] Add <keygen> support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Gaurav Shah]

The patch replaces the temporary link stubs for <keygen> tag handler in Chromium with calls to the browser implementation via ChromiumBridge.

[Gaurav Shah]

Created attachment 41170 [details]
Chromium <keygen> handler patch

[Wan-Teh Chang]

In WebCore/platform/chromium/SSLKeyGeneratorChromium.cpp:

> // Returns the key sizes supported by the HTML keygen tag.  The first string
> // is displayed as the default key size in the keygen menu.
>-Vector<String> supportedKeySizes()
>+void getSupportedKeySizes(Vector<String>& sizes)
> {
>-    Vector<String> sizes(2);
>+    sizes.resize(2);
>     sizes[0] = keygenMenuHighGradeKeySize();
>     sizes[1] = keygenMenuMediumGradeKeySize();
>-    return sizes;
> }

Darin, please make sure we can set the size of a Vector
using the resize() method like this.  Thanks.

[Darin Fisher (:fishd, Google)]

Comment on attachment 41170 [details]
Chromium <keygen> handler patch 

> Index: WebCore/ChangeLog
...
> [phone]  Gaurav Shah  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +	Replaces temporary link stub for <keygen> tag handler for the Chromium
> +	browser with a call via the Chromium Bridge.

nit: please replace tabs with spaces.


> Index: WebCore/platform/SSLKeyGenerator.h

> +    // Get set of strings denoting keystrength in the <keygen> menu.
>      void getSupportedKeySizes(Vector<String>&);

This comment doesn't seem to tell me much more than what the function
name already says.


> +
> +    // This function handles the <keygen> tag in form elements for
> +    // requesting CAs to generate a client certificate from within
> +    // the browser. Returns the signed public key and challenge string
> +    // from a newly generated key pair.

It is probably not necessary to describe the keygen tag here.

The last sentence seems to suggest that the challenge string is a
returned value, but it appears to be a const reference type, so that
means that it must be an input value instead of a returned value.


>      String signedPublicKeyAndChallengeString(unsigned keySizeIndex, const String& challengeString, const KURL&);

> Index: WebCore/platform/chromium/ChromiumBridge.h
...
> +        // Keygen -------------------------------------------------------------
> +        static String genKeyAndSignChallenge(unsigned keySizeIndex, const String& challenge, const KURL& url);

nit: please insert this section in alphabetical order (i.e., just
after the "JavaScript" section).  Thanks!


> Index: WebCore/platform/chromium/SSLKeyGeneratorChromium.cpp
...
> +// These are defined in webkit/api/src//localized_strings.cpp.

s/localized_strings/LocalizedStrings/


> +// For handling <keygen> form elements
> +// Returns the signed public key and challenge string from a newly generated
> +// key pair for creating client certificates from within the browser

nit: no need to repeat the comment from the header file here.


> +String signedPublicKeyAndChallengeString(unsigned keySizeIndex,
> +                                         const String& challengeString,
> +                                         const KURL& url)
> +{
> +     return ChromiumBridge::genKeyAndSignChallenge(keySizeIndex, challengeString,
> +                                                url);

since the ChromiumBridge method is just a 1-1 pass through, it would be nice
if it had exactly the same name, signedPublicKeyAndChallengeString.

or, do you think that signedPublicKeyAndChallengeString should have a better
name?


> Index: WebCore/platform/chromium/TemporaryLinkStubs.cpp

thanks for cleaning up this file.

[Gaurav Shah]

(In reply to comment #3)
> (From update of attachment 41170 [details])
> > Index: WebCore/ChangeLog
> ...
> > [phone]  Gaurav Shah  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +	Replaces temporary link stub for <keygen> tag handler for the Chromium
> > +	browser with a call via the Chromium Bridge.
> 
> nit: please replace tabs with spaces.

Fixed.

> 
> 
> > Index: WebCore/platform/SSLKeyGenerator.h
> 
> > +    // Get set of strings denoting keystrength in the <keygen> menu.
> >      void getSupportedKeySizes(Vector<String>&);
> 
> This comment doesn't seem to tell me much more than what the function
> name already says.

It's not clear from the function name where and when it's used. I have tweaked the comment a little bit. 
 
> 
> 
> > +
> > +    // This function handles the <keygen> tag in form elements for
> > +    // requesting CAs to generate a client certificate from within
> > +    // the browser. Returns the signed public key and challenge string
> > +    // from a newly generated key pair.
> 
> It is probably not necessary to describe the keygen tag here.
> 
> The last sentence seems to suggest that the challenge string is a
> returned value, but it appears to be a const reference type, so that
> means that it must be an input value instead of a returned value.

Yes, I meant to say that both public key and challenge string combined together are signed. Will fix the comment.

> 
> 
> >      String signedPublicKeyAndChallengeString(unsigned keySizeIndex, const String& challengeString, const KURL&);
> 
> > Index: WebCore/platform/chromium/ChromiumBridge.h
> ...
> > +        // Keygen -------------------------------------------------------------
> > +        static String genKeyAndSignChallenge(unsigned keySizeIndex, const String& challenge, const KURL& url);
> 
> nit: please insert this section in alphabetical order (i.e., just
> after the "JavaScript" section).  Thanks!

Fixed.

> 
> 
> > Index: WebCore/platform/chromium/SSLKeyGeneratorChromium.cpp
> ...
> > +// These are defined in webkit/api/src//localized_strings.cpp.
> 
> s/localized_strings/LocalizedStrings/
> 
> 
> > +// For handling <keygen> form elements
> > +// Returns the signed public key and challenge string from a newly generated
> > +// key pair for creating client certificates from within the browser
> 
> nit: no need to repeat the comment from the header file here.

Fixed.

> 
> 
> > +String signedPublicKeyAndChallengeString(unsigned keySizeIndex,
> > +                                         const String& challengeString,
> > +                                         const KURL& url)
> > +{
> > +     return ChromiumBridge::genKeyAndSignChallenge(keySizeIndex, challengeString,
> > +                                                url);
> 
> since the ChromiumBridge method is just a 1-1 pass through, it would be nice
> if it had exactly the same name, signedPublicKeyAndChallengeString.
> 
> or, do you think that signedPublicKeyAndChallengeString should have a better
> name?

genKeyAndSignChallenge() is what the WebKit layer is asking the browser to do, so more accurate IMO. But WebCore uses signedPublicKeyAndChallenge() at various places (incl. html/HTMLKeygenElement.cpp), so I didn't change it. 

Maybe, we can make it signedPublicKeyAndChallengeString for ChromiumBridge but keep genKeyAndSignChallenge in the Chromium portion? 

> 
> 
> > Index: WebCore/platform/chromium/TemporaryLinkStubs.cpp
> 
> thanks for cleaning up this file.

[Gaurav Shah]

Created attachment 41183 [details]
Updated Chromium <keygen> handler patch

[Wan-Teh Chang]

(In reply to comment #4)
>
> Maybe, we can make it signedPublicKeyAndChallengeString for ChromiumBridge but
> keep genKeyAndSignChallenge in the Chromium portion?

Darin, signedPublicKeyAndChallengeString does more than signing;
it also generates a new key pair.  This is why I suggested the
genKeyAndSignChallenge function name to Gaurav.

But I agree that the since ChromiumBridge method is just a 1-1
pass through, it should have the same name.  Gaurav, let's do
what you suggested above.  The signedPublicKeyAndChallengeString
name should extend all the way to
chrome/renderer/renderer_webkitclient_impl.cc in the Chromium
source tree.

[Darin Fisher (:fishd, Google)]

(In reply to comment #6)
> Darin, signedPublicKeyAndChallengeString does more than signing;
> it also generates a new key pair.  This is why I suggested the
> genKeyAndSignChallenge function name to Gaurav.

That makes sense.  My point was that if the WebCore name is poor, then let's fix it :-)

[Darin Fisher (:fishd, Google)]

One note about naming:  In WebKit, the style is to avoid abbreviations when naming methods.

[Gaurav Shah]

(In reply to comment #6)
> (In reply to comment #4)
> >
> > Maybe, we can make it signedPublicKeyAndChallengeString for ChromiumBridge but
> > keep genKeyAndSignChallenge in the Chromium portion?
> 
> Darin, signedPublicKeyAndChallengeString does more than signing;
> it also generates a new key pair.  This is why I suggested the
> genKeyAndSignChallenge function name to Gaurav.
> 
> But I agree that the since ChromiumBridge method is just a 1-1
> pass through, it should have the same name.  Gaurav, let's do
> what you suggested above.  The signedPublicKeyAndChallengeString
> name should extend all the way to
> chrome/renderer/renderer_webkitclient_impl.cc in the Chromium
> source tree.

Ok, I made the change to make it a 1-1 pass through.

[Gaurav Shah]

Created attachment 41294 [details]
Chromium <keygen> handler patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 41294 [details]
Chromium <keygen> handler patch 

> Index: WebCore/ChangeLog
...
> +        Replaces temporary link stub for <keygen> tag handler for the Chromium
> +        browser with a call via the Chromium Bridge.

nit: there should be a bug link here.  i'll add that before committing.

-darin

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/49947

[Gaurav Shah]

(In reply to comment #12)
> Landed as http://trac.webkit.org/changeset/49947

Thanks Darin!