-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TACACSPLUS_PASSKEY_ENCRYPTION.md #1471
Conversation
doc/TACACSPLUS_PASSKEY_ENCRYPTION.md
Outdated
The implementation stands on three key pillars. | ||
1. OPENSSL toolkit is used for encryption / decryption | ||
2. aes-128-cbc is the encoding format used for encryption / decryption | ||
3. Device MAC address is used as a Password for encryption / decryption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can easily decrypt and know the actual password if we know the device MAC address, right? any option for user to configure the password other than MAC-address?
Also, if I want to use the config_db (encrypted passkey) generated on one system to another (may be we forced to replace the failed node with same config), hope passkey cant be used on another device, what procedure should be followed for this use-case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @venkatmahalingam you are right. We can decrypt the passkey only if we knew the system MAC address. And if we allow the user to use any custom password then it will be bit difficult in terms of maintainability.
For instance, either we need to use some fixed cipher for encryption / decryption which will be more susceptible to the overall network breach. Alternatively, we need to save the password somewhere (for instance in redis). This is the reason I chosen system MAC as a password which will make every encrypted passkey unique within the network.
An encrypted passkey on one node will not be applicable or useable on another as hostcfgd will not be able to decrypt that passkey. To handle this sort of scenario, the Orchestrator / network admin has to create the config_db.json or the encrypted passkey for every node (considering the orchestrator have the list of system MAC of all the nodes).
Let's say for some debugging scenario, we need to use same config_db.json on another device, we can simply re-generate the encrypted passkey on the device where we have copied the config_db.json by running "config tacacs passkey <actual_passkey_in_plaintext>" command.
Adding @Yarden-Z as reviewer who is interested in this feature. @venkatmahalingam @Yarden-Z thanks for reviewing and please let us know we will work on the code PRs. |
|
||
### Requirements | ||
|
||
The primary objective of this feature is to safeguard the TACACS passkey, which is stored in its plaintext format within CONFIG_DB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the section for DB migrator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the YANG change for the new TACACS passkey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@venkatmahalingam passkey is already part of yang model, no new schema introduced part of HLD - https://github.com/sonic-net/sonic-buildimage/blob/master/src/sonic-yang-models/yang-models/sonic-system-tacacs.yang
Only the backend will use system mac as unique salt to encrypt/decrypt the passkey.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need DB migrator as no change to the schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about existing plaintext passkey to encrypted key migration in the config-db during image upgrade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@venkatmahalingam The current yang model description already reflect the change you requested - ' description "Shared secret used for encrypting the communication";' Is the sufficient or do we think still need an update?
leaf passkey {
type string {
length "1..65";
pattern "[^ #,]*" {
error-message 'TACACS shared secret (Valid chars are ASCII printable except SPACE, "#", and ",")';
}
}
description "Shared secret used for encrypting the communication";
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@venkatmahalingam is sonic-system-tacacs.yang a pull model or push? If we need to configure the passkey via this model, we don't need any change as it will still be in a plaintext format only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmoray , I think the length part need change, because when you encrypt a text to binary then convert to text, the length will usually be much longer, currently sonic already enable yang validation, if the length not change, yang validation will failed with some TACACS key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuh-80 one question related to sonic-system-tacacs.yang model. Is it being used for pushing the TACACS configs or pulling the telemetry data related to TACACS or both?
And in case of increasing the length of passkey leaf, do I need to make any other backward compatibility changes or nothing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sonic-system-tacacs.yang is used for push TACACS config, also when change TACACS config on device with CLI, the model also will be used for validation.
Increase the length of passkey is backward compatibility. No need other change.
|
||
show tacacs | ||
["TACPLUS global passkey configured Yes / No"] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a section for limitations/restrictions, mention that if the MAC address of the device is known, we could decrypt the key and add some thoughts for the future work if you are not planning to fix in the initial release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed. Added a section to the HLD describing the limitations and release plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@venkatmahalingam the reason behind choosing system MAC as the encryption salt is, it will be known only to the network admin. Additionally, it will be readily accessible within the Redis so, can be pulled in different modules without hardcoding anything.
Moreover, we could consider other alternate salt too but again it will be an overhead of creating that uniquely for every node and also maintaining the same some where so that it can be consumed during the orchestration process.
Updated HLD section with DB migration and limitations.
The idea of encrypting the TACACS+ shared secret before it is written to the configuration (CONFIG_DB) is good. However, the drawbacks are: (one of more of these might already be mentioned in the limitations)
|
@a-barboza the replies are inline. |
[a-barboza] So, if the system MAC address is used as the salt, what is the key used for encryption ? IMHO, the salt has to be unique or pseudo-random (even if it is not secret). [a-barboza] Even if the encrypted passkey is unique for every node, how can it be changed by the admin? Most admins will not like to have their passwords assigned to them. They would rather choose their passwords. [a-barboza] ok, so the workaround is to use a CLI in a script to set the TACACS+ encrypted passkeys. What's the workaround if the admin would like to generate the encrypted passkeys remotely and just give a pre-cooked CONFIG_DB json for configuration? [a-barboza] The Implementation details mentions "A unique Device MAC address used as a salt/password to encrypt/decrypt the configured pass key". So, it is not clear if the device MAC address is used as the salt (initialization vector), key, or both. So, if there are multiple TACACS+ passkeys, will the same salt be used for all of them? |
[nmoray] Yes, the salt will be system MAC address and passkey will be any user defined secret string. This combination always produces an unique encrypted passkey. [nmoray] As I mentioned previously, network admin will be able to produce different encrypted unique passkey. They will have two options or I can say two combinations.
[nmoray] The network admin certainly be able to generate the encrypted passkey remotely as the admin knows both, plaintext passkey + system MAC of that device. Thus, the encrypted passkey generation would be pretty simple. Alternatively, the admin can use the previously generated encrypted key for that device and pre-cook it with CONFIG_DB json. Both the options will allow the network admin to login successfully to that respective device. [nmoray] First of all, multiple TACACS+ passkey support in currently not there in SONIC. Furthermore, in practice, the encryption mechanism will appear as follows.
|
[a-barboza] the python snippet helps answer the mechanism of encryption. |
@a-barboza Okay, Are you suggesting that we should maintain a configurable password, ensuring that the same password is utilised for generating the encrypted passkey across the entire fleet? If this is indeed the situation, decoding a single node could lead to the compromise of all other nodes. BTW, this mechanism may not be limited to TACACS and could be applied to other protocols as well, such as Radius. :-) |
doc/TACACSPLUS_PASSKEY_ENCRYPTION.md
Outdated
The implementation stands on three key pillars. | ||
1. OPENSSL toolkit is used for encryption / decryption | ||
2. aes-128-cbc is the encoding format used for encryption / decryption | ||
3. A unique Device MAC address used as a salt/password to encrypt/decrypt the configured pass key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the key will only be the MAC address of the switch? If so - this is not a strong encryption (as it is not a random password or something else).
What if we do not want it to be the MAC address, is there an option to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have considered system MAC address as a password for encrypting the user entered plaintext passkey. Network admin can change the passkey but not the password (which is system MAC). With this combination, OPENSSL is able to generate the random encrypted hash / passkey.
If we provide a configurable password, we need to store that somewhere (for instance redis which will again defeat the purpose of safe guarding the secret). Additionally, we need to generate unique password for every node within the network which will be a cumbersome job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the key is not something that is saved on top of the system, but rather a manipulation of the device MAC.
This means the the encryption key consists only of the system MAC (this means that anyone with network proximity to the device can figure out this key) and therefor we don't need to save anything additionally from this feature on top of the system (as device MAC already has a stored location).
Question - isn't using the system MAC (which is exposed via network proximity) not a risk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say, the encrypted key is essentially the manipulation of a passkey done using the system MAC address. And yes, there is an obvious risk if an intruder is having physical network proximity to the device. :-)
Alternatively, we can combine the system MAC with the plaintext user entered passkey and will use that as a password for the encryption.
Lastly, we can certainly consider a user configurable password similar to the TACACS passkey. But then where are we going to store it? and how are we going to create a unique password for all the devices within the network?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are exactly my questions here :)
If we are going to encrypt this passphrase - what are we trying to protect ourselves against? If it's a user without privileges reading the passkey, then this won't suffice (as this user can review the MAC address).
If it's against someone getting the redis dump and figuring out the passkey - I think that is also not sufficient as he can get the MAC address as well from it (or maybe in this case - it is more difficult).
If we are creating some DB encryption key (let's call it like that for now), we can use a shadow file or something similar to protect it.
Question is - what are we trying to achieve here and from who are we trying to protect the passphrase for the tacacs server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, a secure passphrase is necessary to verify the identity of users or devices seeking access to network resources. Without that unauthorised users could gain access to sensitive systems and data. Thus, encryption of the plaintext passphrase is needed.
But yeah, I understand that the only MAC based encryption won't be enough. Using shadow file is also a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that creating a secure passphrase is critical for protecting any sensitive data that we have on top of the system.
My question is whether this will suffice, as I have presented different vectors where a user can get this data therefor bypassing this protection that was proposed here.
I'm still trying to understand the original security mitigation proposed here and from what type of attacks/users it protects.
In the case we are going with a shadow file for the DB password/key/secret - we can explore that, but we need to elaborate here what we would like to do with said key, how should we store it, manage it and export it if required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presently, we are only targeting the outsiders who doesn't have access to the local network. For instance, if we shared the config_db.json to a third party vendor. In that case, passkey needs to be safe guarded. In the later releases, we will think of enhancing this implementation with the shadow file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make use of shadow file for replacing MAC address (as a salt) with an encrypted user specific password. For
instance,
<snip_from_shadowfile>
admin:$6$YTJ7JKnfsB4esnbS$5XvmYk2.GXVWhDo2TYGN2hCitD/wU9Kov.uZD8xsnleuf1r0ARX3qodIKiDsdoQA444b8IMPMOnUWDmVJVkeg1:19446:0:99999:7:::
<snip_from_shadowfile>
"YTJ7JKnfsB4esnbS$5XvmYk2.GXVWhDo2TYGN2hCitD/wU9Kov.uZD8xsnleuf1r0ARX3qodIKiDsdoQA444b8IMPMOnUWDmVJVkeg1" is the encrypted password for user "admin". We can use the same hash for encrypting the TACACS passkey. What are your thoughts @Yarden-Z ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way, salt will only be visible to the root user and it will be same across all the devices. Additionally, same config_db.json can also be loaded on to another device without any intermediate modifications.
doc/TACACSPLUS_PASSKEY_ENCRYPTION.md
Outdated
|
||
#### Show CLI changes | ||
|
||
Furthermore, aside from encrypting the passkey stored within CONFIG_DB, this infrastructure ensures that the passkey itself remains concealed from any of the displayed CLI outputs. Consequently, the passkey field has been eliminated from the "show tacacs" output, and it will now solely indicate the status whether the passkey is configured or not. For instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also remove this password from all log files in the system?
I assume it must remain in the tacplus_nss_conf and also in PAM, but we must make sure that it is not exposed in other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @Yarden-Z. The passkey in plaintext format will only be visible in PAM config file. Else where it will be masked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, can we make sure to test that the key is not present in anywhere else in the system once it is configured?
Also, might be worth checking that the files where this key is present is not globally readable.
If it is and an un-privileged user can read it, then encrypting it in the DB will serve minimal purpose.
doc/TACACSPLUS_PASSKEY_ENCRYPTION.md
Outdated
["TACPLUS global passkey configured Yes / No"] | ||
|
||
### DB migration | ||
A DB migration script will be added for users to migrate existing config_db to convert tacacs passkey plaintext to encypted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about DB sync and DB export?
If we have device A with it's configuration and we would like to copy the configuration to device B, how would the redis export perform in the case we have encrypted the Tacacs password?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have to copy configuration from device A to B, there are two ways.
-
Copy configuration from device A to B and later generate a new encrypted key in device B so that the hostcfgd on device B must be able to decode the encrypted key. This is required as the system MAC address is used as an encryption password.
-
Generate the configuration for device B (including a new encrypted passkey for device B) and later apply the same.
Code Reference:
1. passkey: User entered passkey in plaintext
2. pswd: system MAC
def encryption(passkey, pswd):
cmd = [ 'openssl', 'enc', '-aes-128-cbc', -A', '-a', '-salt', '-pbkdf2', '-pass', 'pass:' + pswd ]
p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
encryptedkey, err = p.communicate(input=passkey)
return encryptedkey, err
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is no seamless transition between devices.
If we export a config file from device A it will be applicable only for device A and not for any other device in the network.
And, therefor - applying this configuration on another device requires some adjustments to the configuration itself (currently, it's only the Tacacs passkey, but this is the current state as this feature might be applied to other items as well in the system such as radius passkey, snmp community string, ntp key, etc')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. To make the config export seamless between the devices, we have to generate an encrypted passkey which can work on all the devices. And for that we need to use same password for encryption across all the devices as passkey can not be changed per device.
doc/TACACSPLUS_PASSKEY_ENCRYPTION.md
Outdated
Need to add new / update the existing TACACS testcases to incorporate this new feature | ||
Test cases to unit test encrypt and decrypt fucntions | ||
Test cases to add test the TACACS+ functionality with passkey encryption | ||
Test cases to cover DB migration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upgrade tests should be added, as well as checking on a per server vs global test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Could you please explain how TACACS key encryption is handled during configuration backups & restores ? As far as I understand, in case of RMA, the system MAC will change, therefore the encrypted key stored in the config file won't be usable on the new box :(
Could you please check and advise ?
Thanks.
Note : config backup/restore is explained here : https://github.com/sonic-net/SONiC/blob/master/doc/ztp/SONiC-config-setup.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ludal35 for RMA case, Backup the config and while restore the config encrypt the passkey with new system mac, update the config and load it. Typically, customers integrate with ZTP process or orchestrion used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of config restore, we need to write post-hook script to generate a new encrypted passkey based on the new MAC.
doc/TACACSPLUS_PASSKEY_ENCRYPTION.md
Outdated
|
||
### Benifits | ||
|
||
TACACS passkey encryption adds an extra layer of security to safeguard the passkey on each device throughout the network. Moreover, the utilization of MAC address-based encryption ensures that each network device possesses its distinct encrypted passkey. This strategy effectively mitigates the risk of a major network breach in case one of the devices is compromised. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature enforces password encryption across all tacacs password settings (both global and per-server passwords), correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, right.
[a-barboza] There are multiple clauses or parts in that question. No, that is not the suggestion. My concern is that an admin is being forced to encrypt the TACACS+ protocol passkey with a key that is assigned, and cannot be changed. Would you use a combination lock with the condition that it be only locked with the combination which happens to be the serial number of that box which also happens to be engraved on it? Probably not, right?
[a-barboza] It makes sense to have a common framework/infrastructure that all protocols could use. So, a thorough analysis might be good thing! |
After careful consideration of the feedback, we are proposing the a fixed / hardcoded hash will be used as a salt/password for encryption/decryption and it will be saved locally not in any of the databases. The unique salt will be the device admin password (encrypted hash from linux shadow file) and it will be saved local to the device and never need to store it any of the the databases. |
doc/TACACSPLUS_PASSKEY_ENCRYPTION.md
Outdated
The implementation stands on four key pillars. | ||
1. OPENSSL toolkit is used for encryption / decryption. | ||
2. aes-128-cbc is the encoding format used for encryption / decryption. | ||
3. A unique device admin password (encrypted hash from linux shadow file) will be used as a salt/password to encrypt/decrypt the configured pass key in ConfigDB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to handle password change?
There was a password hardening feature will make the password expired: https://github.com/sonic-net/SONiC/blob/master/doc/passw_hardening/hld_password_hardening.md?plain=1
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the encryption and decryption is solely based on the "admin" password only. If the password expires and it has been changed, the network admin needs to generate a new encrypted passkey using the new admin password hash present in the shadow file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmoray , I think the encrypted key update should be automatically done when admin password changed.
If not, when the admin password changed, device can't connect with tacacs server anymore, which will break many automation service in production environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuh-80 Automating this would not be straight forward as we need to modify "passwd" linux command to achieve the same.
We can document a requirement that this implementation is based on the admin password present in shadow file. Thus, if the operator is planing to change the "admin" password, he / she has to propagate that change to all the devices. Normally, the operators have their own ZTP scripts to do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there will be added effort to adjust the hash key and perform the encryption-decryption once we change the admin password?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best approach would be to introduce an infra that can provide encryption/decryption as a service across SONiC which will also take care of the key management and storage. This way, all protocols can use the service without worrying about management of the key. The service must be accessible to applications across dockers and host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested approach can be made common across all the features. But are we fine with the encryption / decryption and password management approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best approach would be to introduce an infra that can provide encryption/decryption as a service across SONiC which will also take care of the key management and storage. This way, all protocols can use the service without worrying about management of the key. The service must be accessible to applications across dockers and host.
The solution probably consists in deploying some sort of vault that could safely store passwords, keys, certificates. SONiC should be able to create such a vault during the NOS installation, populate the vault using a script (typically for ZTP deployments) rather than waiting for user input on the CLI. Last but not least, SONiC should be able to manage the associated access rights (TACACS client should not be allowed to read the LDAP password, and vice versa).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best approach would be to introduce an infra that can provide encryption/decryption as a service across SONiC which will also take care of the key management and storage. This way, all protocols can use the service without worrying about management of the key. The service must be accessible to applications across dockers and host.
@nmoray This design looks generic and works for all protocols (TACACS+, RADIUS, LDAP, BGP..etc) that need the encryption/decryption. Refer below diagram for some details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@venkatmahalingam thank you, I have updated the HLD. Yes the design is generic, support all three protocols (TACACS+, RADIUS, LDAP, BGP..etc) that need the encryption/decryption. 202311 release is around the corner, could you please approve and merge the HLD, we will work on code PRs and get it done.
The code PRs for this HLD are as follows. |
doc/TACACSPLUS_PASSKEY_ENCRYPTION.md
Outdated
show tacacs | ||
["TACPLUS global passkey configured Yes / No"] | ||
### DB migration | ||
A DB migration script will be added for users to migrate existing config_db to convert tacacs passkey plaintext to encrypted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this feature be enable/disable by CONFIG_DB setting?
I'm warry about backward compatibility, if a production environment has hundreds of thousands of devices need OS upgrade, and they are installed different old version, upgrade is difficult and may cause livesite.
If there is a flag to disable this feature, then it's much easy to do the upgrade, also can keep backward compatibility.
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuh-80 We can achieve the backward compatibility by following your suggestion. We will add check in hostcfgd if the already configured passkey is in plaintext or not. If it is in plantext (same as the one in common-auth-sonic), we will skip the decryption and proceed. This way, operator doesn't need to change anything if he / she upgrades the image. The logic will allow the user to login with existing TACACS credentials.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuh-80 what are your thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK, please update design doc and code.
Also if the feature been disabled, there need some signal check, maybe hostcfgd write a warning/error syslog when render the TACACS config files.
+----v----+ | | |t | +-------+--------+ | ||
| | | | -- | | | ||
| CLI +----------------+ +------------------------> ConifgDB | | ||
| | Encrypted passkey | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the typo. ConifgDB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it. @venkatmahalingam can you merge this PR? or should I ask @zhangyanzhao ?
Fixed Typo.
@venkatmahalingam Created a code PR for Master key Manager. |
@venkatmahalingam @zhangyanzhao We have addressed all the review comments, can you merge the PR to master branch. Code PRs are in review. Thanks |
@madhupalu can you please help to add the code PRs by referring to #806 ? Currently, it is hard to find all code PRs from the comments of this HLD. BTW, I will move this feature to 202405 release. |
|
||
|
||
### Approach | ||
1. A runtime flag (via config_db) for Enable / disable feature: "key_encrypt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yang model need update to include this change.
because this is config_db change, please also add schema and yang model to this HLD.
When device already has passkey and key_encrypt is false, then user set this flag to true, the passkey need encrypt in config DB, which component will do that?
Also, how to rollout this feature? there still backward compatibility issue:
Before rollout, passkey was plaintext and key_encrypt is false.
After rollout, passkey need encrypt and key_encrypt is true.
The gap is passkey need change from plaintext to encrypted when set key_encrypt flag.
Will load_minigraph command do the migration when apply the new config?
if not migrate, current hostcfgd code will report error because passkey decrypt failed.
#Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The passkey encryption in CONFIG_DB will be done during the configuration part (in aaa.py).
No Liuh, we don't need to have 'key_encrypt' flag in the existing schema. It is optional. If this flag is absent, encryption will not happen and the passkey will be pushed in plain text format only inside CONFIG_DB.
After rollout, if the user wants to enable this feature, he / she needs to simply enable "key_encrypt" flag and re-configure the passkey so that it will get encrypted and further replaced the same in CONFIG_DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When user enable the "key_encrypt" flag, he / she needs to reconfigure the passkey. This is the requirement. Two steps are required to be followed if anyone wants to use this feature. But this will certainly not hamper the back-compatibility. It is backward compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is like, if we need to configure SVI, we first need to add a vlan interface and later mark it as L3 interface by assigning an IP. Two steps. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, if we are using CLI to encrypt the passkey, it will take care of updating the same into CONFIG_DB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want me to add CLI to enable 'key_encrypt' flag for TACACS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuh-80 I have now provided an option to set the key_encrypt flag in TACPLUS table via CLI. Please check the PR-Part I. Now user need to provide -e option for enabling encryption feature. If not used, it will internally mark key_encrypt flag as false. Thus, post rollout of this feature, that flag will automatically be added in during the configuration itself.
sonic/# config tacacs passkey -h
Usage: config tacacs passkey [OPTIONS] <secret_string>
Specify TACACS+ server global passkey <STRING>
Options:
-e, --encrypt Enable passkey encryption feature
-h, -?, --help Show this message and exit.
And as I mentioned in the previous message, hostcfgd will take care of handling the backward compatibility by doing the decryption only when key_encrypt flag is present and it is set to true. Please refer PR - Part II
@madhupalu please add below set of PRs in the description. |
@qiluo-msft can you please help to double check this HLD and merge this PR after your check? Thanks. |
community review recording https://zoom.us/rec/share/7U0nSCopaMzKnOOOXjGF0tXHKwoznifSBYMDctqwwASNQGNvQob2WAGL9zwOaXwd.qiD2kqkwN0BZpQ1h |
|
||
### Approach | ||
1. A runtime flag (via config_db) for Enable / disable feature: "key_encrypt" | ||
2. CLI will ask for a encryption master key/password while configuring the TACACS passkey and at the backend it will be stored at /etc/cipher_pass file which is accessible to only root user with read only permissions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI :
- Cisco devices running IOS (-XE or -XR flavors) rely on a very poor encryption (type 7) that can be easily reversed using online tools (https://packetlife.net/toolbox/type7/). AFAIK there's no master key on IOS, therefore configuration backups can be easily redeployed on another box.
- Juniper devices running Junos rely by default on an obfuscation algorithm (type 9), which is also a weak encryption for shared secrets stored in configuration. Such passwords can be decoded using online tools (https://www.m00nie.com/juniper-type-9-password-tool/) or even using a tool provided in Junos (request system decrypt password) so that configuration backup files can be restored on RMA'ed devices for example. Nevertheless, Juniper introduced new CLI commands to configure a system master password to provide stronger encryption for configuration secrets (the master password itself is not saved as part of the configuration). The master password is used as input to the password based key derivation function (PBKDF2) to generate an encryption key. The key is used as input to the Advanced Encryption Standard in Galois/Counter Mode (AES256-GCM). The plain text that the user enters is processed by the encryption algorithm (with key) to produce the encrypted text (cipher text). More details avaible here : https://www.juniper.net/documentation/us/en/software/junos/user-access/topics/topic-map/master-password-configuration-encryption.html#id-hardening-shared-secrets-in-junos-os
HTH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ludal35 for the references from the popular NOSs.
@qiluo-msft If we want to have unique encrypted key for every device, it is possible with or without changing the password too.
But the pain point here is, if we have to vary the password for every device, network operator has to keep track of those many passwords (in case he / she wants to change it in the future).
Note: Here the encryption is solely based on two things, original passkey (in plaintext) and the associated password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Junos approach seems to be a good compromise:
- by default, password is not stored in plain text in the config file (obfuscated format) so that a config file can be redeployed on another device (in case of RMA for exemple).
- if needed, the network administrator can use a master key to encrypt the secrets (not only the TACACS server key, but also the Radius one, the OSPF/BGP neighbor passwords, IKE preshared keys...). There are no restrictions on the master password (@nmoray reusing the same master password across all devices is possible).
- Moreover, Junos can optionnaly rely on the TPM chip of the Juniper device (if available) to protects secrets such as private keys, system primary passwords, and other sensitive data by storing it in an AES256 encrypted format (instead of storing sensitive data in a clear text format).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ludal35 you mean single master password for all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmoray Yes, a single master password for all secrets stored in the SONiC config file (TACACS / Radius server keys and maybe the SNMP communities/secrets as well). Regarding the OSPF/BGP neighbor passwords, I assume this is handled by FRR so this is probably out of the SONiC scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah @ludal35 as I mentioned in the community review meeting, I intentionally kept the design flexible so that either we can have single master password for all or can keep different passwords for different features.
merged based on release triage |
code PRs need be reviewed. |
code PRs are not approved yet, move to backlog |
Adding support for TACACS+ passkey encryption for SONiC deployments.
Related PRs:
+++++++++