Skip to content

TACACSPLUS_PASSKEY_ENCRYPTION support Part - I #3027

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.

Sign up for GitHub

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

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

TACACSPLUS_PASSKEY_ENCRYPTION support Part - I #3027

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9dc424c
TACACSPLUS_PASSKEY_ENCRYPTION support
nmoray Oct 25, 2023
256cb7b
Removed debug prints
nmoray Oct 25, 2023
b710e7b
Addressed comments
nmoray Oct 30, 2023
bccbe6a
Addressed comments
nmoray Oct 30, 2023
084e5a8
Incorporated security_cipher class into the implementation
nmoray Nov 24, 2023
98648ae
Added an option to set key_encrypt flag under TACPLUS table in CONFIG…
nmoray Jan 9, 2024
f173660
Added a logic to remove the cipher_pass file when encryption is disabled
nmoray Feb 1, 2024
485eb0e
Added try catch block to catch the getpass() abort
nmoray Feb 1, 2024
1100d58
Updated class name
nmoray Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 50 additions & 4 deletions config/aaa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,21 @@
import click
import ipaddress
import re
import subprocess
from swsscommon.swsscommon import ConfigDBConnector
from .validated_config_db_connector import ValidatedConfigDBConnector
from jsonpatch import JsonPatchConflict
from jsonpointer import JsonPointerException
import utilities_common.cli as clicommon
from sonic_py_common.security_cipher import master_key_mgr

ADHOC_VALIDATION = True
RADIUS_MAXSERVERS = 8
RADIUS_PASSKEY_MAX_LEN = 65
VALID_CHARS_MSG = "Valid chars are ASCII printable except SPACE, '#', and ','"
TACACS_PASSKEY_MAX_LEN = 65

secure_cipher = master_key_mgr()

def is_secret(secret):
return bool(re.match('^' + '[^ #,]*' + '$', secret))
Expand Down Expand Up @@ -234,13 +239,37 @@ def authtype(ctx, type):

@click.command()
@click.argument('secret', metavar='<secret_string>', required=False)
@click.option('-e', '--encrypt', help='Enable passkey encryption feature', is_flag=True)
@click.pass_context
def passkey(ctx, secret):
def passkey(ctx, secret, encrypt):
"""Specify TACACS+ server global passkey <STRING>"""
if ctx.obj == 'default':
del_table_key('TACPLUS', 'global', 'passkey')
elif secret:
add_table_kv('TACPLUS', 'global', 'passkey', secret)
if len(secret) > TACACS_PASSKEY_MAX_LEN:
click.echo('Maximum of %d chars can be configured' % TACACS_PASSKEY_MAX_LEN)
return
elif not is_secret(secret):
click.echo(VALID_CHARS_MSG)
return

if encrypt:
try:
passwd = getpass.getpass()
except Exception as e:
click.echo('getpass aborted' % e)
Copy link

Choose a reason for hiding this comment

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

you need to add a string formatting operator here (%)

return
add_table_kv('TACPLUS', 'global', 'key_encrypt', True)
Copy link

Choose a reason for hiding this comment

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

the value must be "true" and not a literal True (and also not "True"), look at yang boolean value handling for details

outsecret, errs = secure_cipher.encrypt_passkey('TACPLUS', secret, passwd)
if not errs:
add_table_kv('TACPLUS', 'global', 'passkey', outsecret)
else:
click.echo('Passkey configuration failed' % errs)
Copy link

Choose a reason for hiding this comment

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

you need to add a string formatting operator here (%)

return
else:
add_table_kv('TACPLUS', 'global', 'key_encrypt', False)
Copy link

Choose a reason for hiding this comment

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

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"

add_table_kv('TACPLUS', 'global', 'passkey', secret)
secure_cipher.del_cipher_pass()
Copy link

Choose a reason for hiding this comment

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

this will delete the entire file, are we sure that's what you want?

else:
click.echo('Argument "secret" is required')
tacacs.add_command(passkey)
Expand All @@ -256,7 +285,8 @@ def passkey(ctx, secret):
@click.option('-o', '--port', help='TCP port range is 1 to 65535, default 49', type=click.IntRange(1, 65535), default=49)
@click.option('-p', '--pri', help="Priority, default 1", type=click.IntRange(1, 64), default=1)
@click.option('-m', '--use-mgmt-vrf', help="Management vrf, default is no vrf", is_flag=True)
def add(address, timeout, key, auth_type, port, pri, use_mgmt_vrf):
@click.option('-e', '--encrypt', help='Enable passkey encryption feature', is_flag=True)
def add(address, timeout, key, auth_type, port, pri, use_mgmt_vrf, encrypt):
"""Specify a TACACS+ server"""
if ADHOC_VALIDATION:
if not clicommon.is_ipaddress(address):
Expand All @@ -278,7 +308,23 @@ def add(address, timeout, key, auth_type, port, pri, use_mgmt_vrf):
if timeout is not None:
data['timeout'] = str(timeout)
if key is not None:
data['passkey'] = key
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()
Comment on lines +311 to +327
Copy link

Choose a reason for hiding this comment

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

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.

if use_mgmt_vrf :
data['vrf'] = "mgmt"
try:
Expand Down
Loading