Skip to content

feat(security): add security hardening — auth, encryption, validation - #175

Closed
jessecurry wants to merge 8 commits into
MemPalace:mainfrom
BoutLabs:feat/security-hardening
Closed

feat(security): add security hardening — auth, encryption, validation#175
jessecurry wants to merge 8 commits into
MemPalace:mainfrom
BoutLabs:feat/security-hardening

Conversation

@jessecurry

Copy link
Copy Markdown

Summary

Adds opt-in security hardening to MemPalace based on a comprehensive security audit. All features are backward-compatible — MemPalace works exactly as before unless security is explicitly enabled in config.

What's included

  • Token-based authentication on the MCP server — tokens stored in OS keychain (macOS Keychain, Windows Credential Manager, Linux Secret Service) via keyring, with file fallback
  • Fernet encryption at rest for palace data — encryption keys also stored in OS keychain. Encrypted content stored in ChromaDB metadata; when encryption is enabled, the documents field stores only a content-hash placeholder to prevent plaintext persistence on disk
  • Input validation on all MCP tool calls — required field checking, unknown field rejection, configurable content size limits (default 1MB)
  • File permission hardening0o600 on sensitive files, 0o700 on config directories
  • SHA-256 replacing MD5 for all ID generation (5 call sites)
  • Localhost-only guard for future HTTP transport — start_http_server() rejects non-localhost bind addresses
  • Updated .gitignore.env, *.key, auth_token, *.sqlite3, entities.json, IDE configs

Important: Semantic search and encryption

When encryption is disabled (the default), semantic search works exactly as it does today — no behavior change whatsoever.

When encryption is enabled, the ChromaDB documents field stores a [encrypted:<hash>] placeholder instead of plaintext, so content is not persisted unencrypted on disk. The trade-off is that semantic search will not return meaningful results for encrypted drawers, since ChromaDB cannot compute useful embeddings on the placeholder. This is an honest, unavoidable trade-off — you cannot have encryption at rest and semantic search over plaintext simultaneously without an external embeddings service. The authoritative content lives only in the encrypted_content metadata field.

Users who need both search and encryption should pair this with OS-level disk encryption (FileVault, LUKS, BitLocker) and leave application-level encryption disabled.

Configuration

All security features live under a security block in ~/.mempalace/config.json:

{
  "security": {
    "auth_enabled": true,
    "encryption_enabled": true,
    "max_content_size": 1048576
  }
}

Environment variable overrides: MEMPALACE_AUTH_ENABLED, MEMPALACE_ENCRYPTION_ENABLED.

Dependencies

cryptography and keyring added as optional [security] extra:

pip install mempalace[security]

Core mempalace continues to work with zero extra dependencies.

Test plan

  • All 151 tests pass (101 original + 50 new)
  • Auth: requests without token rejected (-32001), valid token succeeds, backward compat when auth disabled
  • Encryption: plaintext NOT stored in documents field, encrypted_content in metadata, search returns decrypted text, unencrypted drawers still readable
  • Input validation: missing required fields rejected, unknown fields rejected, size limits enforced
  • File permissions: 0o600 on files, 0o700 on directories
  • Localhost guard: rejects 0.0.0.0 and public IPs, accepts 127.0.0.1/::1/localhost
  • SHA-256 hashing produces correct output
  • Ruff lint and format clean

🤖 Generated with Claude Code

jessecurry and others added 8 commits April 7, 2026 19:34
… hashing

- Create mempalace/security.py centralizing all security primitives
- Replace MD5 with SHA-256 for ID generation across 5 call sites
- Add secure_file (0o600) and secure_dir (0o700) permission helpers
- Apply restrictive permissions to config dir, config files, and SQLite DB
- Add localhost-only bind address validation for future HTTP transport
- Add security config section (auth_enabled, encryption_enabled, max_content_size)
- Support env var overrides (MEMPALACE_AUTH_ENABLED, MEMPALACE_ENCRYPTION_ENABLED)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Validate required fields are present before dispatching tool calls
- Reject unknown fields not defined in tool input_schema
- Enforce configurable content size limits on write operations
- Strip _meta from tool_args before handler dispatch (reserved for auth)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Auth tokens stored in OS keychain via keyring (macOS Keychain, Windows
  Credential Manager, Linux Secret Service) with file fallback
- Constant-time token verification via hmac.compare_digest
- Auth check on tools/list and tools/call, initialize always open
- initialize response includes authRequired: true when auth is enabled
- All auth features opt-in via config security.auth_enabled

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Encryption keys stored in OS keychain via keyring (macOS Keychain,
  Windows Credential Manager, Linux Secret Service) with file fallback
- Encrypted content stored in ChromaDB metadata field encrypted_content
- Documents field keeps plaintext for embedding/search to work
- Transparent decrypt on retrieval in MCP tools and searcher
- Knowledge graph entity properties encrypted when enabled
- Backward compatible: unencrypted drawers still readable
- cryptography and keyring added as optional [security] dependency

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- start_http_server() validates bind address is localhost before binding
- Rejects 0.0.0.0, public IPs, and non-localhost addresses
- HTTP transport config support (transport: "http" in security config)
- Currently raises NotImplementedError — stdio remains the only transport
- Defensive scaffolding to prevent accidental network exposure

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add .env, *.key, auth_token to prevent secret leaks
- Add *.sqlite3, entities.json (may contain PII)
- Add venv/, .venv/, IDE configs, test coverage artifacts

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused variable in test_search_returns_decrypted_content
- Apply ruff formatter to searcher.py, test_mcp_server.py, test_security.py

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

Critical:
- Store content hash placeholder in ChromaDB documents field when encryption
  is enabled instead of plaintext — encrypted_content in metadata is now the
  only copy of the real content

High:
- Add logging to keyring helpers — separate ImportError from other exceptions
  so keyring failures leave a diagnostic trail instead of silently falling back
- Secure config directory (0o700) in token/key file fallback paths
- Remove file paths from warning log messages

Medium:
- Decrypt failures now return clear error markers instead of silently
  falling back to raw documents/empty data
- Knowledge graph _decrypt_props returns empty JSON on failure, not ciphertext

Low:
- Remove timing leak: eliminate short-circuit before hmac.compare_digest
  in auth token check

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

bgauryy commented Apr 8, 2026

Copy link
Copy Markdown

PR Review: feat(security): add security hardening — auth, encryption, validation

Executive Summary

Aspect Value
PR Goal Add opt-in security hardening — token auth, Fernet encryption at rest, input validation, SHA-256 hashing
Files Changed 12 (+1098 / -42)
Risk Level 🔴 HIGH - Core data pipeline changed (hashing, encryption, search), critical correctness issues found
Review Effort 4/5 - Complex security feature touching multiple modules
Recommendation 🔄 REQUEST_CHANGES

Affected Areas: security.py (new), mcp_server.py, config.py, searcher.py, knowledge_graph.py, miner.py, convo_miner.py

Business Impact: Encrypted content becomes unsearchable via semantic search. Re-mining after upgrade creates duplicate drawers.

Flow Changes: All drawer writes now use SHA-256 IDs (was MD5). MCP server optionally encrypts content at rest and requires auth tokens. Search path gains decryption layer.

Ratings

Aspect Score
Correctness 2/5
Security 4/5
Performance 4/5
Maintainability 3/5

PR Health

  • Has clear description
  • References ticket/issue (if applicable)
  • Appropriate size (or justified if large) — 1098 additions across 12 files; could split hashing/auth/encryption
  • Has relevant tests (228 + 292 + 39 new test lines)

High Priority Issues

(Must fix before merge)

🐛 #1: Encryption destroys semantic search quality

Location: mempalace/mcp_server.py:313-316 | Confidence: ✅ HIGH

When encryption is enabled, tool_add_drawer stores a placeholder [encrypted:<hash>] as the ChromaDB document. ChromaDB generates embeddings from the documents field at add() time. This means all encrypted drawers get embeddings based on the placeholder string, not the actual content — making semantic search return essentially random results.

The test test_search_returns_decrypted_content passes only because it inserts a single document and ChromaDB returns top-N regardless of similarity score.

Fix: Store the plaintext in documents for embedding generation, and only encrypt in metadata. ChromaDB's persistent storage will contain plaintext embeddings (vector numbers, not recoverable text), but the documents field on disk will contain plaintext. Alternative: generate embeddings separately and use col.add(embeddings=[...]) with the plaintext embedding, then store the placeholder.

- doc_content = f"[encrypted:{content_hash(content, length=32)}]"
+ # ChromaDB needs plaintext for embedding generation.
+ # Encrypt in metadata only; embeddings are not reversible to plaintext.
+ doc_content = content

Note: This is a fundamental design tension — true encryption at rest conflicts with ChromaDB storing documents as plaintext alongside embeddings. A proper solution may require encrypting the ChromaDB storage directory at the filesystem/OS level rather than per-document encryption.


🐛 #2: _decrypt_props defined but never called — encrypted KG properties silently unreadable

Location: mempalace/knowledge_graph.py:110-130 | Confidence: ✅ HIGH

add_entity() now encrypts properties via _encrypt_props(), but query_entity() and all read paths never call _decrypt_props(). Entity properties written with encryption enabled will be returned as raw Fernet ciphertext (or {} if fernet is available but decryption context is lost).

  # In query methods that read entity properties, add:
  props_str = row["properties"]
- # currently: properties are returned raw (ciphertext)
+ props_str = self._decrypt_props(props_str)

🐛 #3: MD5→SHA-256 hash change breaks deduplication for existing data

Location: mempalace/miner.py:418, mempalace/convo_miner.py:359, mempalace/mcp_server.py:303 | Confidence: ✅ HIGH

All drawer ID generation switched from hashlib.md5(...).hexdigest()[:16] to content_hash(...) (SHA-256). Since drawer IDs are content-addressed, re-mining the same project after this upgrade will produce different IDs for the same content, creating duplicate drawers in the palace.

Fix: Either (a) add a migration step that rehashes existing IDs, (b) keep MD5 for backward-compatible ID generation and use SHA-256 only for new security features, or (c) document the breaking change and provide a mempalace rebuild command.


🏗️ #4: CLI mining path has no encryption support

Location: mempalace/miner.py, mempalace/convo_miner.py | Confidence: ✅ HIGH

miner.py and convo_miner.py store content directly via collection.add(documents=[content]) without any encryption. When a user enables encryption in config, only MCP server writes encrypt data — CLI mining (mempalace mine) stores everything in plaintext. This creates an inconsistent security posture.

  # miner.py add_drawer() should check encryption config
  # and encrypt content if enabled, matching the MCP server behavior

Medium Priority Issues

(Should fix, not blocking)

🏗️ #5: Private config internals accessed directly in main()

Location: mempalace/mcp_server.py:889-892 | Confidence: ✅ HIGH

main() accesses _config._file_config.get("security", {}) directly to read transport, http_host, and http_port. This bypasses the config layer and couples to internal implementation. These should be config properties like auth_enabled.

- transport = _config._file_config.get("security", {}).get("transport", "stdio")
- host = _config._file_config.get("security", {}).get("http_host", "127.0.0.1")
- port = _config._file_config.get("security", {}).get("http_port", 8766)
+ transport = _config.transport
+ host = _config.http_host
+ port = _config.http_port

🔗 #6: Encrypt-then-placeholder pattern duplicated

Location: mempalace/mcp_server.py:310-316 and mempalace/mcp_server.py:421-427 | Confidence: ✅ HIGH

tool_add_drawer and tool_diary_write have identical encrypt-and-placeholder logic. Extract into a shared helper:

def _prepare_content(content, metadata):
    """Encrypt content if encryption is enabled, updating metadata in-place."""
    if _fernet:
        metadata["encrypted_content"] = encrypt(_fernet, content)
        return f"[encrypted:{content_hash(content, length=32)}]"
    return content

🚨 #7: Decryption failure in KG returns empty properties, silently losing data

Location: mempalace/knowledge_graph.py:120-130 | Confidence: ⚠️ MED

_decrypt_props() catches all exceptions and returns "{}" when decryption fails. This silently drops entity properties with only a log message. A user with the wrong key would see empty entities with no indication of data loss in the API response.

Consider raising an explicit error or including a "_decryption_failed": true marker in the returned properties dict.


Low Priority Issues

(Nice to have)

🎨 #8: _check_auth on tools/list may break MCP client discovery

Location: mempalace/mcp_server.py:786-788 | Confidence: ⚠️ MED

Requiring auth on tools/list means an MCP client cannot discover available tools before authenticating. The MCP spec typically allows tools/list without auth so clients can present tool information to users. Consider allowing unauthenticated tools/list calls.


🎨 #9: KnowledgeGraph.__init__ now requires fernet parameter for encryption

Location: mempalace/knowledge_graph.py:56 | Confidence: ⚠️ MED

The _kg = KnowledgeGraph() singleton in mcp_server.py is created at module load time (line 31) without a fernet argument. If encryption is enabled, _fernet is only set later in main(). The KG instance will never have encryption capability unless it's re-created after main() sets _fernet. Entity property encryption in the MCP server is silently disabled.

  # In main(), after loading fernet:
  if _config.encryption_enabled:
      _fernet = load_or_create_key(_config._config_dir)
+     _kg._fernet = _fernet  # Enable KG encryption

Created by Octocode MCP https://octocode.ai

@bensig

bensig commented Apr 11, 2026

Copy link
Copy Markdown
Contributor

Thanks for the security work. This conflicts with main and the core security hardening landed in #387. If there are specific pieces not covered, happy to look at a focused follow-up PR.

@bensig bensig closed this Apr 11, 2026
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.

3 participants