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

Validate password by signing a blank transaction. Good idea? #1324

Closed
jefft0 opened this issue Oct 31, 2023 · 2 comments · Fixed by #1047
Closed

Validate password by signing a blank transaction. Good idea? #1324

jefft0 opened this issue Oct 31, 2023 · 2 comments · Fixed by #1047
Labels
📦 🤖 gnovm Issues or PRs gnovm related ❓ question Questions about Gno

Comments

@jefft0
Copy link
Contributor

jefft0 commented Oct 31, 2023

The gnoclient package proposed in #1047 has a Validate method with a TODO for "Also verify if the password unlocks the account." GnoMobile has a copy of the gnoclient package. In our copy we have implemented the password check by creating a blank transaction and calling the available interface method for Sign. This works and is fairly compact.

Our question: Is there a preferred or better way to do a test decryption of the private key? (There are lower-level functions but those are private and specific to a particular Keybase implementation.)

	msg := vm.MsgCall{
		Caller: s.Info().GetAddress(),
	}
	signCfg := SignCfg{
		UnsignedTX: std.Tx{
			Msgs: []std.Msg{msg},
			Fee:  std.NewFee(0, std.NewCoin("ugnot", 1000000)),
		},
	}
	if _, err = s.Sign(signCfg); err != nil {
		return err
	}
@moul
Copy link
Member

moul commented Nov 26, 2023

I agree this is a smart approach. What's important is not just having a valid password, but being able to use the private key's functions. When using alternatives like a ledger or multisig, the user experience (UX) won't be the same.

For command-line interfaces (CLI), a good method is to skip the initial checks and only check for errors or problems when you actually need to sign something with the private key.

For mobile apps or systems with a sign-up process, I see why you'd want this method, and I think it's a good enough solution.

However, with a ledger, you wouldn't do this check because it's not helpful and can be annoying. Instead, you should just confirm the public key, either through the ledger's API or by scanning a QR code or something similar. So, in some cases, like with a ledger, you might choose to not check at all and just wait until you really need to use the key.

@jefft0
Copy link
Contributor Author

jefft0 commented Nov 27, 2023

Thanks for the detailed response. This answers our question. Should we close this issue?

@thehowl thehowl closed this as completed Dec 7, 2023
thehowl pushed a commit that referenced this issue Jan 18, 2024
This PR add a dedicated client library for interacting seamlessly with
the Gno.land RPC API.
This library simplifies the process of querying or sending transactions
to the Gno.land RPC API and interpreting the responses.

- [x] initial structure and calls
- [x] unit test (depends on #1101)
- [x] add additional query methods
- [ ] add additional transaction methods (can/should be done later)

Closes #1324


<details><summary>Previous list of ideas</summary>

Note: Some content may be outdated. Please overlook for the moment, but
we'll reassess at the conclusion of this PR to identify any current
requirements.


- [ ] port
https://github.com/gnolang/gno/tree/master/tm2/pkg/crypto/keys/client
into a simple `./gno.land/pkg/gnoclient` package (Call, Send, AddPkg,
Query, Eval, Package, File)
- [ ] add unit tests so that crypto/keys/client can have less unit tests
and be more focused on CLI stuff
- [ ] create mock/testing helpers to make it more useful when writing
integration tests -> `./gno.land/cmd/gnoland/*_test.go`
- [ ] port a client, i.e., `gnofaucet`, `gnoweb`, `gnoblog`, etc; so
they use this new library instead of hardcoded manual clients
- [ ] consider having right now underlying `tm2client` and `gnovmclient`
libs that this lib will depend on, but will probably do it later and
start with a monolith library
- [ ] alternative configuration (pass existing websocket?)
- [ ] minimal go.mod to make it light to import
- [ ] open issue on other clients (gnoblog, discord faucet, gnomobile)
</details>

<details><summary>Contributors' checklist...</summary>

- [ ] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: Manfred Touron <94029+moul@users.noreply.github.com>
Signed-off-by: Jeff Thompson <jeff@thefirst.org>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Co-authored-by: Jeff Thompson <jeff@thefirst.org>
Co-authored-by: Guilhem Fanton <8671905+gfanton@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related ❓ question Questions about Gno
Projects
Development

Successfully merging a pull request may close this issue.

3 participants