subscriber-01: Specialise to only one peer aggregator #1976
Conversation
inahga
force-pushed
the
inahga/sub01-taskprov-config
branch
5 times, most recently
from
d4714f7 to
b91106d
Compare
| let request_token = aggregator_auth_token.ok_or(Error::UnauthorizedRequest(*task_id))?; | ||
| if !self | ||
| .cfg | ||
| .auth_tokens | ||
| .iter() | ||
| .map(|url| url.try_into()) | ||
| .collect::<Result<Vec<Url>, _>>()?; | ||
| if aggregator_urls.len() < 2 { | ||
| return Err(Error::UnrecognizedMessage( | ||
| Some(*task_id), | ||
| "taskprov configuration is missing one or both aggregators", | ||
| )); | ||
| } | ||
| let peer_aggregator_url = &aggregator_urls[peer_role.index().unwrap()]; | ||
|
|
||
| let peer_aggregator = self | ||
| .peer_aggregators | ||
| .get(peer_aggregator_url, peer_role) | ||
| .ok_or(Error::InvalidTask( | ||
| *task_id, | ||
| OptOutReason::NoSuchPeer(*peer_role), | ||
| ))?; | ||
|
|
||
| if !aggregator_auth_token | ||
| .map(|t| peer_aggregator.check_aggregator_auth_token(t)) | ||
| .unwrap_or(false) | ||
| .any(|token| token == request_token) |
There was a problem hiding this comment.
This can (and should be) replaced with a trillium handler, along with rejecting non-taskprov requests. However, I'm leaving that for a later PR to keep this one simpler.
Use a global aggregator auth token
inahga
force-pushed
the
inahga/sub01-taskprov-config
branch
from
b91106d to
f21bf01
Compare
There was a problem hiding this comment.
I'm a little wary of doing more work on authentication on this branch because we don't yet understand the requirements, so effort we expend here may be thrown away. That said, this PR doesn't change how authentication works, just the arity of the tokens.
I'd also like to better understand the direction we want to take this release branch. Is the goal to strip out functionality to the point that it only implements a helper that can only have tasks provisioned via taskprov?
| @@ -171,6 +165,18 @@ fn build_aggregator_api_handler<'a>( | |||
| ))) | |||
| } | |||
|
|
|||
| fn parse_auth_tokens(auth_tokens: &[String]) -> Result<Vec<AuthenticationToken>> { | |||
There was a problem hiding this comment.
nit: take &[&str] if possible, or IntoIterator<Item = &str>, even.
There was a problem hiding this comment.
Neither turns out very straightforward--I don't think you can in-place turn a [String] into a [&str] at the call site, and IntoIterator requires lifetime annotations. For a library function I would try harder for IntoIterator, but I don't think this is improvement for a small private helper function.
| @@ -161,7 +165,21 @@ async fn make_handler( | |||
| max_upload_batch_size: 100, | |||
| max_upload_batch_write_delay: std::time::Duration::from_millis(100), | |||
| batch_aggregation_shard_count: 32, | |||
| ..Default::default() | |||
| global_hpke_configs_refresh_interval: GlobalHpkeKeypairCache::DEFAULT_REFRESH_INTERVAL, | |||
There was a problem hiding this comment.
It bothers me that the interop test stuff needs to be aware of taskprov at all. That doesn't really make sense, since taskprov isn't exercised in the interop test setup and shouldn't be used. If the higher level goal here is to move toward specializing the Janus on release/0.subscriber-01 to exclusively be a taskprov helper, then I think we could just drop support for the interop testing API altogether.
There was a problem hiding this comment.
Discussed in slack. We can move forward with this PR. If we have any more intrusive changes to the interop API, we should drop it.
This is the direction I'm vaguely aiming for, if everyone else thinks that's wise. I'm not sure that we should entirely remove the interop test harness quite yet, as we haven't determined what a taskprov E2E test will look like--such a decision IMO should wait until we have some engineering collaboration with our partner. We might be able to use the pattern of running their leader in a container. |
We're just filling them with bogus values rather than reading the config anyways.
inahga
force-pushed
the
inahga/sub01-taskprov-config
branch
from
60a5a4c to
5abd088
Compare
This deployment will have only one peer aggregator. Make simplifications based on that assumption.
The peer aggregator config is no longer stored in the database. Instead, keep non-secret parameters in the plaintext configuration, and have secret values supplied via environment variable (which in turn will be sourced from kubernetes secrets).
This incidentally simplifies the authentication mechanism--we no longer have to parse untrusted input before authenticating (note that we still do this, I'll leave the final optimization for a future PR to keep this one simple).
Supports #1728.