Implement VSock secret notifier.#4565
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an opt-in Linux-only path for exporting newly-added secret mask definitions over vsock (configured via GITHUB_ACTIONS_RUNNER_VSOCK_CID_PORT) so an external receiver (e.g., a host firewall process) can learn which values/patterns should be treated as secrets.
Changes:
- Introduces a
NewSecretAddedevent on the secret masker contract and raises it when regex/value secrets are added. - Hooks the worker’s secret-mask initialization to start a vsock notifier and forward newly-added secrets.
- Adds a new
VSockSecretNotifierservice that connects to a host vsock endpoint and streams length-prefixed JSON payloads.
Show a summary per file
| File | Description |
|---|---|
| src/Sdk/DTLogging/Logging/SecretMasker.cs | Raises a new event when secrets are added so external components can be notified. |
| src/Sdk/DTLogging/Logging/ISecretMasker.cs | Extends the secret masker API with a notification event and event-args types for serialization. |
| src/Runner.Worker/Worker.cs | Starts the vsock notifier on Linux and wires secret-add notifications during job initialization. |
| src/Runner.Common/VSockSecretNotifier.cs | Implements the vsock client + background send loop for secret notification payloads. |
Review details
Comments suppressed due to low confidence (2)
src/Runner.Common/VSockSecretNotifier.cs:84
- Since the background task isn’t otherwise observed, assigning it to an unused field will fail the build (unused field warning). Prefer starting it with a discard and rely on the method’s internal exception handling/logging.
_secretNotificationTask = ProcessSecretChannel();
src/Runner.Common/VSockSecretNotifier.cs:168
Serialize()constructs theSocketAddresswithAddressFamily.Unspecified. TheSocketAddressshould be created with the endpoint’s actual address family (vsock) so the socket stack sees the correct family without relying on manually written bytes.
SocketAddress socketAddress = new SocketAddress(AddressFamily.Unspecified, SocketAddressSize);
- Files reviewed: 4/4 changed files
- Comments generated: 6
- Review effort level: Low
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
src/Sdk/DTLogging/Logging/ISecretMasker.cs:17
- Adding
NewSecretAddedto the existing publicISecretMaskerinterface is a source/binary breaking change for any external consumers that implement this interface (they must add the event to compile). If this notification mechanism is intended to be opt-in/internal, consider moving the event off the existing interface (e.g., expose it only onSecretMasker, or introduce a new derived interface likeISecretMaskerWithNotifications).
public interface ISecretMasker
{
void AddRegex(String pattern);
void AddValue(String value);
void AddValueEncoder(ValueEncoder encoder);
ISecretMasker Clone();
String MaskSecrets(String input);
public event EventHandler<NewSecretEventArgs> NewSecretAdded;
}
src/Runner.Common/VSockSecretNotifier.cs:164
ProcessSecretChannelwill commonly observe cancellation during normal shutdown (e.g., whenDisposeAsynccancels the token). Currently this is logged as an error via the broadcatch (Exception), which can generate noisy error logs on expected shutdown paths. Consider handlingOperationCanceledExceptionseparately when cancellation was requested.
catch (Exception ex)
{
Trace.Error($"Failed to process secret channel: {ex}");
}
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Low
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (4)
src/Runner.Common/VSockSecretNotifier.cs:190
- HostVsockEndPoint.Serialize() constructs the SocketAddress with AddressFamily.Unspecified, but this SocketAddress’s AddressFamily is used by Socket APIs to validate/route the endpoint. Returning an Unspecified-family SocketAddress can cause ConnectAsync to fail even though the byte payload is patched. Use the endpoint’s AddressFamily when constructing the SocketAddress.
public override SocketAddress Serialize()
{
SocketAddress socketAddress = new SocketAddress(AddressFamily.Unspecified, SocketAddressSize);
// sockaddr_vm layout: family(0-1), reserved1(2-3), port(4-7), cid(8-11)
src/Runner.Common/VSockSecretNotifier.cs:105
- _channel.Writer.TryWrite can return false when the writer has been completed (e.g., after cancellation or an exception in ProcessSecretChannel). The current comment says an unbounded channel always accepts items, which isn’t true in that state, and silently dropping notifications can make debugging harder.
// we don't need to check return since unbounded channel will always accept the item.
_channel.Writer.TryWrite(fullPayload);
src/Sdk/DTLogging/Logging/ISecretMasker.cs:16
- Adding NewSecretAdded to the public ISecretMasker interface is a breaking change for any downstream code that implements ISecretMasker (they must add the new event to compile). If this is intended to be internal/opt-in, consider introducing a derived interface (e.g., ISecretMaskerWithNotifications) or exposing notifications via the concrete SecretMasker type (and casting) instead of changing the existing contract.
void AddRegex(String pattern);
void AddValue(String value);
void AddValueEncoder(ValueEncoder encoder);
ISecretMasker Clone();
String MaskSecrets(String input);
public event EventHandler<NewSecretEventArgs> NewSecretAdded;
src/Runner.Worker/Worker.cs:96
- The new VSock secret-notification path (TryStartNotifierAsync + NewSecretAdded subscription + NotifyNewSecret) isn’t covered by the existing Worker L0 tests. This is testable by configuring IVSockSecretNotifier.TryStartNotifierAsync to return true, adding at least one secret variable/mask hint in the job message, and verifying NotifyNewSecret is called.
// Initialize the secret masker and set the thread culture.
if (Constants.Runner.Platform == Constants.OSPlatform.Linux &&
await secretNotifier.TryStartNotifierAsync())
{
HostContext.SecretMasker.NewSecretAdded += (sender, e) =>
{
secretNotifier.NotifyNewSecret(e);
};
}
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Low
steiza
left a comment
There was a problem hiding this comment.
One minor comment, otherwise LGTM!
We'll have to adapt the Actions network firewall to understand this new message format, but it will safely ignore messages that it does not understand.
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (3)
src/Runner.Common/VSockSecretNotifier.cs:166
ProcessSecretChannellogs an error for all exceptions, includingOperationCanceledExceptiontriggered by normal shutdown (Dispose callsCancel()). That will produce noisy/incorrect error logs during expected disposal. Handle cancellation separately and treat it as a normal exit path.
catch (Exception ex)
{
Trace.Error($"Failed to process secret channel: {ex}");
}
src/Sdk/DTLogging/Logging/ISecretMasker.cs:16
- Adding
NewSecretAddedtoISecretMaskeris a binary/compile-time breaking change for any downstream implementations of this public interface (they must now implement the event). If the notification is intended to be optional/internal, consider introducing a new derived interface (e.g.ISecretMaskerNotifications : ISecretMasker) or exposing notifications via the concreteSecretMaskertype/adapter so existing implementers don’t break.
public event EventHandler<NewSecretEventArgs> NewSecretAdded;
src/Runner.Worker/Worker.cs:93
- The new vsock path (subscribing to
HostContext.SecretMasker.NewSecretAddedand callingsecretNotifier.NotifyNewSecret) isn’t exercised by the existing L0 tests.WorkerL0now injectsIVSockSecretNotifier, but it never sets upTryStartNotifier()to return true or asserts that secrets added duringInitializeSecretMaskerresult inNotifyNewSecretcalls. Adding a focused L0 test would help prevent regressions in this new integration.
if (Constants.Runner.Platform == Constants.OSPlatform.Linux &&
secretNotifier.TryStartNotifier())
{
HostContext.SecretMasker.NewSecretAdded += (sender, e) =>
{
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Low
Build based on #4537
On Linux runner, when
GITHUB_ACTIONS_RUNNER_VSOCK_CID_PORTenv is set to2:9999, the runner will try to connect to vsocket on CID:2 PORT:9999.The runner will notify all secrets to the receiver of the vsocket.
For a regex secret:
{"type":"regex","pattern":"my-secret-pattern"}For a variable secret:
{"type":"variable","values":["password","password_base64encoded", "password_all_encoder"]}