Integrate secure GRPC client to DKG and QC clients #1224

kc1116 · 2021-08-28T19:05:29Z

This PR integrates secure access client into collection/consensus node

- removes ability for LN and SN to be run without machine account config - adds new flags --insecure-access-api (allows GRPC connection to access node to be insecure) & --access-node-grpc-public-key (networking public key of the access node being connected to) - adds func to get GRPC dial options with TLS config to be used with the flow-client

codecov-commenter · 2021-08-28T20:34:31Z

Codecov Report

Merging #1224 (ad89c53) into master (b322b29) will decrease coverage by 0.07%.
The diff coverage is 12.90%.

@@            Coverage Diff             @@
##           master    #1224      +/-   ##
==========================================
- Coverage   56.16%   56.08%   -0.08%     
==========================================
  Files         484      485       +1     
  Lines       29807    29870      +63     
==========================================
+ Hits        16742    16754      +12     
- Misses      10782    10831      +49     
- Partials     2283     2285       +2

Flag	Coverage Δ
unittests	`56.08% <12.90%> (-0.08%)`	⬇️

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

Impacted Files	Coverage Δ
engine/access/rpc/backend/connection_factory.go	`75.51% <ø> (ø)`
engine/execution/rpc/engine.go	`46.37% <ø> (ø)`
module/ingress/ingress.go	`13.95% <ø> (ø)`
utils/grpcutils/grpc.go	`51.85% <ø> (ø)`
cmd/collection/main.go	`2.85% <3.84%> (-0.48%)`	⬇️
cmd/consensus/main.go	`3.34% <3.84%> (-0.35%)`	⬇️
utils/grpcutils/grpc_client.go	`60.00% <60.00%> (ø)`
cmd/bootstrap/cmd/machine_account_key.go	`61.90% <0.00%> (-6.52%)`	⬇️
fvm/script.go	`89.74% <0.00%> (-4.86%)`	⬇️
fvm/fvm.go	`82.05% <0.00%> (-4.44%)`	⬇️
... and 8 more

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 b322b29...ad89c53. Read the comment docs.

- adds additional flags for secure GRPC conf during networking bootstrapping - updates integration tests to now use secured GRPC conn

- access node needed for secure GRPC conn

jordanschalm · 2021-08-30T21:30:18Z

cmd/collection/main.go

+		accessAddress       string
+		securedAccessAddress string


I feel like it is simpler to only have one access address flag, since we will only ever connect to one of them based on the --insecure-access-api flag.

Suggested change

accessAddress string

securedAccessAddress string

accessAPIAddress string

jordanschalm · 2021-08-30T21:32:13Z

cmd/collection/main.go

+			err = flowClient.Ping(context.Background())
+			if err != nil {
+				return nil, fmt.Errorf("failed to ping, flow client may be misconfigured check GRPC options %w", err)
+			}


JFYI this will be replaced by https://github.com/dapperlabs/flow-go/issues/5792, which will check for connectivity as well as correct configuration.

jordanschalm · 2021-08-30T21:34:31Z

integration/localnet/bootstrap.go

 	for i, c := range containers {
 		fmt.Printf("%d: %s", i+1, c.Identity().String())
 		if c.Unstaked {
 			fmt.Printf(" (unstaked)")
 		}
+
+		if c.Role == flow.RoleAccess {


Suggested change

if c.Role == flow.RoleAccess {

if c.Role == flow.RoleAccess && !c.Unstaked {

I don't think we want to use unstaked ANs for this

jordanschalm · 2021-08-30T21:47:27Z

integration/localnet/bootstrap.go

+		fmt.Sprintf("--insecure-access-api=false"),
+		fmt.Sprintf("--secured-access-address=%s", securedAccessAddress),
+		fmt.Sprintf("--access-node-grpc-public-key=%s", accessNodeGRPCPubKey),
 	)

+	// IMPORTANT: additional flags will contain correct flags for secure GRPC conn
+	service.Command = append(service.Command, container.AdditionalFlags...)
+


If we are going to make use of the AdditionalFlags (which seems like a good idea to me), why do we also need to specify the flag values on line 411-413 as well?

What is in AdditionalFlags which is not on lines 411-413, and can we just put that in AdditionalFlags to avoid needing to pass in the new arguments?

jordanschalm · 2021-08-30T21:52:54Z

cmd/collection/main.go

 			// construct QC contract client
-			qcContractClient, err := createQCContractClient(node, accessAddress)
+			qcContractClient, err := createQCContractClient(node, accessAddress, flowClient)


⚠️ Another reason to not have 2 flags for the access address. Here we are checking the validity w.r.t. the unsecured access address, regardless of whether we will be connecting to it.

jordanschalm · 2021-08-30T21:54:19Z

cmd/collection/main.go

@@ -138,6 +143,9 @@ func main() {

 			// epoch qc contract flags
 			flags.StringVar(&accessAddress, "access-address", "", "the address of an access node")
+			flags.StringVar(&securedAccessAddress, "secured-access-address", "", "the address for secured GRPC conn to an access node")
+			flags.StringVar(&accessApiNodePubKey, "access-node-grpc-public-key", "", "the networking public key of the secured access node being connected to")
+			flags.BoolVar(&insecureAccessAPI, "insecure-access-api", true, "required if insecure GRPC connection should be used")


This should default to false. For the upcoming spork, a node operator should not need to specify this flag at all.

Integration tests are not configured properly to handle new flags , can we create separate ticket to make secure API default and update integration tests?

👍 Sure, sounds good

https://github.com/dapperlabs/flow-internal/issues/1574

jordanschalm · 2021-08-30T21:56:20Z

cmd/consensus/main.go

+		accessAddress        string
+		securedAccessAddress string


Same comment here about using one flag

cmd/consensus/main.go

cmd/test/main.go

jordanschalm · 2021-08-30T22:01:48Z

integration/localnet/bootstrap.go

 		fmt.Sprintf("%d:%d", AccessAPIPort+i, RPCPort),
+		fmt.Sprintf("%d:%d", AccessAPIPort+(i+1), SecuredRPCPort),


I think this will overlap ports with >1 AN

Suggested change

fmt.Sprintf("%d:%d", AccessAPIPort+i, RPCPort),

fmt.Sprintf("%d:%d", AccessAPIPort+(i+1), SecuredRPCPort),

fmt.Sprintf("%d:%d", AccessAPIPort+2*i, RPCPort),

fmt.Sprintf("%d:%d", AccessAPIPort+(2*i+1), SecuredRPCPort),

- now LN and SN nodes depend on the AN - only use staked AN for secured GRPC connection - fix port assignment

…into 5793/secured-grpc-client

- --secure-access-node-id will be used to look up networking key of node from state

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

jordanschalm

One final suggestion, otherwise this looks good to me.

Where is this ticket at with the latest updates? Have we tested an epoch transition with these changes on localnet?

jordanschalm · 2021-08-31T15:39:12Z

integration/localnet/bootstrap.go

+			services[container.ContainerName] = prepareConsensusService(
+				container,
+				numConsensus,
+				"access_1:9001",


This address is essentially a constant, I think it would be better to store it with the other constants and use the value directly in preparareConsensusService.

flow-go/integration/localnet/bootstrap.go

Lines 38 to 40 in 1ccefc0

AccessAPIPort = 3569

MetricsPort = 8080

RPCPort = 9000

jordanschalm

🎸

huitseeker

I'm a bit worried this may mix two distinct notions:

do we connect securely to a TLS-ed GRPC server using a pre-shared key to check authentication?
That's the server that's introduced in bringing up a GRPC secure sever for the access node #989
This is about using a capability.
do we connect securely (where securely is defined as above) to our "favorite" access node, which is the one node we trust above others for various reasons ?
That's the new functionality this PR introduces, this is meant to make an identity selection.

I note that the former has a wider range of application than the later: I might want to connect to any, no or every access node that runs the necessary server using GRPC + TLS + pre-shared key. I may also, besides that, restrict the access nodes I rely on ("trust") to any subset of those I connect to securely.

I would welcome:

sunsetting insecure encryption for everything but tests (but Auto Cadence Update: Add container mutation tests for inserting functions into dictionaries #1574 is enough for me),
encryption flags that do not require specifying a NodeID (but that may require a fallback, for a while, to insecure access for nodes that happen to not run the secure server => open an issue to mark it for later?),
and on top of that, an option that clearly states "only pick this access node, it's my favorite" (and imply a secure connection is the only one that can be used)

WDYT?
/cc @vishalchangrani

huitseeker · 2021-08-31T17:35:00Z

cmd/collection/main.go

+				}
+			} else {
+				if secureAccessNodeID == "" {
+					return nil, fmt.Errorf("invalid flag --secure-access-node-id required")


Suggested change

return nil, fmt.Errorf("invalid flag --secure-access-node-id required")

return nil, fmt.Errorf("invalid flag, --secure-access-node-id required")

otherwise the reader may think the invalid qualifier applies to your requirement

jordanschalm · 2021-08-31T18:45:15Z

@huitseeker

encryption flags that do not require specifying a NodeID (but that may require a fallback, for a while, to insecure access for nodes that happen to not run the secure server => open an issue to mark it for later?),

The main issue is that within the protocol state, we know an access node's networking public key and libp2p networking address, but we do not know its GRPC server address (hence the --access-address flag). We could make the assumption that a particular relationship exists between the libp2p networking address and the GRPC server address (for example, it uses the canonical port on the same hostname), but we would need to make sure that assumption is always true. Personally I'm more confident specifying the parameters explicitly.

huitseeker · 2021-08-31T20:00:25Z

T We could make the assumption that a particular relationship exists between the libp2p networking address and the GRPC server address, but we would need to make sure that assumption is always true

That assumption (same host, port 9001) is always true for our access nodes, and since transport authentication against a pre-shared key (itself determined by convention from libp2p identifiers, btw) is a prerequisite to using the connection in any way, there is not risk of misunderstanding on method or identity.

kc1116 · 2021-08-31T20:15:55Z

I'm a bit worried this may mix two distinct notions:

do we connect securely to a TLS-ed GRPC server using a pre-shared key to check authentication?
That's the server that's introduced in bringing up a GRPC secure sever for the access node #989
This is about using a capability.

do we connect securely (where securely is defined as above) to our "favorite" access node, which is the one node we trust above others for various reasons ?
That's the new functionality this PR introduces, this is meant to make an identity selection.

secured implies TLS-ed GRPC connection , to the secure GRPC sever AN bring up
There is no concept of a "favorite AN", this PR does not introduce that concept. Your only required to provide the node ID of the access node you are already connecting to in order to use secure GRPC . Under the hood the node ID is used to find the networking key from protocol state of that AN to create TLS'ed GRPC conn. Using NodeID allows networking keys to change.

I note that the former has a wider range of application than the later: I might want to connect to any, no or every access node that runs the necessary server using GRPC + TLS + pre-shared key. I may also, besides that, restrict the access nodes I rely on ("trust") to any subset of those I connect to securely.

I agree , this is nice to have and is not immediate requirement

I would welcome:

sunsetting insecure encryption for everything but tests (but Auto Cadence Update: Add container mutation tests for inserting functions into dictionaries #1574 is enough for me),

encryption flags that do not require specifying a NodeID (but that may require a fallback, for a while, to insecure access for nodes that happen to not run the secure server => open an issue to mark it for later?),

and on top of that, an option that clearly states "only pick this access node, it's my favorite" (and imply a secure connection is the only one that can be used)

Auto Cadence Update: Add container mutation tests for inserting functions into dictionaries #1574 should handle sunsetting insecure api safely
Encryption flags that do not require specifying a NodeID is not possible. Either you need to provide networking key directly , or node ID to look up networking key. The latter provides us with more flexibility.

WDYT?
/cc @vishalchangrani

@huitseeker

vishalchangrani · 2021-08-31T20:32:32Z

cmd/collection/main.go

+			// create flow client with correct GRPC configuration for QC contract client
+			var flowClient *client.Client
+			if insecureAccessAPI {
+				flowClient, err = common.InsecureFlowClient(accessAddress)


if the grpc client is created here and never re-created, what happens if the access node is restarted and the connection is broken?

I will investigate this .

From this issue grpc/grpc-go#351 it looks like the GRPC client should try to automatically reconnect. Because nothing changed as far as where the client is being created, we should see the same behavior for insecure and secure client connections. This probably needs it's own separate ticket for investigation.

yeah we probably need a factory to be passed in, instead of the actual client since the client will only be used once in an epoch and it is highly probable that the access node gets restarted in-between.
Separate ticket also sounds good.

Should be able to test this by running localnet and restarting the access node before the epoch setup phase starts. I would agree with khalil and would expect that the client would recreate the connection as needed

vishalchangrani · 2021-08-31T20:35:20Z

cmd/collection/main.go

+					return nil, fmt.Errorf("could not find identity of secure access node: %s", secureAccessNodeID)
+				}
+
+				flowClient, err = common.SecureFlowClient(accessAddress, identities[0].NetworkPubKey.String()[2:])


can you throw in a comment for the [2:]

…into 5793/secured-grpc-client

huitseeker · 2021-08-31T21:58:35Z

@kc1116 My comments were definitely meant as non-blocking.

utils/grpc/grpc_client.go

vishalchangrani

lgtm (left some minor nit comments)

jordanschalm · 2021-09-01T01:01:19Z

bors merge

bors · 2021-09-01T01:19:04Z

Build succeeded:

kc1116 added 2 commits August 27, 2021 16:42

add new flags for secure GRPC client to localnet

15103b0

kc1116 requested review from AlexHentschel, jordanschalm and zhangchiqing as code owners August 28, 2021 19:05

kc1116 added 5 commits August 28, 2021 15:09

fix lint

2e6ad43

update unit tests

15d1c1f

Update grpc_client_test.go

49a0aea

Update grpc_test.go

6b8f766

Update grpc_test.go

1f1a4cd

kc1116 added 8 commits August 29, 2021 11:57

update node confs on LN & SN nodes

6001f3c

- adds additional flags for secure GRPC conf during networking bootstrapping - updates integration tests to now use secured GRPC conn

add access node to network setup tests

b9ee709

- access node needed for secure GRPC conn

run CI with insecure GRPC conn

49240ae

use secure GRPC conn by default

bd2ef7c

add access node for secure grpc conn

b02a6d8

check if access node conf exists

d19d601

only update localnet flags , remove new flags from integration tests

a515e8e

script to test secured GRPC ping

b484f4a

jordanschalm reviewed Aug 30, 2021

View reviewed changes

kc1116 and others added 10 commits August 30, 2021 20:58

use 1 access address flag

9d3684b

update order that containers come up

2b36fc5

- now LN and SN nodes depend on the AN - only use staked AN for secured GRPC connection - fix port assignment

add todo regarding ping on init

e7ca387

Merge branch 'master' into 5793/secured-grpc-client

3e0b915

update tests, lint files

0e629a9

Merge branch '5793/secured-grpc-client' of github.com:onflow/flow-go …

31f594c

…into 5793/secured-grpc-client

remove test script

e171ad6

add flag for secure access node id

d49b443

- --secure-access-node-id will be used to look up networking key of node from state

update localnet to use new secure-access-node-id flag

1dc4672

remove access-node-grpc-public-key flag

7542f61

kc1116 requested a review from tarakby August 31, 2021 13:52

kc1116 and others added 4 commits August 31, 2021 10:00

Update main.go

cf8e74f

Update cmd/consensus/main.go

0f2c1ad

Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

set insecure access api to default

3ab6d95

Merge branch 'master' into 5793/secured-grpc-client

f5284ff

jordanschalm reviewed Aug 31, 2021

View reviewed changes

jordanschalm approved these changes Aug 31, 2021

View reviewed changes

huitseeker reviewed Aug 31, 2021

View reviewed changes

kc1116 requested review from huitseeker and jordanschalm August 31, 2021 20:17

vishalchangrani reviewed Aug 31, 2021

View reviewed changes

kc1116 added 2 commits August 31, 2021 16:36

remove unused field

4816b34

Merge branch '5793/secured-grpc-client' of github.com:onflow/flow-go …

27e091e

…into 5793/secured-grpc-client

vishalchangrani reviewed Aug 31, 2021

View reviewed changes

utils/grpc/grpc_client.go Outdated Show resolved Hide resolved

vishalchangrani approved these changes Aug 31, 2021

View reviewed changes

huitseeker approved these changes Sep 1, 2021

View reviewed changes

use same dirname as package name for grpcutils

ad89c53

kc1116 requested review from m4ksio and ramtinms as code owners September 1, 2021 00:58

jordanschalm changed the title ~~5793/secured grpc client~~ Integrate secure GRPC client to DKG and QC clients Sep 1, 2021

bors bot merged commit b75db19 into master Sep 1, 2021

bors bot deleted the 5793/secured-grpc-client branch September 1, 2021 01:19

huitseeker mentioned this pull request Sep 1, 2021

Identity Provider #1133

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate secure GRPC client to DKG and QC clients #1224

Integrate secure GRPC client to DKG and QC clients #1224

kc1116 commented Aug 28, 2021 •

edited

codecov-commenter commented Aug 28, 2021 •

edited

jordanschalm Aug 30, 2021

jordanschalm Aug 30, 2021

jordanschalm Aug 30, 2021

jordanschalm Aug 30, 2021

jordanschalm Aug 30, 2021

jordanschalm Aug 30, 2021

kc1116 Aug 31, 2021

jordanschalm Aug 31, 2021

kc1116 Aug 31, 2021

jordanschalm Aug 30, 2021

jordanschalm Aug 30, 2021

jordanschalm left a comment

jordanschalm Aug 31, 2021

jordanschalm left a comment

huitseeker left a comment •

edited

huitseeker Aug 31, 2021

jordanschalm commented Aug 31, 2021 •

edited

huitseeker commented Aug 31, 2021

kc1116 commented Aug 31, 2021

vishalchangrani Aug 31, 2021

kc1116 Aug 31, 2021

kc1116 Aug 31, 2021

vishalchangrani Aug 31, 2021

jordanschalm Aug 31, 2021

vishalchangrani Aug 31, 2021

huitseeker commented Aug 31, 2021

vishalchangrani left a comment

jordanschalm commented Sep 1, 2021

bors bot commented Sep 1, 2021

	accessAddress string
	securedAccessAddress string
	accessAPIAddress string

	if c.Role == flow.RoleAccess {
	if c.Role == flow.RoleAccess && !c.Unstaked {

		fmt.Sprintf("%d:%d", AccessAPIPort+i, RPCPort),
		fmt.Sprintf("%d:%d", AccessAPIPort+(i+1), SecuredRPCPort),

	return nil, fmt.Errorf("invalid flag --secure-access-node-id required")
	return nil, fmt.Errorf("invalid flag, --secure-access-node-id required")

Integrate secure GRPC client to DKG and QC clients #1224

Integrate secure GRPC client to DKG and QC clients #1224

Conversation

kc1116 commented Aug 28, 2021 • edited

codecov-commenter commented Aug 28, 2021 • edited

Codecov Report

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

jordanschalm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm left a comment

Choose a reason for hiding this comment

huitseeker left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanschalm commented Aug 31, 2021 • edited

huitseeker commented Aug 31, 2021

kc1116 commented Aug 31, 2021

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

huitseeker commented Aug 31, 2021

vishalchangrani left a comment

Choose a reason for hiding this comment

jordanschalm commented Sep 1, 2021

bors bot commented Sep 1, 2021

kc1116 commented Aug 28, 2021 •

edited

codecov-commenter commented Aug 28, 2021 •

edited

huitseeker left a comment •

edited

jordanschalm commented Aug 31, 2021 •

edited