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 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

@Saeverix
Copy link

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

+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

@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 pfsense-acme-0.1 branch 3 times, most recently from 8c77fb1 to ba41bee Compare December 23, 2016 00:40
@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
Copy link
Contributor

@jim-p jim-p left a comment

Choose a reason for hiding this comment

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

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}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Must use escapeshellarg() around all parameters involving variables.


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

Choose a reason for hiding this comment

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

Must use escapeshellarg() around all parameters involving variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using escapeshellarg now

safe_mkdir($certpath);

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

Choose a reason for hiding this comment

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

Must use escapeshellarg() around all parameters involving variables.

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

Choose a reason for hiding this comment

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

Must use escapeshellarg() around all parameters involving variables.


$form->add($section);

print $form;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a check basically preventing to show the page

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

Choose a reason for hiding this comment

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

This script needs the execute bit set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.";
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Pool name?

}

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

Choose a reason for hiding this comment

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

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

…warnings, use fa-plus instead of fa-level-down icon
@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

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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=
Copy link
Member

Choose a reason for hiding this comment

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

Remove blank CONFLICT block

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

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
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

@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.
netgate-git-updates pushed a commit that referenced this pull request Jul 11, 2022
Changelog:
 * Fix build with gcc 11.3.0 (resolves #57) by @danvratil in #58
 * Fix ODR violation when LTO is enabled by @danvratil in #59
 * Rework Github CI - add compilers and platforms by @danvratil in #60
 * CI: consolidate the CI builds into a single matrix by @danvratil in #61
 * Export QCoro wrapper classes by @danvratil in #65
 * CI: disable shared libs on Windows by @danvratil in #67
 * Enable "modern Qt" compiler definitions by @danvratil in #66
 * Introduce Generator and AsyncGenerator coroutine return types by @danvratil in #69
 * Fix installing export header by @danvratil in #77
 * Deprecate task.h in favor of qcorotask.h (#70) by @danvratil in #76
 * QtWebSockets support by @danvratil in #75
 * Document -DQCORO_WITH_QTWEBSOCKETS by @dangelog in #81
 * Fix the build by #include'ing by @dangelog in #82
 * QCoroTask: fixup a template type parameter name by @dangelog in #83
 * Add support for clang-cl (fixes #84) by @danvratil in #86
 * Fix possible race in TestWsServer by @danvratil in #88
 * WebSocket bugfixes & documentation by @danvratil in #89
 * Explicitly enable exception support for clang-cl (fixes #90) by @danvratil in #91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.