Conversation
Agent-Logs-Url: https://github.com/etnt/cryptic/sessions/72a58939-24af-4b65-ab35-ea8bca6477ed Co-authored-by: etnt <5860+etnt@users.noreply.github.com>
Agent-Logs-Url: https://github.com/etnt/cryptic/sessions/72a58939-24af-4b65-ab35-ea8bca6477ed Co-authored-by: etnt <5860+etnt@users.noreply.github.com>
Agent-Logs-Url: https://github.com/etnt/cryptic/sessions/72a58939-24af-4b65-ab35-ea8bca6477ed Co-authored-by: etnt <5860+etnt@users.noreply.github.com>
Agent-Logs-Url: https://github.com/etnt/cryptic/sessions/72a58939-24af-4b65-ab35-ea8bca6477ed Co-authored-by: etnt <5860+etnt@users.noreply.github.com>
Agent-Logs-Url: https://github.com/etnt/cryptic/sessions/72a58939-24af-4b65-ab35-ea8bca6477ed Co-authored-by: etnt <5860+etnt@users.noreply.github.com>
Agent-Logs-Url: https://github.com/etnt/cryptic/sessions/72a58939-24af-4b65-ab35-ea8bca6477ed Co-authored-by: etnt <5860+etnt@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new localhost-only Cowboy HTTP listener that exposes an MCP-facing admin API under /mcp/v1/admin/..., backed by existing CA admin operations in cryptic_ca_store.
Changes:
- Added a dedicated Cowboy cleartext listener (
cryptic_mcp_listener) bound to127.0.0.1and controlled viamcp_tcp_enabled/mcp_tcp_port. - Introduced
cryptic_mcp_admin_handlerimplementing REST-like JSON endpoints for admin operations (users + certificates). - Added default configuration entries for the MCP listener in
cryptic.app.srcandconfig/sys.config.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/cryptic_server.erl | Starts/stops the new cryptic_mcp_listener and wires routes to the MCP admin handler. |
| src/cryptic_mcp_admin_handler.erl | Implements the MCP admin HTTP API surface and maps requests to CA store operations. |
| src/cryptic.app.src | Adds default app env settings for the MCP TCP listener (disabled by default). |
| config/sys.config | Adds system config defaults for the MCP TCP listener (disabled by default). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GpgFp = maps:get(<<"gpg_fp">>, BodyMap), | ||
| GpgPub = maps:get(<<"gpg_pub">>, BodyMap), | ||
| Metadata = maps:get(<<"metadata">>, BodyMap, null), | ||
| case cryptic_ca_store:register_user(DbRef, GpgFp, GpgPub, AdminFp, Metadata) of |
There was a problem hiding this comment.
register_user passes metadata from the decoded JSON body straight into cryptic_ca_store:register_user/5. With jsx:decode(..., [return_maps]), metadata will be an Erlang map when the client sends a JSON object, but the store API expects JSON-encoded binary (or undefined). This will likely make the DB insert fail (esqlite bind can’t store maps) and also diverges from the existing CA WebSocket admin implementation which encodes metadata before storing it. Consider normalizing metadata here (undefined when missing/null; jsx:encode/1 when it’s a map; accept binary as-is).
| handle_post_operation(<<"revoke_certificate">>, BodyMap, AdminFp, DbRef, Req) -> | ||
| with_required_fields( | ||
| BodyMap, | ||
| [<<"serial">>, <<"reason">>], | ||
| fun() -> | ||
| Serial = maps:get(<<"serial">>, BodyMap), | ||
| Reason = maps:get(<<"reason">>, BodyMap), | ||
| case cryptic_ca_store:revoke_certificate(DbRef, Serial, AdminFp, Reason) of | ||
| ok -> | ||
| {200, #{ | ||
| type => <<"revoke_certificate_response">>, | ||
| status => <<"success">>, | ||
| serial => Serial, | ||
| reason => Reason | ||
| }, Req}; |
There was a problem hiding this comment.
revoke_certificate relies on cryptic_ca_store:revoke_certificate/4, which writes its own audit log entry with ip_address = undefined. Since this handler otherwise captures peer_ip/1 on mutation paths, certificate revocations currently won’t include the caller IP in the audit trail. If the goal is to have peer IP for admin state transitions, consider either extending the store API to accept ip_address (and store it) or inserting a separate audit entry from the handler after a successful revocation.
| get_admin_fingerprint(Req, BodyMap) -> | ||
| %% Priority order: explicit header, then JSON body. | ||
| case cowboy_req:header(<<"x-admin-gpg-fp">>, Req) of | ||
| undefined -> | ||
| case maps:get(<<"admin_gpg_fp">>, BodyMap, undefined) of | ||
| undefined -> | ||
| undefined; | ||
| Fp -> | ||
| Fp | ||
| end; | ||
| Fp -> | ||
| Fp | ||
| end. | ||
|
|
||
| is_admin(undefined, _DbRef) -> | ||
| false; | ||
| is_admin(GpgFp, DbRef) -> | ||
| %% Bootstrap/root admins are identities with no registered_by owner. | ||
| case cryptic_ca_store:get_gpg_identity(DbRef, GpgFp) of | ||
| {ok, #gpg_identity{registered_by = undefined}} -> true; | ||
| _ -> false | ||
| end. |
There was a problem hiding this comment.
Admin authorization is based solely on a caller-supplied fingerprint (x-admin-gpg-fp header or JSON body) plus a DB lookup. Because there’s no cryptographic proof (mTLS, HMAC’d token, etc.), any local process that can reach 127.0.0.1 can impersonate an admin by sending a known bootstrap admin fingerprint. Consider adding an actual authentication factor for this listener (e.g., require a configured shared-secret/Bearer token, use a Unix domain socket with filesystem permissions, or enable TLS client cert auth even on localhost) and validating the fingerprint format (length/hex) before DB lookup.
This PR introduces a new MCP-facing REST-like admin surface equivalent to the existing admin API behavior, and exposes it via a dedicated Cowboy TCP listener bound to localhost only.
New MCP admin API surface
cryptic_mcp_admin_handlerto provide REST-like endpoints for admin operations already supported in server logic:list_usersregister_usersuspend_userrevoke_userreactivate_userget_user_infolist_certificatesrevoke_certificate/mcp/v1/admin/....Dedicated localhost TCP endpoint in Cowboy
cryptic_mcp_listener) incryptic_server.127.0.0.1and a configurable port (mcp_tcp_port), with explicit lifecycle wiring (start + stop).Configuration and defaults
mcp_tcp_enabledandmcp_tcp_portto app/sys config.Admin/authn/authz and audit behavior
registered_by = undefined).x-admin-gpg-fp) or JSON body.Security/error handling hardening
internal_server_error/operation_failed) instead of exposing internal terms.{ok, _} = cowboy:start_clear( cryptic_mcp_listener, [ %% Security boundary: bind admin MCP endpoint to localhost only. {ip, {127, 0, 0, 1}}, {port, Port} ], #{env => #{dispatch => Dispatch}} ).