Add secure minting #110

davebryson · 2022-05-26T17:11:11Z

This PR addresses issue #98 adding support for secure minting. Mint transactions now run through the sentinel and validation. Minters are identified via pre-approved key pairs used during sentinel validation to determine who is allowed to execute a mint transaction. Public keys associated with approved minters are added to the cfg file.

For testing and demonstrations a pre-calculated minter key pair is available. Existing cfg files have been updated with the information.

A mint transaction is identified as having no inputs and 1 or more outputs. Validation has been adjusted to support this to ensure only valid minting transactions are forwarded from the sentinel. This required a change in the Atomizer as it will no longer reject a transaction that has no inputs (as it did previously).

PR #96 should be applied before this PR

HalosGhost · 2022-06-01T20:40:46Z

@davebryson looks like there's a small merge conflict (and some linting issues); but a general rebase is probably a good idea now that the macOS script updates have been merged in.

davebryson · 2022-06-02T01:28:10Z

@HalosGhost Rebased. But still need to address cognitive complexity issue (clang-tidy) in client-cli.cpp

davebryson · 2022-06-02T15:57:40Z

@HalosGhost clang-tidy is failing due to the number of if/else clauses in client-cli.cpp main() exceeding the default threshold of 25 (there are currently 26) for the readability-function-cognitive-complexity.Threshold setting.

Any objections to increasing the threshold setting in .clang-tidy to provide a little room for growth in the command line client?

HalosGhost · 2022-06-02T16:29:19Z

I think a longer-term solution would be to factor out a basic parsing mechanism so that we can have less of an if/else soup (ideally allowing use of a function-pointer map, or switch/case); but that's definitely a larger refactoring effort. I don't have any immediate problem raising the threshold. @metalicjames is there a middle-ground that you think we should pursue instead?

metalicjames · 2022-06-02T16:43:40Z

I would prefer that we move the command dispatch code to its own function than increase the cognitive complexity limit.

Secure minting and small refactor of command line client Signed-off-by: Dave Bryson <[email protected]>

Fix clang-tidy issue with return after if/else Signed-off-by: Dave Bryson <[email protected]>

davebryson · 2022-06-03T00:36:42Z

@HalosGhost Not sure what the issue is with the CI failing on unit/integration tests. Note commit 9fa789e above passed all tests. Last commit (that failed tests) made no changes that would effect tests. Additionally I've tested locally several times and all unit/integration tests are passing.

HalosGhost · 2022-06-03T12:57:47Z

Seems it's the atomizer end-to-end tests:

/home/runner/work/opencbdc-tx/opencbdc-tx/tests/integration/atomizer_end_to_end_test.cpp:58: Failure
Value of: m_ctl_watchtower->get_block_height() > 0
  Actual: false
Expected: true
scripts/test.sh: line 9:  8480 Aborted                 (core dumped) $PWD/$1

I will try to pull the changes and test locally today to see what's going wrong.

HalosGhost · 2022-06-03T21:30:11Z

@davebryson your suspicions were correct; timing issue during the run; rerunning it passed all checks in a breeze. I'll dig in and begin review in the next few days!

HalosGhost

Sorry it has taken me a little while to review this. I think there's a lot of great work here (and a solid step towards having some rough means of secure minting).

I do have a few questions to make sure I'm understanding the flow right, and a few requests for changes that I've made inline.

Please feel free to ask for clarification!

src/uhs/atomizer/sentinel/controller.cpp

src/uhs/atomizer/shard/shard.cpp

src/uhs/client/client-cli.cpp

src/uhs/transaction/validation.cpp

src/uhs/transaction/wallet.cpp

tests/unit/validation_test.cpp

Signed-off-by: Dave Bryson <[email protected]>

davebryson · 2022-06-07T23:40:08Z

@HalosGhost Latest commit addresses requested changes

HalosGhost

@davebryson these changes look solid (sorry there was such a delay in-review). I have a small comment, and I'd like @metalicjames to take a quick look at it before merge, but I'm comfortable with merging.

HalosGhost · 2022-06-09T15:41:27Z

src/uhs/client/client.hpp

@@ -237,6 +251,8 @@ namespace cbdc {
        /// \brief Sends the given minting transaction to a service that will
        ///        accept and process it.
        ///
+        /// TODO:  Remove. No longer needed


Is the point of leaving this just to minimize the change in this PR? (it's otherwise leaving specifically-dead code in the repo.)

@HalosGhost I primarily left it in the event someone objected to the changes and/or wanted the functionality for other reasons (testing?).

Nothing occurs to me on why it would still be needed with the adopting of this (pretty simple even) minting mechanism, but perhaps @metalicjames will think otherwise (I'm open to hearing alternative opinions). :)

If we're not using it, let's remove it.

src/uhs/transaction/wallet.cpp

metalicjames

I have a general question. Can I make multiple mint transactions with this PR?

It seems that all mint transactions will be identical, resulting in the same outpoints for every mint TX, meaning I can actually only make one valid mint TX without overwriting (or re-adding!! previously used outputs). One can see how this could lead to replay attacks where a previously submitted transaction spending a mint output becomes valid again when its input(s) are recreated.

The signatures on the outputs are missing any kind of sequence number or nonce. Does that matter? Is there a replay attack vector on mint transactions without one?

metalicjames · 2022-07-11T20:07:20Z

src/uhs/atomizer/sentinel/controller.cpp

+            if(!m_shard_network.send_to_one(ctx_pkt)) {
+                m_logger->error("Failed to send mint tx to shard");
+            }
+        } else {


Style point: prefer return; over else here to avoid the unnecessary condition nesting (which causes a cognitive complexity penalty).

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f56-avoid-unnecessary-condition-nesting

metalicjames · 2022-07-11T20:13:18Z

src/uhs/atomizer/shard/shard.cpp

-            return cbdc::watchtower::tx_error{
-                tx.m_id,
-                cbdc::watchtower::tx_error_inputs_dne{{}}};
+            msg.m_tx = std::move(tx);


Don't we need the sentinel attestations here?

m_attestations is not set.

@metalicjames This is related to an issue that I asked about early in the process of working on this. The Atomizer, unlike 2PC, actually requires inputs in the compact transaction. A mint transaction doesn't have any inputs. So making mint transactions work with the Atomized feels like a hack and deviates from the normal transaction flow. Any thoughts or insights you have on this would be appreciated.

Hmm, I think the atomizer should be able to understand mint transactions. Otherwise, how would the mint transaction be included in a block and applied by the shards? If the transaction is included in a block, the atomizer should check the sentinel attestations. A valid mint transaction should still be validated by multiple sentinels and attested to, the same way as any other transaction. Otherwise, how can we be confident the mint transaction wasn't validated by a malicious sentinel, ignoring the authorized minter keys?

@metalicjames A mint transaction flows through the sentinel and accumulates attestations the same as any other transaction. The controller checks the attestations before it forwards it to the shard: https://github.com/mit-dci/opencbdc-tx/blob/trunk/src/uhs/atomizer/shard/controller.cpp#L152. All the shard appears to do is collect the input index into the msg attestations set https://github.com/mit-dci/opencbdc-tx/blob/trunk/src/uhs/atomizer/shard/shard.cpp#L151. What concerns me more is the fact that atomizer allow this (at least passes all tests) even though the msg.m_attestations is empty for a mint transaction.

metalicjames · 2022-07-11T20:15:29Z

src/uhs/client/client-cli.cpp

              << std::endl;
+
+    if(resp.has_value()) {


This code seems duplicated from print_tx_result, can we refactor it?

metalicjames · 2022-07-11T20:17:35Z

src/uhs/client/client.hpp

@@ -237,6 +251,8 @@ namespace cbdc {
        /// \brief Sends the given minting transaction to a service that will
        ///        accept and process it.
        ///
+        /// TODO:  Remove. No longer needed


If we're not using it, let's remove it.

metalicjames · 2022-07-11T20:19:40Z

src/uhs/transaction/validation.cpp

@@ -33,8 +33,13 @@ namespace cbdc::transaction::validation {
        return std::tie(m_code, m_idx) == std::tie(rhs.m_code, rhs.m_idx);
    }

-    auto check_tx(const cbdc::transaction::full_tx& tx)
+    auto check_tx(const cbdc::transaction::full_tx& tx,
+                  const std::unordered_set<pubkey_t, hashing::null>& minters)


Is it possible to use a more general container here (like a vector, array etc)? Requiring a std::unordered_set with a custom hasher seems too specific to me.

metalicjames · 2022-07-11T20:25:24Z

src/uhs/transaction/validation.hpp

+    auto check_tx(const transaction::full_tx& tx,
+                  const std::unordered_set<pubkey_t, hashing::null>& minters)
+        -> std::optional<tx_error>;
+    auto


Can you add doc comments for the new functions in this file?

metalicjames · 2022-07-11T20:26:55Z

src/uhs/transaction/wallet.cpp

+        const privkey_t seckey = m_keys.at(pubkey);
+
+        // Sign each output
+        for(size_t i = 0; i < n_outputs; i++) {


This signing code seems duplicated with the code in wallet::sign, can we refactor it?

Some of the code is duplicate. As I was touching a lot of the code base, I didn't want to be too intrusive

I think it's best to refactor the duplicate code in this PR. That will make the code more maintainable in the long run. You can split the refactors into separate commits to reduce the review burden.

metalicjames · 2022-07-11T20:27:34Z

src/uhs/transaction/wallet.cpp

@@ -25,22 +25,55 @@ namespace cbdc {
    auto transaction::wallet::mint_new_coins(const size_t n_outputs,
                                             const uint32_t output_val)
        -> transaction::full_tx {
-        transaction::full_tx ret;


Any reason to rename this variable?

metalicjames · 2022-07-11T20:31:14Z

src/uhs/transaction/wallet.cpp

+            return generate_key();
+        }
+        // first key is always the minter key
+        return m_pubkeys[0];


I think you need to hold the lock to access m_pubkeys here.

metalicjames · 2022-07-11T20:32:53Z

src/uhs/transaction/wallet.cpp

+        return m_pubkeys[0];
+    }
+
+    auto transaction::wallet::generate_test_minter_key() -> pubkey_t {


I'm confused about this. Is the test minter key the one the validation code will check signed the TX? How is that different from the key generated when I can generate_minter_key()?

What if I call generate_minter_key() first, and then generate_test_minter_key() after? Now m_pubkeys[1] contains the minter key. Same thing if I call generate_test_minter_key() multiple times.

generate_test_minter_key generates a pre-determined key that's preset in all existing .cfg files for testing and/or demos. Again, I was looking for a way to minimize changes across the codebase. See

opencbdc-tx/src/uhs/transaction/wallet.hpp

Line 129 in eb16c65

/// Generate a fixed public key for a minter. This is

for a descriptoin

Can we not read the key from the config file? Why do we need code to re-generate it?

@metalicjames Mainly because tests call wallet code to mint (especially the integration tests) and you need a deterministic key for that. Putting the minter's private key in the configuration file for testing would mean you should put all minter private keys in the configuration file (testing and non-testing) to keep the design consistent. But IMO, putting minter (or any) private keys in the configuration file seems like a really bad idea from a security perspective.

Worth noting that this is not a production system, so we don't have to be quite as worried about such things at the moment.

For example, for sentinel attestation checks, the sentinel private keys are stored in the configuration files.

@HalosGhost Agree and I understand what you're saying. But on the other hand, adding security late to the process usually doesn't turn out well. If security is not a high-priority in the current context of the project, then is secure minting even necessary right now? Just thinking out loud on how to best bound the problem...

I take your point. I suppose my view is that we should try to scope our questions as small as possible.

I think this PR is answering the question “How could secure minting be supported?” That doesn't require all other best-practices to be addressed at the same time. To that end, having the keys in-configuration or not is an orthogonal concern (in my opinion).

davebryson · 2022-07-13T19:40:48Z

The signatures on the outputs are missing any kind of sequence number or nonce. Does that matter? Is there a replay attack vector on mint transactions without one?

@metalicjames Great question and I believe you are correct. Adding a nonce/sequence number would be an easy solution. Of course a sequence number would require some form of state known by the system for validation. But either way you have minters using the same key over and over. Policy could dictate a minter must rotate a key after each use, but this complicates configuration.

Signed-off-by: Dave Bryson <[email protected]> Added minting private keys to the configuration file. Removed older minting approach for mint keys. Moving to draft pull request as there are still other issues to resolve

davebryson force-pushed the minting branch from f1615cc to 7e1922d Compare June 2, 2022 00:52

Add support for secure minting

9fa789e

Secure minting and small refactor of command line client Signed-off-by: Dave Bryson <[email protected]>

davebryson force-pushed the minting branch from 7e1922d to 9fa789e Compare June 2, 2022 23:12

Resolve clang-tidy issue

8cc2cb8

Fix clang-tidy issue with return after if/else Signed-off-by: Dave Bryson <[email protected]>

HalosGhost self-requested a review June 3, 2022 21:30

HalosGhost requested changes Jun 6, 2022

View reviewed changes

davebryson added 2 commits June 7, 2022 18:14

Resolved pull request review items

5b417e4

Signed-off-by: Dave Bryson <[email protected]>

Fixed doxygen issue

eb16c65

Signed-off-by: Dave Bryson <[email protected]>

HalosGhost approved these changes Jun 16, 2022

View reviewed changes

HalosGhost requested a review from metalicjames June 16, 2022 22:44

mszulcz-mitre mentioned this pull request Jun 27, 2022

tcp_client::init bug fix #135

Merged

metalicjames suggested changes Jul 11, 2022

View reviewed changes

Updates to minting

b879a34

Signed-off-by: Dave Bryson <[email protected]> Added minting private keys to the configuration file. Removed older minting approach for mint keys. Moving to draft pull request as there are still other issues to resolve

davebryson marked this pull request as draft September 7, 2022 23:02

Add secure minting #110

Are you sure you want to change the base?

Add secure minting #110

Conversation

davebryson commented May 26, 2022

HalosGhost commented Jun 1, 2022

davebryson commented Jun 2, 2022

davebryson commented Jun 2, 2022

HalosGhost commented Jun 2, 2022

metalicjames commented Jun 2, 2022

davebryson commented Jun 3, 2022

HalosGhost commented Jun 3, 2022

HalosGhost commented Jun 3, 2022

HalosGhost left a comment

Choose a reason for hiding this comment

davebryson commented Jun 7, 2022

HalosGhost 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

metalicjames 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

metalicjames Jul 11, 2022 • 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

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

davebryson commented Jul 13, 2022

metalicjames Jul 11, 2022 •

edited

Loading