Derived key fixes

The reason things weren't working here is because libsodium does
something completely unintuitive and called the seed the "secret key"
when it isn't, it's the seed.

This adds a new PrivateKey class (alongside the existing SecretKey and
PubKey) that holds just a private key value but no seed -- which we need
to do because there is no way we can get a seed after calculating a
derived keypair.

With these changes, we now generate exactly the same keys and subkeys as
Tor (and a new test case uses values generated in Tor to verify this).

This is incomplete -- the subkey signing code is still not implemented;
it has to be adapted to create a signature from a PrivateKey rather than
a SecretKey which will probably requiring working around/reimplementing
some of what libsodium does for creating a signature since it expects
"secret keys" i.e. the seed.

llarp/crypto/crypto.hpp

@@ -66,11 +66,11 @@ namespace llarp
 
     /// derive sub keys for public keys
     virtual bool
-    derive_subkey(PubKey &, const PubKey &, uint64_t) = 0;
+    derive_subkey(PubKey &, const PubKey &, uint64_t, const AlignedBuffer<32> * = nullptr) = 0;
 
-    /// derive sub keys for secret keys
+    /// derive sub keys for private keys
     virtual bool
-    derive_subkey_secret(SecretKey &, const SecretKey &, uint64_t) = 0;
+    derive_subkey_private(PrivateKey &, const SecretKey &, uint64_t, const AlignedBuffer<32> * = nullptr) = 0;
 
     /// seed to secretkey
     virtual bool

llarp/crypto/crypto_libsodium.cpp

@@ -230,41 +230,71 @@ namespace llarp
     static AlignedBuffer< 32 > zero;
 
     bool
-    CryptoLibSodium::derive_subkey(PubKey &out_key, const PubKey &root_key,
-                                   uint64_t key_n)
+    CryptoLibSodium::derive_subkey(PubKey &out_pubkey, const PubKey &root_pubkey,
+                                   uint64_t key_n, const AlignedBuffer<32>* hash)
     {
-      // scalar p
-      AlignedBuffer< 32 > p;
-      // p = H( i || in_k )
-      if(not make_scalar(p.data(), root_key, key_n))
-        return false;
-      crypto_core_ed25519_scalar_mul(out_key.data(), root_key.data(), p.data());
-      LogInfo("derive_subkey() scalar = ", p, " root_key = ", root_key,
-              " derived_key = ", out_key);
-      return true;
-    }
-
-    bool
-    CryptoLibSodium::derive_subkey_secret(SecretKey &out_key,
-                                          const SecretKey &in_key,
-                                          uint64_t key_n)
-    {
-      const PubKey root_key = in_key.toPublic();
-      // scalar p
-      AlignedBuffer< 32 > p;
-      // p = H( i || in_key.pub)
-      if(not make_scalar(p.data(), root_key, key_n))
+      // scalar h = H( in_k || root_pubkey )
+      AlignedBuffer< 32 > h;
+      if (hash)
+        h = *hash;
+      else if(not make_scalar(h.data(), root_pubkey, key_n))
       {
         LogError("cannot make scalar");
         return false;
       }
-      // a * p * basepoint
-      crypto_core_ed25519_scalar_mul(out_key.data(), in_key.data(), p.data());
-      if(not out_key.Recalculate())
+
+      return 0 == crypto_scalarmult_ed25519(out_pubkey.data(), h.data(), root_pubkey.data());
+    }
+
+    bool
+    CryptoLibSodium::derive_subkey_private(PrivateKey &out_key,
+                                          const SecretKey &root_key,
+                                          uint64_t key_n,
+                                          const AlignedBuffer<32>* hash
+                                          )
+    {
+
+      // Derives a private subkey from a root key.
+      //
+      // The basic idea is:
+      //
+      // h - hash dependent on the `key_n` value.
+      // a - private key
+      // A = aB - public key
+      // a' = ah - derived private key
+      // A' = a'B = (ah)B - derived public key
+      //
+      // libsodium throws some wrenches in the mechanics which are a nuisance, the biggest of which
+      // is that sodium's secret key is *not* `a`; rather it is the seed.  If you want to get the
+      // private key (i.e. "a"), you need to SHA-512 hash it and then clamp that.
+      //
+      // This also makes signature verification harder: we can't just use sodium's verify function
+      // because it wants to be given the seed rather than the private key, and moreover we can't
+      // actually *get* the seed to make libsodium happy because we only have `ah` above.
+      //
+      const auto root_pubkey = root_key.toPublic();
+
+      // scalar h = H( in_k || root_pubkey )
+      AlignedBuffer< 32 > h;
+      if (hash)
+        h = *hash;
+      else if(not make_scalar(h.data(), root_pubkey, key_n))
+      {
+        LogError("cannot make scalar");
         return false;
-      LogInfo("derive_subkey_secret() scalar = ", p, " root_key = ", root_key,
-              " derived_key = ", out_key.toPublic(),
-              " full_derived_key = ", out_key.ToHex());
+      }
+
+      h[0] &= 248;
+      h[31] &= 63;
+      h[31] |= 64;
+
+      PrivateKey a;
+      if (!root_key.toPrivate(a))
+        return false;
+
+      // a' = ha
+      crypto_core_ed25519_scalar_mul(out_key.data(), h.data(), a.data());
+
       return true;
     }
 

llarp/crypto/crypto_libsodium.hpp

@@ -53,13 +53,15 @@ namespace llarp
       verify(const PubKey &, const llarp_buffer_t &,
              const Signature &) override;
 
-      /// derive sub keys for public keys
+      /// derive sub keys for public keys.  hash is really only intended for testing and overrides
+      /// key_n if given.
       bool
-      derive_subkey(PubKey &, const PubKey &, uint64_t) override;
+      derive_subkey(PubKey& derived, const PubKey& root, uint64_t key_n, const AlignedBuffer<32>* hash = nullptr) override;
 
-      /// derive sub keys for secret keys
+      /// derive sub keys for private keys.  hash is really only intended for testing and overrides
+      /// key_n if given.
       bool
-      derive_subkey_secret(SecretKey &, const SecretKey &, uint64_t) override;
+      derive_subkey_private(PrivateKey& derived, const SecretKey &root, uint64_t key_n, const AlignedBuffer<32>* hash = nullptr) override;
 
       /// seed to secretkey
       bool

llarp/crypto/types.cpp

@@ -61,6 +61,27 @@ namespace llarp
     return crypto_scalarmult_ed25519_base(data() + 32, data()) != -1;
   }
 
+  bool
+  SecretKey::toPrivate(PrivateKey& key) const
+  {
+    // libsodium and ref10 calculate a 512-bit hash, but then only use 256 bits (32 bytes) of it for
+    // the private key.  Because yeah.
+    unsigned char h[crypto_hash_sha512_BYTES];
+    if (crypto_hash_sha512(h, data(), 32) < 0)
+      return false;
+    h[0] &= 248;
+    h[31] &= 63;
+    h[31] |= 64;
+    std::memcpy(key.data(), h, 32);
+    return true;
+  }
+
+  bool
+  PrivateKey::toPublic(PubKey& pubkey) const
+  {
+    return crypto_scalarmult_ed25519_base_noclamp(pubkey.data(), data()) != -1;
+  }
+
   bool
   SecretKey::SaveToFile(const char* fname) const
   {

llarp/crypto/types.hpp

@@ -16,9 +16,7 @@ namespace llarp
 
   struct PubKey final : public AlignedBuffer< PUBKEYSIZE >
   {
-    PubKey() : AlignedBuffer< SIZE >()
-    {
-    }
+    PubKey() = default;
 
     explicit PubKey(const byte_t *ptr) : AlignedBuffer< SIZE >(ptr)
     {
@@ -76,16 +74,31 @@ namespace llarp
     return lhs.as_array() == rhs.as_array();
   }
 
+  struct PrivateKey;
+
+  /// Stores a sodium "secret key" value, which is actually the seed concatenated with the public
+  /// key.  Note that the seed is *not* the private key value itself, but rather the seed from which
+  /// it can be calculated.
   struct SecretKey final : public AlignedBuffer< SECKEYSIZE >
   {
-    SecretKey() : AlignedBuffer< SECKEYSIZE >()
-    {
-    }
+    SecretKey() = default;
 
     explicit SecretKey(const byte_t *ptr) : AlignedBuffer< SECKEYSIZE >(ptr)
     {
     }
 
+    // The full data
+    explicit SecretKey(const AlignedBuffer< SECKEYSIZE > &seed)
+      : AlignedBuffer< SECKEYSIZE >(seed)
+    {}
+
+    // Just the seed, we recalculate the pubkey
+    explicit SecretKey(const AlignedBuffer< 32 > &seed)
+    {
+      std::copy(seed.begin(), seed.end(), begin());
+      Recalculate();
+    }
+
     /// recalculate public component
     bool
     Recalculate();
@@ -104,6 +117,10 @@ namespace llarp
       return PubKey(data() + 32);
     }
 
+    /// Computes the private key from the secret key (which is actually the seed)
+    bool
+    toPrivate(PrivateKey& key) const;
+
     bool
     LoadFromFile(const char *fname);
 
@@ -119,6 +136,42 @@ namespace llarp
     return out << "[secretkey]";
   }
 
+  /// PrivateKey is similar to SecretKey except that it only stores the private key value
+  /// itself unlike SecretKey which stores the seed from which the private key value is generated.
+  /// This is intended for use with derived keys, where we can derive the private key but not the
+  /// seed.
+  struct PrivateKey final : public AlignedBuffer< 32 >
+  {
+    PrivateKey() = default;
+
+    explicit PrivateKey(const byte_t *ptr) : AlignedBuffer< 32 >(ptr)
+    {
+    }
+
+    explicit PrivateKey(const AlignedBuffer< 32 > &seed) : AlignedBuffer< 32>(seed) {}
+
+    std::ostream &
+    print(std::ostream &stream, int level, int spaces) const
+    {
+      Printer printer(stream, level, spaces);
+      printer.printValue("privatekey");
+      return stream;
+    }
+
+    /// Computes the public key
+    bool
+    toPublic(PubKey& pubkey) const;
+  };
+
+  inline std::ostream &
+  operator<<(std::ostream &out, const PrivateKey &)
+  {
+    // return out << k.ToHex();
+    // make sure we never print out private keys
+    return out << "[privatekey]";
+  }
+
+
   /// IdentitySecret is a secret key from a service node secret seed
   struct IdentitySecret final : public AlignedBuffer< 32 >
   {

llarp/service/identity.cpp

@@ -62,7 +62,7 @@ namespace llarp
       crypto_sign_ed25519_sk_to_curve25519(enckey.data(), signkey.data());
       pub.Update(seckey_topublic(signkey));
       crypto->pqe_keygen(pq);
-      if(not crypto->derive_subkey_secret(derivedSignKey, signkey, 1))
+      if(not crypto->derive_subkey_private(derivedSignKey, signkey, 1))
       {
         LogError("failed to generate derived key");
       }
@@ -149,7 +149,7 @@ namespace llarp
       pub.Update(seckey_topublic(signkey), van);
       crypto_sign_ed25519_sk_to_curve25519(enckey.data(), signkey.data());
       auto crypto = CryptoManager::instance();
-      return crypto->derive_subkey_secret(derivedSignKey, signkey, 1);
+      return crypto->derive_subkey_private(derivedSignKey, signkey, 1);
     }
 
     absl::optional< EncryptedIntroSet >
@@ -181,8 +181,10 @@ namespace llarp
       CryptoManager::instance()->xchacha20(buf, k, encrypted.nounce);
       encrypted.introsetPayload.resize(buf.sz);
       std::copy_n(buf.base, buf.sz, encrypted.introsetPayload.data());
+      /* FIXME
       if(not encrypted.Sign(derivedSignKey))
         return {};
+      */
       return encrypted;
     }
   }  // namespace service

llarp/service/identity.hpp

@@ -21,7 +21,7 @@ namespace llarp
     {
       SecretKey enckey;
       SecretKey signkey;
-      SecretKey derivedSignKey;
+      PrivateKey derivedSignKey;
       PQKeyPair pq;
       uint64_t version = LLARP_PROTO_VERSION;
       VanityNonce vanity;

test/crypto/mock_crypto.hpp

@@ -43,10 +43,10 @@ namespace llarp
                    bool(byte_t *, const llarp_buffer_t &,
                         const SharedSecret &));
 
-      MOCK_METHOD3(derive_subkey, bool(PubKey &, const PubKey &, uint64_t));
+      MOCK_METHOD4(derive_subkey, bool(PubKey &, const PubKey &, uint64_t, const AlignedBuffer<32> *));
 
-      MOCK_METHOD3(derive_subkey_secret,
-                   bool(SecretKey &, const SecretKey &, uint64_t));
+      MOCK_METHOD4(derive_subkey_private,
+                   bool(PrivateKey &, const SecretKey &, uint64_t, const AlignedBuffer<32> *));
 
       MOCK_METHOD3(sign,
                    bool(Signature &, const SecretKey &,

test/service/test_llarp_service_identity.cpp

@@ -1,5 +1,6 @@
 #include <crypto/crypto.hpp>
 #include <crypto/crypto_libsodium.hpp>
+#include <sodium/crypto_scalarmult_ed25519.h>
 #include <llarp_test.hpp>
 #include <path/path.hpp>
 #include <service/address.hpp>
@@ -76,7 +77,7 @@ TEST_F(ServiceIdentityTest, EnsureKeys)
 
   const SecretKey k;
 
-  EXPECT_CALL(m_crypto, derive_subkey_secret(_, _, _))
+  EXPECT_CALL(m_crypto, derive_subkey_private(_, _, _, _))
       .WillRepeatedly(Return(true));
 
   EXPECT_CALL(m_crypto, identity_keygen(_))
@@ -143,16 +144,82 @@ struct RealCryptographyTest : public ::testing::Test
   }
 };
 
+TEST_F(RealCryptographyTest, TestKnownDerivation)
+{
+  // These values came out of Tor's test code, so that we can confirm we are doing the same blinding
+  // subkey crypto math as Tor.  Our hash value is generated differently so we use the hash from a
+  // Tor random test suite run.
+
+  AlignedBuffer<32> seed{{
+    0x11, 0x68, 0xae, 0xa6, 0x62, 0x26, 0x6c, 0x53, 0x69, 0x9f, 0xe7, 0xd9, 0xbb, 0xff, 0xf6, 0x8e,
+    0x58, 0x22, 0xde, 0x90, 0x4b, 0x91, 0x28, 0x5a, 0x7c, 0x41, 0xcc, 0x7c, 0x36, 0xb4, 0xf5, 0xa0 }};
+  AlignedBuffer<32> root_key_data{{
+    0x40, 0x64, 0x32, 0x11, 0x19, 0xfc, 0xe8, 0x27, 0x9d, 0x3f, 0xd6, 0xe9, 0xc8, 0x4c, 0x5a, 0xea,
+    0x32, 0xd4, 0xe3, 0x97, 0x4a, 0xe4, 0x00, 0xd0, 0xd8, 0x36, 0xc2, 0x0e, 0xe4, 0xa2, 0x7c, 0x6c }};
+  AlignedBuffer<32> root_pub_data{{
+    0x69, 0x8b, 0x43, 0xbb, 0x54, 0xeb, 0x31, 0x2e, 0x5a, 0x07, 0x3f, 0x59, 0x5f, 0x1a, 0xbf, 0xe3,
+    0x95, 0xf2, 0x7a, 0x6d, 0x1d, 0x64, 0x5c, 0x4b, 0x10, 0x3f, 0xa2, 0xf5, 0xe6, 0x97, 0x5c, 0x70 }};
+  AlignedBuffer<32> hash{{
+    0x22, 0x41, 0xca, 0x66, 0x21, 0x4c, 0x75, 0x40, 0x65, 0x57, 0x9e, 0x81, 0x8c, 0x70, 0x15, 0x2a,
+    0x71, 0xb6, 0xc1, 0x67, 0x3f, 0x3b, 0x4b, 0x22, 0x31, 0xed, 0x22, 0x30, 0x2e, 0x2a, 0x23, 0x8e }};
+  AlignedBuffer<32> derived_key_data{{
+    0xbd, 0x0c, 0x55, 0x32, 0x62, 0x89, 0x61, 0xea, 0x86, 0x10, 0xd2, 0x27, 0x18, 0x51, 0xc0, 0x5e,
+    0x0e, 0xb1, 0x5a, 0x45, 0xb7, 0xb6, 0x16, 0xbe, 0x37, 0xba, 0x9a, 0x34, 0x39, 0xc4, 0xd0, 0x07 }};
+  AlignedBuffer<32> derived_pub_data{{
+    0xa0, 0x72, 0x62, 0x22, 0xd7, 0xc0, 0x91, 0x49, 0xe5, 0xe7, 0x86, 0x0d, 0xc1, 0x53, 0x14, 0x02,
+    0xe9, 0x96, 0xb8, 0xd8, 0x93, 0xb9, 0x2f, 0xe9, 0xc8, 0xf6, 0xf0, 0x5d, 0xe2, 0x30, 0x06, 0x48 }};
+
+  SecretKey root{seed};
+  ASSERT_EQ(root.toPublic(), PubKey{root_pub_data});
+
+  PrivateKey root_key;
+  ASSERT_TRUE(root.toPrivate(root_key));
+  ASSERT_EQ(root_key, PrivateKey{root_key_data});
+
+  auto crypto = CryptoManager::instance();
+
+  PrivateKey aprime; // a'
+  ASSERT_TRUE(crypto->derive_subkey_private(aprime, root, 0, &hash));
+  ASSERT_EQ(aprime, PrivateKey{derived_key_data});
+
+  PubKey Aprime; // A'
+  ASSERT_TRUE(crypto->derive_subkey(Aprime, root.toPublic(), 0, &hash));
+  ASSERT_EQ(Aprime, PubKey{derived_pub_data});
+}
+
 TEST_F(RealCryptographyTest, TestGenerateDeriveKey)
 {
-  SecretKey root_key;
-  SecretKey sub_key;
-  PubKey sub_pubkey;
   auto crypto = CryptoManager::instance();
+  SecretKey root_key;
   crypto->identity_keygen(root_key);
-  ASSERT_TRUE(crypto->derive_subkey_secret(sub_key, root_key, 1));
-  ASSERT_TRUE(crypto->derive_subkey(sub_pubkey, root_key.toPublic(), 1));
-  ASSERT_EQ(sub_key.toPublic(), sub_pubkey);
+
+  PrivateKey root_privkey;
+  ASSERT_TRUE(root_key.toPrivate(root_privkey));
+
+  PrivateKey a;
+  PubKey A;
+  ASSERT_TRUE(root_key.toPrivate(a));
+  ASSERT_TRUE(a.toPublic(A));
+  ASSERT_EQ(A, root_key.toPublic());
+
+  {
+    // paranoid check to ensure this works as expected
+    PubKey aB;
+    crypto_scalarmult_ed25519_base(aB.data(), a.data());
+    ASSERT_EQ(A, aB);
+  }
+
+  PrivateKey aprime; // a'
+  ASSERT_TRUE(crypto->derive_subkey_private(aprime, root_key, 1));
+
+  PubKey Aprime; // A'
+  ASSERT_TRUE(crypto->derive_subkey(Aprime, A, 1));
+
+  // We should also be able to derive A' via a':
+  PubKey Aprime_alt;
+  ASSERT_TRUE(aprime.toPublic(Aprime_alt));
+
+  ASSERT_EQ(Aprime, Aprime_alt);
 }
 
 TEST_F(RealCryptographyTest, TestEncryptAndSignIntroSet)