[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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #290961|review?, commit-queue?      |review+, commit-queue-
              Flags|                            |

--- Comment #7 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 290961
  --> https://bugs.webkit.org/attachment.cgi?id=290961
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.

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

> Source/WebCore/crypto/gcrypt/CryptoAlgorithmHMACGCrypt.cpp:91
> +    UNUSED_PARAM(failureCallback);

Ditto?

> Source/WebCore/platform/crypto/gcrypt/CryptoDigestGCrypt.cpp:3
> + * Copyright (C) 2014 Igalia S.L. All rights reserved.
> + * Copyright (C) 2016 SoftAtHome. All rights reserved.

Ditto.

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

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

> Source/cmake/FindGCrypt.cmake:70
> +endif (GCRYPT_LIBRARIES AND GCRYPT_INCLUDE_DIRS)

WebKit CMake style is endif ()

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

-- 
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/9e2223f1/attachment.html>