Move API keys to config

[RobThree]

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

[brett-dot-tech]

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]

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.

[lyricnz]

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

[sjoerdverweij]

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]

(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]

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]

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.

[RobThree]

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]

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]

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]

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

[RobThree]

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]

Not sure what you mean?

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

[lyricnz]

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