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

acme client package for pfSense, initial commit #89

Closed
wants to merge 14 commits into from

Conversation

@PiBa-NL
Copy link
Contributor

PiBa-NL commented Mar 29, 2016

acme client package for pfSense, initial commit

@mcfedr
Copy link

mcfedr commented Jun 10, 2016

Not sure if its of use, or too late, there is an php implementation of acme thats quite well done, https://github.com/kelunik/acme

@wernerdev
Copy link

wernerdev commented Jun 23, 2016

Can't you just use the FreeBSD Shell script package as base? http://www.freshports.org/security/letsencrypt.sh/

@PiBa-NL
Copy link
Contributor Author

PiBa-NL commented Jun 24, 2016

Yes that 'could' be possible now (it didnt exist when i first created the package..), and i'm willing to change it if one of the ESF guy's tells me that would help get the pfSense package committed. Sofar i have not heard anything from them.

@vpetersson
Copy link

vpetersson commented Oct 1, 2016

+1

@razims
Copy link

razims commented Oct 4, 2016

@rbgarga rbgarga self-assigned this Dec 5, 2016
@rbgarga
Copy link
Member

rbgarga commented Dec 5, 2016

@PiBa-NL you can go ahead and make all necessary changes. Let me know when you have a final version for proper revision. Thanks!

@gonzopancho
Copy link
Member

gonzopancho commented Dec 8, 2016

@PiBa-NL In case it's not clear, when the changes are made and accepted, we'll be bringing this to pfSense.

@idarlund
Copy link

idarlund commented Dec 8, 2016

Take a look at https://github.com/lukas2511/dehydrated - this is the renamed (new) version of the FreeBSD Shell script that @wernerdev referes to.

@PiBa-NL
Copy link
Contributor Author

PiBa-NL commented Dec 8, 2016

Yes i shortly discussed with rbgarga, i am taking a look at the alternatives to the currently used 'lescript' which seems to hardly be maintained at all.. To which might be the 'best' way to integrate one of the other implementations. t.b.h. ive barely looked at further improvement the past half year as it was uncertain to if it would be 'allowed', the 'go ahead' has been received :) , need a few days though to switch

  • kelunik/acme , seems like adding the amp framework just for this is to much complexity/overhead.. evn though the 'php only' implementation is nice..
  • lukas2511/dehydrated , its a official freebsd port (the old version anyhow) which i suppose is a pro..
  • Neilpang/acme.sh , looks to be most actively developed and integrated with dns update options which could prove nice to have

currently i'm leaning towards the Neilpang implementation.. unless there are compelling reasons ive overlooked the others are way better.?

@idarlund
Copy link

idarlund commented Dec 9, 2016

I actually use the DNS update option in acme.sh and it's pretty nice! The good thing about that is that you can still have your haproxy run on port 80 (and 443) without have to turn it off for a cert update. I'd vote for acme.sh if that's the only one who support the DNS update method.

@PiBa-NL
Copy link
Contributor Author

PiBa-NL commented Dec 22, 2016

@rbgarga, i think its ready to commit, still things to improve i guess but those can follow later once people start testing it

@PiBa-NL PiBa-NL force-pushed the PiBa-NL:pfsense-acme-0.1 branch 3 times, most recently from 8c77fb1 to ba41bee Dec 23, 2016
@PiBa-NL PiBa-NL force-pushed the PiBa-NL:pfsense-acme-0.1 branch from ba41bee to d5ebdad Dec 23, 2016
@idarlund
Copy link

idarlund commented Dec 27, 2016

DNS-Manual works as expected! Thanks!
acme/acme_certificates.php is accepting name field with spaces, but the acme script does not wrap the text with quotes. remove space acceptance in php or wrap text.
Last renewed is not updated when running manual "renew" in acme/acme_certificates.php

@hoerup
Copy link
Contributor

hoerup commented Dec 27, 2016

Installed on pfsense 2.3. Works fine with DNS-manual although it couldn't find curl out of the box.

Regarding webroot method, seems that $domain_info['webrootfolder'] was not set so the following code fails

if ($domain_info['method'] == 'webroot') {
echo "webroot\n";
$directory = $domain_info['webrootfolder'];
if(!file_exists($directory) && !@mkdir($directory, 0755, true)) {
throw new \RuntimeException("Couldn't create directory to expose challenge: ${tokenPath}");
}

…th proper refid references, validate certificatename configured does not contain spaces as acme.sh dies not handle them well
@heper
Copy link
Contributor

heper commented Jan 9, 2017

@PiBa-NL
i've tried to make the package in a VM(no clue what i'm doing)
but when doing pkg add on pfSense:
[2.3.2-RELEASE][root@pfsense.lan]/root: pkg add pfSense-pkg-acme-0.1.txz Installing pfSense-pkg-acme-0.1... pkg: Missing dependency 'php56-ftp'

i probably did something wrong when creating the package

i created the php56-ftp package in VM & copied it to pfSense, all seems well.

@rbgarga rbgarga requested review from rbgarga and jim-p Jan 9, 2017
Copy link
Contributor

jim-p left a comment

Seems to operate but could use some refinement and security improvements. See my inline comments for details.

function generateAccountKey() {
unlink_if_exists("{$this->path_account}/account.key");
$this->debug = false;
$this->execacmesh("--home {$this->acmeconf} --createAccountKey --accountkeylength 4096 --accountconf {$this->accountconfig}");

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

Must use escapeshellarg() around all parameters involving variables.


function registeraccount($key) {
file_put_contents("{$this->path_account}/account.key", $key);
$result = $this->execacmesh(""

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

Must use escapeshellarg() around all parameters involving variables.

This comment has been minimized.

Copy link
@PiBa-NL

PiBa-NL Jan 10, 2017

Author Contributor

using escapeshellarg now

safe_mkdir($certpath);

unlink_if_exists("{$certpath}/{$domain}.key");
$this->execacmesh(""

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

Must use escapeshellarg() around all parameters involving variables.

foreach($domainstosign as $domain) {
$domainstr .= " -d {$domain}";
}
$this->execacmesh(""

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

Must use escapeshellarg() around all parameters involving variables.


$form->add($section);

print $form;

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

With only this one option and just the checkbox text this page seems really bare. Maybe at least add a little description text about what the renewal job does?
For example, if it uses cron, how often it runs, etc, etc.

This comment has been minimized.

Copy link
@PiBa-NL

PiBa-NL Jan 10, 2017

Author Contributor

added a check basically preventing to show the page

@@ -0,0 +1,66 @@
#!/usr/local/bin/php -f

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

This script needs the execute bit set.

This comment has been minimized.

Copy link
@PiBa-NL

PiBa-NL Jan 9, 2017

Author Contributor

makefile sets -m 0755, that should be enough right?

This comment has been minimized.

Copy link
@PiBa-NL

PiBa-NL Jan 10, 2017

Author Contributor

Now changed to use INSTALL_DATA/INSTALL_SCRIPT macros in the Makefile.

/* Ensure that our pool names are unique */
for ($i=0; isset($config['installedpackages']['acme']['accountkeys']['item'][$i]); $i++) {
if (($_POST['name'] == $config['installedpackages']['acme']['accountkeys']['item'][$i]['name']) && ($i != $id)) {
$input_errors[] = "This pool name has already been used. Pool names must be unique.";

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

"Pool name"? Shouldn't this be "Account Key Name"?

}

if($accountkey['name'] != "") {
$changedesc .= " modified pool: '{$accountkey['name']}'";

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

Pool?

/* Ensure that our pool names are unique */
for ($i=0; isset($config['installedpackages']['acme']['certificates']['item'][$i]); $i++) {
if (($_POST['name'] == $config['installedpackages']['acme']['certificates']['item'][$i]['name']) && ($i != $id)) {
$input_errors[] = "This pool name has already been used. Pool names must be unique.";

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

Pool name?

}

if($certificate['name'] != "") {
$changedesc .= " modified pool: '{$certificate['name']}'";

This comment has been minimized.

Copy link
@jim-p

jim-p Jan 9, 2017

Contributor

Pool?

@rbgarga
Copy link
Member

rbgarga commented Jan 9, 2017

I'll wait the changes for items pointed out by @jim-p before start reviewing

@jim-p
Copy link
Contributor

jim-p commented Jan 9, 2017

One more question: Why is nsupdate support commented out? That would be one of the best choices for a lot of users.

@PiBa-NL
Copy link
Contributor Author

PiBa-NL commented Jan 10, 2017

Regarding nsupdate, i will add below run_depends item, would that make its binary available.?
RUN_DEPENDS= samba-nsupdate:dns/samba-nsupdate

@jim-p
Copy link
Contributor

jim-p commented Jan 10, 2017

We already have nsupdate, it is what we use for RFC 2136 updates.
: pkg which /usr/local/bin/nsupdate
/usr/local/bin/nsupdate was installed by package bind-tools-9.11.0P1_1

@PiBa-NL PiBa-NL force-pushed the PiBa-NL:pfsense-acme-0.1 branch from c01aeac to f885d77 Jan 10, 2017
…warnings, use fa-plus instead of fa-level-down icon
@PiBa-NL PiBa-NL force-pushed the PiBa-NL:pfsense-acme-0.1 branch from f885d77 to 6c9fa13 Jan 10, 2017
@PiBa-NL
Copy link
Contributor Author

PiBa-NL commented Jan 10, 2017

Ah ok, well then i think all comments have been addressed.

The buttons for generating/registering account key still change to a check-mark when successful, i think thats alright. I've added the nsupdate option (someone should test it..). Added a little more text. Removed the 'pool' texts (which came from copy/paste from haproxy package..)

Lemme know if i missed something :).

@rbgarga
Copy link
Member

rbgarga commented Jan 10, 2017

@PiBa-NL here is output of 'portlint -CN', please fix all warnings and fatal messages

WARN: /usr/home/garga/work/pfsense/freebsd-ports/security/pfSense-pkg-acme/pkg-plist: [32]: empty line found in plist.
WARN: /usr/home/garga/work/pfsense/freebsd-ports/security/pfSense-pkg-acme/pkg-plist: [33]: empty line found in plist.
WARN: /usr/home/garga/work/pfsense/freebsd-ports/security/pfSense-pkg-acme/pkg-plist: [34]: empty line found in plist.
WARN: /usr/home/garga/work/pfsense/freebsd-ports/security/pfSense-pkg-acme/pkg-plist: seems to have unnecessary blank lines at the last part.
WARN: /usr/home/garga/work/pfsense/freebsd-ports/security/pfSense-pkg-acme/pkg-descr: includes lines that exceed 80 characters.
FATAL: Makefile: [13]: use a tab (not space) after a variable name
WARN: Makefile: [15]: whitespace before end of line.
FATAL: Makefile: MAINTAINER address, PiBa-NL, does not appear to be a valid email address.
WARN: Makefile: COMMENT should begin with a capital, and end without a period
WARN: Makefile: Consider defining LICENSE.

MAINTAINER= PiBa-NL
COMMENT= pfSense package acme

This comment has been minimized.

Copy link
@rbgarga

rbgarga Jan 10, 2017

Member

Add LICENSE= APACHE20 between COMMENT and USE_PHP leaving a blank line before and after

MAINTAINER= PiBa-NL
COMMENT= pfSense package acme

USE_PHP= ftp

This comment has been minimized.

Copy link
@rbgarga

rbgarga Jan 10, 2017

Member

USE_PHP alone is deprecated. keep this line replacing space by TAB to separate = and value and add the following line before it:

USES=	php

USE_PHP= ftp

CONFLICTS=

This comment has been minimized.

Copy link
@rbgarga

rbgarga Jan 10, 2017

Member

Remove blank CONFLICT block

do-install:
${MKDIR} ${STAGEDIR}${PREFIX}/pkg
${MKDIR} ${STAGEDIR}${PREFIX}/pkg/acme
${MKDIR} ${STAGEDIR}${PREFIX}/pkg/acme/dnsapi

This comment has been minimized.

Copy link
@rbgarga

rbgarga Jan 10, 2017

Member

MKDIR macro is 'mkdir -p' so you just need to run it to last level and it will create parent directories, remove lines creating pkg and pkg/acme

${MKDIR} ${STAGEDIR}${PREFIX}/pkg
${MKDIR} ${STAGEDIR}${PREFIX}/pkg/acme
${MKDIR} ${STAGEDIR}${PREFIX}/pkg/acme/dnsapi
${MKDIR} ${STAGEDIR}${PREFIX}/www

This comment has been minimized.

Copy link
@rbgarga

rbgarga Jan 10, 2017

Member

Remove this line, next one will create this directory too

www/acme/acme_generalsettings.php
/etc/inc/priv/acme.priv.inc
%%DATADIR%%/info.xml
@dir /etc/inc/priv

This comment has been minimized.

Copy link
@rbgarga

rbgarga Jan 10, 2017

Member

Add @dir /etc/inc and remove all blank lines

@jim-p
Copy link
Contributor

jim-p commented Jan 17, 2017

I'm still checking this out but got sidetracked by other things I was working on. I did notice that in the Domain SAN list when you edit an entry the form fields only take up a little amount of space horizontally and it looks weird.

And it may be beyond the scope of this code, but I'd like to see the nsupdate method support an additional field for a domain suffix or fulldomain override. I can see people not wanting to add dynamic updates for their entire zone, but you could make a new dynamic zone or subdomain and CNAME the _acme-challenge record there. For example, dynamic zones happen in .le.example.com. -- so _acme-challenge.server would be a CNAME to server.example.com.le.example.com similar to what is mentioned at https://www.crc.id.au/using-centralised-management-with-lets-encrypt/ -- That may be something we can add on after this gets merged.

@jim-p
Copy link
Contributor

jim-p commented Jan 17, 2017

Screen cap of the weird formatting: http://i.imgur.com/p1JjE4I.png

…e for nsupdate method, improved layout for these extra fields
@jim-p
jim-p approved these changes Jan 23, 2017
@jim-p
Copy link
Contributor

jim-p commented Jan 23, 2017

Looks like maybe it's to a point where we could pull it in and hack on it some more as we go.

@rbgarga
Copy link
Member

rbgarga commented Jan 23, 2017

Merged in a single commit (979e112) to make it easier. Thanks!!!

@rbgarga rbgarga closed this Jan 23, 2017
@OmgImAlexis
Copy link

OmgImAlexis commented Jan 24, 2017

@jim-p roughly how long until this commit will make it into master?

@jim-p
Copy link
Contributor

jim-p commented Jan 24, 2017

It's in master, but on master new packages only show up when a new snapshot is built. So the next snapshot will have it. It's possible as well that it wasn't built with the latest snapshot run and needs adjustments yet. So probably tomorrow sometime if it isn't up already

netgate-git-updates pushed a commit that referenced this pull request Sep 5, 2018
  - move public facing stuff into public/, this allows us to stop exposing
    templates_c/ etc. to the world (but also means you'll need to adjust your
    webserver config)
  - enable users to reset their passwords by mail or SMS
    ($CONF['forgotten_user_password_reset'],
    $CONF['forgotten_admin_password_reset'], $CONF['sms_send_function'])
  - allow local alias targets (without @Domain) - see #134
  - add $CONF['edit_alias'] to disable "edit_alias" function for users
  - add php_crypt $CONF["encrypt"] option (see #170 for examples)
  - add random_compat phar (see: https://github.com/paragonie/random_compat) to
    support random_int()/random_string() in older PHP versions.
  - add support for MySQL connections over SSL
  - language updates: sk, ja, nl, bg, fr, cz
  - update bundled smarty library (lib/smarty to 3.1.32; includes security fixes)
  - split up pacrypt() into different functions; add some minimal test coverage
  - add id autoincrement field to log table (#89)
  - add token to login.php to prevent CSRF
  - lots of bugfixes and code cleanup
  - drop unused code in postfixadmin-cli
  - introduce PHP-CS-Fixer to enforce code style
  - vacation.pl:
    - avoid answering to more known autoresponders
    - add $no_vacation_pattern to avoid sending autoresponders based on the To:
      address
    - replace Deprecated Mail::Sender by Email::Sender
    - use MIME:EncWords
    - remove unused MIME::Base64
- add docker repo, see https://github.com/postfixadmin/docker

PR:		229370
Submitted by:	Melissa Pilgrim <ports.maintainer@evilphi.com> (maintainer)
Sponsored by:	Netzkommune
netgate-git-updates pushed a commit that referenced this pull request Jan 3, 2019
This is a minor bugfix release. From the changelog:
  * Fix duplicate reference values
  * Fix double prompts for overwriting files
  * Fix map variables visibility when opening maps
  * Fix document generator dialog's tab navigation
  * Fix boundary objects' text alignment (diagrams)
  * Fix pipe objects' resizing (diagrams)
  * Fix component objects' margin sizes
  * Enable newlines on most diagram objects #89
  * Add global settings for commands to execute after a template is called
netgate-git-updates pushed a commit that referenced this pull request Dec 20, 2019
  * add -q and document -v

  * Use CURLOPT_PATH_AS_IS option if it is available

  * Correct -r to be "-r rrset". Correct batch rdata/ip to be
    "rdata/ip/ADDR[,PFXLEN]"

  * In man page: remove duplicate -v flag description. Move -q flag to be
    in alphabetical order

  * Change -R option to be a -N option.  Add a new -R option that searches
    the left-hand-side. Reference dnstable-encoding(5)

  * fix no-newline-eof bug in -f for jonas; add -f -f support (verbose
    batching)

  * correct behaviour of -l and -L with respect to -f and -m

  * only call writer_fini() from the scope who called writer_init(),
    except in the my_exit() path

  * kill the sort on overcount

  * add error code and description to the -f -f ++ marker

  *  fix abort in writer_status() by changing where "once" lives from
     reader to writer

  * fix no-newline-eof bug in -f; add -f -f support (verbose batching)

  * Change parsing -l and -L to use parse_long so it will catch parsing
    errors

  * Fix an assert

  * fix #90 with some warning and documentation changes

  * fix #89 by restructuring the validate_verb() system

  * fix #88 by better explaining text vs. dns formats

  * fix #87 by reorganizing some man page text

  * fix #86 by checking argument to -A and -B options

  * fix #83 by rewording the -k section of the man page

  * replace text for -A and -B options

  * improve documentation about interaction of -s/-S with -l/-L

  * fix #81

  * improve documentation of -c to explain about -A + -B quota impact

  * time, not date

  * fix everything noted in #85

  * document RRset (raw) query. document HEX better (#111)

Restrict portscount to real release tags.

Sponsored by:	Farsight Security, Inc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.