Support secrets and signing by Marenz · Pull Request #186 · frequenz-floss/frequenz-client-dispatch-python

Marenz · 2025-07-15T14:22:24Z

This PR adds support for request signing via an optional secret and propagates that change through tests, the client library, and the CLI.

llucax

LGTM in general, but I have a few comments.

llucax · 2025-07-17T12:55:07Z

mkdocs.yml

            show_signature_annotations: true
            show_source: true
            signature_crossrefs: true
+            members_order: source


I would leave thing the same as they are configured in other repos. I think making this inconsistent across repos will make the docs harder to navigate to users.

(or it will make this one repo easier and every other stays harder to navigate :P )

For me the side bar is an index, and as an index it should be sorted alphabetically. If you want to sort things in a more organized way, it might be better to have an overview at the module/class documentation.

mkdocs.yml

llucax · 2025-07-17T12:56:07Z

src/frequenz/client/dispatch/__main__.py

+    "--secret",
+    help="API signing secret for authentication",
+    envvar="DISPATCH_API_SECRET",


Suggested change

"--secret",

help="API signing secret for authentication",

envvar="DISPATCH_API_SECRET",

"--sign-secret",

help="API secret for signing",

envvar="DISPATCH_API_SIGN_SECRET",

Uh.. you wanna rename the env variable?
what about the API_KEY then? Consistently it should become DISPATCH_API_AUTH_KEY

Yes, that would be the ideal IMHO. If it is too disruptive by now, I would accept both for a transitional period, but again, not sure how possible that is using click's built-in env var management.

I mean, it's only used by ppl directly and it's very easy to adjust to it. I can make it print a warning or so if that var is set. It's not like an API. Just a TUI.

Right, yeah, not that disruptive. Sounds good to me!

src/frequenz/client/dispatch/__main__.py

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Copilot

Pull Request Overview

This PR adds support for request signing via optional secrets by updating the authentication mechanism from a simple key-based system to support both auth keys and optional signing secrets. Key changes include:

Replaces key parameter with auth_key and adds optional sign_secret parameter throughout the codebase
Updates CLI to support both deprecated --api-key and new --auth-key/--sign-secret options with proper deprecation warnings
Removes authentication/access control logic from the fake test service

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/test_cli.py	Updates test environment to use new auth key naming
src/frequenz/client/dispatch/test/client.py	Updates FakeClient constructor to use new auth_key parameter
src/frequenz/client/dispatch/test/_service.py	Removes authentication logic and metadata parameters from all service methods
src/frequenz/client/dispatch/_client.py	Updates DispatchApiClient to accept auth_key and sign_secret, removes manual metadata handling
src/frequenz/client/dispatch/main.py	Adds CLI support for new auth parameters with deprecation handling
pyproject.toml	Updates frequenz-client-base dependency version requirement
RELEASE_NOTES.md	Documents new secret signing feature

Comments suppressed due to low confidence (1)

src/frequenz/client/dispatch/test/client.py:27

The auth_key value "what" is unclear and non-descriptive. Consider using a more descriptive test value like "test_auth_key" or "mock_auth_key".

        super().__init__(server_url="mock", auth_key="what", connect=False)

RELEASE_NOTES.md

src/frequenz/client/dispatch/__main__.py

llucax

LGTM, only one small suggestion.

llucax · 2025-07-22T14:53:52Z

src/frequenz/client/dispatch/__main__.py

-    "--key",
-    help="API key for authentication",
+    "--api-key",
+    help="API key for authentication (deprecated, use DISPATCH_API_AUTH_KEY)",


Ah, nice, maybe include --api-key too.

Suggested change

help="API key for authentication (deprecated, use DISPATCH_API_AUTH_KEY)",

help="API key for authentication (deprecated, use --auth-key or DISPATCH_API_AUTH_KEY instead)",

llucax · 2025-07-22T14:55:30Z

Removed from the queue in case you want to address it.

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Copilot AI review requested due to automatic review settings July 15, 2025 14:22

Marenz requested review from a team as code owners July 15, 2025 14:22

Marenz requested review from shsms and stefan-brus-frequenz July 15, 2025 14:22

github-actions bot added part:docs Affects the documentation part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:cli Affects the command-line interface part:test-utils Affects the test utilities part:dispatcher labels Jul 15, 2025

This comment was marked as outdated.

Sign in to view

Marenz force-pushed the sign_here_please branch from 8e794f9 to dafa6ed Compare July 15, 2025 14:50

github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Jul 15, 2025

Marenz requested a review from Copilot July 15, 2025 14:50

This comment was marked as outdated.

Sign in to view

Marenz requested a review from ela-kotulska-frequenz July 16, 2025 08:41

Marenz enabled auto-merge July 16, 2025 09:14

llucax reviewed Jul 17, 2025

View reviewed changes

Marenz force-pushed the sign_here_please branch from dafa6ed to 66be679 Compare July 22, 2025 08:36

Marenz requested review from Copilot and llucax July 22, 2025 08:40

This comment was marked as outdated.

Sign in to view

Marenz added 3 commits July 22, 2025 15:38

Support secrets & signing

fc4c515

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Fix tests after updating base client

b0e9bca

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Mask both Auth Key and Signing Secret

ddb594b

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the sign_here_please branch from 66be679 to e47cb6f Compare July 22, 2025 13:38

Marenz requested a review from Copilot July 22, 2025 13:39

Copilot AI reviewed Jul 22, 2025

View reviewed changes

RELEASE_NOTES.md Outdated Show resolved Hide resolved

src/frequenz/client/dispatch/__main__.py Outdated Show resolved Hide resolved

Marenz force-pushed the sign_here_please branch 2 times, most recently from fff264b to 4a3020c Compare July 22, 2025 14:01

llucax previously approved these changes Jul 22, 2025

View reviewed changes

Marenz added this pull request to the merge queue Jul 22, 2025

llucax removed this pull request from the merge queue due to a manual request Jul 22, 2025

Rename api-key to auth-key and deprecate old parameter

833e531

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz dismissed llucax’s stale review via 833e531 July 22, 2025 15:04

Marenz force-pushed the sign_here_please branch from 4a3020c to 833e531 Compare July 22, 2025 15:04

Marenz merged commit 769f2bd into frequenz-floss:v0.x.x Jul 22, 2025
5 checks passed

Marenz deleted the sign_here_please branch July 29, 2025 11:56

	help="API key for authentication (deprecated, use DISPATCH_API_AUTH_KEY)",
	help="API key for authentication (deprecated, use --auth-key or DISPATCH_API_AUTH_KEY instead)",

Comments

Conversation

Marenz commented Jul 15, 2025 • edited by llucax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llucax commented Jul 22, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Marenz commented Jul 15, 2025 •

edited by llucax

Loading