Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enforce auth token type when checking authentication headers in requests #1300

Closed
tgeoghegan opened this issue Apr 25, 2023 · 2 comments · Fixed by #1400
Closed

Enforce auth token type when checking authentication headers in requests #1300

tgeoghegan opened this issue Apr 25, 2023 · 2 comments · Fixed by #1400
Assignees
@tgeoghegan

Comments

@tgeoghegan
Copy link
Contributor

tgeoghegan commented Apr 25, 2023 •
edited
Loading

The task_aggregator_auth_tokens and task_collector_auth_tokens tables now have a type column explicitly indicating what kind of token it is. This should be enforced when checking authentication in a request. For instance, if a request contains a header DAP-Auth-Token: <token>, it should not validate against a stored token with type = 'BEARER' even if the token values match.

In #472 we're moving to checking for a bearer token in an Authorization HTTP header in Janus' DAP implementation (the aggregator API already does this). At some point, we should stop checking the DAP-Auth-Token header.
@tgeoghegan tgeoghegan mentioned this issue Apr 25, 2023
Closed
@tgeoghegan
Copy link
Contributor Author

I now suspect we can never drop support for DAP-Auth-Token because we'll need it to support the interop tests. While DAP implementations are free to implement whatever auth scheme they want, implementations opting into the interop test framework have to agree to either use some auth scheme or to disable auth altogether. Specifying DAP-Auth-Token in the interop test framework seems like a reasonable way forward, and potentially less risky than implementing a "turn off authentication" mode.

@tgeoghegan
Copy link
Contributor Author

tgeoghegan commented May 22, 2023 •
edited
Loading

I reviewed draft-dcook-ppm-dap-interop-test-design and it turns out it already specifies DAP-Auth-Token independently of draft-ietf-ppm-dap-01 (in the leader' add_task and the collector's add_task. So that means Janus must keep DAP-Auth-Token support indefinitely to continue supporting the interop test framework, which I think is fine.

I'm repurposing this issue to track making Janus consult the type columns in the aggregator and collector auth token tables when authenticating requests.

@tgeoghegan tgeoghegan changed the title Stop accepting authorization tokens in DAP-Auth-Token header Enforce auth token type when checking authentication headers in requests May 22, 2023
@tgeoghegan tgeoghegan self-assigned this May 22, 2023
tgeoghegan added a commit that referenced this issue May 22, 2023
@tgeoghegan
Test that auth token format must match
93e7754 
If some task accepts an `AuthenticationToken::DapAuth`, it should not be
possible to present its value as an `AuthenticationToken::Bearer` and
have it be accepted (or vice versa, should a Bearer token happen to also
be a legal `DAP-Auth-Token` value). This was already the case because we
evaluate tokens using `AuthenticationToken::eq`, but this commit adds a
test to explicitly verify this.

While we're in here, improve a doccomment to explain that
`AuthenticationToken::DapAuth` complies with the interop testing
framework as well as the now-obsolete `draft-ietf-ppm-dap-01`, and add a
constant for the collection job route to match the ones for aggregation
jobs and aggregate shares.

Closes #1300
@tgeoghegan tgeoghegan mentioned this issue May 22, 2023
Merged
tgeoghegan added a commit that referenced this issue May 23, 2023
@tgeoghegan
Test that auth token format must match (#1400)
583cfad 
If some task accepts an `AuthenticationToken::DapAuth`, it should not be
possible to present its value as an `AuthenticationToken::Bearer` and
have it be accepted (or vice versa, should a Bearer token happen to also
be a legal `DAP-Auth-Token` value). This was already the case because we
evaluate tokens using `AuthenticationToken::eq`, but this commit adds a
test to explicitly verify this.

While we're in here, improve a doccomment to explain that
`AuthenticationToken::DapAuth` complies with the interop testing
framework as well as the now-obsolete `draft-ietf-ppm-dap-01`, and add a
constant for the collection job route to match the ones for aggregation
jobs and aggregate shares.

Closes #1300
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tgeoghegan tgeoghegan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Test that auth token format must match divviup/janus
1 participant
@tgeoghegan