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

Proposed by Tiago Pasqualini da Silva
Status: Merged
Approved by: James Troup
Approved revision: 12d8b6fa562226312dfed76721a4573211b4f618
Merged at revision: 841480d41d7ac8cb0aaaa8f1c35013094e8ae937
Proposed branch: ~tiago.pasqualini/charm-nagios:livestatus
Merge into: charm-nagios:master
Diff against target: 150 lines (+89/-0)
4 files modified
config.yaml (+19/-0)
hooks/templates/livestatus.tmpl (+19/-0)
hooks/upgrade_charm.py (+33/-0)
tests/functional/test_config.py (+18/-0)
Reviewer Review Type Date Requested Status
James Troup Approve
Tom Haddon Abstain
BootStack Reviewers Pending
Drew Freiberger Pending
Xav Paice Pending
Review via email: mp+403157@code.launchpad.net

This proposal supersedes a proposal from 2020-10-20.

Commit message

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

To post a comment you must log in.
Revision history for this message
Drew Freiberger (afreiberger) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Xav Paice (xavpaice) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Drew Freiberger (afreiberger) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Tiago Pasqualini da Silva (tiago.pasqualini) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Drew Freiberger (afreiberger) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Xiyue Wang (ziyiwang) wrote : Posted in a previous version of this proposal

code LGTM

Revision history for this message
Linda Guo (lihuiguo) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Tiago Pasqualini da Silva (tiago.pasqualini) wrote : Posted in a previous version of this proposal

Hi Linda,

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

Thanks,
Tiago

Revision history for this message
Tiago Pasqualini da Silva (tiago.pasqualini) wrote : Posted in a previous version of this proposal

Hi Folks,

Any updates on this?

Thanks,
Tiago

Revision history for this message
Drew Freiberger (afreiberger) wrote : Posted in a previous version of this proposal

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
Revision history for this message
James Troup (elmo) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Tiago Pasqualini da Silva (tiago.pasqualini) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Canonical IS Mergebot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
Tom Haddon (mthaddon) wrote :

Abstaining, just claimed the review to take it off the canonical-is-reviewers dashboard, mergebot has been switched to add bootstack-reviewers for future MPs.

review: Abstain
Revision history for this message
James Troup (elmo) wrote :

Thanks, this looks better to me.

Revision history for this message
Canonical IS Mergebot (canonical-is-mergebot) wrote :

Change has no commit message, setting status to needs review.

Revision history for this message
James Troup (elmo) wrote :

Bot, merge.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/config.yaml b/config.yaml
2index 1b6d739..765d70b 100644
3--- a/config.yaml
4+++ b/config.yaml
5@@ -58,6 +58,25 @@ options:
6 default: ""
7 description: |
8 Arguments to be passed to the livestatus module, defaults to empty.
9+ livestatus_enable_xinetd:
10+ type: boolean
11+ default: false
12+ description: |
13+ Expose livestatus socket through a network port using xinetd.
14+ NOTE: this socket does not support authentication nor authorization, so exposing it to the
15+ network will enable everyone to query it, so it is recommended to limit the hosts that
16+ can send queries by listing those hosts in the the livestatus_xinetd_only_from option.
17+ livestatus_xinetd_port:
18+ type: string
19+ default: '6557'
20+ description: |
21+ Port to be used for livestatus through xinetd.
22+ livestatus_xinetd_only_from:
23+ type: string
24+ default: '127.0.0.1'
25+ description: |
26+ List of space-separated IPs that will be able to access livestatus through xinetd. If left empty,
27+ only localhost will be able to access it.
28 nagios_user:
29 type: string
30 default: nagios
31diff --git a/hooks/templates/livestatus.tmpl b/hooks/templates/livestatus.tmpl
32new file mode 100644
33index 0000000..0480573
34--- /dev/null
35+++ b/hooks/templates/livestatus.tmpl
36@@ -0,0 +1,19 @@
37+service livestatus
38+{
39+ type = UNLISTED
40+ socket_type = stream
41+ protocol = tcp
42+ wait = no
43+ cps = 100 3
44+ instances = 500
45+ per_source = 250
46+ flags = NODELAY
47+ {%- if xinetd_only_from %}
48+ only_from = {{ xinetd_only_from }}
49+ {%- endif %}
50+ disable = no
51+ port = {{ xinetd_port }}
52+ user = nagios
53+ server = /usr/bin/unixcat
54+ server_args = {{ livestatus_path }}
55+}
56\ No newline at end of file
57diff --git a/hooks/upgrade_charm.py b/hooks/upgrade_charm.py
58index 03f0845..6338561 100755
59--- a/hooks/upgrade_charm.py
60+++ b/hooks/upgrade_charm.py
61@@ -54,6 +54,11 @@ ro_password = hookenv.config("ro-password")
62 nagiosadmin = hookenv.config("nagiosadmin") or "nagiosadmin"
63 contactgroup_members = hookenv.config("contactgroup-members")
64
65+livestatus_xinetd_path = "/etc/xinetd.d/livestatus"
66+livestatus_enable_xinetd = hookenv.config("livestatus_enable_xinetd")
67+livestatus_xinetd_port = hookenv.config("livestatus_xinetd_port")
68+livestatus_xinetd_only_from = hookenv.config("livestatus_xinetd_only_from")
69+
70 # this global var will collect contactgroup members that must be forced
71 # it will be changed by functions
72 forced_contactgroup_members = []
73@@ -626,6 +631,33 @@ def update_password(account, password):
74 subprocess.call(["htpasswd", "-D", "/etc/nagios3/htpasswd.users", account])
75
76
77+def configure_livestatus_xinetd():
78+ if not enable_livestatus or not livestatus_enable_xinetd:
79+ hookenv.log("Livestatus xinetd not enabled, skipping...", "INFO")
80+ return
81+
82+ hookenv.log("Configuring livestatus xinetd...", "INFO")
83+
84+ fetch.apt_update()
85+ fetch.apt_install("xinetd")
86+
87+ template_values = {
88+ "livestatus_path": livestatus_path,
89+ "xinetd_port": livestatus_xinetd_port,
90+ "xinetd_only_from": livestatus_xinetd_only_from,
91+ }
92+
93+ with open("hooks/templates/livestatus.tmpl", "r") as f:
94+ template_def = f.read()
95+
96+ t = Template(template_def)
97+ with open(livestatus_xinetd_path, "w") as f:
98+ f.write(t.render(template_values))
99+
100+ host.service_reload("xinetd")
101+ hookenv.log("Livestatus xinetd configured.", "INFO")
102+
103+
104 warn_legacy_relations()
105 write_extra_config()
106 # enable_traps_config and enable_pagerduty_config modify forced_contactgroup_members
107@@ -642,6 +674,7 @@ update_localhost()
108 update_cgi_config()
109 update_contacts()
110 update_password("nagiosro", ro_password)
111+configure_livestatus_xinetd()
112
113 if password:
114 update_password(nagiosadmin, password)
115diff --git a/tests/functional/test_config.py b/tests/functional/test_config.py
116index 5b5423f..a607878 100644
117--- a/tests/functional/test_config.py
118+++ b/tests/functional/test_config.py
119@@ -57,6 +57,17 @@ async def livestatus_path(unit):
120 yield app_config["livestatus_path"]["value"]
121
122
123+@pytest.fixture
124+async def livestatus_socket(unit):
125+ """Enable livestatus socket before a test, then disable after test.
126+
127+ :param Agent unit: unit from the fixture
128+ """
129+ async with config(unit, "livestatus_enable_xinetd", "true", "false"):
130+ app_config = await unit.application.get_config()
131+ yield app_config["livestatus_enable_xinetd"]["value"]
132+
133+
134 @pytest.fixture()
135 async def enable_pagerduty(unit):
136 """Enable enable_pagerduty before first test, then disable after last test.
137@@ -133,6 +144,13 @@ async def test_live_status(unit, livestatus_path, file_stat):
138 assert stat["size"] == 0, "File %s didn't match expected size" % livestatus_path
139
140
141+async def test_livestatus_xinetd(unit, livestatus_path, livestatus_socket, run_command):
142+ assert livestatus_socket is True, "Livestatus xinetd is not enabled"
143+ out = await run_command(
144+ "echo -e 'GET hosts\nColumns: name\n' | nc 127.0.0.1 6557", unit.u)
145+ assert "nagios" in out["Stdout"], "Livestatus output is not expected"
146+
147+
148 async def test_pager_duty(unit, enable_pagerduty, file_stat):
149 stat = await file_stat(enable_pagerduty, unit.u)
150 assert stat["size"] != 0, "Directory %s wasn't a non-zero size" % enable_pagerduty

Subscribers

People subscribed via source and target branches