Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

security/acme-client: Fix wrong permissions in 'Update to local Unifi keystore' automation #4417

Open
3 tasks done
mdbraber opened this issue Dec 19, 2024 · 3 comments
Open
3 tasks done

Comments

@mdbraber
Copy link
Contributor

mdbraber commented Dec 19, 2024

Important notices
Before you add a new report, we ask you kindly to acknowledge the following:

Describe the bug
When deploy an certificate obtained via acme-client, the permissions on the key store are wrong. Also see here: https://forum.opnsense.org/index.php?topic=43556.msg216736#msg216736

To Reproduce
(using the Unifi plugin from the mimugmail repo)

  1. Obtain a certificate
  2. Use the "Deploy to Unifi" command after obtaining the certificate
  3. Try to access Unifi

Expected behavior
Expected to have a clean reload

Suggested fix

What could help is to change this line here:

$this->acme_env['DEPLOY_UNIFI_RELOAD'] = 'service unifi restart';
to

$this->acme_env['DEPLOY_UNIFI_RELOAD'] = 'service unifi restart';

change into

$this->acme_env['DEPLOY_UNIFI_RELOAD'] = 'chown unifi:wheel ' + (string)$this->config->acme_unifi_keystore + '; service unifi restart'

I've considered having a separate chown command, but order would not be expected to be the same and that solution seems brittle. Post-hooks in acme.sh are run before deploy hooks in acme.sh so that won't work either. Also changing the deploy hook in acme.sh is very specific to OPNsense and will possibly not be accepted in the distribution (which would be fair). So I think the above would be the best place to fix this.

@adn77
Copy link
Contributor

adn77 commented Dec 19, 2024

Thank you for getting to the bottom of it. I noticed some weirdness with the certificate deployment as well in recent versions but didn't think much of it.

Either acme.sh or Unifi changed the user they are running under.

I proposed an addition to the upstream project. If that finds support, I am going to fix it there.
Maybe we'll get around changing anything here then.

@adn77
Copy link
Contributor

adn77 commented Dec 20, 2024

I created an acme.sh PR here
acmesh-official/acme.sh#6168

You can simply apply the changes in /usr/local/share/examples/acme.sh/deploy/unifi.sh until it gets merged.

@adn77
Copy link
Contributor

adn77 commented Dec 22, 2024

The PR has been merged to the acme.sh Dev branch. Should be in the next release of acme.sh 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants