From f5b59a00a2a73c5725633ac5ddf28fd7237dc7dc Mon Sep 17 00:00:00 2001 From: lthievenaz-keeper Date: Sat, 23 May 2026 01:09:36 +0100 Subject: [PATCH 01/20] Update doc_url for SSL error in REST (#2067) If packet inspection is detected our error message redirects to a dead link: https://docs.keeper.io/secrets-manager/commander-cli/using-commander/troubleshooting-commander-cli#ssl-certificate-errors Changed this to the new link (from keeperpam directory) https://docs.keeper.io/keeperpam/commander-cli/troubleshooting-commander-cli#ssl-certificate-errors --- keepercommander/rest_api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keepercommander/rest_api.py b/keepercommander/rest_api.py index b56fb8cdc..2e3be217f 100644 --- a/keepercommander/rest_api.py +++ b/keepercommander/rest_api.py @@ -202,7 +202,7 @@ def execute_rest(context, endpoint, payload, timeout=None): proxies=context.proxies, verify=context.certificate_check, timeout=timeout or DEFAULT_TIMEOUT) except requests.exceptions.SSLError as e: - doc_url = 'https://docs.keeper.io/secrets-manager/commander-cli/using-commander/troubleshooting-commander-cli#ssl-certificate-errors' + doc_url = 'https://docs.keeper.io/keeperpam/commander-cli/troubleshooting-commander-cli#ssl-certificate-errors' if len(e.args) > 0: inner_e = e.args[0] if hasattr(inner_e, 'reason'): From b14b50c7dab77caa467451956582c7399e568499 Mon Sep 17 00:00:00 2001 From: Craig Lurey Date: Fri, 22 May 2026 17:27:30 -0700 Subject: [PATCH 02/20] Fix: honor custom label on type=password fields in record display MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a v3 typed field has type="password" with a custom label (e.g. "Passphrase" on the built-in "SSH key with strong passphrase" record type), both `keeper get ` and the supershell TUI were showing the field as "Password: ********" — the label was discarded during the v3-to-legacy mapping in Record.load() and `display()` printed the hard-coded "Password" string. Capture the label in Record.password_label during load() and use it in display() when set, falling back to "Password" so legacy/un-labeled records are unchanged. Update the supershell so its password-field detection (used for masking, audit-logged copy via ClipboardCommand, and the unmask toggle) matches both "Password" and any custom label set on the underlying type=password field. Without this the field would render correctly but lose unmask/copy semantics. Co-Authored-By: Claude Opus 4.7 (1M context) --- keepercommander/commands/supershell/app.py | 7 +++++-- keepercommander/record.py | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/keepercommander/commands/supershell/app.py b/keepercommander/commands/supershell/app.py index 87b74f783..bfe23f67e 100644 --- a/keepercommander/commands/supershell/app.py +++ b/keepercommander/commands/supershell/app.py @@ -586,6 +586,8 @@ def _load_vault_data(self): record_dict['password'] = field_value[0] elif isinstance(field_value, str): record_dict['password'] = field_value + if field_label: + record_dict['password_label'] = field_label # Extract login from typed field if not already set if field_type == 'login' and field_value and not record_dict.get('login'): @@ -2077,9 +2079,10 @@ def display_launch_section(): # Show 'app' for app records if type is blank display_type = value if value else 'app' if record_uid in self.app_record_uids else '' mount_line(f"[{t['text_dim']}]{key}:[/{t['text_dim']}] [{t['primary_dim']}]{rich_escape(str(display_type))}[/{t['primary_dim']}]", display_type) - elif key == 'Password': + elif key == 'Password' or (record_data.get('password_label') and key == record_data['password_label']): # Show masked password but use ClipboardCommand to copy (generates audit event) - # Respect unmask_secrets toggle + # Respect unmask_secrets toggle. Matches both the canonical "Password" label + # and any custom label (e.g. "Passphrase") set on a type=password field. if self.unmask_secrets: display_value = actual_password if actual_password else value else: diff --git a/keepercommander/record.py b/keepercommander/record.py index 472254e16..282ff99e5 100644 --- a/keepercommander/record.py +++ b/keepercommander/record.py @@ -86,6 +86,7 @@ def __init__(self, record_uid='', folder='', title='', login='', password='', lo self.unmasked_password = None self.totp = None self.version = 2 + self.password_label = '' # custom label on the v3 password field (e.g. "Passphrase") def load(self, data, **kwargs): self.version = kwargs.get('version', 2) @@ -129,6 +130,7 @@ def load(self, data, **kwargs): continue if field_type == 'password' and not self.password: self.password = field_value + self.password_label = field_label continue if field_type == 'url' and not self.login_url: self.login_url = field_value @@ -225,7 +227,8 @@ def display(self, unmask=False): if self.login: print('{0:>20s}: {1:<20s}'.format('Login', self.login)) if self.password: display_password = (self.unmasked_password or self.password) if unmask else '********' - print('{0:>20s}: {1:<20s}'.format('Password', display_password)) + password_label = self.password_label or 'Password' + print('{0:>20s}: {1:<20s}'.format(password_label, display_password)) if self.login_url: print('{0:>20s}: {1:<20s}'.format('URL', self.login_url)) # print('{0:>20s}: https://keepersecurity.com/vault#detail/{1}'.format('Link',self.record_uid)) From 6b2631abe332bbb70a1458d7abced44d2766e3f3 Mon Sep 17 00:00:00 2001 From: Craig Lurey Date: Fri, 22 May 2026 17:48:30 -0700 Subject: [PATCH 03/20] Extend custom-label fix to login, url, and oneTimeCode fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2078 fixed the symptom for type=password fields. The same label-discard pattern exists in Record.load() for login, url, and oneTimeCode fields, and customer vaults already contain records with custom labels on these types — importers from Thycotic and 1Password explicitly create them (see thycotic.py:526/636/680 for login/url/oneTimeCode, one_password.py :154/288 for url/login). Capture each field's label in self.login_label / self.url_label / self.totp_label during load(), and use them in display() with fall-back to the canonical "Login" / "URL" / "TOTP URL" labels so un-labeled and legacy v2 records are unchanged. Update the supershell parallel matchers: - Capture login_label and login_url_label into record_dict so the supershell knows the renamed field is still the canonical login/URL. - Extend the "URL" key matcher so the post-URL display_totp() call still fires when the URL field has a custom label. - "Login" has no special-case branch in the supershell text parser, so no matcher change is needed there — only the label capture. Co-Authored-By: Claude Opus 4.7 (1M context) --- keepercommander/commands/supershell/app.py | 9 +++++++-- keepercommander/record.py | 17 ++++++++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/keepercommander/commands/supershell/app.py b/keepercommander/commands/supershell/app.py index bfe23f67e..8275f40ab 100644 --- a/keepercommander/commands/supershell/app.py +++ b/keepercommander/commands/supershell/app.py @@ -595,6 +595,8 @@ def _load_vault_data(self): record_dict['login'] = field_value[0] elif isinstance(field_value, str): record_dict['login'] = field_value + if field_label: + record_dict['login_label'] = field_label # Extract URL from typed field if not already set if field_type == 'url' and field_value and not record_dict.get('login_url'): @@ -602,6 +604,8 @@ def _load_vault_data(self): record_dict['login_url'] = field_value[0] elif isinstance(field_value, str): record_dict['login_url'] = field_value + if field_label: + record_dict['login_url_label'] = field_label # Extract TOTP URL from oneTimeCode field if field_type == 'oneTimeCode' and field_value and not record_dict.get('totp_url'): @@ -2089,8 +2093,9 @@ def display_launch_section(): display_value = '******' if actual_password else value copy_value = actual_password if actual_password else None mount_line(f"[{t['text_dim']}]{key}:[/{t['text_dim']}] [{t['primary']}]{rich_escape(str(display_value))}[/{t['primary']}]", copy_value, is_password=True) - elif key == 'URL': - # Display URL, then TOTP if present + elif key == 'URL' or (record_data.get('login_url_label') and key == record_data['login_url_label']): + # Display URL, then TOTP if present. Matches both the canonical "URL" label + # and any custom label set on a type=url field. mount_line(f"[{t['text_dim']}]{key}:[/{t['text_dim']}] [{t['primary']}]{rich_escape(str(value))}[/{t['primary']}]", value) display_totp() # Add TOTP section right after URL (before Notes) elif key == 'Notes': diff --git a/keepercommander/record.py b/keepercommander/record.py index 282ff99e5..897180003 100644 --- a/keepercommander/record.py +++ b/keepercommander/record.py @@ -87,6 +87,9 @@ def __init__(self, record_uid='', folder='', title='', login='', password='', lo self.totp = None self.version = 2 self.password_label = '' # custom label on the v3 password field (e.g. "Passphrase") + self.login_label = '' # custom label on the v3 login field + self.url_label = '' # custom label on the v3 url field + self.totp_label = '' # custom label on the v3 oneTimeCode field def load(self, data, **kwargs): self.version = kwargs.get('version', 2) @@ -127,6 +130,7 @@ def load(self, data, **kwargs): if isinstance(field_value, str): if field_type == 'login' and not self.login: self.login = field_value + self.login_label = field_label continue if field_type == 'password' and not self.password: self.password = field_value @@ -134,9 +138,11 @@ def load(self, data, **kwargs): continue if field_type == 'url' and not self.login_url: self.login_url = field_value + self.url_label = field_label continue if field_type == 'oneTimeCode' and not self.totp: self.totp = field_value + self.totp_label = field_label continue if field_type: @@ -224,12 +230,16 @@ def display(self, unmask=False): print('{0:>20s}: {1:<20s}'.format('UID', self.record_uid)) print('{0:>20s}: {1:<20s}'.format('Type', self.record_type if self.record_type else '')) if self.title: print('{0:>20s}: {1:<20s}'.format('Title', self.title)) - if self.login: print('{0:>20s}: {1:<20s}'.format('Login', self.login)) + if self.login: + login_label = self.login_label or 'Login' + print('{0:>20s}: {1:<20s}'.format(login_label, self.login)) if self.password: display_password = (self.unmasked_password or self.password) if unmask else '********' password_label = self.password_label or 'Password' print('{0:>20s}: {1:<20s}'.format(password_label, display_password)) - if self.login_url: print('{0:>20s}: {1:<20s}'.format('URL', self.login_url)) + if self.login_url: + url_label = self.url_label or 'URL' + print('{0:>20s}: {1:<20s}'.format(url_label, self.login_url)) # print('{0:>20s}: https://keepersecurity.com/vault#detail/{1}'.format('Link',self.record_uid)) if len(self.custom_fields) > 0: @@ -311,7 +321,8 @@ def display(self, unmask=False): 'Attachments:' if i == 0 else '', atta.get('title') or atta.get('name'), sz, scale, 'ID', atta.get('id'))) if self.totp: - print('{0:>20s}: {1}'.format('TOTP URL', self.totp if unmask else '********')) + totp_label = self.totp_label or 'TOTP URL' + print('{0:>20s}: {1}'.format(totp_label, self.totp if unmask else '********')) code, remain, _ = get_totp_code(self.totp) if code: print('{0:>20s}: {1:<20s} valid for {2} sec'.format('Two Factor Code', code, remain)) From a561662a701b413c60aa19ed85e6503ea8682458 Mon Sep 17 00:00:00 2001 From: Craig Lurey Date: Fri, 22 May 2026 18:18:15 -0700 Subject: [PATCH 04/20] Fix: import --update duplicates Login records instead of updating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit prepare_record_add_or_update computes a partial-key hash (type + title + login + url) for each record in the vault and for each record being imported, and matches them to decide whether --update should rewrite an existing record vs. add a new one. For Login type records the hash from the vault side and from the CSV side diverged on the $url token, so no match was found and a duplicate was created. Root cause: convert_keeper_record refused to promote a typed url field to rec.login_url when the field carried any label. KC-1163 stamps the schema $ref as a default label on every standard field at write time (so KSM can resolve them), which means every Commander-imported login record has label='url' on its URL field. The label='url' default then blocked URL promotion on the next round-trip, so rec.login_url stayed None and the partial-hash $url token was empty. Relax the URL-promotion guard to accept the schema-default label (when label equals the field type) as canonical. A genuinely-labeled URL ("Backup URL", etc.) is still kept as a typed field so it doesn't shadow the canonical URL. Non-Login record types weren't affected because their partial hash walks rec.fields, where the URL ends up either way — both sides of the comparison see the same labeled field and hash identically. Co-Authored-By: Claude Opus 4.7 (1M context) --- keepercommander/importer/imp_exp.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/keepercommander/importer/imp_exp.py b/keepercommander/importer/imp_exp.py index 13b5bbe96..0ccec3cf0 100644 --- a/keepercommander/importer/imp_exp.py +++ b/keepercommander/importer/imp_exp.py @@ -205,7 +205,12 @@ def convert_keeper_record(record, has_attachments=False): rec.login = field_value elif field_type == 'password' and not rec.password and isinstance(field_value, str): rec.password = field_value - elif field_type == 'url' and not field.get('label') and not rec.login_url and isinstance(field_value, str): + elif field_type == 'url' and not rec.login_url and isinstance(field_value, str) and \ + (not field.get('label') or field.get('label') == field_type): + # Treat label='url' as canonical (KC-1163 stamps the type name as a + # default label on standard fields). Without this, the URL stored on + # imported login records is left out of rec.login_url, the partial + # hash diverges, and import --update creates duplicates. rec.login_url = field_value elif field_type.endswith('Ref'): ref_type = field_type[:-3] From 56188824d0ca54cd305b981ca7ac29f67861a980 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Mon, 25 May 2026 11:02:59 +0530 Subject: [PATCH 05/20] Add share-folder -roe and list-sf --roe-eligible flags (#2071) * Add sf -roe and lsf --roe-eligible flags * Fix unit test --- keepercommander/commands/record.py | 6 ++ keepercommander/commands/register.py | 86 ++++++++++++++++++---------- keepercommander/vault_extensions.py | 25 ++++++++ unit-tests/test_command_record.py | 15 +++++ unit-tests/test_command_register.py | 70 +++++++++++++++++++--- 5 files changed, 164 insertions(+), 38 deletions(-) diff --git a/keepercommander/commands/record.py b/keepercommander/commands/record.py index 7ae6a5229..a507c0bd6 100644 --- a/keepercommander/commands/record.py +++ b/keepercommander/commands/record.py @@ -165,6 +165,9 @@ def register_command_info(aliases, command_info): list_sf_parser = argparse.ArgumentParser(prog='list-sf', description='List all shared folders', parents=[base.report_output_parser]) +list_sf_parser.add_argument('--roe-eligible', dest='roe_eligible', action='store_true', + help='only list shared folders eligible for --rotate-on-expiration ' + '(contain at least one pamUser record with rotation configured)') list_sf_parser.add_argument('pattern', nargs='?', type=str, action='store', help='search pattern') @@ -1766,6 +1769,9 @@ def execute(self, params, **kwargs): fmt = kwargs.get('format', 'table') pattern = kwargs['pattern'] if 'pattern' in kwargs else None results = api.search_shared_folders(params, pattern or '') + if kwargs.get('roe_eligible'): + results = [sf for sf in results + if vault_extensions.shared_folder_has_pam_user_with_rotation(params, sf.shared_folder_uid)] if any(results): table = [] headers = ['shared_folder_uid', 'name'] if fmt == 'json' else ['Shared Folder UID', 'Name'] diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 08b381232..63f1e9fd7 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -32,7 +32,7 @@ from .helpers.timeout import parse_timeout from .record import RecordRemoveCommand from .utils import SyncDownCommand -from .. import api, utils, crypto, constants, rest_api, vault +from .. import api, utils, crypto, constants, rest_api, vault, vault_extensions from ..display import bcolors from ..error import KeeperApiError, CommandError, Error from ..loginv3 import LoginV3API @@ -89,8 +89,8 @@ def register_command_info(aliases, command_info): help='share expiration: never or period') share_record_parser.add_argument('-roe', '--rotate-on-expiration', dest='rotate_on_expiration', action='store_true', help='rotate the password when the share access expires. ' - 'Requires --action=grant, a positive --expire-at/--expire-in ' - '(not "never"), and a pamUser record.') + 'Only valid on grant; requires a positive --expire-at/--expire-in ' + '(not "never") and a pamUser record with rotation configured.') share_record_parser.add_argument('record', nargs='?', type=str, action='store', help='record/shared folder path/UID') share_folder_parser = argparse.ArgumentParser(prog='share-folder', description='Change the permissions of a shared folder') @@ -118,6 +118,11 @@ def register_command_info(aliases, command_info): help='share expiration: never or ISO datetime (yyyy-MM-dd[ hh:mm:ss])') expiration.add_argument('--expire-in', dest='expire_in', action='store', metavar='PERIOD', help='share expiration: never or period ([(y)ears|(mo)nths|(d)ays|(h)ours(mi)nutes]') +share_folder_parser.add_argument('-roe', '--rotate-on-expiration', dest='rotate_on_expiration', action='store_true', + help='rotate the password when the share access expires. ' + 'Only valid on grant; requires a positive --expire-at/--expire-in ' + '(not "never") and at least one pamUser record with rotation ' + 'configured in the folder.') share_folder_parser.add_argument('folder', nargs='+', type=str, action='store', help='shared folder path or UID') share_report_parser = argparse.ArgumentParser(prog='share-report', description='Generates a report of shared records', @@ -307,6 +312,25 @@ def get_share_admin_obj_uids(obj_names, obj_type): if action == 'grant': share_expiration = get_share_expiration(kwargs.get('expire_at'), kwargs.get('expire_in')) + rotate_on_expiration = bool(kwargs.get('rotate_on_expiration')) + if rotate_on_expiration: + if action != 'grant': + raise CommandError('share-folder', + '--rotate-on-expiration is only valid when granting access') + if not isinstance(share_expiration, int) or share_expiration <= 0: + raise CommandError('share-folder', + '--rotate-on-expiration requires a positive --expire-at / --expire-in ' + '(cannot be "never").') + ineligible_folders = [ + uid for uid in shared_folder_uids + if not vault_extensions.shared_folder_has_pam_user_with_rotation(params, uid) + ] + if ineligible_folders: + raise CommandError( + 'share-folder', + '--rotate-on-expiration requires the folder to contain at least one pamUser record ' + 'with rotation configured. Ineligible folder(s): ' + ', '.join(sorted(ineligible_folders))) + as_users = set() as_teams = set() @@ -368,7 +392,8 @@ def get_share_admin_obj_uids(obj_names, obj_type): def prep_rq(recs, users, curr_sf): return self.prepare_request(params, kwargs, curr_sf, users, sf_teams, recs, default_record=default_record, - default_account=default_account, share_expiration=share_expiration) + default_account=default_account, share_expiration=share_expiration, + rotate_on_expiration=rotate_on_expiration) for sf_uid in shared_folder_uids: sf_users = as_users.copy() @@ -419,7 +444,7 @@ def prep_rq(recs, users, curr_sf): @staticmethod def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, default_record=False, default_account=False, - share_expiration=None): + share_expiration=None, rotate_on_expiration=False): rq = folder_pb2.SharedFolderUpdateV3Request() rq.sharedFolderUid = utils.base64_url_decode(curr_sf['shared_folder_uid']) if 'revision' in curr_sf: @@ -429,6 +454,19 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, action = kwargs.get('action') or 'grant' mr = kwargs.get('manage_records') mu = kwargs.get('manage_users') + + def apply_share_expiration(target): + """Set expiration / timer / rotateOnExpiration on a User/Team/Record update proto.""" + if not isinstance(share_expiration, int): + return + if share_expiration > 0: + target.expiration = share_expiration * 1000 + target.timerNotificationType = record_pb2.NOTIFY_OWNER + if rotate_on_expiration: + target.rotateOnExpiration = True + elif share_expiration < 0: + target.expiration = -1 + if default_account and action == 'grant': if mr is not None: rq.defaultManageRecords = folder_pb2.BOOLEAN_TRUE if mr == 'on' else folder_pb2.BOOLEAN_FALSE @@ -444,12 +482,7 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, for email in users: uo = folder_pb2.SharedFolderUpdateUser() uo.username = email - if isinstance(share_expiration, int): - if share_expiration > 0: - uo.expiration = share_expiration * 1000 - uo.timerNotificationType = record_pb2.NOTIFY_OWNER - elif share_expiration < 0: - uo.expiration = -1 + apply_share_expiration(uo) if email in existing_users: if action == 'grant': uo.manageRecords = folder_pb2.BOOLEAN_NO_CHANGE if mr is None else folder_pb2.BOOLEAN_TRUE if mr == 'on' else folder_pb2.BOOLEAN_FALSE @@ -489,12 +522,7 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, for team_uid in teams: to = folder_pb2.SharedFolderUpdateTeam() to.teamUid = utils.base64_url_decode(team_uid) - if isinstance(share_expiration, int): - if share_expiration > 0: - to.expiration = share_expiration * 1000 - to.timerNotificationType = record_pb2.NOTIFY_OWNER - elif share_expiration < 0: - to.expiration = -1 + apply_share_expiration(to) if team_uid in existing_teams: team = existing_teams[team_uid] if action == 'grant': @@ -546,12 +574,7 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, for record_uid in rec_uids: ro = folder_pb2.SharedFolderUpdateRecord() ro.recordUid = utils.base64_url_decode(record_uid) - if isinstance(share_expiration, int): - if share_expiration > 0: - ro.expiration = share_expiration * 1000 - ro.timerNotificationType = record_pb2.NOTIFY_OWNER - elif share_expiration < 0: - ro.expiration = -1 + apply_share_expiration(ro) if record_uid in existing_records: if action == 'grant': @@ -562,8 +585,10 @@ def prepare_request(params, kwargs, curr_sf, users, teams, rec_uids, *, rq.sharedFolderRemoveRecord.append(ro.recordUid) else: if action == 'grant': - ro.canEdit = curr_sf.get('default_can_edit') is True if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE - ro.canShare = curr_sf.get('default_can_share') is True if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE + default_ce = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_edit') is True else folder_pb2.BOOLEAN_FALSE + default_cs = folder_pb2.BOOLEAN_TRUE if curr_sf.get('default_can_share') is True else folder_pb2.BOOLEAN_FALSE + ro.canEdit = default_ce if ce is None else folder_pb2.BOOLEAN_TRUE if ce == 'on' else folder_pb2.BOOLEAN_FALSE + ro.canShare = default_cs if cs is None else folder_pb2.BOOLEAN_TRUE if cs == 'on' else folder_pb2.BOOLEAN_FALSE sf_key = curr_sf.get('shared_folder_key_unencrypted') if sf_key: rec = params.record_cache[record_uid] @@ -671,7 +696,7 @@ def prep_request(params, kwargs): # type: (KeeperParams, Dict[str, Any]) -> Un if rotate_on_expiration: if action != 'grant': raise CommandError('share-record', - '--rotate-on-expiration is only valid with --action=grant') + '--rotate-on-expiration is only valid when granting access') if not isinstance(share_expiration, int) or share_expiration <= 0: raise CommandError('share-record', '--rotate-on-expiration requires a positive --expire-at / --expire-in ' @@ -763,15 +788,16 @@ def prep_request(params, kwargs): # type: (KeeperParams, Dict[str, Any]) -> Un raise CommandError('share-record', 'There are no records to share selected') if rotate_on_expiration: - non_pam_user_uids = [ + ineligible_record_uids = [ ruid for ruid in record_uids if getattr(vault.KeeperRecord.load(params, ruid), 'record_type', None) != 'pamUser' + or not vault_extensions.record_has_rotation_configured(params, ruid) ] - if non_pam_user_uids: + if ineligible_record_uids: raise CommandError( 'share-record', - '--rotate-on-expiration is only supported for pamUser records. ' - 'Not pamUser: ' + ', '.join(sorted(non_pam_user_uids))) + '--rotate-on-expiration requires a pamUser record with rotation configured. ' + 'Ineligible record(s): ' + ', '.join(sorted(ineligible_record_uids))) if action == 'owner' and len(emails) > 1: raise CommandError('share-record', 'You can transfer ownership to a single account only') diff --git a/keepercommander/vault_extensions.py b/keepercommander/vault_extensions.py index 7666a9a93..c13411f46 100644 --- a/keepercommander/vault_extensions.py +++ b/keepercommander/vault_extensions.py @@ -119,6 +119,31 @@ def matches_record(record, pattern, search_fields=None): # type: (vault.Keepe return False +def record_has_rotation_configured(params, record_uid): + # type: (KeeperParams, str) -> bool + """Return True if the record has a rotation configuration set up.""" + return record_uid in (params.record_rotation_cache or {}) + + +def shared_folder_has_pam_user_with_rotation(params, shared_folder_uid): + # type: (KeeperParams, str) -> bool + """Return True if the shared folder contains at least one pamUser record with rotation configured. + + Required precondition for the server to accept --rotate-on-expiration on a folder share. + """ + sf = params.shared_folder_cache.get(shared_folder_uid) + if not sf: + return False + for r in sf.get('records', []): + record_uid = r['record_uid'] + record = vault.KeeperRecord.load(params, record_uid) + if (record is not None + and record.record_type == 'pamUser' + and record_has_rotation_configured(params, record_uid)): + return True + return False + + def find_records(params, # type: KeeperParams search_str=None, # type: Optional[str] record_type=None, # type: Union[str, Iterable[str], None] diff --git a/unit-tests/test_command_record.py b/unit-tests/test_command_record.py index 18d213b8a..fc61833c9 100644 --- a/unit-tests/test_command_record.py +++ b/unit-tests/test_command_record.py @@ -177,6 +177,21 @@ def test_shared_list_command(self): cmd.execute(params, pattern='INVALID') mock_print.assert_not_called() + def test_shared_list_filters_by_roe_eligible(self): + params = get_synced_params() + cmd = record.RecordListSfCommand() + + with mock.patch('builtins.print') as mock_print: + cmd.execute(params, roe_eligible=True) + mock_print.assert_not_called() + + with mock.patch( + 'keepercommander.vault_extensions.shared_folder_has_pam_user_with_rotation', + return_value=True): + with mock.patch('builtins.print') as mock_print: + cmd.execute(params, roe_eligible=True) + mock_print.assert_called() + def test_team_list_command(self): params = get_synced_params() cmd = record.RecordListTeamCommand() diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 6e2a73b01..58d4998c3 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -1,3 +1,5 @@ +import datetime +from contextlib import contextmanager from unittest import TestCase, mock from data_vault import get_synced_params, VaultEnvironment @@ -64,18 +66,21 @@ def shared(params, record_uids, is_share_admin): cmd.execute(params, email=['user2@keepersecurity.com'], action='revoke', record=record_uid) self.assertEqual(len(TestRegister.expected_commands), 0) - def _patch_record_type_as_pam_user(self, target_uid): - """Patch vault.KeeperRecord.load so the given record reports record_type='pamUser'.""" + @contextmanager + def _make_record_rotation_eligible(self, params, target_uid): + """Present the record as a pamUser with rotation configured (ROE-eligible).""" from keepercommander import vault + params.record_rotation_cache[target_uid] = {'record_uid': target_uid, 'revision': 1} real_load = vault.KeeperRecord.load - def fake_load(params, record_uid, *args, **kwargs): - loaded = real_load(params, record_uid, *args, **kwargs) - if record_uid == target_uid and loaded is not None: + def fake_load(p, uid, *args, **kwargs): + loaded = real_load(p, uid, *args, **kwargs) + if uid == target_uid and loaded is not None: loaded.get_record_type = lambda: 'pamUser' return loaded - return mock.patch.object(vault.KeeperRecord, 'load', side_effect=fake_load) + with mock.patch.object(vault.KeeperRecord, 'load', side_effect=fake_load): + yield def _capture_shared_record_requests(self): """Patch the response-builder so we can assert on outbound SharedRecord protos.""" @@ -119,7 +124,7 @@ def test_share_record_rotate_on_expiration_sets_flag(self): cmd = register.ShareRecordCommand() TestRegister.expected_commands.extend(['records_share_update']) - with self._patch_record_type_as_pam_user(record_uid): + with self._make_record_rotation_eligible(params, record_uid): cmd.execute(params, email=['user2@keepersecurity.com'], action='grant', @@ -148,7 +153,7 @@ def test_share_record_rotate_on_expiration_sets_flag_on_existing_share(self): cmd = register.ShareRecordCommand() TestRegister.expected_commands.extend(['records_share_update']) - with self._patch_record_type_as_pam_user(record_uid): + with self._make_record_rotation_eligible(params, record_uid): cmd.execute(params, email=['user2@keepersecurity.com'], action='grant', @@ -294,6 +299,55 @@ def test_share_folder(self): cmd.execute(params, action='revoke', user=['user2@keepersecurity.com'], folder=shared_folder_uid) self.assertEqual(len(TestRegister.expected_commands), 0) + def test_share_folder_prepare_request_sets_rotate_on_expiration(self): + """SharedFolderUpdateUser/Team/Record all carry rotateOnExpiration when -roe is on.""" + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + record_uid = next(iter([x['record_uid'] for x in params.meta_data_cache.values() if x['can_share']])) + team_uid = utils.base64_url_encode(b'a' * 16) + + curr_sf = dict(params.shared_folder_cache[shared_folder_uid]) + curr_sf.setdefault('users', []) + curr_sf['teams'] = [{'team_uid': team_uid, 'manage_records': True, 'manage_users': True}] + curr_sf.setdefault('records', []) + + future_ts = int(datetime.datetime.now().timestamp()) + 86_400 + + params.key_cache['user2@keepersecurity.com'] = mock.MagicMock( + rsa=utils.base64_url_decode(vault_env.encoded_public_key), ec=None) + + rq = register.ShareFolderCommand.prepare_request( + params, + kwargs={'action': 'grant'}, + curr_sf=curr_sf, + users=['user2@keepersecurity.com'], + teams=[team_uid], + rec_uids=[record_uid], + share_expiration=future_ts, + rotate_on_expiration=True, + ) + + user_msgs = list(rq.sharedFolderAddUser) + list(rq.sharedFolderUpdateUser) + team_msgs = list(rq.sharedFolderAddTeam) + list(rq.sharedFolderUpdateTeam) + record_msgs = list(rq.sharedFolderAddRecord) + list(rq.sharedFolderUpdateRecord) + + for msgs, label in [(user_msgs, 'user'), (team_msgs, 'team'), (record_msgs, 'record')]: + self.assertTrue(msgs, f'expected at least one {label} proto on the wire') + for m in msgs: + self.assertTrue(m.rotateOnExpiration, + f'rotateOnExpiration must be set on every {label} proto') + self.assertGreater(m.expiration, 0) + self.assertEqual(m.timerNotificationType, record_pb2.NOTIFY_OWNER) + + def test_share_folder_rotate_on_expiration_rejects_folder_without_pam_user(self): + params = get_synced_params() + shared_folder_uid = next(iter(params.shared_folder_cache.keys())) + cmd = register.ShareFolderCommand() + with self.assertRaises(CommandError) as ctx: + cmd.execute(params, action='grant', user=['user2@keepersecurity.com'], + folder=shared_folder_uid, expire_in='1d', rotate_on_expiration=True) + self.assertIn('pamUser', str(ctx.exception)) + @staticmethod def record_share_rq_rs(rq): status = record_pb2.SharedRecordStatus() From b549adc71b7262dd7073c582e34d86941232d3df Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Mon, 25 May 2026 12:23:52 +0530 Subject: [PATCH 06/20] Add list-sf to integration command list --- .../service/commands/integrations/integration_setup_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keepercommander/service/commands/integrations/integration_setup_base.py b/keepercommander/service/commands/integrations/integration_setup_base.py index 5451e7088..504e6b1df 100644 --- a/keepercommander/service/commands/integrations/integration_setup_base.py +++ b/keepercommander/service/commands/integrations/integration_setup_base.py @@ -97,7 +97,7 @@ def get_commander_container_name(self) -> str: return f'keeper-service-{self.get_integration_name().lower()}' def get_service_commands(self) -> str: - return 'search,share-record,nsf-share-record,share-folder,nsf-share-folder,share-report,record-add,nsf-record-add,one-time-share,epm,pedm,device-approve,get,tree,server,sync-down' + return 'search,share-record,nsf-share-record,share-folder,nsf-share-folder,share-report,record-add,nsf-record-add,one-time-share,epm,pedm,device-approve,get,tree,server,sync-down,list-sf' # -- Parser (auto-built from name, cached per subclass) ---------- From 125f840e2e9ff38bef536c105cd56597fb65a2fc Mon Sep 17 00:00:00 2001 From: Sergey Kolupaev Date: Fri, 22 May 2026 19:00:47 -0700 Subject: [PATCH 07/20] Use platform user data forlder for storing config.json file --- keepercommander/utils.py | 39 ++++++++++++++++++++------------------- requirements.txt | 1 + setup.cfg | 1 + 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/keepercommander/utils.py b/keepercommander/utils.py index 3def9f50d..fd0ebfdde 100644 --- a/keepercommander/utils.py +++ b/keepercommander/utils.py @@ -38,29 +38,30 @@ def get_logger(): def get_default_path(override_path=None): """ Get the default path for Commander's data directory. - + Precedence order (highest to lowest): 1. override_path parameter (e.g., from CLI --data-dir) - auto-appends .keeper if needed 2. KEEPER_DATA_HOME environment variable - auto-appends .keeper if needed - 3. XDG_DATA_HOME/.keeper (XDG Base Directory Specification) - 4. HOME/.keeper (legacy default) - + 3. Legacy ~/.keeper directory if it already exists (backward compatibility) + 4. platformdirs.user_data_dir() + .keeper - platform-appropriate location for new installs + + The legacy check uses directory existence rather than an XDG environment + variable so the resolved path is the same whether Commander is launched + from an interactive shell or a context like `systemctl --user` that does + not inherit the user's environment. + Args: override_path (str, optional): Override path, typically from CLI argument - + Returns: Path: The path to use for Commander's data directory """ - import os - + import platformdirs + def ensure_keeper_suffix(path_str): - """Helper to ensure path ends with .keeper suffix""" - expanded_path = Path(os.path.expanduser(path_str)) - if expanded_path.name == '.keeper': - return expanded_path - else: - return expanded_path.joinpath('.keeper') - + expanded = Path(os.path.expanduser(path_str)) + return expanded if expanded.name == '.keeper' else expanded.joinpath('.keeper') + if override_path: default_path = ensure_keeper_suffix(override_path) else: @@ -68,12 +69,12 @@ def ensure_keeper_suffix(path_str): if keeper_data_home: default_path = ensure_keeper_suffix(keeper_data_home) else: - xdg_data_home = os.getenv('XDG_DATA_HOME') - if xdg_data_home: - default_path = Path(xdg_data_home).joinpath('.keeper') + legacy_path = Path.home().joinpath('.keeper') + if legacy_path.is_dir(): + default_path = legacy_path else: - default_path = Path.home().joinpath('.keeper') - + default_path = Path(platformdirs.user_data_dir()).joinpath('.keeper') + default_path.mkdir(parents=True, exist_ok=True) return default_path diff --git a/requirements.txt b/requirements.txt index 9aff49960..5264fa503 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,6 +21,7 @@ flask-limiter psutil python-dotenv keyring +platformdirs fpdf2>=2.8.3 cbor2; sys_platform == "darwin" and python_version>='3.10' pyobjc-framework-LocalAuthentication; sys_platform == "darwin" and python_version>='3.10' diff --git a/setup.cfg b/setup.cfg index 4fd1e78c6..dd9a215b7 100644 --- a/setup.cfg +++ b/setup.cfg @@ -49,6 +49,7 @@ install_requires = pyperclip python-dotenv keyring + platformdirs requests>=2.31.0 tabulate websockets From 7455baa4302f64a692aba12982f45880c8e1f4f8 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Tue, 26 May 2026 19:33:46 +0530 Subject: [PATCH 08/20] Restrict cross api-key usage in GET in service mode and prevent command injection via line break (#2058) (#2059) --- keepercommander/cli.py | 48 +++++- keepercommander/service/api/command.py | 31 ++-- keepercommander/service/core/request_queue.py | 153 +++++++++--------- unit-tests/service/test_api_routes.py | 4 +- 4 files changed, 139 insertions(+), 97 deletions(-) diff --git a/keepercommander/cli.py b/keepercommander/cli.py index 7418a0274..6edd16969 100644 --- a/keepercommander/cli.py +++ b/keepercommander/cli.py @@ -14,6 +14,7 @@ import os import re import shlex +import shutil import subprocess import sys import threading @@ -248,7 +249,15 @@ def command_and_args_from_cmd(command_line): return cmd, args +_DISALLOWED_COMMAND_CHARS = ('\r', '\n', '\x00') + + def do_command(params, command_line): + # Block control characters to prevent newline-injection at the shell boundary. + if isinstance(command_line, str) and any(ch in command_line for ch in _DISALLOWED_COMMAND_CHARS): + logging.error('Invalid command: control characters are not allowed in command input.') + return + def is_msp(params_local): if params_local.enterprise: if 'licenses' in params_local.enterprise: @@ -301,20 +310,43 @@ def is_msp(params_local): return if command_line.startswith('ksm'): - try: - subprocess.check_call([OS_WHICH_CMD, 'ksm'], stdout=subprocess.DEVNULL) - except subprocess.CalledProcessError: + # Exclude cwd from PATH on Windows so a local `ksm.bat` can't hijack + # the `ksm` prefix via CreateProcess's implicit cwd search. + search_path = os.environ.get('PATH', os.defpath) + if sys.platform.startswith('win'): + try: + cwd_abs = os.path.abspath(os.getcwd()) + except OSError: + cwd_abs = None + if cwd_abs: + search_path = os.pathsep.join( + p for p in search_path.split(os.pathsep) + if p and os.path.abspath(p) != cwd_abs + ) + + ksm_path = shutil.which('ksm', path=search_path) + if not ksm_path: logging.error( 'Please install the ksm application to run ksm commands.\n' 'See https://docs.keeper.io/secrets-manager/secrets-manager' '/secrets-manager-command-line-interface' '#secrets-manager-cli-installation' ) - else: - if sys.platform.startswith('win'): - subprocess.check_call(command_line) - else: - subprocess.check_call(shlex.split(command_line)) + return + + try: + ksm_args = shlex.split(command_line, posix=not sys.platform.startswith('win')) + except ValueError as e: + logging.error('Invalid ksm command: %s', e) + return + + if not ksm_args or ksm_args[0].lower() != 'ksm': + logging.error('Invalid ksm command.') + return + + # Invoke via argv list with shell=False so arguments aren't re-parsed. + ksm_args[0] = ksm_path + subprocess.check_call(ksm_args, shell=False) return elif '-h' in command_line.lower(): if command_line.lower().startswith('h ') or command_line.lower().startswith('history '): diff --git a/keepercommander/service/api/command.py b/keepercommander/service/api/command.py index 8092533f8..c80c6b1ea 100644 --- a/keepercommander/service/api/command.py +++ b/keepercommander/service/api/command.py @@ -15,10 +15,14 @@ from ..decorators.unified import unified_api_decorator from ..util.command_util import CommandExecutor from ..decorators.logging import logger -from ..core.request_queue import queue_manager +from ..core.request_queue import queue_manager, derive_owner_key from ..util.request_validation import RequestValidator +def _caller_owner_key(): + return derive_owner_key(request.headers.get('api-key')) + + def _prepare_command_request(): """Validate the request body and build the command to execute.""" json_error = RequestValidator.validate_request_json() @@ -36,8 +40,11 @@ def _prepare_command_request(): def _submit_queue_request(processed_command: str, temp_files: list, wait_for_completion: bool): """Submit a request to the queue, optionally waiting for the result.""" request_submitted = False + owner_key = _caller_owner_key() try: - request_id = queue_manager.submit_request(processed_command, temp_files) + request_id = queue_manager.submit_request( + processed_command, temp_files, owner_key=owner_key + ) request_submitted = True except queue.Full: RequestValidator.cleanup_temp_files(temp_files) @@ -47,7 +54,7 @@ def _submit_queue_request(processed_command: str, temp_files: list, wait_for_com }), 503), False if wait_for_completion: - result_data = queue_manager.wait_for_result(request_id) + result_data = queue_manager.wait_for_result(request_id, owner_key=owner_key) if result_data is None: return (jsonify({ "status": "error", @@ -136,9 +143,10 @@ def execute_command(**kwargs) -> Tuple[Response, int]: @bp.route("/status/", methods=["GET"]) @unified_api_decorator() def get_request_status(request_id: str, **kwargs) -> Tuple[Response, int]: - """Get the status of a specific request.""" + """Get status; only the submitting API key can read its own request.""" try: - status_info = queue_manager.get_request_status(request_id) + owner_key = _caller_owner_key() + status_info = queue_manager.get_request_status(request_id, owner_key=owner_key) if status_info is None: return jsonify({ "status": "error", @@ -158,12 +166,12 @@ def get_request_status(request_id: str, **kwargs) -> Tuple[Response, int]: @bp.route("/result/", methods=["GET"]) @unified_api_decorator() def get_request_result(request_id: str, **kwargs) -> Tuple[Union[Response, bytes], int]: - """Get the result of a completed request.""" + """Get result; only the submitting API key can read its own request.""" try: - result_data = queue_manager.get_request_result(request_id) + owner_key = _caller_owner_key() + result_data = queue_manager.get_request_result(request_id, owner_key=owner_key) if result_data is None: - # Check if request exists but isn't completed - status_info = queue_manager.get_request_status(request_id) + status_info = queue_manager.get_request_status(request_id, owner_key=owner_key) if status_info is None: return jsonify({ "status": "error", @@ -186,9 +194,10 @@ def get_request_result(request_id: str, **kwargs) -> Tuple[Union[Response, bytes @bp.route("/queue/status", methods=["GET"]) @unified_api_decorator() def get_queue_status(**kwargs) -> Tuple[Response, int]: - """Get overall queue status information.""" + """Get queue status, scoped to the caller's API key.""" try: - queue_status = queue_manager.get_queue_status() + owner_key = _caller_owner_key() + queue_status = queue_manager.get_queue_status(owner_key=owner_key) return jsonify({ "success": True, **queue_status diff --git a/keepercommander/service/core/request_queue.py b/keepercommander/service/core/request_queue.py index 7dad15e5a..3aed2816e 100644 --- a/keepercommander/service/core/request_queue.py +++ b/keepercommander/service/core/request_queue.py @@ -9,6 +9,8 @@ # Contact: ops@keepersecurity.com # +import hashlib +import hmac import queue import threading import time @@ -22,6 +24,13 @@ from ..decorators.logging import logger, debug_decorator +def derive_owner_key(api_key: Optional[str]) -> Optional[str]: + """Return a SHA-256 hash of the api-key, or None if none provided.""" + if not api_key: + return None + return hashlib.sha256(api_key.strip().encode('utf-8')).hexdigest() + + # Queue configuration constants DEFAULT_QUEUE_MAX_SIZE = 100 DEFAULT_REQUEST_TIMEOUT = 300 # 5 minutes in seconds @@ -51,10 +60,13 @@ class QueuedRequest: status_code: Optional[int] = None # HTTP status code from command execution error_message: Optional[str] = None temp_files: list = None # List of temporary file paths to clean up - + # Hashed api-key of the submitter; used to scope queue reads per caller. + owner_key: Optional[str] = None + def to_dict(self) -> Dict[str, Any]: """Convert request to dictionary for JSON serialization.""" data = asdict(self) + data.pop("owner_key", None) # Convert datetime objects to ISO strings and handle bytes objects for key, value in data.items(): if isinstance(value, datetime): @@ -121,26 +133,17 @@ def stop(self): logger.info("Request queue worker stopped") @debug_decorator - def submit_request(self, command: str, temp_files: list = None) -> str: - """Submit a new command request to the queue. - - Args: - command: The command string to execute - temp_files: List of temporary file paths to clean up after execution - - Returns: - str: Unique request ID - - Raises: - queue.Full: If the queue is at capacity - """ + def submit_request(self, command: str, temp_files: list = None, + owner_key: Optional[str] = None) -> str: + """Submit a request; ``owner_key`` scopes who can read it back.""" request_id = str(uuid.uuid4()) request = QueuedRequest( request_id=request_id, command=command, status=RequestStatus.QUEUED, created_at=datetime.now(), - temp_files=temp_files or [] + temp_files=temp_files or [], + owner_key=owner_key, ) try: @@ -153,71 +156,57 @@ def submit_request(self, command: str, temp_files: list = None) -> str: logger.error("Error: Request queue is full") raise + @staticmethod + def _is_owner(request: QueuedRequest, owner_key: Optional[str]) -> bool: + # Anonymous matches anonymous; otherwise require an exact owner match. + if request.owner_key is None and owner_key is None: + return True + if request.owner_key is None or owner_key is None: + return False + return hmac.compare_digest(request.owner_key, owner_key) + @debug_decorator - def get_request_status(self, request_id: str) -> Optional[Dict[str, Any]]: - """Get the status of a specific request. - - Args: - request_id: The unique request identifier - - Returns: - Dict containing request status and metadata, or None if not found - """ + def get_request_status(self, request_id: str, + owner_key: Optional[str] = None) -> Optional[Dict[str, Any]]: + """Return status for ``request_id`` only if owned by ``owner_key``.""" with self.data_lock: - # Check active requests - if request_id in self.active_requests: - return self.active_requests[request_id].to_dict() - - # Check completed requests - if request_id in self.completed_requests: - return self.completed_requests[request_id].to_dict() - - return None - + request = self.active_requests.get(request_id) \ + or self.completed_requests.get(request_id) + if request is None or not self._is_owner(request, owner_key): + return None + return request.to_dict() + @debug_decorator - def get_request_result(self, request_id: str) -> Optional[Tuple[Any, int]]: - """Get the result of a completed request. - - Args: - request_id: The unique request identifier - - Returns: - Tuple of (result, status_code) or None if not found/not completed - """ + def get_request_result(self, request_id: str, + owner_key: Optional[str] = None) -> Optional[Tuple[Any, int]]: + """Return result for ``request_id`` only if owned by ``owner_key``.""" with self.data_lock: - if request_id in self.completed_requests: - request = self.completed_requests[request_id] - if request.status == RequestStatus.COMPLETED: - status_code = request.status_code if request.status_code is not None else 200 - return request.result, status_code - elif request.status == RequestStatus.FAILED: - return {"error": request.error_message}, 500 - elif request.status == RequestStatus.EXPIRED: - return { - "error": request.error_message or "Error: Request expired before execution" - }, 504 + request = self.completed_requests.get(request_id) + if request is None or not self._is_owner(request, owner_key): + return None + if request.status == RequestStatus.COMPLETED: + status_code = request.status_code if request.status_code is not None else 200 + return request.result, status_code + elif request.status == RequestStatus.FAILED: + return {"error": request.error_message}, 500 + elif request.status == RequestStatus.EXPIRED: + return { + "error": request.error_message or "Error: Request expired before execution" + }, 504 return None @debug_decorator - def wait_for_result(self, request_id: str, timeout: Optional[float] = None) -> Optional[Tuple[Any, int]]: - """Wait synchronously for a queued request result. - - Args: - request_id: The unique request identifier - timeout: Maximum seconds to wait while the request remains queued. - Once processing starts, continue waiting for completion. - - Returns: - Tuple of (result, status_code), or None if the request vanished unexpectedly. - """ + def wait_for_result(self, request_id: str, timeout: Optional[float] = None, + owner_key: Optional[str] = None) -> Optional[Tuple[Any, int]]: + """Block until the request completes; only returns to its owner.""" deadline = time.monotonic() + (timeout if timeout is not None else self.request_timeout) while True: - result_data = self.get_request_result(request_id) + result_data = self.get_request_result(request_id, owner_key=owner_key) if result_data is not None: return result_data - status_info = self.get_request_status(request_id) + status_info = self.get_request_status(request_id, owner_key=owner_key) if status_info is None: return None @@ -268,18 +257,30 @@ def _expire_queued_request(self, request_id: str, error_message: str) -> bool: return True @debug_decorator - def get_queue_status(self) -> Dict[str, Any]: - """Get overall queue status information. - - Returns: - Dict containing queue statistics - """ + def get_queue_status(self, owner_key: Optional[str] = None) -> Dict[str, Any]: + """Return queue status scoped to ``owner_key``.""" with self.data_lock: + active_for_caller = sum( + 1 for r in self.active_requests.values() + if self._is_owner(r, owner_key) + ) + completed_for_caller = sum( + 1 for r in self.completed_requests.values() + if self._is_owner(r, owner_key) + ) + + current_id = self.current_request_id + if current_id is not None: + current_request = self.active_requests.get(current_id) \ + or self.completed_requests.get(current_id) + if current_request is None or not self._is_owner(current_request, owner_key): + current_id = None + return { "queue_size": self.request_queue.qsize(), - "active_requests": len(self.active_requests), - "completed_requests": len(self.completed_requests), - "currently_processing": self.current_request_id, + "active_requests": active_for_caller, + "completed_requests": completed_for_caller, + "currently_processing": current_id, "worker_running": self.is_running and self.worker_thread and self.worker_thread.is_alive() } diff --git a/unit-tests/service/test_api_routes.py b/unit-tests/service/test_api_routes.py index ec85eb4a3..758fcaec4 100644 --- a/unit-tests/service/test_api_routes.py +++ b/unit-tests/service/test_api_routes.py @@ -55,8 +55,8 @@ def test_v1_compatibility_route_waits_for_queue_result(self): self.assertEqual(response.status_code, 200) self.assertEqual(response.headers.get('X-API-Legacy'), 'true') self.assertEqual(response.get_json(), {"status": "success", "data": {"command": "ls"}}) - mock_submit.assert_called_once_with('ls', []) - mock_wait.assert_called_once_with('req-1') + mock_submit.assert_called_once_with('ls', [], owner_key=None) + mock_wait.assert_called_once_with('req-1', owner_key=None) def test_v1_direct_route_keeps_legacy_execution_path(self): app = Flask(__name__) From 3ee81d0afd3ba662548244a5bc1e532a0ed5ab90 Mon Sep 17 00:00:00 2001 From: Sergey Kolupaev Date: Tue, 26 May 2026 12:56:26 -0700 Subject: [PATCH 09/20] Release 18.0.4 --- keepercommander/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keepercommander/__init__.py b/keepercommander/__init__.py index d460533cc..38f11ce99 100644 --- a/keepercommander/__init__.py +++ b/keepercommander/__init__.py @@ -10,4 +10,4 @@ # Contact: commander@keepersecurity.com # -__version__ = '18.0.3' +__version__ = '18.0.4' From e7f442072093a0a99c564180e391065d5470fd6f Mon Sep 17 00:00:00 2001 From: lthievenaz-keeper Date: Mon, 25 May 2026 14:23:41 +0100 Subject: [PATCH 10/20] Correct the secret template character-limit in Thycotic import When importing secrets from Thycotic/Delinea, we may map secret templates to custom record types: - We make a list of all available record types - We check the secret's template type and if it's a match, we import it as that custom record type. However, since custom record type names have a character limit in Keeper, we have an additional logic: 1. Consider template name "This string is 33 characters long" 2. We truncate the name to its first 30 characters: "This string is 33 characters l", then compare with available custom record type names The problem with this logic is that custom record types have a 32-character limit in Keeper, not 30. As such the truncate in (2) is incorrect. Changed truncate action to 32 characters to match true custom record type limit. --- keepercommander/importer/thycotic/thycotic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index ccd5b9029..120480160 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -371,7 +371,7 @@ def do_import(self, filename, **kwargs): record.folders.append(record_folder) template_name = secret.get('secretTemplateName', '') - template_name = template_name[:30] + template_name = template_name[:32] if template_name in loaded_record_types: record.type = template_name elif template_name in ('Pin', 'Security Alarm Code'): From be354421bfda93f793b9e1eda459134693973c24 Mon Sep 17 00:00:00 2001 From: "Joao Paulo Oliveira Santos (JP)" Date: Thu, 21 May 2026 11:35:53 -0400 Subject: [PATCH 11/20] Add CNAPP integration commands and helpers - Introduced `PAMCnappCommand` to manage Cloud-Native Application Protection Platform (CNAPP) integration. - Added `cnapp_commands.py` for CNAPP user-facing commands, including configuration and queue management. - Implemented `cnapp_helper.py` for CNAPP REST endpoint interactions. - Created protocol buffer definitions in `cnapp_pb2.py` for CNAPP data structures. - Added unit tests for CNAPP commands and helpers in `test_cnapp.py` to ensure functionality and reliability. --- keepercommander/commands/discoveryrotation.py | 3 + .../commands/pam/cnapp_commands.py | 608 +++++++++++++ keepercommander/commands/pam/cnapp_helper.py | 241 +++++ keepercommander/proto/cnapp_pb2.py | 181 ++++ unit-tests/pam/test_cnapp.py | 837 ++++++++++++++++++ 5 files changed, 1870 insertions(+) create mode 100644 keepercommander/commands/pam/cnapp_commands.py create mode 100644 keepercommander/commands/pam/cnapp_helper.py create mode 100644 keepercommander/proto/cnapp_pb2.py create mode 100644 unit-tests/pam/test_cnapp.py diff --git a/keepercommander/commands/discoveryrotation.py b/keepercommander/commands/discoveryrotation.py index 621b40b05..2d1e67df4 100644 --- a/keepercommander/commands/discoveryrotation.py +++ b/keepercommander/commands/discoveryrotation.py @@ -89,6 +89,7 @@ from .pam_saas.config import PAMActionSaasConfigCommand from .pam_saas.update import PAMActionSaasUpdateCommand from .tunnel_and_connections import PAMTunnelCommand, PAMConnectionCommand, PAMRbiCommand, PAMSplitCommand +from .pam.cnapp_commands import PAMCnappCommand # These characters are based on the Vault PAM_DEFAULT_SPECIAL_CHAR = '''!@#$%^?();',.=+[]<>{}-_/\\*&:"`~|''' @@ -196,6 +197,8 @@ def __init__(self): self.register_command('workflow', PAMWorkflowCommand(), 'Manage PAM Workflows', 'w') self.register_command('access', PAMPrivilegedAccessCommand(), 'Manage privileged cloud access operations', 'ac') + self.register_command('cnapp', PAMCnappCommand(), + 'Manage Cloud-Native Application Protection Platform integration', 'cn') class PAMGatewayCommand(GroupCommand): diff --git a/keepercommander/commands/pam/cnapp_commands.py b/keepercommander/commands/pam/cnapp_commands.py new file mode 100644 index 000000000..ed0304c42 --- /dev/null +++ b/keepercommander/commands/pam/cnapp_commands.py @@ -0,0 +1,608 @@ +# _ __ +# | |/ /___ ___ _ __ ___ _ _ ® +# | ' bytes|None + """Encrypter keys are typically `openssl rand -base64 32`. Try standard base64 first, + then base64url (legacy notes). 16 or 32 bytes only — anything else is rejected so we + don't pass garbage to AES-GCM.""" + if not raw or not isinstance(raw, str): + return None + candidate = raw.strip() + for decoder in (base64.b64decode, base64.urlsafe_b64decode): + try: + padding = '=' * (-len(candidate) % 4) + data = decoder(candidate + padding) + except (binascii.Error, ValueError): + continue + if len(data) in (16, 32): + return data + return None + + +def _load_encrypter_key(params, config_record_uid): + """Resolve the AES key from the CNAPP encrypter vault record. Returns None when the + record can't be loaded or doesn't carry a recognizable key — callers should fall back + to showing the encrypted payload as-is.""" + if not config_record_uid: + return None + try: + record = vault.KeeperRecord.load(params, config_record_uid) + except Exception as e: + logging.debug('CNAPP: failed to load encrypter record %s: %s', config_record_uid, e) + return None + if not isinstance(record, vault.TypedRecord): + return None + # Match vault/cloudSecurityUtils.ts: prefer `secret` then `note` labeled "Encryption Key", + # then the first `note` field, then any record-level `note`. + candidates = [] + secret_field = record.get_typed_field('secret', CNAPP_ENCRYPTION_KEY_LABEL) + if secret_field and secret_field.value: + candidates.append(secret_field.value[0]) + note_labeled = record.get_typed_field('note', CNAPP_ENCRYPTION_KEY_LABEL) + if note_labeled and note_labeled.value: + candidates.append(note_labeled.value[0]) + first_note = record.get_typed_field('note') + if first_note and first_note.value: + candidates.append(first_note.value[0]) + for raw in candidates: + key = _decode_aes_key(raw) + if key: + return key + return None + + +def _decrypt_cnapp_payload(payload_bytes, key): + """Decrypt a CNAPP queue payload using the Encrypter's AES-256-GCM key. + + Wire format (matches vault's `decryptCnappQueueItem` in cloudSecurityUtils.ts): + payload_bytes (proto field, base64url-decoded by us) is UTF-8 base64url text + of a JSON envelope `{"encrypted_payload":"","alg":"AES-256-GCM","version":"1"}`. + encrypted_payload base64url-decodes to `nonce(12) || ciphertext || tag(16)` — + the standard layout AESGCM.decrypt expects. + + Returns a dict on success; raises Exception on bad envelope / wrong key / bad alg + so the caller can surface a meaningful warning.""" + from cryptography.hazmat.primitives.ciphers.aead import AESGCM + envelope_b64 = payload_bytes.decode('utf-8') + envelope_json = base64.urlsafe_b64decode(envelope_b64 + '=' * (-len(envelope_b64) % 4)) + envelope = json.loads(envelope_json) + if envelope.get('alg') and envelope['alg'] != 'AES-256-GCM': + raise ValueError(f"Unsupported CNAPP payload algorithm: {envelope['alg']}") + ciphertext_b64 = envelope.get('encrypted_payload') or '' + ciphertext = base64.urlsafe_b64decode(ciphertext_b64 + '=' * (-len(ciphertext_b64) % 4)) + if len(ciphertext) < 12 + 16: + raise ValueError('CNAPP ciphertext shorter than nonce+tag — corrupt payload') + nonce, body = ciphertext[:12], ciphertext[12:] + plaintext = AESGCM(key).decrypt(nonce, body, None) + return json.loads(plaintext.decode('utf-8')) + + +def _resolve_status(value): # type: (str) -> int + """Accept either the numeric status id or its case-insensitive name.""" + if value is None or value == '': + return 0 + if isinstance(value, int): + return value + s = str(value).strip().lower() + if s.isdigit(): + return int(s) + if s in QUEUE_STATUS_BY_NAME: + return QUEUE_STATUS_BY_NAME[s] + raise CommandError('pam cnapp', f"Unknown status '{value}'. Valid: {', '.join(QUEUE_STATUS_BY_NAME)} or 0 for ALL.") + + +def _format_timestamp(epoch_ms): + """krouter emits epoch-millis for received/resolved timestamps; render as UTC ISO.""" + if not epoch_ms: + return '' + try: + return datetime.fromtimestamp(int(epoch_ms) / 1000, tz=timezone.utc).isoformat() + except (ValueError, TypeError, OSError): + return str(epoch_ms) + + +class PAMCnappCommand(GroupCommand): + """Root for the `pam cnapp ...` command tree.""" + + def __init__(self): + super(PAMCnappCommand, self).__init__() + self.register_command('config', PAMCnappConfigCommand(), + 'Manage CNAPP provider configuration', 'c') + self.register_command('queue', PAMCnappQueueCommand(), + 'Manage CNAPP issue queue', 'q') + self.default_verb = 'queue' + + +# --------------------------------------------------------------------------- +# Configuration sub-tree +# --------------------------------------------------------------------------- + +class PAMCnappConfigCommand(GroupCommand): + + def __init__(self): + super(PAMCnappConfigCommand, self).__init__() + self.register_command('set', PAMCnappConfigSetCommand(), + 'Create or update CNAPP provider configuration') + self.register_command('test', PAMCnappConfigTestCommand(), + 'Validate CNAPP provider credentials without saving') + self.register_command('test-encrypter', PAMCnappConfigTestEncrypterCommand(), + 'Health-check the customer Encrypter at /health') + self.register_command('read', PAMCnappConfigReadCommand(), + 'Read the persisted CNAPP configuration for a network') + self.register_command('delete', PAMCnappConfigDeleteCommand(), + 'Delete the CNAPP configuration on a network') + self.default_verb = 'read' + + +def _add_configuration_args(parser, require_secret=True): + parser.add_argument('--network-uid', '-n', required=True, dest='network_uid', + help='Network record UID (base64url).') + parser.add_argument('--provider', '-p', required=True, dest='provider', + help='CNAPP provider keyword: wiz (case-insensitive).') + parser.add_argument('--client-id', required=True, dest='client_id', + help='Provider API client ID / app ID.') + parser.add_argument('--client-secret', required=require_secret, dest='client_secret', + help='Provider API client secret. Pass empty on `config set` to keep the existing secret.') + parser.add_argument('--api-endpoint', required=True, dest='api_endpoint_url', + help='Provider API endpoint URL (e.g. https://api.us1.app.wiz.io/graphql).') + + +class PAMCnappConfigSetCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp config set') + _add_configuration_args(parser, require_secret=True) + parser.add_argument('--config-record', required=True, dest='cnapp_config_record_uid', + help='UID of the vault record holding the Encrypter URL + key.') + + def get_parser(self): + return PAMCnappConfigSetCommand.parser + + def execute(self, params, **kwargs): + provider = cnapp_helper.provider_from_name(kwargs.get('provider')) + response = cnapp_helper.set_cnapp_configuration( + params, + network_uid=kwargs.get('network_uid'), + provider=provider, + client_id=kwargs.get('client_id'), + client_secret=kwargs.get('client_secret') or '', + api_endpoint_url=kwargs.get('api_endpoint_url'), + cnapp_config_record_uid=kwargs.get('cnapp_config_record_uid'), + ) + print(f"{bcolors.OKGREEN}CNAPP configuration saved.{bcolors.ENDC}") + if response is not None: + _print_configuration(response) + return None + + +class PAMCnappConfigTestCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp config test') + _add_configuration_args(parser, require_secret=True) + + def get_parser(self): + return PAMCnappConfigTestCommand.parser + + def execute(self, params, **kwargs): + provider = cnapp_helper.provider_from_name(kwargs.get('provider')) + cnapp_helper.test_cnapp_configuration( + params, + network_uid=kwargs.get('network_uid'), + provider=provider, + client_id=kwargs.get('client_id'), + client_secret=kwargs.get('client_secret'), + api_endpoint_url=kwargs.get('api_endpoint_url'), + ) + print(f"{bcolors.OKGREEN}CNAPP credentials validated successfully.{bcolors.ENDC}") + + +class PAMCnappConfigTestEncrypterCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp config test-encrypter') + parser.add_argument('--url', '-u', required=True, dest='url', + help='Base URL of the Encrypter. krouter probes /health.') + + def get_parser(self): + return PAMCnappConfigTestEncrypterCommand.parser + + def execute(self, params, **kwargs): + cnapp_helper.test_cnapp_encrypter(params, url_base_encrypter=kwargs.get('url')) + print(f"{bcolors.OKGREEN}Encrypter is reachable.{bcolors.ENDC}") + + +class PAMCnappConfigReadCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp config read') + parser.add_argument('--network-uid', '-n', required=True, dest='network_uid', + help='Network record UID (base64url).') + parser.add_argument('--provider', '-p', required=True, dest='provider', + help='CNAPP provider keyword: wiz.') + parser.add_argument('--format', dest='format', choices=['table', 'json'], default='table', + help='Output format.') + + def get_parser(self): + return PAMCnappConfigReadCommand.parser + + def execute(self, params, **kwargs): + provider = cnapp_helper.provider_from_name(kwargs.get('provider')) + response = cnapp_helper.read_cnapp_configuration( + params, + network_uid=kwargs.get('network_uid'), + provider=provider, + ) + if response is None: + logging.warning('No CNAPP configuration returned.') + return None + if kwargs.get('format') == 'json': + print(json.dumps(_configuration_to_dict(response), indent=2)) + return None + _print_configuration(response) + return None + + +class PAMCnappConfigDeleteCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp config delete') + parser.add_argument('--network-uid', '-n', required=True, dest='network_uid', + help='Network record UID (base64url).') + + def get_parser(self): + return PAMCnappConfigDeleteCommand.parser + + def execute(self, params, **kwargs): + cnapp_helper.delete_cnapp_configuration(params, network_uid=kwargs.get('network_uid')) + print(f"{bcolors.OKGREEN}CNAPP configuration deleted.{bcolors.ENDC}") + + +# --------------------------------------------------------------------------- +# Queue sub-tree +# --------------------------------------------------------------------------- + +class PAMCnappQueueCommand(GroupCommand): + + def __init__(self): + super(PAMCnappQueueCommand, self).__init__() + self.register_command('list', PAMCnappQueueListCommand(), 'List CNAPP queue items', 'l') + self.register_command('associate', PAMCnappQueueAssociateCommand(), + 'Attach a vault record to a queue item', 'a') + self.register_command('remediate', PAMCnappQueueRemediateCommand(), + 'Trigger a remediation action against the gateway', 'r') + self.register_command('set-status', PAMCnappQueueSetStatusCommand(), + 'Update local queue item status (notifies provider best-effort)', 's') + self.register_command('delete', PAMCnappQueueDeleteCommand(), 'Delete a queue item', 'd') + self.default_verb = 'list' + + +class PAMCnappQueueListCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp queue list') + parser.add_argument('--network-uid', '-n', required=True, dest='network_uid', + help='Network record UID (base64url).') + parser.add_argument('--status', '-s', required=False, dest='status', default=0, + help='Filter by status name or id (pending/in_progress/resolved/failed/cancelled). Default: all.') + parser.add_argument('--provider', '-p', required=False, dest='provider', default='wiz', + help='CNAPP provider keyword for the config lookup (default: wiz).') + parser.add_argument('--config-record', required=False, dest='config_record_uid', + help='Explicit encrypter vault record UID. Overrides the lookup via `config read`.') + parser.add_argument('--no-decrypt', dest='no_decrypt', action='store_true', + help="Skip payload decryption — show the raw encrypted envelope's metadata only.") + parser.add_argument('--format', dest='format', choices=['table', 'json'], default='table', + help='Output format. Table and JSON are mutually exclusive.') + + def get_parser(self): + return PAMCnappQueueListCommand.parser + + def _resolve_encrypter_key(self, params, kwargs): + """Resolve the AES key: --config-record wins; otherwise fetch `config read` to get + the cnappConfigRecordUid and load the encrypter record from the local vault.""" + if kwargs.get('no_decrypt'): + return None, None + config_record_uid = kwargs.get('config_record_uid') + if not config_record_uid: + try: + provider = cnapp_helper.provider_from_name(kwargs.get('provider') or 'wiz') + config = cnapp_helper.read_cnapp_configuration( + params, network_uid=kwargs.get('network_uid'), provider=provider) + except Exception as e: + logging.debug('CNAPP: could not read configuration for decryption: %s', e) + return None, None + if config is None or not config.cnappConfigRecordUid: + return None, None + config_record_uid = bytes_to_base64(config.cnappConfigRecordUid) + key = _load_encrypter_key(params, config_record_uid) + return key, config_record_uid + + @staticmethod + def _decrypted_summary(decrypted): + """Compact human-readable summary for the table column. Mirrors the columns the + vault Cloud Security view shows: severity, title, resource.""" + if not isinstance(decrypted, dict): + return '' + issue = decrypted.get('issue') or {} + resource = decrypted.get('resource') or {} + control = decrypted.get('control') or {} + bits = [] + sev = issue.get('severity') + if sev: + bits.append(str(sev).upper()) + title = control.get('name') or issue.get('id') + if title: + bits.append(str(title)) + resource_name = resource.get('name') or resource.get('id') + if resource_name: + bits.append(f"on {resource_name}") + return ' · '.join(bits) + + def execute(self, params, **kwargs): + status_filter = _resolve_status(kwargs.get('status')) + response = cnapp_helper.list_cnapp_queue( + params, + network_uid=kwargs.get('network_uid'), + status_filter=status_filter, + ) + items = list(response.items) if response is not None else [] + has_more = bool(response.hasMore) if response is not None else False + + encrypter_key, encrypter_uid = self._resolve_encrypter_key(params, kwargs) + decrypted_by_id = {} + decrypt_errors = [] + if encrypter_key: + for item in items: + if not item.payload: + continue + try: + decrypted_by_id[item.cnappQueueId] = _decrypt_cnapp_payload(item.payload, encrypter_key) + except Exception as e: + decrypt_errors.append((item.cnappQueueId, str(e))) + + if kwargs.get('format') == 'json': + json_items = [] + for item in items: + d = _queue_item_to_dict(item) + d.pop('payload', None) + if item.cnappQueueId in decrypted_by_id: + d['decryptedPayload'] = decrypted_by_id[item.cnappQueueId] + json_items.append(d) + payload = {'items': json_items, 'hasMore': has_more} + print(json.dumps(payload, indent=2, default=str)) + return None + + if not items: + print('No CNAPP queue items.') + return None + + if encrypter_key is None and not kwargs.get('no_decrypt'): + print(f"{bcolors.WARNING}No encrypter key resolved — payloads will be shown as 'encrypted'. " + f"Pass --config-record or run after `pam cnapp config read` succeeds.{bcolors.ENDC}") + + headers = ['Queue ID', 'Provider', 'Status', 'Received (UTC)', 'Resolved (UTC)', 'Record UID', 'Issue'] + rows = [] + for item in items: + if item.cnappQueueId in decrypted_by_id: + issue_cell = self._decrypted_summary(decrypted_by_id[item.cnappQueueId]) + elif not item.payload: + issue_cell = '' + else: + issue_cell = f"{bcolors.WARNING}{bcolors.ENDC}" + rows.append([ + item.cnappQueueId, + cnapp_helper.CnappProvider.Name(item.cnappProviderId), + QUEUE_STATUS_BY_ID.get(item.cnappQueueStatusId, str(item.cnappQueueStatusId)), + _format_timestamp(item.receivedAt), + _format_timestamp(item.resolvedAt), + bytes_to_base64(item.recordUid) if item.recordUid else '', + issue_cell, + ]) + dump_report_data(rows, headers, fmt='table', filename='', row_number=False) + for queue_id, msg in decrypt_errors: + print(f"{bcolors.WARNING}Queue item {queue_id}: failed to decrypt payload ({msg}).{bcolors.ENDC}") + if has_more: + print(f"{bcolors.WARNING}More items available — narrow with --status to page.{bcolors.ENDC}") + return None + + +class PAMCnappQueueAssociateCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp queue associate') + parser.add_argument('--queue-id', '-q', required=True, type=int, dest='cnapp_queue_id', + help='Queue item ID (from `pam cnapp queue list`).') + parser.add_argument('--record-uid', '-r', required=True, dest='record_uid', + help='Vault record UID to associate (base64url).') + + def get_parser(self): + return PAMCnappQueueAssociateCommand.parser + + def execute(self, params, **kwargs): + cnapp_helper.associate_cnapp_record( + params, + cnapp_queue_id=kwargs.get('cnapp_queue_id'), + record_uid=kwargs.get('record_uid'), + ) + print(f"{bcolors.OKGREEN}Record associated with queue item {kwargs.get('cnapp_queue_id')}.{bcolors.ENDC}") + + +class PAMCnappQueueRemediateCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp queue remediate') + parser.add_argument('--queue-id', '-q', required=True, type=int, dest='cnapp_queue_id', + help='Queue item ID.') + parser.add_argument('--action', '-a', required=True, dest='action_type', + help='Remediation action: rotate_credentials, manage_access, jit_access, remove_standing_privilege.') + parser.add_argument('--provider', '-p', required=False, dest='provider', + help='Provider keyword (wiz). Optional — krouter resolves from queue item if omitted.') + parser.add_argument('--config-record', required=False, dest='cnapp_config_record_uid', + help='Configuration record UID (only required for some action types).') + parser.add_argument('--resource-ref', required=False, dest='resource_ref', + help='Resource reference UID for the action.') + parser.add_argument('--pwd-complexity', required=False, dest='pwd_complexity', + help='Password complexity JSON (rotate_credentials).') + parser.add_argument('--controller-uid', required=False, dest='controller_uid', + help='Override gateway UID.') + parser.add_argument('--message-uid', required=False, dest='message_uid', + help='Client-generated conversation UID for streaming responses.') + parser.add_argument('--group-name', required=False, dest='group_name', + help='Group name (remove_standing_privilege only).') + + def get_parser(self): + return PAMCnappQueueRemediateCommand.parser + + def execute(self, params, **kwargs): + action = cnapp_helper.action_from_name(kwargs.get('action_type')) + provider = None + if kwargs.get('provider'): + provider = cnapp_helper.provider_from_name(kwargs.get('provider')) + response = cnapp_helper.remediate_cnapp_queue_item( + params, + cnapp_queue_id=kwargs.get('cnapp_queue_id'), + action_type=action, + provider=provider, + cnapp_config_record_uid=kwargs.get('cnapp_config_record_uid'), + resource_ref=kwargs.get('resource_ref'), + pwd_complexity=kwargs.get('pwd_complexity'), + controller_uid=kwargs.get('controller_uid'), + message_uid=kwargs.get('message_uid'), + group_name=kwargs.get('group_name'), + ) + if response is None: + print(f"{bcolors.OKGREEN}Remediation dispatched.{bcolors.ENDC}") + return None + action_name = cnapp_helper.CnappRemediationAction.Name(response.actionType) + status_name = QUEUE_STATUS_BY_ID.get(response.cnappQueueStatusId, str(response.cnappQueueStatusId)) + print(f"{bcolors.OKGREEN}Remediation dispatched.{bcolors.ENDC}") + print(f" Action: {action_name}") + print(f" Status: {status_name}") + if response.result: + print(f" Result: {response.result}") + return None + + +class PAMCnappQueueSetStatusCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp queue set-status') + parser.add_argument('--queue-id', '-q', required=True, type=int, dest='cnapp_queue_id', + help='Queue item ID.') + parser.add_argument('--status', '-s', required=True, dest='status', + help='New status: pending/in_progress/resolved/failed/cancelled, or its numeric id.') + parser.add_argument('--reason', required=False, dest='reason', + help='Free-form reason (forwarded to provider notification).') + + def get_parser(self): + return PAMCnappQueueSetStatusCommand.parser + + def execute(self, params, **kwargs): + status_id = _resolve_status(kwargs.get('status')) + if status_id == 0: + raise CommandError('pam cnapp queue set-status', 'A specific status is required (cannot be 0/ALL).') + response = cnapp_helper.set_cnapp_queue_status( + params, + cnapp_queue_id=kwargs.get('cnapp_queue_id'), + cnapp_queue_status_id=status_id, + reason=kwargs.get('reason'), + ) + applied = response.cnappQueueStatusId if response is not None else status_id + print(f"{bcolors.OKGREEN}Status applied: {QUEUE_STATUS_BY_ID.get(applied, applied)}.{bcolors.ENDC}") + return None + + +class PAMCnappQueueDeleteCommand(Command): + parser = argparse.ArgumentParser(prog='pam cnapp queue delete') + parser.add_argument('--queue-id', '-q', required=True, type=int, dest='cnapp_queue_id', + help='Queue item ID to delete.') + + def get_parser(self): + return PAMCnappQueueDeleteCommand.parser + + def execute(self, params, **kwargs): + cnapp_helper.delete_cnapp_queue_item(params, cnapp_queue_id=kwargs.get('cnapp_queue_id')) + print(f"{bcolors.OKGREEN}Queue item {kwargs.get('cnapp_queue_id')} deleted.{bcolors.ENDC}") + + +# --------------------------------------------------------------------------- +# Formatting helpers +# --------------------------------------------------------------------------- + +def _configuration_to_dict(config): + return { + 'networkUid': bytes_to_base64(config.networkUid) if config.networkUid else '', + 'provider': cnapp_helper.CnappProvider.Name(config.provider), + 'clientId': config.clientId, + 'apiEndpointUrl': config.apiEndpointUrl, + 'cnappConfigRecordUid': bytes_to_base64(config.cnappConfigRecordUid) if config.cnappConfigRecordUid else '', + } + + +def _queue_item_to_dict(item): + return { + 'cnappQueueId': item.cnappQueueId, + 'cnappProviderId': cnapp_helper.CnappProvider.Name(item.cnappProviderId), + 'cnappQueueStatusId': item.cnappQueueStatusId, + 'cnappQueueStatusName': QUEUE_STATUS_BY_ID.get(item.cnappQueueStatusId, str(item.cnappQueueStatusId)), + 'receivedAt': item.receivedAt, + 'resolvedAt': item.resolvedAt, + 'networkId': bytes_to_base64(item.networkId) if item.networkId else '', + 'recordUid': bytes_to_base64(item.recordUid) if item.recordUid else '', + } + + +def _print_configuration(config): + print(f"{bcolors.OKBLUE}CNAPP Configuration{bcolors.ENDC}") + print(f" Network UID : {bytes_to_base64(config.networkUid) if config.networkUid else ''}") + print(f" Provider : {cnapp_helper.CnappProvider.Name(config.provider)}") + print(f" Client ID : {config.clientId}") + print(f" API Endpoint : {config.apiEndpointUrl}") + if config.cnappConfigRecordUid: + print(f" Config Record : {bytes_to_base64(config.cnappConfigRecordUid)}") diff --git a/keepercommander/commands/pam/cnapp_helper.py b/keepercommander/commands/pam/cnapp_helper.py new file mode 100644 index 000000000..7171ceb12 --- /dev/null +++ b/keepercommander/commands/pam/cnapp_helper.py @@ -0,0 +1,241 @@ +# _ __ +# | |/ /___ ___ _ __ ___ _ _ ® +# | ' set_cnapp_configuration + configuration/test -> test_cnapp_configuration + configuration/test-encrypter -> test_cnapp_encrypter + configuration/read -> read_cnapp_configuration + configuration/delete -> delete_cnapp_configuration + + Queue: + queue -> list_cnapp_queue + queue/associate -> associate_cnapp_record + queue/remediate -> remediate_cnapp_queue_item + queue/set-status -> set_cnapp_queue_status + queue/delete -> delete_cnapp_queue_item + +Failures from the helper layer bubble up as Python exceptions raised by the underlying +HTTP/proto plumbing; callers convert them to user-readable output. +""" + +from typing import Optional + +from keeper_secrets_manager_core.utils import url_safe_str_to_bytes + +from .router_helper import _post_request_to_router +from ...params import KeeperParams +from ...proto import cnapp_pb2 + + +# Public re-exports — let commands/tests reach proto types via the helper module so they +# don't need to know the on-disk proto path. +CnappProvider = cnapp_pb2.CnappProvider +CnappRemediationAction = cnapp_pb2.CnappRemediationAction + + +# --------------------------------------------------------------------------- +# Conversion utilities +# --------------------------------------------------------------------------- + +def _to_uid_bytes(uid): # type: (Optional[str]) -> bytes + """Convert a base64url-encoded UID string to bytes; empty/None -> empty bytes.""" + if not uid: + return b'' + if isinstance(uid, bytes): + return uid + return url_safe_str_to_bytes(uid) + + +def provider_from_name(name): # type: (str) -> int + """Resolve a human-typed provider name (e.g. "wiz") to a CnappProvider enum value. + + Accepts the bare provider keyword ("wiz") or the full proto symbol + ("CNAPP_PROVIDER_WIZ"); case-insensitive. Raises ValueError on unknown input.""" + if not name: + return cnapp_pb2.CNAPP_PROVIDER_UNSPECIFIED + normalized = name.strip().upper() + if not normalized.startswith('CNAPP_PROVIDER_'): + normalized = 'CNAPP_PROVIDER_' + normalized + try: + return cnapp_pb2.CnappProvider.Value(normalized) + except ValueError as e: + valid = [n for n in cnapp_pb2.CnappProvider.keys() if n != 'CNAPP_PROVIDER_UNSPECIFIED'] + raise ValueError(f"Unknown CNAPP provider '{name}'. Valid options: {', '.join(valid)}") from e + + +def action_from_name(name): # type: (str) -> int + """Resolve a remediation action name to its enum int. Case-insensitive; accepts the + short keyword (e.g. "rotate_credentials") or the full proto symbol.""" + if not name: + return cnapp_pb2.UNSPECIFIED + normalized = name.strip().upper().replace('-', '_') + try: + return cnapp_pb2.CnappRemediationAction.Value(normalized) + except ValueError as e: + valid = [n for n in cnapp_pb2.CnappRemediationAction.keys() if n != 'UNSPECIFIED'] + raise ValueError(f"Unknown remediation action '{name}'. Valid options: {', '.join(valid)}") from e + + +# --------------------------------------------------------------------------- +# Configuration endpoints +# --------------------------------------------------------------------------- + +def _build_configuration(network_uid, provider, client_id=None, client_secret=None, + api_endpoint_url=None, cnapp_config_record_uid=None): + # type: (str, int, Optional[str], Optional[str], Optional[str], Optional[str]) -> cnapp_pb2.CnappConfiguration + rq = cnapp_pb2.CnappConfiguration() + rq.networkUid = _to_uid_bytes(network_uid) + rq.provider = provider + if client_id: + rq.clientId = client_id + if client_secret: + rq.clientSecret = client_secret + if api_endpoint_url: + rq.apiEndpointUrl = api_endpoint_url + if cnapp_config_record_uid: + rq.cnappConfigRecordUid = _to_uid_bytes(cnapp_config_record_uid) + return rq + + +def set_cnapp_configuration(params, network_uid, provider, client_id, client_secret, + api_endpoint_url, cnapp_config_record_uid): + # type: (KeeperParams, str, int, str, str, str, str) -> cnapp_pb2.CnappConfiguration + """Create or update the CNAPP provider configuration on a network. + + krouter validates the credentials against the provider before persisting; an empty + `client_secret` tells krouter to keep the previously stored value (useful for edits + that only change the endpoint or record UID).""" + rq = _build_configuration(network_uid, provider, client_id, client_secret, + api_endpoint_url, cnapp_config_record_uid) + return _post_request_to_router(params, 'cnapp/configuration/set', rq_proto=rq, + rs_type=cnapp_pb2.CnappConfiguration) + + +def test_cnapp_configuration(params, network_uid, provider, client_id, client_secret, + api_endpoint_url): + # type: (KeeperParams, str, int, str, str, str) -> None + """Probe the provider with the supplied credentials without persisting anything. + + Returns None on success; raises on validation failure (RRC_BAD_REQUEST with the + provider's reason in the message).""" + rq = _build_configuration(network_uid, provider, client_id, client_secret, + api_endpoint_url, cnapp_config_record_uid=None) + return _post_request_to_router(params, 'cnapp/configuration/test', rq_proto=rq) + + +def test_cnapp_encrypter(params, url_base_encrypter): + # type: (KeeperParams, str) -> None + """Issue a `GET /health` against the customer-deployed Encrypter via krouter. + + Used by the UI/CLI to check that the Encrypter URL is reachable before saving a + configuration that references it. Raises on non-200 or transport error.""" + rq = cnapp_pb2.CnappTestEncrypterRequest() + rq.urlBaseEncrypter = url_base_encrypter + return _post_request_to_router(params, 'cnapp/configuration/test-encrypter', rq_proto=rq) + + +def read_cnapp_configuration(params, network_uid, provider): + # type: (KeeperParams, str, int) -> cnapp_pb2.CnappConfiguration + """Read the persisted CNAPP configuration for a network. Note: krouter never returns + the `clientSecret` field — only the endpoint, client id and config record UID.""" + rq = _build_configuration(network_uid, provider) + return _post_request_to_router(params, 'cnapp/configuration/read', rq_proto=rq, + rs_type=cnapp_pb2.CnappConfiguration) + + +def delete_cnapp_configuration(params, network_uid): + # type: (KeeperParams, str) -> None + """Remove the CNAPP configuration on a network. Raises RRC_BAD_STATE if none exists.""" + rq = cnapp_pb2.CnappDeleteConfigurationRequest() + rq.networkUid = _to_uid_bytes(network_uid) + return _post_request_to_router(params, 'cnapp/configuration/delete', rq_proto=rq) + + +# --------------------------------------------------------------------------- +# Queue endpoints +# --------------------------------------------------------------------------- + +def list_cnapp_queue(params, network_uid, status_filter=0): + # type: (KeeperParams, str, int) -> cnapp_pb2.CnappQueueListResponse + """List queued CNAPP issues for a network. `status_filter=0` returns all statuses.""" + rq = cnapp_pb2.CnappQueueListRequest() + rq.networkUid = _to_uid_bytes(network_uid) + rq.statusFilter = int(status_filter) if status_filter is not None else 0 + return _post_request_to_router(params, 'cnapp/queue', rq_proto=rq, + rs_type=cnapp_pb2.CnappQueueListResponse) + + +def associate_cnapp_record(params, cnapp_queue_id, record_uid): + # type: (KeeperParams, int, str) -> None + """Attach a vault record to a queue item — required before remediation.""" + rq = cnapp_pb2.CnappAssociateRequest() + rq.cnappQueueId = int(cnapp_queue_id) + rq.recordUid = _to_uid_bytes(record_uid) + return _post_request_to_router(params, 'cnapp/queue/associate', rq_proto=rq) + + +def remediate_cnapp_queue_item(params, cnapp_queue_id, action_type, provider=None, + cnapp_config_record_uid=None, resource_ref=None, + pwd_complexity=None, controller_uid=None, + message_uid=None, group_name=None): + # type: (KeeperParams, int, int, Optional[int], Optional[str], Optional[str], Optional[str], Optional[str], Optional[str], Optional[str]) -> cnapp_pb2.CnappRemediateResponse + """Trigger a remediation action against the gateway for a queued issue. + + Currently krouter only dispatches `ROTATE_CREDENTIALS`; other actions return + RRC_BAD_REQUEST. The optional fields are forwarded as-is so this helper stays + forward-compatible with new action types.""" + rq = cnapp_pb2.CnappRemediateRequest() + rq.cnappQueueId = int(cnapp_queue_id) + rq.actionType = int(action_type) + if provider is not None: + rq.provider = int(provider) + if cnapp_config_record_uid: + rq.cnappConfigurationRecordUid = _to_uid_bytes(cnapp_config_record_uid) + if resource_ref: + rq.resourceRef = _to_uid_bytes(resource_ref) + if pwd_complexity: + rq.pwdComplexity = pwd_complexity + if controller_uid: + rq.controllerUid = controller_uid + if message_uid: + rq.messageUid = _to_uid_bytes(message_uid) + if group_name: + rq.groupName = group_name + return _post_request_to_router(params, 'cnapp/queue/remediate', rq_proto=rq, + rs_type=cnapp_pb2.CnappRemediateResponse) + + +def set_cnapp_queue_status(params, cnapp_queue_id, cnapp_queue_status_id, reason=None): + # type: (KeeperParams, int, int, Optional[str]) -> cnapp_pb2.CnappSetStatusResponse + """Set the local status on a queue item; krouter best-effort notifies the provider.""" + rq = cnapp_pb2.CnappSetStatusRequest() + rq.cnappQueueId = int(cnapp_queue_id) + rq.cnappQueueStatusId = int(cnapp_queue_status_id) + if reason: + rq.reason = reason + return _post_request_to_router(params, 'cnapp/queue/set-status', rq_proto=rq, + rs_type=cnapp_pb2.CnappSetStatusResponse) + + +def delete_cnapp_queue_item(params, cnapp_queue_id): + # type: (KeeperParams, int) -> None + """Remove a queue item entirely. Raises RRC_BAD_REQUEST if the queue id is unknown.""" + rq = cnapp_pb2.CnappDeleteQueueItemRequest() + rq.cnappQueueId = int(cnapp_queue_id) + return _post_request_to_router(params, 'cnapp/queue/delete', rq_proto=rq) diff --git a/keepercommander/proto/cnapp_pb2.py b/keepercommander/proto/cnapp_pb2.py new file mode 100644 index 000000000..ea05bd946 --- /dev/null +++ b/keepercommander/proto/cnapp_pb2.py @@ -0,0 +1,181 @@ +# -*- coding: utf-8 -*- +# Generated by the protocol buffer compiler. DO NOT EDIT! +# source: cnapp.proto +"""Generated protocol buffer code.""" +from google.protobuf.internal import enum_type_wrapper +from google.protobuf import descriptor as _descriptor +from google.protobuf import descriptor_pool as _descriptor_pool +from google.protobuf import message as _message +from google.protobuf import reflection as _reflection +from google.protobuf import symbol_database as _symbol_database +# @@protoc_insertion_point(imports) + +_sym_db = _symbol_database.Default() + + + + +DESCRIPTOR = _descriptor_pool.Default().AddSerializedFile(b'\n\x0b\x63napp.proto\x12\x05\x43NAPP\"A\n\x15\x43nappQueueListRequest\x12\x12\n\nnetworkUid\x18\x01 \x01(\x0c\x12\x14\n\x0cstatusFilter\x18\x02 \x01(\x05\"O\n\x16\x43nappQueueListResponse\x12$\n\x05items\x18\x01 \x03(\x0b\x32\x15.CNAPP.CnappQueueItem\x12\x0f\n\x07hasMore\x18\x02 \x01(\x08\"\xd0\x01\n\x0e\x43nappQueueItem\x12\x14\n\x0c\x63nappQueueId\x18\x01 \x01(\x05\x12-\n\x0f\x63nappProviderId\x18\x02 \x01(\x0e\x32\x14.CNAPP.CnappProvider\x12\x1a\n\x12\x63nappQueueStatusId\x18\x03 \x01(\x05\x12\x12\n\nreceivedAt\x18\x04 \x01(\x03\x12\x12\n\nresolvedAt\x18\x05 \x01(\x03\x12\x11\n\tnetworkId\x18\x06 \x01(\x0c\x12\x0f\n\x07payload\x18\x07 \x01(\x0c\x12\x11\n\trecordUid\x18\x08 \x01(\x0c\"@\n\x15\x43nappAssociateRequest\x12\x11\n\trecordUid\x18\x01 \x01(\x0c\x12\x14\n\x0c\x63nappQueueId\x18\x02 \x01(\x05\"4\n\x16\x43nappAssociateResponse\x12\x1a\n\x12\x63nappQueueStatusId\x18\x01 \x01(\x05\"\x97\x02\n\x15\x43nappRemediateRequest\x12\x14\n\x0c\x63nappQueueId\x18\x01 \x01(\x05\x12\x31\n\nactionType\x18\x02 \x01(\x0e\x32\x1d.CNAPP.CnappRemediationAction\x12#\n\x1b\x63nappConfigurationRecordUid\x18\x03 \x01(\x0c\x12\x15\n\rpwdComplexity\x18\x04 \x01(\t\x12\x13\n\x0bresourceRef\x18\x05 \x01(\x0c\x12&\n\x08provider\x18\x06 \x01(\x0e\x32\x14.CNAPP.CnappProvider\x12\x15\n\rcontrollerUid\x18\x07 \x01(\t\x12\x12\n\nmessageUid\x18\x08 \x01(\x0c\x12\x11\n\tgroupName\x18\t \x01(\t\"w\n\x16\x43nappRemediateResponse\x12\x31\n\nactionType\x18\x01 \x01(\x0e\x32\x1d.CNAPP.CnappRemediationAction\x12\x0e\n\x06result\x18\x02 \x01(\t\x12\x1a\n\x12\x63nappQueueStatusId\x18\x03 \x01(\x05\"Y\n\x15\x43nappSetStatusRequest\x12\x14\n\x0c\x63nappQueueId\x18\x01 \x01(\x05\x12\x1a\n\x12\x63nappQueueStatusId\x18\x02 \x01(\x05\x12\x0e\n\x06reason\x18\x03 \x01(\t\"4\n\x16\x43nappSetStatusResponse\x12\x1a\n\x12\x63nappQueueStatusId\x18\x01 \x01(\x05\"3\n\x1b\x43nappDeleteQueueItemRequest\x12\x14\n\x0c\x63nappQueueId\x18\x01 \x01(\x05\"\x1e\n\x1c\x43nappDeleteQueueItemResponse\"\xae\x01\n\x12\x43nappConfiguration\x12\x12\n\nnetworkUid\x18\x01 \x01(\x0c\x12&\n\x08provider\x18\x02 \x01(\x0e\x32\x14.CNAPP.CnappProvider\x12\x10\n\x08\x63lientId\x18\x03 \x01(\t\x12\x14\n\x0c\x63lientSecret\x18\x04 \x01(\t\x12\x16\n\x0e\x61piEndpointUrl\x18\x05 \x01(\t\x12\x1c\n\x14\x63nappConfigRecordUid\x18\x06 \x01(\x0c\"5\n\x1f\x43nappDeleteConfigurationRequest\x12\x12\n\nnetworkUid\x18\x01 \x01(\x0c\"5\n\x19\x43nappTestEncrypterRequest\x12\x18\n\x10urlBaseEncrypter\x18\x01 \x01(\t*G\n\rCnappProvider\x12\x1e\n\x1a\x43NAPP_PROVIDER_UNSPECIFIED\x10\x00\x12\x16\n\x12\x43NAPP_PROVIDER_WIZ\x10\x01*\x83\x01\n\x16\x43nappRemediationAction\x12\x0f\n\x0bUNSPECIFIED\x10\x00\x12\x16\n\x12ROTATE_CREDENTIALS\x10\x01\x12\x11\n\rMANAGE_ACCESS\x10\x02\x12\x0e\n\nJIT_ACCESS\x10\x03\x12\x1d\n\x19REMOVE_STANDING_PRIVILEGE\x10\x04\x42!\n\x18\x63om.keepersecurity.protoB\x05\x43nappb\x06proto3') + +_CNAPPPROVIDER = DESCRIPTOR.enum_types_by_name['CnappProvider'] +CnappProvider = enum_type_wrapper.EnumTypeWrapper(_CNAPPPROVIDER) +_CNAPPREMEDIATIONACTION = DESCRIPTOR.enum_types_by_name['CnappRemediationAction'] +CnappRemediationAction = enum_type_wrapper.EnumTypeWrapper(_CNAPPREMEDIATIONACTION) +CNAPP_PROVIDER_UNSPECIFIED = 0 +CNAPP_PROVIDER_WIZ = 1 +UNSPECIFIED = 0 +ROTATE_CREDENTIALS = 1 +MANAGE_ACCESS = 2 +JIT_ACCESS = 3 +REMOVE_STANDING_PRIVILEGE = 4 + + +_CNAPPQUEUELISTREQUEST = DESCRIPTOR.message_types_by_name['CnappQueueListRequest'] +_CNAPPQUEUELISTRESPONSE = DESCRIPTOR.message_types_by_name['CnappQueueListResponse'] +_CNAPPQUEUEITEM = DESCRIPTOR.message_types_by_name['CnappQueueItem'] +_CNAPPASSOCIATEREQUEST = DESCRIPTOR.message_types_by_name['CnappAssociateRequest'] +_CNAPPASSOCIATERESPONSE = DESCRIPTOR.message_types_by_name['CnappAssociateResponse'] +_CNAPPREMEDIATEREQUEST = DESCRIPTOR.message_types_by_name['CnappRemediateRequest'] +_CNAPPREMEDIATERESPONSE = DESCRIPTOR.message_types_by_name['CnappRemediateResponse'] +_CNAPPSETSTATUSREQUEST = DESCRIPTOR.message_types_by_name['CnappSetStatusRequest'] +_CNAPPSETSTATUSRESPONSE = DESCRIPTOR.message_types_by_name['CnappSetStatusResponse'] +_CNAPPDELETEQUEUEITEMREQUEST = DESCRIPTOR.message_types_by_name['CnappDeleteQueueItemRequest'] +_CNAPPDELETEQUEUEITEMRESPONSE = DESCRIPTOR.message_types_by_name['CnappDeleteQueueItemResponse'] +_CNAPPCONFIGURATION = DESCRIPTOR.message_types_by_name['CnappConfiguration'] +_CNAPPDELETECONFIGURATIONREQUEST = DESCRIPTOR.message_types_by_name['CnappDeleteConfigurationRequest'] +_CNAPPTESTENCRYPTERREQUEST = DESCRIPTOR.message_types_by_name['CnappTestEncrypterRequest'] +CnappQueueListRequest = _reflection.GeneratedProtocolMessageType('CnappQueueListRequest', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPQUEUELISTREQUEST, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappQueueListRequest) + }) +_sym_db.RegisterMessage(CnappQueueListRequest) + +CnappQueueListResponse = _reflection.GeneratedProtocolMessageType('CnappQueueListResponse', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPQUEUELISTRESPONSE, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappQueueListResponse) + }) +_sym_db.RegisterMessage(CnappQueueListResponse) + +CnappQueueItem = _reflection.GeneratedProtocolMessageType('CnappQueueItem', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPQUEUEITEM, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappQueueItem) + }) +_sym_db.RegisterMessage(CnappQueueItem) + +CnappAssociateRequest = _reflection.GeneratedProtocolMessageType('CnappAssociateRequest', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPASSOCIATEREQUEST, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappAssociateRequest) + }) +_sym_db.RegisterMessage(CnappAssociateRequest) + +CnappAssociateResponse = _reflection.GeneratedProtocolMessageType('CnappAssociateResponse', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPASSOCIATERESPONSE, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappAssociateResponse) + }) +_sym_db.RegisterMessage(CnappAssociateResponse) + +CnappRemediateRequest = _reflection.GeneratedProtocolMessageType('CnappRemediateRequest', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPREMEDIATEREQUEST, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappRemediateRequest) + }) +_sym_db.RegisterMessage(CnappRemediateRequest) + +CnappRemediateResponse = _reflection.GeneratedProtocolMessageType('CnappRemediateResponse', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPREMEDIATERESPONSE, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappRemediateResponse) + }) +_sym_db.RegisterMessage(CnappRemediateResponse) + +CnappSetStatusRequest = _reflection.GeneratedProtocolMessageType('CnappSetStatusRequest', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPSETSTATUSREQUEST, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappSetStatusRequest) + }) +_sym_db.RegisterMessage(CnappSetStatusRequest) + +CnappSetStatusResponse = _reflection.GeneratedProtocolMessageType('CnappSetStatusResponse', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPSETSTATUSRESPONSE, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappSetStatusResponse) + }) +_sym_db.RegisterMessage(CnappSetStatusResponse) + +CnappDeleteQueueItemRequest = _reflection.GeneratedProtocolMessageType('CnappDeleteQueueItemRequest', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPDELETEQUEUEITEMREQUEST, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappDeleteQueueItemRequest) + }) +_sym_db.RegisterMessage(CnappDeleteQueueItemRequest) + +CnappDeleteQueueItemResponse = _reflection.GeneratedProtocolMessageType('CnappDeleteQueueItemResponse', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPDELETEQUEUEITEMRESPONSE, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappDeleteQueueItemResponse) + }) +_sym_db.RegisterMessage(CnappDeleteQueueItemResponse) + +CnappConfiguration = _reflection.GeneratedProtocolMessageType('CnappConfiguration', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPCONFIGURATION, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappConfiguration) + }) +_sym_db.RegisterMessage(CnappConfiguration) + +CnappDeleteConfigurationRequest = _reflection.GeneratedProtocolMessageType('CnappDeleteConfigurationRequest', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPDELETECONFIGURATIONREQUEST, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappDeleteConfigurationRequest) + }) +_sym_db.RegisterMessage(CnappDeleteConfigurationRequest) + +CnappTestEncrypterRequest = _reflection.GeneratedProtocolMessageType('CnappTestEncrypterRequest', (_message.Message,), { + 'DESCRIPTOR' : _CNAPPTESTENCRYPTERREQUEST, + '__module__' : 'cnapp_pb2' + # @@protoc_insertion_point(class_scope:CNAPP.CnappTestEncrypterRequest) + }) +_sym_db.RegisterMessage(CnappTestEncrypterRequest) + +if _descriptor._USE_C_DESCRIPTORS == False: + + DESCRIPTOR._options = None + DESCRIPTOR._serialized_options = b'\n\030com.keepersecurity.protoB\005Cnapp' + _CNAPPPROVIDER._serialized_start=1421 + _CNAPPPROVIDER._serialized_end=1492 + _CNAPPREMEDIATIONACTION._serialized_start=1495 + _CNAPPREMEDIATIONACTION._serialized_end=1626 + _CNAPPQUEUELISTREQUEST._serialized_start=22 + _CNAPPQUEUELISTREQUEST._serialized_end=87 + _CNAPPQUEUELISTRESPONSE._serialized_start=89 + _CNAPPQUEUELISTRESPONSE._serialized_end=168 + _CNAPPQUEUEITEM._serialized_start=171 + _CNAPPQUEUEITEM._serialized_end=379 + _CNAPPASSOCIATEREQUEST._serialized_start=381 + _CNAPPASSOCIATEREQUEST._serialized_end=445 + _CNAPPASSOCIATERESPONSE._serialized_start=447 + _CNAPPASSOCIATERESPONSE._serialized_end=499 + _CNAPPREMEDIATEREQUEST._serialized_start=502 + _CNAPPREMEDIATEREQUEST._serialized_end=781 + _CNAPPREMEDIATERESPONSE._serialized_start=783 + _CNAPPREMEDIATERESPONSE._serialized_end=902 + _CNAPPSETSTATUSREQUEST._serialized_start=904 + _CNAPPSETSTATUSREQUEST._serialized_end=993 + _CNAPPSETSTATUSRESPONSE._serialized_start=995 + _CNAPPSETSTATUSRESPONSE._serialized_end=1047 + _CNAPPDELETEQUEUEITEMREQUEST._serialized_start=1049 + _CNAPPDELETEQUEUEITEMREQUEST._serialized_end=1100 + _CNAPPDELETEQUEUEITEMRESPONSE._serialized_start=1102 + _CNAPPDELETEQUEUEITEMRESPONSE._serialized_end=1132 + _CNAPPCONFIGURATION._serialized_start=1135 + _CNAPPCONFIGURATION._serialized_end=1309 + _CNAPPDELETECONFIGURATIONREQUEST._serialized_start=1311 + _CNAPPDELETECONFIGURATIONREQUEST._serialized_end=1364 + _CNAPPTESTENCRYPTERREQUEST._serialized_start=1366 + _CNAPPTESTENCRYPTERREQUEST._serialized_end=1419 +# @@protoc_insertion_point(module_scope) diff --git a/unit-tests/pam/test_cnapp.py b/unit-tests/pam/test_cnapp.py new file mode 100644 index 000000000..ab1486f3a --- /dev/null +++ b/unit-tests/pam/test_cnapp.py @@ -0,0 +1,837 @@ +"""Unit tests for the Commander CNAPP helper and command surface. + +Strategy: every test patches `_post_request_to_router` so we can assert on what the +helper sends to krouter and feed deterministic responses back into the commands. We +deliberately stay one layer below the network — no socket calls, no real protobuf +encryption — but we exercise the real proto serializers so wire-format breakage +surfaces here. +""" +import base64 +import io +import json +import os +import unittest +from contextlib import redirect_stdout +from unittest.mock import MagicMock, patch + +from cryptography.hazmat.primitives.ciphers.aead import AESGCM +from keeper_secrets_manager_core.utils import bytes_to_base64 + +from keepercommander.commands.pam import cnapp_helper +from keepercommander.commands.pam import cnapp_commands +from keepercommander.error import CommandError +from keepercommander.proto import cnapp_pb2 + + +# Sample 16-byte UIDs as base64url (the format Commander callers pass in). +NETWORK_UID = 'AAAAAAAAAAAAAAAAAAAAAA' # 16 zero bytes +RECORD_UID = 'AQEBAQEBAQEBAQEBAQEBAQ' # 16 0x01 bytes +CONFIG_RECORD_UID = 'AgICAgICAgICAgICAgICAg' + + +def _mock_params(): + """Minimal KeeperParams stand-in — the helpers only use it to drive the router_helper + transport, which is mocked here, so a MagicMock is enough.""" + return MagicMock() + + +# --------------------------------------------------------------------------- +# cnapp_helper: enum parsing +# --------------------------------------------------------------------------- + +class TestEnumParsing(unittest.TestCase): + """provider_from_name and action_from_name must accept short or full names and + reject unknown values with a helpful error listing valid options.""" + + def test_provider_short_name(self): + self.assertEqual(cnapp_helper.provider_from_name('wiz'), cnapp_pb2.CNAPP_PROVIDER_WIZ) + + def test_provider_full_name_case_insensitive(self): + self.assertEqual( + cnapp_helper.provider_from_name('cnapp_provider_wiz'), + cnapp_pb2.CNAPP_PROVIDER_WIZ, + ) + + def test_provider_empty_returns_unspecified(self): + self.assertEqual(cnapp_helper.provider_from_name(''), cnapp_pb2.CNAPP_PROVIDER_UNSPECIFIED) + + def test_provider_unknown_raises_with_valid_options(self): + with self.assertRaises(ValueError) as ctx: + cnapp_helper.provider_from_name('aws') + self.assertIn('WIZ', str(ctx.exception).upper()) + + def test_action_short_name(self): + self.assertEqual( + cnapp_helper.action_from_name('rotate_credentials'), + cnapp_pb2.ROTATE_CREDENTIALS, + ) + + def test_action_hyphenated(self): + # The CLI accepts hyphens (`--action remove-standing-privilege`) for ergonomics; + # helper must normalize before resolving the enum. + self.assertEqual( + cnapp_helper.action_from_name('remove-standing-privilege'), + cnapp_pb2.REMOVE_STANDING_PRIVILEGE, + ) + + def test_action_unknown_raises(self): + with self.assertRaises(ValueError): + cnapp_helper.action_from_name('teleport') + + def test_action_empty_returns_unspecified(self): + self.assertEqual(cnapp_helper.action_from_name(''), cnapp_pb2.UNSPECIFIED) + + +# --------------------------------------------------------------------------- +# cnapp_helper: configuration endpoints +# --------------------------------------------------------------------------- + +class TestConfigurationHelpers(unittest.TestCase): + """Each helper must dispatch to the right krouter path with a correctly populated + protobuf request and return the typed response.""" + + def setUp(self): + self.params = _mock_params() + + def _patch_post(self, return_value=None): + return patch.object(cnapp_helper, '_post_request_to_router', return_value=return_value) + + def test_set_configuration_dispatches_with_full_payload(self): + expected_response = cnapp_pb2.CnappConfiguration( + clientId='abc', apiEndpointUrl='https://api.wiz.io') + with self._patch_post(return_value=expected_response) as post: + result = cnapp_helper.set_cnapp_configuration( + self.params, + network_uid=NETWORK_UID, + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ, + client_id='abc', + client_secret='secret', + api_endpoint_url='https://api.wiz.io', + cnapp_config_record_uid=CONFIG_RECORD_UID, + ) + self.assertIs(result, expected_response) + args, kwargs = post.call_args + self.assertEqual(args[1], 'cnapp/configuration/set') + rq = kwargs['rq_proto'] + self.assertEqual(rq.provider, cnapp_pb2.CNAPP_PROVIDER_WIZ) + self.assertEqual(rq.clientId, 'abc') + self.assertEqual(rq.clientSecret, 'secret') + self.assertEqual(rq.apiEndpointUrl, 'https://api.wiz.io') + self.assertEqual(len(rq.networkUid), 16) + self.assertEqual(len(rq.cnappConfigRecordUid), 16) + self.assertIs(kwargs['rs_type'], cnapp_pb2.CnappConfiguration) + + def test_set_configuration_omits_empty_secret_to_keep_existing(self): + """Edge case: passing '' for client_secret on set must leave the field blank in + the request so krouter can splice in the previously stored secret.""" + with self._patch_post() as post: + cnapp_helper.set_cnapp_configuration( + self.params, + network_uid=NETWORK_UID, + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ, + client_id='abc', + client_secret='', + api_endpoint_url='https://api.wiz.io', + cnapp_config_record_uid=CONFIG_RECORD_UID, + ) + rq = post.call_args.kwargs['rq_proto'] + self.assertEqual(rq.clientSecret, '') + + def test_test_configuration_dispatches_to_test_endpoint(self): + with self._patch_post() as post: + cnapp_helper.test_cnapp_configuration( + self.params, + network_uid=NETWORK_UID, + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ, + client_id='abc', + client_secret='secret', + api_endpoint_url='https://api.wiz.io', + ) + self.assertEqual(post.call_args.args[1], 'cnapp/configuration/test') + # test endpoint never persists, so it must not require / send config record UID + self.assertEqual(post.call_args.kwargs['rq_proto'].cnappConfigRecordUid, b'') + + def test_test_encrypter_sets_url(self): + with self._patch_post() as post: + cnapp_helper.test_cnapp_encrypter(self.params, url_base_encrypter='https://encr.local') + rq = post.call_args.kwargs['rq_proto'] + self.assertEqual(post.call_args.args[1], 'cnapp/configuration/test-encrypter') + self.assertEqual(rq.urlBaseEncrypter, 'https://encr.local') + + def test_read_configuration_uses_read_endpoint(self): + with self._patch_post(return_value=cnapp_pb2.CnappConfiguration()) as post: + cnapp_helper.read_cnapp_configuration( + self.params, network_uid=NETWORK_UID, provider=cnapp_pb2.CNAPP_PROVIDER_WIZ) + self.assertEqual(post.call_args.args[1], 'cnapp/configuration/read') + self.assertIs(post.call_args.kwargs['rs_type'], cnapp_pb2.CnappConfiguration) + + def test_delete_configuration_uses_delete_endpoint(self): + with self._patch_post() as post: + cnapp_helper.delete_cnapp_configuration(self.params, network_uid=NETWORK_UID) + self.assertEqual(post.call_args.args[1], 'cnapp/configuration/delete') + self.assertEqual(len(post.call_args.kwargs['rq_proto'].networkUid), 16) + + +# --------------------------------------------------------------------------- +# cnapp_helper: queue endpoints +# --------------------------------------------------------------------------- + +class TestQueueHelpers(unittest.TestCase): + def setUp(self): + self.params = _mock_params() + + def _patch_post(self, return_value=None): + return patch.object(cnapp_helper, '_post_request_to_router', return_value=return_value) + + def test_list_queue_with_status_filter(self): + items = cnapp_pb2.CnappQueueListResponse( + items=[cnapp_pb2.CnappQueueItem(cnappQueueId=42)]) + with self._patch_post(return_value=items) as post: + response = cnapp_helper.list_cnapp_queue( + self.params, network_uid=NETWORK_UID, status_filter=1) + self.assertEqual(post.call_args.args[1], 'cnapp/queue') + self.assertEqual(post.call_args.kwargs['rq_proto'].statusFilter, 1) + self.assertEqual(response.items[0].cnappQueueId, 42) + + def test_list_queue_defaults_to_all_status(self): + with self._patch_post(return_value=cnapp_pb2.CnappQueueListResponse()) as post: + cnapp_helper.list_cnapp_queue(self.params, network_uid=NETWORK_UID) + self.assertEqual(post.call_args.kwargs['rq_proto'].statusFilter, 0) + + def test_associate_record_dispatches(self): + with self._patch_post() as post: + cnapp_helper.associate_cnapp_record( + self.params, cnapp_queue_id=7, record_uid=RECORD_UID) + rq = post.call_args.kwargs['rq_proto'] + self.assertEqual(post.call_args.args[1], 'cnapp/queue/associate') + self.assertEqual(rq.cnappQueueId, 7) + self.assertEqual(len(rq.recordUid), 16) + + def test_remediate_forwards_optional_fields(self): + with self._patch_post(return_value=cnapp_pb2.CnappRemediateResponse()) as post: + cnapp_helper.remediate_cnapp_queue_item( + self.params, + cnapp_queue_id=3, + action_type=cnapp_pb2.ROTATE_CREDENTIALS, + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ, + cnapp_config_record_uid=CONFIG_RECORD_UID, + resource_ref=RECORD_UID, + pwd_complexity='{"len":24}', + controller_uid='gateway-1', + message_uid=RECORD_UID, + group_name='Admins', + ) + rq = post.call_args.kwargs['rq_proto'] + self.assertEqual(post.call_args.args[1], 'cnapp/queue/remediate') + self.assertEqual(rq.cnappQueueId, 3) + self.assertEqual(rq.actionType, cnapp_pb2.ROTATE_CREDENTIALS) + self.assertEqual(rq.provider, cnapp_pb2.CNAPP_PROVIDER_WIZ) + self.assertEqual(rq.pwdComplexity, '{"len":24}') + self.assertEqual(rq.controllerUid, 'gateway-1') + self.assertEqual(rq.groupName, 'Admins') + + def test_remediate_minimal_fields(self): + """No optional fields — only queueId and actionType must be set on the wire.""" + with self._patch_post(return_value=cnapp_pb2.CnappRemediateResponse()) as post: + cnapp_helper.remediate_cnapp_queue_item( + self.params, cnapp_queue_id=9, action_type=cnapp_pb2.ROTATE_CREDENTIALS) + rq = post.call_args.kwargs['rq_proto'] + self.assertEqual(rq.cnappQueueId, 9) + self.assertEqual(rq.provider, 0) + self.assertEqual(rq.pwdComplexity, '') + self.assertEqual(rq.controllerUid, '') + self.assertEqual(rq.groupName, '') + + def test_set_status_with_reason(self): + with self._patch_post(return_value=cnapp_pb2.CnappSetStatusResponse(cnappQueueStatusId=3)) as post: + response = cnapp_helper.set_cnapp_queue_status( + self.params, cnapp_queue_id=11, cnapp_queue_status_id=3, reason='Manually resolved') + self.assertEqual(post.call_args.args[1], 'cnapp/queue/set-status') + self.assertEqual(post.call_args.kwargs['rq_proto'].reason, 'Manually resolved') + self.assertEqual(response.cnappQueueStatusId, 3) + + def test_delete_queue_item_dispatches(self): + with self._patch_post() as post: + cnapp_helper.delete_cnapp_queue_item(self.params, cnapp_queue_id=11) + self.assertEqual(post.call_args.args[1], 'cnapp/queue/delete') + self.assertEqual(post.call_args.kwargs['rq_proto'].cnappQueueId, 11) + + +# --------------------------------------------------------------------------- +# cnapp_helper: error propagation +# --------------------------------------------------------------------------- + +class TestHelperErrorPropagation(unittest.TestCase): + """The router layer raises on RRC_!=OK; helpers must NOT swallow those errors.""" + + def test_set_configuration_propagates_router_error(self): + params = _mock_params() + with patch.object(cnapp_helper, '_post_request_to_router', + side_effect=Exception('Credential validation failed: Unauthorized')): + with self.assertRaises(Exception) as ctx: + cnapp_helper.set_cnapp_configuration( + params, + network_uid=NETWORK_UID, + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ, + client_id='abc', + client_secret='bad', + api_endpoint_url='https://api.wiz.io', + cnapp_config_record_uid=CONFIG_RECORD_UID, + ) + self.assertIn('Credential validation failed', str(ctx.exception)) + + +# --------------------------------------------------------------------------- +# cnapp_commands: status resolver +# --------------------------------------------------------------------------- + +class TestStatusResolver(unittest.TestCase): + + def test_numeric_passes_through(self): + self.assertEqual(cnapp_commands._resolve_status('1'), 1) + self.assertEqual(cnapp_commands._resolve_status(2), 2) + + def test_zero_is_all(self): + self.assertEqual(cnapp_commands._resolve_status('0'), 0) + self.assertEqual(cnapp_commands._resolve_status(None), 0) + self.assertEqual(cnapp_commands._resolve_status(''), 0) + + def test_named_status_case_insensitive(self): + self.assertEqual(cnapp_commands._resolve_status('PENDING'), 1) + self.assertEqual(cnapp_commands._resolve_status('in_progress'), 2) + self.assertEqual(cnapp_commands._resolve_status('Resolved'), 3) + + def test_unknown_status_raises_command_error(self): + with self.assertRaises(CommandError): + cnapp_commands._resolve_status('flapping') + + +# --------------------------------------------------------------------------- +# cnapp_commands: end-to-end (helpers patched) +# --------------------------------------------------------------------------- + +class TestConfigCommands(unittest.TestCase): + def setUp(self): + self.params = _mock_params() + + def _capture_stdout(self): + buf = io.StringIO() + return buf, redirect_stdout(buf) + + def test_config_set_calls_helper_with_resolved_provider(self): + with patch.object(cnapp_commands.cnapp_helper, 'set_cnapp_configuration', + return_value=cnapp_pb2.CnappConfiguration(clientId='abc', + apiEndpointUrl='https://api.wiz.io', + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ)) as helper: + buf, ctx = self._capture_stdout() + with ctx: + cnapp_commands.PAMCnappConfigSetCommand().execute( + self.params, + network_uid=NETWORK_UID, + provider='wiz', + client_id='abc', + client_secret='secret', + api_endpoint_url='https://api.wiz.io', + cnapp_config_record_uid=CONFIG_RECORD_UID, + ) + helper.assert_called_once() + kwargs = helper.call_args.kwargs + self.assertEqual(kwargs['provider'], cnapp_pb2.CNAPP_PROVIDER_WIZ) + self.assertEqual(kwargs['client_secret'], 'secret') + self.assertIn('saved', buf.getvalue().lower()) + + def test_config_set_blank_secret_passes_through(self): + """Edge case: the CLI must forward an empty secret unchanged so krouter can + keep the existing value.""" + with patch.object(cnapp_commands.cnapp_helper, 'set_cnapp_configuration', + return_value=cnapp_pb2.CnappConfiguration()) as helper: + with redirect_stdout(io.StringIO()): + cnapp_commands.PAMCnappConfigSetCommand().execute( + self.params, + network_uid=NETWORK_UID, + provider='wiz', + client_id='abc', + client_secret='', # explicit + api_endpoint_url='https://api.wiz.io', + cnapp_config_record_uid=CONFIG_RECORD_UID, + ) + self.assertEqual(helper.call_args.kwargs['client_secret'], '') + + def test_config_set_invalid_provider_raises(self): + with self.assertRaises(ValueError): + cnapp_commands.PAMCnappConfigSetCommand().execute( + self.params, + network_uid=NETWORK_UID, + provider='bogus', + client_id='abc', + client_secret='secret', + api_endpoint_url='https://api.wiz.io', + cnapp_config_record_uid=CONFIG_RECORD_UID, + ) + + def test_config_test_prints_success(self): + with patch.object(cnapp_commands.cnapp_helper, 'test_cnapp_configuration', return_value=None): + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappConfigTestCommand().execute( + self.params, + network_uid=NETWORK_UID, + provider='wiz', + client_id='abc', + client_secret='secret', + api_endpoint_url='https://api.wiz.io', + ) + self.assertIn('validated', buf.getvalue().lower()) + + def test_config_test_propagates_helper_error(self): + with patch.object(cnapp_commands.cnapp_helper, 'test_cnapp_configuration', + side_effect=Exception('Credential validation failed: bad')): + with self.assertRaises(Exception): + cnapp_commands.PAMCnappConfigTestCommand().execute( + self.params, + network_uid=NETWORK_UID, + provider='wiz', + client_id='abc', + client_secret='bad', + api_endpoint_url='https://api.wiz.io', + ) + + def test_config_test_encrypter_success(self): + with patch.object(cnapp_commands.cnapp_helper, 'test_cnapp_encrypter', return_value=None) as helper: + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappConfigTestEncrypterCommand().execute( + self.params, url='https://encr.local') + helper.assert_called_once_with(self.params, url_base_encrypter='https://encr.local') + self.assertIn('reachable', buf.getvalue().lower()) + + def test_config_read_table_format(self): + config = cnapp_pb2.CnappConfiguration( + clientId='abc', + apiEndpointUrl='https://api.wiz.io', + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ, + ) + with patch.object(cnapp_commands.cnapp_helper, 'read_cnapp_configuration', return_value=config): + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappConfigReadCommand().execute( + self.params, network_uid=NETWORK_UID, provider='wiz', format='table') + output = buf.getvalue() + self.assertIn('CNAPP Configuration', output) + self.assertIn('https://api.wiz.io', output) + + def test_config_read_json_format(self): + config = cnapp_pb2.CnappConfiguration( + clientId='abc', + apiEndpointUrl='https://api.wiz.io', + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ, + ) + with patch.object(cnapp_commands.cnapp_helper, 'read_cnapp_configuration', return_value=config): + buf = io.StringIO() + with redirect_stdout(buf): + result = cnapp_commands.PAMCnappConfigReadCommand().execute( + self.params, network_uid=NETWORK_UID, provider='wiz', format='json') + payload = json.loads(buf.getvalue()) + self.assertEqual(payload['clientId'], 'abc') + self.assertEqual(payload['provider'], 'CNAPP_PROVIDER_WIZ') + self.assertEqual(payload['apiEndpointUrl'], 'https://api.wiz.io') + self.assertIsNone(result, 'JSON output is the channel — no value returned to the REPL') + + def test_config_read_handles_none_response(self): + with patch.object(cnapp_commands.cnapp_helper, 'read_cnapp_configuration', return_value=None): + self.assertIsNone(cnapp_commands.PAMCnappConfigReadCommand().execute( + self.params, network_uid=NETWORK_UID, provider='wiz', format='table')) + + def test_config_delete_success(self): + with patch.object(cnapp_commands.cnapp_helper, 'delete_cnapp_configuration', return_value=None) as helper: + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappConfigDeleteCommand().execute(self.params, network_uid=NETWORK_UID) + helper.assert_called_once_with(self.params, network_uid=NETWORK_UID) + self.assertIn('deleted', buf.getvalue().lower()) + + +class TestQueueCommands(unittest.TestCase): + def setUp(self): + self.params = _mock_params() + + def _queue_response(self, items=None, has_more=False): + return cnapp_pb2.CnappQueueListResponse(items=items or [], hasMore=has_more) + + def _queue_item(self, queue_id=1, status_id=1, record_uid=b''): + return cnapp_pb2.CnappQueueItem( + cnappQueueId=queue_id, + cnappProviderId=cnapp_pb2.CNAPP_PROVIDER_WIZ, + cnappQueueStatusId=status_id, + receivedAt=1700000000000, + networkId=b'\x00' * 16, + recordUid=record_uid, + ) + + def test_queue_list_empty(self): + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', + return_value=self._queue_response()): + buf = io.StringIO() + with redirect_stdout(buf): + result = cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='table', + no_decrypt=True) + self.assertIn('No CNAPP queue items', buf.getvalue()) + self.assertIsNone(result, 'queue list must not return the proto so the REPL does not dump bytes') + + def test_queue_list_with_items_table(self): + item = self._queue_item(queue_id=99, status_id=2, record_uid=b'\x01' * 16) + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', + return_value=self._queue_response([item])): + buf = io.StringIO() + with redirect_stdout(buf): + result = cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='table', + no_decrypt=True) + output = buf.getvalue() + self.assertIn('99', output) + self.assertIn('IN_PROGRESS', output) + self.assertIn('CNAPP_PROVIDER_WIZ', output) + self.assertIsNone(result) + + def test_queue_list_filter_resolves_named_status(self): + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', + return_value=self._queue_response()) as helper: + with redirect_stdout(io.StringIO()): + cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status='pending', format='table', + no_decrypt=True) + self.assertEqual(helper.call_args.kwargs['status_filter'], 1) + + def test_queue_list_json_format(self): + item = self._queue_item(queue_id=5, status_id=3, record_uid=b'\x02' * 16) + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', + return_value=self._queue_response([item], has_more=True)): + buf = io.StringIO() + with redirect_stdout(buf): + result = cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='json', + no_decrypt=True) + payload = json.loads(buf.getvalue()) + self.assertEqual(payload['items'][0]['cnappQueueId'], 5) + self.assertEqual(payload['items'][0]['cnappQueueStatusName'], 'RESOLVED') + self.assertTrue(payload['hasMore']) + self.assertEqual(payload['items'][0]['recordUid'], + bytes_to_base64(b'\x02' * 16)) + self.assertNotIn('payload', payload['items'][0], + 'raw encrypted payload bytes must not leak into JSON output') + self.assertIsNone(result, 'JSON output stream must not also return a value') + + def test_queue_list_warns_when_has_more(self): + item = self._queue_item() + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', + return_value=self._queue_response([item], has_more=True)): + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='table', + no_decrypt=True) + self.assertIn('More items', buf.getvalue()) + + def test_queue_associate_success(self): + with patch.object(cnapp_commands.cnapp_helper, 'associate_cnapp_record', return_value=None) as helper: + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappQueueAssociateCommand().execute( + self.params, cnapp_queue_id=12, record_uid=RECORD_UID) + helper.assert_called_once_with(self.params, cnapp_queue_id=12, record_uid=RECORD_UID) + self.assertIn('12', buf.getvalue()) + + def test_queue_remediate_prints_response(self): + response = cnapp_pb2.CnappRemediateResponse( + actionType=cnapp_pb2.ROTATE_CREDENTIALS, + result='Scheduled', + cnappQueueStatusId=2, + ) + with patch.object(cnapp_commands.cnapp_helper, 'remediate_cnapp_queue_item', + return_value=response): + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappQueueRemediateCommand().execute( + self.params, + cnapp_queue_id=4, + action_type='rotate_credentials', + provider='wiz', + ) + output = buf.getvalue() + self.assertIn('ROTATE_CREDENTIALS', output) + self.assertIn('IN_PROGRESS', output) + self.assertIn('Scheduled', output) + + def test_queue_remediate_unsupported_action_propagates(self): + with patch.object(cnapp_commands.cnapp_helper, 'remediate_cnapp_queue_item', + side_effect=Exception('Unsupported action type response code: RRC_BAD_REQUEST')): + with self.assertRaises(Exception) as ctx: + cnapp_commands.PAMCnappQueueRemediateCommand().execute( + self.params, + cnapp_queue_id=4, + action_type='jit_access', + ) + self.assertIn('Unsupported', str(ctx.exception)) + + def test_queue_remediate_invalid_action_name(self): + with self.assertRaises(ValueError): + cnapp_commands.PAMCnappQueueRemediateCommand().execute( + self.params, cnapp_queue_id=1, action_type='nuke_everything') + + def test_queue_set_status_normalizes_named(self): + response = cnapp_pb2.CnappSetStatusResponse(cnappQueueStatusId=3) + with patch.object(cnapp_commands.cnapp_helper, 'set_cnapp_queue_status', + return_value=response) as helper: + with redirect_stdout(io.StringIO()): + cnapp_commands.PAMCnappQueueSetStatusCommand().execute( + self.params, cnapp_queue_id=8, status='resolved', reason='manual') + kwargs = helper.call_args.kwargs + self.assertEqual(kwargs['cnapp_queue_status_id'], 3) + self.assertEqual(kwargs['reason'], 'manual') + + def test_queue_set_status_rejects_zero(self): + with self.assertRaises(CommandError): + cnapp_commands.PAMCnappQueueSetStatusCommand().execute( + self.params, cnapp_queue_id=8, status=0) + + def test_queue_set_status_rejects_unknown_name(self): + with self.assertRaises(CommandError): + cnapp_commands.PAMCnappQueueSetStatusCommand().execute( + self.params, cnapp_queue_id=8, status='snoozed') + + def test_queue_delete_success(self): + with patch.object(cnapp_commands.cnapp_helper, 'delete_cnapp_queue_item', + return_value=None) as helper: + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappQueueDeleteCommand().execute(self.params, cnapp_queue_id=22) + helper.assert_called_once_with(self.params, cnapp_queue_id=22) + self.assertIn('22', buf.getvalue()) + + def test_queue_delete_unknown_id_propagates_error(self): + with patch.object(cnapp_commands.cnapp_helper, 'delete_cnapp_queue_item', + side_effect=Exception('Queue item not found: 99 Response code: RRC_BAD_REQUEST')): + with self.assertRaises(Exception): + cnapp_commands.PAMCnappQueueDeleteCommand().execute(self.params, cnapp_queue_id=99) + + +# --------------------------------------------------------------------------- +# Command tree wiring +# --------------------------------------------------------------------------- + +class TestCommandTree(unittest.TestCase): + """Sanity check that the cnapp commands are reachable via `pam cnapp ...`.""" + + def test_pam_cnapp_subcommands(self): + from keepercommander.commands.discoveryrotation import PAMControllerCommand + pam = PAMControllerCommand() + self.assertIn('cnapp', pam.subcommands) + config = pam.subcommands['cnapp'].subcommands['config'] + queue = pam.subcommands['cnapp'].subcommands['queue'] + self.assertEqual( + sorted(config.subcommands), + ['delete', 'read', 'set', 'test', 'test-encrypter'], + ) + self.assertEqual( + sorted(queue.subcommands), + ['associate', 'delete', 'list', 'remediate', 'set-status'], + ) + + +# --------------------------------------------------------------------------- +# Payload decryption — round-trip an AES-256-GCM envelope and decrypt it back +# --------------------------------------------------------------------------- + +def _encrypt_cnapp_payload_for_test(plaintext_json, key): + """Produce a CNAPP queue payload byte string the way the Encrypter would so we can + exercise `_decrypt_cnapp_payload` end-to-end without mocking AES-GCM.""" + nonce = os.urandom(12) + ciphertext = AESGCM(key).encrypt(nonce, plaintext_json.encode('utf-8'), None) + enc_b64url = base64.urlsafe_b64encode(nonce + ciphertext).rstrip(b'=').decode('ascii') + envelope = json.dumps({ + 'encrypted_payload': enc_b64url, + 'alg': 'AES-256-GCM', + 'version': '1', + }).encode('utf-8') + envelope_b64url = base64.urlsafe_b64encode(envelope).rstrip(b'=').decode('ascii') + return envelope_b64url.encode('utf-8') + + +class TestPayloadDecryption(unittest.TestCase): + """`_decrypt_cnapp_payload` must round-trip the envelope produced by the customer + Encrypter (UTF-8 base64url envelope wrapping nonce||ciphertext||tag).""" + + def setUp(self): + self.key = os.urandom(32) + self.plaintext = { + 'issue': {'id': 'wiz-001', 'severity': 'HIGH', 'created': '2026-05-01T00:00:00Z'}, + 'resource': {'name': 'i-abc', 'type': 'EC2', 'cloudPlatform': 'AWS'}, + 'control': {'name': 'Public S3', 'risks': ['data-exposure']}, + 'tags': ['team:platform'], + } + + def test_roundtrip(self): + payload = _encrypt_cnapp_payload_for_test(json.dumps(self.plaintext), self.key) + decrypted = cnapp_commands._decrypt_cnapp_payload(payload, self.key) + self.assertEqual(decrypted['issue']['id'], 'wiz-001') + self.assertEqual(decrypted['resource']['name'], 'i-abc') + + def test_wrong_key_raises(self): + payload = _encrypt_cnapp_payload_for_test(json.dumps(self.plaintext), self.key) + with self.assertRaises(Exception): + cnapp_commands._decrypt_cnapp_payload(payload, os.urandom(32)) + + def test_unsupported_alg_raises(self): + envelope = json.dumps({'encrypted_payload': '', 'alg': 'ChaCha20', 'version': '1'}).encode('utf-8') + payload = base64.urlsafe_b64encode(envelope).rstrip(b'=') + with self.assertRaises(ValueError): + cnapp_commands._decrypt_cnapp_payload(payload, self.key) + + def test_short_ciphertext_raises(self): + envelope = json.dumps({ + 'encrypted_payload': base64.urlsafe_b64encode(b'abc').rstrip(b'=').decode('ascii'), + 'alg': 'AES-256-GCM', + }).encode('utf-8') + payload = base64.urlsafe_b64encode(envelope).rstrip(b'=') + with self.assertRaises(ValueError): + cnapp_commands._decrypt_cnapp_payload(payload, self.key) + + +class TestKeyDecode(unittest.TestCase): + """`_decode_aes_key` must accept both standard and url-safe base64, only when the + decoded length is 16 or 32 bytes.""" + + def test_standard_base64_32(self): + raw = base64.b64encode(b'\x11' * 32).decode('ascii') + self.assertEqual(cnapp_commands._decode_aes_key(raw), b'\x11' * 32) + + def test_urlsafe_base64_32(self): + raw = base64.urlsafe_b64encode(b'\x22' * 32).decode('ascii') + self.assertEqual(cnapp_commands._decode_aes_key(raw), b'\x22' * 32) + + def test_wrong_length_returns_none(self): + raw = base64.b64encode(b'\x33' * 24).decode('ascii') + self.assertIsNone(cnapp_commands._decode_aes_key(raw)) + + def test_garbage_returns_none(self): + self.assertIsNone(cnapp_commands._decode_aes_key('not base64 at all!!!')) + self.assertIsNone(cnapp_commands._decode_aes_key('')) + self.assertIsNone(cnapp_commands._decode_aes_key(None)) + + +class TestQueueListDecryptionIntegration(unittest.TestCase): + """End-to-end: `queue list` resolves the encrypter key via the vault record, decrypts + each payload, and writes the human summary into the table cell.""" + + def setUp(self): + self.params = _mock_params() + self.key = os.urandom(32) + + def _make_item(self, queue_id, plaintext): + return cnapp_pb2.CnappQueueItem( + cnappQueueId=queue_id, + cnappProviderId=cnapp_pb2.CNAPP_PROVIDER_WIZ, + cnappQueueStatusId=1, + receivedAt=1700000000000, + networkId=b'\x00' * 16, + payload=_encrypt_cnapp_payload_for_test(json.dumps(plaintext), self.key), + ) + + def test_table_shows_decrypted_summary_when_key_resolves(self): + items = [self._make_item(101, { + 'issue': {'id': 'wiz-999', 'severity': 'CRITICAL'}, + 'control': {'name': 'Open SSH'}, + 'resource': {'name': 'prod-db-1'}, + })] + response = cnapp_pb2.CnappQueueListResponse(items=items) + config = cnapp_pb2.CnappConfiguration( + cnappConfigRecordUid=b'\xab' * 16, + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ, + ) + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', return_value=response), \ + patch.object(cnapp_commands.cnapp_helper, 'read_cnapp_configuration', return_value=config), \ + patch.object(cnapp_commands, '_load_encrypter_key', return_value=self.key): + buf = io.StringIO() + with redirect_stdout(buf): + result = cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='table', + provider='wiz', config_record_uid=None, no_decrypt=False) + output = buf.getvalue() + self.assertIn('CRITICAL', output) + self.assertIn('Open SSH', output) + self.assertIn('prod-db-1', output) + self.assertNotIn('', output, 'payload should have been decrypted') + self.assertIsNone(result) + + def test_table_marks_encrypted_when_key_unavailable(self): + items = [self._make_item(7, {'issue': {'id': 'x'}})] + response = cnapp_pb2.CnappQueueListResponse(items=items) + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', return_value=response), \ + patch.object(cnapp_commands.cnapp_helper, 'read_cnapp_configuration', + return_value=cnapp_pb2.CnappConfiguration()): + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='table', + provider='wiz', no_decrypt=False) + output = buf.getvalue() + self.assertIn('', output) + self.assertIn('No encrypter key', output) + + def test_json_includes_decrypted_payload_and_no_raw_payload(self): + plaintext = {'issue': {'id': 'wiz-42'}, 'resource': {'name': 'i-xyz'}} + response = cnapp_pb2.CnappQueueListResponse(items=[self._make_item(42, plaintext)]) + config = cnapp_pb2.CnappConfiguration(cnappConfigRecordUid=b'\xcd' * 16, + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ) + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', return_value=response), \ + patch.object(cnapp_commands.cnapp_helper, 'read_cnapp_configuration', return_value=config), \ + patch.object(cnapp_commands, '_load_encrypter_key', return_value=self.key): + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='json', + provider='wiz', no_decrypt=False) + payload = json.loads(buf.getvalue()) + self.assertEqual(payload['items'][0]['decryptedPayload']['issue']['id'], 'wiz-42') + self.assertNotIn('payload', payload['items'][0]) + + def test_decrypt_failure_keeps_other_rows_and_reports(self): + good = self._make_item(1, {'issue': {'id': 'wiz-good'}, 'control': {'name': 'OK'}}) + bad = cnapp_pb2.CnappQueueItem( + cnappQueueId=2, + cnappProviderId=cnapp_pb2.CNAPP_PROVIDER_WIZ, + cnappQueueStatusId=1, + payload=b'this-is-not-a-valid-envelope', + ) + response = cnapp_pb2.CnappQueueListResponse(items=[good, bad]) + config = cnapp_pb2.CnappConfiguration(cnappConfigRecordUid=b'\xef' * 16, + provider=cnapp_pb2.CNAPP_PROVIDER_WIZ) + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', return_value=response), \ + patch.object(cnapp_commands.cnapp_helper, 'read_cnapp_configuration', return_value=config), \ + patch.object(cnapp_commands, '_load_encrypter_key', return_value=self.key): + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='table', + provider='wiz', no_decrypt=False) + output = buf.getvalue() + self.assertIn('wiz-good', output.lower().replace('wiz-good', 'wiz-good')) + self.assertIn('failed to decrypt payload', output) + + def test_no_decrypt_flag_skips_key_lookup(self): + items = [self._make_item(11, {'issue': {'id': 'x'}})] + response = cnapp_pb2.CnappQueueListResponse(items=items) + with patch.object(cnapp_commands.cnapp_helper, 'list_cnapp_queue', return_value=response), \ + patch.object(cnapp_commands, '_load_encrypter_key') as key_loader: + buf = io.StringIO() + with redirect_stdout(buf): + cnapp_commands.PAMCnappQueueListCommand().execute( + self.params, network_uid=NETWORK_UID, status=0, format='table', + no_decrypt=True) + key_loader.assert_not_called() + self.assertNotIn('No encrypter key', buf.getvalue()) + + +if __name__ == '__main__': + unittest.main() From 0f5ab7759143ea0a84cd87fcdee68d55f01746f9 Mon Sep 17 00:00:00 2001 From: "Joao Paulo Oliveira Santos (JP)" Date: Tue, 26 May 2026 16:45:56 -0400 Subject: [PATCH 12/20] Refactor CNAPP helper to lazily import router_helper - Introduced a lazy import for `router_helper` within the `_post_request_to_router` function to avoid circular import issues during module loading. - Updated documentation to clarify the purpose of the lazy import and its implications for testing and production code paths. --- keepercommander/commands/pam/cnapp_helper.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/keepercommander/commands/pam/cnapp_helper.py b/keepercommander/commands/pam/cnapp_helper.py index 7171ceb12..b3842b3c5 100644 --- a/keepercommander/commands/pam/cnapp_helper.py +++ b/keepercommander/commands/pam/cnapp_helper.py @@ -38,11 +38,28 @@ from keeper_secrets_manager_core.utils import url_safe_str_to_bytes -from .router_helper import _post_request_to_router from ...params import KeeperParams from ...proto import cnapp_pb2 +# NOTE: `router_helper` is imported lazily inside `_post_request_to_router` below. +# Importing it at module top creates this import-time chain: +# cnapp_helper -> router_helper -> gateway_helper +# -> keepercommander.commands.utils -> commands.ksm +# -> commands.record -> commands.ksm (ksm still partially loaded — crash) +# That `record <-> ksm` cycle is pre-existing and only works because production +# code paths load `record` first. Tests that import `cnapp_helper` cold hit the +# cycle directly. Keep the indirection — do not "inline" this import. +def _post_request_to_router(params, endpoint, **kwargs): + """Lazy proxy to `router_helper._post_request_to_router`. + + Defined as a module-level function so callers (and `unittest.mock.patch.object`) + can keep referring to `cnapp_helper._post_request_to_router` as if it were the + original symbol.""" + from .router_helper import _post_request_to_router as _real_post + return _real_post(params, endpoint, **kwargs) + + # Public re-exports — let commands/tests reach proto types via the helper module so they # don't need to know the on-disk proto path. CnappProvider = cnapp_pb2.CnappProvider From 9ef4534a8890d75a695122f9c91edb00c693720f Mon Sep 17 00:00:00 2001 From: "Joao Paulo Oliveira Santos (JP)" Date: Tue, 26 May 2026 17:51:16 -0400 Subject: [PATCH 13/20] Refactor test_cnapp.py to resolve circular import issues and enhance test assertions - Pre-load `keepercommander.commands.record` to avoid circular import problems when running tests in isolation. - Adjusted import order for clarity and compliance with linting rules. - Updated test assertions in `test_decrypt_failure_keeps_other_rows_and_reports` to ensure accurate validation of output, including checks for 'good-resource' and '' in the output. --- unit-tests/pam/test_cnapp.py | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/unit-tests/pam/test_cnapp.py b/unit-tests/pam/test_cnapp.py index ab1486f3a..616561327 100644 --- a/unit-tests/pam/test_cnapp.py +++ b/unit-tests/pam/test_cnapp.py @@ -14,13 +14,20 @@ from contextlib import redirect_stdout from unittest.mock import MagicMock, patch -from cryptography.hazmat.primitives.ciphers.aead import AESGCM -from keeper_secrets_manager_core.utils import bytes_to_base64 +# Pre-load `keepercommander.commands.record` before anything else from the +# `keepercommander.commands` package. There is a pre-existing record <-> ksm +# circular import that only resolves when `record` is loaded first; running this +# file in isolation (e.g. `pytest unit-tests/pam/test_cnapp.py`) would otherwise +# hit the cycle via `discoveryrotation -> ksm -> record -> ksm`. +import keepercommander.commands.record # noqa: F401, E402 - intentional import-order guard -from keepercommander.commands.pam import cnapp_helper -from keepercommander.commands.pam import cnapp_commands -from keepercommander.error import CommandError -from keepercommander.proto import cnapp_pb2 +from cryptography.hazmat.primitives.ciphers.aead import AESGCM # noqa: E402 +from keeper_secrets_manager_core.utils import bytes_to_base64 # noqa: E402 + +from keepercommander.commands.pam import cnapp_helper # noqa: E402 +from keepercommander.commands.pam import cnapp_commands # noqa: E402 +from keepercommander.error import CommandError # noqa: E402 +from keepercommander.proto import cnapp_pb2 # noqa: E402 # Sample 16-byte UIDs as base64url (the format Commander callers pass in). @@ -797,7 +804,11 @@ def test_json_includes_decrypted_payload_and_no_raw_payload(self): self.assertNotIn('payload', payload['items'][0]) def test_decrypt_failure_keeps_other_rows_and_reports(self): - good = self._make_item(1, {'issue': {'id': 'wiz-good'}, 'control': {'name': 'OK'}}) + # Payload shape matters: `_decrypted_summary` picks `control.name` over + # `issue.id`, so to assert the good row was rendered we use a payload where + # the marker we look for is actually what surfaces in the table cell. + good = self._make_item(1, {'issue': {'id': 'wiz-good'}, + 'resource': {'name': 'good-resource'}}) bad = cnapp_pb2.CnappQueueItem( cnappQueueId=2, cnappProviderId=cnapp_pb2.CNAPP_PROVIDER_WIZ, @@ -816,7 +827,9 @@ def test_decrypt_failure_keeps_other_rows_and_reports(self): self.params, network_uid=NETWORK_UID, status=0, format='table', provider='wiz', no_decrypt=False) output = buf.getvalue() - self.assertIn('wiz-good', output.lower().replace('wiz-good', 'wiz-good')) + self.assertIn('wiz-good', output) + self.assertIn('good-resource', output) + self.assertIn('', output) self.assertIn('failed to decrypt payload', output) def test_no_decrypt_flag_skips_key_lookup(self): From 01402f46a3350f43273a965e79eaa20a549b35a6 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Wed, 27 May 2026 01:46:09 +0530 Subject: [PATCH 14/20] Fix keyring slack-app issue --- keepercommander/service/docker/setup_base.py | 54 ++------------------ 1 file changed, 3 insertions(+), 51 deletions(-) diff --git a/keepercommander/service/docker/setup_base.py b/keepercommander/service/docker/setup_base.py index 465afcb77..9bc1ff6f6 100644 --- a/keepercommander/service/docker/setup_base.py +++ b/keepercommander/service/docker/setup_base.py @@ -203,15 +203,11 @@ def _create_config_record(self, params, record_name: str, folder_uid: str) -> st def _upload_config_file(self, params, record_uid: str, config_path: str) -> None: """Upload config.json as attachment, or store as custom field if no file storage plan. - - When OS-native keychain storage is active, config.json only contains the - ``config_storage`` pointer and no credentials. In that case the essential - auth fields are sourced from ``params`` (which already has them loaded from - the keychain) and written to a temporary file for upload. + """ temp_config_path = None try: - cleaned_config_path = self._get_uploadable_config(params, config_path) + cleaned_config_path = self._clean_config_json(config_path) if cleaned_config_path != config_path: temp_config_path = cleaned_config_path @@ -230,51 +226,6 @@ def _upload_config_file(self, params, record_uid: str, config_path: str) -> None except OSError: pass - def _get_uploadable_config(self, params, config_path: str) -> str: - """Return a path to a config file with only essential auth keys for vault upload.""" - try: - with open(config_path, 'r') as f: - config_data = json.load(f) - except Exception: - return config_path - - config_storage = config_data.get('config_storage', '') - from ...config_storage.loader import _is_os_keychain_url - if _is_os_keychain_url(config_storage): - # OS-keychain mode: credentials live in the keychain, not in the file. - # Build the essential-keys dict from the already-loaded params object. - # Other backends (aws-kms://, aws-sm://, etc.) manage their own upload - # semantics and should not be rewritten here. - essential_config = {} - for file_key, params_attr in [ - ('server', 'server'), - ('user', 'user'), - ('device_token', 'device_token'), - ('private_key', 'device_private_key'), - ('clone_code', 'clone_code'), - ]: - value = getattr(params, params_attr, None) - if value: - essential_config[file_key] = value - - if not essential_config: - raise CommandError( - 'docker-setup', - 'No credentials found in params. Please log in before running this command.' - ) - - with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as tmp: - json.dump(essential_config, tmp, indent=2) - temp_path = tmp.name - - DockerSetupPrinter.print_success( - f'Config sourced from OS keychain ({len(essential_config)} essential keys)' - ) - return temp_path - - # File-based storage: clean the JSON as before. - return self._clean_config_json(config_path) - def _upload_config_as_attachment(self, params, record_uid: str, config_path: str) -> None: """Upload config.json as a file attachment (requires file storage plan).""" record = vault.KeeperRecord.load(params, record_uid) @@ -283,6 +234,7 @@ def _upload_config_as_attachment(self, params, record_uid: str, config_path: str self._delete_existing_config_attachments(record, params) upload_task = attachment.FileUploadTask(config_path) + upload_task.name = 'config.json' upload_task.title = 'config.json' attachment.upload_attachments(params, record, [upload_task]) From b451a453c21af669f0e6756d52e1061c18e35f61 Mon Sep 17 00:00:00 2001 From: Sergey Kolupaev Date: Tue, 26 May 2026 16:45:09 -0700 Subject: [PATCH 15/20] Build Docker image: Use config.json file from params --- .gitignore | 3 ++- keepercommander/service/commands/service_docker_setup.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 0f3836ed6..1b40c8dc0 100644 --- a/.gitignore +++ b/.gitignore @@ -25,4 +25,5 @@ __pycache__/ .mcp/ tests/*_results/ tests/*.log -tests/compliance_test.env \ No newline at end of file +tests/compliance_test.env +docker-compose.yml diff --git a/keepercommander/service/commands/service_docker_setup.py b/keepercommander/service/commands/service_docker_setup.py index 2228d1d1a..af74f3480 100644 --- a/keepercommander/service/commands/service_docker_setup.py +++ b/keepercommander/service/commands/service_docker_setup.py @@ -72,7 +72,7 @@ def execute(self, params, **kwargs): self._require_file_based_config(params, 'service-docker-setup') # Parse arguments - config_path = self._get_config_path(kwargs.get('config_path')) + config_path = self._get_config_path(kwargs.get('config_path') or params.config_filename) # Print header DockerSetupPrinter.print_header("Docker Setup") From 629221b07847f516f3b94e755be83b4d1dd6eb21 Mon Sep 17 00:00:00 2001 From: idimov-keeper <78815270+idimov-keeper@users.noreply.github.com> Date: Wed, 27 May 2026 13:32:47 -0500 Subject: [PATCH 16/20] Add PAM RBI file uploads/downloads and `pam connection edit --scrollback` (#2093) --- .../tunnel/port_forward/tunnel_helpers.py | 26 ++ .../commands/tunnel_and_connections.py | 114 ++++++- .../pam/test_get_config_uid_via_pam_link.py | 70 ++++ .../test_pam_connection_edit_scrollback.py | 309 ++++++++++++++++++ unit-tests/pam/test_pam_rbi_edit.py | 102 +++++- 5 files changed, 619 insertions(+), 2 deletions(-) create mode 100644 unit-tests/pam/test_get_config_uid_via_pam_link.py create mode 100644 unit-tests/pam/test_pam_connection_edit_scrollback.py diff --git a/keepercommander/commands/tunnel/port_forward/tunnel_helpers.py b/keepercommander/commands/tunnel/port_forward/tunnel_helpers.py index 82a494ff9..311b004e3 100644 --- a/keepercommander/commands/tunnel/port_forward/tunnel_helpers.py +++ b/keepercommander/commands/tunnel/port_forward/tunnel_helpers.py @@ -834,6 +834,32 @@ def get_config_uid(params, encrypted_session_token, encrypted_transmission_key, return None +def get_config_uid_via_pam_link(params, record_uid): + """Resolve a resource record's PAM Config UID via KRouter's PAM_LINK graph. + + Returns the config_uid that owns the resource. Used as a fallback when + ``get_config_uid`` (legacy ``/api/user/get_leafs`` with graphId=0) returns + nothing for resources whose link lives only in the new PAM_LINK stream. + + Returns the config_uid as a base64-url-safe string, or empty string on + failure / no link. + """ + try: + from ....keeper_dag.proto import GraphSync_pb2 as gs_pb2 + from ...pam.router_helper import _post_request_to_router + record_uid_bytes = url_safe_str_to_bytes(record_uid) + rq = gs_pb2.GraphSyncLeafsQuery(vertices=[record_uid_bytes]) + rs = _post_request_to_router(params, 'graph-sync/pam/get_leafs', + rq_proto=rq, rs_type=gs_pb2.GraphSyncRefsResult) + if rs and rs.refs: + for ref in rs.refs: + if ref.value: + return utils.base64_url_encode(ref.value) + except Exception as e: + logging.debug('get_config_uid_via_pam_link: lookup failed for %s: %s', record_uid, e) + return '' + + def get_keeper_tokens(params): transmission_key = generate_random_bytes(32) server_public_key = rest_api.SERVER_PUBLIC_KEYS[params.rest_context.server_key_id] diff --git a/keepercommander/commands/tunnel_and_connections.py b/keepercommander/commands/tunnel_and_connections.py index 77ef48fff..050d7b962 100644 --- a/keepercommander/commands/tunnel_and_connections.py +++ b/keepercommander/commands/tunnel_and_connections.py @@ -29,7 +29,8 @@ from .base import Command, GroupCommand, dump_report_data, RecordMixin from .tunnel.port_forward.TunnelGraph import TunnelDAG -from .tunnel.port_forward.tunnel_helpers import find_open_port, get_config_uid, get_keeper_tokens, \ +from .tunnel.port_forward.tunnel_helpers import find_open_port, get_config_uid, get_config_uid_via_pam_link, \ + get_keeper_tokens, \ get_or_create_tube_registry, get_gateway_uid_from_record, resolve_record, resolve_pam_config, resolve_folder, \ remove_field, start_rust_tunnel, get_tunnel_session, unregister_tunnel_session, CloseConnectionReasons, \ wait_for_tunnel_connection, create_rust_webrtc_settings, escalate_close, \ @@ -2624,6 +2625,10 @@ class PAMConnectionEditCommand(Command): 'the port from the record will be used.') parser.add_argument('--key-events', '-k', dest='key_events', choices=choices, help='Toggle Key Events settings') + parser.add_argument('--scrollback', '-sb', required=False, dest='scrollback', action='store', + help='Maximum Scrollback Size (terminal history). Integer to set, ' + 'empty string to remove. Supported only for pamDatabase (any DB protocol) and ' + 'pamMachine/pamDirectory (ssh/telnet/kubernetes).') parser.add_argument('--rotate-on-termination', required=False, dest='rotate_on_termination', choices=['on', 'off'], help='Rotate launch credentials when the PAM session ends (DAG resource meta)') @@ -2672,6 +2677,51 @@ def execute(self, params, **kwargs): f"pamRemoteBrowser, pamNetworkConfiguration pamAwsConfiguration, and " f"pamAzureConfiguration records{bcolors.ENDC}") + # --scrollback: validate record type + effective protocol before any mutation + scrollback_arg = kwargs.get('scrollback', None) + scrollback_clear = False + scrollback_value = None # parsed int, or None to skip apply + if scrollback_arg is not None: + db_scrollback_protocols = {'mysql', 'postgresql', 'sql-server', 'mariadb', 'oracle', + 'mongodb', 'redis', 'elasticsearch', 'clickhouse', 'dynamodb'} + terminal_scrollback_protocols = {'ssh', 'telnet', 'kubernetes'} + if record_type == 'pamDatabase': + allowed_protocols = db_scrollback_protocols + elif record_type in ('pamMachine', 'pamDirectory'): + allowed_protocols = terminal_scrollback_protocols + else: + raise CommandError('pam connection edit', + f'{bcolors.FAIL}--scrollback is only supported for pamDatabase, pamMachine, and pamDirectory ' + f'records. Record "{record_uid}" is of type "{record_type}".{bcolors.ENDC}') + + existing_ps = record.get_typed_field('pamSettings') + existing_protocol = '' + if existing_ps and existing_ps.value and isinstance(existing_ps.value[0], dict): + existing_protocol = existing_ps.value[0].get('connection', {}).get('protocol') or '' + new_protocol_arg = kwargs.get('protocol', None) + if kwargs.get('connections') == 'on' and new_protocol_arg is not None: + effective_protocol = new_protocol_arg # may be '' to clear + else: + effective_protocol = existing_protocol + if effective_protocol not in allowed_protocols: + raise CommandError('pam connection edit', + f'{bcolors.FAIL}--scrollback is not supported for protocol "{effective_protocol or "(unset)"}" ' + f'on {record_type} records. Allowed protocols: {", ".join(sorted(allowed_protocols))}.{bcolors.ENDC}') + + if scrollback_arg == '': + scrollback_clear = True + else: + try: + scrollback_value = int(scrollback_arg) + except (ValueError, TypeError): + raise CommandError('pam connection edit', + f'{bcolors.FAIL}--scrollback must be a non-negative integer or empty string. ' + f'Got: "{scrollback_arg}".{bcolors.ENDC}') + if scrollback_value < 0: + raise CommandError('pam connection edit', + f'{bcolors.FAIL}--scrollback must be a non-negative integer or empty string. ' + f'Got: "{scrollback_arg}".{bcolors.ENDC}') + encrypted_session_token, encrypted_transmission_key, transmission_key = get_keeper_tokens(params) if record_type in "pamNetworkConfiguration pamAwsConfiguration pamAzureConfiguration".split(): tdag = TunnelDAG(params, encrypted_session_token, encrypted_transmission_key, record_uid, is_config=True, @@ -2758,6 +2808,24 @@ def execute(self, params, **kwargs): else: logging.debug(f'Unexpected value for --key-events {key_events} (ignored)') + # --scrollback: apply (validated above; record_type + effective protocol already checked) + if scrollback_clear or scrollback_value is not None: + psv = pam_settings.value[0] if pam_settings and pam_settings.value else {} + vcon = psv.get('connection', {}) if isinstance(psv, dict) else {} + current_sb = vcon.get('scrollback') if isinstance(vcon, dict) else None + if scrollback_clear: + if current_sb is not None: + pam_settings.value[0]["connection"].pop('scrollback', None) + dirty = True + else: + logging.debug(f'scrollback is already unset on record={record_uid}') + else: + if current_sb != scrollback_value: + pam_settings.value[0]["connection"]["scrollback"] = scrollback_value + dirty = True + else: + logging.debug(f'scrollback is already {scrollback_value} on record={record_uid}') + if dirty: record_management.update_record(params, record) api.sync_down(params) @@ -2768,8 +2836,36 @@ def execute(self, params, **kwargs): f"Please make sure you have edit rights to record {record_uid} {bcolors.ENDC}") dirty = False + # If only record-level args were passed (e.g. --scrollback, --key-events, --protocol + # alone), the record update above is complete — skip the DAG/config lookup, which + # would otherwise raise a misleading "No PAM Configuration UID set" error when the + # resource isn't linked to a config yet. Mirrors the PAMRbiEditCommand pattern. + dag_affecting = (kwargs.get('config') or kwargs.get('admin') or kwargs.get('launch_user') + or kwargs.get('clear_launch_user') or kwargs.get('connections') + or kwargs.get('recording') or kwargs.get('typescriptrecording') + or kwargs.get('rotate_on_termination')) + if not dag_affecting: + return + existing_config_uid = get_config_uid(params, encrypted_session_token, encrypted_transmission_key, record_uid) + # When the user did not pass --configuration, fall back to + # first the legacy /api/user/get_leafs if that also returned nothing, + # then the new /api/user/graph-sync/pam/get_leafs (PAM_LINK stream) + # Without this, `tdag` would be built with config_uid=None, has_graph=False, + # and the misleading "No PAM Configuration UID set" error would fire + # even though the link is resolvable. + if not config_uid: + if existing_config_uid: + config_uid = existing_config_uid + logging.debug('pam connection edit: using DAG-resolved config_uid: %s', config_uid) + else: + found = get_config_uid_via_pam_link(params, record_uid) + if found: + logging.debug('pam connection edit: resolved config_uid via graph-sync/pam fallback: %s', found) + existing_config_uid = found + config_uid = found + tdag = TunnelDAG(params, encrypted_session_token, encrypted_transmission_key, config_uid, transmission_key=transmission_key) old_dag = TunnelDAG(params, encrypted_session_token, encrypted_transmission_key, existing_config_uid, @@ -3149,6 +3245,10 @@ class PAMRbiEditCommand(Command): help='Allow navigation via direct URL manipulation (on/off/default)') parser.add_argument('--ignore-server-cert', '-isc', dest='ignore_server_cert', choices=choices, help='Ignore server certificate errors (on/off/default)') + parser.add_argument('--allow-file-uploads', '-fu', dest='allow_file_uploads', choices=choices, + help='Allow file uploads in RBI sessions (on/off/default)') + parser.add_argument('--allow-file-downloads', '-fd', dest='allow_file_downloads', choices=choices, + help='Allow file downloads in RBI sessions (on/off/default)') # URL Filtering parser.add_argument('--allowed-urls', '-au', dest='allowed_urls', action='append', @@ -3197,6 +3297,8 @@ def execute(self, params, **kwargs): # New RBI settings (Phase 1 - KC-1034) allow_url_navigation = kwargs.get('allow_url_navigation') # on/off/default/None ignore_server_cert = kwargs.get('ignore_server_cert') # on/off/default/None + allow_file_uploads = kwargs.get('allow_file_uploads') # on/off/default/None + allow_file_downloads = kwargs.get('allow_file_downloads') # on/off/default/None allowed_urls = kwargs.get('allowed_urls') # list or None allowed_resource_urls = kwargs.get('allowed_resource_urls') # list or None autofill_targets = kwargs.get('autofill_targets') # list or None @@ -3214,6 +3316,8 @@ def execute(self, params, **kwargs): has_new_settings = any([ allow_url_navigation is not None, ignore_server_cert is not None, + allow_file_uploads is not None, + allow_file_downloads is not None, allowed_urls is not None, allowed_resource_urls is not None, autofill_targets is not None, @@ -3389,6 +3493,14 @@ def update_connection_int(field_name, value): if ignore_server_cert: update_connection_toggle('ignoreInitialSslCert', ignore_server_cert) + # Browser Settings - allowFileUploads (on/off/default) + if allow_file_uploads: + update_connection_toggle('allowFileUploads', allow_file_uploads) + + # Browser Settings - allowFileDownloads (on/off/default) + if allow_file_downloads: + update_connection_toggle('allowFileDownloads', allow_file_downloads) + # URL Filtering - allowedUrlPatterns (multi-value, joined with newlines) if allowed_urls is not None: update_connection_string('allowedUrlPatterns', allowed_urls) diff --git a/unit-tests/pam/test_get_config_uid_via_pam_link.py b/unit-tests/pam/test_get_config_uid_via_pam_link.py new file mode 100644 index 000000000..54735fa23 --- /dev/null +++ b/unit-tests/pam/test_get_config_uid_via_pam_link.py @@ -0,0 +1,70 @@ +""" +Unit tests for tunnel_helpers.get_config_uid_via_pam_link — the KRouter +graph-sync/pam/get_leafs fallback used by `pam connection edit` when the +legacy get_leafs lookup misses. Mirrors the web vault's +`getMultiLeafsPamLinkDag` call (see vault/js/lib/api/pam/api-dag-pam-link.ts). +""" + +import unittest +from unittest import mock + +skip_tests = False +skip_reason = "" +try: + from keepercommander.commands.tunnel.port_forward.tunnel_helpers import get_config_uid_via_pam_link + from keepercommander.keeper_dag.proto import GraphSync_pb2 as gs_pb2 + from keepercommander import utils +except ImportError as e: + skip_tests = True + skip_reason = f"Cannot import tunnel_helpers/GraphSync_pb2: {e}" + + +@unittest.skipIf(skip_tests, skip_reason) +class TestGetConfigUidViaPamLink(unittest.TestCase): + def setUp(self): + self.params = mock.MagicMock() + self.record_uid = 'AAAAAAAAAAAAAAAAAAAAAA' # roundtrip-safe base64url for 16 bytes + self.config_uid = 'AQEBAQEBAQEBAQEBAQEBAQ' # roundtrip-safe base64url for 16 bytes + + @mock.patch('keepercommander.commands.pam.router_helper._post_request_to_router') + def test_returns_config_uid_on_single_ref(self, mock_post): + cfg_bytes = utils.base64_url_decode(self.config_uid) + mock_post.return_value = gs_pb2.GraphSyncRefsResult( + refs=[gs_pb2.GraphSyncRef(type=gs_pb2.RFT_PAM_NETWORK, value=cfg_bytes, name='')] + ) + result = get_config_uid_via_pam_link(self.params, self.record_uid) + self.assertEqual(result, self.config_uid) + # Verify endpoint + query shape + args, kwargs = mock_post.call_args + self.assertEqual(args[1], 'graph-sync/pam/get_leafs') + rq = kwargs.get('rq_proto') or args[2] + self.assertEqual(len(rq.vertices), 1) + self.assertEqual(rq.vertices[0], utils.base64_url_decode(self.record_uid)) + + @mock.patch('keepercommander.commands.pam.router_helper._post_request_to_router') + def test_returns_empty_string_when_no_refs(self, mock_post): + mock_post.return_value = gs_pb2.GraphSyncRefsResult(refs=[]) + self.assertEqual(get_config_uid_via_pam_link(self.params, self.record_uid), '') + + @mock.patch('keepercommander.commands.pam.router_helper._post_request_to_router') + def test_returns_empty_string_when_response_is_none(self, mock_post): + mock_post.return_value = None + self.assertEqual(get_config_uid_via_pam_link(self.params, self.record_uid), '') + + @mock.patch('keepercommander.commands.pam.router_helper._post_request_to_router') + def test_skips_refs_with_empty_value(self, mock_post): + cfg_bytes = utils.base64_url_decode(self.config_uid) + mock_post.return_value = gs_pb2.GraphSyncRefsResult(refs=[ + gs_pb2.GraphSyncRef(type=gs_pb2.RFT_PAM_NETWORK, value=b'', name=''), + gs_pb2.GraphSyncRef(type=gs_pb2.RFT_PAM_NETWORK, value=cfg_bytes, name=''), + ]) + self.assertEqual(get_config_uid_via_pam_link(self.params, self.record_uid), self.config_uid) + + @mock.patch('keepercommander.commands.pam.router_helper._post_request_to_router') + def test_swallows_exceptions_and_returns_empty(self, mock_post): + mock_post.side_effect = RuntimeError('krouter unreachable') + self.assertEqual(get_config_uid_via_pam_link(self.params, self.record_uid), '') + + +if __name__ == '__main__': + unittest.main() diff --git a/unit-tests/pam/test_pam_connection_edit_scrollback.py b/unit-tests/pam/test_pam_connection_edit_scrollback.py new file mode 100644 index 000000000..7f758652d --- /dev/null +++ b/unit-tests/pam/test_pam_connection_edit_scrollback.py @@ -0,0 +1,309 @@ +""" +Unit tests for PAM Connection Edit `--scrollback` flag. + +Covers argument parsing and the up-front validation that runs before any DAG / +record mutation: allowed record types (pamDatabase, pamMachine, pamDirectory), +allowed protocols per type, and value parsing (int / empty string / invalid). +""" + +import unittest +from unittest import mock + +skip_tests = False +skip_reason = "" +try: + from keepercommander.commands.tunnel_and_connections import PAMConnectionEditCommand + from keepercommander.error import CommandError + from keepercommander import vault +except ImportError as e: + skip_tests = True + skip_reason = f"Cannot import tunnel_and_connections: {e}" + + +@unittest.skipIf(skip_tests, skip_reason) +class TestPamConnectionEditScrollbackArgs(unittest.TestCase): + def setUp(self): + self.parser = PAMConnectionEditCommand.parser + + def test_scrollback_int(self): + args = self.parser.parse_args(['rec', '--scrollback', '1234']) + self.assertEqual(args.scrollback, '1234') + + def test_scrollback_empty_string(self): + args = self.parser.parse_args(['rec', '--scrollback', '']) + self.assertEqual(args.scrollback, '') + + def test_scrollback_short_alias(self): + args = self.parser.parse_args(['rec', '-sb', '5000']) + self.assertEqual(args.scrollback, '5000') + + def test_scrollback_not_provided(self): + args = self.parser.parse_args(['rec']) + self.assertIsNone(args.scrollback) + + def test_help_includes_scrollback(self): + help_text = self.parser.format_help() + self.assertIn('--scrollback', help_text) + self.assertIn('-sb', help_text) + + +@unittest.skipIf(skip_tests, skip_reason) +class TestPamConnectionEditScrollbackValidation(unittest.TestCase): + """Validation runs before DAG / token operations, so we can drive execute() + with mocks that only need to satisfy resolve_single_record + the typed-field + accessor for pamSettings.""" + + def _mock_record(self, record_type, protocol): + rec = mock.MagicMock(spec=vault.TypedRecord) + rec.record_uid = 'rec-uid' + rec.record_type = record_type + rec.version = 3 + ps_field = mock.MagicMock() + if protocol is None: + ps_field.value = [] + else: + ps_field.value = [{'connection': {'protocol': protocol}}] + rec.get_typed_field.side_effect = lambda name: ps_field if name == 'pamSettings' else None + return rec + + def _execute(self, record, **kwargs): + cmd = PAMConnectionEditCommand() + params = mock.MagicMock() + with mock.patch( + 'keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record', + return_value=record, + ): + cmd.execute(params, record='rec', **kwargs) + + def test_pam_remote_browser_rejected(self): + rec = self._mock_record('pamRemoteBrowser', 'http') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='100') + self.assertIn('--scrollback is only supported for pamDatabase, pamMachine, and pamDirectory', + str(ctx.exception)) + + def test_pam_user_rejected(self): + rec = self._mock_record('pamUser', None) + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='100') + # pamUser fails the outer record-type check before scrollback validation runs + self.assertIn("type is not supported for connections", str(ctx.exception)) + + def test_pam_network_configuration_rejected(self): + rec = self._mock_record('pamNetworkConfiguration', None) + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='100') + self.assertIn('--scrollback is only supported for pamDatabase, pamMachine, and pamDirectory', + str(ctx.exception)) + + def test_pam_machine_rdp_rejected(self): + rec = self._mock_record('pamMachine', 'rdp') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='100') + msg = str(ctx.exception) + self.assertIn('not supported for protocol "rdp"', msg) + self.assertIn('pamMachine', msg) + + def test_pam_machine_vnc_rejected(self): + rec = self._mock_record('pamMachine', 'vnc') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='100') + self.assertIn('not supported for protocol "vnc"', str(ctx.exception)) + + def test_pam_machine_no_protocol_rejected(self): + rec = self._mock_record('pamMachine', None) + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='100') + self.assertIn('not supported for protocol "(unset)"', str(ctx.exception)) + + def test_pam_directory_http_rejected(self): + rec = self._mock_record('pamDirectory', 'http') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='100') + self.assertIn('not supported for protocol "http"', str(ctx.exception)) + + def test_non_numeric_rejected(self): + rec = self._mock_record('pamMachine', 'ssh') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='not-a-number') + self.assertIn('--scrollback must be a non-negative integer', str(ctx.exception)) + + def test_float_rejected(self): + rec = self._mock_record('pamMachine', 'ssh') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='1.5') + self.assertIn('--scrollback must be a non-negative integer', str(ctx.exception)) + + def test_negative_integer_rejected(self): + rec = self._mock_record('pamMachine', 'ssh') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='-100') + self.assertIn('--scrollback must be a non-negative integer', str(ctx.exception)) + + def test_negative_one_rejected(self): + rec = self._mock_record('pamMachine', 'ssh') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='-1') + self.assertIn('--scrollback must be a non-negative integer', str(ctx.exception)) + + def test_zero_accepted(self): + """Zero is a non-negative integer and should pass validation.""" + rec = self._mock_record('pamMachine', 'ssh') + try: + self._execute(rec, scrollback='0') + except CommandError as e: + self.assertNotIn('--scrollback must be', str(e)) + except Exception: + pass # downstream DAG failure expected; only validation is under test + + def test_protocol_change_in_same_command_validated_against_new(self): + """When --connections=on and --protocol=rdp are passed alongside --scrollback, + validation uses the new (post-mutation) protocol — rdp -> reject.""" + rec = self._mock_record('pamMachine', 'ssh') + with self.assertRaises(CommandError) as ctx: + self._execute(rec, scrollback='100', connections='on', protocol='rdp') + self.assertIn('not supported for protocol "rdp"', str(ctx.exception)) + + def test_protocol_change_without_connections_uses_existing(self): + """--protocol is only honored alongside --connections=on; without it, + validation uses the existing record protocol.""" + rec = self._mock_record('pamMachine', 'ssh') + # Without --connections=on, the bogus --protocol is ignored, existing + # 'ssh' wins. Validation should not raise (it gets past the protocol + # check); we expect it to proceed to the DAG layer and fail there. + # We just assert the error is NOT the scrollback-protocol error. + with self.assertRaises(Exception) as ctx: + self._execute(rec, scrollback='100', protocol='rdp') + self.assertNotIn('not supported for protocol', str(ctx.exception)) + + +@unittest.skipIf(skip_tests, skip_reason) +class TestPamConnectionEditScrollbackAllowedCombinations(unittest.TestCase): + """For each allowed (record_type, protocol) pair, validation must not raise + a scrollback-related error. We don't run the full execute path (which would + require mocking the entire DAG layer), only verify validation passes.""" + + DB_PROTOCOLS = ['mysql', 'postgresql', 'sql-server', 'mariadb', 'oracle', + 'mongodb', 'redis', 'elasticsearch', 'clickhouse', 'dynamodb'] + TERMINAL_PROTOCOLS = ['ssh', 'telnet', 'kubernetes'] + + def _assert_validation_passes(self, record_type, protocol): + rec = mock.MagicMock(spec=vault.TypedRecord) + rec.record_uid = 'rec-uid' + rec.record_type = record_type + rec.version = 3 + ps_field = mock.MagicMock() + ps_field.value = [{'connection': {'protocol': protocol}}] + rec.get_typed_field.side_effect = lambda name: ps_field if name == 'pamSettings' else None + + cmd = PAMConnectionEditCommand() + params = mock.MagicMock() + with mock.patch( + 'keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record', + return_value=rec, + ): + try: + cmd.execute(params, record='rec', scrollback='100') + except CommandError as e: + self.assertNotIn('--scrollback is only supported', str(e)) + self.assertNotIn('--scrollback is not supported for protocol', str(e)) + self.assertNotIn('--scrollback must be an integer', str(e)) + except Exception: + pass # downstream DAG/token failures are not what we're testing + + def test_pam_database_all_db_protocols(self): + for proto in self.DB_PROTOCOLS: + with self.subTest(protocol=proto): + self._assert_validation_passes('pamDatabase', proto) + + def test_pam_machine_terminal_protocols(self): + for proto in self.TERMINAL_PROTOCOLS: + with self.subTest(protocol=proto): + self._assert_validation_passes('pamMachine', proto) + + def test_pam_directory_terminal_protocols(self): + for proto in self.TERMINAL_PROTOCOLS: + with self.subTest(protocol=proto): + self._assert_validation_passes('pamDirectory', proto) + + +@unittest.skipIf(skip_tests, skip_reason) +class TestPamConnectionEditScrollbackEarlyReturn(unittest.TestCase): + """When only record-level args (scrollback, key-events, protocol alone) are passed, + the command should return after the record update without touching the DAG. Locks in + the fix for the misleading 'No PAM Configuration UID set' error when --scrollback is + used on a resource that isn't linked to a config.""" + + def _mock_record(self, record_type='pamMachine', protocol='ssh'): + rec = mock.MagicMock(spec=vault.TypedRecord) + rec.record_uid = 'rec-uid' + rec.record_type = record_type + rec.version = 3 + rec.fields = [] + rec.custom = [] + ps_field = mock.MagicMock() + ps_field.value = [{'connection': {'protocol': protocol}}] + rec.get_typed_field.side_effect = lambda name: ps_field if name == 'pamSettings' else ( + mock.MagicMock(value=['seed']) if name == 'trafficEncryptionSeed' else None + ) + return rec + + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + @mock.patch('keepercommander.commands.tunnel_and_connections.get_keeper_tokens', + return_value=(b'st', b'tk', b'tr')) + @mock.patch('keepercommander.commands.tunnel_and_connections.get_config_uid') + @mock.patch('keepercommander.commands.tunnel_and_connections.TunnelDAG') + def test_scrollback_alone_skips_dag(self, mock_tdag, mock_get_config_uid, + mock_tokens, mock_sync, mock_update, mock_resolve): + """Running with only --scrollback should NOT invoke get_config_uid or TunnelDAG.""" + rec = self._mock_record() + mock_resolve.return_value = rec + cmd = PAMConnectionEditCommand() + cmd.execute(mock.MagicMock(), record='rec', scrollback='1234') + mock_get_config_uid.assert_not_called() + mock_tdag.assert_not_called() + # The record update IS expected to run (scrollback was written) + mock_update.assert_called_once() + + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + @mock.patch('keepercommander.commands.tunnel_and_connections.get_keeper_tokens', + return_value=(b'st', b'tk', b'tr')) + @mock.patch('keepercommander.commands.tunnel_and_connections.get_config_uid') + @mock.patch('keepercommander.commands.tunnel_and_connections.TunnelDAG') + def test_key_events_alone_skips_dag(self, mock_tdag, mock_get_config_uid, + mock_tokens, mock_sync, mock_update, mock_resolve): + """Same early-return applies to --key-events alone.""" + rec = self._mock_record() + mock_resolve.return_value = rec + cmd = PAMConnectionEditCommand() + cmd.execute(mock.MagicMock(), record='rec', key_events='on') + mock_get_config_uid.assert_not_called() + mock_tdag.assert_not_called() + + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + @mock.patch('keepercommander.commands.tunnel_and_connections.get_keeper_tokens', + return_value=(b'st', b'tk', b'tr')) + @mock.patch('keepercommander.commands.tunnel_and_connections.get_config_uid', + return_value=None) + def test_scrollback_with_connections_still_runs_dag(self, mock_get_config_uid, + mock_tokens, mock_sync, mock_update, mock_resolve): + """When --connections is passed alongside --scrollback, the DAG block must still run + (and is expected to surface its own errors). We just verify get_config_uid is reached.""" + rec = self._mock_record() + mock_resolve.return_value = rec + cmd = PAMConnectionEditCommand() + try: + cmd.execute(mock.MagicMock(), record='rec', scrollback='1234', connections='on') + except Exception: + pass # downstream TunnelDAG instantiation will fail; not under test here + mock_get_config_uid.assert_called_once() + + +if __name__ == '__main__': + unittest.main() diff --git a/unit-tests/pam/test_pam_rbi_edit.py b/unit-tests/pam/test_pam_rbi_edit.py index 94c91ee84..ce52be84d 100644 --- a/unit-tests/pam/test_pam_rbi_edit.py +++ b/unit-tests/pam/test_pam_rbi_edit.py @@ -2,7 +2,7 @@ Unit tests for PAM RBI Edit command - KC-1034 Feature Parity Tests the new CLI arguments added to expose RBI settings: -- Browser Settings: --allow-url-navigation, --ignore-server-cert (on/off/default) +- Browser Settings: --allow-url-navigation, --ignore-server-cert, --allow-file-uploads, --allow-file-downloads (on/off/default) - URL Filtering: --allowed-urls, --allowed-resource-urls (multi-value) - Autofill: --autofill-targets (multi-value) - Clipboard: --allow-copy, --allow-paste (on/off/default) @@ -67,6 +67,46 @@ def test_ignore_server_cert_default(self): args = self.parser.parse_args(['--record', 'test-record', '--ignore-server-cert', 'default']) self.assertEqual(args.ignore_server_cert, 'default') + def test_allow_file_uploads_on(self): + args = self.parser.parse_args(['--record', 'test-record', '--allow-file-uploads', 'on']) + self.assertEqual(args.allow_file_uploads, 'on') + + def test_allow_file_uploads_off(self): + args = self.parser.parse_args(['--record', 'test-record', '--allow-file-uploads', 'off']) + self.assertEqual(args.allow_file_uploads, 'off') + + def test_allow_file_uploads_default(self): + args = self.parser.parse_args(['--record', 'test-record', '--allow-file-uploads', 'default']) + self.assertEqual(args.allow_file_uploads, 'default') + + def test_allow_file_uploads_invalid(self): + with self.assertRaises(SystemExit): + self.parser.parse_args(['--record', 'test-record', '--allow-file-uploads', 'invalid']) + + def test_allow_file_uploads_not_provided(self): + args = self.parser.parse_args(['--record', 'test-record', '--key-events', 'on']) + self.assertIsNone(args.allow_file_uploads) + + def test_allow_file_downloads_on(self): + args = self.parser.parse_args(['--record', 'test-record', '--allow-file-downloads', 'on']) + self.assertEqual(args.allow_file_downloads, 'on') + + def test_allow_file_downloads_off(self): + args = self.parser.parse_args(['--record', 'test-record', '--allow-file-downloads', 'off']) + self.assertEqual(args.allow_file_downloads, 'off') + + def test_allow_file_downloads_default(self): + args = self.parser.parse_args(['--record', 'test-record', '--allow-file-downloads', 'default']) + self.assertEqual(args.allow_file_downloads, 'default') + + def test_allow_file_downloads_invalid(self): + with self.assertRaises(SystemExit): + self.parser.parse_args(['--record', 'test-record', '--allow-file-downloads', 'invalid']) + + def test_allow_file_downloads_not_provided(self): + args = self.parser.parse_args(['--record', 'test-record', '--key-events', 'on']) + self.assertIsNone(args.allow_file_downloads) + def test_allowed_urls_single(self): args = self.parser.parse_args(['--record', 'test-record', '--allowed-urls', '*.example.com']) self.assertEqual(args.allowed_urls, ['*.example.com']) @@ -214,6 +254,56 @@ def test_ignore_server_cert_on_sets_true(self, mock_sync, mock_update, mock_reso self.command.execute(self.mock_params, record='test-record', ignore_server_cert='on') self.assertEqual(self.pam_settings['connection'].get('ignoreInitialSslCert'), True) + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + def test_allow_file_uploads_on_sets_true(self, mock_sync, mock_update, mock_resolve): + mock_resolve.return_value = self.mock_record + self.command.execute(self.mock_params, record='test-record', allow_file_uploads='on') + self.assertEqual(self.pam_settings['connection'].get('allowFileUploads'), True) + + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + def test_allow_file_uploads_off_sets_false(self, mock_sync, mock_update, mock_resolve): + mock_resolve.return_value = self.mock_record + self.command.execute(self.mock_params, record='test-record', allow_file_uploads='off') + self.assertEqual(self.pam_settings['connection'].get('allowFileUploads'), False) + + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + def test_allow_file_uploads_default_removes_field(self, mock_sync, mock_update, mock_resolve): + mock_resolve.return_value = self.mock_record + self.pam_settings['connection']['allowFileUploads'] = True + self.command.execute(self.mock_params, record='test-record', allow_file_uploads='default') + self.assertNotIn('allowFileUploads', self.pam_settings['connection']) + + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + def test_allow_file_downloads_on_sets_true(self, mock_sync, mock_update, mock_resolve): + mock_resolve.return_value = self.mock_record + self.command.execute(self.mock_params, record='test-record', allow_file_downloads='on') + self.assertEqual(self.pam_settings['connection'].get('allowFileDownloads'), True) + + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + def test_allow_file_downloads_off_sets_false(self, mock_sync, mock_update, mock_resolve): + mock_resolve.return_value = self.mock_record + self.command.execute(self.mock_params, record='test-record', allow_file_downloads='off') + self.assertEqual(self.pam_settings['connection'].get('allowFileDownloads'), False) + + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') + @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') + def test_allow_file_downloads_default_removes_field(self, mock_sync, mock_update, mock_resolve): + mock_resolve.return_value = self.mock_record + self.pam_settings['connection']['allowFileDownloads'] = True + self.command.execute(self.mock_params, record='test-record', allow_file_downloads='default') + self.assertNotIn('allowFileDownloads', self.pam_settings['connection']) + @mock.patch('keepercommander.commands.tunnel_and_connections.RecordMixin.resolve_single_record') @mock.patch('keepercommander.commands.tunnel_and_connections.record_management.update_record') @mock.patch('keepercommander.commands.tunnel_and_connections.api.sync_down') @@ -356,6 +446,8 @@ def test_help_includes_new_arguments(self): help_text = PAMRbiEditCommand.parser.format_help() self.assertIn('--allow-url-navigation', help_text) self.assertIn('--ignore-server-cert', help_text) + self.assertIn('--allow-file-uploads', help_text) + self.assertIn('--allow-file-downloads', help_text) self.assertIn('--allowed-urls', help_text) self.assertIn('--allowed-resource-urls', help_text) self.assertIn('--autofill-targets', help_text) @@ -386,6 +478,14 @@ def test_alias_isc(self): args = self.parser.parse_args(['--record', 'test-record', '-isc', 'on']) self.assertEqual(args.ignore_server_cert, 'on') + def test_alias_fu(self): + args = self.parser.parse_args(['--record', 'test-record', '-fu', 'on']) + self.assertEqual(args.allow_file_uploads, 'on') + + def test_alias_fd(self): + args = self.parser.parse_args(['--record', 'test-record', '-fd', 'on']) + self.assertEqual(args.allow_file_downloads, 'on') + def test_alias_au(self): args = self.parser.parse_args(['--record', 'test-record', '-au', '*.example.com']) self.assertEqual(args.allowed_urls, ['*.example.com']) From a1b8d5285558489a1a82dd8fbf6343143f625403 Mon Sep 17 00:00:00 2001 From: sshrushanth-ks Date: Thu, 28 May 2026 21:01:16 +0530 Subject: [PATCH 17/20] KC-1260: Support combined folder search with multiple -c flags (#2084) (#2096) * Support combined folder search with multiple -c flags search -c s -c d now returns Classic shared folders and KeeperDrive folders together in one JSON response. Changed -c from action='store' to action='append' so repeated flags accumulate instead of overwriting. JSON details field now shows Folder Category: Classic or Folder Category: KeeperDrive for folder results. * Fix nested_share_folder details always shown in JSON search output NSF folders with no parent UID previously rendered an empty details field. Now always shows Folder Category: NestedShare, with Parent UID appended when the folder is not at root level. * Fix help text for -c flag to use Nested Share Folders instead of KeeperDrive --- keepercommander/commands/record.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/keepercommander/commands/record.py b/keepercommander/commands/record.py index a507c0bd6..422243d21 100644 --- a/keepercommander/commands/record.py +++ b/keepercommander/commands/record.py @@ -143,9 +143,11 @@ def register_command_info(aliases, command_info): search_parser = argparse.ArgumentParser(prog='search', description='Search the vault. Words can be in any order.') search_parser.add_argument('pattern', nargs='*', type=str, action='store', help='search terms (space-separated, order independent)') search_parser.add_argument('-v', '--verbose', dest='verbose', action='store_true', help='verbose output') -search_parser.add_argument('-c', '--categories', dest='categories', action='store', - help='One or more of these letters for categories to search: "r" = records, ' - '"s" = shared folders, "t" = teams, "d" = Nested Share Folders') +search_parser.add_argument('-c', '--categories', dest='categories', action='append', + help='Category to search — repeatable: "r" = records, "s" = shared folders, ' + '"t" = teams, "d" = Nested Share folders. ' + 'Pass multiple times (e.g. -c s -c d) or combine letters (e.g. -c sd). ' + 'Default when omitted: all categories (rstd).') search_parser.add_argument('--regex', dest='regex', action='store_true', help='treat pattern as a regular expression instead of space-separated search terms') search_parser.add_argument('--device', dest='device', action='store_true', @@ -1455,7 +1457,7 @@ def execute(self, params, **kwargs): else: pattern = '' # Empty pattern matches all in token mode - categories = (kwargs.get('categories') or 'rstd').lower() + categories = ''.join(kwargs.get('categories') or ['rstd']).lower() skip_details = not verbose nsf_records_map = getattr(params, 'nested_share_records', {}) or {} @@ -1575,13 +1577,14 @@ def execute(self, params, **kwargs): f"Type: {item['record_type']}, Description: {item['description']}, Record Category: {item.get('record_category', 'Classic')}"] elif item['type'] == 'shared_folder': row = [item['type'], item['shared_folder_uid'], item['name'], - f"Can Edit: {item['can_edit']}, Can Share: {item['can_share']}"] + f"Folder Category: Classic, Can Edit: {item['can_edit']}, Can Share: {item['can_share']}"] elif item['type'] == 'team': row = [item['type'], item['team_uid'], item['name'], f"Restrict Edit: {item['restrict_edit']}, Restrict View: {item['restrict_view']}, Restrict Share: {item['restrict_share']}"] elif item['type'] == 'nested_share_folder': - details = (f"Parent UID: {item['parent_uid']}" - if item.get('parent_uid') else '') + details = 'Folder Category: NestedShare' + if item.get('parent_uid'): + details += f", Parent UID: {item['parent_uid']}" row = [item['type'], item['folder_uid'], item['name'], details] table.append(row) From 99cb2b4cd8a073daf0b2c462adcfc813156c343b Mon Sep 17 00:00:00 2001 From: pvagare-ks Date: Fri, 29 May 2026 17:10:50 +0530 Subject: [PATCH 18/20] Fix NSF CLI record updates, nested directory creation and Service Mode delete response handling (#2097) --- .../nested_share_folder/folder_commands.py | 112 +++++++++++++----- .../commands/nested_share_folder/parsers.py | 4 +- .../nested_share_folder/record_commands.py | 52 ++++++-- .../nested_share_folder/folder_api.py | 13 +- .../nested_share_folder/record_api.py | 4 +- 5 files changed, 139 insertions(+), 46 deletions(-) diff --git a/keepercommander/commands/nested_share_folder/folder_commands.py b/keepercommander/commands/nested_share_folder/folder_commands.py index fa1218a84..57c04a7af 100644 --- a/keepercommander/commands/nested_share_folder/folder_commands.py +++ b/keepercommander/commands/nested_share_folder/folder_commands.py @@ -61,52 +61,102 @@ def execute(self, params, **kwargs): if current and current in getattr(params, 'nested_share_folders', {}): base_folder_uid = current - folder_name = self._parse_path(folder_path) + segments = self._parse_path(folder_path) + + parent_uid = base_folder_uid + last_idx = len(segments) - 1 + created_uid = None + + for idx, segment in enumerate(segments): + is_leaf = (idx == last_idx) + existing_uid = self._find_existing_child(params, segment, parent_uid) + if existing_uid: + if is_leaf: + logging.warning('nsf-mkdir: Folder "%s" already exists', segment) + return existing_uid + parent_uid = existing_uid + continue - existing_uid = self._find_existing_child(params, folder_name, base_folder_uid) - if existing_uid: - logging.warning('nsf-mkdir: Folder "%s" already exists', folder_name) - return existing_uid + seg_color = color if is_leaf else None + seg_inherit = inherit_permissions if is_leaf else True - with command_error_handler('nsf-mkdir'): - result = _nsf.create_folder_v3( - params=params, folder_name=folder_name, - parent_uid=base_folder_uid, - color=color, - inherit_permissions=inherit_permissions, - ) - check_result(result, 'nsf-mkdir') + with command_error_handler('nsf-mkdir'): + result = _nsf.create_folder_v3( + params=params, folder_name=segment, + parent_uid=parent_uid, + color=seg_color, + inherit_permissions=seg_inherit, + ) + check_result(result, 'nsf-mkdir') + + created_uid = result['folder_uid'] + self._cache_new_folder( + params, created_uid, segment, parent_uid, + folder_key=result.get('folder_key_unencrypted')) + + if not is_leaf: + logging.debug('nsf-mkdir: Created intermediate folder "%s"', segment) + parent_uid = created_uid params.sync_data = True - return result['folder_uid'] + return created_uid @staticmethod def _parse_path(folder_path): - # Collapse escaped slashes (//) to a sentinel so we can detect any - # stray path separator and refuse it — nsf-mkdir creates a single - # folder, not a nested hierarchy. - collapsed = folder_path.replace('//', '\x00') - if '/' in collapsed: - raise CommandError('nsf-mkdir', - 'Character "/" is reserved. Use "//" inside folder name') - name = collapsed.replace('\x00', '/').strip() - if not name: + """Split *folder_path* into a list of segment names. + """ + sentinel = '\x00' + collapsed = folder_path.replace('//', sentinel) + raw_segments = collapsed.split('/') + segments = [] + for raw in raw_segments: + name = raw.replace(sentinel, '/').strip() + if name: + segments.append(name) + if not segments: raise CommandError('nsf-mkdir', 'Invalid folder name') - return name + return segments + + @staticmethod + def _cache_new_folder(params, folder_uid, name, parent_uid, folder_key=None): + """Insert a just-created folder into the local NSF cache so that + subsequent segments in the same path can discover it as a parent + without requiring a full sync round-trip. + """ + nsf = getattr(params, 'nested_share_folders', None) + if nsf is None: + return + entry = { + 'name': name, + 'parent_uid': parent_uid or '', + } + if folder_key: + entry['folder_key_unencrypted'] = folder_key + nsf[folder_uid] = entry @staticmethod def _find_existing_child(params, folder_name, parent_uid): + """Find an existing NSF folder named *folder_name* whose parent matches + *parent_uid*. ``parent_uid=None`` means "root level". + """ nsf_folders = getattr(params, 'nested_share_folders', {}) name_lower = folder_name.lower() - expected_parent = parent_uid or '' + looking_for_root = not parent_uid for fuid, fobj in nsf_folders.items(): if fobj.get('name', '').lower() != name_lower: continue - existing_parent = normalize_parent_uid(fobj.get('parent_uid', '')) - if existing_parent == 'root': - existing_parent = '' - if existing_parent == expected_parent: - return fuid + raw_parent = fobj.get('parent_uid') or '' + normalized = normalize_parent_uid(raw_parent) + is_root_child = ( + normalized in ('', 'root') + or (raw_parent and raw_parent not in nsf_folders) + ) + if looking_for_root: + if is_root_child: + return fuid + else: + if raw_parent == parent_uid: + return fuid return None @@ -459,7 +509,7 @@ def _preview_and_confirm(self, params, removals, operation, force, dry_run, quie self._impact_summary(pr['folder_uid'], name, operation, pr.get('impact'), quiet) ) - if summary_lines: + if summary_lines and (dry_run or not force): for line in summary_lines: print(line) diff --git a/keepercommander/commands/nested_share_folder/parsers.py b/keepercommander/commands/nested_share_folder/parsers.py index 1539371ea..0d0e821b0 100644 --- a/keepercommander/commands/nested_share_folder/parsers.py +++ b/keepercommander/commands/nested_share_folder/parsers.py @@ -36,7 +36,9 @@ def _make_parser(prog, description): 'nsf-mkdir', 'Create a new Nested Share Folder using v3 API') nested_share_folder_mkdir_parser.add_argument( 'folder', type=str, - help='Folder name to create (use "//" to embed a literal "/" in the name)') + help='Folder name or path (e.g. "Parent/Child/Grand") to create. ' + 'Intermediate folders are created automatically. ' + 'Use "//" to embed a literal "/" in a segment name.') nested_share_folder_mkdir_parser.add_argument( '--color', type=str, choices=['none', 'red', 'orange', 'yellow', 'green', 'blue', 'gray'], diff --git a/keepercommander/commands/nested_share_folder/record_commands.py b/keepercommander/commands/nested_share_folder/record_commands.py index 164acab01..bbcdfe005 100644 --- a/keepercommander/commands/nested_share_folder/record_commands.py +++ b/keepercommander/commands/nested_share_folder/record_commands.py @@ -179,6 +179,26 @@ def __init__(self): def get_parser(self): return nested_share_record_update_parser + def _resolve_field_value(self, parsed): + raw = parsed.value + if not raw: + return raw + + action_params = [] + if self.is_json_value(raw, action_params): + return action_params[0] if action_params else None + action_params.clear() + if self.is_generate_value(raw, action_params): + if parsed.type == 'password': + return self.generate_password(action_params) + if parsed.type in ('oneTimeCode', 'otp'): + return self.generate_totp_url() + return raw + action_params.clear() + if self.is_base64_value(raw, action_params): + return action_params[0] if action_params else None + return raw + def execute(self, params, **kwargs): if kwargs.get('syntax_help'): print(record_fields_description) @@ -198,15 +218,24 @@ def execute(self, params, **kwargs): for spec in [f.strip() for f in kwargs.get('fields', []) if f.strip()]: try: parsed = RecordEditMixin.parse_field(spec) + value = self._resolve_field_value(parsed) + if value is None: + continue if parsed.type in fields: existing = fields[parsed.type] fields[parsed.type] = ([existing] if not isinstance(existing, list) - else existing) + [parsed.value] + else existing) + [value] else: - fields[parsed.type] = parsed.value + fields[parsed.type] = value except ValueError as e: raise CommandError('nsf-record-update', f'Invalid field specification: {e}') + if self.warnings: + for w in self.warnings: + logging.warning(w) + if not kwargs.get('force'): + return + with command_error_handler('nsf-record-update'): for identifier in record_uids: record_uid = _nsf.resolve_nested_share_record_uid(params, identifier) @@ -524,28 +553,35 @@ def _build_removals(self, params, record_args, folder_uid, operation): def _preview_and_confirm(self, params, removals, operation, force, dry_run): result = _nsf.remove_record_v3(params, removals, dry_run=True) any_error = False - summary_lines = [] + error_lines = [] + info_lines = [] for pr in result['preview_results']: title = self._record_title(params, pr['record_uid']) if pr.get('error'): any_error = True err = pr['error'] - summary_lines.append( + error_lines.append( f" {title} [{pr['record_uid']}]: " f"{err.get('code', '')} — {err.get('message', '')}" ) else: - summary_lines.extend( + info_lines.extend( self._impact_summary(pr['record_uid'], title, operation, pr.get('impact')) ) - for line in summary_lines: - print(line) - + # Errors must always surface, even in --force mode, so the caller (or + # Service Mode HTTP layer) can see why the operation aborted. if any_error: + for line in error_lines: + print(line) print('\nOne or more records could not be previewed. Aborting.') return + + if dry_run or not force: + for line in info_lines: + print(line) + if dry_run: print('\n[Dry-run] No records were deleted.') return diff --git a/keepercommander/nested_share_folder/folder_api.py b/keepercommander/nested_share_folder/folder_api.py index 94f90179e..e6d8f2307 100644 --- a/keepercommander/nested_share_folder/folder_api.py +++ b/keepercommander/nested_share_folder/folder_api.py @@ -93,7 +93,7 @@ def _prepare_folder_for_creation(params, folder_uid, folder_name, parent_uid, else SetBooleanValue.BOOLEAN_FALSE), color=color) fd.folderKey = encrypted_fk - return fd + return fd, folder_key # ══════════════════════════════════════════════════════════════════════════ @@ -173,13 +173,14 @@ def resolve_folder_identifier(params, folder_identifier): def create_folder_v3(params, folder_name, parent_uid=None, color=None, inherit_permissions=True): uid = utils.generate_uid() - fd = _prepare_folder_for_creation(params, uid, folder_name, parent_uid, - color, inherit_permissions) + fd, folder_key = _prepare_folder_for_creation( + params, uid, folder_name, parent_uid, color, inherit_permissions) response = folder_add_v3(params, [fd]) if response.folderAddResults: r = response.folderAddResults[0] return { 'folder_uid': uid, + 'folder_key_unencrypted': folder_key, 'status': folder_pb2.FolderModifyStatus.Name(r.status), 'message': r.message, 'success': r.status == folder_pb2.SUCCESS, @@ -190,20 +191,22 @@ def create_folder_v3(params, folder_name, parent_uid=None, color=None, def create_folders_batch_v3(params, folder_specs): if len(folder_specs) > 100: raise ValueError("Maximum 100 folders at a time") - fd_list, uid_map = [], {} + fd_list, uid_map, key_map = [], {}, {} for idx, spec in enumerate(folder_specs): uid = utils.generate_uid() uid_map[idx] = uid name = spec.get('name') if not name: raise ValueError(f"Spec at index {idx} missing 'name'") - fd = _prepare_folder_for_creation( + fd, folder_key = _prepare_folder_for_creation( params, uid, name, spec.get('parent_uid'), spec.get('color'), spec.get('inherit_permissions', True)) fd_list.append(fd) + key_map[idx] = folder_key response = folder_add_v3(params, fd_list) return [{ 'folder_uid': uid_map.get(i, utils.base64_url_encode(r.folderUid)), + 'folder_key_unencrypted': key_map.get(i), 'name': folder_specs[i].get('name'), 'status': folder_pb2.FolderModifyStatus.Name(r.status), 'message': r.message, diff --git a/keepercommander/nested_share_folder/record_api.py b/keepercommander/nested_share_folder/record_api.py index 7eb432567..db886a78b 100644 --- a/keepercommander/nested_share_folder/record_api.py +++ b/keepercommander/nested_share_folder/record_api.py @@ -151,7 +151,9 @@ def update_record_v3(params, record_uid, data=None, title=None, if 'data_unencrypted' in rec: raw = rec['data_unencrypted'] if isinstance(raw, bytes): - existing = json.loads(raw.decode()) + existing = json.loads(raw.decode('utf-8')) + elif isinstance(raw, str): + existing = json.loads(raw) data = existing.copy() if existing else {'fields': []} if title is not None: data['title'] = title From 7918c6ba89b268f42d7d601dfcf1bf332d1e7833 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Fri, 29 May 2026 18:04:41 +0530 Subject: [PATCH 19/20] Fix: Stop silently inheriting SSL_CERT_FILE from process env (#2094) * fix: honor bundled certifi store on Windows * fix: rename os-keychain plugin directory to os_keychain for valid Python package name The hyphen in os-keychain caused find_packages() to skip the directory, so it was not included in the PyPI wheel. PyInstaller also failed to bundle it correctly. Renaming to os_keychain (underscore) fixes both. loader.py normalises the URI scheme (os-keychain -> os_keychain) before resolving the plugin module, so config.json entries are unchanged. Co-authored-by: Cursor --------- Co-authored-by: sshrushanth-ks Co-authored-by: Cursor --- keepercommander/__main__.py | 73 +------------- keepercommander/config_storage/loader.py | 2 +- .../{os-keychain => os_keychain}/__init__.py | 0 .../os_keychain_storage.py | 0 keepercommander/utils.py | 94 ++++++++----------- 5 files changed, 44 insertions(+), 125 deletions(-) rename keepercommander/config_storage/{os-keychain => os_keychain}/__init__.py (100%) rename keepercommander/config_storage/{os-keychain => os_keychain}/os_keychain_storage.py (100%) diff --git a/keepercommander/__main__.py b/keepercommander/__main__.py index eaf7cba03..cb69ef2c4 100644 --- a/keepercommander/__main__.py +++ b/keepercommander/__main__.py @@ -12,15 +12,12 @@ import argparse -import certifi import json import logging import os import re import shlex import sys -import ssl -import platform from pathlib import Path from typing import Optional @@ -178,69 +175,6 @@ def handle_exceptions(exc_type, exc_value, exc_traceback): sys.exit(-1) -def get_ssl_cert_file(): - """Get SSL certificate file path, preferring system CA store for corporate environments like Zscaler""" - - # Allow user to override via environment variable - user_cert_file = os.getenv('KEEPER_SSL_CERT_FILE') - if user_cert_file: - if user_cert_file.lower() == 'system': - # User explicitly wants system certs - pass # Continue with system detection below - elif user_cert_file.lower() == 'certifi': - # User explicitly wants certifi - return certifi.where() - elif user_cert_file.lower() == 'none' or user_cert_file.lower() == 'false': - # User wants to disable SSL verification (not recommended) - return None - elif os.path.exists(user_cert_file): - # User provided specific cert file - return user_cert_file - else: - logging.warning(f"SSL cert file specified in KEEPER_SSL_CERT_FILE not found: {user_cert_file}") - - # Try to use system CA store first for corporate environments - try: - # On macOS, try Homebrew certificates first (better for corporate environments like Zscaler) - if platform.system() == 'Darwin': - system_ca_paths = [ - '/opt/homebrew/etc/ca-certificates/cert.pem', # Homebrew CA bundle (best for Zscaler) - '/usr/local/etc/ssl/cert.pem', # Homebrew SSL (older location) - '/etc/ssl/cert.pem', # macOS system CA bundle - ] - for ca_path in system_ca_paths: - if os.path.exists(ca_path): - return ca_path - - # On Linux/Unix systems - elif platform.system() == 'Linux': - system_ca_paths = [ - '/etc/ssl/certs/ca-certificates.crt', # Debian/Ubuntu - '/etc/pki/tls/certs/ca-bundle.crt', # RHEL/CentOS - '/etc/ssl/ca-bundle.pem', # OpenSUSE - '/etc/ssl/cert.pem', # Generic - ] - for ca_path in system_ca_paths: - if os.path.exists(ca_path): - return ca_path - - # Try to get default SSL context locations - try: - default_locations = ssl.get_default_verify_paths() - if default_locations.cafile and os.path.exists(default_locations.cafile): - return default_locations.cafile - if default_locations.capath and os.path.exists(default_locations.capath): - return default_locations.capath - except: - pass - - except Exception: - pass - - # Fall back to certifi if system CA not available - return certifi.where() - - def main(from_package=False): if sys.platform == 'win32': try: @@ -253,15 +187,12 @@ def main(from_package=False): if logger: logger.name = 'keepercommander' - # Use system CA certificates when available (supports Zscaler), fallback to certifi - ssl_cert_file = get_ssl_cert_file() + ssl_cert_file = utils.get_ssl_cert_file() if ssl_cert_file: os.environ['SSL_CERT_FILE'] = ssl_cert_file else: - # User explicitly disabled SSL verification logging.warning("Warning: SSL certificate verification has been disabled. This is not recommended for production use.") - if 'SSL_CERT_FILE' in os.environ: - del os.environ['SSL_CERT_FILE'] + os.environ.pop('SSL_CERT_FILE', None) errno = 0 diff --git a/keepercommander/config_storage/loader.py b/keepercommander/config_storage/loader.py index 978a37d61..219e5951e 100644 --- a/keepercommander/config_storage/loader.py +++ b/keepercommander/config_storage/loader.py @@ -110,7 +110,7 @@ def _get_plugin(url): # type: (str) -> SecureStorageBase if not par.scheme: raise SecureStorageException(f'Configuration file error: "{CONFIG_STORAGE_URL}" is not URL') - plugin_name = par.scheme + plugin_name = par.scheme.replace('-', '_') if not any(x for x in pkgutil.iter_modules(config_storage.__path__) if x.name == plugin_name): raise SecureStorageException(f'Protected storage "{plugin_name}" is not supported') diff --git a/keepercommander/config_storage/os-keychain/__init__.py b/keepercommander/config_storage/os_keychain/__init__.py similarity index 100% rename from keepercommander/config_storage/os-keychain/__init__.py rename to keepercommander/config_storage/os_keychain/__init__.py diff --git a/keepercommander/config_storage/os-keychain/os_keychain_storage.py b/keepercommander/config_storage/os_keychain/os_keychain_storage.py similarity index 100% rename from keepercommander/config_storage/os-keychain/os_keychain_storage.py rename to keepercommander/config_storage/os_keychain/os_keychain_storage.py diff --git a/keepercommander/utils.py b/keepercommander/utils.py index fd0ebfdde..d9c8c7cef 100644 --- a/keepercommander/utils.py +++ b/keepercommander/utils.py @@ -531,66 +531,54 @@ def value_to_boolean(value): return None def get_ssl_cert_file(): - """Get SSL certificate file path, preferring system CA store for corporate environments like Zscaler""" - import ssl - import platform + """Resolve the SSL CA bundle path. + + KEEPER_SSL_CERT_FILE accepts: 'certifi', 'system', 'none'/'false', or a PEM path. + Defaults to the bundled certifi store; does not consult SSL_CERT_FILE from the + environment to avoid silent trust-store hijacking by unrelated tools. + """ import certifi - import os - - # Allow user to override via environment variable + user_cert_file = os.getenv('KEEPER_SSL_CERT_FILE') if user_cert_file: - if user_cert_file.lower() == 'system': - pass # Continue with system detection below - elif user_cert_file.lower() == 'certifi': + choice = user_cert_file.lower() + if choice == 'certifi': return certifi.where() - elif user_cert_file.lower() == 'none' or user_cert_file.lower() == 'false': - return False # Disable SSL verification - elif os.path.exists(user_cert_file): - return user_cert_file - else: - # Don't use logging here as it can interfere with main logging config - print(f"Warning: SSL cert file specified in KEEPER_SSL_CERT_FILE not found: {user_cert_file}", file=sys.stderr) - - # Try to use system CA store first for corporate environments + if choice in ('none', 'false'): + return False + if choice != 'system': + if os.path.exists(user_cert_file): + return user_cert_file + print( + f"Warning: KEEPER_SSL_CERT_FILE points to a non-existent file: " + f"{user_cert_file}; falling back to the bundled certifi store.", + file=sys.stderr, + ) + return certifi.where() + try: - # On macOS, try Homebrew certificates first (better for corporate environments like Zscaler) - if platform.system() == 'Darwin': - system_ca_paths = [ - '/opt/homebrew/etc/ca-certificates/cert.pem', # Homebrew CA bundle (best for Zscaler) - '/usr/local/etc/ssl/cert.pem', # Homebrew SSL (older location) - '/etc/ssl/cert.pem', # macOS system CA bundle - ] - for ca_path in system_ca_paths: - if os.path.exists(ca_path): - return ca_path - - # On Linux/Unix systems - elif platform.system() == 'Linux': - system_ca_paths = [ - '/etc/ssl/certs/ca-certificates.crt', # Debian/Ubuntu - '/etc/pki/tls/certs/ca-bundle.crt', # RHEL/CentOS - '/etc/ssl/ca-bundle.pem', # OpenSUSE - '/etc/ssl/cert.pem', # Generic - ] - for ca_path in system_ca_paths: - if os.path.exists(ca_path): - return ca_path - - # Try to get default SSL context locations - try: - default_locations = ssl.get_default_verify_paths() - if default_locations.cafile and os.path.exists(default_locations.cafile): - return default_locations.cafile - if default_locations.capath and os.path.exists(default_locations.capath): - return default_locations.capath - except: - pass - + system = platform.system() + if system == 'Darwin': + candidates = ( + '/opt/homebrew/etc/ca-certificates/cert.pem', + '/usr/local/etc/ssl/cert.pem', + '/etc/ssl/cert.pem', + ) + elif system == 'Linux': + candidates = ( + '/etc/ssl/certs/ca-certificates.crt', + '/etc/pki/tls/certs/ca-bundle.crt', + '/etc/ssl/ca-bundle.pem', + '/etc/ssl/cert.pem', + ) + else: + candidates = () + for ca_path in candidates: + if os.path.exists(ca_path): + return ca_path except Exception: pass - - # Fall back to certifi if system CA not available + return certifi.where() From 917cfd4a31709ea11d786ff2be381a0d5fd06817 Mon Sep 17 00:00:00 2001 From: idimov-keeper <78815270+idimov-keeper@users.noreply.github.com> Date: Fri, 29 May 2026 20:25:06 -0500 Subject: [PATCH 20/20] Remove force_close escalation on workflow lease expiry; soft-close only (#2100) --- keepercommander/commands/pam_launch/launch.py | 40 ++++------ .../commands/pam_launch/rust_log_filter.py | 23 ++---- .../tunnel/port_forward/tunnel_helpers.py | 74 ------------------- .../commands/tunnel_and_connections.py | 30 ++++---- requirements.txt | 2 +- setup.cfg | 2 +- 6 files changed, 34 insertions(+), 137 deletions(-) diff --git a/keepercommander/commands/pam_launch/launch.py b/keepercommander/commands/pam_launch/launch.py index 1435ab112..503478a75 100644 --- a/keepercommander/commands/pam_launch/launch.py +++ b/keepercommander/commands/pam_launch/launch.py @@ -62,7 +62,6 @@ unregister_tunnel_session, unregister_conversation_key, get_keeper_tokens, - escalate_close, CloseConnectionReasons, ) from ..tunnel.port_forward.TunnelGraph import TunnelDAG @@ -1570,17 +1569,13 @@ def signal_handler_fn(signum, frame): original_handler = signal.signal(signal.SIGINT, signal_handler_fn) - # Workflow lease expiry: schedule a hard kill at expiresOn matching + # Workflow lease expiry: schedule teardown at expiresOn matching # the web vault (immediate teardown, no grace period, no reconnect). # The "Access expired" line is printed AFTER terminal reset in finally # so the message survives reset_local_terminal_after_pam_session(). - # On expiry we soft-close the tube and escalate to force_close_tube - # after FORCE_CLOSE_DELAY_SECONDS so any in-flight forwarded streams - # (SSH bytes etc.) are severed instead of lingering until the user - # disconnects manually. Escalation is gated on local hasattr + - # remote SDP version (FORCE_CLOSE_MIN_VERSION). + # On expiry we close the tube; the connection-closed cleanup path then + # stops the websocket and unregisters the tunnel session. lease_timer = None - force_close_timer_holder = {} # mutable holder so cleanup can cancel if workflow_expires_on_ms and workflow_expires_on_ms > 0: seconds_until_expiry = (workflow_expires_on_ms / 1000.0) - time.time() _lease_tube_id = tunnel_result['tunnel'].get('tube_id') @@ -1591,27 +1586,20 @@ def _on_lease_expired(): lease_expired = True shutdown_requested = True if _lease_tube_id and _lease_tube_registry is not None: - # Fetch remote version lazily: the SDP answer arrives - # asynchronously; capturing eagerly at schedule time - # would race for short leases scheduled before SDP. - remote_ver = tunnel_result['tunnel'].get('remote_webrtc_version') - if not remote_ver: - sess = get_tunnel_session(_lease_tube_id) - remote_ver = ( - getattr(sess, 'remote_webrtc_version', None) - if sess else None + try: + _lease_tube_registry.close_tube( + _lease_tube_id, + reason=CloseConnectionReasons.AdminClosed, + ) + except Exception as e: + logging.debug( + f"[lease-expiry launch tube={_lease_tube_id[:8]}] " + f"close_tube failed: {e}" ) - force_close_timer_holder['t'] = escalate_close( - _lease_tube_registry, - _lease_tube_id, - remote_webrtc_version=remote_ver, - reason=CloseConnectionReasons.AdminClosed, - log_prefix=f"[lease-expiry launch tube={_lease_tube_id[:8]}] ", - ) if seconds_until_expiry <= 0: - # Already expired at session start: run the close-and-escalate - # path immediately so cleanup goes through the same flow as a + # Already expired at session start: run the close path + # immediately so cleanup goes through the same flow as a # mid-session expiry. _on_lease_expired() else: diff --git a/keepercommander/commands/pam_launch/rust_log_filter.py b/keepercommander/commands/pam_launch/rust_log_filter.py index b1ee55e96..54f75d8fa 100644 --- a/keepercommander/commands/pam_launch/rust_log_filter.py +++ b/keepercommander/commands/pam_launch/rust_log_filter.py @@ -191,23 +191,9 @@ def enter_pam_launch_terminal_rust_logging(): # the channel is torn down, or TURN ``fail to refresh permissions`` warnings # from the relay-conn task as it observes the deallocated allocation. # -# The window must outlive both: -# 1. The soft→hard close escalation in ``escalate_close`` -# (``FORCE_CLOSE_DELAY_SECONDS`` = 3 s) -# 2. A brief TURN refresh-task latency after the PeerConnection drop cascade -# -# Imported lazily below to avoid a top-level cycle (this module is imported -# during pam_launch init, before the tunnel helpers are loaded for some -# callers). -def _force_close_delay_seconds(): - try: - from ..tunnel.port_forward.tunnel_helpers import FORCE_CLOSE_DELAY_SECONDS - return FORCE_CLOSE_DELAY_SECONDS - except Exception: - return 3.0 - - -_DEFAULT_RUST_LOG_FILTER_GRACE_SEC = _force_close_delay_seconds() + 1.5 +# The window must outlive both the tube close + teardown cascade (~3 s) and a +# brief TURN refresh-task latency after the PeerConnection drop cascade. +_DEFAULT_RUST_LOG_FILTER_GRACE_SEC = 4 # Refcount of active pam-launch sessions that have rust-log filtering installed. # Incremented in enter_*, decremented at the END of the grace timer in @@ -282,7 +268,8 @@ def _do_exit_rust_logging(token): def exit_pam_launch_terminal_rust_logging(token, grace_sec=_DEFAULT_RUST_LOG_FILTER_GRACE_SEC): """Restore Rust/webrtc logger state after pam launch terminal session. - The filter is removed after ``grace_sec`` seconds (default 2.5s) so that + The filter is removed after ``grace_sec`` seconds (default + ``_DEFAULT_RUST_LOG_FILTER_GRACE_SEC``) so that late records from the Rust runtime (e.g. ``webrtc-sctp`` stream teardown messages that arrive just after session exit) are still caught by the filter and do not leak to the console in front of the subsequent diff --git a/keepercommander/commands/tunnel/port_forward/tunnel_helpers.py b/keepercommander/commands/tunnel/port_forward/tunnel_helpers.py index 311b004e3..1349f71d2 100644 --- a/keepercommander/commands/tunnel/port_forward/tunnel_helpers.py +++ b/keepercommander/commands/tunnel/port_forward/tunnel_helpers.py @@ -129,20 +129,6 @@ def parse(v): return False -# Minimum keeper-pam-webrtc-rs version that exposes force_close_tube. Both the -# local Rust crate AND the remote peer must satisfy this gate before Commander -# escalates a soft close to a force close. Local check uses hasattr (the binding -# attribute is missing on older crates), remote check uses the SDP-advertised -# version string. -FORCE_CLOSE_MIN_VERSION = "2.1.18" - -# Default delay between the soft close and the force-close escalation. Matches -# the consumer-side budget agreed with the gateway (gateway-side -# KEEPER_GATEWAY_FORCE_CLOSE_TIMEOUT is 6s; we run faster on the consumer because -# at lease expiry there is no reason to wait long). -FORCE_CLOSE_DELAY_SECONDS = 3.0 - - def print_above_keeper_prompt(msg): """Print ``msg`` so the keeper-shell prompt redraws itself underneath it. @@ -181,66 +167,6 @@ def print_above_keeper_prompt(msg): pass -def escalate_close( - tube_registry, - tube_id, - *, - remote_webrtc_version=None, - reason=None, - hard_after_seconds=FORCE_CLOSE_DELAY_SECONDS, - log_prefix="", -): - """ - Soft-close a tube now, then escalate to force_close_tube after - `hard_after_seconds` if both endpoints support it. - - The soft close stops new channel creation and emits CloseConnection control - frames; the force close (when available) drops the local TCP listener, - severs in-flight forwarded TCP streams (SSH, MySQL, etc.) and tears down - the peer connection on a short bounded budget. - - Returns the scheduled `threading.Timer` (or None if escalation is not - available) so callers can cancel it on a clean exit. - """ - if reason is None: - reason = CloseConnectionReasons.AdminClosed - - try: - tube_registry.close_tube(tube_id, reason=reason) - except Exception as e: - logging.debug(f"{log_prefix}soft close_tube failed: {e}") - - has_local = hasattr(tube_registry, "force_close_tube") - has_remote = _version_at_least(remote_webrtc_version, FORCE_CLOSE_MIN_VERSION) - if not has_local: - logging.debug( - f"{log_prefix}force_close_tube unavailable in local keeper_pam_webrtc_rs - " - f"soft close only" - ) - return None - if not has_remote: - logging.debug( - f"{log_prefix}remote keeper-pam-webrtc {remote_webrtc_version!r} < " - f"{FORCE_CLOSE_MIN_VERSION} - soft close only" - ) - return None - - def _do_force_close(): - try: - logging.debug( - f"{log_prefix}escalating to force_close_tube({tube_id}) after " - f"{hard_after_seconds}s" - ) - tube_registry.force_close_tube(tube_id, reason=reason) - except Exception as e: - logging.debug(f"{log_prefix}force_close_tube failed: {e}") - - timer = threading.Timer(hard_after_seconds, _do_force_close) - timer.daemon = True - timer.start() - return timer - - # Constants NONCE_LENGTH = 12 MAIN_NONCE_LENGTH = 16 diff --git a/keepercommander/commands/tunnel_and_connections.py b/keepercommander/commands/tunnel_and_connections.py index 050d7b962..c5f165c35 100644 --- a/keepercommander/commands/tunnel_and_connections.py +++ b/keepercommander/commands/tunnel_and_connections.py @@ -33,7 +33,7 @@ get_keeper_tokens, \ get_or_create_tube_registry, get_gateway_uid_from_record, resolve_record, resolve_pam_config, resolve_folder, \ remove_field, start_rust_tunnel, get_tunnel_session, unregister_tunnel_session, CloseConnectionReasons, \ - wait_for_tunnel_connection, create_rust_webrtc_settings, escalate_close, \ + wait_for_tunnel_connection, create_rust_webrtc_settings, \ print_above_keeper_prompt from .pam.router_helper import get_dag_leafs from .tunnel_registry import ( @@ -1015,13 +1015,9 @@ def execute(self, params, **kwargs): self._print_keeperdb_proxy_banner(host, port, db_type_for_banner) # Workflow lease expiry handling. # - # At expiresOn we soft-close the tube (stops new channels, sends - # CloseConnection control frames) and, after a short delay, escalate - # to force_close_tube which drops the local TCP listener and severs - # any active forwarded streams (SSH, MySQL, etc.). The escalation - # only fires when both the local Rust crate and the remote peer - # advertise FORCE_CLOSE_MIN_VERSION; older peers get the soft close - # only and the in-flight session lingers until natural disconnect. + # At expiresOn we close the tube (stops new channels, sends + # CloseConnection control frames); the connection-closed cleanup + # path then stops the websocket and unregisters the tunnel session. if workflow_expires_on_ms and workflow_expires_on_ms > 0: seconds_until_expiry = (workflow_expires_on_ms / 1000.0) - time.time() tube_id = result.get('tube_id') @@ -1046,15 +1042,15 @@ def _close_on_lease_expiry(_tube_id=tube_id, _record_uid=record_uid): f"forwarded connections will be terminated." f"{bcolors.ENDC}" ) - sess = get_tunnel_session(_tube_id) - remote_ver = getattr(sess, 'remote_webrtc_version', None) if sess else None - escalate_close( - tube_registry, - _tube_id, - remote_webrtc_version=remote_ver, - reason=CloseConnectionReasons.AdminClosed, - log_prefix=f"[lease-expiry tunnel record={_record_uid}] ", - ) + try: + tube_registry.close_tube( + _tube_id, reason=CloseConnectionReasons.AdminClosed, + ) + except Exception as e: + logging.debug( + f"[lease-expiry tunnel record={_record_uid}] " + f"close_tube failed: {e}" + ) # Wake any --foreground / --run blocking wait so the # process self-terminates. Default interactive mode # does not register an event here. diff --git a/requirements.txt b/requirements.txt index 5264fa503..7af34666c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -13,7 +13,7 @@ requests>=2.31.0 cryptography>=46.0.6 protobuf>=5.29.6 keeper-secrets-manager-core>=16.6.0 -keeper_pam_webrtc_rs>=2.1.17 +keeper_pam_webrtc_rs>=2.1.18 pydantic>=2.6.4 flask pyngrok>=7.5.0 diff --git a/setup.cfg b/setup.cfg index dd9a215b7..6c52380c5 100644 --- a/setup.cfg +++ b/setup.cfg @@ -53,7 +53,7 @@ install_requires = requests>=2.31.0 tabulate websockets - keeper_pam_webrtc_rs>=2.1.17 + keeper_pam_webrtc_rs>=2.1.18 pydantic>=2.6.4 fpdf2>=2.8.3 tzlocal>=5.0