Adding support of common security cipher module for encryption and decryption of a passkey #17201

nmoray · 2023-11-16T16:54:11Z

Why I did it

This module is created to handle the passkey encryption, decryption and the cipher storage. It's a common module which will be used for feature like TACACS, RADIUS, LDAP etc.
This implementation is aligned with HLD

How I did it

This module will expose following APIs.

Passkey encryption for a given feature
Passkey decryption for a given feature
Check whether encryption feature is enabled for a given feature

How to verify it

Cipher / password used for the encryption:
root@sonic:/tmp# cat /etc/cipher_pass 
#Auto generated file for storing the encryption passwords
TACPLUS : TEST1
RADIUS : TEST2
LDAP : TEST3


Is encryption enabled for TACACS: False
Encrypted passkey for Feature: TACPLUS - U2FsdGVkX1/frdwl4GGD7bTKyzLi+lr2K76v0IECzkQ=
Passkey post decryption:TACPLUS - passkey1
Encrypted passkey for Feature: RADIUS - U2FsdGVkX1/fdiBo3RWWxIIPFJYCy1CF/ZQeLt8N96Q=
Passkey post decryption: RADIUS - passkey2
Encrypted passkey for Feature: LDAP - U2FsdGVkX1/o0UkHtWgOjr46UzLQRhXKAHngctey9TE=
Passkey post decryption: LDAP - passkey3

`

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

Tested branch (Please provide the tested image version)

Master

Link to config_db schema for YANG module changes

Schema

Related PRs:

Part I
Part II

src/sonic-py-common/sonic_py_common/security_cipher.py

src/sonic-py-common/tests/test_security_cipher.py

madhupalu

Changes looks good me except few minor comments. please address it.

nmoray · 2023-11-24T02:43:55Z

@venkatmahalingam I have added the AUT too. Can you please approve the PR now? This is an independent module. It doesn't have any impact on any other functionalities. Thus we can merge this into master. I will work on other PRs parallelly to integrate this module into TACACS feature.

nmoray · 2023-11-24T04:20:13Z

@venkatmahalingam Once this PR is merged, build will get pass of both the following PRs.

PR: TACACS passkey encryption Part - I
PR: TACACS passkey encryption Part - II

venkatmahalingam

Please address the outstanding comments.

nmoray · 2023-11-28T06:39:49Z

Please address the outstanding comments.

@venkatmahalingam I have replied to the comments. IMO, Flexible design is a good approach. Why to restrict user to go on a certain path only. He / she should have multiple options.

avinothcgl · 2023-11-29T19:09:46Z

Please address the outstanding comments.

@venkatmahalingam I have replied to the comments. IMO, Flexible design is a good approach. Why to restrict user to go on a certain path only. He / she should have multiple options.

@nmoray @venkatmahalingam giving multiple option is good but also customers who migrates from other well known NOS to SONiC will be using one master key. To accommodate those customers , Can you include one master key option as well.

nmoray · 2023-12-01T08:00:19Z

Please address the outstanding comments.

@venkatmahalingam I have replied to the comments. IMO, Flexible design is a good approach. Why to restrict user to go on a certain path only. He / she should have multiple options.

@nmoray @venkatmahalingam giving multiple option is good but also customers who migrates from other well known NOS to SONiC will be using one master key. To accommodate those customers , Can you include one master key option as well.

@avinothcgl Using one master key for all the feature is still possible with the current implementation. Just keep same master key for all in cipher_pass file.

For instance,
"#Auto generated file for storing the encryption passwords"
TACPLUS : <SAME_MASTER_KEY>
RADIUS : <SAME_MASTER_KEY>
LDAP : <SAME_MASTER_KEY>

One more advantage of this design is, same security_cipher module can be extended to other applications as well which need to make use of encryption / decryption. That application can have it's own master_key which will be different from TACPLUS, RADIUS and LDAP.

madhupalu · 2023-12-05T23:16:33Z

@venkatmahalingam @avinothcgl
Thanks bringing up migration use case (Migrating NON-SONiC to SONiC).
With the current implementation, using single master key for all the feature is still quite possible. the network admin Just keep same master key for all in cipher_pass file.

@venkatmahalingam shouldn't it be enough to solve the migration use case? please suggest to move forward with this PR?

@zhangyanzhao

For instance,
"#Auto generated file for storing the encryption passwords"
TACPLUS : <SAME_MASTER_KEY>
RADIUS : <SAME_MASTER_KEY>
LDAP : <SAME_MASTER_KEY>

One more advantage of this design is, same security_cipher module can be extended to other applications as well which need to make use of encryption / decryption. That application can have it's own master_key which will be different from TACPLUS, RADIUS and LDAP.

zhangyanzhao · 2023-12-12T16:37:31Z

Reviewers, if you are ok with this PR, please help to approve it. Thanks.

nmoray · 2024-04-12T03:58:59Z

@venkatmahalingam As requested, updated the class name as "master_key_mgr".

brholmes1 · 2024-05-29T17:05:30Z

This is a great feature! In addition to the TACPLUS PRs already proposed, I would also recommend that the RADIUS keys (partially implemented per this PR) and SNMPv3 user passwords/privs also have the ability to be encrypted to align with industry security standards. If a new issue needs to be opened for tracking of RADIUS/SNMPv3 components let me know and I can submit them.

nmoray · 2024-05-30T06:11:23Z

This is a great feature! In addition to the TACPLUS PRs already proposed, I would also recommend that the RADIUS keys (partially implemented per this PR) and SNMPv3 user passwords/privs also have the ability to be encrypted to align with industry security standards. If a new issue needs to be opened for tracking of RADIUS/SNMPv3 components let me know and I can submit them.

Thanks @brholmes1. IMO, let's first merge the initial infra. Later, we can easily extend the support for other modules. For tracking purpose, please open the new issues in the community.

nmoray · 2024-05-30T06:14:02Z

@lguohan Can you please help in approving / merging this PR.

nmoray · 2025-01-23T05:40:43Z

Folks please help in merging the PR if there are no more comments. It is open from more than 6 months now. Please help in merging the PR.
cc: @zhangyanzhao @qiluo-msft @madhupalu

src/sonic-py-common/sonic_py_common/security_cipher.py

anders-nexthop · 2025-01-22T17:41:46Z

src/sonic-py-common/sonic_py_common/security_cipher.py

+            if not os.path.exists(self._file_path):
+                with open(self._file_path, 'w') as file:
+                    file.writelines("#Auto generated file for storing the encryption passwords\n")
+                    file.writelines("TACPLUS : \nRADIUS : \nLDAP :\n")


It's probably worth generating this file from the features in _feature_list. that way if we want to add a new feature we just add a list entry instead of having to make changes here.

src/sonic-py-common/sonic_py_common/security_cipher.py

anders-nexthop · 2025-01-22T18:07:56Z

src/sonic-py-common/sonic_py_common/security_cipher.py

+                return data[key]
+        return False
+
+    def del_cipher_pass(self):


Looking at how this is used in later commits, I think this is incorrect. We should not be removing the entire file, but instead just changing/deleting the entry. If I have both TACACS and RADIUS configured and i remove the passkey for one of them, I still want the other one to work.

I will update the method.

src/sonic-py-common/tests/test_security_cipher.py

src/sonic-yang-models/yang-models/sonic-system-tacacs.yang

mssonicbld · 2025-03-10T13:29:24Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2025-03-10T13:29:30Z

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

nmoray · 2025-03-10T13:33:09Z

@qiluo-msft @anders-nexthop I have addressed the comments.

nmoray · 2025-03-10T15:32:14Z

/azp run Azure.sonic-buildimage

…-r-----)

mssonicbld · 2025-05-20T05:53:48Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2025-05-20T05:53:59Z

Azure Pipelines successfully started running 1 pipeline(s).

nmoray · 2025-05-20T05:57:40Z

hat to re-trigger the tests to make sure it's not an

@anders-nexthop you mean just rebase the

@qiluo-msft Will you please help me understand the test failures? Why the kvmtest-* failed? BTW I have fixed the Semgrep errors.

It looks like unrelated failures in test setup code to me, but @qiluo-msft will know more.
From ElasticTest page:
Sanity check failed for vms-kvm-t1-lag on running test_pretest.py|||vms-kvm-t1-lag_399220
I looked through and saw a few failures that were ignored (NTP, a bad key "get_testbed_info\nKeyError: 'vms-kvm-t1-lag'\n", and an ACL one that's seemingly always there). I don't see how any of these would be related to your change.
You could try pulling the latest from master branch and pushing that to re-trigger the tests to make sure it's not an intermittent issue.
I tried to re-trigger the failed tests manually and got this error:

TF400813: The user 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa' is not authorized to access this resource.

@anders-nexthop I just rebased my remote branch and re-pushed it. Let's see if it makes any difference.
@qiluo-msft BTW, In case these failures are known ones, will you please help in merging the changes.

nmoray · 2025-05-20T14:32:20Z

hat to re-trigger the tests to make sure it's not an

@anders-nexthop you mean just rebase the
@qiluo-msft Will you please help me understand the test failures? Why the kvmtest-* failed? BTW I have fixed the Semgrep errors.

It looks like unrelated failures in test setup code to me, but @qiluo-msft will know more.
From ElasticTest page:
Sanity check failed for vms-kvm-t1-lag on running test_pretest.py|||vms-kvm-t1-lag_399220
I looked through and saw a few failures that were ignored (NTP, a bad key "get_testbed_info\nKeyError: 'vms-kvm-t1-lag'\n", and an ACL one that's seemingly always there). I don't see how any of these would be related to your change.
You could try pulling the latest from master branch and pushing that to re-trigger the tests to make sure it's not an intermittent issue.
I tried to re-trigger the failed tests manually and got this error:
TF400813: The user 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa' is not authorized to access this resource.
@anders-nexthop I just rebased my remote branch and re-pushed it. Let's see if it makes any difference. @qiluo-msft BTW, In case these failures are known ones, will you please help in merging the changes.

@qiluo-msft The checks are still failing. Please let us know what needs to be done to fix these failures.

src/sonic-py-common/tests/test_security_cipher.py

mssonicbld · 2025-05-22T09:15:51Z

/azp run Azure.sonic-buildimage

azure-pipelines · 2025-05-22T09:16:03Z

Azure Pipelines successfully started running 1 pipeline(s).

nmoray · 2025-05-22T09:18:23Z

@qiluo-msft Please re-approve the PR. I pushed one more commit for removing commented code.

nmoray · 2025-05-23T04:24:10Z

@qiluo-msft can you please re-approve and merge the PR. All the checks have been passed now.

…cryption of a passkey (sonic-net#17201) Why I did it This module is created to handle the passkey encryption, decryption and the cipher storage. It's a common module which will be used for feature like TACACS, RADIUS, LDAP etc. This implementation is aligned with HLD How I did it This module will expose following APIs. Passkey encryption for a given feature Passkey decryption for a given feature Check whether encryption feature is enabled for a given feature How to verify it Cipher / password used for the encryption: root@sonic:/tmp# cat /etc/cipher_pass #Auto generated file for storing the encryption passwords TACPLUS : TEST1 RADIUS : TEST2 LDAP : TEST3 Is encryption enabled for TACACS: False Encrypted passkey for Feature: TACPLUS - U2FsdGVkX1/frdwl4GGD7bTKyzLi+lr2K76v0IECzkQ= Passkey post decryption:TACPLUS - passkey1 Encrypted passkey for Feature: RADIUS - U2FsdGVkX1/fdiBo3RWWxIIPFJYCy1CF/ZQeLt8N96Q= Passkey post decryption: RADIUS - passkey2 Encrypted passkey for Feature: LDAP - U2FsdGVkX1/o0UkHtWgOjr46UzLQRhXKAHngctey9TE= Passkey post decryption: LDAP - passkey3

qiluo-msft · 2025-05-25T06:54:09Z

src/sonic-yang-models/yang-models/sonic-system-tacacs.yang

                }

+		leaf key_encrypt {
+		    type boolean;


boolean

Please add a default value (false). This is needed for backward compatible.

Sure. I will raise a new PR to accomodate this change.

qiluo-msft · 2025-05-25T07:07:13Z

src/sonic-yang-models/yang-models/sonic-system-tacacs.yang

                    default 5;
                }

+		leaf key_encrypt {


Mix tabs with spaces, please reformat this "leaf key_encrypt" section.

You mean, replace the tabs with spaces for the entire leaf section?

@qiluo-msft The link that you shared is not accessible.

I removed the link. Yes, I mean to replace the tabs with spaces for the entire leaf section

Sure, will do as a part of new PR.

qiluo-msft · 2025-05-25T07:19:03Z

src/sonic-py-common/sonic_py_common/security_cipher.py

+
+    def __init__(self):
+        if not self._initialized:
+            self._file_path = "/etc/cipher_pass"


/etc/cipher_pass

When you boot into a new version, the /etc/config_db.json will be carried over, but this file will be lost. Then you have only encrypted passkey and lost cipher_pass. How does the new version survive?

@qiluo-msft As per https://github.com/sonic-net/SONiC/blob/master/doc/ztp/SONiC-config-setup.md#222-config-migration
the backup and restore of cipher_pass file will automatically be taken care, right?

Thanks for the link! The file "/etc/cipher_pass" is outside the folder of config migration design, could you check again?

oh yes. Okay then, I will create another PR to add pre-hook and post-hook scripts to backup and restore the cipher_pass file during the up-gradation. Is that okay? Or just change the location to "/etc/sonic"?

nmoray mentioned this pull request Nov 16, 2023

TACACSPLUS_PASSKEY_ENCRYPTION.md sonic-net/SONiC#1471

Merged

venkatmahalingam reviewed Nov 16, 2023

View reviewed changes

src/sonic-py-common/sonic_py_common/security_cipher.py Show resolved Hide resolved

venkatmahalingam reviewed Nov 16, 2023

View reviewed changes

src/sonic-py-common/sonic_py_common/security_cipher.py Outdated Show resolved Hide resolved

venkatmahalingam reviewed Nov 16, 2023

View reviewed changes

src/sonic-py-common/sonic_py_common/security_cipher.py Show resolved Hide resolved

venkatmahalingam reviewed Nov 16, 2023

View reviewed changes

src/sonic-py-common/sonic_py_common/security_cipher.py Show resolved Hide resolved

madhupalu reviewed Nov 16, 2023

View reviewed changes

madhupalu reviewed Nov 23, 2023

View reviewed changes

src/sonic-py-common/tests/test_security_cipher.py Outdated Show resolved Hide resolved

src/sonic-py-common/tests/test_security_cipher.py Outdated Show resolved Hide resolved

src/sonic-py-common/tests/test_security_cipher.py Outdated Show resolved Hide resolved

madhupalu approved these changes Nov 23, 2023

View reviewed changes

nmoray requested a review from qiluo-msft as a code owner November 24, 2023 11:36

venkatmahalingam suggested changes Nov 28, 2023

View reviewed changes

nmoray requested a review from venkatmahalingam December 4, 2023 13:31

nmoray mentioned this pull request Dec 12, 2023

TACACS Passkey encryption feature sonic-net/SONiC#1407

Open

anders-nexthop reviewed Jan 23, 2025

View reviewed changes

anders-nexthop mentioned this pull request Jan 23, 2025

TACACS+ passkey encryption #13846

Open

nmoray added 7 commits May 20, 2025 05:47

Change the access permission of cipher_pass file from 644 to 640 (-rw…

8268021

…-r-----)

updated scripts to mock chmod

2fbec21

Removed unwated debugs

95a4c10

renamed class name to master_key_mgr

719053b

Addressed new comments

1ab7067

Fixed semgrep findings

5a94a62

updated tests as per the new changes

33e88cb

nmoray force-pushed the security_cipher branch from 586d076 to 33e88cb Compare May 20, 2025 05:53

anders-nexthop reviewed May 22, 2025

View reviewed changes

src/sonic-py-common/tests/test_security_cipher.py Outdated Show resolved Hide resolved

Removed commented code

f388f71

nmoray dismissed qiluo-msft’s stale review via f388f71 May 22, 2025 09:15

nmoray requested a review from qiluo-msft May 22, 2025 09:16

venkatmahalingam approved these changes May 22, 2025

View reviewed changes

qiluo-msft approved these changes May 23, 2025

View reviewed changes

qiluo-msft merged commit bce7621 into sonic-net:master May 23, 2025
19 checks passed

qiluo-msft reviewed May 25, 2025

View reviewed changes

nmoray mentioned this pull request May 27, 2025

Security Cipher Module: Added password rotate feature and enabled Backup and Restore support for NOS Upgrades #22711

Open

8 tasks

This was referenced Jun 25, 2025

Integrated Security Cipher for TACPLUS passkey encryption sonic-net/sonic-utilities#3936

Open

Integrated Security Cipher module with Hostcfgd for passkey decryption sonic-net/sonic-host-services#274

Open

Adding support of common security cipher module for encryption and decryption of a passkey #17201

Adding support of common security cipher module for encryption and decryption of a passkey #17201

Conversation

nmoray commented Nov 16, 2023 • 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)

Tested branch (Please provide the tested image version)

Link to config_db schema for YANG module changes

Related PRs:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madhupalu left a comment

Choose a reason for hiding this comment

Uh oh!

nmoray commented Nov 24, 2023

Uh oh!

nmoray commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

venkatmahalingam left a comment

Choose a reason for hiding this comment

Uh oh!

nmoray commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avinothcgl commented Nov 29, 2023

Uh oh!

nmoray commented Dec 1, 2023

Uh oh!

madhupalu commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhangyanzhao commented Dec 12, 2023

Uh oh!

nmoray commented Apr 12, 2024

Uh oh!

brholmes1 commented May 29, 2024

Uh oh!

nmoray commented May 30, 2024

Uh oh!

nmoray commented May 30, 2024

Uh oh!

nmoray commented Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

mssonicbld commented Mar 10, 2025

Uh oh!

azure-pipelines bot commented Mar 10, 2025

Uh oh!

nmoray commented Nov 16, 2023 •

edited

Loading

nmoray commented Nov 24, 2023 •

edited

Loading

nmoray commented Nov 28, 2023 •

edited

Loading

madhupalu commented Dec 5, 2023 •

edited

Loading

nmoray commented Jan 23, 2025 •

edited

Loading

qiluo-msft May 25, 2025 •

edited

Loading

nmoray May 26, 2025 •

edited

Loading