Skip to content

Integrated Security Cipher for TACPLUS passkey encryption #3936

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 8 commits into
base: master
Choose a base branch
from
Open

Integrated Security Cipher for TACPLUS passkey encryption #3936

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f01c202
Integrated Security Cipher for TACPLUS passkey encryption/decryption
nmoray-ebay Jun 25, 2025
df0cbec
Added missed callback argument
nmoray-ebay Jun 26, 2025
a12b028
Fixed precommit failures
nmoray-ebay Jun 26, 2025
d3b1ce2
Fixed build issues
nmoray-ebay Jun 26, 2025
2e8e79a
Fixed precommit issues
nmoray-ebay Jun 26, 2025
92201bc
Fixed precommits issues
nmoray-ebay Jun 26, 2025
ca28b5c
Updated implementation as per modified design
nmoray-ebay Jun 27, 2025
b70ad67
Fixed build issues
nmoray-ebay Jun 27, 2025
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
122 changes: 114 additions & 8 deletions config/aaa.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,17 @@
from jsonpatch import JsonPatchConflict
from jsonpointer import JsonPointerException
import utilities_common.cli as clicommon
from sonic_py_common.security_cipher import master_key_mgr
import getpass

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(security_cipher_clbk_lookup)
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

where is security_cipher_cblk_lookup defined?



def is_secret(secret):
return bool(re.match('^' + '[^ #,]*' + '$', secret))
Expand Down Expand Up @@ -122,7 +128,7 @@ def trace(db, option):
@clicommon.pass_db
def login(db, auth_protocol):
"""Switch login authentication [ {ldap, radius, tacacs+, local} | default ]"""
if len(auth_protocol) is 0:
if len(auth_protocol) == 0:
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

seems unrelated?

click.echo('Argument "auth_protocol" is required')
return
elif len(auth_protocol) > 2:
Expand Down Expand Up @@ -243,16 +249,66 @@ def authtype(db, ctx, type):

@click.command()
@click.argument('secret', metavar='<secret_string>', required=False)
@click.option('-e', '--encrypt', help='Enable secret encryption', is_flag=True)
@click.option('-r', '--rotate', help='Rotate encryption secret', is_flag=True)
@click.pass_context
@clicommon.pass_db
def passkey(db, ctx, secret):
def passkey(db, ctx, secret, encrypt, rotate):
"""Specify TACACS+ server global passkey <STRING>"""
if ctx.obj == 'default':
del_table_key(db, 'TACPLUS', 'global', 'passkey')
elif secret:
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

in secret is not defined? then what? is the idea that an empty secret disables the passkey entirely? what happens if we pass an empty secret and --encrypt?

add_table_kv(db, '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:
# Set new passwd if not set already
if secure_cipher.is_key_encrypt_enabled("TACPLUS", "global") is False:
# Register feature with Security Cipher module for the 1st time
secure_cipher.register("TACPLUS", "TACPLUS|global")
passwd = getpass.getpass()
# Set new password for encryption
secure_cipher.set_feature_password("TACPLUS", passwd)
else:
# Check if password rotation is enabled
if rotate:
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

nit: could be else-if with else statement above

passwd = getpass.getpass()
# Rotate password for TACPLUS feature and re-encrypt the secret
secure_cipher.rotate_feature_passwd("TACPLUS", passwd)
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

don't we need to encrypt the secret here as well? maybe this shouldn't return?

return
b64_encoded = secure_cipher.encrypt_passkey("TACPLUS", secret)
if b64_encoded is not None:
# Update key_encrypt flag
add_table_kv('TACPLUS', 'global', 'key_encrypt', True)
add_table_kv('TACPLUS', 'global', 'passkey', b64_encoded)
else:
# Deregister feature with Security Cipher module
secure_cipher.deregister("TACPLUS", rotate_tacplus_key)
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

why would we deregister if the encryption failed? also, where is rotate_tacplus_key defined?

click.echo('Passkey encryption failed')
return
except (EOFError, KeyboardInterrupt):
# Deregister feature with Security Cipher module
secure_cipher.deregister("TACPLUS", "TACPLUS|global")
add_table_kv('TACPLUS', 'global', 'key_encrypt', False)
click.echo('Input cancelled')
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

where would we get the EOFError from? inputting nothing for the password entry maybe? also do we actually want to deregister the table in all cases? If we do that then we will get the following behavior:

-> run with --encrypt for the first time, user inputs password to register table
-> run with --encrypt for second time, user cancels command while it's running (table is deregistered)
-> run with --encrypt for the third time, user has to input password to register table AGAIN

do we really need change the feature password in this case?

return
except Exception as e:
# Deregister feature with Security Cipher module
secure_cipher.deregister("TACPLUS", "TACPLUS|global")
add_table_kv('TACPLUS', 'global', 'key_encrypt', False)
click.echo('Unexpected error: %s' % e)
return
else:
click.echo('Argument "secret" is required')
if secure_cipher.is_key_encrypt_enabled("TACPLUS", "global"):
secure_cipher.deregister("TACPLUS", "TACPLUS|global")
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

so if we run without --encrypt, we will remove the associated key encryption password and set key_encrypt to false, since the user is no longer wanting encryption. this seems correct.

What about the --rotate option though? if a user passes --rotate without --encrypt, what then? we should at least throw an error in that case, rather than removing the encryption.

# Update key_encrypt flag to false
add_table_kv('TACPLUS', 'global', 'key_encrypt', False)
add_table_kv('TACPLUS', 'global', 'passkey', secret)
tacacs.add_command(passkey)
default.add_command(passkey)

Expand All @@ -261,13 +317,15 @@ def passkey(db, ctx, secret):
@click.command()
@click.argument('address', metavar='<ip_address>')
@click.option('-t', '--timeout', help='Transmission timeout interval, default 5', type=int)
@click.option('-k', '--key', help='Shared secret')
@click.option('-k', '--key', help='Shared secret, stored in plaintext')
@click.option('-K', '--encrypted_key', help='Shared secret, stored in encrypted format')
@click.option('-r', '--rotate', help='Rotate encryption secret', is_flag=True)
@click.option('-a', '--auth_type', help='Authentication type, default pap', type=click.Choice(["chap", "pap", "mschap", "login"]))
@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)
@clicommon.pass_db
def add(db, address, timeout, key, auth_type, port, pri, use_mgmt_vrf):
def add(address, timeout, key, encrypted_key, rotate, auth_type, port, pri, use_mgmt_vrf, encrypt):
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

I don't see --encrypt as an arg?

"""Specify a TACACS+ server"""
if ADHOC_VALIDATION:
if not clicommon.is_ipaddress(address):
Expand All @@ -288,8 +346,56 @@ def add(db, address, timeout, key, auth_type, port, pri, use_mgmt_vrf):
data['auth_type'] = auth_type
if timeout is not None:
data['timeout'] = str(timeout)
if key is not None:
data['passkey'] = key

if key and encrypted_key:
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

would it make more sense to keep just --key and have an --encrypt argument, instead of --encrypted_key? --encrypted_key sounds like you are passing in a key that is already encrypted, which is not true; you are passing in a plaintext key that you WANT to be encrypted.

raise click.UsageError("You must provide either --key or --encrypted_key")

if encrypted_key is not None:
try:
Copy link
Contributor

@anders-nexthop anders-nexthop Sep 10, 2025

Choose a reason for hiding this comment

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

this is almost the same as the block above, lets refactor them to re-use the logic

# Set new passwd if not set already
if secure_cipher.is_key_encrypt_enabled("TACPLUS_SERVER", address) is False:
# Register feature with Security Cipher module for the 1st time
secure_cipher.register("TACPLUS", f"TACPLUS_SERVER|{address}")
passwd = getpass.getpass()
# Set new password for encryption
secure_cipher.set_feature_password("TACPLUS", passwd)
else:
# Check if password rotation is enabled
if rotate:
passwd = getpass.getpass()
# Rotate password for TACPLUS feature and re-encrypt the secret
secure_cipher.rotate_feature_passwd("TACPLUS", passwd)
return
b64_encoded = secure_cipher.encrypt_passkey("TACPLUS", encrypted_key)
if b64_encoded is not None:
# Update key_encrypt flag
add_table_kv('TACPLUS_SERVER', address, 'key_encrypt', True)
add_table_kv('TACPLUS_SERVER', address, 'passkey', b64_encoded)
else:
# Deregister feature with Security Cipher module
secure_cipher.deregister("TACPLUS", f"TACPLUS_SERVER|{address}")
click.echo('Passkey encryption failed')
return
except (EOFError, KeyboardInterrupt):
# Deregister feature with Security Cipher module
secure_cipher.deregister("TACPLUS", f"TACPLUS_SERVER|{address}")
add_table_kv('TACPLUS_SERVER', address, 'key_encrypt', False)
click.echo('Input cancelled')
return
except Exception as e:
# Deregister feature with Security Cipher module
secure_cipher.deregister("TACPLUS", f"TACPLUS_SERVER|{address}")
add_table_kv('TACPLUS_SERVER', address, 'key_encrypt', False)
click.echo('Unexpected error: %s' % e)
return
else:
if secure_cipher.is_key_encrypt_enabled("TACPLUS_SERVER", address):
secure_cipher.deregister("TACPLUS", f"TACPLUS_SERVER|{address}")
if key is not None:
# Update key_encrypt flag to false
add_table_kv('TACPLUS_SERVER', address, 'key_encrypt', False)
data['passkey'] = key

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