Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node v18 migration impossible with eccrypto #1227

Closed
alexandre-abrioux opened this issue Nov 2, 2023 · 2 comments · Fixed by #1229
Closed

Node v18 migration impossible with eccrypto #1227

alexandre-abrioux opened this issue Nov 2, 2023 · 2 comments · Fixed by #1229

Comments

@alexandre-abrioux
Copy link
Member

alexandre-abrioux commented Nov 2, 2023

Issue

Migrating a codebase that uses the Request Network library to Node.js 18 is virtually impossible because of the dependency eccrypto.

Context

Native version incompatibility

eccrypto contains a "native" C++ binary that is compiled during installation of the package. This C++ code depends on the nan.h header file developed by Node and that contains native abstractions for Node.js. This header file has received breaking changes in Node v18, and compiling the C++ code now gives the following error:

In file included from ../../nan/nan.h:2884:
../../nan/nan_typedarray_contents.h: In constructor 'Nan::TypedArrayContents<T>::TypedArrayContents(v8::Local<v8::Value>)':
../../nan/nan_typedarray_contents.h:34:43: error: 'class v8::ArrayBuffer' has no member named 'GetContents'
   34 |       data   = static_cast<char*>(buffer->GetContents().Data()) + byte_offset;
      |                                           ^~~~~~~~~~~

This is also described here: bitchan/eccrypto#94

Browser version incompatibility

The issue described above should not, in theory, cause too much trouble because eccrypto also contains a browser (JS-only) implementation that is also the fallback on Node.js in case the C++ binary is not found (see here).

However, because of an issue with the library, data encrypted with the Native C++ module has a high probability of not being decryptable by the browser (JS-only) implementation. This means that data encrypted by Node.js before the upgrade to Node18, with the help of the C++ binary, cannot be decrypted after the upgrade to Node18 because we're now forced to use the browser (JS-only) fallback implementation.

This is also described here:

How to reproduce

Native version incompatibility

With the following Dockerfile:

FROM node:18.18.0-alpine3.18 AS base
RUN apk add --no-cache bash git openssh python3 make g++
WORKDIR /app

Run:

docker build -t test .
docker run --rm test sh -c "yarn add @requestnetwork/utils@^0.35.0 && cd node_modules/eccrypto && yarn run install"

Explanation:

  1. We install @requestnetwork/utils that itself depends on eccrypto
  2. It will not build the C++ binary, but to make sure of it, we cd to the eccrypto directory and we try to compile it manually
  3. To do so, we use yarn run install which is the NPM script used by eccrypto to compile the C++ biunary; see here

The C++ module will not compile, and you will get the error. If you repeat the same test with node:16.15.0-alpine3.15, the compilation works fine.

Browser version incompatibility

Generate a thousand encrypted data with the following code, with the C++ module in Node 16:

import { EncryptionTypes } from "@requestnetwork/types";
import Utils from "@requestnetwork/utils";
import { Wallet } from "ethers";
import fs from "fs";
import { randomBytes } from "node:crypto";

const run = async () => {
  const array = [];
  for (let i = 0; i < 1000; i++) {
    const wallet = Wallet.createRandom();
    const data = randomBytes(20).toString("hex");
    const encrypted = await Utils.encryption.encrypt(data, {
      key: wallet.publicKey,
      method: EncryptionTypes.METHOD.ECIES,
    });
    array.push({
      data,
      key: wallet.privateKey,
      encrypted: encrypted.value,
    });
    fs.writeFileSync("crypto.json", JSON.stringify(array, null, 2));
  }
};
void run();

You can make sure the dataset is generated with the C++ module by either:

  • the fact that you do not see the following output in your logs: secp256k1 unavailable, reverting to browser version
  • or by running the code with the following environment variable: ECCRYPTO_NO_FALLBACK=true (see here)

Then, in Node18, try to decrypt the data with the browser (JS-only) implementation. Ensure eccrypto falls back to the browser implementation in Node18; you should see the following log: secp256k1 unavailable, reverting to browser version.

import { EncryptionTypes } from "@requestnetwork/types";
import Utils from "@requestnetwork/utils";
import fs from "fs";

const run = async () => {
  const json = fs.readFileSync("crypto.json").toString();
  const array = JSON.parse(json);
  for (const rowNb in array) {
    const row = array[rowNb];
    const { data, key, encrypted } = row;
    try {
      await Utils.encryption.decrypt(  // it will throw here in some cases
        { type: EncryptionTypes.METHOD.ECIES, value: encrypted },
        {
          key,
          method: EncryptionTypes.METHOD.ECIES,
        },
      );
    } catch (e) {
      console.error(`error with row ${rowNb}/${array.length} (data=${data})`);
    }
  }
};
void run();

Conclusion

We should find an alternative library to replace eccrypto in order to move forward. New tests should be added before the migration to make sure that data encrypted with eccrypto, by the native C++ implementation as well as by the browser JS-only implementation can both be decrypted by the replacement library.

@alexandre-abrioux
Copy link
Member Author

Attempt to fix the issue started in #1229

@MantisClone
Copy link
Member

This issue is well written and enjoyable to read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants