[Webkit-unassigned] [Bug 163125] [GTK] Use libgcrypt instead of GnuTLS for CryptoDigest and SubtleCrypto HMAC implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=163125

--- Comment #8 from Olivier Blin <olivier.blin at softathome.com> ---
(In reply to comment #7)
> Comment on attachment 290961 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290961&action=review
> 
> 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.

This is actually copied from CryptoAlgorithmHMACGnuTLS.cpp, from Igalia.

Is the following fine?

+ * Copyright (C) 2014 Igalia S.L.
+ * Copyright (C) 2016 SoftAtHome

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

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.

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

Sure, I kept the existing style from the gnutls file, but I will fix.

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

I copied it from https://github.com/substack/libssh/blob/master/cmake/Modules/FindGCrypt.cmake , but it seems to be an old copy.
The real upstream of libssh is this one, with an additional "cmake: Update GCrypt module" commit:
https://git.libssh.org/projects/libssh.git/tree/cmake/Modules/FindGCrypt.cmake

Does it look better?

It still has a "GCRYPT_FOUND" comment on top, while the file never defines it.

Another alternative is this one:
https://quickgit.kde.org/?p=kwallet.git&a=blob&h=20053f1a790c99d5d75c495ed7c3255867d4efb7&hb=23344979ed047053256e21638338ef67a1a937ff&f=cmake%2FFindLibGcrypt.cmake

> > Source/cmake/FindGCrypt.cmake:70
> > +endif (GCRYPT_LIBRARIES AND GCRYPT_INCLUDE_DIRS)
> 
> WebKit CMake style is endif ()

Ok

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

For glib-networking in jhbuild

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20161007/7478a53e/attachment-0001.html>