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 support Part - II #81

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

nmoray
Copy link

@nmoray nmoray commented Oct 25, 2023

  • What I did
    Added a support of TACACS passkey encryption feature.
    Ref. : HLD
    This PR comprises the decryption logic.

  • How I did it
    Implemented the feature by following HLD

  • How to verify it
    `1. Configured TACACS passkey:
    root@sonic:/# config tacacs passkey

  1. Verified whether passkey is encrypted:
    root@sonic:/# show runningconfiguration all | grep passkey
    "passkey": "U2FsdGVkX19kFwDeP3IhgqbLJeed3pXtazJ73FtmD3I="

  2. Verified /etc/pam.d/common-auth-sonic file to validate if the passkey is decrypted correctly [Referred while ssh'ing into the device]
    root@sonic:~# cat /etc/pam.d/common-auth-sonic | grep secret
    auth [success=done new_authtok_reqd=done default=ignore auth_err=die] pam_tacplus.so server=:49 secret=<pass_in_plaintext> login=login timeout=5 try_first_pass
    auth [success=done new_authtok_reqd=done default=ignore auth_err=die] pam_tacplus.so server=:49 secret=<pass_in_plaintext> login=login timeout=5 try_first_pass

  3. Verified passkey is hidden in show tacacs output
    root@sonic:~# show tacacs
    TACPLUS global auth_type pap (default)
    TACPLUS global timeout 5 (default)
    TACPLUS global passkey configured Yes

  4. Verified user able to login into device with TACACS credentials`

@nmoray nmoray changed the title Tacacs passkey encryption support Tacacs passkey encryption support Part - II Oct 25, 2023
@nmoray nmoray changed the title Tacacs passkey encryption support Part - II TACACSPLUS_PASSKEY_ENCRYPTION support Part - II Oct 25, 2023
code cleanup

Added syslogs

Removed debug prints

Fixed AUT issues

Fixed build issues

Fixed build issues

fixed build issues

Added a check for passkey before appending into server configs

Fixed build issues
scripts/hostcfgd Outdated Show resolved Hide resolved
@madhupalu
Copy link

@nmoray
Please take care of code coverage?

Thanks

scripts/hostcfgd Outdated
if server['passkey'] is not None:
config_db = ConfigDBConnector()
config_db.connect()
output, errs = decrypt_passkey(server['passkey'])
Copy link
Contributor

@liuh-80 liuh-80 Nov 1, 2023

Choose a reason for hiding this comment

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

What will happen when OS upgrade from old version which not support this feature? the passkey not encrypt in old version, decrypt will failed here, code here need identify this.
#closed

another solution is add code to DB migrator, but that script is very complex.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, you are right. Decrypt will fail if someone has either manually added the passkey in config_db or the device has old config where encrypted passkey was absent. So is it fine to add a check if the given server['passkey'] is the same as the one present in common-auth-sonic file (in plaintext). If so, skip decrypt_passkey() API and directly write the passkey in the same file in plaintext format only. Is it okay?

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed in the HLD: A DB migration script will be added for users to migrate existing config_db to convert tacacs passkey plaintext to encrypted.

So please also create the DB migration script PR.

Copy link
Author

Choose a reason for hiding this comment

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

@liuh-80 As I mentioned in the HLD PR, we can achieve the backward compatibility without DB migration by simply following the logic stated above. Please share your thoughts on 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.

I think it's OK, please update code and HLD according to this design change.

@zhangyanzhao
Copy link

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

Copy link

@madhupalu madhupalu left a comment

Choose a reason for hiding this comment

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

I am good with the changes

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

Successfully merging this pull request may close these issues.

4 participants