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

[Client Tooling] Keybase (issue #455) #459

Merged
merged 60 commits into from
Feb 2, 2023
Merged

[Client Tooling] Keybase (issue #455) #459

merged 60 commits into from
Feb 2, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Jan 23, 2023

Description

This is an implementation of a Keybase for the pocket V1 client.

It uses BadgerDB as the local database for the persistent storage of keys. The current Keybase interface has the following methods

image

These are covered with the following tests

image

The tests use an in-memory database created with NewKeybaseInMemory() whereas when exposed to the user the CLI should call NewKeybase() instead with a default path/user-supplied path. The CLI integration has not been implemented as part of this PR however.

How it works

The keybase heavily relies upon shared/crypto/ed25519.go and the PublicKey and PrivateKey interfaces it exposes.

The keys are created using the methods from this library. Keys are then encrypted and armoured in the same fashion as in V0 and stored in the DB as a KeyPair struct (encoded to a []byte).

image

This struct contains the public key and the encrypted JSON encoded private key string. This struct has the following methods:

  • GetAddressBytes() []byte -> returns []byte of the public key address
  • GetAddressString() string -> returns the hex string of the public key address
  • Unarmour(passphrase string) (crypto.PrivateKey, error) -> returns the unencrypted unarmoured private key if passphrase is correct
  • ExportString(passphrase string) (string, error) -> returns raw private key string is passphrase is correct
  • ExportJSON(passphrase string) (string, error) -> returns json armoured private key string if the passphrase is correct

Keys are stored in the database in the following manner:

  • The keys for the BadgerDB key-value storage are the []byte value returned by KeyPair.GetAddressBytes() aka the []byte address of the public key
  • The values are the []byte encoded KeyPair structs of the key

Key encryption/decryption works with or without a passphrase (when no passphrase is given "" must still be passed to the function as the passphrase argument)

For ease of use the hex string returned by KeyPair.GetAddressString() is used to access the keys in storage.

Importing and exporting keys in either JSON string format or the private key hex string is fully interoperable between V1 and V0

Signing and Verification of messages works - and is covered with a Tx in the tests. I would need to look into the use cases for this more (for example multisig) as the current implementation is very rudimentary and in theory should work for producing a signature on any []byte message but I would like to look into this more.

Issue

Fixes #455

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Implement a keybase with BadgerDB local DB
  • Add functionality to Import/Export keys, Get/List keys, Create new keys, Delete Keys, Sign Messages, Verify Signatures
  • Add make test_app entry point in Makefile
  • Add unit tests to cover Keybase use cases
  • Add documentation for Keybase

Testing

  • make develop_test
  • LocalNet w/ all of the steps outlined in the README

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@h5law h5law added the client work needed to interface with the node (rpc, cli, etc..) label Jan 23, 2023
@h5law h5law added this to the M1: Pocket PoS (Proof of Stake) milestone Jan 23, 2023
@h5law h5law self-assigned this Jan 23, 2023
@h5law h5law requested a review from Olshansk January 23, 2023 17:45
@h5law
Copy link
Contributor Author

h5law commented Jan 24, 2023

Thanks to @jessicadaugherty for giving me a hint to what might be the issue - HEX ENCODING!. V0's ed25519 library has 2 extra functions for the PrivateKey type RawBytes() and RawString() I previously ignored these as they are not in the V1 library of the same name but they were the key to this issue.

Import/Export JSON is fully interoperable between V0 and V1 now:

When encrypting the private key now convert the hex encoded private key string to []byte

image

This is instead of directly getting the result of PrivateKey.Bytes() to match how V0 does the same task - although this extra conversion step does nothing for security it changes the length of the cipher text in the JSON encoded string.

Now when decrypting we must do another conversion/decoding step:

image

And the private key is then created from bz. When originally implementing this I thought this was weird and instead used the more direct approach (in my opinion) however this made the encrypted cipher text much shorter (despite representing the same values). This is now fully interoperable with V0 and is covered in a unit test with an account generated by wallet.pokt.network (manually testing the reverse works by t.Log(jsonStr) in the ExportJSON test works too)

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really impressive work, in terms of quality, speed and impact.

Regarding your comment that fixes the V0 <-> V1 conversion. Tbh, I'm not a fan of all the data transformations, and I'm sure there's some redundancy since we ported some of the crypto library logic in v0.

If you see opportunities to clean it up in the future, just go for it.

giphy (1)

I will follow up in #455 regarding some next steps on this & other tickets.

app/client/keybase/keybase_test.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keys.go Outdated Show resolved Hide resolved
app/client/keybase/keys.go Outdated Show resolved Hide resolved
app/client/keybase/keybase_test.go Outdated Show resolved Hide resolved
app/client/keybase/keybase_test.go Outdated Show resolved Hide resolved
app/client/keybase/keybase_test.go Outdated Show resolved Hide resolved
app/client/keybase/keys.go Outdated Show resolved Hide resolved
@h5law h5law added the tooling tooling to support development, testing et al label Jan 28, 2023
@h5law
Copy link
Contributor Author

h5law commented Jan 30, 2023

@Olshansk I have left a reply to the tx.Delete() comment you made - personally I think it is better to stick to using the Update() transaction wrapper provided by BadgerDB as 1) It fits the pattern 2) it is less lines of code (it handles commit for us). I can change this if you really want the explicit tx.Delete() outside of a wrapper but this is why I didn't change it first time.

@h5law h5law requested a review from Olshansk January 30, 2023 22:34
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h5law Almost over the finish line.

Left a couple small comments but also took the liberty to make some updates to the README while reading through it.

Take a look and fill free to update anything I missed or misrepresented.

Should be good to go afterwards.

app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Outdated Show resolved Hide resolved
shared/crypto/README.md Show resolved Hide resolved
@h5law h5law requested a review from Olshansk February 2, 2023 12:01
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment but otherwise LGTM 🔥

app/client/keybase/keystore.go Outdated Show resolved Hide resolved
app/client/keybase/keystore.go Show resolved Hide resolved
@h5law h5law merged commit ed1a62a into main Feb 2, 2023
@h5law h5law deleted the keybase_implementation branch February 2, 2023 22:41
@h5law h5law mentioned this pull request Feb 2, 2023
16 tasks
h5law added a commit that referenced this pull request Feb 3, 2023
## Description

Following the merge of #459 the `app/client/cli/doc` directory has moved
to `app/client/doc` this PR fixes the links in the README

## Issue

Fixes N/A

## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Major breaking change
- [x] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- Fix links to app documentation
- Change CLI to APP in links

## Testing

- [x] `make develop_test`
- [x]
[LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md)
w/ all of the steps outlined in the `README`


## Required Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have tested my changes using the available tooling
- [x] I have updated the corresponding CHANGELOG

### If Applicable Checklist
- [x] I have updated the corresponding README(s); local and/or global
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I have added, or updated,
[mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding
README(s)
- [ ] I have added, or updated, documentation and
[mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*`
if I updated `shared/*`README(s)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client work needed to interface with the node (rpc, cli, etc..) tooling tooling to support development, testing et al
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Client Tooling] Keybase
2 participants