Skip to content

Conversation

@invario
Copy link
Contributor

@invario invario commented Jul 7, 2025

Deploy-hook to very simply copy files to set directories and then execute whatever reloadcmd the admin needs afterwards. This can be useful for configurations where the "multideploy" hook (in development) is used or when an admin wants ACME.SH to renew certs but needs to manually configure deployment via an external script (e.g. The deploy-freenas script for TrueNAS Core/Scale

https://github.com/danb35/deploy-freenas/

Note: replaces my earlier PR #6379 which I closed.
acme.sh wiki: https://github.com/acmesh-official/acme.sh/wiki/deployhooks#40-deploy-to-local-directory-via-copy

Deploy-hook to very simply copy files to set directories and then execute whatever reloadcmd
the admin needs afterwards. This can be useful for configurations where the "multideploy"
hook (in development) is used or when an admin wants ACME.SH to renew certs but needs to
manually configure deployment via an external script (e.g. The deploy-freenas script for TrueNAS Core/Scale

https://github.com/danb35/deploy-freenas/

Signed-off-by: invario <[email protected]>
@rbicker
Copy link

rbicker commented Jul 14, 2025

Hi,

We have a use case where we need to deploy a combined PEM file (containing both the full certificate chain and the private key) to multiple web services. While acme.sh itself supports specifying the same path for both --key-file and --fullchain-file (or --cert-file), the current localcopy deploy hook does not account for this scenario—it performs separate copy operations, resulting in one file overwriting the other.

Would it be possible to enhance the hook by adding logic similar to the snippet below (around line 41), to detect when the cert and key targets are the same and generate a proper combined PEM?

  _combined_target=""
  _combined_srccert=""

  if [ "$DEPLOY_LOCALCOPY_CERTKEY" ] && \
     { [ "$DEPLOY_LOCALCOPY_CERTKEY" = "$DEPLOY_LOCALCOPY_FULLCHAIN" ] || \
       [ "$DEPLOY_LOCALCOPY_CERTKEY" = "$DEPLOY_LOCALCOPY_CERTIFICATE" ]; }; then

    _combined_target="$DEPLOY_LOCALCOPY_CERTKEY"

    if [ "$DEPLOY_LOCALCOPY_CERTKEY" = "$DEPLOY_LOCALCOPY_FULLCHAIN" ]; then
      _combined_srccert="$_cfullchain"
      DEPLOY_LOCALCOPY_FULLCHAIN=""
    else
      _combined_srccert="$_ccert"
      DEPLOY_LOCALCOPY_CERTIFICATE=""
    fi

    _info "Creating combined PEM at $_combined_target"
    _tmpfile="$(mktemp)"
    if ! cat "$_combined_srccert" "$_ckey" > "$_tmpfile"; then
      _err "Failed to build combined PEM file"
      return 1
    fi
    if ! mv "$_tmpfile" "$_combined_target"; then
      _err "Failed to move combined PEM into place"
      return 1
    fi

    DEPLOY_LOCALCOPY_CERTKEY=""
    _savedeployconf DEPLOY_LOCALCOPY_CERTKEY "$_combined_target"
  fi

This change would make the deploy hook more robust in real-world deployment scenarios (e.g. HAProxy), without affecting current behavior for users who specify separate files.

Thanks!

@invario
Copy link
Contributor Author

invario commented Jul 14, 2025

    if ! cat "$_combined_srccert" "$_ckey" > "$_tmpfile"; then
      _err "Failed to build combined PEM file"
      return 1
    fi
    if ! mv "$_tmpfile" "$_combined_target"; then
      _err "Failed to move combined PEM into place"
      return 1
    fi

Thanks for the suggestion! Is there any reason a temp file has to be used as opposed to just concatenating directly to the target?

@rbicker
Copy link

rbicker commented Jul 28, 2025

Hi
Please excuse my late response, unfortunately I have missed your reply.
I opted for using a temporary file mainly as a safety measure, to avoid partially written or corrupted files in case something goes wrong during the concatenation (e.g. disk full, permissions, interruption, etc.).

@invario
Copy link
Contributor Author

invario commented Jul 28, 2025

Hi Please excuse my late response, unfortunately I have missed your reply. I opted for using a temporary file mainly as a safety measure, to avoid partially written or corrupted files in case something goes wrong during the concatenation (e.g. disk full, permissions, interruption, etc.).

I used your code and made some changes and incorporated it. Let me know if it works for you, thanks!

@rbicker
Copy link

rbicker commented Oct 14, 2025

Thanks! I’ve had a chance to test the hook a few times over the last couple of weeks, and it works really well.

One thing I noticed that could be optimized: currently, the hook does not preserve permissions for existing files. It would be great if the temporary file used for creating the combined PEM could inherit the target file’s attributes. For example:

if ! cp --attributes-only "$_combined_target" "$_tmpfile"; then
    _err "Failed to copy file attributes"
    return 1
fi

Perhaps there’s a better approach, but this would ensure ownership and permissions remain consistent.

Also, could you clarify how pull requests like this get released? At the moment, I’m manually copying the hooks I need (this one + multideploy) from GitHub, which obviously isn’t ideal for updates.

Thanks again for your work on this!

@invario
Copy link
Contributor Author

invario commented Oct 18, 2025

Thanks! I’ve had a chance to test the hook a few times over the last couple of weeks, and it works really well.

One thing I noticed that could be optimized: currently, the hook does not preserve permissions for existing files. It would be great if the temporary file used for creating the combined PEM could inherit the target file’s attributes. For example:

if ! cp --attributes-only "$_combined_target" "$_tmpfile"; then
    _err "Failed to copy file attributes"
    return 1
fi

Perhaps there’s a better approach, but this would ensure ownership and permissions remain consistent.

Also, could you clarify how pull requests like this get released? At the moment, I’m manually copying the hooks I need (this one + multideploy) from GitHub, which obviously isn’t ideal for updates.

Thanks again for your work on this!

You're very welcome.

The permissions issue I'll work on, but your code would only work if there were an existing target file already. Might I suggest putting a chown/chmod in the reloadcmd as necessary?

I only wrote this snippet of code and created the PR and am awaiting approval. I'm not the acm.sh repo maintainer; that would be @neilpang, so the only way is for the maintainer to review and approve the PR.

@rbicker
Copy link

rbicker commented Oct 21, 2025

Thanks for the quick reply!
Good point about the existing file. It should be easy to handle by checking if the target file already exists before copying attributes, for example:

if [ -e "$_combined_target" ]; then
  cp --attributes-only "$_combined_target" "$_tmpfile" || _err "Failed to copy file attributes"
fi

That way, ownership and permissions are preserved when a file already exists (e.g., during renewal), without needing to rely on reloadcmd. Also, other deploy hooks like nginx or apache already handle permissions internally, so keeping that behavior consistent here would avoid confusion for users who expect similar handling.

And got it about the PR process — I’ll keep an eye out for @neilpang's review. Thanks again for your work on this hook; it’s been really helpful in my setup!

@invario
Copy link
Contributor Author

invario commented Nov 3, 2025

Thanks for the quick reply! Good point about the existing file. It should be easy to handle by checking if the target file already exists before copying attributes, for example:

if [ -e "$_combined_target" ]; then
  cp --attributes-only "$_combined_target" "$_tmpfile" || _err "Failed to copy file attributes"
fi

That way, ownership and permissions are preserved when a file already exists (e.g., during renewal), without needing to rely on reloadcmd. Also, other deploy hooks like nginx or apache already handle permissions internally, so keeping that behavior consistent here would avoid confusion for users who expect similar handling.

And got it about the PR process — I’ll keep an eye out for @neilpang's review. Thanks again for your work on this hook; it’s been really helpful in my setup!

Sorry for the delay in getting back to you. So as it turns out, cp should preserve existing permissions when overwriting the file, but I'm using mv and that's why the permissions are being lost. Any reason you see why I can't just use cp to overwrite the existing permissions, and then subsequently rm the temp file?

@meineerde
Copy link

meineerde commented Nov 30, 2025

Generally, I'd prefer you just use a plain cat "..." > "$_combined_target" (or the equivalent echo) and just report any errors happening there. That way, existing permissions and ownerships of the target file would be fully preserved in a way which may not be possible otherwise. This also doesn't require to write into the target file's directory but just to the file itself, allowing minimal permissions to be set on the target.

If this is not desired, many operating systems (at least Linux and MacOS) also offer the install tool to copy or link files while preserving or explicitly setting permissions, owner, ... This may be more robust than a cp or mv.

In any case, you should make sure that any file containing private key material is set to file mode 600 to avoid the key material to be world-readable. acme.sh itself uses the following approach in the _installcert function which I think should work fine (apart from the TOCTOU race-conditions which are hard to avoid in plain shell without using the install command):

if [ -f "$target" ]; then
  cat "$source >"$target" || return 1
else
  touch "$target" || return 1
  chmod 600 "$target"
  cat "$source" >"$target" || return 1
fi

@rbicker
Copy link

rbicker commented Nov 30, 2025

Yes, using cat is a safe and simple approach. Setting 0600 as the default permissions for new keys is also a solid choice.

@invario
Copy link
Contributor Author

invario commented Nov 30, 2025

Generally, I'd prefer you just use a plain cat "..." > "$_combined_target" (or the equivalent echo) and just report any errors happening there. That way, existing permissions and ownerships of the target file would be fully preserved in a way which may not be possible otherwise. This also doesn't require to write into the target file's directory but just to the file itself, allowing minimal permissions to be set on the target.

If this is not desired, many operating systems (at least Linux and MacOS) also offer the install tool to copy or link files while preserving or explicitly setting permissions, owner, ... This may be more robust than a cp or mv.

In any case, you should make sure that any file containing private key material is set to file mode 600 to avoid the key material to be world-readable. acme.sh itself uses the following approach in the _installcert function which I think should work fine (apart from the TOCTOU race-conditions which are hard to avoid in plain shell without using the install command):

if [ -f "$target" ]; then
  cat "$source >"$target" || return 1
else
  touch "$target" || return 1
  chmod 600 "$target"
  cat "$source" >"$target" || return 1
fi

Thanks, makes sense, I'll make the changes and push an update!

@invario
Copy link
Contributor Author

invario commented Dec 3, 2025

Pushed update. Also removed use of temp file.

@meineerde
Copy link

Thanks for the update.

However, for other options, you still use cp with possible too open permissions on the target file, e.g. when setting DEPLOY_LOCALCOPY_CERTKEY to copy the privat key. Thus, it's likely a good idea to use the cat approach everywhere.

In the end, what this deploy script provides is quite similar to what the existing _install_cert function does (minus the backup copies which are probably not needed here).

@invario
Copy link
Contributor Author

invario commented Dec 3, 2025

Thanks for the update.

However, for other options, you still use cp with possible too open permissions on the target file, e.g. when setting DEPLOY_LOCALCOPY_CERTKEY to copy the privat key. Thus, it's likely a good idea to use the cat approach everywhere.

In the end, what this deploy script provides is quite similar to what the existing _install_cert function does (minus the backup copies which are probably not needed here).

Thank you again, you're right, I didn't even check those sections and only changed the PEM section. I was too focused on that. Will update shortly!

@invario
Copy link
Contributor Author

invario commented Dec 4, 2025

Pushed new one, should be 👍

@meineerde
Copy link

Pushed update. Also removed use of temp file.

For files which do not contain key material, the permissions are likely too restrictive now as you now create all files with mode 600.

As mentioned a few times, the _installcert function in acme.sh does this differently. See

acme.sh/acme.sh

Lines 6039 to 6088 in 1bd2922

if [ "$_real_cert" ]; then
_info "Installing cert to: $_real_cert"
if [ -f "$_real_cert" ] && [ ! "$_ACME_IS_RENEW" ]; then
cp "$_real_cert" "$_backup_path/cert.bak"
fi
if [ "$CERT_PATH" != "$_real_cert" ]; then
cat "$CERT_PATH" >"$_real_cert" || return 1
fi
fi
if [ "$_real_ca" ]; then
_info "Installing CA to: $_real_ca"
if [ "$_real_ca" = "$_real_cert" ]; then
echo "" >>"$_real_ca"
cat "$CA_CERT_PATH" >>"$_real_ca" || return 1
else
if [ -f "$_real_ca" ] && [ ! "$_ACME_IS_RENEW" ]; then
cp "$_real_ca" "$_backup_path/ca.bak"
fi
if [ "$CA_CERT_PATH" != "$_real_ca" ]; then
cat "$CA_CERT_PATH" >"$_real_ca" || return 1
fi
fi
fi
if [ "$_real_key" ]; then
_info "Installing key to: $_real_key"
if [ -f "$_real_key" ] && [ ! "$_ACME_IS_RENEW" ]; then
cp "$_real_key" "$_backup_path/key.bak"
fi
if [ "$CERT_KEY_PATH" != "$_real_key" ]; then
if [ -f "$_real_key" ]; then
cat "$CERT_KEY_PATH" >"$_real_key" || return 1
else
touch "$_real_key" || return 1
chmod 600 "$_real_key"
cat "$CERT_KEY_PATH" >"$_real_key" || return 1
fi
fi
fi
if [ "$_real_fullchain" ]; then
_info "Installing full chain to: $_real_fullchain"
if [ -f "$_real_fullchain" ] && [ ! "$_ACME_IS_RENEW" ]; then
cp "$_real_fullchain" "$_backup_path/fullchain.bak"
fi
if [ "$_real_fullchain" != "$CERT_FULLCHAIN_PATH" ]; then
cat "$CERT_FULLCHAIN_PATH" >"$_real_fullchain" || return 1
fi
fi

For the following files in the localcopy deploy hook, acme.sh would just uses cp instead of touch / chmod / cat:

  • $DEPLOY_LOCALCOPY_CERTIFICATE
  • $DEPLOY_LOCALCOPY_FULLCHAIN
  • $DEPLOY_LOCALCOPY_CA

In addition to that, your touch / chmod approach for files containing key material ($_combined_target, $DEPLOY_LOCALCOPY_CERTKEY, $DEPLOY_LOCALCOPY_PFX) could possibly succeed unexpectedly:

if ! (
  touch "$DEPLOY_LOCALCOPY_CERTKEY"
  chmod 600 "$DEPLOY_LOCALCOPY_CERTKEY"
); then
  #...
fi

With your approach, I believe, the result of the touch command is ignored, even if it fails. This may not be extremely bad here, because if touch fails, the chmod command is also likely to fail.

However, it may still be a good idea to convert this to the acme.sh logic to ensure that the touch succeeds and to ignore any chmod errors (in case we are not allowed to set the file mode). If we then can not write to the file with cat later, an error is still thrown.

That would make sure that the behavior of acme.sh itself is the same as this hook and to avoid some edge-cases.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants