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.

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

aggregator API: default to bearer tokens #1548

Merged
merged 1 commit into from
Aug 1, 2023

Conversation

tgeoghegan
Copy link
Contributor

Previously, the aggregator API assumed that any aggregator or collector auth tokens it was handling were AuthenticationToken::DapAuth. Janus must support those for the interop testing API and to work with Daphne, but generally, we prefer to use more boring Authorization: Bearer tokens. Certainly any tasks provisioned via the aggregator API and divviup-api should use bearer tokens.

With this PR, we augment the PostTaskReq and PostTaskResp messages so that they use AuthenticationToken to represent aggregator and collector tokens, meaning that on the wire, a token now looks like:

{
  "type": "Bearer",
  "token": "AAAAAAAAA<etc.>"
}

Also, when the aggregator API generates either kind of token, it now generates a bearer token. I'm not aware of any scenarios where we need to generate DapAuth tokens in the aggregator API so there is no affordance for requesting that kind of token.

See #472

@tgeoghegan
Copy link
Contributor Author

Corresponding divviup-api change: divviup/divviup-api#247

@tgeoghegan tgeoghegan marked this pull request as ready for review June 29, 2023 22:46
@tgeoghegan tgeoghegan requested a review from a team as a code owner June 29, 2023 22:46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid representing these tokens as base64 in some places and base64url in others.

A more flexible approach would be to not round-trip bearer tokens through either base64 or base64url, but instead validate that they match the syntax in RFC6750 when deserializing, store them as-is, and compare them as-is. This should be addressed separately, at any rate.

Copy link
Contributor Author

@tgeoghegan tgeoghegan Jun 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you that we need to be consistent about encodings. We previously discussed how we prefer unpadded Base64URL where possible, because:

  • we are forced to use it to interpolate values into URLs
  • it's more copy-pasteable than Base64 (whose alphabet contains /)

I had thought that the Authorization: Bearer header was the one place where tokens must be represented as plain Base64. For that reason, I have been insisting that we need to consider the canonical form of an authentication token to be the bytes, and then Base64 or unpadded Base64URL encodings are details of a particular serialization context.

However, I just noticed that I have misread RFC 6750 2.1: its grammar for the Authorization: Bearer scheme allows both the Base64 and Base64URL alphabets (and some extra symbols like . and ~).

I think that means we can use unpadded Base64URL everywhere: config files, JSON messages sent to APIs, and HTTP headers. Doing this would break existing clients/users of:

  1. divviup-api
  2. the Janus aggregator API
  3. janus_cli's task provisioning subcommand
  4. the Janus DAP API, if authenticated by bearer tokens instead of DAP-Auth-Token

But (1), (2) and (3) are broken by the recent changes anyway. And I'm pretty sure (4) doesn't exist, because the only external users of Janus that have existed so far have used DAP-Auth-Token. Meaning I think we can still change the representation of bearer tokens in HTTP headers without (further) breaking anyone.

Now, if unpadded Base64URL is permissible everywhere we need to transmit an authentication token, then that means that we can, as you suggest, treat them opaquely. In other words, we can start treating the unpadded Base64URL encoding of some bytes as the canonical form of the token.

Long story short: I want to implement your suggestion. I think this PR is the right place to do it, as well.

edit: I should acknowledge that several people, including jbr, have been telling me to do this for some time, but I was too dense and hung up on my incorrect understanding of RFC 6750 to listen.

Comment on lines 419 to 421
/// If this aggregator is the leader, this is the token to use to authenticate requests to
/// the helper, as Base64 encoded bytes. If this aggregator is the helper, the value is
/// `None`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the token is now a composite value

Suggested change
/// If this aggregator is the leader, this is the token to use to authenticate requests to
/// the helper, as Base64 encoded bytes. If this aggregator is the helper, the value is
/// `None`.
/// If this aggregator is the leader, this is the token to use to authenticate requests to
/// the helper. If this aggregator is the helper, the value is `None`.

aggregator_api/src/lib.rs Show resolved Hide resolved
@tgeoghegan tgeoghegan marked this pull request as draft July 1, 2023 23:00
@tgeoghegan
Copy link
Contributor Author

Downgrading to a draft; I'll tinker with this while on trains next week.

@tgeoghegan tgeoghegan force-pushed the timg/aggregator-api-bearer-token-default branch from 5422828 to 8b4e9ea Compare July 26, 2023 23:52
@tgeoghegan tgeoghegan marked this pull request as ready for review July 27, 2023 23:20
@tgeoghegan
Copy link
Contributor Author

We now use Base64URL unpadded everywhere, and this works in conjunction with the changes in divviup/divviup-api#247 to do an end to end test where we use the BYOHelper flow to pair aggregators, provision a task and then run an aggregation. This is ready for review.

@tgeoghegan tgeoghegan requested review from divergentdave and a team July 27, 2023 23:21
@tgeoghegan tgeoghegan force-pushed the timg/aggregator-api-bearer-token-default branch from 4b6cce2 to 9a9002d Compare July 31, 2023 19:12
tgeoghegan added a commit to divviup/divviup-api that referenced this pull request Aug 1, 2023
As of [1], the Janus aggregator API is explicit about which kind of
authentication token it uses (`Bearer` or `DapAuth`) and also generates
bearer tokens by default. This PR adopts changes to the aggregator API
message definitions.

[1]: divviup/janus#1548
@tgeoghegan tgeoghegan force-pushed the timg/aggregator-api-bearer-token-default branch from 9a9002d to 69a9546 Compare August 1, 2023 17:59
Copy link
Contributor

@divergentdave divergentdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this will be a disruptive change, I think it will be a nice improvement to have all auth tokens have the same representation in memory, on the wire in DAP requests, on the wire in aggregator API requests, in YAML files, and on the command line. We should mention this in the next release's notes, and also communicate it to our partners that have currently deployed Janus for testing purposes. (for example, old task YAML files may no longer parse, bearer-type authentication tokens in a database would be interpreted differently when loading and will likely throw an error)

// unfortunate but necessary since other arms must allocate.
Self::DapAuth(token) => (DAP_AUTH_HEADER, token.as_ref().to_vec()),
Self::Bearer(token) => (AUTHORIZATION.as_str(), format!("Bearer {}", token.as_str())),
// Cloning is unfortunate but necessary since other arms must allocate.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a good place for a Cow<'_, str>, but it's moot because it'll likely get moved, cloned, or copied into a request structure.

@@ -606,8 +578,7 @@ mod models {
min_batch_size: task.min_batch_size(),
time_precision: *task.time_precision(),
tolerable_clock_skew: *task.tolerable_clock_skew(),
aggregator_auth_token: URL_SAFE_NO_PAD
.encode(task.aggregator_auth_tokens()[0].as_ref()),
aggregator_auth_token: task.aggregator_auth_tokens()[0].clone(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use task.primary_aggregator_auth_token(), instead, which the last token in the list, not the first.

}
Some(task.collector_auth_tokens()[0].clone())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below, task.primary_collector_auth_token()

Previously, the aggregator API assumed that any aggregator or collector
auth tokens it was handling were `AuthenticationToken::DapAuth`. Janus
must support those for the interop testing API and to work with Daphne,
but generally, we prefer to use more boring `Authorization: Bearer`
tokens. Certainly any tasks provisioned via the aggregator API and
`divviup-api` should use bearer tokens.

With this PR, we augment the `PostTaskReq` and `PostTaskResp` messages
so that they use `AuthenticationToken` to represent aggregator and
collector tokens, meaning that on the wire, a token now looks like:

```
{
  "type": "Bearer",
  "token": "AAAAAAAAA<etc.>"
}
```

Also, when the aggregator API generates either kind of token, it now
generates a bearer token. I'm not aware of any scenarios where we need
to generate `DapAuth` tokens in the aggregator API so there is no
affordance for requesting that kind of token.

See #472
@tgeoghegan tgeoghegan force-pushed the timg/aggregator-api-bearer-token-default branch from 69a9546 to c85a508 Compare August 1, 2023 20:19
@tgeoghegan tgeoghegan merged commit 1da6547 into main Aug 1, 2023
7 checks passed
@tgeoghegan tgeoghegan deleted the timg/aggregator-api-bearer-token-default branch August 1, 2023 23:00
tgeoghegan added a commit to divviup/divviup-api that referenced this pull request Aug 1, 2023
As of [1], the Janus aggregator API is explicit about which kind of
authentication token it uses (`Bearer` or `DapAuth`) and also generates
bearer tokens by default. This PR adopts changes to the aggregator API
message definitions.

[1]: divviup/janus#1548
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.

2 participants