hassio-addons / addon-grafana

Grafana - Home Assistant Community Add-ons
https://addons.community
MIT License
213 stars 61 forks source link

Change default cert/key path and alllow the use of a different fullpath for cert/key #402

Closed arsenicks closed 1 month ago

arsenicks commented 1 month ago

Proposed Changes

This would change the default config for fullchain.pem and privkey.pem to add the fullpath /ssl/*.pem as default location and remove the hardcoded path of /ssl/ in the nginx config to allow the use of fullpath Ex. /share/my_ssl_cert_nfs_mount/fullchain.pem.

This should be verified with existing configuration before merging as I was not able to confirm this will not cause regression for people already having a working configuration using the old defaults.

Related Issues

fixes #401

arsenicks commented 1 month ago

This will absolutely break existing config, for a very niche use case.

Hi, thanks for your input. I'm more than open to other suggestions. This was the easiest way I saw to do it.

I'm not sure how it would break existing config tho ? from what I saw in the code( first time looking at it, I'm not saying I know more than you..) I was under the impression the addon was moving the nginx .disabled config and using sed to replace the %%certfile%% with /ssl/ preceding it. So if people are using the default path/filename this would still work because now the default would be /ssl/fullchain.pem for example, no ?

Again, I'm not saying you are wrong, I'm trying to find a way to get this to work and not break anything 👍

sinclairpaul commented 1 month ago

The config isn't refreshed from the defaults if already saved.

arsenicks commented 1 month ago

Is this would be better ?

I simply add a new setting(sslpath) that default to /ssl/ so we don't touch the certfile and keyfile setting that people could have already have changed

sinclairpaul commented 1 month ago

Unless people refresh the config, I don't believe it will update.

My honest opinion is run a reverse proxy somewhere, where you can manage the certs. I don't see this being added.

arsenicks commented 1 month ago

Yeah that's what I was doing but didn't add any value so I just wanted to simplify and tough I could use the native addon config and felt it was a weird default to force the /ssl/ path anyway. The workaround of adding "../share/mymountpoint/fullchain.pem" work anyway, just weird the leading ../ disapear once saved.. Maybe this could be an easy fix..

Anyway I'll close the PR then.

sinclairpaul commented 1 month ago

just weird the leading ../ disapear once saved

This should likely be logged against the HA repo.

Edit - In all honesty it likely shouldn't accept it in the first place as its traversal.