Conversation
Sixteen settings is more than anyone gets right from scratch, and the two that
decide how exposed a run is both default to the unguarded option: BIOCHEF_RUNNER
runs every tool on the host as this service's user, and BIOCHEF_AUTH answers
anybody. Neither default is wrong for a laptop. Both are wrong for a machine
holding data, and nothing in a response says which one is running.
BIOCHEF_PROFILE names a starting point -- dev, server or tre. It is applied
after .env and before the first getenv, because every setting here is read at
import, and a profile arriving later would be describing a service that had
already decided who may call it and whether tools run in a container.
A profile sets DEFAULTS. Anything already in the environment wins, because a
configuration system that silently discards what an operator explicitly set is
worse than one that makes them set more. The cost of that choice is that a
profile can be undone by a stray variable, so startup prints what actually took
effect and NAMES every setting the profile did not get to decide:
BIOCHEF_AUTH=none (profile asks for bearer; the environment already set
this and wins)
Selecting tre and having authentication off is something an operator might
genuinely want and something they might do by accident, and those two are
indistinguishable unless startup says so. An unrecognised name refuses to start
rather than falling back to a default nobody chose.
tre states an egress allowlist and does not enforce it, which E4 permits at this
stage. The wording is deliberate: a dictionary in Python does not constrain
outbound traffic, and documentation implying otherwise is the dangerous kind.
What it is for is giving the proxy or systemd unit that does enforce it a
written list to be checked against.
A test refuses any profile that sets a variable nothing reads. That is how this
feature would rot -- a profile listing BIOCHEF_ISOLATION_LEVEL would look like
it hardened something, would do nothing whatever, and would print in the startup
line exactly like the settings that work.
Four things the first version got wrong, none of which CI could have caught,
because each is a case of the feature working and describing something else.
The startup report listed values. BIOCHEF_AUTH=none repeats what the operator
typed; what they need to know is that it means this service answers anybody who
can reach it. The report now says that, for the four settings where the meaning
is the whole point -- open authentication, tools on the host, plain HTTP to the
registry, run directories kept.
Those lines are read from the EFFECTIVE environment and not from the profile.
Reading them off the profile's intent would print the exact opposite of the
truth in the one case that matters: a deployment asking for tre while something
already set BIOCHEF_AUTH=none would have been told it requires a bearer token,
directly under the word tre. A test pins that, and fails if the source is
changed back to the profile.
server and tre were byte-identical dictionaries differing only in a printed
sentence, which is a fine state of affairs and a bad one to leave unexplained.
With the settings this service has there is nothing further to tighten for a
TRE; what distinguishes it is an egress expectation nothing here enforces. Both
the module and the README now say so, and a test asserts tre is never WEAKER
than server -- because the tempting way to differentiate them later is to relax
something in server.
convert printed the report as a side effect of being imported. The catalogue
converter and the whole test suite import that module, and one that writes to
stdout on import interleaves with whatever its importer was saying. It applies
the profile there, because every setting is read at import and a profile
arriving later would describe a service that had already decided who may call
it; main prints it, which is the first point where saying so is not somebody
else's import.
And BIOCHEF_PROFILE escaped the guard that covers every other setting.
test_settings_are_documented scans for getenv("NAME"), and this one is read as
environ.get so apply() can be handed a dictionary -- so the single variable that
turns the feature on was the one variable nothing checked was documented.
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.
Closes E4 (#17), the last open item in workstream E. Based on
auth-provider(#63), which is the C2 this depends on.Why
Sixteen settings is more than anyone configures correctly from scratch, and the
two that decide how exposed a run is both default to the unguarded option:
BIOCHEF_RUNNERruns every tool on the host, as this service’s userBIOCHEF_AUTHanswers anybodyNeither default is wrong for a laptop. Both are wrong for a machine holding
data, and nothing in a response tells you which one you are running.
devserver--containtre--containApplied after
.envand before the firstgetenv, because every setting hereis read at import — a profile arriving later would be describing a service
that had already decided who may call it.
The part I think is worth reviewing
A profile sets defaults; the environment wins. A configuration system that
silently discards what an operator explicitly set is worse than one that makes
them set more.
But that means a profile can be quietly undone by one stray variable. So
startup prints what actually took effect and names every setting the profile did
not get to decide:
Selecting
treand having authentication off is something you might genuinelywant and something you might do by accident. Those are indistinguishable unless
startup says so.
Verified that
BIOCHEF_PROFILE=tregenuinely reaches the real decision —main.AUTH.namebecomesbearer— rather than only printing a line. Anunrecognised name refuses to start rather than falling back to a default nobody
chose.
What
tredeliberately does not doIt states an egress allowlist and does not enforce it, which E4 permits
at this stage (external proxy or systemd). The wording is deliberate: a Python
dictionary does not constrain outbound traffic, and documentation implying
otherwise is the dangerous kind. Its purpose is to give whatever does enforce
it a written list to be checked against. A test asserts only
tremakes anyclaim about egress, and that the disclaimer is present.
How this feature would rot
A profile listing
BIOCHEF_ISOLATION_LEVELwould look like it hardenedsomething, would do nothing whatever, and would appear in the startup line
exactly like the settings that work. A test refuses any profile key that nothing
in the code reads — verified by planting one and watching it fail.
151 passed, 3 skipped.Note for whoever sequences these
BIOCHEF_SIGNING_MODEfrom #75 is not in these profiles, because it doesnot exist on this branch and the decorative-key test would rightly reject it.
When #75 and this both land,
serverandtreshould set it tostrict— thatis the setting that makes those profiles mean what their names suggest, and it
is a one-line addition to each once both are in the same tree.
Audit: four defects, all of them the feature working while describing something else
The report listed values, not meaning.
BIOCHEF_AUTH=noneonly repeats whatthe operator typed. This README opens by complaining that nothing tells you
which service you are running — and the first version reproduced exactly that.
It now says what the setting permits, for the four where the meaning is the
point:
And it reads them from the effective environment, not from the profile.
Reading them off the profile’s intent would have printed the opposite of the
truth in the only case that matters — a deployment asking for
trewhilesomething already set
BIOCHEF_AUTH=nonewould have been told it requires abearer token, two lines under the word
tre. Verified by mutation: switchingthe source back to the profile fails the test.
serverandtrewere byte-identical dictionaries differing only in aprinted sentence — a fine state of affairs and a bad one to leave unexplained.
With the settings this service has, there is nothing further to tighten for a
TRE; what distinguishes it is an egress expectation nothing here enforces. Both
the module and the README now say so, and a test asserts
treis never weakerthan
server, because the tempting way to differentiate them later is to relaxsomething in
server.convertprinted the report as a side effect of being imported. Thecatalogue converter and the whole test suite import that module, and one that
writes to stdout on import interleaves with whatever its importer was saying. It
still applies the profile there — every setting is read at import — but
mainnow prints it, which is the first point where saying so is not somebody else’s
import.
BIOCHEF_PROFILEescaped the guard that covers every other setting.test_settings_are_documentedscans forgetenv("NAME"); this one is read asenviron.getsoapply()can be handed a dictionary. So the single variablethat turns the whole feature on was the one variable nothing checked was
documented. Now explicitly asserted against both README and
example.env.157 passed, 3 skipped, CI green on all four jobs.