Skip to content

[confignet] Allow setting security descriptors for npipes#15212

Open
swiatekm wants to merge 8 commits into
open-telemetry:mainfrom
swiatekm:feat/confignet-npipe-security-descriptor
Open

[confignet] Allow setting security descriptors for npipes#15212
swiatekm wants to merge 8 commits into
open-telemetry:mainfrom
swiatekm:feat/confignet-npipe-security-descriptor

Conversation

@swiatekm

Copy link
Copy Markdown
Contributor

Description

Add dialer.security_descriptor field to set an SDDL security descriptor on Windows named pipe (npipe) listeners in confignet.

Link to tracking issue

Fixes #15211

Testing

Added unit tests which actually create the named pipe with specified descriptors.

Documentation

Documented the field in the struct definition, the schema, and the readme.

@codecov

codecov Bot commented Apr 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.03%. Comparing base (52e6bf4) to head (7dc37bb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15212   +/-   ##
=======================================
  Coverage   91.03%   91.03%           
=======================================
  Files         729      729           
  Lines       48491    48499    +8     
=======================================
+ Hits        44144    44152    +8     
  Misses       3020     3020           
  Partials     1327     1327           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@swiatekm
swiatekm force-pushed the feat/confignet-npipe-security-descriptor branch from 426cd68 to 5a83e39 Compare April 28, 2026 11:33
@swiatekm
swiatekm marked this pull request as ready for review April 28, 2026 11:33
@swiatekm
swiatekm requested a review from a team as a code owner April 28, 2026 11:33
@swiatekm
swiatekm requested a review from codeboten April 28, 2026 11:33
Comment thread config/confignet/confignet.go
@github-actions

Copy link
Copy Markdown
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@linux-foundation-easycla

linux-foundation-easycla Bot commented May 15, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@swiatekm
swiatekm force-pushed the feat/confignet-npipe-security-descriptor branch from e2d7b28 to aad6cd5 Compare May 15, 2026 15:28
@github-actions github-actions Bot removed the Stale label May 19, 2026
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions Bot added the Stale label Jun 3, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions Bot closed this Jun 30, 2026
@mx-psi mx-psi reopened this Jul 9, 2026
@codspeed-hq

codspeed-hq Bot commented Jul 9, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 3 untouched benchmarks
🗄️ 2 archived benchmarks run1


Comparing swiatekm:feat/confignet-npipe-security-descriptor (7dc37bb) with main (52e6bf4)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them.

@github-actions github-actions Bot removed the Stale label Jul 10, 2026
Comment thread .chloggen/confignet-npipe-security-descriptor.yaml Outdated
Co-authored-by: Andrzej Stencel <andrzej.stencel@elastic.co>
@swiatekm
swiatekm requested a review from axw July 10, 2026 17:18
@andrzej-stencel andrzej-stencel added the ready-to-merge Code review completed; ready to merge by maintainers label Jul 13, 2026
@evan-bradley evan-bradley removed the ready-to-merge Code review completed; ready to merge by maintainers label Jul 17, 2026

@callegarimattia callegarimattia left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

First time reviewing here, so bear with me. I went through the confignet npipe changes and have a couple of suggestions below, feel free to discard if you've already considered them.

Comment thread config/confignet/npipe_windows.go Outdated
Comment on lines +26 to +29
var cfg *winio.PipeConfig
if securityDescriptor != "" {
cfg = &winio.PipeConfig{SecurityDescriptor: securityDescriptor}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

winio.ListenPipe already normalises a nil config to &PipeConfig{} internally (pipe.go:515), so the nil-guard branch isn't needed. Passing &winio.PipeConfig{SecurityDescriptor: securityDescriptor} directly is equivalent.

If a second field is added to NpipeConfig, the guard would silently drop it when securityDescriptor is empty.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, simplified this down to a single line.

Comment thread config/confignet/confignet.go Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should the security descriptor be part of the validation? Right now, if the SDDL is invalid, it fails when Listen is called, but it could be caught here beforehand. go-winio already exposes a conversion method that could be used to validate. It's the same one used in the Listen flow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added the validation.

@swiatekm
swiatekm requested a review from callegarimattia July 25, 2026 14:51
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.

Allow setting a security descriptor for Windows named pipes created by confignet

7 participants