feat: add rpcserver #6

Merged

rcsheets merged 3 commits from feat/rpcserver into main 2026-04-13 05:44:50 +00:00

Conversation 2 Commits 3 Files changed 6 +933

rcsheets commented 2026-04-13 05:19:37 +00:00

Owner

Copy link

No description provided.

rcsheets added 2 commits 2026-04-13 05:19:38 +00:00

Add LookupUniverse to approvalstore client ... 38d127b8d8

The rpcserver needs to resolve a universe_id from a ReadyOperation
into the universe row's kind + driver_config so it can build the
right driver. Add a typed lookup that returns nil when no row matches
(callers decide whether that's a hard error). Integration tests
cover the happy path and the missing-row case under the rpc_server
role.

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

Add cmd/rpcserver: poll the approval store, execute approved ops ... eff2b631d5

First runnable binary. The rpcserver authenticates to Postgres as the
rpc_server role, polls the approval store every 2s for operations
whose status is 'approved', and walks each through:

  1. content-hash recomputation and constant-time comparison against
     the stored hash — refuse to execute if they diverge
  2. mandatory policy evaluation via the trust ledger
  3. universe-driver resolution (cached per universe_id) and subject
     validation through the driver
  4. ops registry lookup and Operation.Execute against the universe's
     transport host (the DC for ad_domain)
  5. record an executions row with the result, success/error, and the
     content hash this server independently recomputed

Every code path either succeeds or records a failed execution, so
operators see *why* something didn't run instead of staring at a stuck
'approved' status.

The processor is split from main so unit tests can drive it with
fakes for the store, executor, and universe lookup. Tests cover the
happy path plus hash mismatch, policy denial, missing universe,
subject-validation failure, transport error during execute, empty
poll, list-error propagation, and driver caching.

main wires real dependencies: pgxpool, ops.Default(), universe.Default(),
policy.NewMandatory, and a winrm executor factory pulled from env
config. Graceful shutdown on SIGINT/SIGTERM via signal.NotifyContext.

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

pr-reviewer-bot commented 2026-04-13 05:19:39 +00:00

Collaborator

Copy link

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

Status: ✅ Completed

---

✅ Verdict: Approve — Well-architected RPC server implementation with excellent error handling, security considerations, comprehensive test coverage, and clear separation of concerns.
Complexity: complex

Review of PR #6: feat: add rpcserver

This is a well-implemented addition of an RPC server component that executes approved operations against Windows hosts. The code demonstrates excellent architecture, comprehensive error handling, and thorough testing.

Architecture & Design ✅

The design is exemplary:

• Clean separation: main.go handles dependency wiring, processor.go contains testable business logic
• Interface-driven: All dependencies are defined as interfaces enabling easy testing
• Layered validation: Hash verification → policy check → subject validation → execution
• Proper caching: Universe drivers are cached with thread-safe access
• Fail-safe design: Every execution path either succeeds or records a failure (no silent drops)

Security & Integrity ✅

Strong security measures are implemented:

• Content hash verification: Uses crypto/subtle.ConstantTimeCompare to prevent timing attacks (line 150 in processor.go)
• Mandatory policy enforcement: All operations go through policy evaluation before execution
• No credential exposure: WinRM credentials are passed through config, not logged
• Audit trail: Every execution is recorded with timestamps and executor identity

The automated detection of "possible secret or credential" in main.go is a false positive - the code only references cfg.WinRM.Password from configuration, not hardcoded secrets.

Error Handling ✅

Comprehensive error handling throughout:

• Graceful degradation: Per-tick errors are logged but don't crash the service
• Detailed error recording: Failed operations include specific error messages in the database
• Context cancellation: Proper handling of shutdown signals
• No silent failures: Every code path in processOne() either succeeds or calls recordFailure()

Test Coverage ✅

Excellent test coverage with realistic scenarios:

• Happy path: End-to-end success case (TestProcessOneSuccess)
• Security validation: Hash mismatch prevention (TestProcessOneHashMismatchRefusesExecution)
• Policy enforcement: Unknown operations denied (TestProcessOneUnknownOpDeniedByPolicy)
• Error conditions: Transport failures, missing universes, invalid subjects
• Caching behavior: Driver reuse validation (TestDriverFromCacheReused)
• Integration tests: Full lifecycle testing in store_test.go

The test architecture with fake implementations is well-designed and maintainable.

Code Quality ✅

High quality implementation:

• Clear documentation: Every function and type is well-documented
• Consistent error formatting: All errors use fmt.Errorf with proper wrapping
• Thread safety: Proper mutex usage around shared state
• Resource management: Database connections and tickers are properly cleaned up

Minor Observations

1. Go version: Using Go 1.26.2 which is valid (newer than my training data cutoff)
2. Hard-coded scope: Line 169 in processor.go uses a hardcoded "single" scope with a TODO comment - this is properly documented as a known limitation for MVP
3. Test files without tests: The automated observation about missing tests for main.go and some other files is expected - main.go is just dependency wiring, and the core logic is thoroughly tested in processor_test.go

Dependencies ✅

All new dependencies are legitimate and appropriate:

• github.com/jackc/pgx/v5/pgxpool: PostgreSQL connection pooling
• Additional pgx-related dependencies for database functionality

Performance Considerations ✅

Good performance characteristics:

• Efficient polling: 2-second intervals with immediate first tick
• Connection pooling: Uses pgxpool for database connections
• Driver caching: Expensive driver builds are cached per universe
• Bounded operations: Processes operations sequentially to avoid resource exhaustion

Summary

This is a high-quality implementation that follows best practices for security, error handling, testing, and architecture. The code is production-ready with excellent maintainability. The comprehensive test suite and integration tests provide strong confidence in correctness.

The RPC server properly implements the security model with hash verification, policy enforcement, and complete audit trails. The failure handling ensures operators have visibility into why operations succeed or fail.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.15.0 | Full Review | anthropic | tracking id `r-dc7cea-f07d5a`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: Approve** — Well-architected RPC server implementation with excellent error handling, security considerations, comprehensive test coverage, and clear separation of concerns. **Complexity:** complex # Review of PR #6: feat: add rpcserver This is a well-implemented addition of an RPC server component that executes approved operations against Windows hosts. The code demonstrates excellent architecture, comprehensive error handling, and thorough testing. ## Architecture & Design ✅ The design is exemplary: - **Clean separation**: `main.go` handles dependency wiring, `processor.go` contains testable business logic - **Interface-driven**: All dependencies are defined as interfaces enabling easy testing - **Layered validation**: Hash verification → policy check → subject validation → execution - **Proper caching**: Universe drivers are cached with thread-safe access - **Fail-safe design**: Every execution path either succeeds or records a failure (no silent drops) ## Security & Integrity ✅ Strong security measures are implemented: - **Content hash verification**: Uses `crypto/subtle.ConstantTimeCompare` to prevent timing attacks (line 150 in `processor.go`) - **Mandatory policy enforcement**: All operations go through policy evaluation before execution - **No credential exposure**: WinRM credentials are passed through config, not logged - **Audit trail**: Every execution is recorded with timestamps and executor identity The automated detection of "possible secret or credential" in `main.go` is a false positive - the code only references `cfg.WinRM.Password` from configuration, not hardcoded secrets. ## Error Handling ✅ Comprehensive error handling throughout: - **Graceful degradation**: Per-tick errors are logged but don't crash the service - **Detailed error recording**: Failed operations include specific error messages in the database - **Context cancellation**: Proper handling of shutdown signals - **No silent failures**: Every code path in `processOne()` either succeeds or calls `recordFailure()` ## Test Coverage ✅ Excellent test coverage with realistic scenarios: - **Happy path**: End-to-end success case (`TestProcessOneSuccess`) - **Security validation**: Hash mismatch prevention (`TestProcessOneHashMismatchRefusesExecution`) - **Policy enforcement**: Unknown operations denied (`TestProcessOneUnknownOpDeniedByPolicy`) - **Error conditions**: Transport failures, missing universes, invalid subjects - **Caching behavior**: Driver reuse validation (`TestDriverFromCacheReused`) - **Integration tests**: Full lifecycle testing in `store_test.go` The test architecture with fake implementations is well-designed and maintainable. ## Code Quality ✅ High quality implementation: - **Clear documentation**: Every function and type is well-documented - **Consistent error formatting**: All errors use `fmt.Errorf` with proper wrapping - **Thread safety**: Proper mutex usage around shared state - **Resource management**: Database connections and tickers are properly cleaned up ## Minor Observations 1. **Go version**: Using Go 1.26.2 which is valid (newer than my training data cutoff) 2. **Hard-coded scope**: Line 169 in `processor.go` uses a hardcoded `"single"` scope with a TODO comment - this is properly documented as a known limitation for MVP 3. **Test files without tests**: The automated observation about missing tests for `main.go` and some other files is expected - `main.go` is just dependency wiring, and the core logic is thoroughly tested in `processor_test.go` ## Dependencies ✅ All new dependencies are legitimate and appropriate: - `github.com/jackc/pgx/v5/pgxpool`: PostgreSQL connection pooling - Additional pgx-related dependencies for database functionality ## Performance Considerations ✅ Good performance characteristics: - **Efficient polling**: 2-second intervals with immediate first tick - **Connection pooling**: Uses pgxpool for database connections - **Driver caching**: Expensive driver builds are cached per universe - **Bounded operations**: Processes operations sequentially to avoid resource exhaustion ## Summary This is a high-quality implementation that follows best practices for security, error handling, testing, and architecture. The code is production-ready with excellent maintainability. The comprehensive test suite and integration tests provide strong confidence in correctness. The RPC server properly implements the security model with hash verification, policy enforcement, and complete audit trails. The failure handling ensures operators have visibility into why operations succeed or fail.

rcsheets requested review from pr-reviewer-bot 2026-04-13 05:19:42 +00:00

pr-reviewer-bot commented 2026-04-13 05:19:43 +00:00

Collaborator

Copy link

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

Status: ✅ Completed

---

✅ Verdict: Approve — Well-architected RPC server implementation with thorough testing, proper error handling, and good security practices.
Complexity: complex

RPC Server Implementation Review

This PR introduces a well-designed RPC server that executes approved operations against Windows hosts. The implementation demonstrates excellent architectural patterns and comprehensive testing.

Strengths

Architecture & Design

• Clean separation of concerns: main.go handles dependency injection while processor.go contains testable business logic
• Interface-based design: All dependencies use interfaces, enabling easy testing and future extensibility
• Proper caching: Universe drivers are cached to avoid repeated WinRM client allocations
• Graceful shutdown: Uses signal.NotifyContext for proper cleanup

Security & Integrity

• Content hash verification: Uses constant-time comparison to prevent timing attacks (crypto/subtle.ConstantTimeCompare)
• Mandatory policy enforcement: Every operation goes through policy evaluation before execution
• Comprehensive audit trail: All execution attempts are logged, including failures
• No credential leakage: The automated "secret detection" appears to be a false positive - no actual credentials are present

Error Handling

• Fail-safe design: Every code path either succeeds or records an execution row - nothing gets "dropped on the floor"
• Proper error wrapping: Uses fmt.Errorf with %w verb consistently for error chains
• Context cancellation: Respects context cancellation throughout the processing pipeline

Testing

• Comprehensive test coverage: Tests cover happy path, error conditions, hash verification, policy enforcement, and caching behavior
• Excellent fakes: Well-designed test doubles with proper synchronization
• Integration tests: store_test.go includes full database integration tests with role-based access control verification

Minor Observations

Code Quality

• Hardcoded scope kind: Line 169 in processor.go uses a hardcoded "single" scope kind with a TODO comment explaining this is temporary for MVP
• Database connection: Uses connection pooling appropriately with pgxpool.Pool
• Logging: Consistent structured logging with appropriate log levels

Dependencies

• Go version: Uses Go 1.26.2 which is appropriate
• Clean dependency management: No unnecessary dependencies, appropriate use of pgx v5 and other libraries

Test Coverage Analysis

The automated observation about missing tests for cmd/rpcserver/main.go, cmd/rpcserver/processor.go, and internal/approvalstore/store.go needs clarification:

• main.go: Contains only dependency wiring - integration tests would be more appropriate than unit tests
• processor.go: Has comprehensive unit tests in processor_test.go
• store.go: Has thorough integration tests in store_test.go

The test coverage appears adequate for the complexity and risk profile of this code.

Security Considerations

The code demonstrates good security practices:

• Uses prepared statements via pgx (SQL injection prevention)
• Implements proper authentication via role-based database access
• Uses constant-time comparison for hash verification
• No hardcoded credentials (the automation false positive likely triggered on the WinRM config structure)

Performance

• Driver caching prevents repeated expensive WinRM client creation
• Efficient database queries using views and proper indexing
• Reasonable polling interval (2 seconds) for the execution loop

This is a high-quality implementation that follows Go best practices and demonstrates thoughtful system design. The code is ready for production use.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.15.0 | Full Review | anthropic | tracking id `r-dc7cef-8c19e2`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: Approve** — Well-architected RPC server implementation with thorough testing, proper error handling, and good security practices. **Complexity:** complex # RPC Server Implementation Review This PR introduces a well-designed RPC server that executes approved operations against Windows hosts. The implementation demonstrates excellent architectural patterns and comprehensive testing. ## Strengths ### Architecture & Design - **Clean separation of concerns**: `main.go` handles dependency injection while `processor.go` contains testable business logic - **Interface-based design**: All dependencies use interfaces, enabling easy testing and future extensibility - **Proper caching**: Universe drivers are cached to avoid repeated WinRM client allocations - **Graceful shutdown**: Uses signal.NotifyContext for proper cleanup ### Security & Integrity - **Content hash verification**: Uses constant-time comparison to prevent timing attacks (`crypto/subtle.ConstantTimeCompare`) - **Mandatory policy enforcement**: Every operation goes through policy evaluation before execution - **Comprehensive audit trail**: All execution attempts are logged, including failures - **No credential leakage**: The automated "secret detection" appears to be a false positive - no actual credentials are present ### Error Handling - **Fail-safe design**: Every code path either succeeds or records an execution row - nothing gets "dropped on the floor" - **Proper error wrapping**: Uses `fmt.Errorf` with `%w` verb consistently for error chains - **Context cancellation**: Respects context cancellation throughout the processing pipeline ### Testing - **Comprehensive test coverage**: Tests cover happy path, error conditions, hash verification, policy enforcement, and caching behavior - **Excellent fakes**: Well-designed test doubles with proper synchronization - **Integration tests**: `store_test.go` includes full database integration tests with role-based access control verification ## Minor Observations ### Code Quality - **Hardcoded scope kind**: Line 169 in `processor.go` uses a hardcoded `"single"` scope kind with a TODO comment explaining this is temporary for MVP - **Database connection**: Uses connection pooling appropriately with `pgxpool.Pool` - **Logging**: Consistent structured logging with appropriate log levels ### Dependencies - **Go version**: Uses Go 1.26.2 which is appropriate - **Clean dependency management**: No unnecessary dependencies, appropriate use of pgx v5 and other libraries ## Test Coverage Analysis The automated observation about missing tests for `cmd/rpcserver/main.go`, `cmd/rpcserver/processor.go`, and `internal/approvalstore/store.go` needs clarification: - `main.go`: Contains only dependency wiring - integration tests would be more appropriate than unit tests - `processor.go`: Has comprehensive unit tests in `processor_test.go` - `store.go`: Has thorough integration tests in `store_test.go` The test coverage appears adequate for the complexity and risk profile of this code. ## Security Considerations The code demonstrates good security practices: - Uses prepared statements via pgx (SQL injection prevention) - Implements proper authentication via role-based database access - Uses constant-time comparison for hash verification - No hardcoded credentials (the automation false positive likely triggered on the WinRM config structure) ## Performance - Driver caching prevents repeated expensive WinRM client creation - Efficient database queries using views and proper indexing - Reasonable polling interval (2 seconds) for the execution loop This is a high-quality implementation that follows Go best practices and demonstrates thoughtful system design. The code is ready for production use.

rcsheets added 1 commit 2026-04-13 05:25:16 +00:00

Clarify hash-verification comment in rpcserver processor ... 47c4f2ec4a

The previous wording claimed re-fetch was 'implicit', which was just
wrong — nothing is re-fetched. ListApprovedReadyToExecute already
returns the canonical operation row fields (the view JOINs against
operations) and the operations table is append-only, so a separate
re-fetch wouldn't add integrity. Replace with a description of what
the code actually does and why a re-fetch isn't needed.

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

rcsheets merged commit 346de202ed into main 2026-04-13 05:44:50 +00:00

rcsheets referenced this pull request from a commit 2026-04-13 05:44:52 +00:00

Merge pull request 'feat: add rpcserver' (#6) from feat/rpcserver into main

rcsheets deleted branch feat/rpcserver 2026-04-13 05:44:52 +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

Subscribe

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!6

No description provided.