greg/info-orbs
Watch
1
0
Fork
You've already forked info-orbs
 0
Code Issues 32 Pull requests 25 Projects Releases Packages Wiki Activity Actions

Move API keys to config #176

Open
RobThree wants to merge 3 commits from RobThree/apikeys_to_config into dev
pull from: RobThree/apikeys_to_config
merge into: greg:dev
Conversation 15 Commits 3 Files changed 4 +12 -10
RobThree commented 2024-12-10 22:25:12 +00:00 (Migrated from github.com)
Copy link

API keys should be configured in one single place, not be sprinkled and hardcoded in different files throughout the project.

API keys should be configured in one single place, not be sprinkled and hardcoded in different files throughout the project.
RobThree (Migrated from github.com) reviewed 2024-12-10 22:40:46 +00:00
firmware/config/config.h.template
@ -123,6 +123,7 @@
#define TIMEZONE_API_KEY "97R9WKDPBLIO"
#define TIMEZONE_API_URL "http://api.timezonedb.com/v2.1/get-time-zone"
#define WEATHER_API_KEY "XW2RDGD6XK432AF25BNK2A3C7"
#define STOCK_API_KEY "e03fc53524454ab8b65d91b23c669cc5"
RobThree (Migrated from github.com) commented 2024-12-10 22:40:46 +00:00
Copy link

Yes, this key has been "revoked", but apparently it keeps working (for some time)?

Yes, this key has been "revoked", but apparently it keeps working (for some time)?
brett-dot-tech commented 2024-12-22 20:32:30 +00:00 (Migrated from github.com)
Copy link

Hey @RobThree, this has been on my list for some time.

The reason they're a little bit "buried" right now is due to them being paid keys I wanted them as hidden as possbile.
I know that anyone that's looking could easily find them, but I thought by having them buried in the widget's it is atleast somewhat more secure than in the config file that everyone that uses the project looks at.

granted there are huge downsides to this, like the lack of ease of configurability, and I hope to soon figure out how to better obfuscate these paid keys (might just proxy the requests?) but untill then I'm going to leave this PR open.

I know this may sound silly, but should a bad actor get these keys and rate limit out the API to a point that I cant fund it further, it will result in everyones orbs going down for both weather and stocks.

Hey @RobThree, this has been on my list for some time. The reason they're a little bit "buried" right now is due to them being paid keys I wanted them as hidden as possbile. I know that anyone that's looking could easily find them, but I thought by having them buried in the widget's it is atleast somewhat more secure than in the config file that everyone that uses the project looks at. granted there are huge downsides to this, like the lack of ease of configurability, and I hope to soon figure out how to better obfuscate these paid keys (might just proxy the requests?) but untill then I'm going to leave this PR open. I know this may sound silly, but should a bad actor get these keys and rate limit out the API to a point that I cant fund it further, it will result in everyones orbs going down for both weather and stocks.
RobThree commented 2024-12-22 22:27:12 +00:00 (Migrated from github.com)
Copy link

and I hope to soon figure out how to better obfuscate these paid keys

Unless you're gonna proxy (which introduces a huge single point of failure) this is not a good idea.

I really don't see the point on why you keep insisting providing (paid or not) API keys for everyone to use (which in many cases may even be against the terms of use, aside the risks we talked about earlier like on bad actor getting everyone rate-limited of blocked). I get that you want to make things easy for people so they don't have to bother, but for those that do want to bother I'd say: why not make it easy for them too? Put the keys in a single place. It's highly unusual to provide (and "share") API keys with your project. In fact, I bet Github has warned you on several occasions about API keys being "leaked" as well.

To be perfectly clear: In the end it's all up to you, it's your project, your choice. We might not agree, but whatever you do: it is, and always will be, your choice.

> and I hope to soon figure out how to better obfuscate these paid keys Unless you're gonna proxy (which introduces a huge single point of failure) this is [not a good idea](https://en.wikipedia.org/wiki/Security_through_obscurity). I really don't see the point on why you keep insisting providing (paid or not) API keys for everyone to use (which in many cases may even be against the terms of use, aside the risks we talked about earlier like on bad actor getting everyone rate-limited of blocked). I get that you want to make things easy for people so they don't have to bother, but for those that _do_ want to bother I'd say: why not make it easy for them _too_? Put the keys in a single place. It's highly unusual to provide (and "share") API keys with your project. In fact, I bet Github has warned you on several occasions about API keys being "leaked" as well. To be perfectly clear: In the end it's all up to you, it's _your_ project, your choice. We might not agree, but whatever you do: it is, and always will be, _your_ choice.
lyricnz commented 2025-01-03 02:56:55 +00:00 (Migrated from github.com)
Copy link

I'm with @RobThree . Hiding things away to "protect" them from bad usage is no protection at all.

I'm with @RobThree . Hiding things away to "protect" them from bad usage is no protection at all.
sjoerdverweij commented 2025-01-07 20:56:22 +00:00 (Migrated from github.com)
Copy link

Including the API keys is terrible, yes. However, to do this properly:

  1. Someone has to write a proxy
  2. The proxy has to be hosted somewhere, at additional cost
  3. Most likely, the provider needs to be contacted to make sure they won't ban the account the minute they see a firehose of requests coming from a single IP -- almost all paid APIs have alerts on that and will axe you instantly, because if you're using a proxy, they have no way of knowing whether you're caching or not to save money.

As it stands, I don't think this is a big improvement either way, and has conflicts now, so I vote to close unless and until we get around to properly solving the problem.

Including the API keys is terrible, yes. However, to do this properly: 1) Someone has to write a proxy 2) The proxy has to be hosted somewhere, at additional cost 3) Most likely, the provider needs to be contacted to make sure they won't ban the account the minute they see a firehose of requests coming from a single IP -- almost all paid APIs have alerts on that and will axe you instantly, because if you're using a proxy, they have no way of knowing whether you're caching or not to save money. As it stands, I don't think this is a big improvement either way, and has conflicts now, so I vote to close unless and until we get around to properly solving the problem.
sjoerdverweij commented 2025-01-07 20:57:26 +00:00 (Migrated from github.com)
Copy link

(To be clear, I agree with the change on a code cleanliness and quality basis, but I'm going to accede to the wishes of the person paying for the API)

(To be clear, I agree with the change on a code cleanliness and quality basis, but I'm going to accede to the wishes of the person paying for the API)
RobThree commented 2025-01-07 21:36:38 +00:00 (Migrated from github.com)
Copy link

However, to do this properly:

Someone has to write a proxy

No, people should be using their own API keys. Yes, acquiring them is not easy for everyone and, in that regard, I can understand @brett-dot-tech's wish to "just make it work".

Most likely, the provider needs to be contacted to make sure they won't ban the account the minute they see a firehose of requests coming from a single IP -- almost all paid APIs have alerts on that and will axe you instantly, because if you're using a proxy, they have no way of knowing whether you're caching or not to save money.

That's the whole point; proxy or not: one bad actor and (s)he's gonna blow it for everyone. When everyone uses their own API key, worst case (or best case, depends on your point of view) (s)he blocks him- or herself. What you're referring to is rate-limiting.

This has been addressed before (can't seem to find the PR / Issue right now), but @brett-dot-tech made it clear he doesn't want people to have to go out and get their own API keys. Again, I can understand that. His project, his rules. It has been objected by a few people, that's all we can do. This PR is just 'centralizing' all API keys so when people DO want to put in their own, they're in a single place.

> However, to do this properly: > Someone has to write a proxy No, people should be using their own API keys. Yes, acquiring them is not easy for everyone and, in that regard, I can understand @brett-dot-tech's wish to "just make it work". > Most likely, the provider needs to be contacted to make sure they won't ban the account the minute they see a firehose of requests coming from a single IP -- almost all paid APIs have alerts on that and will axe you instantly, because if you're using a proxy, they have no way of knowing whether you're caching or not to save money. That's the whole point; proxy or not: one bad actor and (s)he's gonna blow it for everyone. When everyone uses their own API key, worst case (or best case, depends on your point of view) (s)he blocks him- or herself. What you're referring to is rate-limiting. This has been addressed before (can't seem to find the PR / Issue right now), but @brett-dot-tech made it clear he doesn't want people to have to go out and get their own API keys. Again, I can understand that. His project, his rules. It has been objected by a few people, that's all we can do. This PR is just 'centralizing' all API keys so when people DO want to put in their own, they're in a single place.
HugoBalder commented 2025-01-07 22:30:41 +00:00 (Migrated from github.com)
Copy link

I created now my free API key at twelvedata. With this key i cannot get data for the German market. Therefore i have to use Brett‘s Key or i pay at least 790$ per year. This would mean for me to write my own Stockwidget with another provider.
image

I created now my free API key at twelvedata. With this key i cannot get data for the German market. Therefore i have to use Brett‘s Key or i pay at least 790$ per year. This would mean for me to write my own Stockwidget with another provider. <img width="531" alt="image" src="https://github.com/user-attachments/assets/b019167e-208c-46f3-9994-68812226ad41" />
RobThree commented 2025-01-07 22:37:57 +00:00 (Migrated from github.com)
Copy link

Nothing in this PR or in my proposal is stopping you from using @brett-dot-tech's API key(s) 😉 This PR is about 'centralizing' the API keys. Another PR (or issue, can't remember, can't find it currently) was about not sharing API keys at all (which I still stand by, but, again, it's up to @brett-dot-tech and I respect his choice in the matter). (Potentially) Changing to not sharing API keys could, indeed, result in some edge-cases like yours having to pay or something else. Yes, that would suck. On the other hand: there's probably plenty providers that provide German data for free(-ish) and that would maybe provide an incentive for people to implement more providers. Win-win in my book 😉 (But $790 is... holy moly, yes, that would suck...)

Nothing in this PR or in my proposal is stopping you from using @brett-dot-tech's API key(s) 😉 This PR is about 'centralizing' the API keys. Another PR (or issue, can't remember, can't find it currently) was about not sharing API keys at all (which I still stand by, but, again, it's up to @brett-dot-tech and I respect his choice in the matter). (Potentially) Changing to not sharing API keys could, indeed, result in some edge-cases like yours having to pay or something else. Yes, that would suck. On the other hand: there's probably plenty providers that provide German data for free(-ish) and that would maybe provide an incentive for people to implement more providers. Win-win in my book 😉 (But $790 is... holy moly, yes, that would suck...)
HugoBalder commented 2025-01-07 22:46:39 +00:00 (Migrated from github.com)
Copy link

You are completely right ! I have not understood, why this twelvedata api key moved from config.h to the stockwidget.cpp. For me personally it would be not a big deal to use my own api key and use only the American market data. For Apple it makes not a huge difference, it would be only a delay.

You are completely right ! I have not understood, why this twelvedata api key moved from config.h to the stockwidget.cpp. For me personally it would be not a big deal to use my own api key and use only the American market data. For Apple it makes not a huge difference, it would be only a delay.
lyricnz commented 2025-01-10 05:04:49 +00:00 (Migrated from github.com)
Copy link

I know this may sound silly, but should a bad actor get these keys and rate limit out the API to a point that I cant fund it further, it will result in everyones orbs going down for both weather and stocks.

(off topic)
Without implementing a proxy for data requests, maybe you could implement a "get the api key" (even from a static URL) where the code fetches the API key? That way, if a bad actor starts to abuse it, you can just delete the API key, and publish a new one, and all the orbs will automagically refresh their API key? (requiring maybe a reboot). Yes, bad actor can do this same - but at least that would require them to write code pretending to be an Orbi.

> I know this may sound silly, but should a bad actor get these keys and rate limit out the API to a point that I cant fund it further, it will result in everyones orbs going down for both weather and stocks. (off topic) Without implementing a proxy for data requests, maybe you could implement a "get the api key" (even from a static URL) where the code fetches the API key? That way, if a bad actor starts to abuse it, you can just delete the API key, and publish a new one, and all the orbs will automagically refresh their API key? (requiring maybe a reboot). Yes, bad actor can do this same - but at least that would require them to write code pretending to be an Orbi.
lyricnz commented 2025-01-10 05:05:42 +00:00 (Migrated from github.com)
Copy link

Even before I merged a PR renaming the HTTP client :) this PR needs rerolling, or at least conflict fixing.

Even before I merged a PR renaming the HTTP client :) this PR needs rerolling, or at least conflict fixing.
RobThree commented 2025-01-10 08:58:19 +00:00 (Migrated from github.com)
Copy link

Yes, bad actor can do this same - but at least that would require them to write code pretending to be an Orbi.

It would be trivial. As soon as this project gets some real momentum you can rest assured someone is going to "have fun" spoiling everyone's fun. Security through obscurity never works.

Even before I merged a PR renaming the HTTP client :) this PR needs rerolling, or at least conflict fixing.

Not sure what you mean?

> Yes, bad actor can do this same - but at least that would require them to write code pretending to be an Orbi. It would be trivial. As soon as this project gets some real momentum you can rest assured someone is going to "have fun" spoiling everyone's fun. Security through obscurity never works. > Even before I merged a PR renaming the HTTP client :) this PR needs rerolling, or at least conflict fixing. Not sure what you mean?
lyricnz commented 2025-01-13 04:10:33 +00:00 (Migrated from github.com)
Copy link

Not sure what you mean?

I merged another PR #218 which totally broke this PR, but it needed a reroll anyway.

> Not sure what you mean? I merged another PR #218 which totally broke this PR, but it needed a reroll anyway.
lyricnz commented 2025-01-13 04:11:20 +00:00 (Migrated from github.com)
Copy link

FWIW the API keys are leaked right onto the serial console with default config:

I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=REMOVED&symbol=BTC/USD
I: ✅ Released semaphore
I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=REMOVED&symbol=USD/CAD
I: ✅ Released semaphore
I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=REMOVED&symbol=XEQT
I: ✅ Released semaphore
FWIW the API keys are leaked right onto the serial console with default config: ``` I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=REMOVED&symbol=BTC/USD I: ✅ Released semaphore I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=REMOVED&symbol=USD/CAD I: ✅ Released semaphore I: 🔵 Starting HTTP request for: https://api.twelvedata.com/quote?apikey=REMOVED&symbol=XEQT I: ✅ Released semaphore ```
This pull request has changes conflicting with the target branch.
  • README.md
  • firmware/config/config.h.template
  • firmware/src/widgets/stockwidget/StockWidget.cpp
  • firmware/src/widgets/stockwidget/StockWidget.h
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin RobThree/apikeys_to_config:RobThree/apikeys_to_config
git switch RobThree/apikeys_to_config

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch dev
git merge --no-ff RobThree/apikeys_to_config
git switch RobThree/apikeys_to_config
git rebase dev
git switch dev
git merge --ff-only RobThree/apikeys_to_config
git switch RobThree/apikeys_to_config
git rebase dev
git switch dev
git merge --no-ff RobThree/apikeys_to_config
git switch dev
git merge --squash RobThree/apikeys_to_config
git switch dev
git merge --ff-only RobThree/apikeys_to_config
git switch dev
git merge RobThree/apikeys_to_config
git push origin dev
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug
Something isn't working

  
conflicts

   
documentation
Improvements or additions to documentation

  
DO NOT MERGE
Work in progress not ready to be considered for merging

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
good first issue
Good for newcomers

  
hardware
Problems or ideas about the hardware

  
help wanted
Extra attention is needed

  
invalid
This doesn't seem right

  
question
Further information is requested

  
wontfix
This will not be worked on

No labels
bug
conflicts
documentation
DO NOT MERGE
duplicate
enhancement
good first issue
hardware
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
greg/info-orbs!176
No description provided.