Add secret_token support to telegram_bot component #100869

zehuanli · 2023-09-25T17:38:16Z

Proposed change

This is an updated PR over #100787. There will be no changes to the YAML configuration. In fact, the creation of Telegram Bot webhook secret token does not need user's entry or any other interactions. The secret token is generated automatically if not exists, and will be stored in Store.

Below is the original proposal in the previous PR.

Supports secret_token in Telegram setWebHook API. This is to prevent an adversary from sending crafted messages to the Telegram webhook, as an additional layer of protection to the trusted networks.

It should be noted that this component's dependency, python-telegram-bot, adds explicit support for secret_token as of the latest version v20.5 (currently v13.1 is used). However, this new version introduces many breaking changes to not only this component telegram_bot, but also telegram, which it is not considered given the simplicity of this feature being added.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Deprecation (breaking change to happen in the future)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
I have followed the perfect PR recommendations
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

…stead

emontnemery

Please fix the lint errors, your PR will not be merged unless they are fixed.
If you follow the development guide, the linters are automatically run when you commit.

Also, why do we need to store a token in a store, why don't we just generate a new one every time Home Assistant is restarted?

home-assistant · 2023-09-27T11:43:10Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

zehuanli · 2023-09-27T22:01:30Z

The code has been reformatted accordingly.

Indeed, it's possible to generate a new secret token for the webhook each time Home Assistant restarts. I've modified the code to eliminate the need for storage; now, the secret token will simply be retained in memory for the duration of each session.

emontnemery

LGTM, thanks @zehuanli 👍

homeassistant/components/telegram_bot/webhooks.py

emontnemery · 2023-09-28T08:33:13Z

Tests are failing.

You can for example patch telegram.bot.Bot.set_webhook to get to know the secret_token, then use that in the test:

-    response = await client.post(TELEGRAM_WEBHOOK_URL, json=update_message_text)
+    response = await client.post(
+        TELEGRAM_WEBHOOK_URL,
+        json=update_message_text,
+        headers={"X-Telegram-Bot-Api-Secret-Token": `secret_token`},
+    )

If it's unclear how to achieve this, comment here or contact me on discord (@emontnemery).

emontnemery

One more suggestion. Tests also need to be fixed.

homeassistant/components/telegram_bot/webhooks.py

zehuanli · 2023-09-28T13:57:34Z

You can for example patch telegram.bot.Bot.set_webhook to get to know the secret_token, then use that in the test

I'm not sure if this is necessary. For the setWebhook API, the token is sent as HTTP POST data, not a header.

I think the test failed because the header X-Telegram-Bot-Api-Secret-Token may not exist in the request to the webhook. I have modified the code to address this scenario: if the header is absent/mismatched while the secret_token exists, access will be denied.

homeassistant/components/telegram_bot/webhooks.py

emontnemery · 2023-09-28T14:36:42Z

I think the test failed because the header X-Telegram-Bot-Api-Secret-Token may not exist in the request to the webhook.

Right, it originally failed because of that.
But now that you correctly handle absence of the token, tests fail because the token is not there.

Hence, you need to fix the tests as I suggested earlier such that the correct secret token is included in the requests.

Also, tests should be added to show that:

Requests without a secret token are rejected
Requests with the wrong secret token are rejected

zehuanli · 2023-09-28T14:48:02Z

Hence, you need to fix the tests as I suggested earlier such that the correct secret token is included in the requests.

Ah, I see what you mean now. I'll look into this.

zehuanli · 2023-09-28T17:14:06Z

The tests have been fixed to incorporate the secret token. As you pointed out, two new tests have been added:

Requests without a secret token are rejected
Requests with the wrong secret token are rejected

I removed the check for the secret token variable since it's created unconditionally.

tests/components/telegram_bot/conftest.py

homeassistant/components/telegram_bot/webhooks.py

zehuanli · 2023-09-29T17:35:53Z

Patching method has been changed; use side_effect on secrets.choice.

emontnemery

Thanks, @zehuanli 👍

Nice first PR! 🎉

MartinHjelmare

Please address the comment in a new PR. Thanks!

MartinHjelmare · 2023-10-02T11:36:47Z

homeassistant/components/telegram_bot/webhooks.py

-    pushbot = PushBot(hass, bot, config)
+
+    # Generate an ephemeral secret token
+    alphabet = string.ascii_letters + string.digits + "-_"


Please use f-strings instead of string concatenation.

home-assistant bot added cla-signed integration: telegram_bot new-feature small-pr PRs with less than 30 lines. Quality Scale: No score labels Sep 25, 2023

zehuanli added 2 commits September 25, 2023 13:56

Support secret_token for setWebHook api

2b11e20

Revert configuration YAML changes; generate and store secret token in…

b7dfcf6

…stead

zehuanli force-pushed the master branch from 3385eaa to b7dfcf6 Compare September 25, 2023 17:56

edenhaus changed the title ~~(Updated) Add secret_token support to telegram_bot component~~ Add secret_token support to telegram_bot component Sep 26, 2023

emontnemery requested changes Sep 27, 2023

View reviewed changes

home-assistant bot marked this pull request as draft September 27, 2023 11:43

zehuanli added 3 commits September 27, 2023 17:20

Reformat codes

dd99cd0

Revert storage of secret token; use ephemeral secret token instead

38d2f2e

Reformat

64f9443

zehuanli marked this pull request as ready for review September 27, 2023 22:01

home-assistant bot requested a review from emontnemery September 27, 2023 22:01

emontnemery approved these changes Sep 28, 2023

View reviewed changes

homeassistant/components/telegram_bot/webhooks.py Outdated Show resolved Hide resolved

Update homeassistant/components/telegram_bot/webhooks.py

c2d9ae5

emontnemery self-requested a review September 28, 2023 08:22

emontnemery requested changes Sep 28, 2023

View reviewed changes

homeassistant/components/telegram_bot/webhooks.py Outdated Show resolved Hide resolved

home-assistant bot marked this pull request as draft September 28, 2023 08:34

Fix when header is not present

e285fc1

Check for non-empty token

c1d7571

zehuanli marked this pull request as ready for review September 28, 2023 14:05

home-assistant bot requested a review from emontnemery September 28, 2023 14:05

emontnemery reviewed Sep 28, 2023

View reviewed changes

homeassistant/components/telegram_bot/webhooks.py Outdated Show resolved Hide resolved

zehuanli marked this pull request as draft September 28, 2023 14:48

zehuanli added 3 commits September 28, 2023 12:41

Fix tests to support secret token

f2c6c36

Add tests for invalid secret token

122159c

Minor: remove comment

47a4fe0

zehuanli marked this pull request as ready for review September 28, 2023 17:10

home-assistant bot requested a review from emontnemery September 28, 2023 17:11

zehuanli added 2 commits September 28, 2023 13:30

Revert back to 401

7fdd4ff

... and for tests

0059761

emontnemery requested changes Sep 29, 2023

View reviewed changes

tests/components/telegram_bot/conftest.py Outdated Show resolved Hide resolved

homeassistant/components/telegram_bot/webhooks.py Outdated Show resolved Hide resolved

home-assistant bot marked this pull request as draft September 29, 2023 11:00

Change patching method for the generation of secret tokens

6a3d5f2

zehuanli marked this pull request as ready for review September 29, 2023 17:36

home-assistant bot requested a review from emontnemery September 29, 2023 17:36

emontnemery approved these changes Oct 2, 2023

View reviewed changes

emontnemery merged commit 41cb852 into home-assistant:dev Oct 2, 2023
21 checks passed

MartinHjelmare reviewed Oct 2, 2023

View reviewed changes

github-actions bot locked and limited conversation to collaborators Oct 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add secret_token support to telegram_bot component #100869

Add secret_token support to telegram_bot component #100869

zehuanli commented Sep 25, 2023 •

edited

emontnemery left a comment •

edited

home-assistant bot commented Sep 27, 2023

zehuanli commented Sep 27, 2023

emontnemery left a comment

emontnemery commented Sep 28, 2023

emontnemery left a comment

zehuanli commented Sep 28, 2023 •

edited

emontnemery commented Sep 28, 2023

zehuanli commented Sep 28, 2023

zehuanli commented Sep 28, 2023 •

edited

zehuanli commented Sep 29, 2023

emontnemery left a comment

MartinHjelmare left a comment

MartinHjelmare Oct 2, 2023

Add secret_token support to telegram_bot component #100869

Add secret_token support to telegram_bot component #100869

Conversation

zehuanli commented Sep 25, 2023 • edited

Proposed change

Type of change

Additional information

Checklist

emontnemery left a comment • edited

Choose a reason for hiding this comment

home-assistant bot commented Sep 27, 2023

zehuanli commented Sep 27, 2023

emontnemery left a comment

Choose a reason for hiding this comment

emontnemery commented Sep 28, 2023

emontnemery left a comment

Choose a reason for hiding this comment

zehuanli commented Sep 28, 2023 • edited

emontnemery commented Sep 28, 2023

zehuanli commented Sep 28, 2023

zehuanli commented Sep 28, 2023 • edited

zehuanli commented Sep 29, 2023

emontnemery left a comment

Choose a reason for hiding this comment

MartinHjelmare left a comment

Choose a reason for hiding this comment

MartinHjelmare Oct 2, 2023

Choose a reason for hiding this comment

zehuanli commented Sep 25, 2023 •

edited

emontnemery left a comment •

edited

zehuanli commented Sep 28, 2023 •

edited

zehuanli commented Sep 28, 2023 •

edited