RouterInfo: refactor signature creation and verification #917

coneiric · 2018-06-24T01:04:50Z

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

I have read and understood the developer 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.

Moves RouterInfo signing into creation, before converting to a buffer. Adds signature verification and unit-test.

anonimal · 2018-06-24T01:58:04Z

src/core/router/info.cc

-          m_BufferLen,
-          reinterpret_cast<std::uint8_t*>(m_Buffer.get()) + m_BufferLen);
-
-      m_BufferLen += private_keys.GetPublic().GetSignatureLen();


But now you haven't updated m_BufferLen, assuming we'll continue to use this member. (I'd love to have time to resolve the buffer TODO).

`m_BufferLen` doesn't need to be updated here anymore, because the `router_info` stream includes the signature after `CreateRouterInfo` returns. So, when it's set on [line 722](https://github.com/monero-project/kovri/pull/917/files/27f03bbdfd4b48e7714abbef348c952102fd646d#diff-b93e1f4bbdfedbfa93b91f9693a4f1ecR722), it's already the correct length. I'll spend time working on the buffer TODO. Hopefully, I'll have something ready by the end of this week.

anonimal · 2018-06-24T01:58:31Z

src/core/router/info.cc

+  bool success = false;
+  try
+    {
+      std::size_t len = m_BufferLen - m_RouterIdentity.GetSignatureLen();


Thanks, will do.

anonimal · 2018-06-24T02:00:06Z

src/core/router/info.cc

+      std::size_t len = m_BufferLen - m_RouterIdentity.GetSignatureLen();
+      if (len < Size::MinUnsignedBuffer)
+        throw std::length_error("RouterInfo: invalid RouterInfo size");
+      success =


The bool'ing isn't necessary, the function should return void. We'll either throw or not depending on outcome of verifier's bool.

Ok, wasn't sure if I should carry on the pattern. Will change with your recommendation.

anonimal · 2018-06-24T02:05:25Z

src/core/router/info.h

+  /// @brief Verify RI signature
+  /// @return Success of signature verification
+  /// @throws std::length_error if unsigned buffer length is below minimum
+  bool Verify();


This should also be implemented outside of the test.

Are you referring to throwing the length exception? If so, I can add that into the buffer refactor. If not, what do you mean?

Actually use this function Verify() outside of the test.

Oh, understood. Maybe at the end of construction (after buffer refactor)? Will look for other places that make sense too.

anonimal · 2018-06-24T02:14:40Z

Referencing #627.

Sign, and append the signature, during RouterInfo creation. Referencing monero-project#627 + monero-project#917

Verify that a router has a valid signature. Referencing monero-project#627 + monero-project#917

Referencing monero-project#627 + monero-project#917

coneiric · 2018-06-27T19:18:00Z

Added your recommended changes, and some tests for invalid signatures.

Was unsure about adding Verify to ReadFromFile. The one place it's called, the path ctor, also calls ReadFromBuffer with signature verification disabled.

Please let me know if you would like me to make further changes.

coneiric · 2018-06-28T18:51:13Z

I added the WIP tag, and would like to rebase this on top of #926. The buffer impl there is much cleaner, and rebasing this PR is simpler than the reverse.

Sign, and append the signature, during RouterInfo creation. Referencing monero-project#627 + monero-project#917

Verify that a router has a valid signature. Referencing monero-project#627 + monero-project#917

Referencing monero-project#627 + monero-project#917

coneiric · 2018-07-02T07:00:39Z

This PR is now rebased on the buffer utility changes, and will pass build checks after rebasing on a merged #935.

anonimal

Please rebase against master and force push so I can base against this branch to apply/build the proper changes.

Sign, and append the signature, during RouterInfo creation. Referencing monero-project#627 + monero-project#917

Verify that a router has a valid signature. Referencing monero-project#627 + monero-project#917

Referencing monero-project#627 + monero-project#917

coneiric · 2018-07-02T20:48:18Z

Please rebase against master and force push so I can base against this branch to apply/build the proper changes.

Changes rebased and pushed. Hopefully, there aren't too many corrections you need to make? Either way, thanks for your help.

Referencing monero-project#917

Unclear contract (and auto* may as well be uint8_t*). Referencing monero-project#917

Buffer is always at least zero initialized. Referencing monero-project#917

Sanely, we would want to throw here but doing so will stop the router instead of allowing the RI to become updated. Referencing monero-project#917

anonimal · 2018-07-02T23:46:38Z

Hopefully, there aren't too many corrections you need to make?

Nope. It's something I should've done in the first place many PRs ago (PR'ing to your fork). coneiric#2 awaits your merge.

Ri sig monero-project#917

Sanely, we would want to throw here but doing so will stop the router instead of allowing the RI to become updated. Referencing monero-project#917

anonimal

coneiric#4

RouterInfo: don't throw on invalid RI size

237e1f5 RouterInfo: don't throw on invalid RI size (anonimal) 18b3526 RouterInfo: don't throw when RI fails sig verify (anonimal) ed041ea RouterInfo: remove unnecessary null buffer check (anonimal) e89e992 RouterInfo: Verify: remove auto for data pointer (anonimal) 580a5b8 Tests: cleanup RI test comments (anonimal) e850f57 Tests: RouterInfo signature unit-tests (oneiric) fcee141 RouterInfo: verify signed router (oneiric) 0739336 RouterInfo: sign during router creation (oneiric)

anonimal suggested changes Jun 24, 2018

View reviewed changes

coneiric force-pushed the ri-sig branch from 27f03bb to fa22b06 Compare June 27, 2018 18:54

coneiric added a commit to coneiric/kovri that referenced this pull request Jun 27, 2018

RouterInfo: sign during router creation

cf42afe

Sign, and append the signature, during RouterInfo creation. Referencing monero-project#627 + monero-project#917

coneiric added a commit to coneiric/kovri that referenced this pull request Jun 27, 2018

RouterInfo: verify signed router

42b7c79

Verify that a router has a valid signature. Referencing monero-project#627 + monero-project#917

coneiric added a commit to coneiric/kovri that referenced this pull request Jun 27, 2018

Tests: RouterInfo signature unit-tests

fa22b06

Referencing monero-project#627 + monero-project#917

coneiric mentioned this pull request Jun 28, 2018

Core: create buffer utility class + buffer unit-test + RI buffer refactor #926

Merged

coneiric changed the title ~~RouterInfo: refactor signature creation and verification~~ [WIP] RouterInfo: refactor signature creation and verification Jun 28, 2018

coneiric force-pushed the ri-sig branch from fa22b06 to 512317f Compare July 2, 2018 06:44

coneiric added a commit to coneiric/kovri that referenced this pull request Jul 2, 2018

RouterInfo: sign during router creation

012017b

Sign, and append the signature, during RouterInfo creation. Referencing monero-project#627 + monero-project#917

coneiric added a commit to coneiric/kovri that referenced this pull request Jul 2, 2018

RouterInfo: verify signed router

ed3efbb

Verify that a router has a valid signature. Referencing monero-project#627 + monero-project#917

coneiric added a commit to coneiric/kovri that referenced this pull request Jul 2, 2018

Tests: RouterInfo signature unit-tests

512317f

Referencing monero-project#627 + monero-project#917

coneiric changed the title ~~[WIP] RouterInfo: refactor signature creation and verification~~ RouterInfo: refactor signature creation and verification Jul 2, 2018

anonimal suggested changes Jul 2, 2018

View reviewed changes

coneiric added 3 commits July 2, 2018 20:30

RouterInfo: sign during router creation

0739336

Sign, and append the signature, during RouterInfo creation. Referencing monero-project#627 + monero-project#917

RouterInfo: verify signed router

fcee141

Verify that a router has a valid signature. Referencing monero-project#627 + monero-project#917

Tests: RouterInfo signature unit-tests

e850f57

Referencing monero-project#627 + monero-project#917

coneiric force-pushed the ri-sig branch from 512317f to e850f57 Compare July 2, 2018 20:31

anonimal added 4 commits July 2, 2018 23:31

Tests: cleanup RI test comments

580a5b8

Referencing monero-project#917

RouterInfo: Verify: remove auto for data pointer

e89e992

Unclear contract (and auto* may as well be uint8_t*). Referencing monero-project#917

RouterInfo: remove unnecessary null buffer check

ed041ea

Buffer is always at least zero initialized. Referencing monero-project#917

RouterInfo: don't throw when RI fails sig verify

18b3526

Sanely, we would want to throw here but doing so will stop the router instead of allowing the RI to become updated. Referencing monero-project#917

anonimal mentioned this pull request Jul 2, 2018

Ri sig coneiric/kovri#2

Merged

coneiric and others added 2 commits July 3, 2018 01:59

Merge pull request #2 from anonimal/ri-sig

7129acd

Ri sig monero-project#917

RouterInfo: don't throw on invalid RI size

237e1f5

Sanely, we would want to throw here but doing so will stop the router instead of allowing the RI to become updated. Referencing monero-project#917

anonimal mentioned this pull request Jul 3, 2018

RouterInfo: don't throw on invalid RI size coneiric/kovri#4

Merged

anonimal suggested changes Jul 3, 2018

View reviewed changes

Merge pull request #4 from anonimal/ri-sig

8079f44

RouterInfo: don't throw on invalid RI size

anonimal approved these changes Jul 3, 2018

View reviewed changes

anonimal merged commit 8079f44 into monero-project:master Jul 3, 2018

coneiric deleted the ri-sig branch July 13, 2018 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RouterInfo: refactor signature creation and verification #917

RouterInfo: refactor signature creation and verification #917

coneiric commented Jun 24, 2018

anonimal Jun 24, 2018

coneiric Jun 25, 2018 •

edited

Loading

anonimal Jun 24, 2018

coneiric Jun 25, 2018

anonimal Jun 24, 2018

coneiric Jun 25, 2018

anonimal Jun 24, 2018

coneiric Jun 25, 2018

anonimal Jun 25, 2018

coneiric Jun 25, 2018

anonimal commented Jun 24, 2018

coneiric commented Jun 27, 2018

coneiric commented Jun 28, 2018

coneiric commented Jul 2, 2018

anonimal left a comment

coneiric commented Jul 2, 2018

anonimal commented Jul 2, 2018

anonimal left a comment

RouterInfo: refactor signature creation and verification #917

RouterInfo: refactor signature creation and verification #917

Conversation

coneiric commented Jun 24, 2018

Choose a reason for hiding this comment

coneiric Jun 25, 2018 • edited Loading

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 Jun 24, 2018

coneiric commented Jun 27, 2018

coneiric commented Jun 28, 2018

coneiric commented Jul 2, 2018

anonimal left a comment

Choose a reason for hiding this comment

coneiric commented Jul 2, 2018

anonimal commented Jul 2, 2018

anonimal left a comment

Choose a reason for hiding this comment

coneiric Jun 25, 2018 •

edited

Loading