Skip to content

Add SFTP deploy hook #6353

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

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Conversation

KayJay7
Copy link

@KayJay7 KayJay7 commented May 14, 2025

This pull request adds an sftp based deploy hook, it is meant as an alternative to the ssh deploy when there is no need to run command on the server, so that the privileges of the deployment user can further reduced.
Specifically, using sftp instead of ssh allows one to set the ForceCommand internal-sftp sshd option and the /sbin/nologin shell on the deployment user, reducing the attack surface.

The hook is loosely inspired by the ssh hook, although it lacks the more advanced options. It supports deploying on many servers, possibly with different ports and users (unlike the ssh hook which requires all servers to have the same user). Like the ssh hook it requires the local deployment user to be able to login without entering a password. It allows specifying custom paths for the certificate files. The environment variables usage is described in a comment at the beginning of the file.

To send the commands to the sftp server I used an HEREDOC, I don't really like it, but I'm not aware better way to do it.

_debug _cfullchain "$_cfullchain"

# HOSTS is required to login by sftp to remote host.
_migratedeployconf Le_Deploy_sftp_hosts DEPLOY_SFTP_HOSTS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you need to migrate Le_Deploy_sftp_hosts ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script needs to save configuration for future runs so they don't have to be passed as environment variables to be useful (i.e. when using --renew-all it needs to use a different the destination location for every certificate, it the location where passed as environment variable, one certificate might overwrite another or simply be stored in the wrong place).

To do that I copied what the ssh.sh hook did assuming it to be the proper way. It also used that _migradedeployconf function.

@Neilpang Neilpang requested a review from Copilot May 17, 2025 19:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a new SFTP-based deploy hook script as an alternative to the SSH deploy hook, reducing required user privileges by using ForceCommand internal-sftp and /sbin/nologin.

  • Introduces deploy/sftp.sh implementing sftp_deploy() with support for multiple hosts
  • Allows optional customization of key, cert, CA, and fullchain file paths via environment variables
  • Uses a heredoc to batch/send SFTP commands per host
Comments suppressed due to low confidence (1)

deploy/sftp.sh:23

  • Consider adding unit or integration tests for sftp_deploy() to cover multiple hosts, missing optional variables, and error scenarios.
sftp_deploy() {

Comment on lines +119 to +120
sftp "$_sftp_host"\
<<EOF
Copy link
Preview

Copilot AI May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script expects host:port syntax (e.g., 192.168.0.1:22), but sftp doesn’t accept that form. Parse $_sftp_host into host and port, then invoke sftp with -P <port> or -oPort=<port>.

Suggested change
sftp "$_sftp_host"\
<<EOF
# Extract host and port from _sftp_host
_host=$(echo "$_sftp_host" | cut -d':' -f1)
_port=$(echo "$_sftp_host" | cut -d':' -f2)
# Check if a port is specified
if [ -n "$_port" ]; then
sftp -P "$_port" "$_host" <<EOF
else
sftp "$_host" <<EOF
fi

Copilot uses AI. Check for mistakes.

Comment on lines +15 to +18
# export DEPLOY_SFTP_KEYFILE="/etc/stunnel/stunnel.pem" # defaults to ~/acme_sftp_deploy/<domain>/<domain>.key
# export DEPLOY_SFTP_CERTFILE="/etc/stunnel/stunnel.pem" ~/acme_sftp_deploy/<domain>/<domain>.cer
# export DEPLOY_SFTP_CAFILE="/etc/stunnel/uca.pem" ~/acme_sftp_deploy/<domain>/ca.cer
# export DEPLOY_SFTP_FULLCHAIN="" ~/acme_sftp_deploy/<domain>/fullchain.cer
Copy link
Preview

Copilot AI May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default-path hint is merged into the export line. Separate it into its own comment (prefix with #) so it's clear what’s the default vs. what’s the example export.

Suggested change
# export DEPLOY_SFTP_KEYFILE="/etc/stunnel/stunnel.pem" # defaults to ~/acme_sftp_deploy/<domain>/<domain>.key
# export DEPLOY_SFTP_CERTFILE="/etc/stunnel/stunnel.pem" ~/acme_sftp_deploy/<domain>/<domain>.cer
# export DEPLOY_SFTP_CAFILE="/etc/stunnel/uca.pem" ~/acme_sftp_deploy/<domain>/ca.cer
# export DEPLOY_SFTP_FULLCHAIN="" ~/acme_sftp_deploy/<domain>/fullchain.cer
# export DEPLOY_SFTP_KEYFILE="/etc/stunnel/stunnel.pem"
# Default: ~/acme_sftp_deploy/<domain>/<domain>.key
# export DEPLOY_SFTP_CERTFILE="/etc/stunnel/stunnel.pem"
# Default: ~/acme_sftp_deploy/<domain>/<domain>.cer
# export DEPLOY_SFTP_CAFILE="/etc/stunnel/uca.pem"
# Default: ~/acme_sftp_deploy/<domain>/ca.cer
# export DEPLOY_SFTP_FULLCHAIN=""
# Default: ~/acme_sftp_deploy/<domain>/fullchain.cer

Copilot uses AI. Check for mistakes.

Comment on lines +121 to +124
put $_ckey $_ckey_path
put $_ccert $_ccert_path
put $_cca $_cca_path
put $_cfullchain $_cfullchain_path
Copy link
Preview

Copilot AI May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap file paths in quotes in the heredoc to handle spaces, e.g., put "$_ckey" "$_ckey_path".

Suggested change
put $_ckey $_ckey_path
put $_ccert $_ccert_path
put $_cca $_cca_path
put $_cfullchain $_cfullchain_path
put "$_ckey" "$_ckey_path"
put "$_ccert" "$_ccert_path"
put "$_cca" "$_cca_path"
put "$_cfullchain" "$_cfullchain_path"

Copilot uses AI. Check for mistakes.

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.

2 participants