Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
base: master
Are you sure you want to change the base?
TACACSPLUS_PASSKEY_ENCRYPTION support Part - II #81
Changes from 14 commits
8b909b8
48e7d1d
c877edb
b37ddcd
945f4f2
5b61c0c
7cb53e6
43641de
66b57b5
c6c8e0a
977a268
a7d01d6
0a1b098
7b14786
911e2c0
bb5e5ee
d745364
1d85eb6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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.
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, 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?
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 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.
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 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.
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 it's OK, please update code and HLD according to this design change.