workloadattestor systemd: dbus use of closed network connection #4360
Conversation
rkaippully
requested review from
evan2645,
amartinezfayo,
azdagron,
MarcosDY and
rturner3
as code owners
Fixes spiffe#4315 The connection to system bus is shared and should not be closed after use in order to avoid errors on concurrent usage. It is typical to share the connection in the same process as per this explanation: godbus/dbus#179 (comment) In case of errors, this shared connection will detect that and attempt to reconnect: https://github.com/godbus/dbus/blob/v5.1.0/conn.go#L124. Signed-off-by: Raghu Kaippully <rkaippully@gmail.com>
rkaippully
force-pushed
the
raghu/fix-4315
branch
from
f8fee6a
to
5d6a33f
Compare
| conn, err := dbus.SystemBus() | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, "failed to open dbus connection: %v", err) | ||
| } | ||
| defer conn.Close() |
There was a problem hiding this comment.
If there is no error, when would be closed the connection?
I see that the connection is closed when the context is done, but the context used would be a new context and not the one from this function.
There was a problem hiding this comment.
Since it is a background context which is never cancelled, it looks like the connection is never closed. Or did I get that wrong?
There was a problem hiding this comment.
Right, I don't think that the connection will be closed, which is a problem.
We will need to avoid using the shared dbus connection, since it doesn't seem that there is a clear way to properly handle the closure of connections when there are concurrent requests.
I think that the right solution here will involve maintaining a connection at the plugin level.
There was a problem hiding this comment.
Do you mean a shared connection at the plugin level that's never closed (unless there is an error)? It's slightly better because it'll protect the plugin from other code inadvertently closing the connection.
There was a problem hiding this comment.
Yes, I mean having a single connection at the plugin level that's closed when the plugin closes. The same connection can be used by the different attestation calls.
There was a problem hiding this comment.
Thank you, @amartinezfayo! I have changed this to use a plugin level shared connection. I could not find the lifecycle mechanism to close the connection on plugin close (probably because I am fairly new to spire code base and go). What am I missing?
There was a problem hiding this comment.
When plugins implement the io.Closer interface, the Close function is invoked before the plugin is unloaded. Since this is documented in the SPIRE Plugin SDK, it may be difficult to find it :)
You just need to add a function func (p *Plugin) Close() error {...} that closes the connection, and it will be automatically called.
...instead of a global shared one. This ensures that the connection is never closed by any other library while this plugin is using it. Signed-off-by: Raghu Kaippully <rkaippully@gmail.com>
Signed-off-by: Raghu Kaippully <rkaippully@gmail.com>
There was a problem hiding this comment.
Thank you @rkaippully for the changes.
When plugins implement the io.Closer interface, the Close function is invoked before the plugin is unloaded. You just need to add a function func (p *Plugin) Close() error {...} that closes the connection, and it will be automatically called.
| @@ -67,12 +71,29 @@ func (p *Plugin) Attest(ctx context.Context, req *workloadattestorv1.AttestReque | |||
| }, nil | |||
| } | |||
|
|
|||
| func getSystemdUnitInfo(ctx context.Context, pid uint) (*DBusUnitInfo, error) { | |||
| conn, err := dbus.SystemBus() | |||
| func (p *Plugin) DBusConn() (*dbus.Conn, error) { | |||
There was a problem hiding this comment.
This function does not really need to be exported. We may call it getDBusConn.
| } | ||
|
|
||
| func getSystemdUnitInfo(ctx context.Context, p *Plugin, pid uint) (*DBusUnitInfo, error) { | ||
| // Do not close this connection because it is shared and will autoclose on errors |
There was a problem hiding this comment.
I'm not sure if this comment is accurate. I would probably mention that the connection is closed when the plugin is unloaded in SPIRE. Something along the lines of:
| // Do not close this connection because it is shared and will autoclose on errors | |
| // We are not closing the connection here because it's closed when the Close() function is called as part of unloading the plugin. |
- Implement `Close()` on the plugin to close the DBus connection - Do not export `DBusConn` method - Fix a comment Signed-off-by: Raghu Kaippully <rkaippully@gmail.com>
The connection to system bus is shared and should not be closed after use in order to avoid errors on concurrent usage.
It is typical to share the connection in the same process as per this explanation:
godbus/dbus#179 (comment)
Pull Request check list
Affected functionality
Bug fix on systemd workload attestor
Description of change
Uses a plugin level dbus connection shared by all attestation requests that is not closed after every attestation. This will prevent errors when a concurrent request closes a connection being used by another request.
Which issue this PR fixes
Fixes #4315