AddressBook + HTTP Proxy: store only unique subscription addresses #835

coneiric · 2018-03-14T06:27:06Z

By submitting this pull-request, I confirm the following:

I have read and understood the contributor guide in kovri-docs.
I have checked that another pull-request for this purpose does not exist.
I have considered and confirmed that this submission will be valuable to others.
I accept that this submission may not be used and that this pull-request may be closed by the will of the maintainer.
I give this submission freely under the BSD 3-clause license.

Fixes #814

edit: HTTP Proxy is changed indirectly through the Address Book singleton the proxy uses to save jump service addresses.

New helper functions validate human readable hostnames, and ensure only unique entries are loaded from subscriptions. Refactor subscription validation + saving with new helpers. Refs monero-project#835

Refactor test cases and fixture for added utility/clarity. New test cases for validating only unique, well-formed entries are added to the address book. Refs monero-project#835

Save jump service addresses to in-memory address book, and remove singleton used for directly storing to disk. Add a URL parser, and refactor various components to use it. Refs monero-project#835

Ensure that saving jump service addresses are handled properly. Refs monero-project#835

Refs monero-project#835

New helper functions validate human readable hostnames, and ensure only unique entries are loaded from subscriptions. Refactor subscription validation + saving with new helpers. Refs monero-project#835

Refactor test cases and fixture for added utility/clarity. New test cases for validating only unique, well-formed entries are added to the address book. Refs monero-project#835

Save jump service addresses to in-memory address book, and remove singleton used for directly storing to disk. Add a URL parser, and refactor various components to use it. Refs monero-project#835

Refs monero-project#835

HTTP jump service requests save valid addresses to in-memory address book, separating storage from HTTP message processing. Refs monero-project#835

Refs monero-project#835

Update test fixture with a destination that has a valid length. Previous destination fails identity validation checks, and throws a length error when decoded from base64. Refs monero-project#835

Refactor fixture for added utility + clarity. New test cases for validating only unique, well-formed entries are added to the address book. Refs monero-project#835

Save jump service addresses to in-memory address book, and remove singleton used for directly storing to disk. Add a URL parser, and refactor various components to use it. Refs monero-project#835

Refs monero-project#835

HTTP jump service requests save valid addresses to in-memory address book, separating storage from HTTP message processing. Refs monero-project#835 + monero-project#838

Refs monero-project#835 + monero-project#838

Rewrite to fix bugs uncovered by unit-tests. Refs monero-project#835 + monero-project#838

Refs monero-project#835 + monero-project#838

Rewrite to fix bugs uncovered by unit-tests. Refs monero-project#835 + monero-project#838

anonimal

At 47 NACKs, I'll finish the review once the issues are resolved. Also look at your commit messages: they are inaccurate:

You didn't rewrite the unit-test: you did unnecessary refactoring and added test-cases.
You didn't rewrite address saving: you moved parsing from extraction and added a check.
"HTTPProxy: rewrite to fix uncovered bugs" doesn't describe the refactor, nor does it describe any of the bugs anywhere (message title or body).

anonimal · 2018-03-20T16:48:15Z

tests/unit_tests/client/address_book/impl.cc

@@ -165,7 +209,9 @@ BOOST_AUTO_TEST_CASE(PGPClearSign) {
    "=P8Ug"
    "-----END PGP SIGNATURE-----";
  lines.push_back(line);
-  BOOST_CHECK(Validate());
+  //TODO(oneiric): PGP-signed subscriptions should pass validation
+  //  mark true after implementing PGP signature verification


implementing PGP signature verification

There's no real reason to do that. You probably mean ignoring pgp signature.

Wasn't sure if PGP signatures are actually verified by AddressBook. Thought it might be a good idea for validating the shipped hosts.txt + other subscriptions.

If AddressBook ignores PGP signatures in subscriptions, is PGP verification a feature somewhere else?

If AddressBook ignores PGP signatures in subscriptions, is PGP verification a feature somewhere else?

Yes, the reason for this is because our monero publisher service pulls the default subscription directly from github (see the pkg directory). I PGP sign the subscription to prevent any tampering; either by github or the very rare potential "I broke your TLS" mitm.

Ok thank you, that answers my question completely.

anonimal · 2018-03-20T16:56:47Z

src/client/address_book/impl.cc

-        file << host << "=" << ident.ToBase64() << '\n';  // TODO(anonimal): this is not optimal, especially for large subscriptions
-        // Add to address book
-        m_Storage->AddAddress(ident);
-        m_Addresses[host] = ident.GetIdentHash();  // TODO(anonimal): setter?


// Add to address book
m_Storage->AddAddress(ident);
m_Addresses[host] = ident.GetIdentHash(); // TODO(anonimal): setter?

.....do I really have to say why removing this is a bad idea?...

No, you don't. This was left over from previous edits, and I re-added it to the try-catch block below.

Thanks for catching it.

anonimal · 2018-03-20T17:11:54Z

src/client/address_book/impl.cc

+              LOG(warning) << "AddressBook: malformed address, skipping";
+              continue;
+            }
+        }


No way. Not only does this vastly decrease efficiency (you're inserting for every line!), but insertion should stay decoupled from validation. There was no reason to refactor this function nor setup a vector nor use boost::split.

This was my mistake, I was trying to check if addresses were already loaded in memory during validation. This function is now unchanged.

anonimal · 2018-03-20T17:34:43Z

src/client/address_book/impl.cc

  std::string line;
-  // To ensure valid hostname
-  // Note: uncomment if this regexp fails on some locales (to not rely on [a-z])
-  //const std::string alpha = "abcdefghijklmnopqrstuvwxyz";


Don't remove this unless you've adequately tested across multiple locales.

I re-added the comment. Wasn't sure if this was leftover, and still don't really understand how the alpha string would help if the regex below fails.

anonimal · 2018-03-20T17:36:01Z

src/client/address_book/impl.cc

@@ -332,16 +338,8 @@ bool AddressBook::SaveSubscription(
 const std::map<std::string, kovri::core::IdentityEx>
 AddressBook::ValidateSubscription(std::istream& stream) {
  LOG(debug) << "AddressBook: validating subscription";
-  // Map host to address identity


There's no functional reason to remove this comment.

You're right, it got caught up when I was trying to separate out host validation from subscription validation.

anonimal · 2018-03-20T19:47:46Z

src/client/proxy/http.cc

+  if (helper_ident.size() != 2)
+    throw std::length_error("HTTPProxyHandler: invalid number of jump service parameters");
+  return helper_ident;
+}


anonimal · 2018-03-20T19:49:49Z

src/client/proxy/http.cc

-      m_Base64Destination = boost::network::uri::decoded(base64);
-      return true;
+      std::vector<std::string> const helper_ident = ParseJumpServiceQuery();
+      if (!helper_ident[1].empty())


Needing to access like this exposes bad design. Don't repeat guzzi's mess.

I'll try not to. I feel infected by it.

anonimal · 2018-03-20T19:49:58Z

src/client/proxy/http.cc

-        pos = pos2;
+      std::vector<std::string> const helper_ident = ParseJumpServiceQuery();
+      for (const auto& helper : m_JumpService)
+        if (helper_ident[0] == helper)


Needing to access like this exposes bad design. Don't repeat guzzi's mess.

I'll try not to. I feel infected by it.

anonimal · 2018-03-20T20:03:54Z

src/client/proxy/http.cc

+          return false;
+        }
+      if (m_Address.empty())
+        ParseURL();


By now, you shouldn't need to parse anything more than once.

100% agree, I went overkill on this. I'll clean up this area in the HTTP proxy rewrite.

anonimal · 2018-03-20T20:08:34Z

src/client/proxy/http.cc

+        ParseURL();
+      core::IdentityEx ident;
+      // Extra sanity check to ensure base64 destination is uri-decoded
+      ident.FromBase64(boost::network::uri::decoded(m_Base64Destination));


By now, you shouldn't need to parse anything more than once.

Right, I was considering not adding this extra URI-decoding step. Reverted, and will keep in mind for the HTTP proxy rewrite.

anonimal · 2018-03-20T21:00:31Z

Note: if your intent is only to AddressBook + HTTP Proxy: store only unique subscription addresses, then keep PRs atomic so unrelated work doesn't block flow.

New helper checks if host + address already loaded into memory. Refactor functions that store addresses to use the new helper. Refs monero-project#835

New helper to validate I2P hostnames, and refactor subscription validation to use it. Refs monero-project#835

New test case to ensure address book only inserts unique entries. Added struct + helper to test fixture for extracting an address book entry from a subscription line. Refs monero-project#835

Save jump service addresses to in-memory address book, and remove singleton used for directly storing to disk. Add a URL parser, and refactor various components to use it. Refs monero-project#835

HTTP jump service requests save valid addresses to in-memory address book, separating storage from HTTP message processing. Refs monero-project#835 + monero-project#838

Refs monero-project#835 + monero-project#838

Rewrite to fix bugs uncovered by unit-tests. Refs monero-project#835 + monero-project#838

Implement inserting an address to the in-memory address book. Will replace the singleton used by HTTP proxy, and is generic to be used by other member functions or external classes. Refs monero-project#835

New test case to ensure address book only inserts unique entries. Added struct + helper to test fixture for extracting an address book entry from a subscription line. Refs monero-project#835

Save jump service addresses to in-memory address book, and remove singleton used for directly storing to disk. Add a URL parser, and refactor various components to use it. Refs monero-project#835

HTTP jump service requests save valid addresses to in-memory address book, separating storage from HTTP message processing. Refs monero-project#835 + monero-project#838

Refs monero-project#835 + monero-project#838

New setter for in-memory address book to insert unique entries. Refactor functions that store addresses to use the new setter. Refs monero-project#835

New test case to ensure address book only inserts unique entries. Added struct + helper to test fixture for extracting an address book entry from a subscription line. Refs monero-project#835

coneiric · 2018-03-23T01:19:06Z

keep PRs atomic so unrelated work doesn't block flow

Painful lesson. I get over-eager to do refactoring as I see the problem. Hard habit to break.

I moved my HTTP Proxy refactor attempts (the few parts worth saving), and your recommendations to a separate branch dedicated to an HTTP message rewrite.

Again, thank you for your patience, and our conversation in IRC about future refactors.

Apologize for any time wasted, or blood vessels popped.

New test case to ensure address book only inserts unique entries. New test fixture struct to convert a subscription line into an address book entry. Refs monero-project#835

anonimal

Thanks for making all the necessary changes.

anonimal · 2018-03-29T22:26:26Z

src/client/address_book/impl.cc

+      {
+        const auto it = m_Addresses.find(host);
+        if (it != m_Addresses.end())
+          throw std::runtime_error("AddressBook: host already loaded");


const auto it = m_Addresses.find(host);
if (it != m_Addresses.end())
throw std::runtime_error("AddressBook: host already loaded");

So, not only is this incredibly expensive, but it's not necessary because of m_Addresses[host] = address

std::map::operator[] returns a reference to the value that is mapped to a key equivalent to key, performing an insertion if such key does not already exist.

A much better solution is to tackle file writing / address saving after the map is properly updated.

I didn't realize std::map worked that way, pretty cool feature. Removed the quoted block, but kept the check for loaded addresses. Eventually, likely after multiple subscriptions PR, there should be a reverse address => hostname lookup, making the current naive lookup unnecessary.

A much better solution is to tackle file writing / address saving after the map is properly updated.

I will keep this in mind while working on the multiple subscription PR. Do we want to follow Java I2P naming for filenames, i.e. userhosts.txt, privatehosts.txt?

Do we want to follow Java I2P naming for filenames, i.e. userhosts.txt, privatehosts.txt?

There's no need, and userhosts.txt is also confusing.

anonimal · 2018-03-29T22:30:51Z

tests/unit_tests/client/address_book/impl.cc

+  struct AddressBookEntry
+  {
+   public:
+    AddressBookEntry() : m_Host(""), m_Address() {}


Initializing a member string like this is unnecessary. Initializing m_Address like this is unnecessary because we do that during Tag construction.

Wasn't sure here about initialization, so I added it just in case. Removed now.

anonimal · 2018-03-29T22:31:03Z

tests/unit_tests/client/address_book/impl.cc

+    AddressBookEntry() : m_Host(""), m_Address() {}
+
+    explicit AddressBookEntry(const std::string& subscription_line)
+        : m_Host(""), m_Address()


Initializing a member string like this is unnecessary. Initializing m_Address like this is unnecessary because we do that during Tag construction.

Wasn't sure here about initialization, so I added it just in case. Removed now.

anonimal · 2018-03-29T22:35:51Z

tests/unit_tests/client/address_book/impl.cc

+        }
+    }
+
+    inline const std::string& host() const { return m_Host; }


inline

Not necessary as this is implicitly inlined. inlines are also suggestions which are not always observed by the compiler and may go ignored. The compiler may also inline when not explicitly stated.

I removed the inline suggestion, obvious now it doesn't help. Will look more into inline for future use.

anonimal · 2018-03-29T22:35:55Z

tests/unit_tests/client/address_book/impl.cc

+
+    inline const std::string& host() const { return m_Host; }
+
+    inline const kovri::core::IdentHash& address() const { return m_Address; }


inline

Not necessary as this is implicitly inlined. inlines are also suggestions which are not always observed by the compiler and may go ignored. The compiler may also inline when not explicitly stated.

I removed the inline suggestion, obvious now it doesn't help. Will look more into inline for future use.

anonimal · 2018-03-29T22:36:33Z

tests/unit_tests/client/address_book/impl.cc

+    inline const kovri::core::IdentHash& address() const { return m_Address; }
+
+   private:
+    /// @var m_Host


/// @var m_Host

Not necessary. In fact, with members, you can do std::string m_Host; ///< Human-readable I2P hostname.

So much cleaner! Updated now.

anonimal · 2018-03-29T22:37:19Z

tests/unit_tests/client/address_book/impl.cc

+    /// @var m_Host
+    /// @brief Human-readable I2P hostname
+    std::string m_Host;
+    /// @var m_Address


/// @var m_Address

Not necessary. In fact, with members, you can do kovri::core::IdentHash m_Address; ///< I2P address hash.

So much cleaner! Updated now.

anonimal · 2018-03-29T22:38:09Z

tests/unit_tests/client/address_book/impl.cc

+BOOST_AUTO_TEST_CASE(RejectDuplicateEntry)
+{
+  // Ensure valid subscription line creates an entry
+  BOOST_CHECK_NO_THROW(AddressBookEntry const entry(subscription.front()));


AddressBookEntry const

const not necessary here.

Right, guess it is a bit too pedantic given the context.

anonimal · 2018-03-29T22:53:43Z

tests/unit_tests/client/address_book/impl.cc

+  // Ensure valid subscription line creates an entry
+  BOOST_CHECK_NO_THROW(AddressBookEntry const entry(subscription.front()));
+
+  AddressBookEntry const entry(subscription.front());


AddressBookEntry const

const not necessary here.

Right, guess it is a bit too pedantic given the context.

New setter for in-memory address book to insert unique entries. Refactor functions that store addresses to use the new setter. Refs monero-project#835

New test case to ensure address book only inserts unique entries. New test fixture struct to convert a subscription line into an address book entry. Refs monero-project#835

coneiric · 2018-03-30T17:50:58Z

Thanks for making all the necessary changes.

You're welcome, thanks for the thorough review.

New test case to ensure address book only inserts unique entries. New test fixture struct to convert a subscription line into an address book entry. Refs monero-project#835

anonimal

Optimizations (design, not compiler) will eventually be needed because you want to iterate through the entire map for every single address in a subscription.

Works well enough for now.

a673466 Tests: Address Book unit-test checks unique entry (oneiric) 05ee550 AddressBook: store only unique addresses (oneiric)

coneiric force-pushed the 814/unique-addresses branch 3 times, most recently from 4db8e67 to 37a7b30 Compare March 17, 2018 06:14

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 17, 2018

HTTPProxy Tests: test new address saving logic

a787501

Ensure that saving jump service addresses are handled properly. Refs monero-project#835

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 17, 2018

AddressBook Style: remove dead code

37a7b30

Refs monero-project#835

coneiric force-pushed the 814/unique-addresses branch from 37a7b30 to eec1345 Compare March 18, 2018 02:32

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 18, 2018

AddressBook Style: remove dead code

ca0291d

Refs monero-project#835

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 18, 2018

Tests: remove HTTPProxy unit-test patch from monero-project#838

f51e6b1

Refs monero-project#835

coneiric force-pushed the 814/unique-addresses branch from eec1345 to 22f9202 Compare March 19, 2018 01:18

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 19, 2018

Style: remove dead code from Address Book

928960c

Refs monero-project#835

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 19, 2018

Tests: resolve HTTP Proxy unit-test TODOs

19f755f

Refs monero-project#835 + monero-project#838

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 19, 2018

HTTPProxy: rewrite to fix uncovered bugs

22f9202

Rewrite to fix bugs uncovered by unit-tests. Refs monero-project#835 + monero-project#838

coneiric force-pushed the 814/unique-addresses branch from 22f9202 to 5944bdd Compare March 20, 2018 05:38

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 20, 2018

Tests: resolve HTTP Proxy unit-test TODOs

cbe059d

Refs monero-project#835 + monero-project#838

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 20, 2018

HTTPProxy: rewrite to fix uncovered bugs

5944bdd

Rewrite to fix bugs uncovered by unit-tests. Refs monero-project#835 + monero-project#838

anonimal suggested changes Mar 20, 2018

View reviewed changes

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 22, 2018

AddressBook: store only unique addresses

f87ebc4

New helper checks if host + address already loaded into memory. Refactor functions that store addresses to use the new helper. Refs monero-project#835

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 22, 2018

AddressBook: validate I2P hostname

18afce2

New helper to validate I2P hostnames, and refactor subscription validation to use it. Refs monero-project#835

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 22, 2018

Tests: resolve HTTP Proxy unit-test TODOs

423a001

Refs monero-project#835 + monero-project#838

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 22, 2018

HTTPProxy: rewrite to fix uncovered bugs

4bcf948

Rewrite to fix bugs uncovered by unit-tests. Refs monero-project#835 + monero-project#838

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 22, 2018

Tests: resolve HTTP Proxy unit-test TODOs

9b6a371

Refs monero-project#835 + monero-project#838

coneiric added a commit to coneiric/kovri that referenced this pull request Mar 22, 2018

AddressBook: store only unique addresses

9275963

New setter for in-memory address book to insert unique entries. Refactor functions that store addresses to use the new setter. Refs monero-project#835

coneiric force-pushed the 814/unique-addresses branch from 5944bdd to 0bbd406 Compare March 23, 2018 01:10

coneiric force-pushed the 814/unique-addresses branch from 0bbd406 to 75b5712 Compare March 23, 2018 03:52

coneiric force-pushed the 814/unique-addresses branch from 75b5712 to 40fd3da Compare March 29, 2018 17:17

anonimal suggested changes Mar 29, 2018

View reviewed changes

AddressBook: store only unique addresses

05ee550

New setter for in-memory address book to insert unique entries. Refactor functions that store addresses to use the new setter. Refs monero-project#835

coneiric force-pushed the 814/unique-addresses branch from 40fd3da to c691441 Compare March 30, 2018 17:49

Tests: Address Book unit-test checks unique entry

a673466

New test case to ensure address book only inserts unique entries. New test fixture struct to convert a subscription line into an address book entry. Refs monero-project#835

anonimal approved these changes Apr 3, 2018

View reviewed changes

coneiric force-pushed the 814/unique-addresses branch from c691441 to a673466 Compare April 3, 2018 02:02

anonimal approved these changes Apr 3, 2018

View reviewed changes

anonimal merged commit a673466 into monero-project:master Apr 3, 2018

anonimal added a commit that referenced this pull request Apr 3, 2018

Merge pull request #835

5ba66f6

a673466 Tests: Address Book unit-test checks unique entry (oneiric) 05ee550 AddressBook: store only unique addresses (oneiric)

coneiric deleted the 814/unique-addresses branch May 30, 2018 00:06


		inline const std::string& host() const { return m_Host; }

		inline const kovri::core::IdentHash& address() const { return m_Address; }

AddressBook + HTTP Proxy: store only unique subscription addresses #835

AddressBook + HTTP Proxy: store only unique subscription addresses #835

Conversation

coneiric commented Mar 14, 2018 • edited Loading

anonimal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonimal commented Mar 20, 2018

coneiric commented Mar 23, 2018 • edited Loading

anonimal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coneiric commented Mar 30, 2018

anonimal left a comment

Choose a reason for hiding this comment

coneiric commented Mar 14, 2018 •

edited

Loading

coneiric commented Mar 23, 2018 •

edited

Loading