Adding support of common security cipher module for encryption and decryption of a passkey #17201
Conversation
…nto security_cipher
| self._config_db.connect() | ||
| # Note: Kept 1st index NA intentionally to map it with the cipher_pass file | ||
| # contents. The file has a comment at the 1st row / line | ||
| self._feature_list = ["NA", "TACPLUS", "RADIUS", "LDAP"] |
There was a problem hiding this comment.
Do we need a master key per feature? I thought we're using one master key for all features.
There was a problem hiding this comment.
I guess it is better to have different Master keys for different feature. This way, there will not be any inter-dependency between them.
There was a problem hiding this comment.
Let's not complicate things for the user, just follow well established practices (e.g a master key for all features) from the popular NOS if possible, my 2 cents.
There was a problem hiding this comment.
With this approach, we can further extend this implementation for other modules too not just TACPLUS, RADIUS and LDAP. Additionally, it is upto the user if he / she needs to use different keys or can use same keys for all.
There was a problem hiding this comment.
It will give flexibility to the user, in my opinion.
There was a problem hiding this comment.
If we go with single key, it will be having huge dependancy in case of changing that key. User needs to change the encrypted passkey in CONFIG_DB for all the features.
There was a problem hiding this comment.
I disagree, IMO, we should just look at the behavior of other popular NOS that's been there for many years, if required for any SONiC use-case, we can think about providing the flexibility in the future.
There was a problem hiding this comment.
With this approach, we are already providing the flexibility to the user (why to wait for the future :-)). They can have either same or different keys for different features.
The proprietary NOSes have different architectures and they have implemented the feature which can be fitted into their infrastructure. :-) I guess, it is always better if we can add bit of flexibility into any of the designs.
|
@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. |
|
@venkatmahalingam Once this PR is merged, build will get pass of both the following PRs. PR: TACACS passkey encryption Part - I |
@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, 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. |
|
@venkatmahalingam @avinothcgl @venkatmahalingam shouldn't it be enough to solve the migration use case? please suggest to move forward with this PR? For instance, 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. |
|
Reviewers, if you are ok with this PR, please help to approve it. Thanks. |
…e in case encryption is disabled
|
@venkatmahalingam As requested, updated the class name as "master_key_mgr". |
|
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. |
|
@lguohan Can you please help in approving / merging this PR. |
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.
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