Merge ~tiago.pasqualini/charm-nagios:livestatus into charm-nagios:master

[Tiago Pasqualini da Silva]

[Drew Freiberger (afreiberger)]

I've left a few nit-picks in the comments, but specifically, this will require functional test coverage before it can be approved.

I would also suggest filing a bug to state the requirement and help us understand urgency of the need.

I can guess why it may be needed/useful for customers to be able to monitor livestatus externally, but we're not generally spending effort on new features in nagios as it's deprecated for focal.

review: Needs Fixing

[Xav Paice (xavpaice)]

I'd be keen to know the reason for this change - exposing livestatus to the network increases the attack surface. If there's a bug to link to this change, please do so.

review: Needs Information

[Drew Freiberger (afreiberger)]

I spoke with Thiago, and this has been requested by a customer. I think with an update to default to secure and closed from the outside (only_from default to 127.0.0.1) would help a lot to ensure operators must intervene to set specific endpoints and consider the security implication.

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Hi Drew and Xav, I have updated the patch with your requirements. Let me know if there's anything else missing

[Drew Freiberger (afreiberger)]

Thank you for this contribution. We will review this for appropriateness and inclusion for the 21.04 release. I see the items requested in the prior reviews have been addressed.

[Celia Wang (ziyiwang)]

code LGTM

[Linda Guo (lihuiguo)]

Hi Tiago

I understood this is a feature requested by customer. For easily tracking, can you please create a bug and link the bug to this change. Thanks.

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Hi Linda,

Thanks for the review. I created the bug and linked on the commit message.

Thanks,
Tiago

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Hi Folks,

Any updates on this?

Thanks,
Tiago

[Drew Freiberger (afreiberger)]

This LGTM now. Thank you for adding in the default least access necessary configs I'd requested. Still suggest a user story to ensure we have proper context for this change to be added to the bug.

review: Approve

[James Troup (elmo)]

The Juju config needs to document the security considerations of enabling this since, last I looked, livestatus supports neither Authentication nor Authorization. I also agree that some sort of user story in the bug would really help people doing archeology on this.

review: Needs Fixing

[Tiago Pasqualini da Silva (tiago.pasqualini)]

Done. Added the user story to the bug and added some security recommendations to the juju config.

Unmerged commits

12d8b6f... by Tiago Pasqualini da Silva on 2020-10-20

Added livestatus through xinetd

This patch adds the possibility to expose the livestatus socket to
the network by using xinetd. If enable_livestatus and
livestatus_enable_xinetd are both True, it will be exposed to the
network.

Closes-bug: #1916530

diff --git a/config.yaml b/config.yaml
index 1b6d739..765d70b 100644
--- a/config.yaml
+++ b/config.yaml
@@ -58,6 +58,25 @@ options:
         default: ""
         description: |
             Arguments to be passed to the livestatus module, defaults to empty.
+    livestatus_enable_xinetd:
+        type: boolean
+        default: false
+        description: |
+            Expose livestatus socket through a network port using xinetd.
+            NOTE: this socket does not support authentication nor authorization, so exposing it to the
+            network will enable everyone to query it, so it is recommended to limit the hosts that
+            can send queries by listing those hosts in the the livestatus_xinetd_only_from option.
+    livestatus_xinetd_port:
+        type: string
+        default: '6557'
+        description: |
+            Port to be used for livestatus through xinetd.
+    livestatus_xinetd_only_from:
+        type: string
+        default: '127.0.0.1'
+        description: |
+            List of space-separated IPs that will be able to access livestatus through xinetd. If left empty,
+            only localhost will be able to access it.
     nagios_user:
         type: string
         default: nagios
diff --git a/hooks/templates/livestatus.tmpl b/hooks/templates/livestatus.tmpl
 new file mode 100644
index 0000000..0480573
--- /dev/null
+++ b/hooks/templates/livestatus.tmpl
@@ -0,0 +1,19 @@
+service livestatus
+{
+        type            = UNLISTED
+        socket_type     = stream
+        protocol        = tcp
+        wait            = no
+        cps             = 100 3
+        instances       = 500
+        per_source      = 250
+        flags           = NODELAY
+    {%- if xinetd_only_from %}
+        only_from       = {{ xinetd_only_from }}
+    {%- endif %}
+        disable         = no
+        port            = {{ xinetd_port }}
+        user            = nagios
+        server          = /usr/bin/unixcat
+        server_args     = {{ livestatus_path }}
+}
 \ No newline at end of file
diff --git a/hooks/upgrade_charm.py b/hooks/upgrade_charm.py
index 03f0845..6338561 100755
--- a/hooks/upgrade_charm.py
+++ b/hooks/upgrade_charm.py
@@ -54,6 +54,11 @@ ro_password = hookenv.config("ro-password")
 nagiosadmin = hookenv.config("nagiosadmin") or "nagiosadmin"
 contactgroup_members = hookenv.config("contactgroup-members")
 
+livestatus_xinetd_path = "/etc/xinetd.d/livestatus"
+livestatus_enable_xinetd = hookenv.config("livestatus_enable_xinetd")
+livestatus_xinetd_port = hookenv.config("livestatus_xinetd_port")
+livestatus_xinetd_only_from = hookenv.config("livestatus_xinetd_only_from")
+
 # this global var will collect contactgroup members that must be forced
 # it will be changed by functions
 forced_contactgroup_members = []
@@ -626,6 +631,33 @@ def update_password(account, password):
         subprocess.call(["htpasswd", "-D", "/etc/nagios3/htpasswd.users", account])
 
 
+def configure_livestatus_xinetd():
+    if not enable_livestatus or not livestatus_enable_xinetd:
+        hookenv.log("Livestatus xinetd not enabled, skipping...", "INFO")
+        return
+
+    hookenv.log("Configuring livestatus xinetd...", "INFO")
+
+    fetch.apt_update()
+    fetch.apt_install("xinetd")
+
+    template_values = {
+        "livestatus_path": livestatus_path,
+        "xinetd_port": livestatus_xinetd_port,
+        "xinetd_only_from": livestatus_xinetd_only_from,
+    }
+
+    with open("hooks/templates/livestatus.tmpl", "r") as f:
+        template_def = f.read()
+
+    t = Template(template_def)
+    with open(livestatus_xinetd_path, "w") as f:
+        f.write(t.render(template_values))
+
+    host.service_reload("xinetd")
+    hookenv.log("Livestatus xinetd configured.", "INFO")
+
+
 warn_legacy_relations()
 write_extra_config()
 # enable_traps_config and enable_pagerduty_config modify forced_contactgroup_members
@@ -642,6 +674,7 @@ update_localhost()
 update_cgi_config()
 update_contacts()
 update_password("nagiosro", ro_password)
+configure_livestatus_xinetd()
 
 if password:
     update_password(nagiosadmin, password)
diff --git a/tests/functional/test_config.py b/tests/functional/test_config.py
index 5b5423f..a607878 100644
--- a/tests/functional/test_config.py
+++ b/tests/functional/test_config.py
@@ -57,6 +57,17 @@ async def livestatus_path(unit):
         yield app_config["livestatus_path"]["value"]
 
 
[e-mail]
+async def livestatus_socket(unit):
+    """Enable livestatus socket before a test, then disable after test.
+
+    :param Agent unit:              unit from the fixture
+    """
+    async with config(unit, "livestatus_enable_xinetd", "true", "false"):
+        app_config = await unit.application.get_config()
+        yield app_config["livestatus_enable_xinetd"]["value"]
+
+
 @pytest.fixture()
 async def enable_pagerduty(unit):
     """Enable enable_pagerduty before first test, then disable after last test.
@@ -133,6 +144,13 @@ async def test_live_status(unit, livestatus_path, file_stat):
     assert stat["size"] == 0, "File %s didn't match expected size" % livestatus_path
 
 
+async def test_livestatus_xinetd(unit, livestatus_path, livestatus_socket, run_command):
+    assert livestatus_socket is True, "Livestatus xinetd is not enabled"
+    out = await run_command(
+        "echo -e 'GET hosts\nColumns: name\n' | nc 127.0.0.1 6557", unit.u)
+    assert "nagios" in out["Stdout"], "Livestatus output is not expected"
+
+
 async def test_pager_duty(unit, enable_pagerduty, file_stat):
     stat = await file_stat(enable_pagerduty, unit.u)
     assert stat["size"] != 0, "Directory %s wasn't a non-zero size" % enable_pagerduty