Merge ~peter-sabaini/charm-sudo-pair:feature/add-pd-logging into charm-sudo-pair:master

Proposed by Peter Sabaini
Status: Rejected
Rejected by: Eric Chen
Proposed branch: ~peter-sabaini/charm-sudo-pair:feature/add-pd-logging
Merge into: charm-sudo-pair:master
Diff against target: 297 lines (+143/-6)
10 files modified
src/actions/actions.py (+13/-0)
src/config.yaml (+8/-0)
src/files/pagerdutyevent.py (+68/-0)
src/lib/libsudopair.py (+33/-1)
src/reactive/sudo_pair.py (+4/-0)
src/templates/sudo_approve.tmpl (+5/-2)
src/templates/sudo_pair.pagerduty.tmpl (+6/-0)
src/tests/unit/conftest.py (+2/-1)
src/tests/unit/test_actions.py (+3/-1)
src/tests/unit/test_libsudopair.py (+1/-1)
Reviewer Review Type Date Requested Status
James Troup (community) Needs Fixing
Paul Goins Approve
Tom Haddon Abstain
BootStack Reviewers mr tracking; do not claim Pending
Giuseppe Petralia Pending
Review via email: mp+403957@code.launchpad.net

This proposal supersedes a proposal from 2021-06-09.

Commit message

Pagerduty alerting, action logging

Ability to have pagerduty alerts triggered on auto-approve. Revamp logging, add logging for remove-sudopair action. Use https proxy to contact the PD events endpoint if set in model config.

To post a comment you must log in.
Revision history for this message
Giuseppe Petralia (peppepetra) wrote : Posted in a previous version of this proposal

Small comment on the pagerduty_proxy that may be removed in favor of the juju model-config if any.

Comments in line.

Other than that looks good to me.

review: Needs Fixing
Revision history for this message
Paul Goins (vultaire) wrote : Posted in a previous version of this proposal

I agree with Giuseppe. I think we should pull the proxy from the env's JUJU_CHARM_HTTPS_PROXY variable to avoid proliferating proxy settings. Other than that I'm +1.

review: Needs Fixing
Revision history for this message
Peter Sabaini (peter-sabaini) wrote : Posted in a previous version of this proposal

Thanks -- proxy config updated as requested. I've resubmitted the branch as there was some code reorg interim.

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

LGTM.

Revision history for this message
Paul Goins (vultaire) wrote : Posted in a previous version of this proposal

Sorry for the delay here - It looks like this is close to if not identical to https://code.launchpad.net/~peter-sabaini/charm-sudo-pair/+git/sudo-pair-charm/+merge/377884, except that it might be based off newer code. I don't see any changes re: proxy variable use; this code is still using pagerduty_proxy rather than e.g. JUJU_CHARM_HTTPS_PROXY.

review: Needs Fixing
Revision history for this message
Peter Sabaini (peter-sabaini) wrote :

Apologies, appears I superseded with the wrong branch here. Trying this again

Revision history for this message
🤖 Canonical IS Merge Bot (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
Paul Goins (vultaire) wrote :

Looks good to me!

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

Couple of comments inline; I consider the config file permissions blocking unless I'm wrong about the perms?

review: Needs Fixing
7b10f8a... by Peter Sabaini

Address review comments

Revision history for this message
Peter Sabaini (peter-sabaini) wrote :

Should be better now, please take a look

Unmerged commits

7b10f8a... by Peter Sabaini

Address review comments

3681b10... by Peter Sabaini

Pagerduty alerting, action logging

Ability to have pagerduty alerts triggered on auto-approve. Revamp
logging, add logging for remove-sudopair action. Use https proxy to
contact the PD events endpoint if set in model config.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/src/actions/actions.py b/src/actions/actions.py
index 9ff1ec5..65c5a13 100755
--- a/src/actions/actions.py
+++ b/src/actions/actions.py
@@ -14,7 +14,10 @@
14# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.14# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15# See the License for the specific language governing permissions and15# See the License for the specific language governing permissions and
16# limitations under the License.16# limitations under the License.
17import logging
18import logging.handlers
17import os19import os
20import subprocess
18import sys21import sys
1922
20from charmhelpers.core.hookenv import action_fail, action_set, status_set23from charmhelpers.core.hookenv import action_fail, action_set, status_set
@@ -25,11 +28,21 @@ sys.path.append("lib")
25import libsudopair # NOQA28import libsudopair # NOQA
2629
2730
31logger = logging.getLogger("sudopair")
32logger.setLevel(logging.INFO)
33handler = logging.handlers.SysLogHandler(
34 address="/dev/log", facility=logging.handlers.SysLogHandler.LOG_AUTH
35)
36logger.addHandler(handler)
37
38
28def remove():39def remove():
29 """Remove sudo-pair config and binaries."""40 """Remove sudo-pair config and binaries."""
30 sph = libsudopair.SudoPairHelper()41 sph = libsudopair.SudoPairHelper()
31 sph.deconfigure()42 sph.deconfigure()
32 msg = "Successfully removed sudo-pair config and binaries"43 msg = "Successfully removed sudo-pair config and binaries"
44 logger.warning(msg)
45 subprocess.run(["/usr/local/bin/pagerdutyevent", msg])
33 action_set({"message": msg})46 action_set({"message": msg})
34 status_set("active", msg)47 status_set("active", msg)
3548
diff --git a/src/config.yaml b/src/config.yaml
index 795269b..6089ac9 100644
--- a/src/config.yaml
+++ b/src/config.yaml
@@ -19,3 +19,11 @@ options:
19 type: boolean19 type: boolean
20 default: true20 default: true
21 description: "If true, auto approval is permitted."21 description: "If true, auto approval is permitted."
22 pagerduty_key:
23 type: string
24 default: ''
25 description: "If set, a pagerduty event will be triggered upon auto-approving"
26 pagerduty_context:
27 type: string
28 default: ''
29 description: "Prefix to add to pagerduty events"
diff --git a/src/files/pagerdutyevent.py b/src/files/pagerdutyevent.py
22new file mode 10064430new file mode 100644
index 0000000..f33be42
--- /dev/null
+++ b/src/files/pagerdutyevent.py
@@ -0,0 +1,68 @@
1#!/usr/bin/env python3
2"""Send events to pagerduty."""
3
4import argparse
5import configparser
6import json
7import logging
8import logging.handlers
9
10import requests
11
12logger = logging.getLogger("sudopair")
13logger.setLevel(logging.INFO)
14handler = logging.handlers.SysLogHandler(
15 address="/dev/log", facility=logging.handlers.SysLogHandler.LOG_AUTH
16)
17logger.addHandler(handler)
18
19
20def args():
21 """Get cli args."""
22 parser = argparse.ArgumentParser()
23 parser.add_argument("summary")
24 return parser.parse_args()
25
26
27def trigger_incident(pd_key, summary, source, https_proxy=None):
28 """Send an event to the PD events endpoint."""
29 header = {"Content-Type": "application/json"}
30
31 payload = {
32 "routing_key": pd_key,
33 "event_action": "trigger",
34 "payload": {"summary": summary, "source": source, "severity": "info"},
35 }
36 if https_proxy:
37 proxies = {"https": https_proxy}
38 else:
39 proxies = None
40 response = requests.post(
41 "https://events.pagerduty.com/v2/enqueue",
42 data=json.dumps(payload),
43 proxies=proxies,
44 headers=header,
45 )
46
47 if response.json()["status"] == "success":
48 logger.info("Triggered alert with key {}".format(response.json()["dedup_key"]))
49 else:
50 logger.warning("Failed to send pagerduty alert: {}".format(response.text))
51
52
53def get_conf():
54 """Return config options from file."""
55 config = configparser.ConfigParser()
56 config.read("/etc/sudo_pair.pagerduty.ini")
57 sec = config["DEFAULT"]
58 return sec["pagerduty_key"], sec["pagerduty_source"], sec.get("https_proxy")
59
60
61def main():
62 """Run the script."""
63 pd_key, source, https_proxy = get_conf()
64 trigger_incident(pd_key, args().summary, source, https_proxy)
65
66
67if __name__ == "__main__":
68 main()
diff --git a/src/lib/libsudopair.py b/src/lib/libsudopair.py
index 2f7c45b..24dfcf2 100644
--- a/src/lib/libsudopair.py
+++ b/src/lib/libsudopair.py
@@ -67,6 +67,9 @@ class SudoPairHelper(object):
67 self.sudoers_perms = 0o44067 self.sudoers_perms = 0o440
68 self.sudo_conf_perms = 0o64468 self.sudo_conf_perms = 0o644
69 self.sudo_approve_perms = 0o75569 self.sudo_approve_perms = 0o755
70 self.pagerduty_conf_path = "/etc/sudo_pair.pagerduty.ini"
71 self.pagerduty_path = "/usr/local/bin/pagerdutyevent"
72 self.pagerduty_perms = 0o755
7073
71 def get_config(self):74 def get_config(self):
72 """Return config as a dict."""75 """Return config as a dict."""
@@ -81,8 +84,11 @@ class SudoPairHelper(object):
81 "gids_exempted": group_names_to_group_ids(84 "gids_exempted": group_names_to_group_ids(
82 self.charm_config["groups_exempted"]85 self.charm_config["groups_exempted"]
83 ),86 ),
87 "pagerduty_key": self.charm_config.get("pagerduty_key"),
84 }88 }
8589 proxy_settings = hookenv.env_proxy_settings()
90 if proxy_settings:
91 config["https_proxy"] = proxy_settings.get("https_proxy")
86 config.update(self.charm_config)92 config.update(self.charm_config)
87 return config93 return config
8894
@@ -90,6 +96,13 @@ class SudoPairHelper(object):
90 """Update configuration."""96 """Update configuration."""
91 self.charm_config = charm_config97 self.charm_config = charm_config
9298
99 def copy_pagerduty(self):
100 """Copy PD script in place."""
101 pd_file = os.path.join(hookenv.charm_dir(), "files", "pagerdutyevent")
102 copy_file(
103 pd_file, self.pagerduty_path, self.owner, self.group, self.pagerduty_perms
104 )
105
93 def render_sudo_conf(self):106 def render_sudo_conf(self):
94 """Render sudo.conf file."""107 """Render sudo.conf file."""
95 return templating.render(108 return templating.render(
@@ -184,6 +197,25 @@ class SudoPairHelper(object):
184 )197 )
185 return None198 return None
186199
200 def render_pagerduty_conf(self):
201 """Create the PD script configuration."""
202 pd_source = "{}-{}".format(
203 self.charm_config.get("pagerduty_context", "juju"), hookenv.principal_unit()
204 )
205 cfg = self.get_config()
206 cfg["pagerduty_source"] = pd_source
207 hookenv.log(
208 "Rendering pagerduty configuration to {}".format(self.pagerduty_conf_path)
209 )
210 return templating.render(
211 "sudo_pair.pagerduty.tmpl",
212 self.pagerduty_conf_path,
213 cfg,
214 owner=self.owner,
215 group=self.group,
216 perms=0o400,
217 )
218
187 def deconfigure(self):219 def deconfigure(self):
188 """Remove sudo-pair configuration."""220 """Remove sudo-pair configuration."""
189 paths = [221 paths = [
diff --git a/src/reactive/sudo_pair.py b/src/reactive/sudo_pair.py
index caab64f..392383b 100644
--- a/src/reactive/sudo_pair.py
+++ b/src/reactive/sudo_pair.py
@@ -32,6 +32,10 @@ def install_sudo_pair():
32 # Add "Defaults log_output to /etc/sudoers32 # Add "Defaults log_output to /etc/sudoers
33 sph.copy_sudoers()33 sph.copy_sudoers()
3434
35 # Add pagerduty script and config
36 sph.copy_pagerduty()
37 sph.render_pagerduty_conf()
38
35 # If there are cmds to bypass sudo pairing create file unders sudoers.d39 # If there are cmds to bypass sudo pairing create file unders sudoers.d
36 sph.render_bypass_cmds()40 sph.render_bypass_cmds()
3741
diff --git a/src/templates/sudo_approve.tmpl b/src/templates/sudo_approve.tmpl
index 7164b5a..5ebace4 100755
--- a/src/templates/sudo_approve.tmpl
+++ b/src/templates/sudo_approve.tmpl
@@ -88,7 +88,7 @@ main() {
88 declare -r username88 declare -r username
8989
90 declare log_line90 declare log_line
91 log_line="$(date "+[%b %d %H:%M:%S] WARNING: ${username} approved is own sudo session.")"91 log_line="WARNING: ${username} approved own sudo session."
92 declare -r log_line92 declare -r log_line
9393
94 if [[ "${uid}" -eq "${ruid}" ]]; then94 if [[ "${uid}" -eq "${ruid}" ]]; then
@@ -97,7 +97,10 @@ main() {
97 exit 197 exit 1
98 {% else %}98 {% else %}
99 echo "You are approving your own session. The incident will be logged."99 echo "You are approving your own session. The incident will be logged."
100 echo ${log_line} >> /var/log/sudo_pair.log100 logger -p auth.warn $log_line
101 {% if pagerduty_key %}
102 /usr/local/bin/pagerdutyevent "$log_line"
103 {% endif %}
101 {% endif %}104 {% endif %}
102 fi105 fi
103106
diff --git a/src/templates/sudo_pair.pagerduty.tmpl b/src/templates/sudo_pair.pagerduty.tmpl
104new file mode 100644107new file mode 100644
index 0000000..2185cf9
--- /dev/null
+++ b/src/templates/sudo_pair.pagerduty.tmpl
@@ -0,0 +1,6 @@
1[DEFAULT]
2pagerduty_key: {{ pagerduty_key }}
3pagerduty_source: {{ pagerduty_source }}
4{% if https_proxy -%}
5https_proxy: {{ https_proxy }}
6{% endif %}
0\ No newline at end of file7\ No newline at end of file
diff --git a/src/tests/unit/conftest.py b/src/tests/unit/conftest.py
index c1df1d0..bc65d15 100644
--- a/src/tests/unit/conftest.py
+++ b/src/tests/unit/conftest.py
@@ -16,7 +16,7 @@ def mock_hookenv_config(monkeypatch):
1616
17 def mock_config():17 def mock_config():
18 cfg = {}18 cfg = {}
19 yml = yaml.load(open("./config.yaml"))19 yml = yaml.safe_load(open("./config.yaml"))
2020
21 # Load all defaults21 # Load all defaults
22 for key, value in yml["options"].items():22 for key, value in yml["options"].items():
@@ -54,4 +54,5 @@ def sph(mock_hookenv_config, mock_charm_dir, tmpdir):
54 sph.sudoers_bypass_path = tmpdir.join(sph.sudoers_bypass_path)54 sph.sudoers_bypass_path = tmpdir.join(sph.sudoers_bypass_path)
55 sph.socket_dir_perms = 0o77555 sph.socket_dir_perms = 0o775
56 sph.sudo_conf_perms = 0o64456 sph.sudo_conf_perms = 0o644
57 sph.pagerduty_path = tmpdir.join(sph.pagerduty_path)
57 return sph58 return sph
diff --git a/src/tests/unit/test_actions.py b/src/tests/unit/test_actions.py
index ddf6003..9c194f3 100644
--- a/src/tests/unit/test_actions.py
+++ b/src/tests/unit/test_actions.py
@@ -12,10 +12,12 @@ import actions # NOQA
12@mock.patch("libsudopair.SudoPairHelper")12@mock.patch("libsudopair.SudoPairHelper")
13@mock.patch("actions.action_set")13@mock.patch("actions.action_set")
14@mock.patch("actions.status_set")14@mock.patch("actions.status_set")
15def test_remove_action(status_set, action_set, sudo_pair_helper):15@mock.patch("actions.subprocess.run")
16def test_remove_action(subprocess_run, status_set, action_set, sudo_pair_helper):
16 """Verify remove action."""17 """Verify remove action."""
17 actions.remove()18 actions.remove()
18 msg = "Successfully removed sudo-pair config and binaries"19 msg = "Successfully removed sudo-pair config and binaries"
19 action_set.assert_called_with({"message": msg})20 action_set.assert_called_with({"message": msg})
20 status_set.assert_called_with("active", msg)21 status_set.assert_called_with("active", msg)
21 sudo_pair_helper().deconfigure.assert_called()22 sudo_pair_helper().deconfigure.assert_called()
23 subprocess_run.assert_called_with(["/usr/local/bin/pagerdutyevent", msg])
diff --git a/src/tests/unit/test_libsudopair.py b/src/tests/unit/test_libsudopair.py
index 860d8db..8d730ad 100644
--- a/src/tests/unit/test_libsudopair.py
+++ b/src/tests/unit/test_libsudopair.py
@@ -144,7 +144,7 @@ class TestSudoPairHelper:
144 def test_render_sudo_approve(self, sph, tmpdir):144 def test_render_sudo_approve(self, sph, tmpdir):
145 """Check that sudo_approve file is rendered correctly."""145 """Check that sudo_approve file is rendered correctly."""
146 # Auto Approve true146 # Auto Approve true
147 expected_content = "echo ${log_line} >> /var/log/sudo_pair.log"147 expected_content = "logger -p auth.warn $log_line"
148 socket_dir = tmpdir.join("/var/run/sudo_pair")148 socket_dir = tmpdir.join("/var/run/sudo_pair")
149 expected_content_socket_dir = 'declare -r SUDO_SOCKET_PATH="{}"'.format(149 expected_content_socket_dir = 'declare -r SUDO_SOCKET_PATH="{}"'.format(
150 socket_dir150 socket_dir

Subscribers

People subscribed via source and target branches