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

charon create enr overwrites an existing one on disk #893

Closed
OisinKyne opened this issue Aug 2, 2022 · 7 comments
Closed

charon create enr overwrites an existing one on disk #893

OisinKyne opened this issue Aug 2, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@OisinKyne
Copy link
Contributor

Problem to be solved

I wanted to reassure people that calling charon enr again on the same private key will make an ENR that looks different, but has the same private key in it under the hood. While testing I ran charon enr, copied it into enr-viewer.com, then ran charon create enr, got an enr that looked different, so I put it int enr-viewer.com in another tab to assert that the public keys would be the same until I realised they didn't match.

Investigating further I saw that charon create enr seemed happy to silently overwrite an existing private key found there.

Note, this was with a local go install charon version: v0.7.0 [git_commit_hash=dd47136,git_commit_time=2022-07-07T15:39:53Z] so this may be fixed in later versions.

Proposed solution

If there is a file at .charon/charon-enr-private-key or wherever directed, log something like charon-enr-private-key already present, using it and generate an ENR from that, rather than creating a new one and overwriting it.

If what's present doesn't work as a private key (can't open/read file, not an integer parsed out of it etc.) the command should fail with Unrecognised charon-enr-private-key file present, failed to issue an ENR from it.

Out of Scope

If there is anything to highlight as out of scope for this issue, please outline it here.

@xenowits xenowits self-assigned this Aug 4, 2022
@xenowits
Copy link
Contributor

xenowits commented Aug 4, 2022

I think that was an issue with the previous versions but not with the latest version v0.9.0 (i am confirming this with v0.9.0 [git_commit_hash=a144a15,git_commit_time=2022-08-04T03:31:06Z]). I have also confirmed by inputting different enrs into https://enr-viewer.com/ by running charon enr multiple times and I got the same secp256k1 pubkey.

It was fixed in #316. See comment.

@xenowits
Copy link
Contributor

xenowits commented Aug 4, 2022

I'm curious as to why you had this issue since you are on v0.7.0 but the fix was already released as part of v0.3.0 ( See release )

@xenowits
Copy link
Contributor

xenowits commented Aug 4, 2022

The current behaviour is as follows:

  1. If charon-enr-private-key already exists, running charon enr DOESN'T create a new key. It simply loads the key from .charon/charon-enr-private-key and derives and prints the ENR from it. Example:
Created ENR private key: .charon/charon-enr-private-key
enr:-JG4QB_nlJIlzGPkSgNSyrqRj4XPnkTlUaOSR--WesFKztkzSNapQgqCz-UektANL6-hQaZZVPNbQNdkXGc4BYkYM82GAYJngsgrgsefgfde3gfdfJc2VjcDI1NmsxoQMmfsahqaM1gTukW6F1Vcgu02gpDqeKIuufBVbl8Uc2MoN0Y3CCDhqDdWRwgg4u

***************** WARNING: Backup key **********************
 PLEASE BACKUP YOUR KEY IMMEDIATELY! IF YOU LOSE YOUR KEY,
 YOU WON'T BE ABLE TO PARTICIPATE IN RUNNING A CHARON CLUSTER.

 YOU CAN FIND YOUR KEY IN .charon/charon-enr-private-key
****************************************************************
  1. If .charon/charon-enr-private-key doesn't exist, running charon enr outputs an error:
Error: private key not found. If this is your first time running this client, create one with `charon create enr`.
Usage:
  charon enr [flags]

Flags:
      --data-dir string                The directory where charon will store all its internal data (default ".charon")
  -h, --help                           Help for enr
      --p2p-allowlist string           Comma-separated list of CIDR subnets for allowing only certain peer connections. Example: 192.168.0.0/16 would permit connections to peers on your local network only. The default is to accept all connections.
      --p2p-bootnode-relay             Enables using bootnodes as libp2p circuit relays. Useful if some charon nodes are not have publicly accessible.
      --p2p-bootnodes strings          Comma-separated list of discv5 bootnode URLs or ENRs. (default [http://bootnode.gcp.obol.tech:3640/enr])
      --p2p-bootnodes-from-lockfile    Enables using cluster lock ENRs as discv5 bootnodes. Allows skipping explicit bootnodes if key generation ceremony included correct IPs.
      --p2p-denylist string            Comma-separated list of CIDR subnets for disallowing certain peer connections. Example: 192.168.0.0/16 would disallow connections to peers on your local network. The default is to accept all connections.
      --p2p-external-hostname string   The DNS hostname advertised by libp2p. This may be used to advertise an external DNS.
      --p2p-external-ip string         The IP address advertised by libp2p. This may be used to advertise an external IP.
      --p2p-tcp-address strings        Comma-separated list of listening TCP addresses (ip and port) for libP2P traffic. (default [127.0.0.1:3610])
      --p2p-udp-address string         Listening UDP address (ip and port) for discv5 discovery. (default "127.0.0.1:3630")

Error: private key not found. If this is your first time running this client, create one with `charon create enr`.

@xenowits
Copy link
Contributor

xenowits commented Aug 4, 2022

The current behaviour for charon create enr is as follows:

  1. If charon-enr-private-key already exists, it OVERWRITES the existing private key and logs the ENR.
  2. If charon-enr-private-key doesn't exist, it CREATES a new one and logs the ENR.

NOTE: No warning is logged when overwriting an existing key when charon create enr is ran again.

@corverroos
Copy link
Contributor

@xenowits just error in charon create enr is the file already exists.

@corverroos
Copy link
Contributor

corverroos commented Aug 4, 2022

For charon enr, I suggest we print out the ENR as currently, BUT ALSO print out the decoded ENR. That way it is obvious why a new ENR is different every time the command is called, but it also clear that the identity/pubkey remains the same.

Note that charon create enr is technically a misnomer, since it creates a private key AND a ENR. charon enr creates a new ENR from that existing private key...

From https://enr-viewer.com/:
image

Example output:

$> charon enr
enr:-JG4QG17y8Y2hbChTBStvzaagbOSVHyAX6XO4VtUEsisFEGzQyjJCT5NfCNSbRuutbO5aKIVz64FpQk6qwMPBvHNx9eGAYJn5o6RgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQNrc9-2akHTZr-eQ0eqgC_-Tmia6dirtrwkTe_W2L1wRYN0Y3CCDhqDdWRwgg4u

****** Decoded ENR (see also https://enr-viewer.com/) **********
public key: 0x036b73dfb66a41d366bf9e4347aa802ffe4e689ae9d8abb6bc244defd6d8bd7045
signature: 0xb7c1d79ccf663e68040ab2f8861b73492f61fba600b30689a8c7017a567b493654e7839255244fd1ba1132730959c6b0d70bbbd0e0dd8cc07dc88e810218b842
seq: 1659598407005
id: v4
ip: 127.0.0.1
tcp: 1234
udp: 3456
******************************************************************

can add a -quiet flag to only print ENR without decoding

@corverroos
Copy link
Contributor

Suggest also updating the charon enr command description.

From: Return information on this node's Ethereum Node Record (ENR)
To: Prints a new generated Ethereum Node Record (ENR) from this nodes charon-enr-private-key

obol-bulldozer bot pushed a commit that referenced this issue Aug 4, 2022
- `charon create enr` now errors when `charon-enr-private-key` already exists.
- `charon enr` now also outputs`secp256k1 pubkey`, `signature` and `seq` in addition to the `enr` when the `--verbose` flag is provided.

category: refactor
ticket: #893
@xenowits xenowits closed this as completed Aug 4, 2022
@OisinKyne OisinKyne added the bug Something isn't working label Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants