k8s-at-home / charts

⚠️ Deprecated : Helm charts for applications you run at home
https://docs.k8s-at-home.com
Apache License 2.0
1.45k stars 623 forks source link

[autobrr] Add autobrr chart #1562

Closed funkypenguin closed 2 years ago

funkypenguin commented 2 years ago

Description of the change

Hey guys! Love your work!

Here's a PR to add a chart for "autobrr" (https://github.com/autobrr/autobrr) - I hope I've done it right!

Benefits

Possible drawbacks

Applicable issues

Additional information

Checklist

onedr0p commented 2 years ago

@all-contributors please add @funkypenguin for code

allcontributors[bot] commented 2 years ago

@onedr0p

I've put up a pull request to add @funkypenguin! :tada:

brettpetch commented 2 years ago

Hey @onedr0p — it might be worth looking at this PR again. The app in question has a secret key that should be rotated across pods, I’m not seeing it being changed in the charts.

onedr0p commented 2 years ago

Huh? Sorry I'm not at all familiar with this application.

brettpetch commented 2 years ago

Huh? Sorry I'm not at all familiar with this application.

There’s a session key that needs to be generated for the config to be considered complete. I’ve provided a review with a potential solution, although I’ve never written a helm chart before.

According to the documentation from the autobrr dev team,

sessionSecret: Used for session cookies. Change to something more random like a UUID.

https://autobrr.com/docs/getting-started/installation

Here’s a working installer that’s made for on the metal installs, but the same technique should be able to be transferred.

https://github.com/swizzin/swizzin/blob/master/scripts/install/autobrr.sh#L52

brettpetch commented 2 years ago

The application also has optional postgres support, but I’m unsure if this needs to be a separate chart or if it can be a switch people can flick— I’m not sure what the project structure is here.

onedr0p commented 2 years ago

@funkypenguin what do you think of these changes?

funkypenguin commented 2 years ago

The app description was a copy/paste oversight, I'll submit a PR to fix that :)

WRT the session secret, my feeling is that in this context, we're better served by keeping the helm chart simple, and avoiding auto-created config values. The chart user is free to alter the config their values.yaml, and set the session secret themselves, but doing it programmatically would be fiddly, and not worth it IMO.

If the config options could also be passed to the app via ENV vars (which override the config), then there's could be a case made for further automating the configuration.

brettpetch commented 2 years ago

Allowing users who have successfully authenticated on another instance with the same key could just login to this one using the same cookie... How is that a good solution to authentication? The session key should be generated for every pod as it pretty much allows unfettered access to the person's accounts, api keys, filters, download clients, and is a huge RCE vulnerability in itself if left unchecked.

For your implementation it may make sense, but at a broader scale of someone just wanting to spin up an instance of the application in a pod, it serves as a massive security hole if the application is not behind some kind of authentication.