Admin interface for node operators #1222

synzhu · 2021-08-28T02:28:15Z

closes https://github.com/dapperlabs/flow-go/issues/5818

TODO

Write tests
Document the protobuf file
Provide a rest interface using grpc Gateway

codecov-commenter · 2021-08-28T02:44:36Z

Codecov Report

Merging #1222 (4998a6a) into master (a783037) will increase coverage by 0.10%.
The diff coverage is 76.51%.

@@            Coverage Diff             @@
##           master    #1222      +/-   ##
==========================================
+ Coverage   54.70%   54.81%   +0.10%     
==========================================
  Files         502      504       +2     
  Lines       31769    31918     +149     
==========================================
+ Hits        17380    17496     +116     
- Misses      12026    12048      +22     
- Partials     2363     2374      +11

Flag	Coverage Δ
unittests	`54.81% <76.51%> (+0.10%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
admin/server.go	`50.00% <50.00%> (ø)`
admin/command_runner.go	`79.69% <79.69%> (ø)`
engine/collection/synchronization/engine.go	`62.90% <0.00%> (ø)`
...ngine/common/synchronization/finalized_snapshot.go	`72.91% <0.00%> (+4.16%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a783037...4998a6a. Read the comment docs.

huitseeker

Thanks for getting started on the admin interface! Besides the points I made inline:

Authentication:
I would note we already have tools in utils/grpcutils/grpc.go for server-side uthentication: creating a gRPC server with a self-signed TLS certificate and a client that checks against the corresponding (pre-shared) PubKey. It would only require minor changes for the server to check the client against a certificate that the gRPC server would have been started with. Not that this is enough - I'd expect this to eventually require token auth - but since it's there ...
Security:
In order to augment the predictability of this interface - incidentally limitiing opportunities for mischief: would it be possible to have RegisterHandler|RegisterValidator be part of the API of a CommandRunnerBootstrapper, rather than part of the runtime?

huitseeker · 2021-09-13T14:34:25Z

cmd/scaffold.go

@@ -722,6 +743,10 @@ func (fnb *FlowNodeBuilder) Initialize() NodeBuilder {
 		fnb.RegisterBadgerMetrics()
 	}

+	if fnb.adminAddr != NotSet {


what about adminAddr at NotSet but with a specified adminHttpAddr?

In this case we would just ignore the adminHttpAddr. I can add validation for this although I don't think it's really that necessary?

The adminHttpAddr only makes sense if adminAddr is set. We cannot have an http server if the underlying gRPC service doesn not even exist.

First, I think you can serve GRPC over a Unix domain socket rather than a TCP socket (and in this context, this might actually not even be such a bad idea).

But more to my original point, you can interpret the presence of an adminHttpAddr alone as an authorization to bind to a random port for the GRPC endpoint, especially if you document that it is the contract.

Sure, I guess we can do that.

Actually on second thought, we cannot just choose a random port because we also need to know which interface to bind to.

As I mentioned in the previous comment, I'm not really a fan of limiting the interface to localhost only.

I think we can have a safe default, and that an address parameter that specifies, e.g. just a port, can be interpreted as a default binding to localhost.
With that said, we distribute this through Docker, which won't expose an unexported port, so at this stage it's a nit.

huitseeker · 2021-09-13T14:51:08Z

cmd/node_builder.go

+	adminAddr             string
+	adminHttpAddr         string


How about making these booleans, binding only to localhost? This way we're less vulnerable to inadvertent firewall openings.

But we need some way to specify the port?

That's fine, let's specify just the port then.

I feel like there are very valid reasons why one might want to bind to an interface other than localhost (for example, if they want the admin service to be accessible remotely).

synzhu · 2021-09-13T18:47:20Z

Thanks for getting started on the admin interface! Besides the points I made inline:

Authentication:
I would note we already have tools in utils/grpcutils/grpc.go for server-side uthentication: creating a gRPC server with a self-signed TLS certificate and a client that checks against the corresponding (pre-shared) PubKey. It would only require minor changes for the server to check the client against a certificate that the gRPC server would have been started with. Not that this is enough - I'd expect this to eventually require token auth - but since it's there ...

Security:
In order to augment the predictability of this interface - incidentally limitiing opportunities for mischief: would it be possible to have RegisterHandler|RegisterValidator be part of the API of a CommandRunnerBootstrapper, rather than part of the runtime?

@huitseeker

In my mind, I don't think we really need authentication at the gRPC server level. I think that securing the admin interface should be something that is left up to the node operator. In particular, they should be responsible for ensuring that the address the admin service is configured to run on (via command line arguments) has proper firewall rules setup to restrict incoming traffic on the specified interface / port. For example, one could setup firewall rule to only allow incoming connections to the admin service from a specified bastion host. Then, one could setup SSH rules on the bastion host so that only certain users can access it.
Not entirely sure what you mean here, can you add more details?

huitseeker · 2021-09-14T14:11:06Z

In my mind, I don't think we really need authentication at the gRPC server level. I think that securing the admin interface should be something that is left up to the node operator.

In theory, I agree with you. In practice, assuming our app is always deployed in a context where a vigilant and wise sysadmin has the time to do that securization well is a non-starter, unfortunately. Moreover, the code necessary to perform this "give credential at bootup, check it later when receiving commands", is, as I mentioned, quite close by.

I mean that in the current approach of the admin interface API it would be easy for a distant code component to dynamically add -or worse remove- a handler / validator to the admin interface, possibly under certain conditions. That makes it more complex to reason about whether the admin interface is exploitable, because it opens the possibility that an adversary would try to manipulate those conditions to activate or deactivate the handlers / validators it wishes. On the other hand, if handlers and validators can only be mutated at the start of the node, and not later, that reasoning is much more simple (hence the builder pattern).
Moreover, if we end up really needing dynamic handlers / validators, nothing will prevent us rom re-adding them at that point.

synzhu · 2021-09-14T21:02:32Z

In my mind, I don't think we really need authentication at the gRPC server level. I think that securing the admin interface should be something that is left up to the node operator.

In theory, I agree with you. In practice, assuming our app is always deployed in a context where a vigilant and wise sysadmin has the time to do that securization well is a non-starter, unfortunately. Moreover, the code necessary to perform this "give credential at bootup, check it later when receiving commands", is, as I mentioned, quite close by.

I mean that in the current approach of the admin interface API it would be easy for a distant code component to dynamically add -or worse remove- a handler / validator to the admin interface, possibly under certain conditions. That makes it more complex to reason about whether the admin interface is exploitable, because it opens the possibility that an adversary would try to manipulate those conditions to activate or deactivate the handlers / validators it wishes. On the other hand, if handlers and validators can only be mutated at the start of the node, and not later, that reasoning is much more simple (hence the builder pattern).
Moreover, if we end up really needing dynamic handlers / validators, nothing will prevent us rom re-adding them at that point.

@huitseeker Okay, I agree with 2 and will make that change.

I'll look into 1. I will have to enable mutual authentication right?

(Reference for self: grpc/grpc-go#403)

huitseeker · 2021-09-15T13:57:26Z

I'll look into 1. I will have to enable mutual authentication right?

Essentially, yes. @vishalchangrani is quite familiar with the feature and can probably help.

huitseeker

Very nice work! Thanks a lot!

huitseeker · 2021-09-18T12:26:51Z

admin/command_runner.go

+			}
+
+			if handler := r.getHandler(command.command); handler != nil {
+				// TODO: we can probably merge the command context with the worker context


That's an excellent idea, which is probably worth tracking in an issue, and has links with #1275 (or at least the context-derived variant thereof I suggest in #1308 ).

huitseeker · 2021-09-18T12:36:33Z

cmd/scaffold.go

+	fnb.flags.StringVar(&fnb.BaseConfig.adminCert, "admin-cert", defaultConfig.adminCert, "admin cert file (for TLS)")
+	fnb.flags.StringVar(&fnb.BaseConfig.adminKey, "admin-key", defaultConfig.adminKey, "admin key file (for TLS)")
+	fnb.flags.StringVar(&fnb.BaseConfig.adminClientCAs, "admin-client-certs", defaultConfig.adminClientCAs, "admin client certs (for mutual TLS)")


We can probably open an issue for the convenience of auto-generating this certificate pair (the generation would happen at boot, for a later use).
Auto-generation may fit the needs of e.g. a partner running a single node, whereas we'd be more interested in passing our own cert, to manage a group of nodes with the same creds.

huitseeker · 2021-09-18T12:38:01Z

cmd/scaffold.go

@@ -722,6 +743,10 @@ func (fnb *FlowNodeBuilder) Initialize() NodeBuilder {
 		fnb.RegisterBadgerMetrics()
 	}

+	if fnb.adminAddr != NotSet {


I think we can have a safe default, and that an address parameter that specifies, e.g. just a port, can be interpreted as a default binding to localhost.
With that said, we distribute this through Docker, which won't expose an unexported port, so at this stage it's a nit.

tools.go

admin/command_runner.go

admin/server.go

vishalchangrani

lgtm

peterargue

LGTM. great work

synzhu · 2021-09-21T03:25:28Z

bors merge

1222: Admin interface for node operators r=smnzhu a=smnzhu closes https://github.com/dapperlabs/flow-go/issues/5818 ### TODO - [x] Write tests - [x] Document the protobuf file - [x] Provide a rest interface using grpc Gateway Co-authored-by: Simon Zhu <simon.zsiyan@gmail.com>

bors · 2021-09-21T03:29:15Z

Build failed:

Lint

synzhu · 2021-09-21T03:35:27Z

bors merge

1222: Admin interface for node operators r=smnzhu a=smnzhu closes https://github.com/dapperlabs/flow-go/issues/5818 ### TODO - [x] Write tests - [x] Document the protobuf file - [x] Provide a rest interface using grpc Gateway Co-authored-by: Simon Zhu <simon.zsiyan@gmail.com>

bors · 2021-09-21T04:02:16Z

Build failed:

Unit Tests (1.16)

synzhu · 2021-09-21T05:57:16Z

bors retry

bors · 2021-09-21T06:14:52Z

Build succeeded:

synzhu added 9 commits August 12, 2021 00:35

Update ci.yml

cb1a1a8

Merge branch 'master' of https://github.com/onflow/flow-go

826cad7

Merge branch 'master' of https://github.com/onflow/flow-go

0fa0a96

Merge branch 'master' of https://github.com/onflow/flow-go

b548f29

Merge branch 'master' of https://github.com/onflow/flow-go

0f75779

define command runner

2273886

Update command_runner.go

1fbbd1b

Cleanup code

5c565a8

implement Ready and Done

14dac3d

synzhu requested review from huitseeker and vishalchangrani August 28, 2021 02:28

synzhu added 3 commits August 27, 2021 19:33

Merge branch 'master' into smnzhu/access-node-controls

e95eed1

add comments

fff622c

Update ci.yml

aaff701

synzhu added 4 commits September 3, 2021 15:02

commit

866a47c

Update command_runner_test.go

dc30d98

Add tests

e6fa7ae

comment protobug

b5fa500

synzhu marked this pull request as ready for review September 8, 2021 00:28

synzhu requested a review from yhassanzadeh13 September 9, 2021 15:25

synzhu added 3 commits September 9, 2021 21:49

Add http server and tests

7cf3791

Update command_runner.go

9a5c974

scaffold changes

fd42507

huitseeker reviewed Sep 13, 2021

View reviewed changes

synzhu requested a review from huitseeker September 13, 2021 18:47

synzhu added 4 commits September 16, 2021 18:23

Address comments

8dbfd11

Merge branch 'master' into smnzhu/access-node-controls

bcb65e5

go mod tidy

248de33

enable mutual TLS and run grpc on unix socket

a354da6

huitseeker approved these changes Sep 18, 2021

View reviewed changes

vishalchangrani requested a review from peterargue September 20, 2021 19:54

vishalchangrani reviewed Sep 20, 2021

View reviewed changes

tools.go Outdated Show resolved Hide resolved

vishalchangrani reviewed Sep 20, 2021

View reviewed changes

admin/command_runner.go Outdated Show resolved Hide resolved

vishalchangrani reviewed Sep 20, 2021

View reviewed changes

admin/server.go Show resolved Hide resolved

vishalchangrani approved these changes Sep 20, 2021

View reviewed changes

peterargue reviewed Sep 20, 2021

View reviewed changes

This was referenced Sep 20, 2021

[Admin server] merge command context with worker context #1314

Open

[Admin server] auto generate client / server cert pair for mutual TLS #1315

Open

synzhu added 5 commits September 20, 2021 16:15

address comments

ea48203

Merge branch 'master' into smnzhu/access-node-controls

c822455

go mod tidy

61ed0b7

go mod tidy

3384b27

fix lint

60436a0

fix lint

4998a6a

bors bot merged commit b4e29d9 into master Sep 21, 2021

bors bot deleted the smnzhu/access-node-controls branch September 21, 2021 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Admin interface for node operators #1222

Admin interface for node operators #1222

synzhu commented Aug 28, 2021 •

edited

codecov-commenter commented Aug 28, 2021 •

edited

huitseeker left a comment

huitseeker Sep 13, 2021

synzhu Sep 13, 2021

huitseeker Sep 14, 2021

synzhu Sep 14, 2021

synzhu Sep 17, 2021

huitseeker Sep 18, 2021

huitseeker Sep 13, 2021

synzhu Sep 13, 2021

huitseeker Sep 14, 2021

synzhu Sep 17, 2021

synzhu commented Sep 13, 2021

huitseeker commented Sep 14, 2021 •

edited

synzhu commented Sep 14, 2021

huitseeker commented Sep 15, 2021

huitseeker left a comment

huitseeker Sep 18, 2021

synzhu Sep 20, 2021

huitseeker Sep 18, 2021

huitseeker Sep 18, 2021

vishalchangrani left a comment

peterargue left a comment

synzhu commented Sep 21, 2021

bors bot commented Sep 21, 2021

synzhu commented Sep 21, 2021

bors bot commented Sep 21, 2021

synzhu commented Sep 21, 2021

bors bot commented Sep 21, 2021

Admin interface for node operators #1222

Admin interface for node operators #1222

Conversation

synzhu commented Aug 28, 2021 • edited

TODO

codecov-commenter commented Aug 28, 2021 • edited

Codecov Report

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

synzhu commented Sep 13, 2021

huitseeker commented Sep 14, 2021 • edited

synzhu commented Sep 14, 2021

huitseeker commented Sep 15, 2021

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalchangrani left a comment

Choose a reason for hiding this comment

peterargue left a comment

Choose a reason for hiding this comment

synzhu commented Sep 21, 2021

bors bot commented Sep 21, 2021

synzhu commented Sep 21, 2021

bors bot commented Sep 21, 2021

synzhu commented Sep 21, 2021

bors bot commented Sep 21, 2021

synzhu commented Aug 28, 2021 •

edited

codecov-commenter commented Aug 28, 2021 •

edited

huitseeker commented Sep 14, 2021 •

edited