TACACSPLUS_PASSKEY_ENCRYPTION support Part - I #3027
Conversation
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" | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
what scenarios do you anticipate no salt from shadow file?
There was a problem hiding this comment.
When there is no admin password configured.
|
@nmoray please take care of unit test coverage for PR to be approved. |
|
@nmoray , please fix coverage issue by add new test case: @azure-pipelines-wrapper Total: 44 lines |
@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 |
|
Reviewers, if you are ok with this PR, please help to approve it. Thanks. |
| try: | ||
| passwd = getpass.getpass() | ||
| except Exception as e: | ||
| click.echo('getpass aborted' % e) |
There was a problem hiding this comment.
you need to add a string formatting operator here (%)
| if not errs: | ||
| add_table_kv('TACPLUS', 'global', 'passkey', outsecret) | ||
| else: | ||
| click.echo('Passkey configuration failed' % errs) |
There was a problem hiding this comment.
you need to add a string formatting operator here (%)
| except Exception as e: | ||
| click.echo('getpass aborted' % e) | ||
| return | ||
| add_table_kv('TACPLUS', 'global', 'key_encrypt', True) |
There was a problem hiding this comment.
the value must be "true" and not a literal True (and also not "True"), look at yang boolean value handling for details
| click.echo('Passkey configuration failed' % errs) | ||
| return | ||
| else: | ||
| add_table_kv('TACPLUS', 'global', 'key_encrypt', False) |
There was a problem hiding this comment.
same as above, the value must be "false" and not a literal False. "False" will work but not as intended, should stick with lowercase "false"
| else: | ||
| add_table_kv('TACPLUS', 'global', 'key_encrypt', False) | ||
| add_table_kv('TACPLUS', 'global', 'passkey', secret) | ||
| secure_cipher.del_cipher_pass() |
There was a problem hiding this comment.
this will delete the entire file, are we sure that's what you want?
| if encrypt: | ||
| try: | ||
| passwd = getpass.getpass() | ||
| except Exception as e: | ||
| click.echo('getpass aborted' % e) | ||
| return | ||
| add_table_kv('TACPLUS', 'global', 'key_encrypt', True) | ||
| outsecret, errs = secure_cipher.encrypt_passkey('TACPLUS', key, passwd) | ||
| if not errs: | ||
| data['passkey'] = outsecret | ||
| else: | ||
| click.echo('Passkey configuration failed' % errs) | ||
| return | ||
| else: | ||
| add_table_kv('TACPLUS', 'global', 'key_encrypt', False) | ||
| data['passkey'] = key | ||
| secure_cipher.del_cipher_pass() |
There was a problem hiding this comment.
this is almost exactly the same as above, should probably be a common function. If you want to keep it separate, all the same comments from above apply here as well.
|
Please add testcases to cover new code, otherwise coverage checker will block this PR. |
@qiluo-msft Sure I will add the testcases. But In my opinion, if sonic-net/sonic-buildimage#17201 PR gets merged first, it will be easy to focus on this dependent PR. |
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
Verified whether passkey is encrypted:
root@sonic:/# show runningconfiguration all | grep passkey
"passkey": "U2FsdGVkX19kFwDeP3IhgqbLJeed3pXtazJ73FtmD3I="
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
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
Verified user able to login into device with TACACS credentials`
PR#81