Add NNS Challenge support #1

Merged
pogpp merged 1 commit from mbiryukova/lego:feature/add_nns_challenge into tcl/master 2023-08-08 11:59:52 +00:00
Member
No description provided.
mbiryukova force-pushed feature/add_nns_challenge from 2ee8e19a37 to 604b9b6e91 2023-07-27 13:38:42 +00:00 Compare
mbiryukova force-pushed feature/add_nns_challenge from 604b9b6e91 to de42ce9ece 2023-07-27 13:55:23 +00:00 Compare
mbiryukova requested review from alexvanin 2023-07-27 14:02:42 +00:00
mbiryukova requested review from pogpp 2023-07-27 14:02:42 +00:00
mbiryukova force-pushed feature/add_nns_challenge from de42ce9ece to f765b0a087 2023-07-28 08:09:16 +00:00 Compare
alexvanin reviewed 2023-07-28 10:09:02 +00:00
@ -0,0 +37,4 @@
invoker interface {
Call(contract util.Uint160, operation string, params ...any) (*result.Invoke, error)
}
Owner

As I proposed in TrueCloudLab/certificates#1 (comment), let's remove invoker interface and use multiSchemeClient interface to simplify the code

As I proposed in https://git.frostfs.info/TrueCloudLab/certificates/pulls/1#issuecomment-17090, let's remove `invoker` interface and use `multiSchemeClient` interface to simplify the code
alexvanin marked this conversation as resolved
@ -0,0 +95,4 @@
}
// Present creates a TXT record using the specified parameters to fulfill the nns-01 challenge.
func (n *NNSProvider) Present(domain, token, keyAuth string) error {
Owner

Present and Cleanup functions implement Provider interface so we can use NNSProvider as Provider type. It worth to mention it in the comment.

`Present` and `Cleanup` functions implement `Provider` interface so we can use `NNSProvider` as `Provider` type. It worth to mention it in the comment.
alexvanin marked this conversation as resolved
@ -0,0 +101,4 @@
return fmt.Errorf("retrieve wallet from file: %w", err)
}
acc := w.Accounts[n.accNumber]
acc.Decrypt(n.accPassword, w.Scrypt)
Owner

I think you can decrypt account once in NewNNSProvider function, store account in the NNSProvider structure and remove fields like walletFile, accPassword, etc. What do you think?

I think you can decrypt account once in `NewNNSProvider` function, store account in the `NNSProvider` structure and remove fields like `walletFile`, `accPassword`, etc. What do you think?
alexvanin marked this conversation as resolved
@ -0,0 +103,4 @@
acc := w.Accounts[n.accNumber]
acc.Decrypt(n.accPassword, w.Scrypt)
cli, err := rpcclient.New(context.Background(), n.nnsServer, rpcclient.Options{})
Owner

You should either close the client in the end of the function. Or you can client in the NewNNSProvider and reuse it in all functions. The same for CleanUp function.

You should either close the client in the end of the function. *Or* you can client in the `NewNNSProvider` and reuse it in all functions. The same for `CleanUp` function.
alexvanin marked this conversation as resolved
@ -127,0 +134,4 @@
switch {
case !ctx.IsSet("wallet"):
log.Fatal("No wallet file provided for nns challenge.")
case !ctx.IsSet("wallet.account-number"):
Owner

Let's do a bit different here. While account number is okay for prototyping, in production we do not operate with account positions. Instead we use account address, which is a short representation of account public key.

Pretty much all wallet configuration in our project looks like this:

wallet:
  path: /path/to/wallet.json # Path to wallet
  passphrase: "" # Passphrase to decrypt wallet. If you're using a wallet without a password, place '' here.
  address: NfgHwwTi3wHAS8aFAN243C5vGbkYDpqLHP # Account address. If omitted default one will be used.

To get required account you can do something like this

import "github.com/nspcc-dev/neo-go/cli/flags"

// parse string address to uint160 type
addr, err := flags.ParseAddress(addrStr)
account := wallet.GetAccount(addr)
Let's do a bit different here. While account number is okay for prototyping, in production we do not operate with account positions. Instead we use account address, which is a short representation of account public key. Pretty much all wallet configuration in our project looks like [this](https://git.frostfs.info/TrueCloudLab/frostfs-s3-gw/src/branch/master/config/config.yaml#L2): ```yaml wallet: path: /path/to/wallet.json # Path to wallet passphrase: "" # Passphrase to decrypt wallet. If you're using a wallet without a password, place '' here. address: NfgHwwTi3wHAS8aFAN243C5vGbkYDpqLHP # Account address. If omitted default one will be used. ``` To get required account you can do something like this ```go import "github.com/nspcc-dev/neo-go/cli/flags" // parse string address to uint160 type addr, err := flags.ParseAddress(addrStr) account := wallet.GetAccount(addr) ```
alexvanin marked this conversation as resolved
mbiryukova force-pushed feature/add_nns_challenge from f765b0a087 to e07e209879 2023-07-28 15:51:27 +00:00 Compare
alexvanin reviewed 2023-07-31 11:22:20 +00:00
@ -0,0 +40,4 @@
core: core,
validate: validate,
provider: provider,
nnsTimeout: 10 * time.Second,
Owner

Can't find a usage of this timeout.

Can't find a usage of this timeout.
pogpp marked this conversation as resolved
@ -0,0 +43,4 @@
acc := w.GetAccount(accAddress)
acc.Decrypt(accountPassword, w.Scrypt)
cli, err := rpcclient.New(context.Background(), nnsServer, rpcclient.Options{})
Owner

Let's redo this a little bit.

We create cli there to use it for actor.NewSimple() and actor.NewPollingWaiter(). First one requires actor.RPCActor interface. Second one requires actor.RPCPollingWaiter interface.

Both interfaces are implemented by client and websocket client which are created in multiSchemeClient. So I think we can expand multiSchemeClient with two new interfaces and reuse it instead rpcclient. One less client and a little bit less code.

Let's redo this a little bit. We create `cli` there to use it for `actor.NewSimple()` and `actor.NewPollingWaiter()`. First one requires `actor.RPCActor` interface. Second one requires `actor.RPCPollingWaiter` interface. Both interfaces are implemented by client and websocket client which are created in `multiSchemeClient`. So I think we can expand `multiSchemeClient` with two new interfaces and reuse it instead `rpcclient`. One less client and a little bit less code.
pogpp marked this conversation as resolved
@ -0,0 +134,4 @@
// CleanUp removes the TXT record matching the specified parameters.
// It implements Provider interface in order to use NNSProvider as Provider.
func (n *NNSProvider) CleanUp(domain, token, keyAuth string) error {
defer n.close()
Owner

This defer looks a bit strange. Defers are usually placed where resource is created to not forget to remove it.

n.dial()
defer n.close()

Here is the different case. I think we can simply call n.close() at the end of this function.

Also, is it correct that after CleanUp there will be no Present() invocations?

This `defer` looks a bit strange. Defers are usually placed where resource is created to not forget to remove it. ``` n.dial() defer n.close() ``` Here is the different case. I think we can simply call `n.close()` at the end of this function. Also, is it correct that _after_ `CleanUp` there will be no `Present()` invocations?
Author
Member

In case of using CLI - yes. If we call the challenge several times from the code - this solution with closing in CleanUp isn't suitable

In case of using CLI - yes. If we call the challenge several times from the code - this solution with closing in `CleanUp` isn't suitable
alexvanin marked this conversation as resolved
dkirillov reviewed 2023-08-01 06:39:41 +00:00
@ -127,0 +136,4 @@
case !ctx.IsSet("wallet"):
log.Fatal("No wallet file provided for nns challenge.")
case !ctx.IsSet("wallet.account-address"):
log.Fatal("No address of account from wallet file provided.")
Member

It seems we can use default address if this flag wallet.account-address was not provided

It seems we can use default address if this flag `wallet.account-address` was not provided
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-08-01 06:47:03 +00:00
@ -0,0 +64,4 @@
}
// Generate the Key Authorization for the challenge
keyAuth, err := c.core.GetKeyAuthorization(chlng.Token)
Member

It seems this code is the same for PreSolve, Solve``, CleanUp` methods. Can we extract it to separate private method and to reuse?

	domain := challenge.GetTargetedDomain(authz)

	chlng, err := challenge.FindChallenge(challenge.NNS01, authz)
	if err != nil {
		return err
	}

	// Generate the Key Authorization for the challenge
	keyAuth, err := c.core.GetKeyAuthorization(chlng.Token)
        if err != nil {
		return err
	}
It seems this code is the same for `PreSolve`, `Solve``, `CleanUp` methods. Can we extract it to separate private method and to reuse? ```golang domain := challenge.GetTargetedDomain(authz) chlng, err := challenge.FindChallenge(challenge.NNS01, authz) if err != nil { return err } // Generate the Key Authorization for the challenge keyAuth, err := c.core.GetKeyAuthorization(chlng.Token) if err != nil { return err } ```
dkirillov marked this conversation as resolved
alexvanin requested review from dkirillov 2023-08-01 06:53:42 +00:00
mbiryukova force-pushed feature/add_nns_challenge from e07e209879 to 0a1ad0970c 2023-08-02 10:09:46 +00:00 Compare
mbiryukova force-pushed feature/add_nns_challenge from 0a1ad0970c to a49270bb2e 2023-08-02 10:16:06 +00:00 Compare
dkirillov reviewed 2023-08-04 14:55:28 +00:00
@ -0,0 +17,4 @@
type ChallengeOption func(*Challenge) error
// CondOption Conditional challenge option.
func CondOption(condition bool, opt ChallengeOption) ChallengeOption {
Member

Currently, we have no option so let's drop this CondOption

Currently, we have no option so let's drop this `CondOption`
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-08-04 14:55:32 +00:00
dkirillov reviewed 2023-08-04 15:01:48 +00:00
@ -0,0 +50,4 @@
}
}
acc := w.GetAccount(address)
acc.Decrypt(accountPassword, w.Scrypt)
Member

Why don't we check error here?

Why don't we check error here?
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-08-04 15:10:40 +00:00
@ -0,0 +100,4 @@
// Present creates a TXT record using the specified parameters to fulfill the nns-01 challenge.
// It implements Provider interface in order to use NNSProvider as Provider.
func (n *NNSProvider) Present(domain, token, keyAuth string) error {
Member

token isn't used. So let's write Present(domain, _, keyAuth string)
The same for CleanUp

`token` isn't used. So let's write ` Present(domain, _, keyAuth string)` The same for `CleanUp`
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-08-04 15:18:16 +00:00
@ -0,0 +101,4 @@
// Present creates a TXT record using the specified parameters to fulfill the nns-01 challenge.
// It implements Provider interface in order to use NNSProvider as Provider.
func (n *NNSProvider) Present(domain, token, keyAuth string) error {
n.dial()
Member

Why don't we check error?

Why don't we check error?
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-08-04 15:34:37 +00:00
@ -0,0 +95,4 @@
// Close closes connections of multiSchemeClient.
func (n *NNSProvider) close() {
n.client.Close()
n.client = nil
Member

We do we set n.client to nil?

We do we set `n.client` to `nil`?
dkirillov marked this conversation as resolved
mbiryukova force-pushed feature/add_nns_challenge from a49270bb2e to f1c224c819 2023-08-07 08:58:56 +00:00 Compare
dkirillov reviewed 2023-08-07 11:40:13 +00:00
@ -0,0 +105,4 @@
func (n *NNSProvider) Present(domain, _, keyAuth string) error {
err := n.dial()
if err != nil {
n.client.Close()
Member

Actually, here panic can happen (if n.client is nil), when this is the first invocation and we couldn't create client in dial.

By the way, why don't we init client once (in NewNNSProvider) and make NNSProvider.close to be exported (so that client code that will use NNSProvided close connection explicitly). I don't know how often client code will invoke Present method, so I don't insist on this changes

Actually, here `panic` can happen (if `n.client` is `nil`), when this is the first invocation and we couldn't create client in `dial`. By the way, why don't we init client once (in `NewNNSProvider`) and make `NNSProvider.close` to be exported (so that client code that will use `NNSProvided` close connection explicitly). I don't know how often client code will invoke `Present` method, so I don't insist on this changes
Author
Member

Removed this line because CleanUp with close will be called.
If we will use lego as a library, Present can be called several times as well as CleanUp, and we can init client in NewNNSProvider and close it explicitly. But if we use lego as CLI there is no place where to close client except CleanUp. So I think the most logical way is to init client in Present and close in CleanUp

Removed this line because `CleanUp` with close will be called. If we will use lego as a library, `Present` can be called several times as well as `CleanUp`, and we can init client in `NewNNSProvider` and close it explicitly. But if we use lego as CLI there is no place where to close client except `CleanUp`. So I think the most logical way is to init client in `Present` and close in `CleanUp`
dkirillov marked this conversation as resolved
mbiryukova force-pushed feature/add_nns_challenge from f1c224c819 to 79c5b83559 2023-08-07 13:17:12 +00:00 Compare
dkirillov approved these changes 2023-08-08 07:20:17 +00:00
pogpp approved these changes 2023-08-08 11:58:54 +00:00
pogpp merged commit 79c5b83559 into tcl/master 2023-08-08 11:59:52 +00:00
pogpp referenced this pull request from a commit 2023-08-08 11:59:54 +00:00
Sign in to join this conversation.
No description provided.