<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Use libgcrypt instead of GnuTLS for CryptoDigest and SubtleCrypto HMAC implementation"
href="https://bugs.webkit.org/show_bug.cgi?id=163125#c8">Comment # 8</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Use libgcrypt instead of GnuTLS for CryptoDigest and SubtleCrypto HMAC implementation"
href="https://bugs.webkit.org/show_bug.cgi?id=163125">bug 163125</a>
from <span class="vcard"><a class="email" href="mailto:olivier.blin@softathome.com" title="Olivier Blin <olivier.blin@softathome.com>"> <span class="fn">Olivier Blin</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=163125#c7">comment #7</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=290961&action=diff" name="attach_290961" title="Patch">attachment 290961</a> <a href="attachment.cgi?id=290961&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=290961&action=review">https://bugs.webkit.org/attachment.cgi?id=290961&action=review</a>
>
> Thanks for working on this! I'm giving r+ but with several things that need
> fixed before committing:
>
> > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:3
> > + * Copyright (C) 2014 Igalia S.L. All rights reserved.
> > + * Copyright (C) 2016 SoftAtHome. All rights reserved.
>
> Please remove the "All rights reserved" since that contradicts the license
> right below.</span >
This is actually copied from CryptoAlgorithmHMACGnuTLS.cpp, from Igalia.
Is the following fine?
+ * Copyright (C) 2014 Igalia S.L.
+ * Copyright (C) 2016 SoftAtHome
<span class="quote">> > Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:81
> > + UNUSED_PARAM(failureCallback);
> > + int algorithm = getGCryptDigestAlgorithm(parameters.hash);
> > + if (algorithm == GCRY_MAC_NONE) {
> > + ec = NOT_SUPPORTED_ERR;
> > + return;
>
> You don't need to call failureCallback when the function fails...?</span >
I wondered about this too, but this is also copied from the gnutls flavor.
The Mac flavor is also ignoring the failure callback.
I can use failureCallback, not sure about unwanted side-effects.
<span class="quote">> > Source/WebCore/platform/crypto/gcrypt/CryptoDigestGCrypt.cpp:73
> > + switch (algorithm) {
> > + case CryptoDigest::Algorithm::SHA_1: {
> > + gcryptAlgorithm = GCRY_MD_SHA1;
> > + break;
> > + }
> > + case CryptoDigest::Algorithm::SHA_224: {
> > + gcryptAlgorithm = GCRY_MD_SHA224;
> > + break;
> > + }
> > + case CryptoDigest::Algorithm::SHA_256: {
> > + gcryptAlgorithm = GCRY_MD_SHA256;
> > + break;
> > + }
> > + case CryptoDigest::Algorithm::SHA_384: {
> > + gcryptAlgorithm = GCRY_MD_SHA384;
> > + break;
> > + }
> > + case CryptoDigest::Algorithm::SHA_512: {
> > + gcryptAlgorithm = GCRY_MD_SHA512;
> > + break;
> > + }
> > + }
>
> You don't need the extra braces around the cases here, since you're not
> declaring any variables in the cases.</span >
Sure, I kept the existing style from the gnutls file, but I will fix.
<span class="quote">> > Source/cmake/FindGCrypt.cmake:23
> > + # set(GCRYPT_FOUND TRUE)
>
> Well that's not right, did you comment it out for testing? Or was it broken
> already when you copied it from wherever its upstream is?</span >
I copied it from <a href="https://github.com/substack/libssh/blob/master/cmake/Modules/FindGCrypt.cmake">https://github.com/substack/libssh/blob/master/cmake/Modules/FindGCrypt.cmake</a> , but it seems to be an old copy.
The real upstream of libssh is this one, with an additional "cmake: Update GCrypt module" commit:
<a href="https://git.libssh.org/projects/libssh.git/tree/cmake/Modules/FindGCrypt.cmake">https://git.libssh.org/projects/libssh.git/tree/cmake/Modules/FindGCrypt.cmake</a>
Does it look better?
It still has a "GCRYPT_FOUND" comment on top, while the file never defines it.
Another alternative is this one:
<a href="https://quickgit.kde.org/?p=kwallet.git&a=blob&h=20053f1a790c99d5d75c495ed7c3255867d4efb7&hb=23344979ed047053256e21638338ef67a1a937ff&f=cmake%2FFindLibGcrypt.cmake">https://quickgit.kde.org/?p=kwallet.git&a=blob&h=20053f1a790c99d5d75c495ed7c3255867d4efb7&hb=23344979ed047053256e21638338ef67a1a937ff&f=cmake%2FFindLibGcrypt.cmake</a>
<span class="quote">> > Source/cmake/FindGCrypt.cmake:70
> > +endif (GCRYPT_LIBRARIES AND GCRYPT_INCLUDE_DIRS)
>
> WebKit CMake style is endif ()</span >
Ok
<span class="quote">> > Source/cmake/OptionsGTK.cmake:35
> > +find_package(GCrypt REQUIRED)
>
> All the functions you used were available in the first release of GCrypt?
>
> > Tools/ChangeLog:8
> > + * gtk/install-dep endencies: List libgcrypt for WebKitGTK+ build, and gnutls for jhbuild only.
>
> Er, why keep GnuTLS? You've completely removed the dependency, so you can
> get rid of it, right?</span >
For glib-networking in jhbuild</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>