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

security/acme-client: bugfix release 1.3 #91

Merged
merged 10 commits into from Mar 21, 2017
Merged

Conversation

@fraenki
Copy link
Member

fraenki commented Mar 20, 2017

New features

  • GUI: hide HTTP-01 validation methods when not selected (similar to DNS-01)
  • GUI: hide parameters for restart actions when not selected
  • Advanced: make the timeout for (custom) restart commands configurable

Bugfixes

  • properly import CA certificates (closes #84)
  • do not make keys world-readable
  • remove DNS-01 and HTTP-01 prefixes from validation names (they are obsolete now)
  • avoid log message on missing restart action (fixes df2a727)

NOTE: Deleting an imported Let's Encrypt CA will automatically delete all LE certificates that are referenced.

For reference: A Let's Encrypt CA will only be imported on issue or renewal of a LE certificate. It may be required to forcefully renew an existing certificate to allow the CA to be imported.

@fraenki fraenki changed the title security/acme-client: next bugfix release [WIP] security/acme-client: next bugfix release Mar 20, 2017
@fraenki

This comment has been minimized.

Copy link
Member Author

fraenki commented Mar 21, 2017

I've removed the prefixes as requested by @fabianfrz in #86 (review).

@fraenki

This comment has been minimized.

Copy link
Member Author

fraenki commented Mar 21, 2017

@AdSchellevis Please review f65be49 :)

$(".table_optional").hide();
if (($("#action\\.type").val() == 'configd') || ($("#action\\.type").val() == 'custom')) {
$("."+service_id).show();
} else {

This comment has been minimized.

Copy link
@AdSchellevis

AdSchellevis Mar 21, 2017

Member

@fraenki empty else clause.

This comment has been minimized.

Copy link
@fraenki

fraenki Mar 21, 2017

Author Member

thx, fixed already

<field>
<label>Required Parameters</label>
<type>header</type>
<style>method_table method_table_custom</style>
</field>
<field>
<id>action.custom</id>

This comment has been minimized.

Copy link
@AdSchellevis

AdSchellevis Mar 21, 2017

Member

@fraenki probably a bit off topic, but what does this run?

This comment has been minimized.

Copy link
@fraenki

fraenki Mar 21, 2017

Author Member

Hmm, it's the "container" for the optional input fields. I'm not sure how it might work without this.

This comment has been minimized.

Copy link
@AdSchellevis

AdSchellevis Mar 21, 2017

Member

I'm not sure what you mean, can you point me to the code where it's used?

This comment has been minimized.

Copy link
@AdSchellevis

AdSchellevis Mar 21, 2017

Member

sorry, I meant the glue to the system action, not the volt template. My question was about the use of the parameter, it looked like an option to input shell commands, so I was curious :)

This comment has been minimized.

Copy link
@fraenki

fraenki Mar 21, 2017

Author Member

It is indeed:

$proc_cmd = (string)$action->custom;
$proc = proc_open($proc_cmd, $proc_desc, $proc_pipes, null, $proc_env);
// Make sure the resource could be setup properly
if (is_resource($proc)) {
fclose($proc_pipes[0]);
// Wait until process terminates normally
while (is_resource($proc)) {
$proc_stdout .= stream_get_contents($proc_pipes[1]);
$proc_stderr .= stream_get_contents($proc_pipes[2]);
// Check if timeout is reached
if (($timeout !== false) and ((time() - $starttime) > $timeout)) {
// Terminate process if timeout is reached
log_error("AcmeClient: timeout running restart action: " . $action->name);
proc_terminate($proc, 9);
$result = '99';
break;
}
// Check if process terminated normally
$status = proc_get_status($proc);
if (!$status['running']) {
fclose($proc_pipes[1]);
fclose($proc_pipes[2]);
proc_close($proc);
$result = $status['exitcode'];
break;
}
usleep(100000);
}
} else {
log_error("AcmeClient: unable to initiate restart action: " . $action->name);
continue; // Continue with next action.
}
$return = $result;

This comment has been minimized.

Copy link
@AdSchellevis

AdSchellevis Mar 21, 2017

Member

Is this really necessary? this causes quite some security concerns, we've tried to prevent for all other components we've developed.
Creating configd templates is not very complex and makes sure you can only execute safe code.
If someone adds rm -rf / your firewall is gone....

This comment has been minimized.

Copy link
@fraenki

fraenki Mar 21, 2017

Author Member

I'm aware of the risk. I want to keep the plugin as flexible as possible and by far not all system commands are available as configd commands. Let me think about this a little bit... maybe we can discuss this further on IRC later this week...

This comment has been minimized.

Copy link
@AdSchellevis

AdSchellevis Mar 21, 2017

Member

@fraenki sure, I'll try to be online on IRC on Friday

$("#action\\.type").change(function(){
var service_id = 'table_optional_' + $(this).val();
$(".table_optional").hide();
if (($("#action\\.type").val() == 'configd') || ($("#action\\.type").val() == 'custom')) {

This comment has been minimized.

Copy link
@AdSchellevis

AdSchellevis Mar 21, 2017

Member

probably easier to just $("."+service_id).show() , I don't expect it would mind if the class isn't there.

This comment has been minimized.

Copy link
@fraenki

fraenki Mar 21, 2017

Author Member

fixed in 6fcc7f8

@fraenki fraenki changed the title [WIP] security/acme-client: next bugfix release [WIP] security/acme-client: bugfix release 1.3 Mar 21, 2017
@fichtner fichtner self-assigned this Mar 21, 2017
@fraenki fraenki changed the title [WIP] security/acme-client: bugfix release 1.3 security/acme-client: bugfix release 1.3 Mar 21, 2017
@fraenki

This comment has been minimized.

Copy link
Member Author

fraenki commented Mar 21, 2017

@fichtner Feel free to merge if the next release of OPNsense is near, otherwise I'll keep on pushing fixes.

@fichtner

This comment has been minimized.

Copy link
Member

fichtner commented Mar 21, 2017

@fraenki are you ok with the other dns updates and acme.sh update that went in previously to land in stable/17.1?

@fraenki

This comment has been minimized.

Copy link
Member Author

fraenki commented Mar 21, 2017

Yes, they should be included in this release.

@fichtner fichtner merged commit a3520c7 into opnsense:master Mar 21, 2017
@fichtner

This comment has been minimized.

Copy link
Member

fichtner commented Mar 21, 2017

@fraenki excellent... merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.