Implement VSock secret notifier.#4565
Open
TingluoHuang wants to merge 6 commits into
Open
Conversation
Contributor
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
Contributor
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
Contributor
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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"]}