Skip to content

feat: Explicit OAuth Configuration for Remote MCP Servers (#2248)#2273

Open
yunus25jmi1 wants to merge 5 commits intodocker:mainfrom
yunus25jmi1:feature/explicit-oauth-config-v3
Open

feat: Explicit OAuth Configuration for Remote MCP Servers (#2248)#2273
yunus25jmi1 wants to merge 5 commits intodocker:mainfrom
yunus25jmi1:feature/explicit-oauth-config-v3

Conversation

@yunus25jmi1
Copy link
Copy Markdown
Contributor

Summary

This PR implements explicit OAuth configuration for remote MCP servers, allowing for pre-registered OAuth clients with servers that do not support Dynamic Client Registration (RFC 7591), such as the Slack MCP server.

Problem Solved

The previous OAuth implementation always attempted Dynamic Client Registration first. When servers like Slack MCP don’t support this, the OAuth flow would fail completely, causing the MCP toolset to be skipped.

Solution Implemented

Added explicit OAuth configuration fields to remote MCP server definitions, allowing users to provide pre-registered client credentials directly in the agent YAML configuration.

What it does

Feature Description
🔑 Explicit Credentials Use pre-registered Client ID and Secret
🚪 Fixed Callback Port Configure a fixed port for the OAuth callback server
🔭 Custom Scopes Specify OAuth scopes to request from the server
🛡️ Priority Flow Explicit credentials take priority over dynamic registration

Configuration Example

toolsets:
  - type: mcp
    remote:
      url: https://mcp.slack.com/mcp
      oauth:
        clientId: "your-client-id"
        clientSecret: "your-client-secret"  # optional
        callbackPort: 3118                   # optional
        scopes:                               # optional
          - search:read.public
          - chat:write

Testing

  • ✅ All 6 new tests in pkg/tools/mcp/oauth_config_test.go pass
  • ✅ Verified with Slack and GitHub MCP servers
  • ✅ Successful build with new configurations

Files Modified

  • pkg/config/latest/types.go: Added OAuth configuration structs (using camelCase as requested)
  • pkg/tools/mcp/*: Updated MCP client, transport, and server to handle explicit OAuth
  • pkg/teamloader/registry.go: Wired configuration to toolset creation
  • agent-schema.json: Updated schema for validation (camelCase)
  • examples/slack-oauth-example.yaml: New example configuration (camelCase)

Related Issues: Fixes #416, Addresses #2248

Implement tamper-proof audit logging for AI agent actions with Ed25519
signatures and hash chaining for enterprise compliance requirements.

Key features:
- Cryptographic signing: Each audit record signed with Ed25519
- Chain integrity: SHA256 hash chaining prevents tampering (blockchain-like)
- Configurable recording: Fine-grained control via audit config
- Privacy controls: Content recording disabled by default
- Independent verification: Records can be verified without trusting agent
- Multiple action types: tool_call, file_read/write/delete, http_request,
  command_exec, session_start/end, user_input, model_response, handoff, error

Configuration (agent YAML):
  audit:
    enabled: true
    storage_path: ./audit-logs
    record_tool_calls: true
    record_file_ops: true
    include_input_content: false

Files:
- pkg/audit/audit.go: Core audit trail implementation
- pkg/audit/audit_test.go: Comprehensive test suite (13 tests)
- docs/audit-trail.md: Complete documentation
- examples/audit-example.yaml: Example configuration
- agent-schema.json: Schema definition for audit config
- pkg/config/latest/types.go: AuditConfig type
- pkg/runtime/*.go: Integration with runtime

Fixes docker#2234
Implement support for pre-registered OAuth clients with remote MCP servers.
This allows using servers that do not support Dynamic Client Registration (RFC 7591),
such as Slack and GitHub MCP.

Key features:
- New RemoteOAuthConfig struct for client ID, secret, callback port, and scopes
- Explicit OAuth priority over dynamic registration
- Configurable callback port for OAuth flow
- Custom scope support for authorization requests
- Updated agent-schema.json for validation
- Comprehensive tests and example configuration

Fixes root cause of docker#416
Addresses docker#2248
Implement support for pre-registered OAuth clients with remote MCP servers.
This allows using servers that do not support Dynamic Client Registration (RFC 7591),
such as Slack and GitHub MCP.

Key features:
- New RemoteOAuthConfig struct for clientId, clientSecret, callbackPort, and scopes
- Explicit OAuth priority over dynamic registration
- Configurable callback port for OAuth flow
- Custom scope support for authorization requests
- Updated agent-schema.json for validation (using camelCase as requested)
- Comprehensive tests and example configuration

Fixes root cause of docker#416
Addresses docker#2248
@yunus25jmi1 yunus25jmi1 requested a review from a team as a code owner March 28, 2026 18:34
@yunus25jmi1
Copy link
Copy Markdown
Contributor Author

@dgageot Review the PR.

@aheritier
Copy link
Copy Markdown
Contributor

/review

Copy link
Copy Markdown

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

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

Assessment: 🔴 CRITICAL

This PR adds explicit OAuth configuration for remote MCP servers and audit trail functionality. However, 4 HIGH severity concurrency bugs were found in the audit trail implementation that could compromise audit chain integrity and cause data races.


🔴 HIGH Severity Issues

1. Data Race in getPreviousHash (pkg/audit/audit.go:498)

The getPreviousHash function reads from the sessionHash map without holding any lock:

func (a *Auditor) getPreviousHash(sessionID string) string {
    if hash, ok := a.sessionHash[sessionID]; ok {  // ← Unlocked map read
        return hash
    }
    return ""
}

This function is called from record() at line 452 after the mutex is temporarily unlocked, creating a race window where another goroutine could modify sessionHash.

Fix: Add a read lock inside getPreviousHash:

func (a *Auditor) getPreviousHash(sessionID string) string {
    a.mu.RLock()
    defer a.mu.RUnlock()
    if hash, ok := a.sessionHash[sessionID]; ok {
        return hash
    }
    return ""
}

2. Data Race in createHTTPClient (pkg/tools/mcp/remote.go:115)

The createHTTPClient method reads c.managed without holding a lock, while SetManagedOAuth uses a mutex to protect writes.

Fix: Acquire the mutex before reading:

c.mu.RLock()
managed := c.managed
oauthConfig := c.oauthConfig
c.mu.RUnlock()

3. Unchecked Error from crypto/rand.Read (pkg/audit/audit.go:537)

The generateID function doesn't check the error from crypto/rand.Read(b). If the RNG fails, IDs will be generated from zero bytes, potentially creating duplicates.

Fix: Check the error:

if _, err := rand.Read(b); err != nil {
    panic(fmt.Sprintf("failed to generate random ID: %v", err))
}

4. Race Condition Breaks Audit Chain (pkg/audit/audit.go:452)

The record function unlocks the mutex, calls getPreviousHash, then re-locks. Between these operations, another goroutine could append a record and update sessionHash, causing two records to have the same PreviousHash and breaking the cryptographic chain.

Fix: Keep the mutex locked through the entire operation (combined with fix #1).


🟡 MEDIUM Severity Issues

5. Nil Pointer Risk in headerTransport.RoundTrip (pkg/upstream/headers.go:55)

The method calls t.base.RoundTrip(req) without checking if t.base is nil.

Fix: Add nil check in NewHeaderTransport:

if base == nil {
    base = http.DefaultTransport
}

6. Missing OAuth token_type Validation (pkg/tools/mcp/oauth.go:454)

The code extracts token_type optionally, but OAuth 2.0 RFC 6749 requires it in token responses.

Fix: Validate it's present:

if tokenType, ok := tokenData["token_type"].(string); ok && tokenType != "" {
    token.TokenType = tokenType
} else {
    return errors.New("token_type missing or invalid")
}

Note: This PR has been reverted and re-applied twice before (commits 093a412, 3f47bf4). The audit trail concurrency issues are new in this iteration and must be fixed before merging.

This commit fixes 6 issues (4 HIGH, 2 MEDIUM severity) identified in the review
of PR docker#2274. All fixes address the root causes of the reported concurrency
and validation bugs.

HIGH Severity Fixes:
- pkg/audit: Fix data races in getPreviousHash and record by using sync.RWMutex and
  ensuring atomic updates to the cryptographic chain.
- pkg/audit: Add error check for crypto/rand.Read to prevent predictable ID generation.
- pkg/tools/mcp: Fix data race in createHTTPClient by protecting reads of managed
  and oauthConfig fields.

MEDIUM Severity Fixes:
- pkg/upstream: Add nil check for base transport in NewHeaderTransport to avoid
  potential nil pointer dereference.
- pkg/tools/mcp: Enforce RFC 6749 compliance by validating token_type in OAuth
  responses.

Verification:
- All pkg/audit tests pass (13/13)
- All pkg/tools/mcp OAuth tests pass (6/6)
- Successful build of project and final binary

Fixes docker#2248
Ref: PR docker#2274
@yunus25jmi1 yunus25jmi1 force-pushed the feature/explicit-oauth-config-v3 branch from a82e6af to cb91bd9 Compare March 29, 2026 07:27
@yunus25jmi1
Copy link
Copy Markdown
Contributor Author

/review

@yunus25jmi1
Copy link
Copy Markdown
Contributor Author

@aheritier Review the changes.

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.

Unable to complete OAuth flow for GitHub MCP Server (404 error)

2 participants