brooktrails/directory-pacifier
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat: Add config, winrm, and ops packages with get_computer_info #4

Merged
rcsheets merged 1 commit from feat/ops-and-winrm into main 2026-04-13 04:55:01 +00:00
Conversation 2 Commits 1 Files changed 9 +1022
rcsheets commented 2026-04-13 04:51:01 +00:00
Owner
Copy link

internal/config — env-driven loader covering the union of fields any
service needs (Postgres, Anthropic, WinRM, advisory rules, HTTP listen,
controlloop URL). FromEnv populates whatever is set; Require* helpers
let each service spell out its own required-field policy.

internal/winrm — thin wrapper over masterzen/winrm exposing a single
RunPowerShell method. Signature matches ops.Executor so *Client
satisfies that interface implicitly. New() applies sensible defaults
(port 5985/5986 by transport, 30s timeout).

internal/ops — Operation interface + thread-safe Registry keyed by
operation_type. First operation: get_computer_info, a read-only AD
diagnostic that runs Get-ADComputer plus Test-Connection over WinRM
and returns combined JSON. Field names are constrained to plain
identifiers and subjects are character-whitelisted + single-quote
escaped to keep PowerShell injection off the table.

Default() returns a registry pre-populated with every operation in
this package — controlloop and rpcserver will both use it.

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

internal/config — env-driven loader covering the union of fields any service needs (Postgres, Anthropic, WinRM, advisory rules, HTTP listen, controlloop URL). FromEnv populates whatever is set; Require* helpers let each service spell out its own required-field policy. internal/winrm — thin wrapper over masterzen/winrm exposing a single RunPowerShell method. Signature matches ops.Executor so *Client satisfies that interface implicitly. New() applies sensible defaults (port 5985/5986 by transport, 30s timeout). internal/ops — Operation interface + thread-safe Registry keyed by operation_type. First operation: get_computer_info, a read-only AD diagnostic that runs Get-ADComputer plus Test-Connection over WinRM and returns combined JSON. Field names are constrained to plain identifiers and subjects are character-whitelisted + single-quote escaped to keep PowerShell injection off the table. Default() returns a registry pre-populated with every operation in this package — controlloop and rpcserver will both use it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add config, winrm, and ops packages with get_computer_info
All checks were successful
test / go-test (pull_request) Successful in 16s
Details
test / schema-test (pull_request) Successful in 52s
Details
51f1cbe24f 
internal/config — env-driven loader covering the union of fields any
service needs (Postgres, Anthropic, WinRM, advisory rules, HTTP listen,
controlloop URL). FromEnv populates whatever is set; Require* helpers
let each service spell out its own required-field policy.

internal/winrm — thin wrapper over masterzen/winrm exposing a single
RunPowerShell method. Signature matches ops.Executor so *Client
satisfies that interface implicitly. New() applies sensible defaults
(port 5985/5986 by transport, 30s timeout).

internal/ops — Operation interface + thread-safe Registry keyed by
operation_type. First operation: get_computer_info, a read-only AD
diagnostic that runs Get-ADComputer plus Test-Connection over WinRM
and returns combined JSON. Field names are constrained to plain
identifiers and subjects are character-whitelisted + single-quote
escaped to keep PowerShell injection off the table.

Default() returns a registry pre-populated with every operation in
this package — controlloop and rpcserver will both use it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pr-reviewer-bot commented 2026-04-13 04:51:04 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.15.0 | Safety Check | anthropic | tracking id r-dc7638-ca48eb
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: LGTM — No security vulnerabilities or obvious bugs found; hardcoded test credentials and high-entropy test files are expected for this type of functionality.
Complexity: moderate

Safety Check Review

Overview

This PR adds configuration, WinRM client, and operations packages to support Windows remote management operations. The code appears well-structured with appropriate security considerations.

Security Analysis

  • PowerShell injection protection: The code includes proper input validation with regex patterns for field names (^[A-Za-z][A-Za-z0-9_]*$) and subject character whitelisting, plus single-quote escaping for PowerShell literals
  • Credential handling: Credentials are properly managed through environment variables and config structs without hardcoding
  • Test fixtures: The "possible secret" detections in test files appear to be legitimate test data/fixtures rather than real credentials

Code Quality

The implementation shows good defensive programming practices:

  • Input validation at multiple layers
  • Proper error handling and wrapping
  • Thread-safe registry implementation
  • Comprehensive test coverage for critical paths

No Major Issues Found

  • Version numbers in dependencies appear legitimate
  • No obvious logic errors or security vulnerabilities
  • High-entropy content in test files is expected for this type of functionality
  • Missing tests for some files is noted but not a safety concern for this initial implementation

The code demonstrates careful attention to security concerns around PowerShell execution and credential management.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.15.0 | Safety Check | anthropic | tracking id `r-dc7638-ca48eb`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: LGTM** — No security vulnerabilities or obvious bugs found; hardcoded test credentials and high-entropy test files are expected for this type of functionality. **Complexity:** moderate # Safety Check Review ## Overview This PR adds configuration, WinRM client, and operations packages to support Windows remote management operations. The code appears well-structured with appropriate security considerations. ## Security Analysis - **PowerShell injection protection**: The code includes proper input validation with regex patterns for field names (`^[A-Za-z][A-Za-z0-9_]*$`) and subject character whitelisting, plus single-quote escaping for PowerShell literals - **Credential handling**: Credentials are properly managed through environment variables and config structs without hardcoding - **Test fixtures**: The "possible secret" detections in test files appear to be legitimate test data/fixtures rather than real credentials ## Code Quality The implementation shows good defensive programming practices: - Input validation at multiple layers - Proper error handling and wrapping - Thread-safe registry implementation - Comprehensive test coverage for critical paths ## No Major Issues Found - Version numbers in dependencies appear legitimate - No obvious logic errors or security vulnerabilities - High-entropy content in test files is expected for this type of functionality - Missing tests for some files is noted but not a safety concern for this initial implementation The code demonstrates careful attention to security concerns around PowerShell execution and credential management.
pr-reviewer-bot commented 2026-04-13 04:52:41 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.15.0 | Full Review | anthropic | tracking id r-dc7698-4b224f
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: Approve — The code is well-structured with good security practices, comprehensive tests, and clear separation of concerns.
Complexity: moderate

Code Review: Add config, winrm, and ops packages

This is a well-crafted addition that introduces foundational infrastructure packages for a Windows remote management system. The code demonstrates strong engineering practices with security-first design, comprehensive testing, and clear architectural boundaries.

Security Analysis ✅

The security implementation is particularly noteworthy:

PowerShell Injection Prevention:

  • internal/ops/get_computer_info.go:48: Field names are restricted to ^[A-Za-z][A-Za-z0-9_]*$ preventing injection via properties
  • internal/ops/get_computer_info.go:138: Subject values are properly single-quote escaped with PS literal string rules (' → '')
  • internal/ops/get_computer_info.go:168-184: Character allowlist validation provides defense in depth beyond quoting

Input Validation:

  • Subject length limit of 512 characters prevents DoS attacks
  • Comprehensive character filtering allows legitimate DN/GUID/SID formats while blocking shell metacharacters
  • JSON parameter validation with proper error handling

Architecture & Design ✅

Clean Separation of Concerns:

  • internal/config: Environment-driven configuration with service-specific validation
  • internal/winrm: Thin wrapper over third-party library with sensible defaults
  • internal/ops: Operation abstraction with registry pattern for extensibility

Interface Design:

  • Executor interface allows clean testing with mocks/fakes
  • Operation interface provides consistent contract for all operations
  • Registry pattern enables runtime dispatch and extensibility

Implementation Quality ✅

Error Handling:

  • Transport errors vs. script execution errors properly distinguished
  • Comprehensive validation at multiple layers (parse-time and execution-time)
  • Meaningful error messages with context

Configuration Management:

  • internal/config/config.go:78-117: Env-only approach is clean and Kubernetes-friendly
  • Optional vs. required field validation delegated to consumers
  • Sensible defaults (ports 5985/5986, 30s timeout, ":8080" listen)

PowerShell Script Generation:

  • internal/ops/get_computer_info.go:136-160: Well-structured script with proper error handling
  • Combines AD query with connectivity test for comprehensive diagnostics
  • JSON output format suitable for LLM consumption

Test Coverage ✅

The test suite is comprehensive:

Config Tests:

  • Default value verification
  • Environment variable parsing
  • Error cases for malformed values
  • Required field validation

Registry Tests:

  • Registration, retrieval, and error cases
  • Concurrent safety through mutex protection
  • Duplicate prevention

Operation Tests:

  • Parameter validation edge cases
  • Script generation and execution
  • Error propagation (transport vs. script errors)
  • JSON output validation

WinRM Tests:

  • Configuration validation
  • Default application
  • Missing required fields

Minor Observations

Automated Flags (Non-Issues):

  • High entropy in go.sum and test files is expected for dependency hashes and test data
  • "Possible credentials" in config/test files are example/test values, not real secrets
  • Go 1.26.2 is a reasonable future version number

Missing Tests (Acceptable for MVP):
The automated observation notes 4 files lack corresponding tests:

  • internal/config/config.go - Has comprehensive tests in config_test.go
  • internal/ops/get_computer_info.go - Tested via registry_test.go
  • internal/ops/registry.go - Has dedicated tests in registry_test.go
  • internal/winrm/client.go - Has tests in client_test.go

The automated tool seems to be looking for 1:1 file naming, but the actual test coverage is excellent.

Conclusion

This is high-quality infrastructure code that establishes solid foundations for the Windows management system. The security-first approach, clean architecture, and comprehensive testing make it ready for production use. The abstraction layers will facilitate future extensibility while maintaining security boundaries.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.15.0 | Full Review | anthropic | tracking id `r-dc7698-4b224f`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: Approve** — The code is well-structured with good security practices, comprehensive tests, and clear separation of concerns. **Complexity:** moderate # Code Review: Add config, winrm, and ops packages This is a well-crafted addition that introduces foundational infrastructure packages for a Windows remote management system. The code demonstrates strong engineering practices with security-first design, comprehensive testing, and clear architectural boundaries. ## Security Analysis ✅ The security implementation is particularly noteworthy: **PowerShell Injection Prevention:** - `internal/ops/get_computer_info.go:48`: Field names are restricted to `^[A-Za-z][A-Za-z0-9_]*$` preventing injection via properties - `internal/ops/get_computer_info.go:138`: Subject values are properly single-quote escaped with PS literal string rules (`'` → `''`) - `internal/ops/get_computer_info.go:168-184`: Character allowlist validation provides defense in depth beyond quoting **Input Validation:** - Subject length limit of 512 characters prevents DoS attacks - Comprehensive character filtering allows legitimate DN/GUID/SID formats while blocking shell metacharacters - JSON parameter validation with proper error handling ## Architecture & Design ✅ **Clean Separation of Concerns:** - `internal/config`: Environment-driven configuration with service-specific validation - `internal/winrm`: Thin wrapper over third-party library with sensible defaults - `internal/ops`: Operation abstraction with registry pattern for extensibility **Interface Design:** - `Executor` interface allows clean testing with mocks/fakes - `Operation` interface provides consistent contract for all operations - Registry pattern enables runtime dispatch and extensibility ## Implementation Quality ✅ **Error Handling:** - Transport errors vs. script execution errors properly distinguished - Comprehensive validation at multiple layers (parse-time and execution-time) - Meaningful error messages with context **Configuration Management:** - `internal/config/config.go:78-117`: Env-only approach is clean and Kubernetes-friendly - Optional vs. required field validation delegated to consumers - Sensible defaults (ports 5985/5986, 30s timeout, ":8080" listen) **PowerShell Script Generation:** - `internal/ops/get_computer_info.go:136-160`: Well-structured script with proper error handling - Combines AD query with connectivity test for comprehensive diagnostics - JSON output format suitable for LLM consumption ## Test Coverage ✅ The test suite is comprehensive: **Config Tests:** - Default value verification - Environment variable parsing - Error cases for malformed values - Required field validation **Registry Tests:** - Registration, retrieval, and error cases - Concurrent safety through mutex protection - Duplicate prevention **Operation Tests:** - Parameter validation edge cases - Script generation and execution - Error propagation (transport vs. script errors) - JSON output validation **WinRM Tests:** - Configuration validation - Default application - Missing required fields ## Minor Observations **Automated Flags (Non-Issues):** - High entropy in `go.sum` and test files is expected for dependency hashes and test data - "Possible credentials" in config/test files are example/test values, not real secrets - Go 1.26.2 is a reasonable future version number **Missing Tests (Acceptable for MVP):** The automated observation notes 4 files lack corresponding tests: - `internal/config/config.go` - Has comprehensive tests in `config_test.go` - `internal/ops/get_computer_info.go` - Tested via `registry_test.go` - `internal/ops/registry.go` - Has dedicated tests in `registry_test.go` - `internal/winrm/client.go` - Has tests in `client_test.go` The automated tool seems to be looking for 1:1 file naming, but the actual test coverage is excellent. ## Conclusion This is high-quality infrastructure code that establishes solid foundations for the Windows management system. The security-first approach, clean architecture, and comprehensive testing make it ready for production use. The abstraction layers will facilitate future extensibility while maintaining security boundaries.
rcsheets deleted branch feat/ops-and-winrm 2026-04-13 04:55:03 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
pr-reviewer-bot
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
brooktrails/directory-pacifier!4
No description provided.