Tasks have only one aggregator, collector token #1973

tgeoghegan · 2023-09-21T05:54:21Z

We've decided that each task has exactly one aggregator and collector auth token, and rotating those means rotating the task. This commit updates the representations of auth tokens in the database and in memory (aggregator_core::task::Task) accordingly.

Several tests relied upon tasks constructed in tests having two aggregator auth tokens, one of each supported type. Those tests are fixed to explicitly construct tasks using DAP-Auth-Token tokens where necessary.

Given that there is now a single aggregator or collector auth token per task, we could remove the task_aggregator_auth_tokens and task_collector_auth_tokens tables and instead add columns aggregator_auth_token, aggregator_auth_token_type, collector_auth_token and collector_auth_token_type to tasks.

I chose not to do this because validating the correctness of those columns would be tricky. We couldn't make them all NOT NULL, because a helper task doesn't have a collector auth token and a task provisioned via taskprov won't have either token (instead it'll use tokens from the taskprov_*_auth_tokens tables). So we would have to write constraints on the tasks table to ensure pairs of columns are either NULL or NOT NULL in tandem, which I think are expressed more clearly in the independent task_*_auth_tokens tables. Plus, if we ever do decide to support auth token rotation, it'll be easier to do with these tables in place.

Part of #1524, #1521

We've decided that each task has exactly one aggregator and collector auth token, and rotating those means rotating the task. This commit updates the representations of auth tokens in the database and in memory (`aggregator_core::task::Task`) accordingly. Several tests relied upon tasks constructed in tests having two aggregator auth tokens, one of each supported type. Those tests are fixed to explicitly construct tasks using `DAP-Auth-Token` tokens where necessary. Given that there is now a single aggregator or collector auth token per task, we could remove the `task_aggregator_auth_tokens` and `task_collector_auth_tokens` tables and instead add columns `aggregator_auth_token`, `aggregator_auth_token_type`, `collector_auth_token` and `collector_auth_token_type` to `tasks`. I chose not to do this because validating the correctness of those columns would be tricky. We couldn't make them all `NOT NULL`, because a helper task doesn't have a collector auth token and a task provisioned via taskprov won't have either token (instead it'll use tokens from the `taskprov_*_auth_tokens` tables). So we would have to write constraints on the `tasks` table to ensure pairs of columns are either `NULL` or `NOT NULL` in tandem, which I think are expressed more clearly in the independent `task_*_auth_tokens` tables. Plus, if we ever _do_ decide to support auth token rotation, it'll be easier to do with these tables in place. Part of #1524, #1521

branlwyd

I would eliminate the separate tables for auth tokens; with this PR, the only reason they exist is because we used to support multiple tokens. If we were to implement the token functionality for the first time, we would not add these tables. I understand the argument that it is challenging to express the necessary constraints, but we could either lean on application-level checks only or implement a CHECK constraint in the DB.

branlwyd · 2023-09-21T22:49:43Z

aggregator_core/src/datastore.rs

-            row_id[TaskId::LEN..].copy_from_slice(&ord.to_be_bytes());
-
+        // Aggregator auth token.
+        let aggregator_auth_token_future = if let Some(token) = task.aggregator_auth_token() {


A more natural way to express this idea would be to move the if & all related bits "into" the future created by async move. Something like:

let aggregator_auth_tokens_future = async move { if let Some(token) = task.aggregator_auth_token() { /* ... code currently inside this if */ } }

(likely with some additional code to move or copy or Arc-ify data needed inside the future)

This is a fairly marginal upside, but this strategy would also allow the statement preparation IO to occur concurrently; as-written (before this PR & with the current PR changes) these preparations would be serialized.

This is moot because now that the distinct auth token tables are gone, we don't need to do these inserts separately from the one in tasks at all.

branlwyd · 2023-09-21T22:56:39Z

aggregator_core/src/datastore.rs

-            let ord: i64 = row.get("ord");
-            let auth_token_type: AuthenticationTokenType = row.get("type");
-            let encrypted_aggregator_auth_token: Vec<u8> = row.get("token");
+        let aggregator_auth_token = if let Some(row) = aggregator_auth_token_row {


This would be more clearly/concisely expressed by aggregator_auth_token_row.map(/* ... code currently inside this if ... */).transpose()?.

(assuming the error types work out, it's hard to reason about the operation of the ? operator without an IDE)

I was a little surprised to see that I could write an Option::map closure with two different fallible calls in it by using Option::transpose, but it works, along with an Option::zip since we no longer have a distinct Option<Row> for the auth tokens.

tgeoghegan · 2023-09-22T00:38:04Z

I would eliminate the separate tables for auth tokens; with this PR, the only reason they exist is because we used to support multiple tokens. If we were to implement the token functionality for the first time, we would not add these tables. I understand the argument that it is challenging to express the necessary constraints, but we could either lean on application-level checks only or implement a CHECK constraint in the DB.

Done, the auth token tables are gone.

inahga

Nice simplification.

inahga · 2023-09-22T14:23:59Z

aggregator_core/src/task.rs

+    /// Token used to authenticate messages sent to or received from the other aggregator. Only set
+    /// if the task was not created via taskprov.
+    aggregator_auth_token: Option<AuthenticationToken>,
+    /// Token used to authenticate messages sent to received from the collector. Only set if this


Suggested change

/// Token used to authenticate messages sent to received from the collector. Only set if this

/// Token used to authenticate messages sent to or received from the collector. Only set if this

inahga · 2023-09-22T14:28:34Z

db/00000000000001_initial_schema.up.sql

+    aggregator_auth_token_type  AUTH_TOKEN_TYPE,    -- the type of the authentication token
+    aggregator_auth_token       BYTEA,              -- encrypted bearer token
+    -- The aggregator_auth_token columns must either both be NULL or both be non-NULL
+    CONSTRAINT aggregator_auth_token_null CHECK ((aggregator_auth_token_type IS NULL) = (aggregator_auth_token IS NULL)),


WDYT about unit tests that exercise this constraint, i.e. try to insert values that violate it?

This is my main worry with inserting business logic into the database--it's difficult to test for. But I think our test harness can support such a test without too much pain.

Good idea. Our definition of struct Task doesn't allow constructing a value where the auth token is set but not its type (or vice-versa), but it's easy enough to write SQL UPDATE statements that exercise these constraints with illegal mutations to an existing task so I did that.

divergentdave · 2023-09-22T16:00:30Z

docs/samples/tasks.yaml

  # authenticate leader-to-helper requests. In the case of a leader-role task,
-  # the leader will include the first token in a header when making requests to


Oof, this was wrong the whole time. (the last token was the primary auth token)

inahga

💯

tgeoghegan added the allow-changed-migrations Override the ci-migrations check to allow migrations that have changed. label Sep 21, 2023

tgeoghegan force-pushed the timg/aggregator-auth-token-hash branch from 732c7f9 to bde5aa8 Compare September 21, 2023 15:50

tgeoghegan force-pushed the timg/aggregator-auth-token-hash branch from bde5aa8 to a647d83 Compare September 21, 2023 16:38

tgeoghegan changed the title ~~WIP: Exactly one aggregator and collector auth token per task~~ Tasks have only one aggregator, collector token Sep 21, 2023

tgeoghegan marked this pull request as ready for review September 21, 2023 16:43

tgeoghegan requested a review from a team as a code owner September 21, 2023 16:43

tgeoghegan added this to the Change representation of tasks in datastore milestone Sep 21, 2023

branlwyd suggested changes Sep 21, 2023

View reviewed changes

review feedback

0273c6d

tgeoghegan requested a review from branlwyd September 22, 2023 00:40

inahga approved these changes Sep 22, 2023

View reviewed changes

divergentdave approved these changes Sep 22, 2023

View reviewed changes

branlwyd approved these changes Sep 22, 2023

View reviewed changes

review feedback

5fca9e0

inahga approved these changes Sep 22, 2023

View reviewed changes

check that test fails due to constraint

c32d41d

tgeoghegan enabled auto-merge (squash) September 22, 2023 17:14

tgeoghegan merged commit aca9f14 into main Sep 22, 2023
8 checks passed

tgeoghegan deleted the timg/aggregator-auth-token-hash branch September 22, 2023 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tasks have only one aggregator, collector token #1973

Tasks have only one aggregator, collector token #1973

tgeoghegan commented Sep 21, 2023 •

edited

Loading

branlwyd left a comment

branlwyd Sep 21, 2023

branlwyd Sep 21, 2023

tgeoghegan Sep 22, 2023

branlwyd Sep 21, 2023

branlwyd Sep 21, 2023

tgeoghegan Sep 22, 2023

tgeoghegan commented Sep 22, 2023

inahga left a comment

inahga Sep 22, 2023

inahga Sep 22, 2023

tgeoghegan Sep 22, 2023

divergentdave Sep 22, 2023

inahga left a comment

	/// Token used to authenticate messages sent to received from the collector. Only set if this
	/// Token used to authenticate messages sent to or received from the collector. Only set if this

		# authenticate leader-to-helper requests. In the case of a leader-role task,
		# the leader will include the first token in a header when making requests to

Tasks have only one aggregator, collector token #1973

Tasks have only one aggregator, collector token #1973

Conversation

tgeoghegan commented Sep 21, 2023 • edited Loading

branlwyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgeoghegan commented Sep 22, 2023

inahga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inahga left a comment

Choose a reason for hiding this comment

tgeoghegan commented Sep 21, 2023 •

edited

Loading