Skip to content

AcmeClient support cert upload via sftp#1455

Merged
fraenki merged 9 commits intoopnsense:masterfrom
jkellerer:ft-acmeclient-sftp-upload
Sep 10, 2019
Merged

AcmeClient support cert upload via sftp#1455
fraenki merged 9 commits intoopnsense:masterfrom
jkellerer:ft-acmeclient-sftp-upload

Conversation

@jkellerer
Copy link
Copy Markdown
Contributor

This PR implements a new automation action for the Let's Encrypt (AcmeClient) client that allows to deploy certs to remote hosts using SFTP after they have been created or renewed.

The purpose of the extension is to securely transfer certificates generated on the firewall to protected hosts that may not be public reachable on ports required to generate certs directly.

Notes:

  • The feature is very similar to the highwinds upload but it works on any SFTP enabled host.
  • Only public key authorization is allowed as it doesn't require exchanging any secrets.
  • Host keys and identities are actively managed in a discrete folder, not mixed with standard locations.
  • The upload script can be called directly (via console/ssh) to test and trigger it outside of the normal workflow with automations. E.g. when configurations for a certificate have been made in the firewall ui, the simplest possible call is: ./upload_sftp.php --log --certificates=my.domain.org which will run all SFTP upload automations assigned to this cert and print detailed log to the console.
    More examples can be found via "--help".

Screenshot of UI enhancements:
image

This change avoids that all contents in any subfolder is set to mode 0750 as this is illegal e.g. for private keys for SSH.
@fraenki fraenki self-assigned this Aug 14, 2019
@fraenki fraenki added the feature Adding new functionality label Aug 14, 2019
@jkellerer jkellerer force-pushed the ft-acmeclient-sftp-upload branch from 29d762c to 16e0baf Compare August 14, 2019 11:15
@fraenki
Copy link
Copy Markdown
Member

fraenki commented Aug 14, 2019

@jkellerer First of all: thanks, this looks very good at first glance. I'll do a proper review later.

I've noticed that the "remote path" is configurable. Could you also make the remote filenames configurable, so that one could freely choose the filename on the target host for the certificate, private key and ca?

@fraenki fraenki self-requested a review August 14, 2019 11:23
@jkellerer
Copy link
Copy Markdown
Contributor Author

jkellerer commented Aug 14, 2019

@fraenki, thanks for the prompt and nice feedback.

Yes it may really make sense to configure file names (or better paths) as well since the certs can then be placed right at the desired location with the correct naming. I'm thinking to use template values like this "%s/cert.pem" or "{{name}}/cert.pem" and also look into the "extended options" filter button to eventually hide some of the fields in the default view to not have an overwhelmingly large set of options to deal with.

@fraenki
Copy link
Copy Markdown
Member

fraenki commented Aug 14, 2019

I'm thinking to use template values like this "%s/cert.pem" or "{{name}}/cert.pem"

Just to be sure we're talking about the same thing: I'd like to be able to use "my_cert_name.crt" as target filename instead of "cert.pem". Same for key and ca filenames. :)

@jkellerer
Copy link
Copy Markdown
Contributor Author

Yes we do, I think :) .. currently the final path is created as "{{remote-path}}/{{cert-name}}/cert.pem". To freely configure it, I'll need 3 new fields (for cert.pem, key.pem & ca.pem) with default values {{cert-name}}/cert.pem, {{cert-name}}/key.pem and {{cert-name}}/ca.pem. This replicates current behaviour and allows to freely configure the path or name used below "remote-path". (I'm also thinking to sanitise names to not allow leaving remote-path).

Filename templates for cert, key and ca-chain can be configured in the UI and user input is sanitized to ensure valid file names are produced.
New and some existing fields are using "advanced" markers to reduce amount of fields visible in the default UI.
Also fixed minor bugs including one with unicode characters in names.
Copy link
Copy Markdown
Member

@fabianfrz fabianfrz left a comment

Choose a reason for hiding this comment

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

some of my findings in a quick review.

@jkellerer jkellerer force-pushed the ft-acmeclient-sftp-upload branch from 6c267d5 to a93b802 Compare August 17, 2019 21:19
@fraenki
Copy link
Copy Markdown
Member

fraenki commented Aug 27, 2019

@jkellerer I haven't read all recent commits yet, so... do you consider it to be ready for merge? If that is the case I'd like to do some testing on my own before merging it.

@jkellerer
Copy link
Copy Markdown
Contributor Author

@fraenki I've tested it after the refactoring on 3 different SSH servers (one with non default port and non default SSH implementation) that's why I added some extra commits.
While I tried to test all options I may still have missed something but I think it is ready for a merge.

@fraenki fraenki merged commit 7548dec into opnsense:master Sep 10, 2019
@fraenki
Copy link
Copy Markdown
Member

fraenki commented Sep 10, 2019

Yes we do, I think :) .. currently the final path is created as "{{remote-path}}/{{cert-name}}/cert.pem". To freely configure it, I'll need 3 new fields (for cert.pem, key.pem & ca.pem) with default values {{cert-name}}/cert.pem, {{cert-name}}/key.pem and {{cert-name}}/ca.pem. This replicates current behaviour and allows to freely configure the path or name used below "remote-path". (I'm also thinking to sanitise names to not allow leaving remote-path).

@jkellerer I've merged the current state. Thanks for your contribution! 👍 However, I'd like to see freely configurable filenames. Hope you'll find some time to contribue this in a future PR :)

(Due to my notoriously bad timing this won't be in tomorrow's 19.7.4 release, but instead in the next release.)

@jkellerer
Copy link
Copy Markdown
Contributor Author

Thanks a lot, @fraenki .

Btw. names for all 3 files can be set in "Advanced Mode" (along with unix permissions, group-id and alternate SSH port). So no need for an additional PR, it's already there :)

@jkellerer jkellerer deleted the ft-acmeclient-sftp-upload branch September 11, 2019 18:42
@fraenki
Copy link
Copy Markdown
Member

fraenki commented Sep 15, 2019

Btw. names for all 3 files can be set in "Advanced Mode"

Thanks! 👍

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

Labels

feature Adding new functionality

Development

Successfully merging this pull request may close these issues.

4 participants