Improve argument passing to C++ and fix function signatures

ranisalt · ranisalt · commit a3658fbccd77 · 2024-02-10T03:20:35.000+01:00
diff --git a/argon2.cjs b/argon2.cjs
@@ -29,7 +29,6 @@ const names = Object.freeze({
 
 const defaults = Object.freeze({
   hashLength: 32,
-  saltLength: 16,
   timeCost: 3,
   memoryCost: 1 << 16,
   parallelism: 4,
@@ -51,7 +50,6 @@ module.exports.limits = limits;
  * @property {number} [timeCost=3]
  * @property {number} [memoryCost=65536]
  * @property {number} [parallelism=4]
- * @property {number} [saltLength=16]
  * @property {keyof typeof names} [type=argon2id]
  * @property {number} [version=19]
  * @property {Buffer} [salt]
@@ -63,25 +61,24 @@ module.exports.limits = limits;
  * Hashes a password with Argon2, producing a raw hash
  *
  * @overload
- * @param {Buffer | string} plain The plaintext password to be hashed
+ * @param {Buffer | string} password The plaintext password to be hashed
  * @param {Options & { raw: true }} options The parameters for Argon2
- * @return {Promise<Buffer>} The raw hash generated from `plain`
+ * @returns {Promise<Buffer>} The raw hash generated from `plain`
  */
 /**
  * Hashes a password with Argon2, producing an encoded hash
  *
  * @overload
- * @param {Buffer | string} plain The plaintext password to be hashed
+ * @param {Buffer | string} password The plaintext password to be hashed
  * @param {Options & { raw?: boolean }} [options] The parameters for Argon2
- * @return {Promise<string>} The encoded hash generated from `plain`
+ * @returns {Promise<string>} The encoded hash generated from `plain`
  */
 /**
- * @param {Buffer | string} plain The plaintext password to be hashed
+ * @param {Buffer | string} password The plaintext password to be hashed
  * @param {Options & { raw?: boolean }} [options] The parameters for Argon2
- * @returns {Promise<Buffer | string>} The raw or encoded hash generated from `plain`
  */
-module.exports.hash = async function (plain, options) {
-  const { raw, salt, saltLength, ...rest } = { ...defaults, ...options };
+async function hash(password, options) {
+  let { raw, salt, ...rest } = { ...defaults, ...options };
 
   for (const [key, { min, max }] of Object.entries(limits)) {
     const value = rest[key];
@@ -91,37 +88,54 @@ module.exports.hash = async function (plain, options) {
     );
   }
 
-  const salt_ = salt ?? (await generateSalt(saltLength));
-
-  const hash = await bindingsHash(Buffer.from(plain), salt_, rest);
-  if (raw) {
-    return hash;
-  }
+  salt = salt ?? (await generateSalt(16));
 
   const {
+    hashLength,
+    secret = Buffer.alloc(0),
     type,
     version,
     memoryCost: m,
     timeCost: t,
     parallelism: p,
-    associatedData: data,
+    associatedData: data = Buffer.alloc(0),
   } = rest;
 
+  const hash = await bindingsHash({
+    password: Buffer.from(password),
+    salt,
+    secret,
+    data,
+    hashLength,
+    m,
+    t,
+    p,
+    version,
+    type,
+  });
+  if (raw) {
+    return hash;
+  }
+
   return serialize({
     id: names[type],
     version,
-    params: { m, t, p, ...(data ? { data } : {}) },
-    salt: salt_,
+    params: { m, t, p, ...(data.byteLength > 0 ? { data } : {}) },
+    salt,
     hash,
   });
-};
+}
+module.exports.hash = hash;
 
 /**
  * @param {string} digest The digest to be checked
- * @param {Options} [options] The current parameters for Argon2
- * @return {boolean} `true` if the digest parameters do not match the parameters in `options`, otherwise `false`
+ * @param {Object} [options] The current parameters for Argon2
+ * @param {number} [options.timeCost=3]
+ * @param {number} [options.memoryCost=65536]
+ * @param {number} [options.parallelism=4]
+ * @returns {boolean} `true` if the digest parameters do not match the parameters in `options`, otherwise `false`
  */
-module.exports.needsRehash = function (digest, options) {
+function needsRehash(digest, options = {}) {
   const { memoryCost, timeCost, version } = { ...defaults, ...options };
 
   const {
@@ -130,38 +144,47 @@ module.exports.needsRehash = function (digest, options) {
   } = deserialize(digest);
 
   return +v !== +version || +m !== +memoryCost || +t !== +timeCost;
-};
+}
+module.exports.needsRehash = needsRehash;
 
 /**
  * @param {string} digest The digest to be checked
- * @param {Buffer | string} plain The plaintext password to be verified
- * @param {Options} [options] The current parameters for Argon2
- * @return {Promise<boolean>} `true` if the digest parameters matches the hash generated from `plain`, otherwise `false`
+ * @param {Buffer | string} password The plaintext password to be verified
+ * @param {Object} [options] The current parameters for Argon2
+ * @param {Buffer} [options.secret]
+ * @returns {Promise<boolean>} `true` if the digest parameters matches the hash generated from `plain`, otherwise `false`
  */
-module.exports.verify = async function (digest, plain, options) {
+async function verify(
+  digest,
+  password,
+  { secret } = { secret: Buffer.alloc(0) },
+) {
   const { id, ...rest } = deserialize(digest);
   if (!(id in types)) {
     return false;
   }
 
   const {
     version = 0x10,
-    params: { m, t, p, data },
+    params: { m, t, p, data = "" },
     salt,
     hash,
   } = rest;
 
   return timingSafeEqual(
-    await bindingsHash(Buffer.from(plain), salt, {
-      ...options,
-      type: types[id],
+    await bindingsHash({
+      password: Buffer.from(password),
+      salt,
+      secret,
+      data: Buffer.from(data, "base64"),
+      hashLength: hash.byteLength,
+      m: +m,
+      t: +t,
+      p: +p,
       version: +version,
-      hashLength: hash.length,
-      memoryCost: +m,
-      timeCost: +t,
-      parallelism: +p,
-      ...(data ? { associatedData: Buffer.from(data, "base64") } : {}),
+      type: types[id],
     }),
     hash,
   );
-};
+}
+module.exports.verify = verify;
diff --git a/argon2_node.cpp b/argon2_node.cpp
@@ -4,71 +4,52 @@
 #include <napi.h>
 #include <vector>
 
-using ustring = std::vector<uint8_t>;
-
-static ustring from_buffer(const Napi::Value &value) {
-    const auto &buf = value.As<Napi::Buffer<uint8_t>>();
-    const auto &data = buf.Data();
-    return {data, data + buf.Length()};
-}
-
-struct Options {
-    ustring secret;
-    ustring ad;
-
-    uint32_t hash_length;
-    uint32_t time_cost;
-    uint32_t memory_cost;
-    uint32_t parallelism;
-    uint32_t version;
-
-    argon2_type type;
-};
-
-static argon2_context make_context(uint8_t *buf, ustring &plain, ustring &salt,
-                                   Options &opts) {
-    argon2_context ctx;
-
-    ctx.out = buf;
-    ctx.outlen = opts.hash_length;
-    ctx.pwd = plain.data();
-    ctx.pwdlen = static_cast<uint32_t>(plain.size());
-    ctx.salt = salt.data();
-    ctx.saltlen = static_cast<uint32_t>(salt.size());
-    ctx.secret = opts.secret.empty() ? nullptr : opts.secret.data();
-    ctx.secretlen = static_cast<uint32_t>(opts.secret.size());
-    ctx.ad = opts.ad.empty() ? nullptr : opts.ad.data();
-    ctx.adlen = static_cast<uint32_t>(opts.ad.size());
-    ctx.t_cost = opts.time_cost;
-    ctx.m_cost = opts.memory_cost;
-    ctx.lanes = opts.parallelism;
-    ctx.threads = opts.parallelism;
-    ctx.allocate_cbk = nullptr;
-    ctx.free_cbk = nullptr;
-    ctx.flags = ARGON2_FLAG_CLEAR_PASSWORD | ARGON2_FLAG_CLEAR_SECRET;
-    ctx.version = opts.version;
-
-    return ctx;
-}
+namespace {
 
 class HashWorker final : public Napi::AsyncWorker {
 public:
-    HashWorker(const Napi::Env &env, ustring &&plain, ustring &&salt,
-               Options &&opts)
+    HashWorker(const Napi::Env &env, const Napi::Buffer<uint8_t> &plain,
+               const Napi::Buffer<uint8_t> &salt,
+               const Napi::Buffer<uint8_t> &secret,
+               const Napi::Buffer<uint8_t> &ad, uint32_t hash_length,
+               uint32_t memory_cost, uint32_t time_cost, uint32_t parallelism,
+               uint32_t version, uint32_t type)
         : AsyncWorker{env, "argon2:HashWorker"}, deferred{env},
-          plain{std::move(plain)}, salt{std::move(salt)},
-          opts{std::move(opts)} {}
+          plain{plain.Data(), plain.Data() + plain.ByteLength()},
+          salt{salt.Data(), salt.Data() + salt.ByteLength()},
+          secret{secret.Data(), secret.Data() + secret.ByteLength()},
+          ad{ad.Data(), ad.Data() + ad.ByteLength()}, hash_length{hash_length},
+          memory_cost{memory_cost}, time_cost{time_cost},
+          parallelism{parallelism}, version{version},
+          type{static_cast<argon2_type>(type)} {}
 
     Napi::Promise GetPromise() { return deferred.Promise(); }
 
 protected:
     void Execute() override {
-        hash = std::make_unique<uint8_t[]>(opts.hash_length);
-
-        auto ctx = make_context(hash.get(), plain, salt, opts);
-        int result = argon2_ctx(&ctx, opts.type);
-
-        if (result != ARGON2_OK) {
+        hash.resize(hash_length);
+
+        argon2_context ctx;
+        ctx.out = hash.data();
+        ctx.outlen = static_cast<uint32_t>(hash.size());
+        ctx.pwd = plain.data();
+        ctx.pwdlen = static_cast<uint32_t>(plain.size());
+        ctx.salt = salt.data();
+        ctx.saltlen = static_cast<uint32_t>(salt.size());
+        ctx.secret = secret.empty() ? nullptr : secret.data();
+        ctx.secretlen = static_cast<uint32_t>(secret.size());
+        ctx.ad = ad.empty() ? nullptr : ad.data();
+        ctx.adlen = static_cast<uint32_t>(ad.size());
+        ctx.m_cost = memory_cost;
+        ctx.t_cost = time_cost;
+        ctx.lanes = parallelism;
+        ctx.threads = parallelism;
+        ctx.allocate_cbk = nullptr;
+        ctx.free_cbk = nullptr;
+        ctx.flags = ARGON2_FLAG_CLEAR_PASSWORD | ARGON2_FLAG_CLEAR_SECRET;
+        ctx.version = version;
+
+        if (int result = argon2_ctx(&ctx, type); result != ARGON2_OK) {
             /* LCOV_EXCL_START */
             SetError(argon2_error_message(result));
             /* LCOV_EXCL_STOP */
@@ -77,43 +58,48 @@ class HashWorker final : public Napi::AsyncWorker {
 
     void OnOK() override {
         deferred.Resolve(
-            Napi::Buffer<uint8_t>::Copy(Env(), hash.get(), opts.hash_length));
+            Napi::Buffer<uint8_t>::Copy(Env(), hash.data(), hash.size()));
     }
 
     void OnError(const Napi::Error &err) override {
         deferred.Reject(err.Value());
     }
 
 private:
+    using ustring = std::basic_string<uint8_t>;
+
     Napi::Promise::Deferred deferred;
+    ustring hash;
+
     ustring plain;
     ustring salt;
-    Options opts;
+    ustring secret;
+    ustring ad;
 
-    std::unique_ptr<uint8_t[]> hash;
-};
+    uint32_t hash_length;
+    uint32_t memory_cost;
+    uint32_t time_cost;
+    uint32_t parallelism;
+    uint32_t version;
 
-static Options extract_opts(const Napi::Object &opts) {
-    return {
-        opts.Has("secret") ? from_buffer(opts["secret"]) : ustring{},
-        opts.Has("associatedData") ? from_buffer(opts["associatedData"])
-                                   : ustring{},
-        opts["hashLength"].ToNumber(),
-        opts["timeCost"].ToNumber(),
-        opts["memoryCost"].ToNumber(),
-        opts["parallelism"].ToNumber(),
-        opts["version"].ToNumber(),
-        argon2_type(int(opts["type"].ToNumber())),
-    };
-}
+    argon2_type type;
+};
 
 static Napi::Value Hash(const Napi::CallbackInfo &info) {
-    assert(info.Length() == 4 && info[0].IsBuffer() && info[1].IsBuffer() &&
-           info[2].IsObject());
-
-    auto worker =
-        new HashWorker{info.Env(), from_buffer(info[0]), from_buffer(info[1]),
-                       extract_opts(info[2].As<Napi::Object>())};
+    NAPI_CHECK(info.Length() == 1, "Hash", "expected 1 argument");
+
+    const auto &args = info[0].As<Napi::Object>();
+    auto worker = new HashWorker{info.Env(),
+                                 args["password"].As<Napi::Buffer<uint8_t>>(),
+                                 args["salt"].As<Napi::Buffer<uint8_t>>(),
+                                 args["secret"].As<Napi::Buffer<uint8_t>>(),
+                                 args["data"].As<Napi::Buffer<uint8_t>>(),
+                                 args["hashLength"].ToNumber(),
+                                 args["m"].ToNumber(),
+                                 args["t"].ToNumber(),
+                                 args["p"].ToNumber(),
+                                 args["version"].ToNumber(),
+                                 args["type"].ToNumber()};
 
     worker->Queue();
     return worker->GetPromise();
@@ -124,4 +110,6 @@ static Napi::Object init(Napi::Env env, Napi::Object exports) {
     return exports;
 }
 
+} // namespace
+
 NODE_API_MODULE(argon2_lib, init)
diff --git a/binding.gyp b/binding.gyp
@@ -69,7 +69,8 @@
               "cflags": ["--coverage"],
               "ldflags": ["-fprofile-arcs", "-ftest-coverage"]
             }]
-          ]
+          ],
+          "defines+": ["NODE_ADDON_API_ENABLE_TYPE_CHECK_ON_AS"]
         }
       }
     }
diff --git a/package.json b/package.json
@@ -60,10 +60,10 @@
     "node-gyp-build": "^4.8.0"
   },
   "devDependencies": {
-    "@types/node": "^20.11.5",
+    "@types/node": "^20.11.17",
     "node-gyp": "^10.0.1",
     "prebuildify": "^6.0.0",
-    "prettier": "^3.2.4",
+    "prettier": "^3.2.5",
     "typescript": "^5.3.3"
   },
   "engines": {
diff --git a/yarn.lock b/yarn.lock

Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,8 @@`
`69`	`69`	`"cflags": ["--coverage"],`
`70`	`70`	`"ldflags": ["-fprofile-arcs", "-ftest-coverage"]`
`71`	`71`	`}]`
`72`		`- ]`
	`72`	`+ ],`
	`73`	`+ "defines+": ["NODE_ADDON_API_ENABLE_TYPE_CHECK_ON_AS"]`
`73`	`74`	`}`
`74`	`75`	`}`
`75`	`76`	`}`