Philipp von Weitershausen [:philikon] Reporter
	Description • 14 years ago

Needs to be compatible with what OpenSSL does (for Fx Home)

	Philipp von Weitershausen [:philikon] Reporter
	Comment 1 • 14 years ago

So we need big int arithmetic for this. This can be done (relatively) easily by using js-ctypes and NSS (freebl3 actually), though that means the method is restricted to Firefox 4. Are we ok with not doing this on Firefox 3.5/3.6?

	Mike Connor [:mconnor]
	Comment 2 • 14 years ago

I think so, given the timeline for getting this up and running and the looming end of 3.x support.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 3 • 14 years ago

Move some general purpose utilities to resource://services-crypto/util.js

This allows us to use them without having to import the world with resource://services-sync/util.js. This is completely backwards compatible to resource://services-sync/util.js

	Philipp von Weitershausen [:philikon] Reporter
	Comment 7 • 14 years ago

Fix random failures/crashes caused by GC issues (thanks to dwitte for spotting the problem).

Sadly we won't be able to use this implementation at all because the relevant functions aren't exposed by the shared libraries. It works on OSX pretty much by accident.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 8 • 14 years ago

Had a chat with bsmith earlier, jotting down some of his concerns:

* Right now we use SHA1 in the zero-knowledge proofs which apparently isn't optimal. We might want to provide at least the option to use SHA256 here, even if OpenSSL doesn't support it right now. Then again, OpenSSL compatibility might not be such a big issue as we could always fork their jpake.c for Fx Home.

* Currently we use SHA256 to turn the key resulting from the J-PAKE mechanism into something with uniformly distributed randomness. It has been suggested to use an HMAC as the key extraction mechanism instead. See bug 609079.

* As of now we use the Diffie-Hellmann group with the smallest modulus (1024 bit) which roughly corresponds to an 80 bit key. We should probably use the 3072 bit one if we're looking to get a 256 bit AES key in the end. This will have some  perf impact, particularly on mobile, but we don't have to worry so much about blocking since this setup UI would be modal anyway. In the worst case we'll have to use the ChromeWorker approach here, too.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 9 • 14 years ago

Moved to beta8 due to a hard dependency on this being available at the same time Fennec 2 Beta 3 ships.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 10 • 14 years ago

When changing from SHA-1 to SHA-256 as explained above, switch from using FreeBL's SHA*_* functions directly to using nsICryptoHash. Also, use nsICryptoHMAC instead of directly using FreeBL's HMAC functions or PKCS#11 HMAC functions.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 11 • 14 years ago

(In reply to comment #8)
> * Currently we use SHA256 to turn the key resulting from the J-PAKE mechanism
> into something with uniformly distributed randomness. It has been suggested to
> use an HMAC as the key extraction mechanism instead. See bug 609079.

We will use HMAC-SHA256() with a fixed 256 bit key.

> * As of now we use the Diffie-Hellmann group with the smallest modulus (1024
> bit) which roughly corresponds to an 80 bit key. We should probably use the
> 3072 bit one if we're looking to get a 256 bit AES key in the end. This will
> have some  perf impact, particularly on mobile, but we don't have to worry so
> much about blocking since this setup UI would be modal anyway. In the worst
> case we'll have to use the ChromeWorker approach here, too.

The 3072 bit modulus group is available and just has to be used by the consumer of the J-PAKE API (in our case bug 602876).

	Philipp von Weitershausen [:philikon] Reporter
	Comment 12 • 14 years ago

Comment on attachment 481839 [details] [diff] [review]
Prereq 1 (v1): Move some general purpose utilities to crypto

># HG changeset patch
># Parent bcff2c38b53eb70cbf8a175ca1f87bd727eb5c67
># User Philipp von Weitershausen <philipp@weitershausen.de>
>Bug 601645 - Move some general purpose utilities to resource://services-crypto/util.js
>
>This allows us to use them without having to import the world with resource://services-sync/util.js.
>This is completely backwards compatible to resource://services-sync/util.js
>
>diff --git a/Makefile b/Makefile
>--- a/Makefile
>+++ b/Makefile
>@@ -172,16 +172,17 @@
> 	$(SLINK) $(TOPSRCDIR)/ui/firefox/AboutWeaveTabs.js $(stage_dir)/components/AboutWeaveTabs.js
> 	
> 	$(MKDIR) $(stage_dir)/chrome/locale/en-US/services
> 	$(SLINK) $(TOPSRCDIR)/services/sync/locales/en-US/errors.properties $(stage_dir)/chrome/locale/en-US/services/errors.properties
> 	$(SLINK) $(TOPSRCDIR)/services/sync/locales/en-US/sync.properties $(stage_dir)/chrome/locale/en-US/services/sync.properties
> 	
> 	$(MKDIR) $(stage_dir)/crypto-modules
> 	$(SLINK) $(TOPSRCDIR)/services/crypto/modules/WeaveCrypto.js $(stage_dir)/crypto-modules/WeaveCrypto.js
>+	$(SLINK) $(TOPSRCDIR)/services/crypto/modules/util.js $(stage_dir)/crypto-modules/util.js
> 	$(MKDIR) $(stage_dir)/modules
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/auth.js $(stage_dir)/modules/auth.js
> 	$(MKDIR) $(stage_dir)/modules/base_records
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/base_records/collection.js $(stage_dir)/modules/base_records/collection.js
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/base_records/crypto.js $(stage_dir)/modules/base_records/crypto.js
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/base_records/keys.js $(stage_dir)/modules/base_records/keys.js
> 	$(SLINK) $(TOPSRCDIR)/services/sync/modules/base_records/wbo.js $(stage_dir)/modules/base_records/wbo.js
> 	$(SUBST) $(TOPSRCDIR)/services/sync/modules/constants.js > $(stage_dir)/modules/constants.js
>diff --git a/services/crypto/modules/util.js b/services/crypto/modules/util.js
>new file mode 100644
>--- /dev/null
>+++ b/services/crypto/modules/util.js
>@@ -0,0 +1,183 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Firefox Sync.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Dan Mills <thunder@mozilla.com>
>+ *  Philipp von Weitershausen <philipp@weitershausen.de>
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+const EXPORTED_SYMBOLS = ['Utils'];
>+
>+const Cc = Components.classes;
>+const Ci = Components.interfaces;
>+const Cr = Components.results;
>+const Cu = Components.utils;
>+
>+let Utils = {
>+
>+  deepEquals: function eq(a, b) {
>+    // If they're triple equals, then it must be equals!
>+    if (a === b)
>+      return true;
>+
>+    // If they weren't equal, they must be objects to be different
>+    if (typeof a != "object" || typeof b != "object")
>+      return false;
>+
>+    // But null objects won't have properties to compare
>+    if (a === null || b === null)
>+      return false;
>+
>+    // Make sure all of a's keys have a matching value in b
>+    for (let k in a)
>+      if (!eq(a[k], b[k]))
>+        return false;
>+
>+    // Do the same for b's keys but skip those that we already checked
>+    for (let k in b)
>+      if (!(k in a) && !eq(a[k], b[k]))
>+        return false;
>+
>+    return true;
>+  },
>+
>+  digest: function digest(message, hasher) {
>+    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>+      createInstance(Ci.nsIScriptableUnicodeConverter);
>+    converter.charset = "UTF-8";
>+
>+    let data = converter.convertToByteArray(message, {});
>+    hasher.update(data, data.length);
>+    return hasher.finish(false);
>+  },
>+
>+  bytesAsHex: function bytesAsHex(bytes) {
>+    // Convert each hashed byte into 2-hex strings then combine them
>+    return [("0" + byte.charCodeAt().toString(16)).slice(-2)
>+            for each (byte in bytes)].join("");
>+  },
>+
>+  sha1Bytes: function sha1Bytes(message) {
>+    let hasher = Cc["@mozilla.org/security/hash;1"].
>+      createInstance(Ci.nsICryptoHash);
>+    hasher.init(hasher.SHA1);
>+    return Utils.digest(message, hasher);
>+  },
>+
>+  sha1: function sha1(message) {
>+    return Utils.bytesAsHex(Utils.sha1Bytes(message));
>+  },
>+
>+  sha1Base32: function sha1Base32(message) {
>+    return Utils.encodeBase32(Utils.sha1Bytes(message));
>+  },
>+
>+  /**
>+   * Generate a sha256 HMAC for a string message and a given nsIKeyObject
>+   */
>+  sha256HMAC: function sha256HMAC(message, key) {
>+    let hasher = Cc["@mozilla.org/security/hmac;1"].
>+      createInstance(Ci.nsICryptoHMAC);
>+    hasher.init(hasher.SHA256, key);
>+    return Utils.bytesAsHex(Utils.digest(message, hasher));
>+  },
>+
>+  /**
>+   * Base32 encode (RFC 4648) a string
>+   */
>+  encodeBase32: function encodeBase32(bytes) {
>+    const key = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
>+    let quanta = Math.floor(bytes.length / 5);
>+    let leftover = bytes.length % 5;
>+
>+    // Pad the last quantum with zeros so the length is a multiple of 5.
>+    if (leftover) {
>+      quanta += 1;
>+      for (let i = leftover; i < 5; i++)
>+        bytes += "\0";
>+    }
>+
>+    // Chop the string into quanta of 5 bytes (40 bits). Each quantum
>+    // is turned into 8 characters from the 32 character base.
>+    let ret = "";
>+    for (let i = 0; i < bytes.length; i += 5) {
>+      let c = [byte.charCodeAt() for each (byte in bytes.slice(i, i + 5))];
>+      ret += key[c[0] >> 3]
>+           + key[((c[0] << 2) & 0x1f) | (c[1] >> 6)]
>+           + key[(c[1] >> 1) & 0x1f]
>+           + key[((c[1] << 4) & 0x1f) | (c[2] >> 4)]
>+           + key[((c[2] << 1) & 0x1f) | (c[3] >> 7)]
>+           + key[(c[3] >> 2) & 0x1f]
>+           + key[((c[3] << 3) & 0x1f) | (c[4] >> 5)]
>+           + key[c[4] & 0x1f];
>+    }
>+
>+    switch (leftover) {
>+      case 1:
>+        return ret.slice(0, -6) + "======";
>+      case 2:
>+        return ret.slice(0, -4) + "====";
>+      case 3:
>+        return ret.slice(0, -3) + "===";
>+      case 4:
>+        return ret.slice(0, -1) + "=";
>+      default:
>+        return ret;
>+    }
>+  },
>+
>+  encodeUTF8: function encodeUTF8(str) {
>+    try {
>+      var unicodeConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>+                             .createInstance(Ci.nsIScriptableUnicodeConverter);
>+      unicodeConverter.charset = "UTF-8";
>+      str = unicodeConverter.ConvertFromUnicode(str);
>+      return str + unicodeConverter.Finish();
>+    } catch(ex) {
>+      return null;
>+    }
>+  },
>+
>+  decodeUTF8: function encodeUTF8(str) {
>+    try {
>+      var unicodeConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>+                             .createInstance(Ci.nsIScriptableUnicodeConverter);
>+      unicodeConverter.charset = "UTF-8";
>+      str = unicodeConverter.ConvertToUnicode(str);
>+      return str + unicodeConverter.Finish();
>+    } catch(ex) {
>+      return null;
>+    }
>+  }
>+
>+};
>diff --git a/services/sync/tests/unit/test_utils_deepEquals.js b/services/crypto/tests/unit/test_utils_deepEquals.js
>rename from services/sync/tests/unit/test_utils_deepEquals.js
>rename to services/crypto/tests/unit/test_utils_deepEquals.js
>--- a/services/sync/tests/unit/test_utils_deepEquals.js
>+++ b/services/crypto/tests/unit/test_utils_deepEquals.js
>@@ -1,10 +1,10 @@
> _("Make sure Utils.deepEquals correctly finds items that are deeply equal");
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>   let data = '[NaN, undefined, null, true, false, Infinity, 0, 1, "a", "b", {a: 1}, {a: "a"}, [{a: 1}], [{a: true}], {a: 1, b: 2}, [1, 2], [1, 2, 3]]';
>   _("Generating two copies of data:", data);
>   let d1 = eval(data);
>   let d2 = eval(data);
> 
>   d1.forEach(function(a) {
>diff --git a/services/sync/tests/unit/test_utils_encodeBase32.js b/services/crypto/tests/unit/test_utils_encodeBase32.js
>rename from services/sync/tests/unit/test_utils_encodeBase32.js
>rename to services/crypto/tests/unit/test_utils_encodeBase32.js
>--- a/services/sync/tests/unit/test_utils_encodeBase32.js
>+++ b/services/crypto/tests/unit/test_utils_encodeBase32.js
>@@ -1,9 +1,9 @@
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>   // Test vectors from RFC 4648
>   do_check_eq(Utils.encodeBase32(""), "");
>   do_check_eq(Utils.encodeBase32("f"), "MY======");
>   do_check_eq(Utils.encodeBase32("fo"), "MZXQ====");
>   do_check_eq(Utils.encodeBase32("foo"), "MZXW6===");
>   do_check_eq(Utils.encodeBase32("foob"), "MZXW6YQ=");
>diff --git a/services/sync/tests/unit/test_utils_sha1.js b/services/crypto/tests/unit/test_utils_sha1.js
>rename from services/sync/tests/unit/test_utils_sha1.js
>rename to services/crypto/tests/unit/test_utils_sha1.js
>--- a/services/sync/tests/unit/test_utils_sha1.js
>+++ b/services/crypto/tests/unit/test_utils_sha1.js
>@@ -1,10 +1,10 @@
> _("Make sure sha1 digests works with various messages");
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>   let mes1 = "hello";
>   let mes2 = "world";
> 
>   _("Make sure right sha1 digests are generated");
>   let dig1 = Utils.sha1(mes1);
>   do_check_eq(dig1, "aaf4c61ddcc5e8a2dabede0f3b482cd9aea9434d");
>diff --git a/services/sync/tests/unit/test_utils_sha256HMAC.js b/services/crypto/tests/unit/test_utils_sha256HMAC.js
>rename from services/sync/tests/unit/test_utils_sha256HMAC.js
>rename to services/crypto/tests/unit/test_utils_sha256HMAC.js
>--- a/services/sync/tests/unit/test_utils_sha256HMAC.js
>+++ b/services/crypto/tests/unit/test_utils_sha256HMAC.js
>@@ -1,14 +1,16 @@
> _("Make sure sha256 hmac works with various messages and keys");
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>-  let key1 = Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, "key1");
>-  let key2 = Svc.KeyFactory.keyFromString(Ci.nsIKeyObject.HMAC, "key2");
>+  let keyFactory = Cc["@mozilla.org/security/keyobjectfactory;1"]
>+                     .getService(Ci.nsIKeyObjectFactory);
>+  let key1 = keyFactory.keyFromString(Ci.nsIKeyObject.HMAC, "key1");
>+  let key2 = keyFactory.keyFromString(Ci.nsIKeyObject.HMAC, "key2");
> 
>   let mes1 = "message 1";
>   let mes2 = "message 2";
> 
>   _("Make sure right sha256 hmacs are generated");
>   let hmac11 = Utils.sha256HMAC(mes1, key1);
>   do_check_eq(hmac11, "54f035bfbd6b44445d771c7c430e0753f1c00823974108fb4723703782552296");
>   let hmac12 = Utils.sha256HMAC(mes1, key2);
>diff --git a/services/sync/tests/unit/test_utils_utf8.js b/services/crypto/tests/unit/test_utils_utf8.js
>rename from services/sync/tests/unit/test_utils_utf8.js
>rename to services/crypto/tests/unit/test_utils_utf8.js
>--- a/services/sync/tests/unit/test_utils_utf8.js
>+++ b/services/crypto/tests/unit/test_utils_utf8.js
>@@ -1,8 +1,8 @@
>-Cu.import("resource://services-sync/util.js");
>+Cu.import("resource://services-crypto/util.js");
> 
> function run_test() {
>   let str = "Umlaute: \u00FC \u00E4\n"; // Umlaute: ü ä
>   let encoded = Utils.encodeUTF8(str);
>   let decoded = Utils.decodeUTF8(encoded);
>   do_check_eq(decoded, str);
> }
>diff --git a/services/sync/modules/util.js b/services/sync/modules/util.js
>--- a/services/sync/modules/util.js
>+++ b/services/sync/modules/util.js
>@@ -43,21 +43,26 @@
> 
> Cu.import("resource://services-sync/constants.js");
> Cu.import("resource://services-sync/ext/Observers.js");
> Cu.import("resource://services-sync/ext/Preferences.js");
> Cu.import("resource://services-sync/ext/StringBundle.js");
> Cu.import("resource://services-sync/ext/Sync.js");
> Cu.import("resource://services-sync/log4moz.js");
> 
>+let Crypto = {};
>+Cu.import("resource://services-crypto/util.js", Crypto);
>+
> /*
>  * Utility functions
>  */
> 
> let Utils = {
>+  __proto__: Crypto.Utils,
>+
>   /**
>    * Wrap a function to catch all exceptions and log them
>    *
>    * @usage MyObj._catch = Utils.catch;
>    *        MyObj.foo = function() { this._catch(func)(); }
>    */
>   catch: function Utils_catch(func) {
>     let thisArg = this;
>@@ -378,42 +383,16 @@
>     dest.__defineGetter__(prop, getter);
>   },
> 
>   lazyStrings: function Weave_lazyStrings(name) {
>     let bundle = "chrome://weave/locale/services/" + name + ".properties";
>     return function() new StringBundle(bundle);
>   },
> 
>-  deepEquals: function eq(a, b) {
>-    // If they're triple equals, then it must be equals!
>-    if (a === b)
>-      return true;
>-
>-    // If they weren't equal, they must be objects to be different
>-    if (typeof a != "object" || typeof b != "object")
>-      return false;
>-
>-    // But null objects won't have properties to compare
>-    if (a === null || b === null)
>-      return false;
>-
>-    // Make sure all of a's keys have a matching value in b
>-    for (let k in a)
>-      if (!eq(a[k], b[k]))
>-        return false;
>-
>-    // Do the same for b's keys but skip those that we already checked
>-    for (let k in b)
>-      if (!(k in a) && !eq(a[k], b[k]))
>-        return false;
>-
>-    return true;
>-  },
>-
>   deepCopy: function Weave_deepCopy(thing, noSort) {
>     if (typeof(thing) != "object" || thing == null)
>       return thing;
>     let ret;
> 
>     if (Utils.isArray(thing)) {
>       ret = [];
>       for (let i = 0; i < thing.length; i++)
>@@ -497,102 +476,16 @@
> 
>     return false;
>   },
> 
>   ensureStatus: function Weave_ensureStatus(args) {
>     if (!Utils.checkStatus.apply(Utils, arguments))
>       throw 'checkStatus failed';
>   },
>-
>-  digest: function digest(message, hasher) {
>-    let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
>-      createInstance(Ci.nsIScriptableUnicodeConverter);
>-    converter.charset = "UTF-8";
>-
>-    let data = converter.convertToByteArray(message, {});
>-    hasher.update(data, data.length);
>-    return hasher.finish(false);
>-  },
>-
>-  bytesAsHex: function bytesAsHex(bytes) {
>-    // Convert each hashed byte into 2-hex strings then combine them
>-    return [("0" + byte.charCodeAt().toString(16)).slice(-2)
>-            for each (byte in bytes)].join("");
>-  },
>-
>-  _sha1: function _sha1(message) {
>-    let hasher = Cc["@mozilla.org/security/hash;1"].
>-      createInstance(Ci.nsICryptoHash);
>-    hasher.init(hasher.SHA1);
>-    return Utils.digest(message, hasher);
>-  },
>-
>-  sha1: function sha1(message) {
>-    return Utils.bytesAsHex(Utils._sha1(message));
>-  },
>-
>-  sha1Base32: function sha1Base32(message) {
>-    return Utils.encodeBase32(Utils._sha1(message));
>-  },
>-
>-  /**
>-   * Generate a sha256 HMAC for a string message and a given nsIKeyObject
>-   */
>-  sha256HMAC: function sha256HMAC(message, key) {
>-    let hasher = Cc["@mozilla.org/security/hmac;1"].
>-      createInstance(Ci.nsICryptoHMAC);
>-    hasher.init(hasher.SHA256, key);
>-    return Utils.bytesAsHex(Utils.digest(message, hasher));
>-  },
>-
>-  /**
>-   * Base32 encode (RFC 4648) a string
>-   */
>-  encodeBase32: function encodeBase32(bytes) {
>-    const key = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
>-    let quanta = Math.floor(bytes.length / 5);
>-    let leftover = bytes.length % 5;
>-
>-    // Pad the last quantum with zeros so the length is a multiple of 5.
>-    if (leftover) {
>-      quanta += 1;
>-      for (let i = leftover; i < 5; i++)
>-        bytes += "\0";
>-    }
>-
>-    // Chop the string into quanta of 5 bytes (40 bits). Each quantum
>-    // is turned into 8 characters from the 32 character base.
>-    let ret = "";
>-    for (let i = 0; i < bytes.length; i += 5) {
>-      let c = [byte.charCodeAt() for each (byte in bytes.slice(i, i + 5))];
>-      ret += key[c[0] >> 3]
>-           + key[((c[0] << 2) & 0x1f) | (c[1] >> 6)]
>-           + key[(c[1] >> 1) & 0x1f]
>-           + key[((c[1] << 4) & 0x1f) | (c[2] >> 4)]
>-           + key[((c[2] << 1) & 0x1f) | (c[3] >> 7)]
>-           + key[(c[3] >> 2) & 0x1f]
>-           + key[((c[3] << 3) & 0x1f) | (c[4] >> 5)]
>-           + key[c[4] & 0x1f];
>-    }
>-
>-    switch (leftover) {
>-      case 1:
>-        return ret.slice(0, -6) + "======";
>-      case 2:
>-        return ret.slice(0, -4) + "====";
>-      case 3:
>-        return ret.slice(0, -3) + "===";
>-      case 4:
>-        return ret.slice(0, -1) + "=";
>-      default:
>-        return ret;
>-    }
>-  },
>-
>   makeURI: function Weave_makeURI(URIString) {
>     if (!URIString)
>       return null;
>     try {
>       return Svc.IO.newURI(URIString, null, null);
>     } catch (e) {
>       let log = Log4Moz.repository.getLogger("Service.Util");
>       log.debug("Could not create URI: " + Utils.exceptionStr(e));
>@@ -801,40 +694,16 @@
>   readStream: function Weave_readStream(is) {
>     let ret = "", str = {};
>     while (is.readString(4096, str) != 0) {
>       ret += str.value;
>     }
>     return ret;
>   },
> 
>-  encodeUTF8: function(str) {
>-    try {
>-      var unicodeConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>-                             .createInstance(Ci.nsIScriptableUnicodeConverter);
>-      unicodeConverter.charset = "UTF-8";
>-      str = unicodeConverter.ConvertFromUnicode(str);
>-      return str + unicodeConverter.Finish();
>-    } catch(ex) {
>-      return null;
>-    }
>-  },
>-
>-  decodeUTF8: function(str) {
>-    try {
>-      var unicodeConverter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
>-                             .createInstance(Ci.nsIScriptableUnicodeConverter);
>-      unicodeConverter.charset = "UTF-8";
>-      str = unicodeConverter.ConvertToUnicode(str);
>-      return str + unicodeConverter.Finish();
>-    } catch(ex) {
>-      return null;
>-    }
>-  },
>-
>   /**
>    * Generate 20 random characters a-z
>    */
>   generatePassphrase: function() {
>     let rng = Cc["@mozilla.org/security/random-generator;1"]
>                 .createInstance(Ci.nsIRandomGenerator);
>     let bytes = rng.generateRandomBytes(20);
>     return [String.fromCharCode(97 + Math.floor(byte * 26 / 256))

	Philipp von Weitershausen [:philikon] Reporter
	Comment 13 • 14 years ago

Comment on attachment 481840 [details] [diff] [review]
Prereq 2 (v1): Implement Utils.sha256()

># HG changeset patch
># Parent 097b68f44f00d6d8da06dbe031ef1d2fee93815e
># User Philipp von Weitershausen <philipp@weitershausen.de>
>Bug 601645 - Implement Utils.sha256()
>
>diff --git a/services/crypto/modules/util.js b/services/crypto/modules/util.js
>--- a/services/crypto/modules/util.js
>+++ b/services/crypto/modules/util.js
>@@ -97,16 +97,27 @@
>   sha1: function sha1(message) {
>     return Utils.bytesAsHex(Utils.sha1Bytes(message));
>   },
> 
>   sha1Base32: function sha1Base32(message) {
>     return Utils.encodeBase32(Utils.sha1Bytes(message));
>   },
> 
>+  sha256Bytes: function sha256Bytes(message) {
>+    let hasher = Cc["@mozilla.org/security/hash;1"].
>+      createInstance(Ci.nsICryptoHash);
>+    hasher.init(hasher.SHA256);
>+    return Utils.digest(message, hasher);
>+  },
>+
>+  sha256: function sha256(message) {
>+    return Utils.bytesAsHex(Utils.sha256Bytes(message));
>+  },
>+
>   /**
>    * Generate a sha256 HMAC for a string message and a given nsIKeyObject
>    */
>   sha256HMAC: function sha256HMAC(message, key) {
>     let hasher = Cc["@mozilla.org/security/hmac;1"].
>       createInstance(Ci.nsICryptoHMAC);
>     hasher.init(hasher.SHA256, key);
>     return Utils.bytesAsHex(Utils.digest(message, hasher));
>diff --git a/services/crypto/tests/unit/test_utils_sha256.js b/services/crypto/tests/unit/test_utils_sha256.js
>new file mode 100644
>--- /dev/null
>+++ b/services/crypto/tests/unit/test_utils_sha256.js
>@@ -0,0 +1,15 @@
>+Cu.import("resource://services-crypto/util.js");
>+
>+function run_test() {
>+  _("Hex variant");
>+  do_check_eq(Utils.sha256(""),
>+              "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855");
>+  do_check_eq(Utils.sha256("Bacon is a vegetable"),
>+              "684f507d70b0f5145967e77f6a12142fdc90a48d06658ece12f45fbbd404ceb2");
>+
>+  _("Bytes variant");
>+  do_check_eq(Utils.sha256Bytes("Cookies are delicious delicacies"),
>+              "\xd1U9ah\x06\t'\xd8\xbfH\xe3\xcal\xc9\xc2\xd0\xdd\xbaC\xe9\xef`6\x18hS\xf12\x9a\x95\xce");
>+  do_check_eq(Utils.sha256Bytes("Gort! Klaatu barada nikto!"),
>+              "'\xe8q\x07;\x1c{y\xf7l\xf5\x06\xce\xebY'u\x03\x11\xfb\xfe\x92X\xf9\xf7\xd8\xc6z\xc1af2");
>+}

	Philipp von Weitershausen [:philikon] Reporter
	Comment 14 • 14 years ago

Changes compared to v2 patch:

* Support SHA256 in addition to SHA1 when computing the Schnorr signature for zero-knowledge proofs.

* Support HMAC-SHA256 as a key extraction algorithm in addition to SHA256, using a fixed 256 bit key.

* Avoid using SHA1 functions from freebl, instead use nsICryptoHash.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 15 • 14 years ago

Tests updated to reflect changes in v3 patch. Also added more test vectors generated with python-jpake (3072 bit modulus with SHA1 and SHA256 hashing in the Schnorr signature)

	Philipp von Weitershausen [:philikon] Reporter
	Comment 16 • 14 years ago

Use zeros as extraction key in the HMAC-SHA256 extraction (feedback from bsmith)

	Philipp von Weitershausen [:philikon] Reporter
	Comment 17 • 14 years ago

Updated hmac-sha256 generated test vectors.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 18 • 14 years ago

Use these instructions to build a new version of NSS that supports J-PAKE using the patches from Bug 609068, Bug 609076, and Bug 614076:

cvs co NSPR NSS
cd mozilla/security/nss
patch -p0 < jpake-freebl-bsmith-2.patch
patch -p0 < jpake-freebl-test-bsmith-1.patch
patch -p0 < jpake-softoken-bsmith-1.patch
patch -p0 < jpake-util-bsmith-1.patch
patch -p0 < jpake-pk11mode-bsmith-1.patch
patch -p0 < jpake-bug-609076-hkdf-bsmith-1.patch

Hopefully, you can also be able to apply these patches directly to the NSS that is in mozilla-central with minimal changes.

Look at the changes in jpake-pk11mode-bsmith-1.patch to see what you need to do in Sync to use the NSS J-PAKE implementation. It won't be exactly the same because you need to use the pk11wrap functions instead of directly calling the PKCS#11 functions. To find the corresponding pk11wrap function, I suggest grepping through the pk11wrap directory for the name of the PKCS#11 function, and then look for the functions with a capital PK11_* prefix that call it (directly or indirectly). I will help you with this tomorrow.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 19 • 14 years ago

This patch is NOT designed to be checked into mozilla-central. I just created it so that we could start working on the Sync JS changes needed to interface with it, so that the Sync code is ready to go once the NSS release is checked into mozilla-central.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 21 • 14 years ago

Comment on attachment 495211 [details] [diff] [review]
Patch for testing J-PAKE in mozilla-central until the new NSS release is checked in

Use "python client.py update_nss NSS_3_12_BRANCH" instead.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 22 • 14 years ago

(In reply to comment #21)
> Use "python client.py update_nss NSS_3_12_BRANCH" instead.

Thanks, that works like a charm. I can now start rewriting the JS implementation on top of NSS using your example code now.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 23 • 14 years ago

Updating the summary to better reflect what the bug is about now that J-PAKE landed in NSS. Also assigning back to me.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 25 • 14 years ago

Removed bug 609068, bug 609076, and bug 614076 from the blocker list because the code that Sync needs has been checked into the 3.12.9 release. Those bugs will remain open until the automated tests for the new code to be added but that will probably happen after the NSS 3.12.9 release.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 26 • 14 years ago

Removed bug 609068, bug 609076, and bug 614076 from the blocker list because the code that Sync needs has been checked into the 3.12.9 release. Those bugs will remain open until the automated tests for the new code to be added but that will probably happen after the NSS 3.12.9 release.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 28 • 14 years ago

Dave, this is an empty C++ XPCOM shell that we're going to add an implementation to soon. Could you take a look and see if this makes sense? The idea is to instantiate one of these objects via

 Cc["@mozilla.org/services-crypto/sync-jpake;1"].createInstance(Ci.nsISyncJPAKE)

Thanks!

	Philipp von Weitershausen [:philikon] Reporter
	Comment 29 • 14 years ago

Comment on attachment 491941 [details] [diff] [review]
Implement J-PAKE (v4)

This mpi+ctypes implemetation is now obsolete.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 30 • 14 years ago

JPAKE tests rewritten against the API of the XPCOM component that Brian is working on.

	Dave Townsend [:mossop]
	Comment 31 • 14 years ago

Comment on attachment 495693 [details] [diff] [review]
IDL and XPCOM shell for nsISyncJPAKE v2 (not for check in)

Looks generally fine, made a couple of alternate naming choices since you may or may not have just been following old form, feel free to ignore them, naming is up to you and the module owner.

>diff --git a/services/crypto/Makefile.in b/services/crypto/Makefile.in
>--- a/services/crypto/Makefile.in
>+++ b/services/crypto/Makefile.in
>@@ -43,16 +43,28 @@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE = services-crypto
> XPIDL_MODULE = services-crypto

Might be better just to call this crypto rather than prepending services everywhere.

> XPIDLSRCS = \
>   IWeaveCrypto.idl \
>+  nsISyncJPAKE.idl \

Kind of old form to use nsI but you could equally use syncIJPAKE or maybe since this is the crypto code cryptIJPAKE or something.

>diff --git a/services/crypto/nsISyncJPAKE.idl b/services/crypto/nsISyncJPAKE.idl

I'm not really reviewing anything in here.

>diff --git a/services/crypto/nsSyncJPAKE.cpp b/services/crypto/nsSyncJPAKE.cpp

>+#include "nsISyncJPAKE.h"
>+
>+/*** TODO should this go into a separate nsSyncJPAKE.h file? ***/

Yes that is standard and probably best to stick to that

>+#define NS_SYNCJPAKE_IID \
>+  {0x5ab02a98, 0x5122, 0x4b90, \
>+    { 0x93, 0xcd, 0xf2, 0x59, 0xc4, 0xb4, 0x2e, 0x3a }}

Shouldn't need to define this, it should be in nsISyncJPAKE.h. NS_GET_IID(nsISyncJPAKE) should make it if you need it.

>+#define NS_SYNCJPAKE_CONTRACTID \
>+  "@mozilla.org/services-crypto/sync-jpake;1"

I might suggest just "@mozilla.org/crypto/jpake;1"

>+class nsSyncJPAKE : public nsISyncJPAKE
>+{
>+public:
>+  nsSyncJPAKE();
>+
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSISYNCJPAKE
>+
>+private:
>+  ~nsSyncJPAKE();
>+}

Here is where nsSyncJPAKE.h ends

>+NS_IMPL_ISUPPORTS1(nsSyncJPAKE, nsISyncJPAKE)
>+
>+/*** TODO implementation goes here ***/

>+/*** TODO should this go into a separate nsServicesCryptoModule.cpp file? ***/

Kind of a judgement call. There is only one component so there is not much point right now. If you add more in the future then you'll definitely want to split it out then for clarity so I guess if you reasonably suspect you'll add any more in the future then might as well split it now and hg blame will be cleaner for it.

>+NS_GENERIC_FACTORY_CONSTRUCTOR(nsSyncJPAKE)

How badly would the implementation break if people created multiple instances of it? You may want to consider using NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR to make that impossible though it is a little extra work on your part.

>+NS_DEFINE_NAMED_CID(NS_ISYNCJPAKE_CID);
>+
>+static const mozilla::Module::CIDEntry kServicesCryptoCIDs[] = {
>+  { &kNS_ISYNCJPAKE_CID, false, NULL, nsSyncJPAKEConstructor },
>+  { NULL }
>+};
>+
>+static const mozilla::Module::ContractIDEntry kServicesCryptoContracts[] = {
>+  { NS_SYNCJPAKE_CONTRACTID, &kNS_ISYNCJPAKE_CID },
>+  { NULL }
>+};
>+
>+static const mozilla::Module kServicesCryptoModule = {
>+  mozilla::Module::kVersion,
>+  kServicesCryptoCIDs,
>+  kServicesCryptoContracts,
>+  kServicesCryptoCategories

I don't see kServicesCryptoCategories defined anywhere so I'm guessing this doesn't compile yet. You don't need it anyway normally.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 32 • 14 years ago

Thanks a lot for the feedback, Dave!

(In reply to comment #31)
> Looks generally fine, made a couple of alternate naming choices since you may
> or may not have just been following old form, feel free to ignore them, naming
> is up to you and the module owner.

Ok, I'll have mconnor have the last word here. ;)

> >+#define NS_SYNCJPAKE_CONTRACTID \
> >+  "@mozilla.org/services-crypto/sync-jpake;1"
> 
> I might suggest just "@mozilla.org/crypto/jpake;1"

I had that initially. But this is a particular JPAKE API that's 100% tailoured to Sync's credentials exchange, so I think the naming should reflect that.

> >+NS_GENERIC_FACTORY_CONSTRUCTOR(nsSyncJPAKE)
> 
> How badly would the implementation break if people created multiple instances
> of it? You may want to consider using NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR
> to make that impossible though it is a little extra work on your part.

Actually, we specifically want to allow multiple instances of this.

> >+static const mozilla::Module kServicesCryptoModule = {
> >+  mozilla::Module::kVersion,
> >+  kServicesCryptoCIDs,
> >+  kServicesCryptoContracts,
> >+  kServicesCryptoCategories
> 
> I don't see kServicesCryptoCategories defined anywhere so I'm guessing this
> doesn't compile yet. You don't need it anyway normally.

Good point, I just copied it from other modules ;). So what to use here, NULL?

	Philipp von Weitershausen [:philikon] Reporter
	Comment 33 • 14 years ago

Incorporate Mossop's feedback. This one actually compiles! \o/

	Dave Townsend [:mossop]
	Comment 34 • 14 years ago

(In reply to comment #32)
> > >+static const mozilla::Module kServicesCryptoModule = {
> > >+  mozilla::Module::kVersion,
> > >+  kServicesCryptoCIDs,
> > >+  kServicesCryptoContracts,
> > >+  kServicesCryptoCategories
> > 
> > I don't see kServicesCryptoCategories defined anywhere so I'm guessing this
> > doesn't compile yet. You don't need it anyway normally.
> 
> Good point, I just copied it from other modules ;). So what to use here, NULL?

You just shouldn't need to include it at all.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 36 • 14 years ago

Switched to nsStringAPI, switched to 2-space indent, and added some missing return value logic.

At this point, it compiles but it doesn't link. I'm not sure where to add the references to the necessary libraries (pk11wrap and the string library).

	Philipp von Weitershausen [:philikon] Reporter
	Comment 37 • 14 years ago

(In reply to comment #36)
> Switched to nsStringAPI, switched to 2-space indent, and added some missing
> return value logic.

Thanks for the quick fix!

> At this point, it compiles but it doesn't link. I'm not sure where to add the
> references to the necessary libraries (pk11wrap and the string library).

Needed to add this to the very *bottom* of Makefile.in:

EXTRA_DSO_LDOPTS += \
	$(XPCOM_GLUE_LDOPTS) \
	$(NSPR_LIBS) \
	$(NSS_LIBS) \
	$(NULL)

Now builds fine, but the unit test fails with:

WARNING: NS_ENSURE_TRUE(slot != __null) failed: file /Users/philipp/dev/mozilla-weave/mc-jpake/services/crypto/nsSyncJPAKE.cpp, line 166
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsISyncJPAKE.round1]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: /Users/philipp/dev/mozilla-weave/mc-jpake/obj-ff-dbg/_tests/xpcshell/services/crypto/tests/unit/test_jpake.js :: test_same_signerids :: line 25"  data: no]

	Mike Connor [:mconnor]
	Comment 38 • 14 years ago

Comment on attachment 495729 [details] [diff] [review]
Tests for XPCOM J-PAKE implementation

these look good, but definitely would like Brian's feedback as well.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 39 • 14 years ago

This depends on the branch version of the patch in bug 617492.

TEST-PASS | c:\p\mc\ff-dbg\_tests\xpcshell\services\crypto\tests\unit\test_jpake.js | test passed
INFO | Result summary:
INFO | Passed: 4
INFO | Failed: 0

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 40 • 14 years ago

Comment on attachment 495729 [details] [diff] [review]
Tests for XPCOM J-PAKE implementation

1. Add the PSM initialization to the top of the file: Cc["@mozilla.org/psm;1"].getService(Ci.nsISupports);

2. Move test_success() to be the first sub-test. This way, if we get errors from NSS during the first sub-test, it probably means there is a bug in this code or in NSS; if the first sub-test passes, and then we get errors, then it probably means that the code is probably correct.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 41 • 14 years ago

(In reply to comment #39)
> Created attachment 496105 [details] [diff] [review]
> 
> This depends on the branch version of the patch in bug 617492.

... and the first change to the test (to initialize PSM) mentioned in comment 39.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 42 • 14 years ago

With base64 -> base16 for key confirmation + build system changes + philiKON's build system changes.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 43 • 14 years ago

Incorporate bsmith's feedback:
* Initialise PSM at the beginning of the test
* Change order of tests

	Philipp von Weitershausen [:philikon] Reporter
	Comment 44 • 14 years ago

Update tests: we're going to get rid of the aKeyVerification parameter in the final() method.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 45 • 14 years ago

With changes to support new key confirmation.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 46 • 14 years ago

Firefox and Firefox are not deriving the same keys from the J-PAKE exchange. Stefan, philiKON, and I are debugging that now.

	Mike Connor [:mconnor]
	Comment 47 • 14 years ago

Comment on attachment 496345 [details] [diff] [review]
IDL, XPCOM shell, and implementation for nsISyncJPAKE (v7)

Overall this looks fine, though the style is a bit odd in places.  Don't really care right now though.

>diff --git a/services/crypto/nsISyncJPAKE.idl b/services/crypto/nsISyncJPAKE.idl

>+  /**
>+   * Perform first round of the JPAKE exchange.

round2 is second round, I hope.

>+  void round2(in ACString aPeerID,

>diff --git a/services/crypto/nsSyncJPAKE.cpp b/services/crypto/nsSyncJPAKE.cpp

>+#define NUM_ELEM(x) (sizeof(x) / sizeof (x)[0])
>+
>+// TODO: validate PQG

Is there a bug on this and other TODO comments?  If so, please note in the comment, if not, file and note in the comment.

>+//NS_DEFINE_STATIC_IID_ACCESSOR(nsISyncJPAKE, NS_ISYNCJPAKE_IID)

if not needed, please remove

	Philipp von Weitershausen [:philikon] Reporter
	Comment 48 • 14 years ago

This gets us to build as part of libxul, despite being an app tier, by adding some declarations to confvars.sh and build.mk. Needed changes to nsSyncCrypto because we're now using interal strings (diverging APIs FTL).

Thanks a lot of mwu for pointing me to the right bits.

	Matt Brubeck (:mbrubeck)
	Comment 49 • 14 years ago

(In reply to comment #48)
> This gets us to build as part of libxul, despite being an app tier, by adding
> some declarations to confvars.sh and build.mk.

If you want to do the same for Fennec, you will need to modify confvars.sh in the mobile-browser repo too.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 50 • 14 years ago

It would be very useful if we could replicate at least one of the standard test vectors in the Objective-C implementation. For example, test vector (prk3, info3, expectedValue3). It is very strange that Python, Obj-C, and Javascript agree with each other and the NSS C implementation agrees with the spec test vectors, but the test case for interop NSS <-> Obj-C fails. Maybe there is some disagreement between the algorithm description on our wiki (used for the scripting language implementations) and the actual spec (which I based the NSS implementation on).

If somebody wants to look over my test to make sure I am not making some dumb mistake that is causing a false test failure, please do so. This file is mozilla/security/nss/cmd/pk11mode.c where I am adding the tests for the HKDF. The function with the actual tests is PKM_HKDFTest.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 51 • 14 years ago

Comment on attachment 496413 [details]
Updated pk11mode.c that I'm using to test HKDF implementation against expected values

It seems like the cause of the interoperability problem has been identified and a separate bug has been filed about it. No changes to NSS's HKDF implementation will be needed. I will upload an updated versions of my patches shortly.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 52 • 14 years ago

My patch needs the following change:

+  PK11SymKey * keyMaterial = PK11_Derive(key, CKM_NSS_JPAKE_FINAL_SHA256,
+                                         &paramsItem, CKM_NSS_HKDF_SHA256,
+                                         CKA_DERIVE, 0);
+  PK11SymKey * expanded = NULL;

   if (keyMaterial == NULL)
     rv = mapErrno();

+  if (rv == NS_OK) {

I will fix this in the next version after we have verified interoperability with FF home in the morning.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 53 • 14 years ago

Comment on attachment 496410 [details] [diff] [review]
Patch on top of v7 to build as part of libxul

Requesting feedback from Mark Finkle to ensure the Fennec guys are happy with this.

Also requesting feedback from Mike Hommey. Mike has experience with Firefox on top of XULRunner. Basically this is an XPCOM component that lives in services (part of app-tier) but should be in libxul. We're using the same tricks that Thunderbird/SeaMonkey do to make that happen.

	Mike Hommey [:glandium]
	Comment 54 • 14 years ago

Comment on attachment 496410 [details] [diff] [review]
Patch on top of v7 to build as part of libxul

The problem I can see, with the patch as is, is that the services/crypto directory is not going to be built in ffx-on-xr cases. But there is currently nothing allowing that to happen in the build system.

I do wonder, however, if there isn't an interest in making this component part of the platform instead of the app (and here I mean the crypto component, not the whole sync stack).

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 55 • 14 years ago

Comment on attachment 496410 [details] [diff] [review]
Patch on top of v7 to build as part of libxul

Fennec would be willing to take these changes, in order to expedite getting JPAKE in the product. I do agree with Mike about moving the component into the platform. What are the downsides to that?

	Mike Connor [:mconnor]
	Comment 56 • 14 years ago

Moving just the crypto component is probably okay.  Not ideal, but okay.  What's the best way to do that?

	Benjamin Smedberg
	Comment 57 • 14 years ago

Why is this part of libxul? I helped you *not* make it part of libxul yesterday, no? I don't think we should do the hybrid approach.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 58 • 14 years ago

> I do wonder, however, if there isn't an interest in making this component part
> of the platform instead of the app (and here I mean the crypto component, not
> the whole sync stack).

The crypto component is pretty Sync-specific because of the string parameter encoding/decoding and the API is generally unstable. I don't care where it goes but if it will be considered part of platform then after B8 we need to change the interface to make it suitable as a platform API.

	Benjamin Smedberg
	Comment 59 • 14 years ago

As decided on IRC: we're going to make this particular binary component part of libxul for b8 (and perhaps for final): it should be part of tier_toolkit.

	Benjamin Smedberg
	Comment 61 • 14 years ago

Comment on attachment 496549 [details] [diff] [review]
Patch on top of v7 to build as part of toolkit

I think that EXTRA_DSO_LDOPTS is probably incorrect here: you want to keep it after rules.mk, and it should be MOZ_COMPONENT_LIBS. But it would only matter in non-libxul builds.

But the critical problem here is that the ordering of toolkit-tiers.mk is definitely wrong: you're building services/crypto *after* toolkit/library, and that's definitely not what you want. Did you do a tryserver or clean build of this patch?

	Philipp von Weitershausen [:philikon] Reporter
	Comment 62 • 14 years ago

Incorporated bsmedberg's review comment (quite right about the ordering!)

	Benjamin Smedberg
	Comment 63 • 14 years ago

Comment on attachment 496557 [details] [diff] [review]
Build as part of toolkit (v2)

nit, there's a hard tab in services/crypto/Makefile.in which should be removed

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 64 • 14 years ago

I am having some trouble getting the merged patch to build but I think I fixed my problem. Please take a look at this to make sure I picked up all your recent changes.

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 65 • 14 years ago

Posted wrong version of the patch. Here's the latest I have but there is an error in linking libxul:

   Creating library xul.lib and object xul.exp
nsStaticXULComponents.obj : error LNK2001: unresolved external symbol "struct mozilla::Module const
* const nsServicesCryptoModule_NSModule" (?nsServicesCryptoModule_NSModule@@3QBUModule@mozilla@@B)
xul.dll : fatal error LNK1120: 1 unresolved externals

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 66 • 14 years ago

v9 failed because it was missing changes to libxul-config.mk.

Now everything builds and unit tests pass (on Windows, at least).

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?) Assignee
	Comment 67 • 14 years ago

This is the same as v10 except the typo in the IDL was fixed ("first" -> "second").

(In reply to comment #47)
> >+// TODO: validate PQG
> 
> Is there a bug on this and other TODO comments?  If so, please note in the
> comment, if not, file and note in the comment.

If Sync interoperates with FF home then that is enough validation for me. Aldo, the OOM worries were handled by the change from nsStringAPI.h to nsString.h. I removed the comments.

> >+//NS_DEFINE_STATIC_IID_ACCESSOR(nsISyncJPAKE, NS_ISYNCJPAKE_IID)
> 
> if not needed, please remove

Gone.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 68 • 14 years ago

Comment on attachment 496598 [details] [diff] [review]
nsISyncJPake component (v11) with build system changes

Tests pass, try build on (rev 40de05bd875f) is very green. I'd say this is ready to land.

	Matt Brubeck (:mbrubeck)
	Comment 69 • 14 years ago

This patch makes the same build change for mobile-browser.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 70 • 14 years ago

(In reply to comment #59)
> As decided on IRC: we're going to make this particular binary component part of
> libxul for b8 (and perhaps for final): it should be part of tier_toolkit.

So, why are we doing this?  If I'm reading the patch correctly it is going to break Firefox (and Fennec) on Xulrunner. (Xulrunner is built without MOZ_SERVICES_SYNC, so the contents of services/crypto will be absent.)

If you really really want to split sync across the tiers (which I don't care for, but I'll defer to bsmedberg) you should put the JPAKE stuff into another directory (say, services/jpake) that is built unconditionally as part of tier_platform and then leave everything else alone.

	Mike Connor [:mconnor]
	Comment 71 • 14 years ago

Comment on attachment 496598 [details] [diff] [review]
nsISyncJPake component (v11) with build system changes

gah, thought I marked this earlier!

bombs away, as it were.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 72 • 14 years ago

Comment on attachment 496598 [details] [diff] [review]
nsISyncJPake component (v11) with build system changes

Please address comment 70 (even if "addressing" consists of filing followup bugs with a solid plan to fix Firefox on XR) before landing.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 73 • 14 years ago

(In reply to comment #70)
> If you really really want to split sync across the tiers (which I don't care
> for, but I'll defer to bsmedberg) you should put the JPAKE stuff into another
> directory (say, services/jpake) that is built unconditionally as part of
> tier_platform and then leave everything else alone.

Or we just build services/crypto unconditionally as part of tier_platform. I would say let's to do that in a follow-up bug. Mike already volunteered on IRC to look into it tomorrow. But I don't think it should block progress now.

	Philipp von Weitershausen [:philikon] Reporter
	Comment 74 • 14 years ago

(In reply to comment #72)
> Please address comment 70 (even if "addressing" consists of filing followup
> bugs with a solid plan to fix Firefox on XR) before landing.

Filed bug 618195 as a follow-up

	Philipp von Weitershausen [:philikon] Reporter
	Comment 75 • 14 years ago

https://hg.mozilla.org/mozilla-central/rev/7916ff577de1
https://hg.mozilla.org/mozilla-central/rev/006396b0b990

	Matt Brubeck (:mbrubeck)
	Comment 76 • 14 years ago

http://hg.mozilla.org/mobile-browser/rev/6761114011ed