pdreker / fritz_exporter

Prometheus exporter for Fritz!Box home routers
Other
144 stars 32 forks source link

feat: allow reading password from a file #296

Closed NyCodeGHG closed 4 months ago

NyCodeGHG commented 4 months ago

Hi, I would prefer to read the password from a file using systemd credentials, so this PR adds support for this. I'm not that fluent in python, so please point out anything that could be improved.

pdreker commented 4 months ago

Hi and first of all thanks for your contribution.

For "not that fluent in Python" the code looks outstanding! :-)

I do have an honest question though: the exporter is designed (OK, designed is too much... "intended" may be better) to run in a Docker container, which is basically why the config works like it currently does. If I understand it correctly, this basically just makes it possible to move the actual password outside the config file to another file and reads it from there. I may not understand the point of that as one could also simply protect the config file with appropriate permissions. Please keep in mind, that I may be missing something here.

The code had a systemd unit file at some point, but I removed it, as it was more or less not a recommended configuration. Could you provide a working systemd config for running the exporter and the appropriate documentation for that "mode of operation" under docs/running.rst?

Another idea: As far as I have grasped the systemd-credentials docs (the page you linked) the mechanism is not limited to simple passwords, but can rather supply the whole config. Wouldn't it be easier then, to simply provide the whole config file from systemd and simply use the --config option to point to the file provided by systemd? This would then work without changes to the code... If I understood everything correctly...

Again: I am not knowledgeable in systemd, so I am willing to entertain arguments about this ;-)

Thanks again!

pdreker commented 4 months ago

Note to self: Tests fail due to SonarCloud Secret not being available in external Pull Requests - ignore.

NyCodeGHG commented 4 months ago

I'm maintaining the fritz-exporter package in NixOS. NixOS has a concept of so called options which allow you to configure your whole system using the Nix DSL. We support a lot of prometheus exporters and I wanted to add support for yours too.

The problem with this is, because of the way nix works, the configuration would end up in the world-readable nix store and with that, would expose the passwords. A common pattern is to read secrets from files outside of the nix store instead.

Of course it would be possible to just read the whole file from outside of the nix store, but then there's no point in using the NixOS options. I thought about hacking this in via config generation at runtime, but adding an option here seems easier.

Could you provide a working systemd config for running the exporter and the appropriate documentation for that "mode of operation" under docs/running.rst?

Sure!

pdreker commented 4 months ago

The problem with this is, because of the way nix works, the configuration would end up in the world-readable nix store and with that, would expose the passwords. A common pattern is to read secrets from files outside of the nix store instead.

Of course it would be possible to just read the whole file from outside of the nix store, but then there's no point in using the NixOS options. I thought about hacking this in via config generation at runtime, but adding an option here seems easier.

Ah! That makes quite a lot of sense :-)

How would you handle the fact, that the exporter supports multiple device in a singular config? Actually this was the original reason I deprecated "Environment Configuration", as this turned out be messy and hackish...

If the answer to that is "We'll just support one device per Config, you can run multiple instances of the exporter." that is fine with me but if you have an idea it would be greatly appreciated.

Could you provide a working systemd config for running the exporter and the appropriate documentation for that "mode of operation" under docs/running.rst?

Sure! Great work.

If we can clear up the "multiple devices" question somehow, I'll merge this and publish a new version right away :-)

Thanks again for your great work :-)

NyCodeGHG commented 4 months ago

How would you handle the fact, that the exporter supports multiple device in a singular config?

I'm not sure I understand your question correctly, but this should still work with this PR, right?

Or do you mean how this would work out in the NixOS options? I just generate a yaml file, so this also works with multiple devices.

pdreker commented 4 months ago

The current config allows for the fact, that one may have e.g. 3 devices, and each device has a different password, which is then configured in the config file accordingly. As far as I can see, the new code assumes, that all devices have the same password, which will probably be valid in the majority of situations, but isn't neccessarily guaranteed.

In the end this would then create a situation where the "normal" config allows for different password, the password file config doesn't allow for multiple passwords and the env config option only allows for a single device... While this can be chalked up to "documented limitations" the whole situation is needlessly complicated in my opinion and feels ugly.

I'm just looking for ideas, not a general fix, but in an ideal world all the different options should be somewhat equivalent in that they work more or less the same way, with the same features.

If you have a quick idea, which makes it possible to have different password for different devices, I'll take it, but I am willing to accept "no" for an answer. Maybe the "different password for different devices" feature needs to go in some future version.

NyCodeGHG commented 4 months ago

As far as I can see, the new code assumes, that all devices have the same password

Where do I assume that? I didn't intent to do that.

It should be possible to specify different password files for each device, when using the config file, right?

pdreker commented 4 months ago

Oh, for &%&&%§" sake :-D

I checked out the code instead of reading the diff and yes, you are absolutely right. Sorry for the confusion. Shouldn't be doing this in between other tasks ;-)

pdreker commented 4 months ago

PR merged. Thank you very much for the contribution and sorry again for the confusion.

I'll roll a new version shortly containing the new feature.

NyCodeGHG commented 3 months ago

@pdreker hi again, when do you plan to release a new version?

pdreker commented 3 months ago

Errr... There was an issue in the current "main" code, which I wanted fix before releasing, then I found another problem, then I got sidetracked, then I got Fiber and then "life happened" :-D

I have the code open right now and intend to get it "out there" really quick now. No complex fixes, just make it work and release.

Stay tuned...

pdreker commented 3 months ago

And as a personal courtesy, here is your release notification ;-)

2.4.0 just hit the usual spots.

NyCodeGHG commented 3 months ago

thank you very much :)