Skip to content

Decide who may ask, with none and bearer - #63

Open
jorgeMFS wants to merge 2 commits into
digest-validationfrom
auth-provider
Open

jorgeMFS wants to merge 2 commits into
digest-validationfrom
auth-provider

Conversation

@jorgeMFS

Copy link
Copy Markdown
Contributor

Closes #10 (C2), given the chain below lands first.

Merge order

Stacked. Base this on digest-validation (#62).

#48 → #45 → #46 → #57 → #58 → #59 → #62 → this

What was true before

There was no authentication of any kind — no dependency on /convert, no header read, no token compared, and no setting that could switch one on. The endpoint accepted a workflow, pulled binaries from a registry, executed them, and returned their output, to any caller that could open a socket to it.

That is defensible on a laptop. It is the wrong default for a service whose reason to exist is dispatching work into a TRE, where the point is that not everyone may ask.

The shape

Deliberately the same as runner.py: an interface, providers by name, shared policy written once — so F3 (Passports) is a third provider rather than a rewrite of the second.

none is the default and leaves behaviour unchanged, which is what C2 asks for. It is kept as a named provider rather than an absence: "no authentication" then appears in the settings, can be logged, and can be seen to be wrong in a deployment review.

bearer takes a single shared token. That is not identity — every holder is the same caller — and the code says so rather than implying otherwise.

The decision worth reviewing: middleware, not a route dependency

A Depends(...) would be the obvious place and is the wrong one. Starlette parses and spools the whole multipart payload before the endpoint is entered, so a dependency lets an anonymous caller upload up to BIOCHEF_MAX_UPLOAD_BYTES before anything asks who they are. Headers are in the ASGI scope from the start, so refusing there costs nothing.

It is added last so it is outermost, which puts it outside the body limit. One test pins that ordering; another proves no body chunk is consumed before the refusal.

Four smaller things that could each have gone quietly wrong

compare_digest, not == string comparison returns at the first difference, so timing leaks how much of the token was right and it can be recovered a character at a time
401, not 403 403 means "you are known and still may not" — a statement this service cannot yet make
WWW-Authenticate mandatory on a 401 per RFC 9110, and it is what tells a client which scheme to try
refuse to start bearer with an empty token would compare "" against "" and admit anyone sending a bare Authorization: Bearer; an unknown BIOCHEF_AUTH would silently leave the service open, which is the worst way for this setting to fail

Ten mutations, no survivors

token compared with ==             compared_in_constant_time
missing header allowed through     without_a_token_is_refused, before_its_body_is_read
any scheme accepted                anything_other_than_the_configured_token[Basic]
empty configured token accepted    bearer_without_a_token_refuses_to_start
unknown provider falls back        an_unknown_provider_stops_the_process
401 loses WWW-Authenticate         a_request_without_a_token_is_refused
403 instead of 401                 without_a_token_is_refused, before_its_body_is_read
middleware never checks            without_a_token_is_refused, before_its_body_is_read
auth inside the body limit         authentication_wraps_the_body_limit
service ignores BIOCHEF_AUTH       resolves_a_provider_and_defaults_to_none

Plus a control — the right token is let through — because refusing everything would otherwise pass every other test here.

Two housekeeping notes

Two verification tests were deleted, not kept. They asserted /convert had no dependencies and that main.py never mentioned Authorization; both still pass, since the check is middleware and lives in auth.py, but they now describe the wrong thing and would fail someone for adding a route dependency. The verification commit records that state.

README's deployment warning said there is no authentication of any kind. There is; it is off by default, and a deployment that leaves it off is choosing to. Corrected, along with the settings table.

140 tests pass.

C2 (#10) asks for an AuthProvider interface with none and bearer. There is no
authentication of any kind to replace.

Recorded before changing anything:

  /convert has no dependencies      the place a check would live is empty
  nothing reads a credential        no Authorization, HTTPBearer, APIKeyHeader,
                                    Security or 401 anywhere in main.py
  no setting could switch it on     BIOCHEF_RUNNER chooses how a workflow
                                    executes, BIOCHEF_MAX_UPLOAD_BYTES bounds
                                    what may be sent, BIOCHEF_APPTAINER_ARGS
                                    decides what a step may see -- there is no
                                    equivalent for who may ask
  an anonymous request is served    refused only for its contents, never for
                                    its lack of credentials

The endpoint accepts a workflow, pulls binaries from a registry, executes them,
and returns their output, to any caller that can open a socket to it. That is a
defensible default on a laptop. It is the wrong one for a service whose reason
to exist is dispatching work into a Trusted Research Environment, where the
point is that not everyone may ask.

The interface matters as much as the two providers: F3 (Passports) should be a
third provider rather than a rewrite.

Refs #10
C2 (#10). The same shape as runner.py: an interface, providers selected by name,
and the shared policy written once, so that F3 (Passports) is a third provider
rather than a rewrite of the second.

`none` is the default and leaves behaviour unchanged, which is what C2 asks for.
It is kept as a named provider rather than an absence on purpose: "no
authentication" appears in the settings, can be logged, and can be seen to be
wrong in a deployment review. The same state as before, but visible.

`bearer` takes a single shared token. That is not identity -- every holder is the
same caller -- and the code says so rather than implying otherwise. It is the
smallest thing that stops an open endpoint being open.

The check is middleware, not a route dependency, and that is the decision worth
reviewing. Starlette parses and spools the whole multipart payload before the
endpoint is entered, so a dependency would let an anonymous caller upload up to
BIOCHEF_MAX_UPLOAD_BYTES before anything asked who they were. Headers are in the
ASGI scope from the start, so refusing there costs nothing. It is added last so
it is outermost, which puts it outside the body limit; a test pins that order
and another proves no body chunk is consumed before the refusal.

Four smaller decisions, each of which could have gone quietly wrong:

  compare_digest, not ==   string comparison returns at the first difference, so
                           how long it takes leaks how much of the token was
                           right, and a token can be recovered a character at a
                           time
  401, not 403             403 means "you are known and still may not", which is
                           a statement this service cannot yet make
  WWW-Authenticate         mandatory on a 401 per RFC 9110, and it is what tells
                           a client which scheme to try
  refuse to start          bearer with an empty token would otherwise compare ""
                           against "" and admit anyone sending a bare
                           `Authorization: Bearer`; an unknown BIOCHEF_AUTH would
                           silently leave the service open, which is the worst
                           way for this particular setting to fail

Ten mutations, no survivors:

  token compared with ==             compared_in_constant_time
  missing header allowed through     without_a_token_is_refused, before_its_body_is_read
  any scheme accepted                anything_other_than_the_configured_token[Basic]
  empty configured token accepted    bearer_without_a_token_refuses_to_start
  unknown provider falls back        an_unknown_provider_stops_the_process
  401 loses WWW-Authenticate         a_request_without_a_token_is_refused
  403 instead of 401                 without_a_token_is_refused, before_its_body_is_read
  middleware never checks            without_a_token_is_refused, before_its_body_is_read
  auth inside the body limit         authentication_wraps_the_body_limit
  service ignores BIOCHEF_AUTH       resolves_a_provider_and_defaults_to_none

There is a control test too -- the right token is let through -- because refusing
everything would otherwise pass every other test here.

Two verification tests were deleted rather than kept. They asserted that
/convert had no dependencies and that main.py never mentioned Authorization;
both still pass, because the check is middleware and lives in auth.py, but they
now describe the wrong thing and would fail someone for adding a route
dependency, which is not a defect. The verification commit records that state.

Also corrected README's deployment warning, which said there is no
authentication of any kind. There is; it is off by default, and a deployment
that leaves it off is choosing to.

Refs #10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant