Mailbox impl - #1395
Conversation
c35eb01 to
e4e4d03
Compare
|
Comment from maintainers call on 5/5: healthKERI is potentially contributing their Courier service and we need to sync this with that. |
There was a problem hiding this comment.
Thanks for building this out, @kentbull — a standalone mailbox host is a real step forward, and the cooperative-doer structure is clear. I used an AI to review. After checking its findings, I think the feature is a good step forward, but I have some picky stuff to ask about since this component has security implications. Verbiage below is excerpted from my AI's findings, which is why it sounds so cocky/harsh.
All four blockers sit on the security/correctness boundary:
- Silent TEL/ACDC drop. The host returns HTTP 204 for TEL/ACDC ilks, but no
Teveryis wired intosetupMailbox, so those events are appended toparser.imsand discarded — the 204 tells the sender "stored" when it wasn't. Wire a Tevery, or reject with a non-204 (mailboxing.py:263-270). - The authorization gate is untested.
AuthorizedForwardHandler— the allow/reject logic that is the whole point of this PR — has no tests, so a regression in it reaches CI undetected (forwarding.py:559). - The OOBI mailbox-role branch is untested.
OOBIEnd.on_get()'s new branch (how a recipient advertises its mailbox) has no coverage for the authorized, unauthorized, or AID-absent paths (ending.py:596-601). - Two different authz predicates, undocumented. Role activation is gated on
end.allowedin one place andend.allowed or end.enabledin others, with no comment on which is correct where. Whoever normalizes these later will silently move a trust boundary. Please pick one and document why (mailboxing.py:187,forwarding.py:564,ending.py:598).
Two mediums are worth pulling forward: the unauthenticated admin on_post ingests unbounded attacker CESR into the shared LMDB before the 403 auth check (mailboxing.py:451) — I'd rate that one HIGH; and a validly-signed /fwd exn with a malformed q raises an uncaught KeyError (forwarding.py:562). Both are small fixes.
Two more have no line to anchor to: the mailbox CLI test dir (tests/app/cli/commands/mailbox/) is missing an __init__.py — every sibling command-test dir has one, and without it a same-named test module elsewhere can silently shadow one of these. And there's no benchmark baseline for what is keripy's first standalone service role; fine to defer, but the perf notes below can't be validated without one.
The rest are deferrable perf and cleanup nits: redundant startup DB reads (start.py:181); a fresh Parser plus full escrow sweep per admin on_post (mailboxing.py:144); unbounded Deck queues under stalled SSE consumers (mailboxing.py:317, matching the existing WitnessStart pattern); vestigial datetime/source fields in the startup dict (start.py:188).
| cues=cues) | ||
| kvy.registerReplyRoutes(router=rvy.rtr) | ||
|
|
||
| parser = parsing.Parser(framed=True, |
There was a problem hiding this comment.
[blocker] Silent TEL/ACDC discard — the 204 is a lie. HttpEnd returns HTTP 204 for TEL/ACDC ilks, but no Tevery is wired into setupMailbox, so these events land in parser.ims and are dropped. A sender that gets 204 believes its credential/registry event was stored when it wasn't. Either wire a Tevery here so they're actually processed, or return a non-2xx status for ilks the host doesn't handle so the sender knows.
| expected state. | ||
| """ | ||
| end = hby.db.ends.get(keys=(cid, Roles.mailbox, mailboxAid)) | ||
| accepted = bool(end and (end.allowed if expected else not end.allowed)) |
There was a problem hiding this comment.
_confirmRoleAuth short-circuits to HTTP 403 when a cut is requested and no EndpointRecord exists. That makes idempotent revocation fragile: a caller unsure whether a mailbox was ever added can't safely issue a cut. A first-ever/redundant cut should succeed (or no-op), not 403.
| return f"{path}/mailboxes" | ||
|
|
||
|
|
||
| def _roleEnabled(hby, cid, role, eid): |
There was a problem hiding this comment.
_roleEnabled is defined identically here and in cli/commands/mailbox/start.py:175. Since it encodes an authorization semantic, the duplicate will drift the moment that semantic changes. Make one canonical (here in mailboxing.py) and import it in start.py.
| self.exc.processEscrow() | ||
| yield | ||
|
|
||
| def cueDo(self, tymth=None, tock=0.0, **kwa): |
There was a problem hiding this comment.
The cooperative-doer wiring (cueDo/msgDo/escrowDo) has no test in a running-controller context. The module docstring itself warns that broken cue wiring silently breaks SSE delivery while storage still appears to work — exactly the kind of failure a test should pin. Worth a small integration test that drives a cue through to an SSE response.
| module; served OOBIs still come from ``loadEndingEnds(...)`` at their | ||
| normal root routes. | ||
| """ | ||
| from .indirecting import createHttpServer, HttpEnd |
There was a problem hiding this comment.
This deferred (in-function) import has no comment. It reads like circular-import protection, but with no note a future refactor will either delete it and break imports, or preserve it as cargo-cult. One line saying which cycle it breaks would save that.
| elif match := owits.intersection(self.hby.prefixes): # We are a witness for identifier | ||
| pre = match.pop() | ||
| hab = self.hby.habs[pre] | ||
| elif role == Roles.mailbox and eid in self.hby.prefixes: |
There was a problem hiding this comment.
[blocker] The new mailbox-role branch is untested. This branch is how a recipient advertises its mailbox to senders, i.e. part of the trust boundary. No test covers the authorized path, the unauthorized path, or the case where the mailbox AID isn't in hby.prefixes. A regression here would be invisible to CI.
| super().__init__(hby=hby, mbx=mbx) | ||
| self.mailboxAid = mailboxAid | ||
|
|
||
| def handle(self, serder, attachments=None): |
There was a problem hiding this comment.
[blocker] The authorization gate has no tests. AuthorizedForwardHandler is the reject-unauthorized-forwards logic that is the stated point of this PR, but nothing exercises the allow path or the reject path. Please add tests for both — this is the one thing most important to keep from regressing.
| """Store the forwarded payload only when the hosted mailbox is allowed.""" | ||
| modifiers = serder.ked.get("q", {}) | ||
| recipient = modifiers["pre"] | ||
| end = self.hby.db.ends.get(keys=(recipient, Roles.mailbox, self.mailboxAid)) |
There was a problem hiding this comment.
Minor/defer: handle() does one LMDB ends.get() per inbound /fwd message, though authorization state only changes on admin add/remove. A process-local dict cache keyed by recipient AID (invalidated from the admin endpoint) removes the per-message read — but establish a benchmark baseline first so the cache-invalidation complexity is justified by numbers.
|
|
||
| # Capture the outbound multipart request without needing a live mailbox | ||
| # server, so the test can assert the exact admin envelope the CLI builds. | ||
| class ClientStub: |
There was a problem hiding this comment.
Defer: ClientStub and WitnessPublisherStub are duplicated verbatim across the two tests. Fine as-is for now; worth lifting to module-level fixtures once a third mailbox-CLI test lands in this dir.
Signed-off-by: Kent Bull <kent@kentbull.com>
Signed-off-by: Kent Bull <kent@kentbull.com>
|
Kent: I'm doing some stuff in a codebase that consumes keripy, and this PR caught my eye again because mailbox code might be useful there, so I thought I'd re-study this PR a bit. Like my review process on your PR #1614, this is deeper water than I'm really qualified to swim in at the moment, so I did some AI analysis but didn't totally trust myself to adjudicate it, so part 2 of my process was to write some tests that proved or disproved the findings I was coming up with. Bottom line is that I found some small stuff, plus one thing that I think is substantive that's worth discussing. And as with the other PR, I've raised a PR against your PR so you can have tests that demonstrate the issues I'm reporting. I don't necessarily think you should merge my PR, but maybe use it as a reference tool? Here's what I found that seemed worth sharing back. The host doesn't store forwarded messages
I reproduced this in a harness that mirrors your wiring exactly (same
The last row is the control — same message, same handler, stores fine. I don't think this is your bug, and I don't think this PR should have to fix it. What does seem fair to raise here is that this is the first component whose entire value is
|
This is a mailbox implementation that:
kli mailbox add/fwdmessages are only supported for recipient AIDs where the mailbox hab has a supporting end role auth for that recipientAID withAuthorizedForwardHandler.MailboxAddRemoveEndHTTP endpoint, compatible withkli mailbox addkli mailbox removedoes not exist yet though when it does theMailboxAddRemoveEndwill be ready for it.delkelinkli mailbox add: Delegated AIDs can send mailbox add request withkli mailbox add--nameand--aliasdoes not exist.AI Sourcing Note:
Codex assisted creation of this PR. I reviewed everything and adjusted things here and there, though it was drafted by Codex with my prompts.