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.

Sign up for GitHub

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

[FAB-17675] Prevent gossip probe from registering as a connection #925

Merged
merged 1 commit into from
Mar 27, 2020
+153 −129
Merged

[FAB-17675] Prevent gossip probe from registering as a connection #925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 21 additions & 6 deletions gossip/comm/comm_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ const (
DefSendBuffSize = 20
)

var (
errProbe = errors.New("probe")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have a more descriptive error message for better readability?

Correct me if I have misunderstood: this error would be returned when the peer tries to authenticate a stream which is not a probe (by passing an appropriate flag) but it ends up being a probe. As a result, we don't store info in the connection store. If my understanding is correct, I am not sure whether it should be an error. Maybe a bool can be used instead as I mentioned in the other comment.

Copy link
Contributor Author

@yacovm yacovm Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You misunderstood. This error just means that the remote peer probed us, so there is no point in doing anything with the returned first value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. That's what I meant may be my wordings didn't convey that. That's why I suggested isProbe bool later in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see a function that returns an object and an error in the following way:
If the error is nil, use the first return value.
Else, use the other return value.

This perfectly fits my needs here.

)

// SecurityAdvisor defines an external auxiliary object
// that provides security and identity related capabilities
type SecurityAdvisor interface {
Expand Down Expand Up @@ -163,7 +167,7 @@ func (c *commImpl) createConnection(endpoint string, expectedPKIID common.PKIidT

ctx, cancel = context.WithCancel(context.Background())
if stream, err = cl.GossipStream(ctx); err == nil {
connInfo, err = c.authenticateRemotePeer(stream, true)
connInfo, err = c.authenticateRemotePeer(stream, true, false)
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this CR. Can be err != nil and return early to avoid the extra indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is by design.
I need to close the gRPC connection, and also to cancel the gRPC stream.
So it's not just an early return. I want cleanup operations to be at the bottom of the function so that I manage them at the same place.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Got it.

pkiID = connInfo.ID
// PKIID is nil when we don't know the remote PKI id's
Expand Down Expand Up @@ -305,7 +309,7 @@ func (c *commImpl) Handshake(remotePeer *RemotePeer) (api.PeerIdentityType, erro
if err != nil {
return nil, err
}
connInfo, err := c.authenticateRemotePeer(stream, true)
connInfo, err := c.authenticateRemotePeer(stream, true, true)
if err != nil {
c.logger.Warningf("Authentication failed: %v", err)
return nil, err
Expand Down Expand Up @@ -404,7 +408,7 @@ func extractRemoteAddress(stream stream) string {
return remoteAddress
}

func (c *commImpl) authenticateRemotePeer(stream stream, initiator bool) (*protoext.ConnectionInfo, error) {
func (c *commImpl) authenticateRemotePeer(stream stream, initiator, isProbe bool) (*protoext.ConnectionInfo, error) {
ctx := stream.Context()
remoteAddress := extractRemoteAddress(stream)
remoteCertHash := extractCertificateHashFromContext(ctx)
Expand All @@ -431,7 +435,7 @@ func (c *commImpl) authenticateRemotePeer(stream stream, initiator bool) (*proto
return nil, fmt.Errorf("No TLS certificate")
}

cMsg, err = c.createConnectionMsg(c.PKIID, selfCertHash, c.peerIdentity, signer)
cMsg, err = c.createConnectionMsg(c.PKIID, selfCertHash, c.peerIdentity, signer, isProbe)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -492,6 +496,10 @@ func (c *commImpl) authenticateRemotePeer(stream stream, initiator bool) (*proto

c.logger.Debug("Authenticated", remoteAddress)

if receivedMsg.Probe {
return connInfo, errProbe
}

return connInfo, nil
}

Expand Down Expand Up @@ -556,7 +564,13 @@ func (c *commImpl) GossipStream(stream proto.Gossip_GossipStreamServer) error {
if c.isStopping() {
return fmt.Errorf("Shutting down")
}
connInfo, err := c.authenticateRemotePeer(stream, false)
connInfo, err := c.authenticateRemotePeer(stream, false, false)

if err == errProbe {
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be confusing to the code reader. Some questions might arise like why we are ignoring an error. If we are trying to not store info in the conn store for probs, can't the return value be an isProbe bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, 3 return values is ugly, and this is an un-exported function.
It's perfectly fine to have a specific error that indicates something.

Look at:

if e, ok := err.(*QueryError); ok && e.Err == ErrPermission {
// query failed because of a permission problem
}

From https://blog.golang.org/go1.13-errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification. Agree that 3 return value is ugly.

c.logger.Infof("Peer %s (%s) probed us", connInfo.ID, connInfo.Endpoint)
return nil
}

if err != nil {
c.logger.Errorf("Authentication failed: %v", err)
return err
Expand Down Expand Up @@ -618,7 +632,7 @@ func readWithTimeout(stream stream, timeout time.Duration, address string) (*pro
}
}

func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte, cert api.PeerIdentityType, signer protoext.Signer) (*protoext.SignedGossipMessage, error) {
func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte, cert api.PeerIdentityType, signer protoext.Signer, isProbe bool) (*protoext.SignedGossipMessage, error) {
m := &proto.GossipMessage{
Tag: proto.GossipMessage_EMPTY,
Nonce: 0,
Expand All @@ -627,6 +641,7 @@ func (c *commImpl) createConnectionMsg(pkiID common.PKIidType, certHash []byte,
TlsCertHash: certHash,
Identity: cert,
PkiId: pkiID,
Probe: isProbe,
},
},
}
Expand Down
4 changes: 2 additions & 2 deletions gossip/comm/comm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ func handshaker(port int, endpoint string, comm Comm, t *testing.T, connMutator
mac := hmac.New(sha256.New, hmacKey)
mac.Write(msg)
return mac.Sum(nil), nil
})
}, false)
// Mutate connection message to test negative paths
msg = connMutator(msg)
// Send your own connection message
Expand Down Expand Up @@ -555,7 +555,7 @@ func TestCloseConn(t *testing.T) {
mac := hmac.New(sha256.New, hmacKey)
mac.Write(msg)
return mac.Sum(nil), nil
})
}, false)
assert.NoError(t, stream.Send(connMsg.Envelope))
stream.Send(createGossipMsg().Envelope)
select {
Expand Down
Loading