Network: re-validate peers after denylist update #2238
Conversation
|
|
||
| // err status is set on connection. Due to EOF it's an unknown error | ||
| assert.Equal(t, codes.Unknown, connection.status.Load().Code()) | ||
| // Last received message is dropped and no status is set. Default value is OK. |
There was a problem hiding this comment.
This is a bit weird, status is now the same as being operational. Shouldn't it be a disconnected status somehow?
There was a problem hiding this comment.
I don't know what the right solution is here.
connection.status is a grpc Status that reflects the state of the last message received. (it is a bit of a hack for the backoff) no-status is considered equal to OK (this situation I think)
When the connection's context is cancelled it means that the receiving side closed the connection. conn.startReceiving ignores any future message (including any possible errors) in this case and the status of the last processed incoming message was still ok.
Also, Unauthenticated is the only status we act on and does not apply here, all other statuses are handled the same as the current situation.
There was a problem hiding this comment.
This test is weird anyway. It supposedly tests conn.startSending, but the only thing it tests is error handling of conn.startReceiving after disconnect with a stub stream that simulates stream closure with an EOF message
network/transport/grpc/connection.go
Outdated
| case <-mc.ctx.Done(): | ||
| // disconnect: drop message and stop receiving | ||
| return | ||
| default: |
There was a problem hiding this comment.
interesting pattern, but note that RecvMsg is blocking anyway, so you could just check ctx.Err() != nil?
There was a problem hiding this comment.
changed to check error
| s.connections.forEach(func(conn Connection) { | ||
| peerCert := conn.Peer().Certificate | ||
| if nowFunc().After(peerCert.NotAfter) { | ||
| log.Logger().WithError(errors.New("certificate expired while in use")).WithFields(conn.Peer().ToFields()).Info("Disconnected peer") |
There was a problem hiding this comment.
why check this? This will be checked by the Golang's TLS when setting up the connection anyway?
There was a problem hiding this comment.
because we're re-validating the peer's certificate on a connection that may last for a month? The only things that could have changed since it was first accepted is that it is banned or expired.
|
|
||
| // Notify all subscribers synchronously | ||
| for _, sub := range b.subscribers { | ||
| sub() |
There was a problem hiding this comment.
can conn.disconnect() block when the other side is a slow responder (closing TCP involves sending packets to thw other side)? Since subscribers are called synchronously?
There was a problem hiding this comment.
conn.disconnect() does not depend on the peer, so no
|
|
||
| // SubscribeDenied registers a callback that is triggered everytime the denylist is updated. | ||
| // This can be used to revalidate all certificates on long-lasting connections by calling Validate on them again. | ||
| SubscribeDenied(f func()) |
There was a problem hiding this comment.
You only subscribe to updates on the denylist, so I wanted to have something indicating that in the name.
gerardsn
deleted the
network/periodically-validate-connection-certificate
branch
closes #2018 alternative solution to #2235
DenyList.Subscribers get pinged when the Denylist is updated (revalidation is not performed when Denylist is not configured). What to do with this information is determined by the subscriber. The connection manager verifies the certificate of all peers and disconnects those that are no longer valid.
diagnostics also show the last time the certificate was updated, which is zero if the denylist is not used