Skip to content
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

Merged
merged 12 commits into from
May 22, 2024
Merged

Conversation

madhupalu
Copy link
Contributor

@madhupalu madhupalu commented Sep 13, 2023

Adding support for TACACS+ passkey encryption for SONiC deployments.

Related PRs:
+++++++++

  1. Security Cipher module - Adding support of common security cipher module for encryption and decryption of a passkey sonic-buildimage#17201
  2. Encryption using security cipher module - TACACSPLUS_PASSKEY_ENCRYPTION support Part - I sonic-utilities#3027
  3. Decryption using security cipher module - TACACSPLUS_PASSKEY_ENCRYPTION support Part - II sonic-host-services#81

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
Copy link
Collaborator

@venkatmahalingam venkatmahalingam Sep 13, 2023

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?

Copy link

@nmoray nmoray Sep 13, 2023

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.

@madhupalu madhupalu requested a review from Yarden-Z September 13, 2023 16:30
@madhupalu
Copy link
Contributor Author

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.
Copy link
Collaborator

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?

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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";
}

Copy link

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.

Copy link
Contributor

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.

Copy link

@nmoray nmoray Nov 2, 2023

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?

Copy link
Contributor

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"]

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link

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.

@a-barboza
Copy link
Collaborator

cc: @shdasari @adyeung

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)

  • The key encryption key is not secret because it is the MAC address of the device.
  • It cannot be changed, therefore the admin has no control over its secrecy.
  • The admin cannot use CONFIG_DB snippets unmodified on other devices, because they are keyed by MAC address.
  • The mechanism of encryption is not described. There could be some concern with using the key encryption key (MAC address) directly if that is indeed the intent. Is that the intent?

@nmoray
Copy link

nmoray commented Sep 14, 2023

  • The mechanism of encryption is not described.

cc: @shdasari @adyeung

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)

  • The key encryption key is not secret because it is the MAC address of the device.
    [nmoray]--> I think I confused you. Let me clarify. The passkey used here is not the system MAC address. The MAC is used an a salt to encrypt the unique passkey (which is in plaintext).
  • It cannot be changed, therefore the admin has no control over its secrecy.
    [nmoray]--> The encrypted passkey will be unique for every node so this design will be less susceptible to the overall network breach.
  • The admin cannot use CONFIG_DB snippets unmodified on other devices, because they are keyed by MAC address.
    [nmoray]--> Yes, that is correct. But the easy way out is, network admin will simply execute "config tacacs passkey <actual_passkey_in_plaintext>" command after copying the CONFIG_DB snippets on a given device so that a new unique encrypted passkey will be generated
  • The mechanism of encryption is not described. There could be some concern with using the key encryption key (MAC address) directly if that is indeed the intent. Is that the intent?
    [nmoray]--> Please refer to the "Implementation details" section where we have explained the encryption details. Can you please elaborate bit more about those concerns?
    The implementation is straight forward. An encryption API which will take two inputs, passkey in plaintext and the system MAC. It will use openssl lib to encrypt the provided passkey and use system MAC as a salt.

@a-barboza the replies are inline.

@a-barboza
Copy link
Collaborator

  • The mechanism of encryption is not described.

cc: @shdasari @adyeung
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)

  • The key encryption key is not secret because it is the MAC address of the device.
    [nmoray]--> I think I confused you. Let me clarify. The passkey used here is not the system MAC address. The MAC is used an a salt to encrypt the unique passkey (which is in plaintext).
  • It cannot be changed, therefore the admin has no control over its secrecy.
    [nmoray]--> The encrypted passkey will be unique for every node so this design will be less susceptible to the overall network breach.
  • The admin cannot use CONFIG_DB snippets unmodified on other devices, because they are keyed by MAC address.
    [nmoray]--> Yes, that is correct. But the easy way out is, network admin will simply execute "config tacacs passkey <actual_passkey_in_plaintext>" command after copying the CONFIG_DB snippets on a given device so that a new unique encrypted passkey will be generated
  • The mechanism of encryption is not described. There could be some concern with using the key encryption key (MAC address) directly if that is indeed the intent. Is that the intent?
    [nmoray]--> Please refer to the "Implementation details" section where we have explained the encryption details. Can you please elaborate bit more about those concerns?
    The implementation is straight forward. An encryption API which will take two inputs, passkey in plaintext and the system MAC. It will use openssl lib to encrypt the provided passkey and use system MAC as a salt.

@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
Copy link

nmoray commented Sep 15, 2023

  • The mechanism of encryption is not described.

cc: @shdasari @adyeung
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)

  • The key encryption key is not secret because it is the MAC address of the device.
    [nmoray]--> I think I confused you. Let me clarify. The passkey used here is not the system MAC address. The MAC is used an a salt to encrypt the unique passkey (which is in plaintext).
  • It cannot be changed, therefore the admin has no control over its secrecy.
    [nmoray]--> The encrypted passkey will be unique for every node so this design will be less susceptible to the overall network breach.
  • The admin cannot use CONFIG_DB snippets unmodified on other devices, because they are keyed by MAC address.
    [nmoray]--> Yes, that is correct. But the easy way out is, network admin will simply execute "config tacacs passkey <actual_passkey_in_plaintext>" command after copying the CONFIG_DB snippets on a given device so that a new unique encrypted passkey will be generated
  • The mechanism of encryption is not described. There could be some concern with using the key encryption key (MAC address) directly if that is indeed the intent. Is that the intent?
    [nmoray]--> Please refer to the "Implementation details" section where we have explained the encryption details. Can you please elaborate bit more about those concerns?
    The implementation is straight forward. An encryption API which will take two inputs, passkey in plaintext and the system MAC. It will use openssl lib to encrypt the provided passkey and use system MAC as a salt.

@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 (initialisation 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.

  1. Generate encrypted passkey with same plaintext passkey + system MAC
  2. Generate encrypted passkey with different plaintext passkey + system MAC
    Note: OpenSSL generates a distinct hash each time if we input the same secret / plaintext passkey along with the same password.
    In this approach, the network admin has a luxury to change the plaintext passkey but not the password.

[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.

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

@a-barboza
Copy link
Collaborator

  • The mechanism of encryption is not described.

cc: @shdasari @adyeung
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)

  • The key encryption key is not secret because it is the MAC address of the device.
    [nmoray]--> I think I confused you. Let me clarify. The passkey used here is not the system MAC address. The MAC is used an a salt to encrypt the unique passkey (which is in plaintext).
  • It cannot be changed, therefore the admin has no control over its secrecy.
    [nmoray]--> The encrypted passkey will be unique for every node so this design will be less susceptible to the overall network breach.
  • The admin cannot use CONFIG_DB snippets unmodified on other devices, because they are keyed by MAC address.
    [nmoray]--> Yes, that is correct. But the easy way out is, network admin will simply execute "config tacacs passkey <actual_passkey_in_plaintext>" command after copying the CONFIG_DB snippets on a given device so that a new unique encrypted passkey will be generated
  • The mechanism of encryption is not described. There could be some concern with using the key encryption key (MAC address) directly if that is indeed the intent. Is that the intent?
    [nmoray]--> Please refer to the "Implementation details" section where we have explained the encryption details. Can you please elaborate bit more about those concerns?
    The implementation is straight forward. An encryption API which will take two inputs, passkey in plaintext and the system MAC. It will use openssl lib to encrypt the provided passkey and use system MAC as a salt.

@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 (initialisation 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.

  1. Generate encrypted passkey with same plaintext passkey + system MAC
  2. Generate encrypted passkey with different plaintext passkey + system MAC
    Note: OpenSSL generates a distinct hash each time if we input the same secret / plaintext passkey along with the same password.
    In this approach, the network admin has a luxury to change the plaintext passkey but not the password.

[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.

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

[a-barboza] the python snippet helps answer the mechanism of encryption.
[a-barboza] the concern about the admin not being able to change the -pass argument remains. In the description of the mechanism of encryption, the pswd is always the system MAC. The admin cannot change that, unless they change the system MAC, which may not be possible (or not desirable to do) always.
[a-barboza] the solution is currently catered to TACACS+ shared secrets only? If each protocol has it's own mechanism, this may become unmanageable quite quickly. Just my 2 cents. Thanks for your replies.

@nmoray
Copy link

nmoray commented Sep 18, 2023

which may not be possible (or not desirable to do) always.

  • The mechanism of encryption is not described.

cc: @shdasari @adyeung
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)

  • The key encryption key is not secret because it is the MAC address of the device.
    [nmoray]--> I think I confused you. Let me clarify. The passkey used here is not the system MAC address. The MAC is used an a salt to encrypt the unique passkey (which is in plaintext).
  • It cannot be changed, therefore the admin has no control over its secrecy.
    [nmoray]--> The encrypted passkey will be unique for every node so this design will be less susceptible to the overall network breach.
  • The admin cannot use CONFIG_DB snippets unmodified on other devices, because they are keyed by MAC address.
    [nmoray]--> Yes, that is correct. But the easy way out is, network admin will simply execute "config tacacs passkey <actual_passkey_in_plaintext>" command after copying the CONFIG_DB snippets on a given device so that a new unique encrypted passkey will be generated
  • The mechanism of encryption is not described. There could be some concern with using the key encryption key (MAC address) directly if that is indeed the intent. Is that the intent?
    [nmoray]--> Please refer to the "Implementation details" section where we have explained the encryption details. Can you please elaborate bit more about those concerns?
    The implementation is straight forward. An encryption API which will take two inputs, passkey in plaintext and the system MAC. It will use openssl lib to encrypt the provided passkey and use system MAC as a salt.

@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 (initialisation 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.

  1. Generate encrypted passkey with same plaintext passkey + system MAC
  2. Generate encrypted passkey with different plaintext passkey + system MAC
    Note: OpenSSL generates a distinct hash each time if we input the same secret / plaintext passkey along with the same password.
    In this approach, the network admin has a luxury to change the plaintext passkey but not the password.

[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.

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

[a-barboza] the python snippet helps answer the mechanism of encryption. [a-barboza] the concern about the admin not being able to change the -pass argument remains. In the description of the mechanism of encryption, the pswd is always the system MAC. The admin cannot change that, unless they change the system MAC, which may not be possible (or not desirable to do) always. [a-barboza] the solution is currently catered to TACACS+ shared secrets only? If each protocol has it's own mechanism, this may become unmanageable quite quickly. Just my 2 cents. Thanks for your replies.

@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. :-)

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
Copy link
Contributor

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?

Copy link

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.

Copy link
Contributor

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?

Copy link

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?

Copy link
Contributor

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?

Copy link

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.

Copy link
Contributor

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.

Copy link

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.

Copy link

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 ?

Copy link

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.


#### 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.
Copy link
Contributor

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.

Copy link

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.

Copy link
Contributor

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.

["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.
Copy link
Contributor

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?

Copy link

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.

  1. 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.

  2. 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

Copy link
Contributor

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')

Copy link

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.

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
Copy link
Contributor

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

Copy link

@nmoray nmoray Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted.

Copy link

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

Copy link
Contributor Author

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.

Copy link

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.


### 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.
Copy link
Contributor

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, right.

@a-barboza
Copy link
Collaborator

which may not be possible (or not desirable to do) always.

  • The mechanism of encryption is not described.

cc: @shdasari @adyeung
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)

  • The key encryption key is not secret because it is the MAC address of the device.
    [nmoray]--> I think I confused you. Let me clarify. The passkey used here is not the system MAC address. The MAC is used an a salt to encrypt the unique passkey (which is in plaintext).
  • It cannot be changed, therefore the admin has no control over its secrecy.
    [nmoray]--> The encrypted passkey will be unique for every node so this design will be less susceptible to the overall network breach.
  • The admin cannot use CONFIG_DB snippets unmodified on other devices, because they are keyed by MAC address.
    [nmoray]--> Yes, that is correct. But the easy way out is, network admin will simply execute "config tacacs passkey <actual_passkey_in_plaintext>" command after copying the CONFIG_DB snippets on a given device so that a new unique encrypted passkey will be generated
  • The mechanism of encryption is not described. There could be some concern with using the key encryption key (MAC address) directly if that is indeed the intent. Is that the intent?
    [nmoray]--> Please refer to the "Implementation details" section where we have explained the encryption details. Can you please elaborate bit more about those concerns?
    The implementation is straight forward. An encryption API which will take two inputs, passkey in plaintext and the system MAC. It will use openssl lib to encrypt the provided passkey and use system MAC as a salt.

@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 (initialisation 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.

  1. Generate encrypted passkey with same plaintext passkey + system MAC
  2. Generate encrypted passkey with different plaintext passkey + system MAC
    Note: OpenSSL generates a distinct hash each time if we input the same secret / plaintext passkey along with the same password.
    In this approach, the network admin has a luxury to change the plaintext passkey but not the password.

[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.

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

[a-barboza] the python snippet helps answer the mechanism of encryption. [a-barboza] the concern about the admin not being able to change the -pass argument remains. In the description of the mechanism of encryption, the pswd is always the system MAC. The admin cannot change that, unless they change the system MAC, which may not be possible (or not desirable to do) always. [a-barboza] the solution is currently catered to TACACS+ shared secrets only? If each protocol has it's own mechanism, this may become unmanageable quite quickly. Just my 2 cents. Thanks for your replies.

@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.

[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?

BTW, this mechanism may not be limited to TACACS and could be applied to other protocols as well, such as Radius. :-)

[a-barboza] It makes sense to have a common framework/infrastructure that all protocols could use. So, a thorough analysis might be good thing!

@madhupalu
Copy link
Contributor Author

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.
For How to - please refer the implementation section. We are working on the code changes will raise PR's soon.
Thanks

@qiluo-msft qiluo-msft requested a review from liuh-80 October 17, 2023 18:15
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.
Copy link
Contributor

@liuh-80 liuh-80 Oct 18, 2023

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

Copy link

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.

Copy link
Contributor

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.

Copy link

@nmoray nmoray Nov 2, 2023

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link

@nmoray nmoray Nov 8, 2023

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?

Copy link

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).

Copy link
Collaborator

@venkatmahalingam venkatmahalingam Nov 9, 2023

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.

image

Copy link
Contributor Author

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.

@nmoray
Copy link

nmoray commented Oct 26, 2023

The code PRs for this HLD are as follows.
Part - I: Encryption
Part - II: Decryption

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.
Copy link
Contributor

@liuh-80 liuh-80 Nov 2, 2023

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

Copy link

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.

Copy link

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?

Copy link
Contributor

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 | |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the typo. ConifgDB

Copy link
Contributor Author

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 ?

@nmoray
Copy link

nmoray commented Nov 16, 2023

@venkatmahalingam Created a code PR for Master key Manager.

@madhupalu
Copy link
Contributor Author

@venkatmahalingam @zhangyanzhao We have addressed all the review comments, can you merge the PR to master branch. Code PRs are in review.

Thanks

@zhangyanzhao
Copy link
Collaborator

@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"
Copy link
Contributor

@liuh-80 liuh-80 Jan 9, 2024

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liuh-80 I have already updated the yang model in the PR. I will also add the new schema in this HLD.

Copy link

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.

Copy link

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.

Copy link

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. :-)

Copy link

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.

Copy link

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?

Copy link

@nmoray nmoray Jan 9, 2024

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

@nmoray
Copy link

nmoray commented Jan 9, 2024

@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.

@madhupalu please add below set of PRs in the description.

  1. Security Cipher module
  2. Encryption using security cipher module
  3. Decryption using security cipher module

@madhupalu madhupalu changed the title Create TACACSPLUS_PASSKEY_ENCRYPTION.md TACACSPLUS_PASSKEY_ENCRYPTION.md Jan 15, 2024
@zhangyanzhao
Copy link
Collaborator

@qiluo-msft can you please help to double check this HLD and merge this PR after your check? Thanks.

@zhangyanzhao
Copy link
Collaborator


### 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

master key

This is a disruptive behavior change on CLI. From network operators' point of view, it is ideal that each device could figure out a unique and random master key and just use it internally.

Could you also check other industry NOS implementation and follow the similar practice?

Copy link

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.

Copy link

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.

Copy link

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).

Copy link

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?

Copy link

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.

Copy link

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.

@zhangyanzhao zhangyanzhao merged commit a4d3306 into sonic-net:master May 22, 2024
1 check passed
@zhangyanzhao
Copy link
Collaborator

merged based on release triage

@zhangyanzhao
Copy link
Collaborator

code PRs need be reviewed.

@zhangyanzhao
Copy link
Collaborator

code PRs are not approved yet, move to backlog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: MovedToBacklog
Development

Successfully merging this pull request may close these issues.