Skip to content

Update acme.sh#86

Merged
fichtner merged 1 commit intoopnsense:masterfrom
fkling:update_acme
Mar 14, 2017
Merged

Update acme.sh#86
fichtner merged 1 commit intoopnsense:masterfrom
fkling:update_acme

Conversation

@fkling
Copy link
Copy Markdown
Contributor

@fkling fkling commented Mar 10, 2017

See #82 for context. As I mentioned there, I wasn't able to cleanly patch opensense via opnsense-patch, so I don't know if my changes actually work. It seems opnsense-patch tries to patch files into /usr/local/security/acme-client/src/opnsense/... instead of /usr/local/opnsense/..., which makes sense since that's the directory structure of the repo, but I don't know how to change that.

I simply copied the acme.sh file and all files in dnsapi/ and edited certhelper.php, dialogValidation.xml and AcmeClient.xml to the best of my abilities.

@fichtner fichtner requested a review from fraenki March 10, 2017 05:31
@fichtner fichtner self-assigned this Mar 10, 2017
Copy link
Copy Markdown
Member

@fraenki fraenki left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍 I'm going to test the patch later today before approving it.

@fichtner
Copy link
Copy Markdown
Member

@fraenki merge this for 17.1.3's devel version?

@fraenki
Copy link
Copy Markdown
Member

fraenki commented Mar 14, 2017

👍

@fichtner
Copy link
Copy Markdown
Member

That is a yes, no? :D

@fraenki
Copy link
Copy Markdown
Member

fraenki commented Mar 14, 2017

yes :)

@fichtner fichtner merged commit 8cc92b0 into opnsense:master Mar 14, 2017
@fichtner
Copy link
Copy Markdown
Member

fichtner commented Mar 14, 2017

@fkling Thank you! If all goes well, this will be released with 17.1.4. On the upcoming 17.1.3, you can see the plugin from the GUI when you have the development version installed:

# opnsense-update -t opnsense-devel

Changing back to the release via:

# opnsense-update -t opnsense

Or you could also manually install the plugin's development version to test your changes:

# pkg install os-acme-client-devel

<type>text</type>
<help></help>
</field>
<field>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I really don't like this strings as they are really questionable when it comes to translation.
Is "DNS-01/" really required or would it be possible to have "Linode" as a single string here impossible / would loose context?

Copy link
Copy Markdown
Member

@fraenki fraenki Mar 14, 2017

Choose a reason for hiding this comment

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

Let's see how Ad's latest JavaScript magic works out. The prefix was added to have a destinction in the GUI between HTTP-01 and DNS-01 validation methods.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fabian is right. But FWIW this isn't going to be in the translation templates for a few months, hopefully by then we have a nicer solution.

@fkling
Copy link
Copy Markdown
Contributor Author

fkling commented Mar 14, 2017

Thank you everybody!

@fkling fkling deleted the update_acme branch March 18, 2017 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants