Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add request verification on send #7

Merged
merged 15 commits into from
Jan 9, 2023
Merged

Conversation

lumtis
Copy link
Contributor

@lumtis lumtis commented Jan 5, 2023

Close ignite/cli#3257

Add the verification logic for requests when they are sent in addition to approve.

For this some refactoring have been done:

  • There is a single method to send requests
  • Request content is initialized outside of it and used in a method to simulate chain from request contents
  • Join is no longer sending requests, it returns request content for simulation

Test

Try to join the chain as a coordinator with a gentx with an invalid signature (it used to work)

ignite network chain join 10 --amount 95000000stake
? Peer's address ...
Source code fetched
Blockchain set up
Source code fetched
Blockchain set up
Requests format verified
Blockchain initialized
Genesis initialized
Genesis built
✘ The chain failed to start

@lumtis lumtis marked this pull request as ready for review January 6, 2023 13:24
network/join_test.go Outdated Show resolved Hide resolved
@tbruyelle
Copy link
Collaborator

Try to join the chain as a coordinator with a gentx with an invalid signature (it used to work)

Sorry but how to use an invalid signature ^^' ?

@lumtis
Copy link
Contributor Author

lumtis commented Jan 6, 2023

Try to join the chain as a coordinator with a gentx with an invalid signature (it used to work)

Sorry but how to use an invalid signature ^^' ?

By opening the gentx file generated from the init command and manually modifying data in the signatures field

@lumtis lumtis requested a review from tbruyelle January 7, 2023 09:31
@tbruyelle
Copy link
Collaborator

tbruyelle commented Jan 9, 2023

@lubtd What I did so far :

With the main version:

ignite network chain init 10

Alter gentx signature in $HOME/spn/10/config/gentx/gentx.json

{
   "body":{
      "messages":[
         {
            "@type":"/cosmos.staking.v1beta1.MsgCreateValidator",
            "description":{
               "moniker":"moniker",
               "identity":"",
               "website":"",
               "security_contact":"",
               "details":""
            },
            "commission":{
               "rate":"0.100000000000000000",
               "max_rate":"0.200000000000000000",
               "max_change_rate":"0.010000000000000000"
            },
            "min_self_delegation":"1",
            "delegator_address":"cosmos13gdkcvyz8us4u5lwdh7n5adldydrzmmgt0rfsw",
            "validator_address":"cosmosvaloper13gdkcvyz8us4u5lwdh7n5adldydrzmmgwmhuua",
            "pubkey":{
               "@type":"/cosmos.crypto.ed25519.PubKey",
               "key":"xYkKtYSNzod+l+fn0wBMmDD7aQZhCz5NCT/nFmk2JYQ="
            },
            "value":{
               "denom":"stake",
               "amount":"95000000"
            }
         }
      ],
      "memo":"[email protected]:26656",
      "timeout_height":"0",
      "extension_options":[
         
      ],
      "non_critical_extension_options":[
         
      ]
   },
   "auth_info":{
      "signer_infos":[
         {
            "public_key":{
               "@type":"/cosmos.crypto.secp256k1.PubKey",
               "key":"ApllJatAUGW+po90temdo1IVfyByvkxAuw2ME0G3MDE7"
            },
            "mode_info":{
               "single":{
                  "mode":"SIGN_MODE_DIRECT"
               }
            },
            "sequence":"0"
         }
      ],
      "fee":{
         "amount":[
            
         ],
         "gas_limit":"200000",
         "payer":"",
         "granter":""
      },
      "tip":null
   },
   "signatures":[
-      "mn2D051PW7l7vXLOxFbzmWjbnZ6p/dDBhTlxi0RmZQMUDBO0fyYWyyvfL5EWeV+CJEaOXOV/rvxU+Sk8iQwX0A=="
+      "mn2D051PW7l7vXLOxFbzmWjbnZ6p/dDBhTlxi0RmZQMUDBO0fyYWyyvfL5EWeV+CJEaOXOV/rvxU+Sk8iQwX0B=="
   ]
}

Run ignite network chain join 10 --amount 95000000stake, as expected it worked because it's the main version. Then I rejected these requests.

Changed my .ignite/plugins/plugins.yml :

plugins:
-- path: github.com/ignite/cli-plugin-network@main
+- path: github.com/ignite/cli-plugin-network@feat/request-verification

Run ignite network chain join 10 --amount 95000000stake again, and it also worked, despite the bad signature. Not sure if I did something wrong.

@lumtis
Copy link
Contributor Author

lumtis commented Jan 9, 2023

@tbruyelle can't reproduce

The command fails with the change:

"signatures":["qQTPrt4U6IrYbTWuvLLBVvw892z28waNd06/cG9zUHwgweMgQAvkFe6kMgInk3hKGYMGoPNywEvTcBt/jKnSkQ=="]
--->
"signatures":["qQTPrt4U6IrYbTWuvLLBVvw892z28waNd06/cG9zUHwgweMgQAvkFe6kMgInk3hKGYMGoPNywEvTcBt/jKnSkA=="]

@tbruyelle
Copy link
Collaborator

@lubtd OK I have also a panic "signature verification failed" if I replace the A with a Q (and that doesn't happen with main).

I'm just wondering why that doesn't happen if I replace the A with a B 🤔

@lumtis
Copy link
Contributor Author

lumtis commented Jan 9, 2023

When I replace the Q by a R (following letter) this is also working. This is very strange

I try to see if this a normal part of the signature since the failing example shows it goes into the same workflow that tries to initialize the genesis and therefore verifies the signatures

@lumtis
Copy link
Contributor Author

lumtis commented Jan 9, 2023

@tbruyelle what about creating a separate issue to investigate this? At this point it's the appd start command that works with a modified signature

@tbruyelle
Copy link
Collaborator

At this point it's the appd start command that works with a modified signature

so you mean the issue should be created in the CLI?

@aljo242
Copy link
Contributor

aljo242 commented Jan 9, 2023

Will approve once new issue is opened

@lumtis
Copy link
Contributor Author

lumtis commented Jan 9, 2023

so you mean the issue should be created in the CLI?

No, the logic that verifies transaction signature in the SDK, since a gentx is a regular Cosmos tx

@lumtis
Copy link
Contributor Author

lumtis commented Jan 9, 2023

Will approve once new issue is opened

#12

@lumtis lumtis merged commit 68267f0 into main Jan 9, 2023
@lumtis lumtis deleted the feat/request-verification branch January 9, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify requests sent by the coordinator
3 participants