Skip to content

Implement VSock secret notifier.#4565

Open
TingluoHuang wants to merge 6 commits into
mainfrom
users/tihuang/vsocket
Open

Implement VSock secret notifier.#4565
TingluoHuang wants to merge 6 commits into
mainfrom
users/tihuang/vsocket

Conversation

@TingluoHuang

Copy link
Copy Markdown
Member

Build based on #4537

On Linux runner, when GITHUB_ACTIONS_RUNNER_VSOCK_CID_PORT env is set to 2: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"]}

@TingluoHuang
TingluoHuang requested a review from a team as a code owner July 22, 2026 14:33
Copilot AI review requested due to automatic review settings July 22, 2026 14:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 NewSecretAdded event 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 VSockSecretNotifier service 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 the SocketAddress with AddressFamily.Unspecified. The SocketAddress should 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

Comment thread src/Sdk/DTLogging/Logging/SecretMasker.cs
Comment thread src/Sdk/DTLogging/Logging/ISecretMasker.cs
Comment thread src/Runner.Worker/Worker.cs Outdated
Comment thread src/Runner.Common/VSockSecretNotifier.cs
Comment thread src/Runner.Common/VSockSecretNotifier.cs
Comment thread src/Runner.Common/VSockSecretNotifier.cs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review details

Comments suppressed due to low confidence (2)

src/Sdk/DTLogging/Logging/ISecretMasker.cs:17

  • Adding NewSecretAdded to the existing public ISecretMasker interface 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 on SecretMasker, or introduce a new derived interface like ISecretMaskerWithNotifications).
    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

  • ProcessSecretChannel will commonly observe cancellation during normal shutdown (e.g., when DisposeAsync cancels the token). Currently this is logged as an error via the broad catch (Exception), which can generate noisy error logs on expected shutdown paths. Consider handling OperationCanceledException separately 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

Comment thread src/Sdk/DTLogging/Logging/ISecretMasker.cs
Comment thread src/Runner.Common/VSockSecretNotifier.cs
Comment thread src/Runner.Common/VSockSecretNotifier.cs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Comment thread src/Runner.Common/VSockSecretNotifier.cs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants