Skip to content

Add HMAC capabilities #181

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: v0.x.x
Choose a base branch
from
Open

Add HMAC capabilities #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55519b0
Add HMAC capabilities
florian-wagner-frequenz May 22, 2025
114af31
Drop `-Werror` and replace with test warning
florian-wagner-frequenz May 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@

## New Features

<!-- Here goes the main new features and examples or instructions on how to use them -->
* Add HMAC generation capabilities.
* The new CLI option "key" can be used to provide the server's key.
* The client itself now has a "key" argument in the constructor.

## Bug Fixes

Expand Down
4 changes: 2 additions & 2 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ dependencies = [
"frequenz-client-common >= 0.3.0, < 0.4",
"grpcio >=1.70.0, < 2",
"protobuf >= 5.29.3, < 7",
"frequenz-client-base >= 0.8.0, < 0.10.0",
"frequenz-client-base >= 0.10.0, < 0.11.0",
]
dynamic = ["version"]

Expand Down Expand Up @@ -155,7 +155,7 @@ disable = [
]

[tool.pytest.ini_options]
addopts = "-W=all -Werror -Wdefault::DeprecationWarning -Wdefault::PendingDeprecationWarning -vv"
addopts = "-W=all -Wdefault::DeprecationWarning -Wdefault::PendingDeprecationWarning -vv"
Copy link

Choose a reason for hiding this comment

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

-Werror should be kept

Copy link
Author

Choose a reason for hiding this comment

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

okay, but I think then I might need to put the specific warning back in. This is the fix Flora applied.

Copy link

Choose a reason for hiding this comment

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

oh, really, I didn't realized that, I guess I missed that change as there were other changes too in that line. Then something went wrong. @flora-hofmann-frequenz :-/

Removing -Werror means we don't generally treat warnings as errors anymore. I will have a look.

Copy link

Choose a reason for hiding this comment

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

OK, I found a fix, and while at it, tried a few improvements, with these options we don't need to add the conftest.py file at all!

[tool.pytest.ini_options]
addopts = "-vv"
filterwarnings = [
  "error",
  "once::DeprecationWarning",
  "once::PendingDeprecationWarning",
  # We use a raw string (single quote) to avoid the need to escape special
  # chars as this is a regex
  'ignore:Protobuf gencode version .*exactly one major version older.*:UserWarning',
]

I think it is better to complete ignore the protobuf warnings, and also not care about the reported version (so we don't need to update the ignore rule when we bump the protobuf version), we know we are doing this, so it only adds noise to the tests to show it. We'll get a proper error if we go beyond the supported range anyways.

Also show deprecations only once, we don't need to get spammed if a deprecated symbol is used more than once.

I will add these improvements to repo-config.

Copy link

Choose a reason for hiding this comment

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

testpaths = ["tests", "src"]
asyncio_mode = "auto"
asyncio_default_fixture_loop_scope = "function"
Expand Down
21 changes: 19 additions & 2 deletions src/frequenz/client/reporting/cli/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
from pprint import pprint
from typing import AsyncIterator

from frequenz.client.base.authentication import AuthenticationOptions
from frequenz.client.base.channel import ChannelOptions
from frequenz.client.base.signing import SigningOptions
from frequenz.client.common.metric import Metric

from frequenz.client.reporting import ReportingApiClient
Expand Down Expand Up @@ -85,6 +88,12 @@ def main() -> None:
help="API key",
default=None,
)
parser.add_argument(
"--secret",
type=str,
help="The secret for the reporting service",
default=None,
)
args = parser.parse_args()
asyncio.run(
run(
Expand All @@ -99,6 +108,7 @@ def main() -> None:
service_address=args.url,
key=args.key,
fmt=args.format,
secret=args.secret,
)
)

Expand All @@ -116,6 +126,7 @@ async def run( # noqa: DOC502
bounds: bool,
service_address: str,
key: str,
secret: str,
Copy link

Choose a reason for hiding this comment

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

Maybe call this signing_secret for extra clarity?

Copy link
Author

Choose a reason for hiding this comment

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

Gladly

fmt: str,
) -> None:
"""Test the ReportingApiClient.
Expand All @@ -130,13 +141,19 @@ async def run( # noqa: DOC502
states: include states in the output
bounds: include bounds in the output
service_address: service address
key: API key
key: API key (per customer)
secret: cryptographic secret of the service (per API)
fmt: output format

Raises:
ValueError: if output format is invalid
"""
client = ReportingApiClient(service_address, key)
auth = AuthenticationOptions(key)
Copy link
Preview

Copilot AI May 23, 2025

Choose a reason for hiding this comment

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

Always constructing SigningOptions with a potentially None secret can lead to runtime errors; add a guard to only initialize it when secret is provided or raise a clear error if missing.

Suggested change
auth = AuthenticationOptions(key)
auth = AuthenticationOptions(key)
if not secret:
raise ValueError("The 'secret' parameter is required and cannot be None.")

Copilot uses AI. Check for mistakes.

sign = SigningOptions(secret)

channel_options = ChannelOptions(auth=auth, sign=sign)

client = ReportingApiClient(service_address, key, channel_defaults=channel_options)
Comment on lines +151 to +156
Copy link

Choose a reason for hiding this comment

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

This looks pretty inconsistent, IMHO this should be done inside the client class, and the signing secret should be taken as a separate argument, like with key. The options passed here are default options. There is also an overlap, because the client is doing its own key management, which if it doesn't collide with the new auth interceptor, it will overlap.

Looking at the ReportingApiClient.__init__, I feel like the current approach we taken with the auth key and sign secret is not consistent with the default channel options :-/

All channel options were designed to be mainly set from the server_url, and the channel_defauls is supposed to just pass defaults, but the real end values are configured in the server_url if different to the default. But we don't support setting the auth key and sign secret. This maybe makes sense in a way, as it probably makes more sense to pass secrets in separate env vars in the future, instead of treating the whole server_url as a secret. But it doesn't make sense to use a default secret that is not None, as either you want to use a secret, in which case you need to specify one, or you don't, but there is no default secret one can provide.

I think we should probably change the base client to pass secrets outside the default_options to the client. Which means there are no more sign and auth option in channel options. So the base client ctor will take a auth_key and sign_secret as explicit arguments defaulting to None. So you either pass it or not, but there is no default. Sorry @florian-wagner-frequenz 🙏, I think this was your original approach and I suggested using the channel options. I think when I proposed it the idea was also to add eventually support to setting this via env vars, which is more reasonable, but I also agree reading env vars deep in some library, specially if they are secrets, is a bad idea.

A for a temporary solution I suggest changing the current ReportingApiClient.__init__ to something like:

class ReportingApiClient(BaseApiClient[ReportingStub]):
    """A client for the Reporting service."""


    def __init__(
        self,
        server_url: str,
        auth_key: str | None = None,
        sign_secret: str | None = None,
        connect: bool = True,
        channel_defaults: ChannelOptions = ChannelOptions(),  # default options
    ) -> None:
        channel_defaults = channel_defaults.replace(sign=SigningOptions(secret=sign_secret))
        channel_defaults = channel_defaults.replace(auth=AuthenticationOptions(api_key=auth_key))
        super().__init__(
            server_url,
            ReportingStub,
            connect=connect,
            channel_defaults=channel_defaults,
        )

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I will do the temporary fix first and see how far I get with doing it properly in the base client. I agree with your arguments.
For me the core issue is:

  • key and secret are not optional (as the service won't respond), so shouldn't live in the channel_defaults (as we can not conjure them from thin air
  • We don't want to pull them from an environment variable deep in the library (compounding the previous issue)
  • We want them to stay secret, so we don't want them in the server_url

So the targeted interface for the clients/apps stays to pass via command line argument (possibly from environment variable).
This of course pushes the burden to app maintainers, but I think that is unavoidable unless we want the "pull from environment variable in library" solution.


metrics = [Metric[mn] for mn in metric_names]

Expand Down
13 changes: 13 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# License: MIT
# Copyright © 2025 Frequenz Energy-as-a-Service GmbH

"""Show Protobuf version warning during tests, but don't treat as error."""

import warnings

warnings.filterwarnings(
"once",
message=r"Protobuf gencode version 5\..*exactly one major version older.*",
category=UserWarning,
)

Loading