Repository navigation
Read ThreatModels from the TrustOnCloud API - #43
Merged
Merged
Conversation
tmxcaliber could only work on files. A subscriber now points it at a
ThreatModel id and the document is fetched, so the commands that matter
work without downloading anything first:
tmxcaliber list threats aws-s3
tmxcaliber filter aws-s3 --severity high
tmxcaliber create-change-log aws-s3@1700000000 aws-s3@1611187200
The last of those is new rather than merely easier: comparing two released
versions of one model was impossible without local copies of both.
Shaped like the API, deliberately. Every group, command, positional and
option is derived from the route contract by one mechanical rule, so none
of them is a naming decision that can be argued about on its own, and a
route the API adds becomes a command with nothing to decide. The table is
held against the vendored OpenAPI document in both directions, because the
other tests mock the transport and so only ever compare the client against
itself: a command naming a route the server does not serve would stay green
forever and fail for whoever ran it.
The contract is vendored because it cannot be fetched. /v1/apis/openapi.json
sits behind both authentication and the tenant feature flag, so a client
cannot read the contract it is about to call.
Assembly pins a release. The canonical document is five calls, and if latest
rolled between the first and the last a client would stitch two releases
into one document that still validates. The detail route names the release
it answered with; the other four are asked for exactly that one.
No new dependency. urllib gives a custom opener, per-call timeouts and full
header control; requests would add a transitive tree to a package that
publishes to PyPI on every merge and is installed unpinned into a Lambda
image.
Some details that are easy to get wrong, and the reason each is the way it
is:
- Retry-After is capped at 60s. The API sends the whole window, so a 429
from the hourly bucket says 3600 and a client obeying it literally hangs
the terminal for an hour. Past the cap it reports instead of sleeping.
- urllib exceptions are re-raised `from None`. They carry the Request, and
the Request carries the Authorization header.
- Credentials has a hand-written repr with no key in it, so the secret
cannot reach a traceback frame or a pytest dump.
- A response body never reaches repair_json_strings. On a truncated body
that regex turns a transport failure into a document that looks fine.
- The cache is new rather than lib/cache.py reused: that one keys on the
last URL segment, which collides for every subresource, and treats "the
file exists" as "the file is complete".
- A reference is only considered when nothing of that name is on disk, so
no existing invocation changes meaning and a mistyped filename still
produces the filesystem error it always did.
Credentials are TrustOnCloud's, not this tool's: TOC_API_KEY and
~/.trustoncloud/credentials, because the same key works with curl or an
SDK. tmxcaliber's own cache stays under ~/.tmxcaliber. There is no
--api-key flag, so the secret never reaches shell history or a CI log.
test/conftest.py now fails any test that opens a socket. There was no guard
before because there was nothing to guard.
Every pre-existing test passes untouched, which is the evidence that the
document commands behave exactly as they did.
All four were real, and two of them would have shipped a credential boundary that does not hold. **The cache was shared across credentials.** Its key was endpoint, model and release, so once one key fetched a model, a second key on the same machine read it with no request and therefore no entitlement check, and a pinned entry never expired so revoking the first key did not stop it. Entries are now partitioned by the key id, which is the public half of the credential and is hashed into the path rather than written there. Pinned entries also gain a lifetime, not for staleness, since the bytes are immutable, but to bound how long a revoked credential keeps reading a cache it populated while it was valid. **A redirect handed the key to another origin.** urllib's default opener follows redirects and copies the original request's headers onto the new one, Authorization included, so a redirect to another host, or to plain HTTP, discloses the tenant key to whatever answers there. The client now builds an opener that refuses any redirect leaving the origin or weakening the scheme. The API publishes no redirecting route, so refusing costs nothing and following blindly was never worth it. **A page missing nextCursor was read as the last page.** The envelope always carries it, null at the end, so an absent one means the response has drifted from the contract. Treating it as "no more" silently truncates a model, and for a pinned release that truncation then caches as complete. A row that is not an object was also skipped rather than refused, which drops an entity just as quietly. Both now raise before anything is written. **`generate` rejected the reference its own help advertises.** The shared source parser accepts one, but `validate` requires a .json or _DFD.xml suffix and runs at parse time, before the document is fetched, so the documented `tmxcaliber generate aws-s3` exited 2. A reference names no file, so the suffix check cannot speak for it. Seven regression tests, one per hole plus the same-origin and default-opener cases, because three of these are invisible in behaviour until the day they are not.
Second pass of the adversarial review, both findings about trusting something that had not been checked. **The endpoint could be plain HTTP.** TOC_API_URL was taken as given, and the key travels in a request header, so a mistyped or untrusted endpoint handed a live tenant credential to anyone able to watch the connection. Only https is accepted now, with http allowed on loopback alone, which is the one case where the connection never leaves the machine and is what local development against the container needs. **The answer was not checked against the question.** The assembler took whatever the detail route returned, built four more calls on it, and cached it under the reference that was asked for. A proxy or a server-side identity slip returning a different model, or a different release of the right model, became persistent and silently wrong output. It now requires the tmId to match, a release to be named, a pinned release to be the one that came back, and the metadata to agree with the top-level version, which is the field the unpinned cache is keyed from. Each check is cheap and each one is the kind that never fires until the day it does, so all five have tests, and the assembly stub gained a way to misbehave on purpose because a stub that always answers correctly cannot exercise them.
…model Third pass of the adversarial review. **A malformed credentials file printed the credential.** ConfigParser puts the offending line into its ParsingError, so a key written without its separator was disclosed to the terminal, and to any CI log, by the error complaining about it. The message now names the file and a line number and carries none of the parser's text. **A falsey nextCursor read as the end of the walk.** The contract marks the last page with null, but `if not cursor` also accepted an empty string, false or zero, so a degraded response truncated the collection and the partial document then cached as a whole one. Only null ends a walk now; anything that is not a non-empty string is refused. **A connection lost mid-body escaped everything.** ConnectionResetError and IncompleteRead are not URLError, so a connection dropped after the headers arrived bypassed both the retry policy and the CLI's error handler, and the caller saw a traceback for a GET that was safe to retry. They are reported as ServiceUnavailable like any other transport failure, and therefore retried. Seven tests, including both halves of the cursor rule, because "stops on null" and "refuses everything else" fail independently.
Fourth pass of the adversarial review, and this one was a bug I had just introduced. `metadata.version` is the canonical template schema version, a date such as `20240423` that matches the schema filename. A release key is an epoch such as `1611187200`. They are different facts with similar names, and the previous commit read one as the other twice: - the consistency check compared the API's release against `metadata.version`, so it would have raised ContractViolation on every published document and remote sources could not have loaded at all; and - the unpinned fetch filed its second cache entry under that same value, so every model landed at `20240423.json` and every pinned request missed and repeated the whole five-call assembly against an hourly budget. Verified against the published corpus rather than assumed: aws-s3, gcp-bigquery and azure-storage all carry `metadata.version` `20240423` and no `metadata.release` at all. `fetch_document` now returns the document beside the release the API answered with, so the release is carried explicitly rather than recovered from a field that never held it. The check that compared them is gone, because comparing those two fields was never meaningful. The fixture was the reason the tests missed this: it put an epoch in `metadata.version`, which no real document does. It now carries the schema date, and two tests pin the distinction, one that the two values differ and one that an unpinned fetch satisfies a later pinned request from cache.
Fifth pass of the adversarial review. The identity check added two commits ago ran only when `tmId` was present, so a detail response carrying no identity passed it: that document was then combined with collections requested for this model and cached under this reference. Absent identity is the same wrong document as a named mismatch, arrived at more quietly, so it is now refused. The metadata names the model a second time, and the two must agree. A correct envelope wrapped around another service's body would otherwise pass on the envelope alone. One finding from this pass is deliberately not taken. It reports that `--output` writes licensed content at 0644 under the usual umask while the cache is private. The asymmetry is real and it is the right one: the cache is a location the tool chose, so the tool owns its permissions, whereas `--output` names a path the user chose, and honouring their umask there is what every other command in this CLI does and what a caller expects. Forcing 0600 on a user-named path would be the surprising behaviour.
Three changes that turn out to be one. **`init` stores the key, so the CLI owns the file's mode.** It asks without echoing, writes ~/.trustoncloud/credentials at 0600 under a 0700 directory, and then calls /v1/me to say which tenant answered. Piped stdin skips the prompts, so `echo "$KEY" | tmxcaliber init` works where there is no tty. Verification reports rather than gates: a 404 usually means the API is not enabled for that organization yet, which is no reason to throw away a key that is otherwise fine. **The credentials file is refused when other users can read it.** The previous answer to this was a README line telling people to create the file by hand, which under a default umask produces a 0644 credential. Now the CLI writes it, so a wider mode means it was hand-made or loosened, and the refusal names `tmxcaliber init` as the cure. `init` itself reads such a file forgivingly, because being the cure means being able to open what the loader rejects. **Profiles are gone rather than merely unreachable from `init`.** Leaving --profile and TOC_PROFILE while the command that owns the file cannot write them would be a half-feature reachable only by hand-editing. One key, one endpoint, and TOC_API_KEY for the "a different one, right now" case, which is what an environment variable is good at. Free to remove: the branch has never been pushed, so profiles never shipped. `init` is the one command not derived from the route contract, and the reason is categorical rather than taste: it configures access to the API instead of calling it, so there is no route to derive it from. Written next to the command, because the next exception will otherwise cite this one. Also the completeness check agreed from the review: a document missing any required section is refused before it is returned or cached. Structural rather than schema validation, since the published corpus does not satisfy its own schema in every detail, and the required list is read from the canonical schema so it has one owner.
This repository is public, and the README named a non-production TrustOnCloud endpoint as the example value for TOC_API_URL. That is infrastructure a customer is never given, so publishing it only tells someone what to aim at. The variable is still documented, now without a host nobody outside needs to know. Also stop getpass warning on the documented CI path: with stdin piped there is no terminal to control the echo on and nothing to echo to, so it reads the line directly instead of printing a warning about a terminal that does not exist.
…ey init wrote Two from the latest adversarial pass, and the first is the more serious. **An API-supplied release became a cache filename unchecked.** `pathlib` discards everything to the left of an absolute component, so a response naming its release `/tmp/target` wrote the assembled document to `/tmp/target.json`; a `../` form escaped too. That is an arbitrary-write primitive driven by whatever answered, and it needs a hostile or simply broken endpoint, which `TOC_API_URL` is one way to be pointed at. The asymmetry is the part worth keeping in mind. The release a caller *types* was already safe, because the reference grammar excludes a slash. Only the release the *server* returns was trusted, after four checks had already been added on the premise of not trusting its answer. Those asked what the document was; none asked what it would make us do. A release is now one inert path component, enforced in three places that each exist for a different reason: `parse_ref` for what a caller types, which also refuses `..`; the assembler for what the server returns, where the other answer checks live and where it costs one call rather than five; and `cached_path` as a backstop, because that is the line turning a string into a path and a later caller may not have checked. **`init` verified whichever key resolution preferred, not the one it just wrote.** With `TOC_API_KEY` set it printed "Verified" for the environment credential while the stored one went untried, which is worse than silence because the shadowing warning sits directly above it. Verification now takes explicit settings. Those are also built before the file is written, so a malformed key or an endpoint that would carry it in the clear is refused rather than stored. `settings_for` is now the single place a key and an endpoint become usable settings, so neither rule can be skipped by a caller assembling a Settings itself. The test that should have caught the second one injected a client, which skips the very decision under test. The replacement fakes only the client's construction and lets the real choice of credential run. That is the second time an obliging stub hid a defect here, after the fixture that put an epoch in metadata.version.
…loudly Three from the next adversarial pass. The first means a documented command did not work at all. **`create-change-log` on remote models raised KeyError.** change_log.py reads `metadata["release"]` directly, and the published document has no such field: it is written by the per-customer stamp on the delivery path, which this API does not apply. So the command written up in the README as the payoff of release pinning failed on a response that was otherwise perfectly valid. The assembler now stamps the release it already validated, which it is entitled to do because the detail route named it and it has been checked against both the pin and the grammar. It went unnoticed because the command was documented and never run. There is now a test that assembles two releases and generates the change log between them, which is what the README claims and what nothing exercised. **A `%` in the credentials file could quote the key back at you.** ConfigParser interpolates by default, and an InterpolationSyntaxError quotes the rest of the offending value. The earlier fix for this closed `parser.read` and left `parser.get`, which sat outside the handler entirely. Interpolation is now off, because a credentials file has nothing to interpolate, and both reads are inside the handler. **`init` exited zero when the key did not work.** The documented piped form runs in CI, where a setup step that cannot work must not look like one that did. It now exits non-zero. The key is still kept, because a failure here is usually the organization not being enabled yet rather than a bad credential, and making the caller re-enter it would help nobody; the message says so.
…s given
The two from the review, plus two the self-audit found.
**Metadata was checked for shape and not for content.** The completeness
check accepted `metadata: {}`, and nothing downstream complains: `list
services` reads `service_name` with `.get`, so a missing one drops the
model from the listing rather than failing, and a pinned entry holds that
for a week. The required fields are now read from the schema, the same way
the section list already is. Safe to require: the published corpus fails
its own schema in places, which is why the document is not validated
wholesale, but every published model carries all six of these.
**`init` discarded a configured endpoint.** It read the existing key and
not the existing endpoint, so a blank answer rewrote the file without it.
Blank meant "keep" for the key and "reset" for the endpoint in the same
command, and the piped form did it with no prompt at all, which is how a
CI re-run could silently point a freshly stored key at the wrong API.
Blank now means keep for both.
Two more from reading my own code rather than waiting to be told:
**The detail-response checks ran too late.** Metadata was validated after
the four calls that build on it, so a response with no metadata cost five
calls to reject instead of one, and the provider/service cross-check was
skipped rather than failing when there was nothing to cross-check against.
Everything knowable from the detail response is now decided before
anything depends on it, and the cross-check is unconditional.
**Collections were walked at the server's default page size.** The
contract permits 500 and the default is 100, so a large model spent whole
extra calls per section against a budget one assembly already spends five
of. The maximum is read from the contract.
Caught by auditing rather than by review. The version lookup caught bare `Exception` where the only thing that can be raised is `PackageNotFoundError`, which `cli.py` already catches by name for the same lookup. A broad catch there would have swallowed a genuine fault and quietly reported a wrong version in the header.
Both of these are mine, and the first is mine twice over.
**`str()` coercion defeated the check it was part of.** The metadata
completeness check read `str(metadata.get(field, "")).strip()`, which
turns `None` into the string "None". That is truthy, so a null
`service_name` passed the very check written to catch an empty one, and
`get_service_rows` then dropped the model from `list services` without
error. Written while fixing this exact class of defect, which is the part
worth remembering: the coercion looked like defensiveness and was the
hole.
**An empty DFD was accepted as a diagram.** `{}` is a dict, so the section
check passed it, and the documented `generate` command then failed on the
missing body while the cache kept serving the same answer until it
expired. The schema requires a body.
Both now ask for a non-empty string rather than for something that can be
printed. Safe to require: the schema types all six metadata fields and
`dfd.body` as strings, and the published corpus holds strings in every
one of them. Eleven cases between the two, covering null, empty,
whitespace and wrong-type.
Found by continuing the sweep rather than waiting for the next review. Both are the same shape as everything else this week: a value that is present and useless, accepted and carried on with. The identity check asked whether the field was truthy and then coerced it with `str()`. A dict or a number passed, and the stored document keys the section by that value, so the result was a nonsense key in a document that still looked valid. It asks for a non-empty string now. A repeated identity silently overwrote the first row. The assembled document would be one entity short and the walk that produced it would look complete, which is the worst version of this failure: nothing anywhere is wrong except the count. The rows are sorted and paged on this key, so a repeat means the page boundary moved under the read, and that is worth failing on.
**Setting TOC_API_KEY silently changed which API was called.** The credentials file was read only when the environment held no key, so a caller with a custom api_url who exported a key went back to the production default without being told. `init` verified against the stored endpoint, so it could report success for one API while every later command called another, which is the worst shape this can take: the check passes and the thing it checked is not what runs. The endpoint is now resolved on its own: TOC_API_URL, else the stored one, else the default, whichever place the key came from. Also made a pre-existing failure legible. `generate_change_log` parses `metadata.release` as an integer, which every published release key satisfies because it is an epoch, but a document without one surfaced as a bare KeyError or ValueError from two layers down naming neither the file nor the field. It now says what is missing. That applies to a document read from disk exactly as to one assembled from the API; the remote path only made it reachable a second way. Not done, deliberately: narrowing the release grammar to digits. Its job is to keep a release from acting as a path, the API types the parameter as a string, and a format this client refuses outright would be worse than one it reports clearly.
**A live entity missing required fields was cached as a document.**
`_by_id` accepted a row carrying only its id, assembly succeeded, and
`list threats` then raised KeyError on a field the schema requires, with
the same answer served until the cache expired.
The check has to know about retired entities: the API serves those as
`{id, retired}` by design, so demanding the full shape of every row would
reject documents that are entirely correct. A row that does not say it is
retired must carry what the schema requires of a live one. Safe to
require, checked rather than assumed: every live entity in the published
aws-s3 model carries all of them, 64 threats, 177 controls and 200
actions.
**`init` still verified the wrong endpoint.** The previous commit made
`load_settings` resolve the endpoint independently of the key, and did
not apply the same precedence inside `init`, so with TOC_API_URL set and
nothing stored it checked the default while every later command called
the custom one. That is the same failure the earlier fix was for, left
half done, which is worth saying plainly. It now resolves the endpoint
the same way and names it in the output, because which API was checked is
exactly what a caller cannot otherwise tell.
`queryable_id`, `queryable_objective_id` and `action_id_int` are not data. The generator computes each as the integer parsed out of the id it belongs to (`int(row.id[offset:])`), and uses it to sort numerically, because "C9" and "C10" sort wrong as text while 9 and 10 do not. Checked rather than assumed: across the published aws-s3 model there is no control or action, out of 377, where the value differs from the suffix of its own id. Requiring them in a canonical document stores one fact twice in two encodings, and the id is the owner. The differ already treats one of them as noise, listing `.queryable_objective_id` beside `.order` and `.score`. The public API reached the same conclusion independently and publishes none of the three. They stay defined in the schema, because every published document carries them and tooling still reads them; they are simply no longer required. Relaxed in the active schema only. Nothing that validated before stops validating, which is what makes editing it in place rather than minting a template version the honest option. The fixture is the part worth keeping. It carried all three, so it was asserting against the canonical shape while the API serves a narrower one, and it hid the fact that the assembler demanded fields no real response contains: every live control would have been rejected the first time this ran against the live API. It now carries exactly what the API publishes, so every assembly test runs against a realistic row. Verified against the real corpus: all three published models, reshaped to the API's projection, pass every assembler check with nothing rejected. Noted while confirming that, and not caused by this change: none of the three published models validate against the canonical schema, for three unrelated reasons (`access.OPTIONAL` carrying an OR/AND object where the schema allows a string or array, and a null `max_dependency` typed as a number). Identical before and after this edit. It is the reason the assembler checks structure rather than running the validator.
Validating the published ThreatModel corpus against 20260925.json produced 34,584 errors and not one valid document. The corpus was not at fault; the schema was, in four places: - max_dependency was typed `number`, and is null far more often than not. - access.OPTIONAL allowed a string or an array, but also carries a nested OR/AND block and a permission-to-condition map. Both are now accepted, the latter via a new accessCondition definition. - access.UNIQUE allowed only an array, and is almost always a string. - The qualitative severity rating was constrained in the control section and unconstrained in the threat section, so the same value was rejected in one place and accepted in the other. The severity scale now has a single owner, `severityRating`, that both sections reference. It is TrustOnCloud's own classification: CVSS-like, but running "Very High" to "Very Low" with no "Critical" rating. The control section had been given the CVSS ratings by mistake, which is the sort of drift one shared definition prevents and two copies invite. Every published document now validates, and the remaining failures are a data defect in a small number of models that carry a rating outside the scale. Those are left failing deliberately: the fix belongs in the data, not in a schema widened to accept it. test_schema_severity.py pins the scale and the fact that both sections read it from the same definition.
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
1 participant
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.
Version 0.4.0 to 0.5.0.
What this adds
tmxcaliber could only read ThreatModels from disk. It can now read them from the TrustOnCloud API, so a subscriber works against their own entitled models without downloading files by hand.
That last line is the main payoff, and was impossible before.
Design
The command tree is derived from the route contract, by one mechanical rule: literal path segments become groups, the last becomes the command, path parameters become one positional, query parameters become options, and a paged response takes
listwhile a single resource takesget. Deriving them is what stops naming being a separate discussion for every route.initis the only command not derived, and the reason is categorical rather than taste: it configures access to the API instead of calling it.Disk wins. A source that exists on disk is a path. One that does not and matches the reference grammar is a reference. Anything else raises exactly the error it raised before, so no existing invocation changes meaning.
test_parsers.py,test_cli.pyand the end-to-end tests pass untouched, which is the proof.No new dependency.
urllib.requestgives a custom opener, per-call timeouts, full header control andHTTPErrorwith.code. Addingrequestswould put a transitive tree into a package that publishes to PyPI on merge and installs unpinned into a container image.The contract is vendored at
lib/remote/openapi/v1.json, because it cannot be fetched: the OpenAPI route sits behind both authentication and a tenant feature flag, so a client cannot read the contract it is about to call. A test holds the table and the document in agreement in both directions.Credentials
TOC_API_KEY, else one[default]section in~/.trustoncloud/credentials, else a refusal raised before any socket. There are no profiles: one stored key and one endpoint, with the environment covering "a different one, right now".A TrustOnCloud credential is not a tmxcaliber one, so it is named and stored for TrustOnCloud; tmxcaliber's own cache keeps
TMXCALIBER_and~/.tmxcaliber/.initwrites the file at0600under a0700directory via a temporary file and a rename, and the loader refuses a file other users can read, naminginitas the cure. There is deliberately no--api-keyflag, so the secret cannot reach shell history or a CI log.What the client refuses
Each is cheap and none fires until the day it does: a cleartext endpoint, a redirect off the origin (urllib copies
Authorizationonto the redirected request), an answer for another model or release, a page with a missing or falseynextCursor, a body that is not JSON, an assembled document missing a required section, a row with no usable id or a repeated one, an emptydfd, and a release that could act as a path component.Every one asks for a value that is present and usable. The recurring defect here was accepting something merely printable:
str(None)is"None", an empty dict is a dict, a stub is a row.Assembly
Five calls: the detail route plus
threats,controls,actionsanddfd. The detail route names the release it answered with and the other four are pinned to exactly that one, so a model republished mid-read cannot be stitched together from two versions. Collections are walked at the maximum page size the contract permits.Canonical schema
Validating all 275 published ThreatModels against
20260925.jsonproduced 34,584 errors and no valid document. The corpus was not at fault:max_dependencytypednumber["number", "null"]access.OPTIONALstring or arrayOR/ANDblock, or a permission-to-condition mapaccess.UNIQUEarray onlyseverityRatingboth sections referenceAll 275 now validate apart from a data defect in 7 models that carry a severity rating outside TrustOnCloud's scale, which is left failing deliberately: the fix belongs in the data.
test_schema_severity.pypins the scale.Tests
523 passing, ruff and mypy clean. A
conftest.pynetwork guard fails any test that opens a socket, which the repo had none of.