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

Feature flag for requiring global HPKE keys #3268

Merged
merged 3 commits into from
Jul 8, 2024
Merged

Conversation

inahga
Copy link
Contributor

@inahga inahga commented Jun 26, 2024

Supports #2147.

Introduces a feature flag require_global_hpke_keys. When this is enabled:

  • The GlobalHpkeKeypairCache blocks and retries fetching keys until it gets a list containing at least one active key. This is a bootstrapping case--it allows for the key rotator to do initialization, whether it's running in-process or separately.
  • The /hpke_configs endpoint unconditionally returns global keys, regardless of the ?task_id parameter.

Draft, as it needs refactoring and more testing. Looking for feedback on whether the overall approach is sane.

Copy link
Member

@branlwyd branlwyd left a comment

Choose a reason for hiding this comment

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

The approach SGTM. 👍🏻

/// Experimental. Always advertise global HPKE keys instead of per-task HPKE keys. This will
/// become on by default in a future version of Janus.
#[serde(default)]
pub require_global_hpke_keys: Option<bool>,
Copy link
Member

Choose a reason for hiding this comment

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

nit: is None the same as Some(false)? if so, for simplicity I think this should just be a bool which defaults to false, so that we only interpret a missing value once.

@inahga inahga force-pushed the inahga/require-global-keys branch from 3503372 to aeaba92 Compare July 5, 2024 17:47
@inahga inahga force-pushed the inahga/require-global-keys branch from aeaba92 to 1543ca7 Compare July 5, 2024 17:48
@inahga inahga marked this pull request as ready for review July 5, 2024 18:17
@inahga inahga requested a review from a team as a code owner July 5, 2024 18:17
@inahga
Copy link
Contributor Author

inahga commented Jul 5, 2024

Eh, pulling this back to draft, I think some work on integration/interop tests is justified here.

@inahga inahga marked this pull request as draft July 5, 2024 18:18
@inahga
Copy link
Contributor Author

inahga commented Jul 5, 2024

I've made integration and inteorp tests use the flag. I'll make unit tests use it in the next PR, to keep this PR relatively small.

@inahga inahga assigned inahga and unassigned inahga Jul 5, 2024
@inahga inahga marked this pull request as ready for review July 8, 2024 15:25
@inahga inahga merged commit 4bd3236 into main Jul 8, 2024
8 checks passed
@inahga inahga deleted the inahga/require-global-keys branch July 8, 2024 22:17
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.

3 participants