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 - I #3027

Open
wants to merge 9 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 encryption 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`

config/aaa.py Outdated
@@ -11,6 +12,41 @@
RADIUS_MAXSERVERS = 8
RADIUS_PASSKEY_MAX_LEN = 65
VALID_CHARS_MSG = "Valid chars are ASCII printable except SPACE, '#', and ','"
TACACS_PASSKEY_MAX_LEN = 65
TACACS_SECRET_SALT = "2e6593364d369fba925092e0c1c51466c276faa127f20d18cc5ed8ae52bedbcd"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this hardcoded SALT? as you choose to use shadow file for SALT, what is the significance of it? shouldn't this value differ per device?

Copy link
Author

Choose a reason for hiding this comment

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

It is random hash and will be fixed on all the devices unless and util an explicit admin password is configured. This a fail-safe mechanism to ensure that the encryption / decryption succeeds even if there is no admin password configured on a device.
If configured, the code will use the salt from a shadow file (from admin user credentials).

config/aaa.py Outdated
except Exception as e:
click.echo('An error occurred: ' % str(e))

if salt == None:
Copy link
Contributor

Choose a reason for hiding this comment

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

what scenarios do you anticipate no salt from shadow file?

Copy link
Author

Choose a reason for hiding this comment

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

When there is no admin password configured.

config/aaa.py Outdated Show resolved Hide resolved
@madhupalu
Copy link
Contributor

@nmoray please take care of unit test coverage for PR to be approved.

config/aaa.py Outdated Show resolved Hide resolved
@liuh-80
Copy link
Contributor

liuh-80 commented Nov 2, 2023

@nmoray , please fix coverage issue by add new test case:

@azure-pipelines-wrapper
coverage.Azure.sonic-utilities.Python3 — Pull Request Coverage

Total: 44 lines
Missing: 39 lines
Coverage: 11%
Threshold: 80%

@nmoray
Copy link
Author

nmoray commented Nov 2, 2023

@nmoray , please fix coverage issue by add new test case:

@azure-pipelines-wrapper coverage.Azure.sonic-utilities.Python3 — Pull Request Coverage

Total: 44 lines Missing: 39 lines Coverage: 11% Threshold: 80%

@liuh-80 can you please give me some pointers for better understanding of this AUT infra. Like, how to write a new testcase.

@liuh-80
Copy link
Contributor

liuh-80 commented Nov 3, 2023

@nmoray , please fix coverage issue by add new test case:
@azure-pipelines-wrapper coverage.Azure.sonic-utilities.Python3 — Pull Request Coverage
Total: 44 lines Missing: 39 lines Coverage: 11% Threshold: 80%

@liuh-80 can you please give me some pointers for better understanding of this AUT infra. Like, how to write a new testcase.

You can find existing test case and add new test case here: https://github.com/sonic-net/sonic-utilities/blob/master/tests/aaa_test.py

@zhangyanzhao
Copy link
Collaborator

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

Copy link
Contributor

@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