[telemetry] Rotate streaming telemetry secrets. by yozhao101 · Pull Request #9600 · sonic-net/sonic-buildimage

yozhao101 · 2021-12-21T00:38:04Z

Signed-off-by: Yong Zhao yozhao@microsoft.com

Why I did it

Initially the secrets of Streaming Telemetry container were managed and deployed by several internal tools. Since the secrets of Streaming Telemetry were not always be able to be deployed successfully on SONiC production devices, we decide to leverage an exising feature in SONiC to manage and deploy the secrets.

How I did it

I mainly did the following two changes:

First I added a python script named certificate_rotation_checker in Streaming Telemtry container and this script will check periodically in background to see whether the certificate and private key were rotated and updated. The Streaming Telemetry server process will be restarted if and only if the certificate and key were actually rotated. We have another open PR to reload gNMI server configuration in telemetry repo: sonic-net/sonic-telemetry#92.

Second, In the script telemetry.sh, the telemetry server process will be started if and only if the certificate and private key files did exist on the device. Otherwise, it will check the existence of certificate and private key files on device periodically.

How to verify it

I did this implementation and tested them on a virtual switch.

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

…uration file. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

lgtm-com · 2021-12-21T01:13:53Z

This pull request introduces 2 alerts and fixes 3 when merging 90d9d03 into 34be53a - view on LGTM.com

new alerts:

1 for Too few arguments in formatting call
1 for Syntax error

fixed alerts:

2 for Unused local variable
1 for Except block handles 'BaseException'

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

lgtm-com · 2021-12-21T01:50:54Z

This pull request introduces 1 alert and fixes 3 when merging 0bfcec9 into 67e40b5 - view on LGTM.com

new alerts:

1 for Syntax error

fixed alerts:

2 for Unused local variable
1 for Except block handles 'BaseException'

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

qiluo-msft · 2022-01-08T00:09:15Z

dockers/docker-sonic-telemetry/certificate_rollover_checker

+    root_process_pid = os.getppid()
+    syslog.syslog(syslog.LOG_INFO,
+                  "Restarting streaming telemetry service by terminating the process with pid: '{}'".format(root_process_pid))
+    os.kill(root_process_pid, signal.SIGTERM)


SIGTERM

The modern convention is to use sighup (ref: https://stackoverflow.com/a/28327659/2514803 ).

The benefit is not to explicitly terminate the other process and trigger critical process monitor alerts.

You may need to add the sighup handler in sonic-telemetry if not supported.

I think we can probably use the supervisorctl restart telemetry command to only restart the streaming telemetry server process once the secrets were rotated. This can avoid triggerring the critical process alerts.

Restarting is using SIGTERM and SIGKILL internally. One big concern is graceful shutdown. Considering the client will often fetch large amount of data, graceful shutdown will make client easier.

I got the point. We can do this by either sending the signal SIGKILL or executing the command supervisorctl restart telemetry. However, our main focus is how we can do some cleanup before gracefully stopping the telemetry server process and disconnecting with gNMI client side.

Please check my first comment in this thread. You did not get the point of using sighup.

qiluo-msft · 2022-01-08T00:16:57Z

files/build_templates/sonic_debian_extension.j2

+# Install Python module for inotify
+sudo https_proxy=$https_proxy LANG=C chroot $FILESYSTEM_ROOT pip3 install inotify
+
+


Remove extra empty line. #Closed

qiluo-msft · 2022-01-08T00:18:39Z

dockers/docker-sonic-telemetry/certificate_rollover_checker

+
+
+def main():
+    certificate_rollover_check()


certificate_rollover_check

Could you try inotify (https://www.linuxjournal.com/content/linux-filesystem-events-inotify ) instead of reinvent the wheel? #Closed

qiluo-msft

As comments

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

…te and private key. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

lgtm-com · 2022-01-10T20:15:03Z

This pull request introduces 1 alert when merging 17c4d60 into 58c5bb6 - view on LGTM.com

new alerts:

1 for Unused import

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

yozhao101 · 2022-01-10T23:26:55Z

/azp run

azure-pipelines · 2022-01-10T23:27:00Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

yozhao101 · 2022-01-11T00:07:53Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2022-01-11T00:08:06Z

Azure Pipelines successfully started running 1 pipeline(s).

qiluo-msft · 2022-01-11T07:31:01Z

dockers/docker-sonic-telemetry/certificate_rotation_checker

+                          "Certificate and private key were rotated and restarting telemetry server process ...")
+            restart_telemetry_server()
+
+            # Wait for specified seconds and then do the next round checking


Wait for specified second

You don't need sleep in an event loop. #Resolved

Initially what I am thinking is since the directory /etc/sonic/credentials/ will store all the certificates and keys for different applications in the future and if one or several certificates under this directory are rotated too frequently (by accident?), this event loop will be executed continuously and then burn too much CPU cycles potentially. Another thinking the polling frequency of our internal tool to get the update from certificate storage is 8 ~ 24 hours. That's why I put a sleep function at here.

I will let inotify to monitor the certificate file directly instead of monitoring the /etc/sonic/credentials/ and then we can remove this sleep function in event loop.

Got your motivation, and the considering may be helpful. However, event you sleep, the event will be queued and processed after each sleep, you don't save too much?

Agree and this will not save too much!

qiluo-msft · 2022-01-11T07:36:46Z

dockers/docker-sonic-telemetry/certificate_rotation_checker

+    inotify_instance.add_watch(CREDENTIALS_DIR_PATH)
+    for event in inotify_instance.event_gen(yield_nones=False):
+        header, event_type, monitoring_path, file_name = event
+        if (file_name == certificate_file_name


if

The FSM logic is complex and may be messed up by some input sequence. Could you use one file as the main indicator, and always rotate if that file changed.

ref: https://github.com/Azure/sonic-restapi/blob/94805a39ac0712219f7dc08faa2cfdbf371dd177/go-server-server/main.go#L103 #Closed

Updated and use the rotation of certificate file as the main indicator.

Please make sure describe the main file in document? This is very critical design assumption and the cert rotator should treat it as a contract.

Updated the design document.

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

qiluo-msft · 2022-01-14T00:21:59Z

dockers/docker-sonic-telemetry/certificate_rotation_checker

+
+    inotify_instance = inotify.adapters.Inotify()
+    inotify_instance.add_watch(CREDENTIALS_DIR_PATH)
+    for event in inotify_instance.event_gen(yield_nones=False):


inotify_instance

In extreme case, the file is deleted by a malicious user, will the inotify_instance still working? I think its link to inode, and deleting file will destroy the inode.

If this is true, a crash is better than a dead loop.

The inotify is inode based and it will monitor the credentials directory /etc/sonic/credentials/ to see whether the telemetry certificate file was rotated or not. If certificate file was deleted by accidentally, the inotify_instance will not be impacted.

I updated the PR to log an error message if the certificate was deleted. What I am thinking is if the certificate was restored later, then it can be treated as a kind of rotation operation and the telemetry server will be restarted by this script.

If certificate file was deleted by accidentally, what is the expected behavior?

I am considering in this case, we can kill telemetry daemon.

qiluo-msft · 2022-01-14T00:25:04Z

dockers/docker-sonic-telemetry/certificate_rotation_checker

+                      "Failed to retrieve the certificate information from 'TELEMETRY' table!")
+        sys.exit(4)
+
+    while True:


while True:

The intention of this loop is to wait until two files exist. why only check one file? #Closed

Updated and checks the existence of both two files.

…sted on device and log an error message if certificate file was deleted accidentally. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

…P` signal. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

lgtm-com · 2022-01-25T22:45:17Z

This pull request introduces 1 alert when merging c6e2fdd into 61e9a76 - view on LGTM.com

new alerts:

1 for Variable defined multiple times

…ing script. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

lgtm-com · 2022-01-26T19:36:19Z

This pull request introduces 1 alert when merging 41377a1 into 1ac140a - view on LGTM.com

new alerts:

1 for Variable defined multiple times

liat-grozovik · 2022-03-15T12:26:40Z

@yozhao101 could you please make sure your PR is rebased to latest and rerun checkers?
this fix is needed for 202111 and we appreciate if it can be ready for signoff.

liat-grozovik · 2022-04-05T11:22:29Z

@yozhao101 could you please rebase your work on latest and rerun checkers?
@qiluo-msft could you review?

yozhao101 · 2022-05-31T18:42:06Z

@qiluo-msft Can you help me review this PR please?

yozhao101 · 2022-05-31T19:15:07Z

@yozhao101 could you please rebase your work on latest and rerun checkers? @qiluo-msft could you review?

Thanks @liat-grozovik for your reviewing! I will rebase my PR.

qiluo-msft · 2022-06-02T23:08:30Z

dockers/docker-sonic-telemetry/certificate_rotation_checker

+
+    while True:
+        if not os.path.exists(certificate_file_path) or not os.path.exists(private_key_file_path):
+            syslog.syslog(syslog.LOG_ERR,


LOG_ERR

Instead of error message and keep loop, it is better to just simply kill telemetry daemon.

liat-grozovik · 2022-07-28T08:35:43Z

@yozhao101 any update on this PR? should it go to 202205? if so, can you add the proper label?

zhangyanzhao · 2022-08-03T15:51:52Z

@yozhao101 can you please help to response the comments and provide an update on this PR? Thanks.

jeflo · 2023-07-19T19:24:12Z

Hi @yozhao101 this looks like a very useful change. Would like to know what is the status of this PR and what's the plan to get it to master? Thanks.

yozhao101 added 2 commits December 20, 2021 16:28

[telemetry] Roll over streaming telemetry secrets by ACMS.

a921b8e

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Fix the indent in the script telemetry.sh.

7da4005

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

yozhao101 changed the title ~~[telemetry] Roll over streaming telemetry secrets by ACMS.~~ [telemetry] Roll over streaming telemetry secrets. Dec 21, 2021

yozhao101 added 3 commits December 20, 2021 16:44

[telemetry] Fix the indent issue in the script telemetry.sh.

ede36b9

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Add the field of buffer_size=1024 in supervisord config…

7c3c7d3

…uration file. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Fix the indent issue.

90d9d03

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Fix a syntax error.

0bfcec9

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Fix a syntax error.

9db5b04

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

lguohan requested a review from qiluo-msft January 5, 2022 18:52

yozhao101 added 2 commits January 6, 2022 12:59

[telemetry] Install inotify module.

b1844b7

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Install the inotify.

e28f64d

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

qiluo-msft reviewed Jan 8, 2022

View reviewed changes

qiluo-msft requested changes Jan 8, 2022

View reviewed changes

yozhao101 added 2 commits January 9, 2022 17:01

[telemetry] Install the inotify in telemetry container.

536fc4c

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Use inotify module to monitor the rotation of certifica…

17c4d60

…te and private key. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

yozhao101 marked this pull request as ready for review January 10, 2022 19:52

yozhao101 requested a review from lguohan as a code owner January 10, 2022 19:52

yozhao101 changed the title ~~[telemetry] Roll over streaming telemetry secrets.~~ [telemetry] Rotate streaming telemetry secrets. Jan 10, 2022

[telemetry] Remove the unused import from script.

59dfe9b

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

qiluo-msft reviewed Jan 11, 2022

View reviewed changes

yozhao101 added 4 commits January 12, 2022 13:47

[telemetry] Use the rotation of certificate file as an indicator.

ce9f713

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Change the script to be executable.

68bd0b6

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Remove the unused import.

6ad0ef5

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Change the script name.

6ce4e4d

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

qiluo-msft reviewed Jan 14, 2022

View reviewed changes

yozhao101 added 2 commits January 17, 2022 17:35

[telemetry] Checks whether both certificate and private key files exi…

6512a6f

…sted on device and log an error message if certificate file was deleted accidentally. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Reload telemetry server configuration by sending a `SIGHU…

c6e2fdd

…P` signal. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

yozhao101 added 3 commits January 26, 2022 10:46

[telemetry] Fix the undefinied fields issue in telemetry server start…

3dbb79d

…ing script. Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Fix the indentation.

0881704

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

[telemetry] Fix the indentation issue.

41377a1

Signed-off-by: Yong Zhao <yozhao@microsoft.com>

yozhao101 mentioned this pull request Feb 2, 2022

[telemetry] Reload telemetry server configuration. sonic-net/sonic-telemetry#92

Open

yozhao101 mentioned this pull request Feb 17, 2022

[telemetry] telemetry.sh script fails to handle missing parameter in config DB and docker does not start #8959

Open

qiluo-msft reviewed Jun 2, 2022

View reviewed changes

yozhao101 requested a review from a team as a code owner June 10, 2022 02:01

		# Install Python module for inotify
		sudo https_proxy=$https_proxy LANG=C chroot $FILESYSTEM_ROOT pip3 install inotify

Conversation

yozhao101 commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why I did it

How I did it

How to verify it

Which release branch to backport (provide reason below if selected)

Description for the changelog

A picture of a cute animal (not mandatory but encouraged)

Uh oh!

lgtm-com bot commented Dec 21, 2021

Uh oh!

lgtm-com bot commented Dec 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yozhao101 Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Jan 10, 2022

Uh oh!

yozhao101 commented Jan 10, 2022

Uh oh!

azure-pipelines bot commented Jan 10, 2022

Uh oh!

yozhao101 commented Jan 11, 2022

Uh oh!

azure-pipelines bot commented Jan 11, 2022

Uh oh!

qiluo-msft Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

yozhao101 commented Dec 21, 2021 •

edited

Loading

yozhao101 Jan 11, 2022 •

edited

Loading

qiluo-msft Jan 8, 2022 •

edited

Loading

qiluo-msft Jan 8, 2022 •

edited

Loading

qiluo-msft Jan 11, 2022 •

edited

Loading

qiluo-msft Jan 11, 2022 •

edited

Loading

qiluo-msft Jan 14, 2022 •

edited

Loading