Conversation
The stored credential format concealed a value but did not authenticate it: anyone able to write to the database could alter a stored username or password without the change being detectable on read. Switch to AES-256-GCM and give stored values a version marker so the format can change again later without a flag day. Values written by earlier versions stay readable indefinitely, not as a courtesy but because a backup archive is swapped in before the database is opened — a value in the old format can therefore appear on any future boot. Existing installs convert on the first start. The conversion copies the database and the key file aside before writing anything, keeps them as a pair (the credentials are worthless without the key), and refuses to write at all when nothing decrypts, which is what a mismatched key file looks like. A single unreadable value is left alone rather than dropped. Failure is logged and the boot continues: both formats read, so an unconverted install keeps working instead of losing fan control at startup. Also stop the connection-test and fan-mode endpoints from answering an unreadable credential with a server error while an unreachable BMC gets an ordinary failure — that difference distinguished the two cases for anyone probing.
Enabling https previously required generating a certificate by hand first; if you turned it on without one, the embedded serve path fell back to plain HTTP silently. So the setting looked available but the honest default stayed cleartext, with the session cookie and every BMC password on the wire. Now a certificate is created on demand when https is on and none is configured, and both serve paths resolve TLS through the same helper so they cannot drift apart again. Certificates the operator supplies are never overwritten, and generation failure falls back to plain HTTP with the reason logged: locking someone out of their own dashboard over a certificate problem is worse than the cleartext they already had. The generated certificate now also covers the machine's hostname and addresses, not just localhost. A dashboard is reached at whatever the operator typed, and a name mismatch is a hard browser failure with no way to click through — unlike the unknown issuer warning, which is expected and documented. Marking it a CA is what allows removing that warning by importing the file, since macOS and Windows will not anchor trust on a plain end-entity certificate. Adds environment overrides for the three transport settings, which is the only way a container can turn TLS on, and makes the startup log state the scheme.
The telemetry socket checked authorisation once, at the handshake, and never again. A connection can stay open for days, so a session that expired — or was ended by a password change — kept streaming to whoever held the socket. It is now re-checked once a minute and closed when it no longer holds; the client sees the policy close and sends the operator back to the login page instead of retrying forever. The handshake also accepted any Origin. That is not covered by the same-origin policy, so any page the operator happened to visit could open a socket here and it would carry their cookie. The stated origin is now required to match the Host the request was addressed to, which needs no configuration and survives being reached by address, hostname or a different port. A request with no Origin is still accepted: command-line clients and the container health check send none, and rejecting them would break every non-browser caller to stop something only a browser can do. Login and the two endpoints that verify a password to change the authentication settings are now capped at five attempts a minute per source. The disable and reconfigure paths had no counter at all behind them, which made either one an unlimited guessing oracle for anyone holding a session. A slot is consumed even when the password turns out to be right, or a caller who already knows it would face no limit; the rejection reuses the existing lockout wording exactly, so it cannot be told apart from an account that is locked — which would itself reveal the account exists. Password verification now runs a throwaway comparison when the username has no row. bcrypt is slow on purpose, so returning early made an unknown name answer in well under a millisecond against tens for a real one — a difference that reads off the wire and names the only valid account without guessing a password. The rate cap alone only lowers the sample rate; it does not remove the difference. The two attacker-keyed tables that back all this are now bounded and their keys capped, since the login endpoint is reachable without a session and both the number of entries and the length of each key were caller-chosen.
Every parser read as many entries as the subprocess handed back, and the subprocess output itself had no byte limit — only a wall-clock timeout, which at LAN speed is hundreds of megabytes. A BMC that is broken, hostile, or simply answering with something other than what was asked could turn one poll cycle into unbounded memory, and the sensor payload is cached and broadcast in full, so it stayed resident and was replayed to every client that connected afterwards. The caps are four to fifty times the worst legitimate case and each one logs when it bites, naming the limit. That matters more than the number: a silently dropped sensor could make a fan curve's source disappear and trip the fail-safe on a healthy machine, so an operator with unusual hardware needs to be able to see why and raise it. The byte cap is applied after the read completes, not during it. Bounding the read would mean replacing the pipe handling wholesale for a case the existing timeout already covers; this bounds everything downstream — parsers, database rows, the broadcast cache, error strings — which is where the memory actually stayed. Deleting a server also now clears the dozen module-level trackers keyed by its id. The database rows went with the delete but none of the in-memory state did, so a removed server kept a fan controller, poll timers and counters alive for the process lifetime. The cached telemetry was the worst of it: a server the operator deleted kept reappearing on the dashboard of every newly connected client, because the existing clear-cache call had never had a caller. Two smaller leaks close with it: the power rate-limit entry was recorded before checking the server existed, so a request naming a server that does not exist left a permanent entry, and moving a server to a different address stranded the old address's command lock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this closes
Five structural weaknesses the security audit deferred rather than fixed. Each is a separate
commit and each was exercised against a real running server, not reasoned about.
9eaa9cc8d86fef9e0607be1e5b64Authenticated encryption for stored credentials
The old format concealed a value but did not authenticate it, so anyone able to write to the
database could alter a stored credential undetectably. Values now use AES-256-GCM and carry a
version marker.
Existing installs convert on the first start. Nothing is re-entered. Before writing
anything the conversion copies
ipmideck.dbandencryption.keyaside as*.pre-authenc-<timestamp>.bak; those copies are credential-grade and the changelog saysso. Values written by older versions stay readable indefinitely — a backup archive is swapped
in before the database is opened, so an old-format value can appear on any future boot.
If nothing decrypts, the key does not belong to that data and nothing is written at all; a
single unreadable value is left alone rather than dropped; a failure logs and the boot
continues, because both formats read and losing fan control at startup would be worse.
Also: an unreadable credential no longer answers with a server error while an unreachable BMC
gets an ordinary failure — that difference distinguished the two for anyone probing.
HTTPS without manual setup
This one turned out differently than planned. HTTPS already existed — config keys, a CLI flag,
a generator, uvicorn wiring, a Settings card. What was missing:
localhost, and a name mismatch is a hard browser failure withno click-through — unlike the unknown-issuer warning, which is expected
A certificate is now created on demand, covers the machine's hostname and addresses, and is
marked so it can be imported into a trust store (macOS and Windows will not anchor trust on a
plain end-entity certificate). Generation failure falls back to plain HTTP with the reason
logged — being locked out of your own dashboard over a certificate problem is worse than the
cleartext you already had.
No UI was added and the setup wizard was not touched.
WebSocket lifetime and origin
The socket authorised once at the handshake and never again. Connections stay open for days,
so an expired session — or one ended by a password change — kept streaming. It is re-checked
once a minute and closed; the client sees the close code and goes to the login page instead of
retrying forever.
The handshake also accepted any Origin, which is not covered by the same-origin policy: any
page the operator visited could open a socket here carrying their cookie. The origin must now
match the Host the request was addressed to. A request with no Origin is still accepted —
command-line clients and the container health check send none.
Rate cap on credential verification
Login and the two endpoints that change authentication settings are capped at five attempts a
minute per source. The disable and reconfigure paths previously verified a password with no
counter of any kind behind them, making either an unlimited guessing oracle for anyone holding
a session.
A slot is consumed even when the password is right, or someone who already knows it would face
no limit. The rejection reuses the existing lockout wording exactly, so it cannot be told
apart from a locked account — which would itself reveal the account exists.
Password verification now also runs a throwaway comparison when the username has no row.
bcrypt is slow on purpose, so returning early made an unknown name answer in under a
millisecond against tens for a real one — a gap that reads off the wire and names the only
valid account. A rate cap lowers the sample rate; it does not remove the difference.
Resource bounds
Every parser read as many entries as handed back, and subprocess output had no byte limit —
only a wall-clock timeout, which at LAN speed is hundreds of megabytes. The sensor payload is
cached and broadcast in full, so it stayed resident and was replayed to every later client.
Caps are 4–50× the worst legitimate case and each logs when it bites, naming the limit.
That matters more than the number: a silently dropped sensor could make a fan curve's source
disappear and trip the fail-safe on a healthy machine.
Deleting a server now clears the dozen trackers keyed by its id. The cached telemetry was the
worst of it — the existing clear-cache method had never had a caller, so a deleted server kept
reappearing on the dashboard of every newly connected client.
Verification
Every criterion was exercised against a real uvicorn on a
/tmpdata directory, attacked fromoutside the process with raw sockets and plain HTTP.
60-character one and one with a significant trailing space — converted, all four still
decrypt to the originals, the copies hold the old format as a rollback point, and a second
boot changes nothing.
refused; the certificate's names inspected; the untrusted-issuer warning verified to
actually happen, so the documentation is proven rather than asserted; restart reuses the
certificate; plaintext still works.
host, since cookies are shared across ports); absent Origin accepted; a held socket closed
with code 1008 at the revalidation interval.
correct password throttled too; the disable endpoint capped mid-guess.
at 4 MiB; a genuine 160-sensor dump untouched; the server still healthy after 300 logins
with 1,400-character usernames.
One correction is worth mentioning, because it nearly passed. The WebSocket proof first
reported a close at 40s with code 1011 — which looks like a pass. It was uvicorn dropping
an unresponsive peer, not the revalidation: the raw client was not answering keepalive pings.
Asserting the close code rather than merely "it closed" is what caught it. Corrected, the
real behaviour is 1008 at 60s.
Gate
pytestdev→ +52)ruff check .npm run testtsc --noEmitbackend/staticrebuilt (frontend was touched)Documentation impact
README.md— the at-rest encryption bullet now says GCM and mentions the automaticconversion; a new Security → HTTPS section covers why the browser warns, per-platform
import instructions (Windows/macOS/Linux/Firefox), supplying your own certificate, the
reverse-proxy alternative, regeneration, and that
server.keyis credential-grade.CHANGELOG.md— the format change, the automatic conversion, what the.bakfiles areand that they are credential-grade, and the downgrade path.
config.example.yaml— thehttpskey documented, including automatic generation.self-signed certificate generation" — still true, and now it also works without the click.
ipmideck/docsfor this branch; the README carries the operator-facingdetail. If you'd like the HTTPS section mirrored to
en/there, say so and I'll open thelinked PR.
Deliberately left open
Stated here rather than left to be discovered:
timeout remains the bound during it. Bounding the read means replacing the pipe handling and
rewriting six lifecycle tests whose fake process exposes no streams. The commit says so.
the CLI. The environment overrides here are the prerequisite; the entrypoint and its
hardcoded
http://health check belong with the container work.lever is
FORWARDED_ALLOW_IPS, which uvicorn already reads — no custom header parsing wasadded, since uvicorn's is correct including CIDR.
X-Forwarded-Forfor a fresh bucket. Pre-existing uvicornbehaviour, but it bounds what the limiter can promise.
rather than memory, and capping them changes a user-visible default — their own finding.
Draft, as agreed: yours to merge, test, then
main+ tag.