Repository navigation
fix(oi): four self-contained audit findings - #179
Merged
Merged
Conversation
`i[status.get]` defines `active_operations` as the number of lifecycle operations currently in progress; the handler emitted the literal `0` regardless of scheduler state, while `state.scheduler` was right there and used by other handlers in the same crate. Anything watching the summary saw an idle runtime however busy it was. One operation is active at a time, so this is 0 or 1 — queued operations are waiting rather than in progress and are not counted.
Fingerprints are lowercase-hex SHA-256 of the client SPKI and the verifier compares byte-for-byte, but `/keys/authorise` stored whatever it was given. An uppercase, whitespace-padded, `sha256:`-prefixed or wrong-length value was accepted and then listed as authorised while no client could ever match it — telling an operator they had granted access they had not. `keys::parse_fingerprint` normalises what can be and rejects what cannot, with `requirements_invalid` rather than a stored non-key. `/keys/revoke` uses it too, so a key authorised from a prefixed or uppercase paste can be revoked with the string the operator actually typed. The existing key_mgmt tests used `"aabbcc"` as a stand-in fingerprint, which is one of the values this now refuses; they use a full 64-character digest instead.
`service_port`, `remote_port` and an attachment's `port` are plain `u16`, so 0 deserialised happily. It is not a routable listener or backend port, and it reached the site-proxy config as `listen :0` / dial `:0` semantics rather than being refused at the interface where the operator could still be told which field was wrong. The BSL side has rejected it all along via `Port::new`. The check lives in `handler` rather than in either module, since both needed it.
…attern `set-acme-dns` left an unknown `dns_provider` to the foreign key, and `db_error` maps every DB error to `not_found`, so an operator typo came back as `not_found: db error: FOREIGN KEY constraint failed` — naming neither the provider nor the remedy, and handing a client the wrong error code to key off. The delete paths in the same file already special-case FK refusals into clear messages. The existence check runs inside the same DB closure, so nothing can remove the provider in between. The handler also accepted any `hostname`, including an empty string, storing a policy that could never match and that looked identical in the policy list to one that works. `r[tls.policy.wildcard]` defines the shapes, and those are what it accepts now. The DNS-name shape check moves to `handler` so both this and the site-service host validation use it; the `localhost` rejection stays with site services, because an exact TLS policy may deliberately name it.
Contributor
Code Coverage OverviewLanguages: TypeScript, Rust TypeScript / code-coverage/vitestThe overall line coverage in commit 29fce78 in the Rust / code-coverage/rustThe overall line coverage in commit 29fce78 in the Show a line coverage summary of the most impacted files.
Updated |
Merged
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.
🤖 Four trivial findings from the logic-bug audit in the OI handlers, one commit each.
/server/statusreported a constantactive_operations: 0.i[status.get]defines it as the number of lifecycle operations in progress, andstate.schedulerwas right there and used by other handlers in the same crate. Anything watching the summary saw an idle runtime however busy it was.Key fingerprints that could never authenticate were stored as authorised. The verifier compares byte-for-byte against a lowercase-hex SHA-256, but
/keys/authorisestored whatever it was given — uppercase, whitespace-padded,sha256:-prefixed or the wrong length — and then listed it, telling an operator they had granted access they had not.keys::parse_fingerprintnormalises what can be and refuses what cannot./keys/revokeuses it too, so a key authorised from a prefixed paste can be revoked with the string the operator typed.Port 0 was accepted for site-service endpoints and ingress attachments. They arrive as plain
u16, so 0 deserialised happily and reached the site-proxy config aslisten :0/ dial:0semantics rather than being refused where the operator could be told which field was wrong. The BSL side has rejected it all along viaPort::new.An unknown DNS provider surfaced as a raw constraint failure.
set-acme-dnsleft it to the foreign key, anddb_errormaps every DB error tonot_found, so a typo returnednot_found: db error: FOREIGN KEY constraint failed— naming neither the provider nor the remedy, and giving a client the wrong error code. The check runs inside the same DB closure, so nothing can remove the provider in between. The same handler also accepted anyhostname, including an empty string, storing a policy that could never match and looked identical in the list to one that works;r[tls.policy.wildcard]defines the shapes and those are what it accepts now.Two notes on shape. The DNS-name check moved to
handlerso both the TLS policy and the site-service host validation use one implementation — thelocalhostrejection stays with site services, since an exact TLS policy may deliberately name it. The port check moved there for the same reason.The existing
key_mgmttests used"aabbcc"as a stand-in fingerprint, which is one of the values this now refuses; they use a full 64-character digest instead.One finding from this subsystem is not here: validating
/forwards/start's port against the service's declared ports. A Service does not carry its own ports — they come from pods mountingServicePort— so it needs a traversal that does not exist yet, and getting it wrong rejects forwards that should work. It is on card V4.