From ab6267f3777511dadedc28222de0fbe07ed744e9 Mon Sep 17 00:00:00 2001 From: lthievenaz-keeper Date: Sat, 13 Jun 2026 04:36:06 +0100 Subject: [PATCH 01/11] Support empty user/ports and other improvements (#2122) * Add support for port mapping If connection has empty port, it will default to the ports defined in this file (can be customized for custom default ports) Also added new parameter allow-file-uploads for RBI * Support empty user/ports and other improvements If a KCM connection has no user, the export will log the record for future process. If a KCM connection has no port, it will default to the port mapping defined in KCM_mappings.json Fixed a duplication issue of SFTP parameters caused by a reference error. Set the autodocker docker-file location as default Reworked how logged records are displayed on output file. --- examples/pam-kcm-import/KCM_mappings.json | 11 ++ examples/pam-kcm-import/kcm_export.py | 132 +++++++++++++--------- 2 files changed, 88 insertions(+), 55 deletions(-) diff --git a/examples/pam-kcm-import/KCM_mappings.json b/examples/pam-kcm-import/KCM_mappings.json index d027b7271..2c60f90fe 100644 --- a/examples/pam-kcm-import/KCM_mappings.json +++ b/examples/pam-kcm-import/KCM_mappings.json @@ -1,4 +1,14 @@ { + "ports":{ + "rdp":3389, + "ssh":22, + "vnc":5900, + "telnet":23, + "kubernetes":8080, + "mysql":3306, + "postgresql":5432, + "sql-server":1433 + }, "users":{ "username":"login", "password":"password", @@ -21,6 +31,7 @@ "disable-copy": "pam_settings.connection.disable_copy", "disable-paste": "pam_settings.connection.disable_paste", "force-lossless": null, + "allow-file-uploads":null, "read-only": null, "backspace": null, "url": "url", diff --git a/examples/pam-kcm-import/kcm_export.py b/examples/pam-kcm-import/kcm_export.py index d589ddd71..2aaf45648 100644 --- a/examples/pam-kcm-import/kcm_export.py +++ b/examples/pam-kcm-import/kcm_export.py @@ -205,8 +205,9 @@ def collect_db_config(self): '(2) I have hardcoded them in the Python script' ]) if handle_prompt({'1':'file','2':'code'}) == 'file': - display('## Please upload your docker-compose file', 'cyan') - self.docker_compose = validate_file_upload('yaml') + display('## Please upload your docker-compose file\n(blank for /etc/kcm-setup/docker-compose.yml)', 'cyan') + docker_file = input('File path: ') or '/etc/kcm-setup/docker-compose.yml' + self.docker_compose = validate_file_upload('yaml',docker_file) port={'MYSQL':3306,'POSTGRES':5432} custom_port = None @@ -340,9 +341,9 @@ def generate_data(self): templ = validate_file_upload('json') templ_resources = templ.get('pam_data',{}).get('resources',[]) if len(templ_resources)>1: - self.template_rs = templ_resources[1] + self.template_rs = deepcopy(templ_resources[1]) if self.template_rs.get('users',[]): - self.template_usr = self.template_rs['users'][0] + self.template_usr = deepcopy(self.template_rs['users'][0]) print() self.group_paths = {} @@ -373,6 +374,55 @@ def resolve_path(group_id): self.users = {} self.shared_folders = [] + def handle_mapping(mapping, name, arg, value, dir): + if mapping == 'ignore': + debug(f'Mapping {arg} ignored', self.debug) + return dir + if mapping == 'log': + if name not in self.logged_records: + self.logged_records[name] = {'name':name} + if 'parameters' not in self.logged_records: + self.logged_records[name]['parameters'] = [] + self.logged_records[name]['parameters'].append(arg) + return dir + if mapping is None: + debug(f'Mapping {arg} recognized but not supported', self.debug) + return dir + if '=' in mapping: + mapping, value = mapping.split('=', 1) + keys = mapping.split('.') + set_nested(dir[id], keys, value) + return dir + + def handle_arg(id,name,arg,value,resource,user): + + if value.startswith('${KEEPER_') and id not in self.dynamic_tokens: + debug('Dynamic token detected',self.debug) + self.dynamic_tokens.append(id) + if name not in self.logged_records: + self.logged_records[name] = {'name':name, 'dynamic_token':True} + else: + self.logged_records[name]['dynamic_token'] = True + elif value and arg.startswith('totp-'): + if 'oneTimeCode' not in user: + user['oneTimeCode'] = { + "totp-algorithm": '', + "totp-digits": "", + "totp-period": "", + "totp-secret": "" + } + user['oneTimeCode'][arg] = value + elif value and arg == 'hostname': + resource['host'] = value + elif value and arg == 'port': + resource['pam_settings']['connection']['port'] = value + elif value and arg in self.mappings['users']: + self.users = handle_mapping(self.mappings['users'][arg],name,arg,value,self.users) + elif arg in self.mappings['resources']: + self.connections = handle_mapping(self.mappings['resources'][arg],name,arg,value,self.connections) + else: + display(f'Error: Unknown parameter detected: {arg}. Add it to KCM_mappings.json to resolve this error','bold red') + for connection in self.connection_data: id = connection['connection_id'] name = connection["name"] @@ -445,50 +495,7 @@ def resolve_path(group_id): resource['pam_settings']['connection']['launch_credentials'] = f'KCM User - {name}' self.connections[id] = resource - def handle_arg(id,name,arg,value,resource,user): - def handle_mapping(mapping, value, dir): - if mapping == 'ignore': - debug(f'Mapping {arg} ignored', self.debug) - return dir - if mapping == 'log': - record = self.logged_records.setdefault(name, {'name': name}) - record[arg] = value - return dir - if mapping is None: - debug(f'Mapping {arg} recognized but not supported', self.debug) - return dir - if '=' in mapping: - mapping, value = mapping.split('=', 1) - keys = mapping.split('.') - set_nested(dir[id], keys, value) - return dir - - if value.startswith('${KEEPER_') and id not in self.dynamic_tokens: - debug('Dynamic token detected',self.debug) - self.dynamic_tokens.append(id) - if name not in self.logged_records: - self.logged_records[name] = {'name':name, 'dynamic_token':True} - else: - self.logged_records[name]['dynamic_token'] = True - elif value and arg.startswith('totp-'): - if 'oneTimeCode' not in user: - user['oneTimeCode'] = { - "totp-algorithm": '', - "totp-digits": "", - "totp-period": "", - "totp-secret": "" - } - user['oneTimeCode'][arg] = value - elif value and arg == 'hostname': - resource['host'] = value - elif value and arg == 'port': - resource['pam_settings']['connection']['port'] = value - elif value and arg in self.mappings['users']: - self.users = handle_mapping(self.mappings['users'][arg],value,self.users) - elif arg in self.mappings['resources']: - self.connections = handle_mapping(self.mappings['resources'][arg],value,self.connections) - else: - display(f'Error: Unknown parameter detected: {arg}. Add it to KCM_mappings.json to resolve this error','bold red') + # Handle args if connection['parameter_name']: @@ -497,8 +504,7 @@ def handle_mapping(mapping, value, dir): if connection['attribute_name']: handle_arg(id,connection['name'],connection['attribute_name'],connection['attribute_value'],self.connections[id],self.users[id]) - - self.user_records = list(user for user in self.users.values()) + self.user_records = list(user for user in self.users.values() if user.get('login',None)) self.resource_records = list(conn for conn in self.connections.values()) # Sanitize totp @@ -511,10 +517,25 @@ def handle_mapping(mapping, value, dir): stripped_secret = ''.join([x for x in secret if x.isnumeric()]) user['otp'] = f'otpauth://totp/{TOTP_ACCOUNT}?secret={stripped_secret}&issuer=&algorithm={alg}&digits={dig}&period={period}' - # Handle SFTP records + usernames = [x['title'] for x in self.user_records] for resource in self.resource_records: + # Replace launch_credentials with autofill_credentials on http + if resource['pam_settings']['connection']['protocol']=='http': + resource['pam_settings']['connection']['autofill_credentials'] = resource['pam_settings']['connection']['launch_credentials'] + del resource['pam_settings']['connection']['launch_credentials'] + else: + # Remove non-existent users and add to logs + if resource['pam_settings']['connection']['launch_credentials'] not in usernames: + if resource['title'] not in self.logged_records: + self.logged_records[resource['title']] = {'name':resource['title']} + self.logged_records[resource['title']]['empty_credentials'] = True + resource['pam_settings']['connection']['launch_credentials'] = None + # Handle empty ports + if not resource['pam_settings']['connection'].get('port',None): + resource['pam_settings']['connection']['port'] = self.mappings['ports'].get(resource['pam_settings']['connection']['protocol'],'') + # Handle SFTP records if 'sftp' in resource['pam_settings']['connection']: - sftp_settings = resource['pam_settings']['connection']['sftp'] + sftp_settings = deepcopy(resource['pam_settings']['connection']['sftp']) # Create resource for SFTP sftp_resource = { 'folder_path':resource['folder_path']+'/SFTP Resources', @@ -556,12 +577,13 @@ def handle_mapping(mapping, value, dir): if self.dynamic_tokens: display(f'- {len(self.dynamic_tokens)} dynamic tokens detected, they will be added to the JSON file.','yellow') - if len(self.logged_records)-len(self.dynamic_tokens)>0: - display(f'- {len(self.logged_records)-len(self.dynamic_tokens)} records logged, they will be added to the JSON file.','yellow') + logged_records_count = len([x for x in self.logged_records if self.logged_records[x].get('parameters',None)]) + if logged_records_count: + display(f'- {logged_records_count} records logged, they will be added to the JSON file.','yellow') logged_records = [] if self.logged_records: - logged_records = (list(record for record in self.logged_records.values())) + logged_records = list(record for record in self.logged_records.values()) shared_folders = [] for folder in self.shared_folders: From 242f85fdeb513696b88afd8cbadb8865cfa82f9e Mon Sep 17 00:00:00 2001 From: Sergey Kolupaev Date: Fri, 12 Jun 2026 20:42:58 -0700 Subject: [PATCH 02/11] Thycotic secret ids arg (#2149) * Add secret-ids argument for thycotic import Add secret-ids arg to import command, to use for debugging Thycotic secret IDs * Pass secret-ids arg from import command to thycotic import Pass secret-ids arg from import command to thycotic import so it can be handled in the Thycotic import * Add handling for secret-ids arg in Thycotic import If user sets secret-ids, the import will: - Check if any of those IDs have come up in the lookup and would have been imported. - Import only the secret-ids set This is useful for debugging, because the lookup API may not return all Thycotic secrets - eg if there a security policy on them, but they may still be fetched. Usage: String (comma separated IDs) `import --format thycotic server_name --secret-ids "123, 124,125"` Python List (strings or integers) `secret_ids=[123, 124, 125]` * Correct secret_ids check Fix conditional logic so import continues if no secret ids are specified --------- Co-authored-by: lthievenaz-keeper --- keepercommander/importer/commands.py | 2 ++ keepercommander/importer/imp_exp.py | 3 ++- keepercommander/importer/thycotic/thycotic.py | 24 +++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/keepercommander/importer/commands.py b/keepercommander/importer/commands.py index 8563dd0c4..25a85f743 100644 --- a/keepercommander/importer/commands.py +++ b/keepercommander/importer/commands.py @@ -79,6 +79,8 @@ def register_command_info(aliases, command_info): help='temp directory used to cache encrypted attachment imports') import_parser.add_argument('--show-skipped', dest='show_skipped', action='store_true', help='Display skipped records') +import_parser.add_argument('--secret-ids', dest='secret_ids', action='store', + help='Comma separated list of secret IDs to fetch (Thycotic)') import_parser.add_argument( 'name', type=str, help='file name (json, csv, keepass, 1password), account name (lastpass), or URL (ManageEngine, Thycotic)' ) diff --git a/keepercommander/importer/imp_exp.py b/keepercommander/importer/imp_exp.py index 0ccec3cf0..99efb826b 100644 --- a/keepercommander/importer/imp_exp.py +++ b/keepercommander/importer/imp_exp.py @@ -715,6 +715,7 @@ def _import(params, file_format, filename, **kwargs): filter_folder = kwargs.get('filter_folder') dry_run = kwargs.get('dry_run') is True show_skipped = kwargs.get('show_skipped') is True + secret_ids = kwargs.get('secret_ids') import_into = kwargs.get('import_into') or '' if import_into: @@ -732,7 +733,7 @@ def _import(params, file_format, filename, **kwargs): filter_folder_lower = filter_folder.lower() if isinstance(filter_folder, str) else '' for x in importer.execute(filename, params=params, users_only=import_users, filter_folder=filter_folder, - old_domain=old_domain, new_domain=new_domain, tmpdir=tmpdir, dry_run=dry_run): + old_domain=old_domain, new_domain=new_domain, tmpdir=tmpdir, secret_ids=secret_ids, dry_run=dry_run): if isinstance(x, ImportRecord): if filter_folder and not importer.support_folder_filter(): if not x.folders: diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index 120480160..f7409609c 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -341,6 +341,30 @@ def do_import(self, filename, **kwargs): secrets_ids.extend([x['id'] for x in auth.thycotic_search(query)]) else: secrets_ids = [x['id'] for x in auth.thycotic_search(f'/v1/secrets/lookup')] + + # secret_ids arg + debug_ids = kwargs.get('secret_ids') + if debug_ids is not None: + # Convert CLI string input into list + if isinstance(debug_ids,str): + debug_ids = debug_ids.replace(' ','').split(',') + # Handle secret IDs list + if isinstance(debug_ids,list): + # Deduplicate + debug_ids = list(set(debug_ids)) + # Check whether secrets were found in the lookup + stringified_secrets_ids = [str(x) for x in secrets_ids] + stringified_debug_ids = [str(x) for x in debug_ids] + found_secrets = [x for x in stringified_debug_ids if x in stringified_secrets_ids] + logging.info(f'From the specified {len(debug_ids)} secret IDs, {len(found_secrets)} were found in the secret server lookup.') + logging.info(', '.join(found_secrets)) + + # Replace import list with specified IDs + secrets_ids = debug_ids + else: + # Quit if secret IDs != list + logging.warning('Invalid input for secret IDs, exiting.') + return self._send_keep_alive_if_needed(params) print(f'Loading {len(secrets_ids)} Records ', flush=True, end='') From c5307dbdabfc5e3f8555e45b833792aa12086771 Mon Sep 17 00:00:00 2001 From: sshrushanth-ks Date: Mon, 15 Jun 2026 15:49:40 +0530 Subject: [PATCH 03/11] Enforce GENERATED_PASSWORD_COMPLEXITY and RESTRICT_RECORD_TYPES on nsf-record-add/update (#2132) --- .../nested_share_folder/record_commands.py | 87 +++++++++- keepercommander/commands/record_edit.py | 2 +- keepercommander/enforcement.py | 38 +++-- unit-tests/test_nested_share_folder.py | 155 ++++++++++++++++++ 4 files changed, 265 insertions(+), 17 deletions(-) diff --git a/keepercommander/commands/nested_share_folder/record_commands.py b/keepercommander/commands/nested_share_folder/record_commands.py index bbcdfe005..31663ac3b 100644 --- a/keepercommander/commands/nested_share_folder/record_commands.py +++ b/keepercommander/commands/nested_share_folder/record_commands.py @@ -16,11 +16,13 @@ (create, update, link/unlink, shortcut management, delete). """ +import json import logging -from typing import List +from typing import Any, Dict, List, Optional from ..base import Command, GroupCommand from ..record_edit import RecordEditMixin, record_fields_description, ParsedFieldValue +from ...enforcement import PasswordComplexityEnforcer, RecordTypeEnforcer from ...error import CommandError from ... import nested_share_folder as _nsf, vault from .helpers import ( @@ -48,6 +50,7 @@ class NestedShareRecordAddCommand(Command, RecordEditMixin): def __init__(self): super().__init__() + RecordEditMixin.__init__(self) def get_parser(self): return nested_share_record_add_parser @@ -64,12 +67,17 @@ def execute(self, params, **kwargs): if not record_type: raise CommandError('nsf-record-add', 'Record type parameter is required.') + RecordTypeEnforcer.enforce(params, record_type, 'nsf-record-add') + + self.warnings.clear() + self._password_policy = PasswordComplexityEnforcer.get_policy(params) + notes = kwargs.get('notes') record_fields, add_attachments = self._parse_fields(kwargs.get('fields', [])) folder_uid = self._resolve_folder(params, kwargs.get('folder_uid')) - self.warnings.clear() data = self._build_record_data(params, record_type, title, notes, record_fields) + self._check_password_policy(params, data, **kwargs) if self.warnings: for w in self.warnings: @@ -165,6 +173,13 @@ def _legacy_to_data(record, title, notes=None): data['notes'] = notes return data + def _check_password_policy(self, params, data, **kwargs): + pw_failures = PasswordComplexityEnforcer.validate_record(params, data) + for failure in pw_failures: + self.on_warning(failure) + if pw_failures and not kwargs.get('force'): + self.on_warning('Use --force to bypass password policy warnings.') + # ══════════════════════════════════════════════════════════════════════════ # nsf-record-update @@ -175,6 +190,7 @@ class NestedShareRecordUpdateCommand(Command, RecordEditMixin): def __init__(self): super().__init__() + RecordEditMixin.__init__(self) def get_parser(self): return nested_share_record_update_parser @@ -190,7 +206,7 @@ def _resolve_field_value(self, parsed): action_params.clear() if self.is_generate_value(raw, action_params): if parsed.type == 'password': - return self.generate_password(action_params) + return self.generate_password(action_params, policy=self._password_policy) if parsed.type in ('oneTimeCode', 'otp'): return self.generate_totp_url() return raw @@ -208,8 +224,12 @@ def execute(self, params, **kwargs): if not record_uids: raise CommandError('nsf-record-update', 'Record UID is required (use -r or --record)') + self.warnings.clear() + self._password_policy = PasswordComplexityEnforcer.get_policy(params) + record_type = kwargs.get('record_type') if record_type and record_type not in ('legacy', 'general'): + RecordTypeEnforcer.enforce(params, record_type, 'nsf-record-update') rt_fields = self.get_record_type_fields(params, record_type) if not rt_fields: raise CommandError('nsf-record-update', f'Record type "{record_type}" cannot be found.') @@ -245,6 +265,20 @@ def execute(self, params, **kwargs): ensure_nested_share_record(params, record_uid, 'nsf-record-update', identifier=identifier) check_record_edit_permission(params, record_uid, 'nsf-record-update') + merged = self._merge_update_data( + params, record_uid, + title=kwargs.get('title'), + record_type=record_type, + fields=fields or None, + notes=kwargs.get('notes'), + ) + self._check_password_policy(params, merged, **kwargs) + if self.warnings: + for w in self.warnings: + logging.warning(w) + if not kwargs.get('force'): + return + self.warnings.clear() result = _nsf.update_record_v3( params=params, record_uid=record_uid, title=kwargs.get('title'), record_type=record_type, @@ -253,6 +287,53 @@ def execute(self, params, **kwargs): check_result(result, 'nsf-record-update') params.sync_data = True + def _check_password_policy(self, params, data, **kwargs): + pw_failures = PasswordComplexityEnforcer.validate_record(params, data) + for failure in pw_failures: + self.on_warning(failure) + if pw_failures and not kwargs.get('force'): + self.on_warning('Use --force to bypass password policy warnings.') + + @staticmethod + def _load_record_data(params, record_uid): # type: (Any, str) -> Optional[Dict] + rec = params.record_cache.get(record_uid) or {} + raw = rec.get('data_unencrypted') + if raw is None: + nsf_data = getattr(params, 'nested_share_record_data', {}).get(record_uid) or {} + raw = nsf_data.get('data_json') + if raw is None: + return None + if isinstance(raw, bytes): + return json.loads(raw.decode('utf-8')) + if isinstance(raw, str): + return json.loads(raw) + if isinstance(raw, dict): + return raw.copy() + return None + + @classmethod + def _merge_update_data(cls, params, record_uid, title=None, record_type=None, + fields=None, notes=None): # type: (...) -> Dict + existing = cls._load_record_data(params, record_uid) + data = existing.copy() if existing else {'fields': []} + if title is not None: + data['title'] = title + if record_type is not None: + data['type'] = record_type + if fields is not None: + by_type = {} + for ef in data.get('fields', []): + by_type.setdefault(ef.get('type'), []).append(ef) + for ft, fv in fields.items(): + fv = fv if isinstance(fv, list) else [fv] + if ft in by_type and by_type[ft]: + by_type[ft][0]['value'] = fv + else: + data.setdefault('fields', []).append({'type': ft, 'value': fv}) + if notes is not None: + data['notes'] = notes + return data + # ══════════════════════════════════════════════════════════════════════════ # nsf-ln diff --git a/keepercommander/commands/record_edit.py b/keepercommander/commands/record_edit.py index 9781bf431..10b62b5f9 100644 --- a/keepercommander/commands/record_edit.py +++ b/keepercommander/commands/record_edit.py @@ -292,7 +292,7 @@ def assign_legacy_fields(self, record, fields): elif parsed_field.type == 'password': action_params.clear() if self.is_generate_value(parsed_field.value, action_params): - record.password = self.generate_password(action_params) + record.password = self.generate_password(action_params, policy=self._password_policy) elif self.is_base64_value(parsed_field.value, action_params): if action_params: record.password = action_params[0] diff --git a/keepercommander/enforcement.py b/keepercommander/enforcement.py index 6d7e802ca..4609568f9 100644 --- a/keepercommander/enforcement.py +++ b/keepercommander/enforcement.py @@ -18,7 +18,7 @@ from typing import Tuple, Optional, List, Dict, Any, Set from . import api, utils, crypto -from .proto import APIRequest_pb2 +from .proto import APIRequest_pb2, record_pb2 from .display import bcolors from .params import KeeperParams from .error import KeeperApiError, CommandError @@ -528,18 +528,30 @@ def get_restricted_record_types(cls, params): # type: (KeeperParams) -> Option cache = getattr(params, 'record_type_cache', None) or {} restricted = set() # type: Set[str] - for rt_id in list(policy.get('std') or []) + list(policy.get('ent') or []): - entry = cache.get(rt_id) - if not entry: - continue - try: - schema = json.loads(entry) if isinstance(entry, str) else entry - except (json.JSONDecodeError, TypeError): - continue - if isinstance(schema, dict): - name = schema.get('$id') - if name: - restricted.add(name) + scope_buckets = ( + ('std', record_pb2.RT_STANDARD), + ('ent', record_pb2.RT_ENTERPRISE), + ) + for bucket, scope in scope_buckets: + for rt_id in policy.get(bucket) or []: + try: + rt_id = int(rt_id) + except (TypeError, ValueError): + continue + # Role policy stores bare recordTypeId; sync_down keys cache by + # recordTypeId + scope * 1_000_000. + scoped_id = rt_id + scope * 1_000_000 + entry = cache.get(scoped_id) or cache.get(rt_id) + if not entry: + continue + try: + schema = json.loads(entry) if isinstance(entry, str) else entry + except (json.JSONDecodeError, TypeError): + continue + if isinstance(schema, dict): + name = schema.get('$id') + if name: + restricted.add(name) return restricted @classmethod diff --git a/unit-tests/test_nested_share_folder.py b/unit-tests/test_nested_share_folder.py index 94ca1b549..c5bd7b591 100644 --- a/unit-tests/test_nested_share_folder.py +++ b/unit-tests/test_nested_share_folder.py @@ -6,6 +6,7 @@ - Key utility/parsing functions """ +import json import os import time from unittest import TestCase, mock @@ -283,6 +284,160 @@ def test_add_record(self, mock_create): title='New Record', folder_uid=fuid, force=True, record_type='general', fields=[]) + @patch('keepercommander.commands.nested_share_folder.record_commands._nsf.create_record_v3') + def test_add_record_rejects_restricted_record_type(self, mock_create): + from keepercommander.commands.nested_share_folder import NestedShareRecordAddCommand + fuid, fobj = _make_folder() + params = _make_params( + nested_share_folders={fuid: fobj}, + record_type_cache={1: json.dumps({'$id': 'login'})}, + enforcements={ + 'jsons': [{'key': 'restrict_record_types', 'value': '{"std": [1], "ent": []}'}], + }, + ) + cmd = NestedShareRecordAddCommand() + with self.assertRaises(CommandError) as ctx: + cmd.execute(params, title='Blocked', record_type='login', fields=[], force=True) + self.assertIn('restricted', str(ctx.exception).lower()) + mock_create.assert_not_called() + + @patch('keepercommander.commands.nested_share_folder.record_commands._nsf.create_record_v3') + def test_add_record_rejects_weak_password_without_force(self, mock_create): + from keepercommander.commands.nested_share_folder import NestedShareRecordAddCommand + fuid, fobj = _make_folder() + params = _make_params( + nested_share_folders={fuid: fobj}, + enforcements={ + 'jsons': [{ + 'key': 'generated_password_complexity', + 'value': json.dumps([{ + 'length': 12, + 'lower-use': True, 'lower-min': 1, + 'upper-use': True, 'upper-min': 1, + 'digit-use': True, 'digit-min': 1, + }]), + }], + }, + ) + cmd = NestedShareRecordAddCommand() + cmd.execute(params, title='Weak', record_type='general', + fields=['password=abc'], force=False) + mock_create.assert_not_called() + + @patch('keepercommander.commands.nested_share_folder.record_commands._nsf.create_record_v3') + def test_add_record_allows_weak_password_with_force(self, mock_create): + from keepercommander.commands.nested_share_folder import NestedShareRecordAddCommand + mock_create.return_value = { + 'record_uid': utils.generate_uid(), 'status': 'SUCCESS', + 'message': '', 'success': True, 'revision': 1, + } + fuid, fobj = _make_folder() + params = _make_params( + nested_share_folders={fuid: fobj}, + enforcements={ + 'jsons': [{ + 'key': 'generated_password_complexity', + 'value': json.dumps([{ + 'length': 12, + 'lower-use': True, 'lower-min': 1, + 'upper-use': True, 'upper-min': 1, + 'digit-use': True, 'digit-min': 1, + }]), + }], + }, + ) + cmd = NestedShareRecordAddCommand() + cmd.execute(params, title='Weak', record_type='general', + fields=['password=abc'], force=True) + mock_create.assert_called_once() + + @patch('keepercommander.commands.nested_share_folder.record_commands._nsf.create_record_v3') + def test_add_record_gen_uses_password_policy(self, mock_create): + from keepercommander.commands.nested_share_folder import NestedShareRecordAddCommand + mock_create.return_value = { + 'record_uid': utils.generate_uid(), 'status': 'SUCCESS', + 'message': '', 'success': True, 'revision': 1, + } + fuid, fobj = _make_folder() + params = _make_params( + nested_share_folders={fuid: fobj}, + enforcements={ + 'jsons': [{ + 'key': 'generated_password_complexity', + 'value': json.dumps([{ + 'length': 16, + 'lower-use': True, 'lower-min': 2, + 'upper-use': True, 'upper-min': 2, + 'digit-use': True, 'digit-min': 2, + 'special-use': True, 'special-min': 1, + 'special': '!@#$', + }]), + }], + }, + ) + cmd = NestedShareRecordAddCommand() + cmd.execute(params, title='Generated', record_type='general', + fields=['password=$GEN'], force=False) + mock_create.assert_called_once() + record_data = mock_create.call_args.kwargs['record_data'] + password = next( + v[0] for f in record_data['fields'] + if f.get('type') == 'password' for v in [f.get('value', [])] if v + ) + self.assertGreaterEqual(len(password), 16) + self.assertGreaterEqual(sum(1 for c in password if c.islower()), 2) + self.assertGreaterEqual(sum(1 for c in password if c.isupper()), 2) + self.assertGreaterEqual(sum(1 for c in password if c.isdigit()), 2) + self.assertGreaterEqual(sum(1 for c in password if c in '!@#$'), 1) + + @patch('keepercommander.commands.nested_share_folder.record_commands._nsf.update_record_v3') + @patch('keepercommander.commands.nested_share_folder.helpers.check_record_edit_permission') + def test_update_record_rejects_restricted_record_type(self, mock_perm, mock_update): + from keepercommander.commands.nested_share_folder import NestedShareRecordUpdateCommand + ruid, robj = _make_record() + params = _make_params( + nested_share_records={ruid: robj}, + record_cache={ruid: {'revision': 1, 'data_unencrypted': json.dumps({ + 'type': 'login', 'title': 'Old', 'fields': [], + })}}, + record_type_cache={1: json.dumps({'$id': 'login'})}, + enforcements={ + 'jsons': [{'key': 'restrict_record_types', 'value': '{"std": [1], "ent": []}'}], + }, + ) + cmd = NestedShareRecordUpdateCommand() + with self.assertRaises(CommandError) as ctx: + cmd.execute(params, record_uids=[ruid], record_type='login', fields=[]) + self.assertIn('restricted', str(ctx.exception).lower()) + mock_update.assert_not_called() + + @patch('keepercommander.commands.nested_share_folder.record_commands._nsf.update_record_v3') + @patch('keepercommander.commands.nested_share_folder.helpers.check_record_edit_permission') + def test_update_record_rejects_weak_password_without_force(self, mock_perm, mock_update): + from keepercommander.commands.nested_share_folder import NestedShareRecordUpdateCommand + ruid, robj = _make_record() + params = _make_params( + nested_share_records={ruid: robj}, + record_cache={ruid: {'revision': 1, 'data_unencrypted': json.dumps({ + 'type': 'login', 'title': 'Old', + 'fields': [{'type': 'password', 'value': ['ExistingPass123']}], + })}}, + enforcements={ + 'jsons': [{ + 'key': 'generated_password_complexity', + 'value': json.dumps([{ + 'length': 12, + 'lower-use': True, 'lower-min': 1, + 'upper-use': True, 'upper-min': 1, + 'digit-use': True, 'digit-min': 1, + }]), + }], + }, + ) + cmd = NestedShareRecordUpdateCommand() + cmd.execute(params, record_uids=[ruid], fields=['password=abc'], force=False) + mock_update.assert_not_called() + @patch('keepercommander.nested_share_folder.folder_record_api.add_record_to_folder_v3') def test_add_record_to_folder(self, mock_add): pass From 49399a1129c314fd77c1f5b036ddeb1a4dbde267 Mon Sep 17 00:00:00 2001 From: Sergey Kolupaev Date: Mon, 15 Jun 2026 21:29:48 -0700 Subject: [PATCH 04/11] KEPM: system collection --- keepercommander/pedm/pedm_shared.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/keepercommander/pedm/pedm_shared.py b/keepercommander/pedm/pedm_shared.py index eed1dad3e..3fecc2045 100644 --- a/keepercommander/pedm/pedm_shared.py +++ b/keepercommander/pedm/pedm_shared.py @@ -48,6 +48,7 @@ class CollectionType(int, enum.Enum): UserName = 10, CustomAppCollection = 102, CustomUserCollection = 103, + SystemAppCollection = 105, CustomMachineCollection = 201, OsVersion = 202, @@ -69,6 +70,8 @@ def collection_type_to_name(collection_type: int) -> str: return 'App Collection' if collection_type == CollectionType.CustomUserCollection: return 'User Collection' + if collection_type == CollectionType.SystemAppCollection: + return 'System App Collection' if collection_type == CollectionType.CustomMachineCollection: return 'Machine Collection' if collection_type == CollectionType.OsVersion: @@ -141,8 +144,12 @@ def get_collection_required_fields(collection_type: Optional[int]) -> Optional[C return CollectionRequiredFields(['Name']) if collection_type == CollectionType.UserName: return CollectionRequiredFields(['Name']) + if collection_type == CollectionType.CustomAppCollection: + return CollectionRequiredFields(['Name']) if collection_type == CollectionType.CustomUserCollection: return CollectionRequiredFields(['Name']) + if collection_type == CollectionType.SystemAppCollection: + return CollectionRequiredFields(['Name']) if collection_type == CollectionType.OsVersion: return CollectionRequiredFields(['Name']) return None From 2a8117771c95cd90d28129ce0c4aa0e1bab0c7c3 Mon Sep 17 00:00:00 2001 From: sshrushanth-ks Date: Tue, 16 Jun 2026 14:54:55 +0530 Subject: [PATCH 05/11] KC-1307, KC-1308, KC-1309 & KC-1310: Fix nsf share expiration updates, rename Supershell Drive folder labels, and standardize list record_category Fixed nsf-share-folder and nsf-share-record expiration updates, enforced a one-minute minimum on NSF and classic share commands, standardized list/search record_category to lowercase classic/nested, and renamed Supershell Drive folder labels to Nested Shared Folder (Shared) and (NonShared). --- .../commands/nested_share_folder/helpers.py | 28 ++- .../nested_share_folder/sharing_commands.py | 47 ++-- keepercommander/commands/record.py | 8 +- keepercommander/commands/register.py | 19 +- keepercommander/commands/supershell/app.py | 12 +- .../commands/supershell/screens/help.py | 4 +- .../nested_share_folder/__init__.py | 1 + .../nested_share_folder/folder_api.py | 15 +- .../nested_share_folder/record_api.py | 199 ++++++++++++--- unit-tests/test_command_register.py | 7 + unit-tests/test_nested_share_folder.py | 229 ++++++++++++++++++ 11 files changed, 500 insertions(+), 69 deletions(-) diff --git a/keepercommander/commands/nested_share_folder/helpers.py b/keepercommander/commands/nested_share_folder/helpers.py index c6624b137..5c990d04a 100644 --- a/keepercommander/commands/nested_share_folder/helpers.py +++ b/keepercommander/commands/nested_share_folder/helpers.py @@ -74,6 +74,9 @@ re.IGNORECASE, ) +MIN_SHARE_EXPIRATION_MS = 60_000 +"""Minimum share expiration is one minute (milliseconds).""" + # ═══════════════════════════════════════════════════════════════════════════ # Error-handling patterns (eliminates repetitive try/except boilerplate) @@ -306,6 +309,18 @@ def walk(fuid): # Expiration parsing # ═══════════════════════════════════════════════════════════════════════════ +def validate_share_expiration_timestamp(expiration_ms, cmd_name): + """Reject finite expirations that are less than one minute.""" + if expiration_ms is None or expiration_ms == -1: + return + min_allowed = int(datetime.datetime.now(timezone.utc).timestamp() * 1000) + MIN_SHARE_EXPIRATION_MS + if expiration_ms < min_allowed: + raise CommandError( + cmd_name, + 'Share expiration must be at least 1 minute.', + ) + + def parse_expiration(expire_at, expire_in, cmd_name): """Parse ``--expire-at`` / ``--expire-in`` into a millisecond timestamp. @@ -320,13 +335,15 @@ def parse_expiration(expire_at, expire_in, cmd_name): if expire_at: try: dt = datetime.datetime.fromisoformat(raw.replace('Z', '+00:00')) - return int(dt.timestamp() * 1000) + expiration_ms = int(dt.timestamp() * 1000) except ValueError: raise CommandError( cmd_name, f'Invalid --expire-at format: {raw!r}. ' f'Use ISO datetime, e.g. 2027-01-01T00:00:00Z or "never"', ) + validate_share_expiration_timestamp(expiration_ms, cmd_name) + return expiration_ms m = _EXPIRATION_RE.fullmatch(raw) if not m: @@ -336,6 +353,11 @@ def parse_expiration(expire_at, expire_in, cmd_name): ) amount = int(m.group(1)) unit = m.group(2).lower() + if unit.startswith('mi') and amount < 1: + raise CommandError( + cmd_name, + 'Share expiration must be at least 1 minute.', + ) now = datetime.datetime.now(timezone.utc) delta_map = { 'mi': timedelta(minutes=amount), @@ -345,7 +367,9 @@ def parse_expiration(expire_at, expire_in, cmd_name): 'y': timedelta(days=amount * 365), } delta = next(v for k, v in delta_map.items() if unit.startswith(k)) - return int((now + delta).timestamp() * 1000) + expiration_ms = int((now + delta).timestamp() * 1000) + validate_share_expiration_timestamp(expiration_ms, cmd_name) + return expiration_ms # ═══════════════════════════════════════════════════════════════════════════ diff --git a/keepercommander/commands/nested_share_folder/sharing_commands.py b/keepercommander/commands/nested_share_folder/sharing_commands.py index a8c5a207b..d4029fac5 100644 --- a/keepercommander/commands/nested_share_folder/sharing_commands.py +++ b/keepercommander/commands/nested_share_folder/sharing_commands.py @@ -98,8 +98,23 @@ def _dispatch(params, action, record_uid, email, access_role_type, expiration): params=params, record_uid=record_uid, new_owner_email=email), 'owner') if action == 'grant': - if NestedShareRecordShareCommand._is_already_shared( - params, record_uid, email): + existing = NestedShareRecordShareCommand._get_direct_user_share( + params, record_uid, email) + if existing: + if _nsf.is_record_share_update_noop( + existing, access_role_type, expiration): + logging.info( + "Record '%s' already shared with '%s' at the requested " + "role and expiration; no change needed.", + record_uid, email) + return ({ + 'results': [{ + 'record_uid': record_uid, + 'success': True, + 'message': 'Already has requested access', + }], + 'success': True, + }, 'update') logging.debug( "Record '%s' is already shared with user '%s'; switching to update.", record_uid, email) @@ -115,6 +130,17 @@ def _dispatch(params, action, record_uid, email, access_role_type, expiration): return (_nsf.unshare_record_v3( params=params, record_uid=record_uid, recipient_email=email), 'revoke') + @staticmethod + def _get_direct_user_share(params, record_uid, email): + """Return direct AT_USER share metadata for *email*, or None.""" + try: + access_result = _nsf.get_record_accesses_v3(params, [record_uid]) + return _nsf.find_direct_user_share_access( + access_result, record_uid, email) + except Exception as exc: + logging.debug("Could not fetch record accesses for '%s': %s", record_uid, exc) + return None + @staticmethod def _is_already_shared(params, record_uid, email): """Return True if *email* already has a *direct* non-owner share on *record_uid*. @@ -126,21 +152,8 @@ def _is_already_shared(params, record_uid, email): causes the caller to dispatch ``share_record_v3`` (a fresh direct grant) which correctly overrides the inherited folder permission. """ - try: - access_result = _nsf.get_record_accesses_v3(params, [record_uid]) - for a in access_result.get('record_accesses', []): - if a.get('record_uid') != record_uid or a.get('owner', False): - continue - if a.get('access_type') and a.get('access_type') != 'AT_USER': - continue - if a.get('inherited'): - continue - if a.get('accessor_name', '').casefold() == email.casefold(): - return True - return False - except Exception as exc: - logging.debug("Could not fetch record accesses for '%s': %s", record_uid, exc) - return False + return NestedShareRecordShareCommand._get_direct_user_share( + params, record_uid, email) is not None @staticmethod def _log_results(result, action, email): diff --git a/keepercommander/commands/record.py b/keepercommander/commands/record.py index 7977d0ab5..87639cf0f 100644 --- a/keepercommander/commands/record.py +++ b/keepercommander/commands/record.py @@ -1558,7 +1558,7 @@ def execute(self, params, **kwargs): 'record_type': record.record_type, 'title': record.title, 'description': vault_extensions.get_record_description(record), - 'record_category': 'Nested' if is_nsf else 'Classic', + 'record_category': 'nested' if is_nsf else 'classic', } all_results.append(result_item) else: @@ -1566,7 +1566,7 @@ def execute(self, params, **kwargs): table = [] headers = ['Record UID', 'Type', 'Title', 'Description', 'Record Category'] for record in records: - record_category = 'Nested' if record.record_uid in nsf_records_map else 'Classic' + record_category = 'nested' if record.record_uid in nsf_records_map else 'classic' row = [record.record_uid, record.record_type, record.title, vault_extensions.get_record_description(record), record_category] table.append(row) @@ -1655,7 +1655,7 @@ def execute(self, params, **kwargs): for item in all_results: if item['type'] == 'record': row = [item['type'], item['record_uid'], item['title'], - f"Type: {item['record_type']}, Description: {item['description']}, Record Category: {item.get('record_category', 'Classic')}"] + 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"Folder Category: Classic, Can Edit: {item['can_edit']}, Can Share: {item['can_share']}"] @@ -1832,7 +1832,7 @@ def execute(self, params, **kwargs): for record in records: # Determine if record is from Nested Share Folder or Classic is_nested_share = hasattr(params, 'nested_share_records') and record.record_uid in params.nested_share_records - record_category = 'Nested' if is_nested_share else 'Classic' + record_category = 'nested' if is_nested_share else 'classic' row = [record.record_uid, record.record_type, record.title, vault_extensions.get_record_description(record), record.shared, record_category] table.append(row) diff --git a/keepercommander/commands/register.py b/keepercommander/commands/register.py index 858f3e6de..5aac059db 100644 --- a/keepercommander/commands/register.py +++ b/keepercommander/commands/register.py @@ -227,7 +227,7 @@ def register_command_info(aliases, command_info): create_account_parser = argparse.ArgumentParser(prog='create-account', description='Create Keeper Account') create_account_parser.add_argument('email', help='email') -def get_share_expiration(expire_at, expire_in): # (Optional[str], Optional[str]) -> Optional[int] +def get_share_expiration(expire_at, expire_in, cmd_name='share-record'): # (Optional[str], Optional[str], str) -> Optional[int] if not expire_at and not expire_in: return @@ -240,11 +240,20 @@ def get_share_expiration(expire_at, expire_in): # (Optional[str], Optional[s if expire_in == 'never': return -1 td = parse_timeout(expire_in) + if td < datetime.timedelta(minutes=1): + raise CommandError( + cmd_name, + 'Share expiration must be at least 1 minute.', + + ) dt = datetime.datetime.now() + td if dt is None: raise ValueError(f'Incorrect expiration: {expire_at or expire_in}') - return int(dt.timestamp()) + expiration_seconds = int(dt.timestamp()) + from .nested_share_folder.helpers import validate_share_expiration_timestamp + validate_share_expiration_timestamp(expiration_seconds * 1000, cmd_name) + return expiration_seconds class ShareFolderCommand(Command): @@ -310,7 +319,8 @@ def get_share_admin_obj_uids(obj_names, obj_type): share_expiration = None if action == 'grant': - share_expiration = get_share_expiration(kwargs.get('expire_at'), kwargs.get('expire_in')) + share_expiration = get_share_expiration( + kwargs.get('expire_at'), kwargs.get('expire_in'), cmd_name='share-folder') rotate_on_expiration = bool(kwargs.get('rotate_on_expiration')) if rotate_on_expiration: @@ -690,7 +700,8 @@ def prep_request(params, kwargs): # type: (KeeperParams, Dict[str, Any]) -> Un share_expiration = None if action == 'grant': - share_expiration = get_share_expiration(kwargs.get('expire_at'), kwargs.get('expire_in')) + share_expiration = get_share_expiration( + kwargs.get('expire_at'), kwargs.get('expire_in'), cmd_name='share-record') rotate_on_expiration = bool(kwargs.get('rotate_on_expiration')) if rotate_on_expiration: diff --git a/keepercommander/commands/supershell/app.py b/keepercommander/commands/supershell/app.py index 8275f40ab..10e48f3f3 100644 --- a/keepercommander/commands/supershell/app.py +++ b/keepercommander/commands/supershell/app.py @@ -254,8 +254,8 @@ def _get_welcome_screen_content(self) -> str: [bold {t['primary_bright']}]Folder Icons[/bold {t['primary_bright']}] [{t['text_dim']}]•[/{t['text_dim']}] Legacy Personal Folder 🔒 [{t['text_dim']}]•[/{t['text_dim']}] Legacy Shared Folder 📦 - [{t['text_dim']}]•[/{t['text_dim']}] Drive Shared Folder 👥 - [{t['text_dim']}]•[/{t['text_dim']}] Drive NonShared Folder 📁 + [{t['text_dim']}]•[/{t['text_dim']}] Nested Shared Folder (Shared) 👥 + [{t['text_dim']}]•[/{t['text_dim']}] Nested Shared Folder (NonShared) 📁 [{t['text_dim']}]Press [/{t['text_dim']}][{t['primary']}]?[/{t['primary']}][{t['text_dim']}] for full keyboard shortcuts[/{t['text_dim']}]""" @@ -393,8 +393,8 @@ async def on_mount(self): [bold {t['primary_bright']}]Folder Icons[/bold {t['primary_bright']}] [{t['text_dim']}]•[/{t['text_dim']}] Legacy Personal Folder 🔒 [{t['text_dim']}]•[/{t['text_dim']}] Legacy Shared Folder 📦 - [{t['text_dim']}]•[/{t['text_dim']}] Drive Shared Folder 👥 - [{t['text_dim']}]•[/{t['text_dim']}] Drive NonShared Folder 📁 + [{t['text_dim']}]•[/{t['text_dim']}] Nested Shared Folder (Shared) 👥 + [{t['text_dim']}]•[/{t['text_dim']}] Nested Shared Folder (NonShared) 📁 [{t['text_dim']}]Press [/{t['text_dim']}][{t['primary']}]?[/{t['primary']}][{t['text_dim']}] for full keyboard shortcuts[/{t['text_dim']}]""" detail_widget.update(help_content) @@ -984,8 +984,8 @@ def _get_folder_icon(self, folder_node): - Legacy Personal Folder (user_folder) → 🔒 - Legacy Shared Folder (shared_folder) → 📦 - Subfolder in Shared (shared_folder_folder) → 📦 - - Drive Shared Folder (nested_share_folder, shared) → 👥 - - Drive NonShared Folder (nested_share_folder, not shared) → 📁 + - Nested Shared Folder (Shared) (nested_share_folder, shared) → 👥 + - Nested Shared Folder (NonShared) (nested_share_folder, not shared) → 📁 """ from ...subfolder import BaseFolderNode if folder_node is None: diff --git a/keepercommander/commands/supershell/screens/help.py b/keepercommander/commands/supershell/screens/help.py index dcf13f088..a92a39c5f 100644 --- a/keepercommander/commands/supershell/screens/help.py +++ b/keepercommander/commands/supershell/screens/help.py @@ -119,8 +119,8 @@ def compose(self) -> ComposeResult: [green]Folder Icons:[/green] 🔒 Legacy Personal Folder 📦 Legacy Shared Folder - 👥 Drive Shared Folder - 📁 Drive NonShared Folder""", classes="help_column") + 👥 Nested Shared Folder (Shared) + 📁 Nested Shared Folder (NonShared)""", classes="help_column") yield Static("[dim]Press Esc or q to close[/dim]", id="help_footer") def action_dismiss(self): diff --git a/keepercommander/nested_share_folder/__init__.py b/keepercommander/nested_share_folder/__init__.py index a4a6641b6..8436b6ba1 100644 --- a/keepercommander/nested_share_folder/__init__.py +++ b/keepercommander/nested_share_folder/__init__.py @@ -40,6 +40,7 @@ 'create_record_data_v3', 'record_add_v3', 'record_update_v3', 'create_record_v3', 'update_record_v3', 'create_records_batch_v3', 'get_record_details_v3', 'get_record_accesses_v3', + 'find_direct_user_share_access', 'is_record_share_update_noop', 'share_record_v3', 'update_record_share_v3', 'unshare_record_v3', 'batch_update_record_shares_v3', 'batch_create_record_shares_v3', 'batch_unshare_records_v3', diff --git a/keepercommander/nested_share_folder/folder_api.py b/keepercommander/nested_share_folder/folder_api.py index 9c40fe00e..6656f324e 100644 --- a/keepercommander/nested_share_folder/folder_api.py +++ b/keepercommander/nested_share_folder/folder_api.py @@ -364,14 +364,15 @@ def grant_folder_access_v3(params, folder_uid, user_uid, role='viewer', existing = _check_existing_access(params, folder_uid, actual_uid_bytes, target_role_name, access_type_label) if existing is not None: - if existing == target_role_name: + if existing == target_role_name and expiration_timestamp is None: return {'folder_uid': folder_uid, 'user_uid': identifier_label, 'access_type': access_type_label, 'status': 'SUCCESS', 'message': f"{'Team' if as_team else 'User'} already has {role} access", 'success': True, 'action_taken': 'already_had_access'} result = update_folder_access_v3(params, folder_uid, identifier_label, - role=role, as_team=as_team) + role=role, as_team=as_team, + expiration_timestamp=expiration_timestamp) result['action_taken'] = 'updated' return result @@ -382,7 +383,7 @@ def grant_folder_access_v3(params, folder_uid, user_uid, role='viewer', ad.accessRoleType = access_role ad.permissions.CopyFrom(get_folder_permissions_for_role(access_role)) - if expiration_timestamp: + if expiration_timestamp is not None: ad.tlaProperties.expiration = expiration_timestamp if share_folder_key: @@ -430,9 +431,9 @@ def _check_existing_access(params, folder_uid, uid_bytes, target_role_name, def update_folder_access_v3(params, folder_uid, user_uid, role=None, hidden=None, - as_team=False): - if role is None and hidden is None: - raise ValueError("At least one field (role or hidden) required") + expiration_timestamp=None, as_team=False): + if role is None and hidden is None and expiration_timestamp is None: + raise ValueError("At least one field (role, hidden, or expiration) required") resolved = resolve_folder_identifier(params, folder_uid) if not resolved: raise ValueError(f"Folder '{folder_uid}' not found") @@ -453,6 +454,8 @@ def update_folder_access_v3(params, folder_uid, user_uid, role=None, hidden=None ad.permissions.CopyFrom(get_folder_permissions_for_role(resolved_role)) if hidden is not None: ad.hidden = hidden + if expiration_timestamp is not None: + ad.tlaProperties.expiration = expiration_timestamp response = folder_access_update_v3(params, folder_access_updates=[ad]) result = parse_folder_access_result(response, folder_uid, identifier_label, diff --git a/keepercommander/nested_share_folder/record_api.py b/keepercommander/nested_share_folder/record_api.py index db886a78b..f0bcc9e69 100644 --- a/keepercommander/nested_share_folder/record_api.py +++ b/keepercommander/nested_share_folder/record_api.py @@ -9,7 +9,7 @@ from .. import utils, crypto, api from ..params import KeeperParams -from ..proto import record_pb2, folder_pb2, record_endpoints_pb2, record_details_pb2, record_sharing_pb2 +from ..proto import record_pb2, folder_pb2, record_endpoints_pb2, record_details_pb2, record_sharing_pb2, tla_pb2 from ..error import KeeperApiError from ..api import pad_aes_gcm @@ -336,21 +336,84 @@ def get_record_accesses_v3(params, record_uids): 'can_update_access', 'can_delete', 'can_change_ownership', 'can_request_access', 'can_approve_access'): ao[flag] = getattr(d, flag, False) + # Proto3 scalar fields have no presence; only test the submessage. + if d.HasField('tlaProperties'): + ao['tla_expiration'] = d.tlaProperties.expiration result['record_accesses'].append(ao) for fu in rs.forbiddenRecords: result['forbidden_records'].append(utils.base64_url_encode(fu)) return result +def find_direct_user_share_access(access_result, record_uid, email): + """Return the direct AT_USER share row for *email* on *record_uid*, or None.""" + email_cf = email.casefold() + for access in access_result.get('record_accesses', []): + if access.get('record_uid') != record_uid or access.get('owner', False): + continue + if access.get('access_type') and access.get('access_type') != 'AT_USER': + continue + if access.get('inherited'): + continue + if access.get('accessor_name', '').casefold() == email_cf: + return access + return None + + +_SHARE_EXPIRATION_NOOP_TOLERANCE_MS = 60_000 + + +def is_record_share_update_noop(existing, access_role_type, expiration_timestamp): + """True when an update would leave role and expiration unchanged.""" + if existing.get('access_role_type') != access_role_type: + return False + existing_exp = existing.get('tla_expiration') + if expiration_timestamp is None: + return True + if expiration_timestamp == -1: + return not existing_exp or existing_exp <= 0 + if expiration_timestamp > 0: + if not existing_exp or existing_exp <= 0: + return False + return abs(existing_exp - expiration_timestamp) <= _SHARE_EXPIRATION_NOOP_TOLERANCE_MS + return False + + # ══════════════════════════════════════════════════════════════════════════ # Record sharing (Strategy: share / update / revoke) # ══════════════════════════════════════════════════════════════════════════ +def _apply_share_expiration_tla(tla_props, expiration_timestamp): + """Set expiration / notification fields on a TLAProperties proto.""" + if expiration_timestamp is None: + return + tla_props.expiration = expiration_timestamp + if expiration_timestamp > 0: + tla_props.timerNotificationType = tla_pb2.NOTIFY_OWNER + + +def _build_revoke_permission(params, record_uid, recipient_email): + """Build a minimal Permissions proto for revokeSharingPermissions.""" + _, _, uid_bytes, _ = get_user_public_key(params, recipient_email) + if not uid_bytes: + raise ValueError(f"User {recipient_email} not found") + + uid_b = utils.base64_url_decode(record_uid) + perm = record_sharing_pb2.Permissions() + perm.recipientUid = uid_bytes + perm.recordUid = uid_b + perm.rules.accessTypeUid = uid_bytes + perm.rules.accessType = folder_pb2.AT_USER + perm.rules.recordUid = uid_b + return perm + + def _build_share_permissions(params, record_uid, recipient_email, access_role_type, - expiration_timestamp, include_role): + expiration_timestamp, include_role, skip_sync=False): """Build a Permissions protobuf for share/update — single source of truth.""" - from .. import sync_down as sd - sd.sync_down(params) + if not skip_sync: + from .. import sync_down as sd + sd.sync_down(params) rec = get_record_from_cache(params, record_uid) if not rec: @@ -381,16 +444,25 @@ def _build_share_permissions(params, record_uid, recipient_email, access_role_ty perm.rules.owner = False if include_role and access_role_type is not None: perm.rules.accessRoleType = access_role_type - if expiration_timestamp: - perm.rules.tlaProperties.expiration = expiration_timestamp + _apply_share_expiration_tla(perm.rules.tlaProperties, expiration_timestamp) return perm -def share_record_v3(params, record_uid, recipient_email, access_role_type, - expiration_timestamp=None): +def _send_revoke_share_request(params, record_uid, recipient_email): + perm = _build_revoke_permission(params, record_uid, recipient_email) + rq = record_sharing_pb2.Request() + rq.revokeSharingPermissions.append(perm) + rs = api.communicate_rest(params, rq, 'vault/records/v3/share', + rs_type=record_sharing_pb2.Response) + results = [parse_sharing_status(s) for s in rs.revokedSharingStatus] + return {'results': results, 'success': all(r['success'] for r in results)} + + +def _send_create_share_request(params, record_uid, recipient_email, access_role_type, + expiration_timestamp, skip_sync=False): perm = _build_share_permissions(params, record_uid, recipient_email, access_role_type, expiration_timestamp, - include_role=True) + include_role=True, skip_sync=skip_sync) rq = record_sharing_pb2.Request() rq.createSharingPermissions.append(perm) rs = api.communicate_rest(params, rq, 'vault/records/v3/share', @@ -399,46 +471,78 @@ def share_record_v3(params, record_uid, recipient_email, access_role_type, return {'results': results, 'success': all(r['success'] for r in results)} -def update_record_share_v3(params, record_uid, recipient_email, - access_role_type=None, expiration_timestamp=None): +def share_record_v3(params, record_uid, recipient_email, access_role_type, + expiration_timestamp=None): perm = _build_share_permissions(params, record_uid, recipient_email, access_role_type, expiration_timestamp, include_role=True) rq = record_sharing_pb2.Request() - rq.updateSharingPermissions.append(perm) + rq.createSharingPermissions.append(perm) rs = api.communicate_rest(params, rq, 'vault/records/v3/share', rs_type=record_sharing_pb2.Response) - results = [parse_sharing_status(s) for s in rs.updatedSharingStatus] + results = [parse_sharing_status(s) for s in rs.createdSharingStatus] return {'results': results, 'success': all(r['success'] for r in results)} -def unshare_record_v3(params, record_uid, recipient_email): +def update_record_share_v3(params, record_uid, recipient_email, + access_role_type=None, expiration_timestamp=None): + """Update an existing direct share. + + Role changes use ``updateSharingPermissions``. Positive expirations use + revoke then create (two requests) because the server only INSERTs + ``record_access_expiration`` on create — sending ``tlaProperties`` on + update fails when the share already exists. + """ from .. import sync_down as sd sd.sync_down(params) rec = get_record_from_cache(params, record_uid) if not rec: raise ValueError(f"Record {record_uid} not found in cache") - _, _, uid_bytes, _ = get_user_public_key(params, recipient_email) - if not uid_bytes: - raise ValueError(f"User {recipient_email} not found") - uid_b = utils.base64_url_decode(record_uid) - perm = record_sharing_pb2.Permissions() - perm.recipientUid = uid_bytes - perm.recordUid = uid_b - perm.rules.accessTypeUid = uid_bytes - perm.rules.accessType = folder_pb2.AT_USER - perm.rules.recordUid = uid_b + if expiration_timestamp is not None and expiration_timestamp > 0: + if access_role_type is None: + raise ValueError( + 'access_role_type is required when setting share expiration') + # Expiration must be applied via create, not update. The server + # rejects revoke+create for the same (recipient, record) in one + # request, so run them sequentially (one sync_down above only). + logger.info( + "Revoking existing share with '%s' before re-granting with expiration...", + recipient_email) + revoke_result = _send_revoke_share_request(params, record_uid, recipient_email) + if not revoke_result.get('success'): + return revoke_result + logger.info( + "Re-granting share to '%s' with updated role and expiration...", + recipient_email) + return _send_create_share_request( + params, record_uid, recipient_email, access_role_type, + expiration_timestamp, skip_sync=True) + perm = _build_share_permissions(params, record_uid, recipient_email, + access_role_type, expiration_timestamp, + include_role=True, skip_sync=True) rq = record_sharing_pb2.Request() - rq.revokeSharingPermissions.append(perm) + rq.updateSharingPermissions.append(perm) rs = api.communicate_rest(params, rq, 'vault/records/v3/share', rs_type=record_sharing_pb2.Response) - results = [parse_sharing_status(s) for s in rs.revokedSharingStatus] + results = [parse_sharing_status(s) for s in rs.updatedSharingStatus] return {'results': results, 'success': all(r['success'] for r in results)} +def unshare_record_v3(params, record_uid, recipient_email, skip_sync=False): + if not skip_sync: + from .. import sync_down as sd + sd.sync_down(params) + + rec = get_record_from_cache(params, record_uid) + if not rec: + raise ValueError(f"Record {record_uid} not found in cache") + + return _send_revoke_share_request(params, record_uid, recipient_email) + + # ══════════════════════════════════════════════════════════════════════════ # Batch record sharing (bulk update / revoke in a single REST call per chunk) # ══════════════════════════════════════════════════════════════════════════ @@ -461,6 +565,45 @@ def batch_update_record_shares_v3(params, updates, expiration_timestamp=None, ch sd.sync_down(params) outcomes = [] + recreate = expiration_timestamp is not None and expiration_timestamp > 0 + if recreate: + for i in range(0, len(updates), chunk_size): + chunk = updates[i:i + chunk_size] + rq = record_sharing_pb2.Request() + built = [] + for u in chunk: + try: + rq.revokeSharingPermissions.append( + _build_revoke_permission(params, u['record_uid'], u['email'])) + built.append(u) + except Exception as exc: + outcomes.append((u, {'success': False, 'skipped': True, + 'message': str(exc)})) + if not built: + continue + try: + rs = api.communicate_rest(params, rq, 'vault/records/v3/share', + rs_type=record_sharing_pb2.Response) + statuses = [parse_sharing_status(x) for x in rs.revokedSharingStatus] + if statuses: + revoked = {s['record_uid'] for s in statuses if s['success']} + else: + revoked = {u['record_uid'] for u in built} + for u in built: + if u['record_uid'] not in revoked: + outcomes.append((u, {'success': False, + 'message': 'Revoke failed'})) + except Exception as exc: + for u in built: + outcomes.append((u, {'success': False, 'message': str(exc)})) + failed_uids = {u['record_uid'] for u, r in outcomes if not r.get('success')} + create_updates = [u for u in updates if u['record_uid'] not in failed_uids] + if create_updates: + create_outcomes = batch_create_record_shares_v3( + params, create_updates, expiration_timestamp, chunk_size) + outcomes.extend(create_outcomes) + return outcomes + for i in range(0, len(updates), chunk_size): chunk = updates[i:i + chunk_size] rq = record_sharing_pb2.Request() @@ -470,7 +613,7 @@ def batch_update_record_shares_v3(params, updates, expiration_timestamp=None, ch perm = _build_share_permissions( params, u['record_uid'], u['email'], u['access_role_type'], expiration_timestamp, - include_role=True, + include_role=True, skip_sync=True, ) rq.updateSharingPermissions.append(perm) built.append(u) @@ -526,7 +669,7 @@ def batch_create_record_shares_v3(params, creates, expiration_timestamp=None, ch perm = _build_share_permissions( params, c['record_uid'], c['email'], c['access_role_type'], expiration_timestamp, - include_role=True, + include_role=True, skip_sync=True, ) rq.createSharingPermissions.append(perm) built.append(c) diff --git a/unit-tests/test_command_register.py b/unit-tests/test_command_register.py index 2da3c9b00..3074af5a7 100644 --- a/unit-tests/test_command_register.py +++ b/unit-tests/test_command_register.py @@ -201,6 +201,13 @@ def test_share_record_update_expiration_without_roe(self): finally: TestRegister.record_share_rq_rs = original + def test_get_share_expiration_rejects_sub_minute(self): + with self.assertRaises(CommandError) as ctx: + register.get_share_expiration(None, '0mi', cmd_name='share-record') + self.assertIn('at least 1 minute', str(ctx.exception)) + with self.assertRaises(CommandError): + register.get_share_expiration('2020-01-01T00:00:00', None, cmd_name='share-record') + def test_share_record_update_clears_expiration_with_never(self): """--expire-at never on an existing share must clear the timer (expiration = -1).""" params = get_synced_params() diff --git a/unit-tests/test_nested_share_folder.py b/unit-tests/test_nested_share_folder.py index c5bd7b591..a9e784ae5 100644 --- a/unit-tests/test_nested_share_folder.py +++ b/unit-tests/test_nested_share_folder.py @@ -69,6 +69,15 @@ def _make_record(record_uid=None, title='Test Record'): } +def _make_sharing_status(record_uid, recipient_uid_bytes=None): + from keepercommander.proto import record_sharing_pb2 + status = record_sharing_pb2.Status() + status.recordUid = utils.base64_url_decode(record_uid) + status.recipientUid = recipient_uid_bytes or utils.base64_url_decode(utils.generate_uid()) + status.status = record_sharing_pb2.SUCCESS + return status + + class TestCommandHelpers(TestCase): def test_parse_expiration_none(self): @@ -99,6 +108,14 @@ def test_parse_expiration_invalid(self): with self.assertRaises(CommandError): parse_expiration(None, 'invalid', 'test') + def test_parse_expiration_rejects_sub_minute(self): + from keepercommander.commands.nested_share_folder.helpers import parse_expiration + with self.assertRaises(CommandError) as ctx: + parse_expiration(None, '0mi', 'test') + self.assertIn('at least 1 minute', str(ctx.exception)) + with self.assertRaises(CommandError): + parse_expiration('2020-01-01T00:00:00Z', None, 'test') + def test_infer_role(self): from keepercommander.commands.nested_share_folder.helpers import infer_role self.assertEqual(infer_role({'can_change_ownership': True}), 'full-manager') @@ -783,6 +800,218 @@ def test_grant_folder_access_sends_invite_when_no_active_share( mock_handle_invite.assert_called_once_with(params, email, True) mock_access_update.assert_not_called() + @patch('keepercommander.nested_share_folder.folder_api.parse_folder_access_result') + @patch('keepercommander.nested_share_folder.folder_api.folder_access_update_v3') + @patch('keepercommander.nested_share_folder.folder_api._resolve_accessor') + @patch('keepercommander.nested_share_folder.folder_api.resolve_folder_identifier') + def test_update_folder_access_v3_sets_expiration( + self, mock_resolve_folder, mock_resolve_accessor, + mock_access_update, mock_parse_result): + from keepercommander.nested_share_folder.folder_api import update_folder_access_v3 + + fuid, _ = _make_folder() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_resolve_folder.return_value = fuid + mock_resolve_accessor.return_value = (uid_bytes, email, 1) + mock_parse_result.return_value = {'success': True} + mock_access_update.return_value = Mock() + + expiration = 1_700_000_000_000 + update_folder_access_v3( + _make_params(), fuid, email, expiration_timestamp=expiration) + + update_call = mock_access_update.call_args + ad = update_call.kwargs['folder_access_updates'][0] + self.assertEqual(ad.tlaProperties.expiration, expiration) + + @patch('keepercommander.nested_share_folder.folder_api.update_folder_access_v3') + @patch('keepercommander.nested_share_folder.folder_api._check_existing_access') + @patch('keepercommander.nested_share_folder.folder_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.folder_api.resolve_folder_identifier') + def test_grant_folder_access_update_passes_expiration( + self, mock_resolve_folder, mock_get_public_key, + mock_existing, mock_update): + from keepercommander.nested_share_folder.folder_api import grant_folder_access_v3 + + fuid, fobj = _make_folder() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_resolve_folder.return_value = fuid + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_existing.return_value = 'viewer' + mock_update.return_value = {'success': True} + + expiration = 1_800_000_000_000 + grant_folder_access_v3( + _make_params(nested_share_folders={fuid: fobj}), + fuid, email, role='content-manager', expiration_timestamp=expiration) + + mock_update.assert_called_once_with( + mock.ANY, fuid, email, role='content-manager', as_team=False, + expiration_timestamp=expiration) + + @patch('keepercommander.nested_share_folder.folder_api.update_folder_access_v3') + @patch('keepercommander.nested_share_folder.folder_api._check_existing_access') + @patch('keepercommander.nested_share_folder.folder_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.folder_api.resolve_folder_identifier') + def test_grant_folder_access_same_role_updates_expiration( + self, mock_resolve_folder, mock_get_public_key, + mock_existing, mock_update): + from keepercommander.nested_share_folder.folder_api import grant_folder_access_v3 + + fuid, fobj = _make_folder() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_resolve_folder.return_value = fuid + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_existing.return_value = 'viewer' + mock_update.return_value = {'success': True} + + expiration = 1_900_000_000_000 + grant_folder_access_v3( + _make_params(nested_share_folders={fuid: fobj}), + fuid, email, role='viewer', expiration_timestamp=expiration) + + mock_update.assert_called_once_with( + mock.ANY, fuid, email, role='viewer', as_team=False, + expiration_timestamp=expiration) + + +class TestNestedShareFolderRecordApi(TestCase): + + def setUp(self): + from keepercommander.nested_share_folder import record_api # noqa: F401 + mock.patch('keepercommander.sync_down.sync_down').start() + + def tearDown(self): + mock.patch.stopall() + + @patch('keepercommander.nested_share_folder.record_api.api.communicate_rest') + @patch('keepercommander.nested_share_folder.record_api.encrypt_for_recipient') + @patch('keepercommander.nested_share_folder.record_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.record_api.get_record_from_cache') + def test_update_record_share_v3_sets_expiration_and_notification( + self, mock_get_record, mock_get_public_key, + mock_encrypt, mock_communicate): + from keepercommander.nested_share_folder.record_api import update_record_share_v3 + from keepercommander.proto import tla_pb2 + + ruid, robj = _make_record() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_get_record.return_value = robj + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_encrypt.return_value = b'enc-key' + mock_response = Mock() + mock_response.updatedSharingStatus = [] + mock_communicate.return_value = mock_response + + update_record_share_v3( + _make_params(nested_share_records={ruid: robj}), + ruid, email, access_role_type=1, expiration_timestamp=-1) + + rq = mock_communicate.call_args[0][1] + perm = rq.updateSharingPermissions[0] + self.assertEqual(perm.rules.tlaProperties.expiration, -1) + + @patch('keepercommander.nested_share_folder.record_api.api.communicate_rest') + @patch('keepercommander.nested_share_folder.record_api.encrypt_for_recipient') + @patch('keepercommander.nested_share_folder.record_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.record_api.get_record_from_cache') + def test_update_record_share_v3_recreate_when_setting_expiration( + self, mock_get_record, mock_get_public_key, + mock_encrypt, mock_communicate): + from keepercommander.nested_share_folder.record_api import update_record_share_v3 + from keepercommander.proto import tla_pb2 + + ruid, robj = _make_record() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_get_record.return_value = robj + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_encrypt.return_value = b'enc-key' + mock_response = Mock() + mock_response.revokedSharingStatus = [_make_sharing_status(ruid, uid_bytes)] + mock_response.createdSharingStatus = [_make_sharing_status(ruid, uid_bytes)] + mock_communicate.side_effect = [mock_response, mock_response] + + expiration = 1_900_000_000_000 + update_record_share_v3( + _make_params(nested_share_records={ruid: robj}), + ruid, email, access_role_type=1, expiration_timestamp=expiration) + + self.assertEqual(mock_communicate.call_count, 2) + revoke_rq = mock_communicate.call_args_list[0][0][1] + create_rq = mock_communicate.call_args_list[1][0][1] + self.assertEqual(len(revoke_rq.revokeSharingPermissions), 1) + self.assertEqual(len(revoke_rq.createSharingPermissions), 0) + self.assertEqual(len(create_rq.createSharingPermissions), 1) + self.assertEqual(len(create_rq.updateSharingPermissions), 0) + perm = create_rq.createSharingPermissions[0] + self.assertEqual(perm.rules.tlaProperties.expiration, expiration) + self.assertEqual(perm.rules.tlaProperties.timerNotificationType, tla_pb2.NOTIFY_OWNER) + + @patch('keepercommander.sync_down.sync_down') + @patch('keepercommander.nested_share_folder.record_api.api.communicate_rest') + @patch('keepercommander.nested_share_folder.record_api.encrypt_for_recipient') + @patch('keepercommander.nested_share_folder.record_api.get_user_public_key') + @patch('keepercommander.nested_share_folder.record_api.get_record_from_cache') + def test_update_record_share_v3_recreate_syncs_once( + self, mock_get_record, mock_get_public_key, + mock_encrypt, mock_communicate, mock_sync_down): + from keepercommander.nested_share_folder import record_api as ra + + ruid, robj = _make_record() + email = 'user@example.com' + uid_bytes = utils.base64_url_decode(utils.generate_uid()) + mock_get_record.return_value = robj + mock_get_public_key.return_value = (Mock(), False, uid_bytes, False) + mock_encrypt.return_value = b'enc-key' + mock_response = Mock() + mock_response.revokedSharingStatus = [_make_sharing_status(ruid, uid_bytes)] + mock_response.createdSharingStatus = [_make_sharing_status(ruid, uid_bytes)] + mock_communicate.side_effect = [mock_response, mock_response] + + ra.update_record_share_v3( + _make_params(nested_share_records={ruid: robj}), + ruid, email, access_role_type=1, expiration_timestamp=1_900_000_000_000) + + self.assertEqual(mock_sync_down.call_count, 1) + + def test_is_record_share_update_noop(self): + from keepercommander.nested_share_folder.record_api import is_record_share_update_noop + + existing = {'access_role_type': 2, 'tla_expiration': 1_900_000_000_000} + self.assertTrue(is_record_share_update_noop(existing, 2, None)) + self.assertTrue(is_record_share_update_noop( + existing, 2, 1_900_000_000_500)) + self.assertFalse(is_record_share_update_noop(existing, 4, None)) + self.assertFalse(is_record_share_update_noop( + existing, 2, 1_900_001_000_000)) + self.assertTrue(is_record_share_update_noop( + {'access_role_type': 2}, 2, -1)) + + @patch('keepercommander.nested_share_folder.record_api.api.communicate_rest') + def test_get_record_accesses_v3_reads_tla_expiration(self, mock_communicate): + from keepercommander.nested_share_folder.record_api import get_record_accesses_v3 + from keepercommander.proto import record_details_pb2, folder_pb2, tla_pb2 + + ruid = utils.generate_uid() + rs = record_details_pb2.RecordAccessResponse() + ra = rs.recordAccesses.add() + ra.data.recordUid = utils.base64_url_decode(ruid) + ra.data.accessTypeUid = b'\x01' * 16 + ra.data.accessType = folder_pb2.AT_USER + ra.data.accessRoleType = folder_pb2.VIEWER + ra.data.tlaProperties.expiration = 1_783_667_017_211 + ra.data.tlaProperties.timerNotificationType = tla_pb2.NOTIFY_OWNER + ra.accessorInfo.name = 'user@example.com' + mock_communicate.return_value = rs + + result = get_record_accesses_v3(_make_params(), [ruid]) + self.assertEqual(result['record_accesses'][0]['tla_expiration'], 1_783_667_017_211) + class TestNestedShareFolderDisplayCommands(TestCase): From 8eff5c52de29fd1345b042bd12ac2fa4425cbc52 Mon Sep 17 00:00:00 2001 From: amangalampalli-ks Date: Tue, 16 Jun 2026 11:35:35 +0530 Subject: [PATCH 06/11] Fix SQL injection in MSSQL password rotation (#2151) * Fix SQL injection in MSSQL password rotation and reject unsafe --password input * allow / and . in login regex --- keepercommander/plugins/commands.py | 38 ++++++++++++++ keepercommander/plugins/mssql/mssql.py | 18 ++++++- keepercommander/plugins/plugin_manager.py | 50 +++++++++++++++++-- .../service/util/verified_command.py | 31 +----------- tests/test_kc1163_record_field_labels.py | 23 +++++++++ 5 files changed, 126 insertions(+), 34 deletions(-) diff --git a/keepercommander/plugins/commands.py b/keepercommander/plugins/commands.py index a1ee7ec34..93b7a1f77 100644 --- a/keepercommander/plugins/commands.py +++ b/keepercommander/plugins/commands.py @@ -62,6 +62,42 @@ def register_command_info(aliases, command_info): rotate_parser.error = raise_parse_exception rotate_parser.exit = suppress_exit +UNSAFE_ROTATION_PASSWORD_PATTERN = re.compile(r"""[';"\\]|--""") +_UNSAFE_ROTATION_PASSWORD_LABELS = { + "'": "single quote (')", + '"': 'double quote (")', + ';': 'semicolon (;)', + '\\': 'backslash (\\)', + '--': 'double hyphen (--)', +} + + +def validate_user_supplied_rotation_password(new_password): + # type: (str) -> bool + matches = UNSAFE_ROTATION_PASSWORD_PATTERN.findall(new_password) + if not matches: + return True + + labels = [] + seen = set() + for match in matches: + if match in seen: + continue + seen.add(match) + labels.append(_UNSAFE_ROTATION_PASSWORD_LABELS.get(match, repr(match))) + + if len(labels) == 1: + logging.error( + 'Password contains character unsafe for database rotation: %s', + labels[0], + ) + else: + logging.error( + 'Password contains characters unsafe for database rotation: %s', + ', '.join(labels), + ) + return False + def adjust_password(password): # type: (str) -> str if not password: @@ -178,6 +214,8 @@ def rotate_password(params, record_uid, rotate_name=None, plugin_name=None, host if not length: length = plugin_kwargs.get('length') new_password = get_new_password(plugin, rules, length) + elif not validate_user_supplied_rotation_password(new_password): + return False if plugin_kwargs.get('password') == new_password: logging.warning('Rotation aborted because the old and new passwords are the same.') diff --git a/keepercommander/plugins/mssql/mssql.py b/keepercommander/plugins/mssql/mssql.py index 99ac18f19..e53e54e04 100644 --- a/keepercommander/plugins/mssql/mssql.py +++ b/keepercommander/plugins/mssql/mssql.py @@ -10,6 +10,8 @@ # Contact: ops@keepersecurity.com # import logging +import re + import pymssql """Commander Plugin for Microsoft SQL Server @@ -17,6 +19,13 @@ pip3 install pymssql """ +_LOGIN_NAME_PATTERN = re.compile(r'^[a-zA-Z_@#.][a-zA-Z0-9_@#.$\\-]*$') + +def _quoted_login_name(login): + if not login or not _LOGIN_NAME_PATTERN.match(login): + return None + return f'[{login.replace("]", "]]")}]' + class Rotator: def __init__(self, login, password, host=None, port=1433, db=None, **kwargs): @@ -47,6 +56,11 @@ def rotate(self, record, new_password, revert=False): old_password = self.password user = self.login + quoted_user = _quoted_login_name(user) + if not quoted_user: + logging.error('Invalid SQL Server login name for rotation: %s', user) + return False + kwargs = {'user': user, 'password': old_password} if self.host: kwargs['server'] = self.host @@ -60,8 +74,8 @@ def rotate(self, record, new_password, revert=False): with connection.cursor() as cursor: host = 'default host' if self.host is None else f'"{self.host}"' logging.debug(f'Connected to {host}') - sql = f"ALTER LOGIN {user} WITH PASSWORD = '{new_password}';" - cursor.execute(sql) + sql = f"ALTER LOGIN {quoted_user} WITH PASSWORD = %s" + cursor.execute(sql, (new_password,)) # connection is not autocommit by default. So you must commit to save your changes. connection.commit() result = True diff --git a/keepercommander/plugins/plugin_manager.py b/keepercommander/plugins/plugin_manager.py index 2fdedaed2..318010ee4 100644 --- a/keepercommander/plugins/plugin_manager.py +++ b/keepercommander/plugins/plugin_manager.py @@ -13,6 +13,7 @@ from urllib.parse import urlparse from . import noop +from ..vault import PasswordRecord, TypedField, TypedRecord CONVERT_KWARG_TO_INT = ('port', 'length') @@ -156,6 +157,51 @@ def get_custom_cmdr_fields(record): } +def get_rotation_record_kwargs(record): + """Extract login/password/url/host for rotation plugins from a vault record.""" + plugin_kwargs = {} + + if isinstance(record, PasswordRecord): + if record.login: + plugin_kwargs['login'] = record.login + if record.password: + plugin_kwargs['password'] = record.password + if record.link: + plugin_kwargs['url'] = record.link + elif isinstance(record, TypedRecord): + login_field = record.get_typed_field('login') + if login_field: + login = login_field.get_default_value(str) + if login: + plugin_kwargs['login'] = login + password_field = record.get_typed_field('password') + if password_field: + password = password_field.get_default_value(str) + if password: + plugin_kwargs['password'] = password + url_field = record.get_typed_field('url') + if url_field: + url = url_field.get_default_value(str) + if url: + plugin_kwargs['url'] = url + host_field = record.get_typed_field('host') + if host_field: + host_value = host_field.get_default_value(dict) + if host_value: + host = TypedField.export_host_field(host_value) + if host: + plugin_kwargs['host'] = host + + for key, value in record.enumerate_fields(): + if not value or key not in ('(login)', '(password)', '(url)', '(host)'): + continue + name = key[1:-1] + if name not in plugin_kwargs: + plugin_kwargs[name] = value + + return plugin_kwargs + + def get_plugin(record, rotate_name, plugin_name=None, host=None, port=None): """Load plugin based on given record and alt identifier @@ -171,9 +217,7 @@ def get_plugin(record, rotate_name, plugin_name=None, host=None, port=None): plugin_kwargs = {} plugin_kwargs.update(cmdr_kwargs) - plugin_kwargs.update({ - k[1:-1]: v for k, v in record.enumerate_fields() if k in ('(login)', '(password)', '(url)', '(host)') and v - }) + plugin_kwargs.update(get_rotation_record_kwargs(record)) if 'host' in plugin_kwargs: server = plugin_kwargs['host'] h, sep, p = server.partition(':') diff --git a/keepercommander/service/util/verified_command.py b/keepercommander/service/util/verified_command.py index 72c9c97ae..e622da0e9 100644 --- a/keepercommander/service/util/verified_command.py +++ b/keepercommander/service/util/verified_command.py @@ -56,18 +56,7 @@ def validate_mkdir_command(command): if missing_params: return f"Missing required parameters: {' and '.join(missing_params)}" return None - - # Legacy methods for backward compatibility - @staticmethod - def is_append_command(command): - """Legacy method - returns True if command is invalid.""" - return Verifycommand.validate_append_command(command) is not None - - @staticmethod - def is_mkdir_command(command): - """Legacy method - returns True if command is invalid.""" - return Verifycommand.validate_mkdir_command(command) is not None - + @staticmethod def validate_transform_folder_command(command): """ @@ -96,20 +85,4 @@ def validate_transform_folder_command(command): if missing_params: return f"Missing required parameters: {' and '.join(missing_params)}" - return None - - # Legacy methods for backward compatibility - @staticmethod - def is_append_command(command): - """Legacy method - returns True if command is invalid.""" - return Verifycommand.validate_append_command(command) is not None - - @staticmethod - def is_mkdir_command(command): - """Legacy method - returns True if command is invalid.""" - return Verifycommand.validate_mkdir_command(command) is not None - - @staticmethod - def is_transform_folder_command(command): - """Legacy method - returns True if command is invalid.""" - return Verifycommand.validate_transform_folder_command(command) is not None \ No newline at end of file + return None \ No newline at end of file diff --git a/tests/test_kc1163_record_field_labels.py b/tests/test_kc1163_record_field_labels.py index 0b30107d2..352f6470b 100644 --- a/tests/test_kc1163_record_field_labels.py +++ b/tests/test_kc1163_record_field_labels.py @@ -144,5 +144,28 @@ def test_old_code_produced_blank_labels(self): "Expected old code to produce blank labels (regression check)") +class TestPluginManagerLabeledFields(unittest.TestCase): + """Rotation plugins must read login/password from labeled typed fields (KC-1163).""" + + def test_get_rotation_record_kwargs_reads_labeled_login_password(self): + from keepercommander.plugins import plugin_manager + + record = vault.TypedRecord() + record.type_name = 'login' + record.fields.append(vault.TypedField.new_field('login', ['sa'], 'login')) + record.fields.append(vault.TypedField.new_field('password', ['Str0ng!Pass'], 'password')) + record.custom.append(vault.TypedField.new_field('text', ['mssql'], 'cmdr:plugin')) + record.custom.append(vault.TypedField.new_field('text', ['127.0.0.1'], 'cmdr:host')) + record.custom.append(vault.TypedField.new_field('text', ['1433'], 'cmdr:port')) + + plugin_name, plugin_kwargs, plugin = plugin_manager.get_plugin(record, rotate_name=None) + self.assertEqual(plugin_name, 'mssql') + self.assertEqual(plugin_kwargs['login'], 'sa') + self.assertEqual(plugin_kwargs['password'], 'Str0ng!Pass') + self.assertEqual(plugin_kwargs['host'], '127.0.0.1') + self.assertEqual(plugin_kwargs['port'], 1433) + self.assertIsNotNone(plugin) + + if __name__ == '__main__': unittest.main() From a2e3ef838ac63624f9df6ea5530f7e58b7331995 Mon Sep 17 00:00:00 2001 From: idimov-keeper <78815270+idimov-keeper@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:15:28 -0500 Subject: [PATCH 07/11] Fix IAM user link permission-check rotation request shape. (#2159) KeeperApp and krouter require configurationUid on set_record_rotation; include PAM config UID, matching revision for existing rotation rows, and an explicit empty resourceUid so IAM semantics are not overridden by stale cache data. Co-authored-by: Cursor --- .../tunnel/port_forward/TunnelGraph.py | 10 ++++ unit-tests/pam/test_dag_layer_b_migration.py | 51 +++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/keepercommander/commands/tunnel/port_forward/TunnelGraph.py b/keepercommander/commands/tunnel/port_forward/TunnelGraph.py index 7ac683d1c..4fb530dc7 100644 --- a/keepercommander/commands/tunnel/port_forward/TunnelGraph.py +++ b/keepercommander/commands/tunnel/port_forward/TunnelGraph.py @@ -379,8 +379,18 @@ def _permission_check_iam_user_link(self, user_uid): server enforces edit-access on the pamUser record at the call boundary, so the per-flag check that other Layer-B endpoints do is not needed here.""" from ...pam.router_helper import router_set_record_rotation_information + current_record_rotation = self.params.record_rotation_cache.get(user_uid) + revision = ( + current_record_rotation.get('revision', 0) + if current_record_rotation else 0 + ) + # IAM link: resourceUid must stay empty so krouter sets isIAM=true + # (resourceUid.isEmpty && noop=False). Never copy resource_uid from cache. rq = router_pb2.RouterRecordRotationRequest( recordUid=url_safe_str_to_bytes(user_uid), + configurationUid=url_safe_str_to_bytes(self.record.record_uid), + revision=revision, + resourceUid=b'', noop=False, ) router_set_record_rotation_information(self.params, rq) diff --git a/unit-tests/pam/test_dag_layer_b_migration.py b/unit-tests/pam/test_dag_layer_b_migration.py index e77c4ee9e..b7be169d1 100644 --- a/unit-tests/pam/test_dag_layer_b_migration.py +++ b/unit-tests/pam/test_dag_layer_b_migration.py @@ -22,6 +22,7 @@ from unittest.mock import MagicMock, patch import pytest +from keeper_secrets_manager_core.utils import url_safe_str_to_bytes sys.path.insert(0, os.path.dirname(__file__)) @@ -697,6 +698,7 @@ def _build_tg(): tg = TunnelDAG.__new__(TunnelDAG) tg.params = MagicMock() + tg.params.record_rotation_cache = {} tg.record = MagicMock() tg.record.record_uid = CONFIG_UID_STR tg.linking_dag = MagicMock() @@ -723,6 +725,7 @@ def _capture(params, rq, *args, **kwargs): assert isinstance(rq, router_pb2.RouterRecordRotationRequest) # recordUid is the pamUser record (the target of the permission check) assert len(rq.recordUid) == 16 + assert rq.configurationUid == url_safe_str_to_bytes(CONFIG_UID_STR) # noop=False is what triggers the is_iam_user write server-side assert rq.noop is False # NO resourceUid, NO saasConfiguration — these would change the semantics @@ -731,6 +734,52 @@ def _capture(params, rq, *args, **kwargs): # After permission check succeeds, the legacy link_user mutation still runs link_user_mock.assert_called_once() + def test_link_user_to_config_passes_revision_from_rotation_cache(self): + """When pamUser already has rotation metadata, KeeperApp requires matching revision.""" + tg, _ = self._build_tg() + tg.params.record_rotation_cache = { + USER_UID_STR: {'revision': 3, 'configuration_uid': CONFIG_UID_STR}, + } + captured = {} + + def _capture(params, rq, *args, **kwargs): + captured['rq'] = rq + + with patch('keepercommander.commands.pam.router_helper.router_set_record_rotation_information', + side_effect=_capture), \ + patch.object(tg, 'link_user'): + tg.link_user_to_config(USER_UID_STR) + + rq = captured.get('rq') + assert rq is not None + assert rq.revision == 3 + assert rq.resourceUid == b'' + + def test_link_user_to_config_clears_stale_resource_uid_from_rotation_cache(self): + """IAM permission-check must not send cached resource_uid (non-IAM semantics).""" + tg, _ = self._build_tg() + tg.params.record_rotation_cache = { + USER_UID_STR: { + 'revision': 2, + 'configuration_uid': CONFIG_UID_STR, + 'resource_uid': 'AAAAAAAAAAAAAAAAAAAAAA', + }, + } + captured = {} + + def _capture(params, rq, *args, **kwargs): + captured['rq'] = rq + + with patch('keepercommander.commands.pam.router_helper.router_set_record_rotation_information', + side_effect=_capture), \ + patch.object(tg, 'link_user'): + tg.link_user_to_config(USER_UID_STR) + + rq = captured.get('rq') + assert rq is not None + assert rq.revision == 2 + assert rq.resourceUid == b'' + def test_link_user_to_config_no_fallback_on_permission_denial(self): """If set_record_rotation fails (permission denied), DO NOT fall back to the legacy DAG write - propagate so the unauthorized link is never @@ -770,6 +819,8 @@ def _capture(params, rq, *args, **kwargs): rq = captured.get('rq') assert rq is not None assert isinstance(rq, router_pb2.RouterRecordRotationRequest) + assert rq.configurationUid == url_safe_str_to_bytes(CONFIG_UID_STR) + assert rq.resourceUid == b'' assert rq.noop is False def test_link_user_to_config_with_options_iam_user_not_true_skips_set_rotation(self): From 63de90e85cce6390129e61a890245c8cbde1e420 Mon Sep 17 00:00:00 2001 From: idimov-keeper <78815270+idimov-keeper@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:59:10 -0500 Subject: [PATCH 08/11] Add --online filter to pam gateway list command (#2158) * Add --online filter to pam gateway list with gateway totals. Co-authored-by: Cursor * Add -o short option for pam gateway list --online. Co-authored-by: Cursor --------- Co-authored-by: Cursor --- keepercommander/commands/discoveryrotation.py | 27 ++++++++ unit-tests/pam/test_pam_rotation.py | 62 +++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/keepercommander/commands/discoveryrotation.py b/keepercommander/commands/discoveryrotation.py index d8e5b3b57..d8797f141 100644 --- a/keepercommander/commands/discoveryrotation.py +++ b/keepercommander/commands/discoveryrotation.py @@ -1399,6 +1399,8 @@ class PAMGatewayListCommand(Command): help='Verbose output') parser.add_argument('--format', dest='format', action='store', choices=['table', 'json'], default='table', help='Output format (table, json)') + parser.add_argument('--online', '-o', required=False, default=False, dest='online_only', action='store_true', + help='Show only online gateways') def get_parser(self): return PAMGatewayListCommand.parser @@ -1408,6 +1410,7 @@ def execute(self, params, **kwargs): is_force = kwargs.get('is_force') is_verbose = kwargs.get('is_verbose') format_type = kwargs.get('format', 'table') + online_only = kwargs.get('online_only', False) is_router_down = False krouter_url = router_helper.get_router_url(params) @@ -1478,6 +1481,8 @@ def execute(self, params, **kwargs): connected_controllers_dict[controller.controllerUid] = [] connected_controllers_dict[controller.controllerUid].append(controller) + gateway_counts = {'online': 0, 'offline': 0, 'total': len(enterprise_controllers_all)} + # Process each gateway and handle multiple instances for c in enterprise_controllers_all: gateway_uid_bytes = c.controllerUid @@ -1485,6 +1490,17 @@ def execute(self, params, **kwargs): connected_instances = connected_controllers_dict.get(gateway_uid_bytes, []) + is_online = not is_router_down and len(connected_instances) > 0 + if is_router_down: + pass + elif is_online: + gateway_counts['online'] += 1 + else: + gateway_counts['offline'] += 1 + + if online_only and not is_online: + continue + ksm_app_uid_str = utils.base64_url_encode(c.applicationUid) ksm_app = KSMCommand.get_app_record(params, ksm_app_uid_str) @@ -1714,6 +1730,9 @@ def execute(self, params, **kwargs): else: result = {"gateways": gateways_data} + if online_only: + result["gateway_counts"] = gateway_counts + return json.dumps(result, indent=2) else: # Separate rows into groups: each parent with its instances @@ -1746,6 +1765,14 @@ def execute(self, params, **kwargs): dump_report_data(table, headers, fmt='table', filename="", row_number=False, column_width=None) + if online_only: + print( + f"\n{bcolors.BOLD}Gateways: " + f"{bcolors.OKGREEN}Online: {gateway_counts['online']}{bcolors.ENDC}, " + f"{bcolors.FAIL}Offline: {gateway_counts['offline']}{bcolors.ENDC}, " + f"Total: {gateway_counts['total']}{bcolors.ENDC}\n" + ) + class PAMConfigurationListCommand(Command): parser = argparse.ArgumentParser(prog='pam config list') diff --git a/unit-tests/pam/test_pam_rotation.py b/unit-tests/pam/test_pam_rotation.py index f7fce197a..09b96d6db 100644 --- a/unit-tests/pam/test_pam_rotation.py +++ b/unit-tests/pam/test_pam_rotation.py @@ -416,6 +416,13 @@ def test_parser(self): self.assertTrue(args.is_verbose) self.assertTrue(args.is_force) + def test_parser_online(self): + args = self.parser.parse_args(['--online']) + self.assertTrue(args.online_only) + + args = self.parser.parse_args(['-o']) + self.assertTrue(args.online_only) + @patch('keepercommander.commands.discoveryrotation.router_get_connected_gateways') @patch('keepercommander.commands.discoveryrotation.router_helper.get_router_url') @patch('keepercommander.commands.discoveryrotation.gateway_helper.get_all_gateways') @@ -488,6 +495,61 @@ def test_execute_router_down(self, mock_dump_report_data, mock_get_all_gateways, self.assertTrue(mock_get_all_gateways.called) self.assertTrue(mock_get_router_url.called) + @patch('keepercommander.commands.discoveryrotation.print') + @patch('keepercommander.commands.discoveryrotation.router_get_connected_gateways') + @patch('keepercommander.commands.discoveryrotation.router_helper.get_router_url') + @patch('keepercommander.commands.discoveryrotation.gateway_helper.get_all_gateways') + @patch('keepercommander.commands.discoveryrotation.KSMCommand.get_app_record') + @patch('keepercommander.commands.discoveryrotation.dump_report_data') + def test_execute_online_only(self, mock_dump_report_data, mock_get_app_record, mock_get_all_gateways, + mock_get_router_url, mock_router_get_connected_gateways, mock_print): + mock_params = create_mock_params() + + online_uid = utils.base64_url_decode('controller_uid') + offline_uid = utils.base64_url_decode('offline_uid') + + mock_router_get_connected_gateways.return_value.controllers = [ + MagicMock(controllerUid=online_uid, version='1.0.0') + ] + + mock_get_all_gateways.return_value = [ + MagicMock( + applicationUid=utils.base64_url_decode('app_uid'), + controllerUid=online_uid, + controllerName='Online Gateway', + deviceName='Device 1', + deviceToken='Token 1', + created=int(datetime.now().timestamp() * 1000), + lastModified=int(datetime.now().timestamp() * 1000), + nodeId='Node 1' + ), + MagicMock( + applicationUid=utils.base64_url_decode('app_uid2'), + controllerUid=offline_uid, + controllerName='Offline Gateway', + deviceName='Device 2', + deviceToken='Token 2', + created=int(datetime.now().timestamp() * 1000), + lastModified=int(datetime.now().timestamp() * 1000), + nodeId='Node 2' + ) + ] + + mock_get_app_record.return_value = { + 'data_unencrypted': json.dumps({'title': 'App Title'}) + } + + kwargs = {'is_force': True, 'online_only': True} + self.command.execute(mock_params, **kwargs) + + self.assertTrue(mock_dump_report_data.called) + table = mock_dump_report_data.call_args[0][0] + self.assertEqual(len(table), 1) + self.assertIn('Online Gateway', table[0][1]) + + totals_printed = any('Online: 1' in str(call.args[0]) for call in mock_print.call_args_list) + self.assertTrue(totals_printed) + @patch('keepercommander.commands.discoveryrotation.router_get_connected_gateways') @patch('keepercommander.commands.discoveryrotation.router_helper.get_router_url') @patch('keepercommander.commands.discoveryrotation.gateway_helper.get_all_gateways') From 2a94cbbf3881447c295257149a02f54d4f8e0be2 Mon Sep 17 00:00:00 2001 From: lthievenaz-keeper Date: Wed, 17 Jun 2026 18:59:45 +0100 Subject: [PATCH 09/11] Add secret-ids arg in Thycotic import (#2150) * Add secret-ids argument for thycotic import Add secret-ids arg to import command, to use for debugging Thycotic secret IDs * Pass secret-ids arg from import command to thycotic import Pass secret-ids arg from import command to thycotic import so it can be handled in the Thycotic import * Add handling for secret-ids arg in Thycotic import If user sets secret-ids, the import will: - Check if any of those IDs have come up in the lookup and would have been imported. - Import only the secret-ids set This is useful for debugging, because the lookup API may not return all Thycotic secrets - eg if there a security policy on them, but they may still be fetched. Usage: String (comma separated IDs) `import --format thycotic server_name --secret-ids "123, 124,125"` Python List (strings or integers) `secret_ids=[123, 124, 125]` --- keepercommander/importer/thycotic/thycotic.py | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/keepercommander/importer/thycotic/thycotic.py b/keepercommander/importer/thycotic/thycotic.py index f7409609c..c5359dad8 100644 --- a/keepercommander/importer/thycotic/thycotic.py +++ b/keepercommander/importer/thycotic/thycotic.py @@ -366,6 +366,30 @@ def do_import(self, filename, **kwargs): logging.warning('Invalid input for secret IDs, exiting.') return + # secret_ids arg + debug_ids = kwargs.get('secret_ids') + if debug_ids is not None: + # Convert CLI string input into list + if isinstance(debug_ids,str): + debug_ids = debug_ids.replace(' ','').split(',') + # Handle secret IDs list + if isinstance(debug_ids,list): + # Deduplicate + debug_ids = list(set(debug_ids)) + # Check whether secrets were found in the lookup + stringified_secrets_ids = [str(x) for x in secrets_ids] + stringified_debug_ids = [str(x) for x in debug_ids] + found_secrets = [x for x in stringified_debug_ids if x in stringified_secrets_ids] + logging.info(f'From the specified {len(debug_ids)} secret IDs, {len(found_secrets)} were found in the secret server lookup.') + logging.info(', '.join(found_secrets)) + + # Replace import list with specified IDs + secrets_ids = debug_ids + else: + # Quit if secret IDs != list + logging.warning('Invalid input for secret IDs, exiting.') + return + self._send_keep_alive_if_needed(params) print(f'Loading {len(secrets_ids)} Records ', flush=True, end='') secrets = [] From dc889d038ee94ca24493d259395ac15b2f5c6227 Mon Sep 17 00:00:00 2001 From: idimov-keeper <78815270+idimov-keeper@users.noreply.github.com> Date: Wed, 17 Jun 2026 23:09:53 -0500 Subject: [PATCH 10/11] Add pam connection ai command for KeeperAI settings on PAM resources. (#2160) * Add pam connection ai command for KeeperAI settings on PAM resources. Implements show, set/unset, and remove with sparse DAG merges, configure_resource meta bootstrap, GSE_DELETION removal, and CLI warnings for duplicate or mirrored options. Co-authored-by: Cursor * Extend pam connection ai to pamRemoteBrowser records. Co-authored-by: Cursor --------- Co-authored-by: Cursor --- .../commands/pam_import/keeper_ai_settings.py | 530 +++++++++++++++++- .../commands/tunnel_and_connections.py | 239 ++++++++ unit-tests/pam/test_dag_layer_b_migration.py | 29 +- unit-tests/pam/test_pam_connection_ai.py | 288 ++++++++++ 4 files changed, 1057 insertions(+), 29 deletions(-) create mode 100644 unit-tests/pam/test_pam_connection_ai.py diff --git a/keepercommander/commands/pam_import/keeper_ai_settings.py b/keepercommander/commands/pam_import/keeper_ai_settings.py index 7d4056c12..9a1bc1df2 100644 --- a/keepercommander/commands/pam_import/keeper_ai_settings.py +++ b/keepercommander/commands/pam_import/keeper_ai_settings.py @@ -21,11 +21,64 @@ from ... import vault from ...display import bcolors from ...proto import pam_pb2 +from ... import utils from ..pam._layer_b import should_fallback_on_layer_b_error, is_layer_b_feature_disabled from ..tunnel.port_forward.tunnel_helpers import get_config_uid, get_keeper_tokens from ...keeper_dag.crypto import encrypt_aes from keeper_secrets_manager_core.utils import url_safe_str_to_bytes +AI_RISK_LEVELS = ('critical', 'high', 'medium', 'low') +AI_SETTINGS_VERSION = 'v1.0.0' + + +def empty_keeper_ai_settings_dict() -> Dict[str, Any]: + """Vault-compatible default KeeperAI settings (``emptyKeeperAISettings`` in dag-pam-link.ts).""" + empty_allow_deny = {'allow': [], 'deny': []} + return { + 'version': AI_SETTINGS_VERSION, + 'riskLevels': { + 'critical': {'aiSessionTerminate': False, 'tags': dict(empty_allow_deny)}, + 'high': {'aiSessionTerminate': False, 'tags': dict(empty_allow_deny)}, + 'medium': {'aiSessionTerminate': False, 'tags': dict(empty_allow_deny)}, + 'low': {'aiSessionTerminate': False, 'tags': {'allow': []}}, + }, + } + + +def is_default_keeper_ai_settings(settings: Optional[Dict[str, Any]]) -> bool: + """True when settings are absent or match the vault empty/default template.""" + if not settings: + return True + risk_levels = settings.get('riskLevels') + if not isinstance(risk_levels, dict) or not risk_levels: + return True + for level, level_data in risk_levels.items(): + if not isinstance(level_data, dict): + return False + if level_data.get('aiSessionTerminate', False): + return False + tags = level_data.get('tags') + if not isinstance(tags, dict): + continue + if tags.get('allow'): + return False + if level != 'low' and tags.get('deny'): + return False + return True + + +def _find_highest_path_edge(vertex, head_uid: str, dag_path: str): + """Return the highest-version edge for a self-loop DATA path (any edge type).""" + best = None + best_version = -1 + for edge in vertex.edges or []: + if not edge or edge.head_uid != head_uid or edge.path != dag_path: + continue + if edge.version > best_version: + best_version = edge.version + best = edge + return best + def list_resource_data_edges( params: KeeperParams, @@ -129,7 +182,8 @@ def get_resource_settings( params: KeeperParams, resource_uid: str, dag_path: str, - config_uid: Optional[str] = None + config_uid: Optional[str] = None, + quiet_if_missing_vertex: bool = False, ) -> Optional[Dict[str, Any]]: """ Generic function to retrieve settings from a DAG DATA edge with the specified path for a resource. @@ -143,9 +197,11 @@ def get_resource_settings( Args: params: KeeperParams instance - resource_uid: UID of the PAM resource (pamMachine, pamDatabase, etc.) + resource_uid: UID of the PAM resource (pamMachine, pamDatabase, pamDirectory, pamRemoteBrowser) dag_path: Path of the DATA edge (e.g., 'ai_settings', 'jit_settings') config_uid: Optional PAM config UID. If not provided, will be looked up. + quiet_if_missing_vertex: Log at debug instead of warning when the resource + vertex is absent (expected before first link to a PAM Configuration). Returns: Dictionary containing settings if found, None otherwise. @@ -209,21 +265,17 @@ def get_resource_settings( # Get the resource vertex resource_vertex = linking_dag.get_vertex_by_uid(resource_uid) if not resource_vertex: - logging.warning(f"Resource vertex {resource_uid} not found in DAG") + log = logging.debug if quiet_if_missing_vertex else logging.warning + log(f"Resource vertex {resource_uid} not found in DAG") return None - # Find the DATA edge with the specified path (get highest version, regardless of active status) - settings_edge = None - highest_version = -1 - for edge in resource_vertex.edges: - if edge and (edge.edge_type == EdgeType.DATA and - edge.path == dag_path): - if edge.version > highest_version: - highest_version = edge.version - settings_edge = edge - - if not settings_edge: - logging.debug(f"No '{dag_path}' DATA edge found for resource {resource_uid}") + # Highest-version edge for this path; DELETION means "no settings" (vault GSE_DELETION). + settings_edge = _find_highest_path_edge(resource_vertex, resource_uid, dag_path) + if settings_edge is None or settings_edge.edge_type == EdgeType.DELETION: + logging.debug(f"No active '{dag_path}' DATA edge for resource {resource_uid}") + return None + if settings_edge.edge_type != EdgeType.DATA: + logging.debug(f"Latest '{dag_path}' edge is not DATA for resource {resource_uid}") return None # Get the content from the edge @@ -340,7 +392,7 @@ def get_resource_jit_settings( Args: params: KeeperParams instance - resource_uid: UID of the PAM resource (pamMachine, pamDatabase, etc.) + resource_uid: UID of the PAM resource (pamMachine, pamDatabase, pamDirectory, pamRemoteBrowser) config_uid: Optional PAM config UID. If not provided, will be looked up. Returns: @@ -352,7 +404,8 @@ def get_resource_jit_settings( def get_resource_keeper_ai_settings( params: KeeperParams, resource_uid: str, - config_uid: Optional[str] = None + config_uid: Optional[str] = None, + quiet_if_missing_vertex: bool = False, ) -> Optional[Dict[str, Any]]: """ Retrieve KeeperAI settings from the DAG DATA edge with path 'ai_settings' for a resource. @@ -366,7 +419,7 @@ def get_resource_keeper_ai_settings( Args: params: KeeperParams instance - resource_uid: UID of the PAM resource (pamMachine, pamDatabase, etc.) + resource_uid: UID of the PAM resource (pamMachine, pamDatabase, pamDirectory, pamRemoteBrowser) config_uid: Optional PAM config UID. If not provided, will be looked up. Returns: @@ -393,7 +446,10 @@ def get_resource_keeper_ai_settings( } Returns None if settings not found or error occurred. """ - return get_resource_settings(params, resource_uid, 'ai_settings', config_uid) + return get_resource_settings( + params, resource_uid, 'ai_settings', config_uid, + quiet_if_missing_vertex=quiet_if_missing_vertex, + ) def set_resource_keeper_ai_settings( @@ -418,7 +474,7 @@ def set_resource_keeper_ai_settings( Args: params: KeeperParams instance - resource_uid: UID of the PAM resource (pamMachine, pamDatabase, etc.) + resource_uid: UID of the PAM resource (pamMachine, pamDatabase, pamDirectory, pamRemoteBrowser) settings: Dictionary containing KeeperAI settings to save config_uid: Optional PAM config UID. If not provided, will be looked up. @@ -431,6 +487,10 @@ def set_resource_keeper_ai_settings( return False record_key, resolved_config_uid = common + if not settings: + logging.debug(f"KeeperAI settings empty for {resource_uid}, skipping save") + return False + encrypted_content = encrypt_aes(json.dumps(settings).encode(), record_key) # krouter's configure_resource only writes a settings edge when it loads the @@ -438,11 +498,15 @@ def set_resource_keeper_ai_settings( # carry meta/jit/connection (UserRest.kt). A keeperAiSettings-only request # leaves loopEdges null and the ai_settings write is silently dropped. The Web # Vault avoids this by always sending meta alongside the AI settings, so mirror - # that: include the resource's current meta in the same request. - meta_bytes = None - current_meta = get_resource_settings(params, resource_uid, 'meta', resolved_config_uid) - if isinstance(current_meta, dict): - meta_bytes = json.dumps(current_meta).encode() + # that: include the resource's current meta in the same request. When the + # resource is not in the DAG yet (first link), bootstrap v1 meta instead. + from ..tunnel.port_forward.TunnelGraph import build_resource_meta_v1 + + current_meta = get_resource_settings( + params, resource_uid, 'meta', resolved_config_uid, quiet_if_missing_vertex=True) + if not isinstance(current_meta, dict): + current_meta = build_resource_meta_v1({}, False) + meta_bytes = json.dumps(current_meta).encode() # Primary: Layer-B configure_resource (permission-checked). from ..pam.router_helper import router_configure_resource, get_router_url @@ -453,9 +517,8 @@ def set_resource_keeper_ai_settings( recordUid=url_safe_str_to_bytes(resource_uid), networkUid=url_safe_str_to_bytes(resolved_config_uid), keeperAiSettings=encrypted_content, + meta=meta_bytes, ) - if meta_bytes is not None: - rq.meta = meta_bytes try: router_configure_resource(params, rq) logging.debug(f"Saved KeeperAI settings via configure_resource for {resource_uid}") @@ -579,6 +642,73 @@ def _set_resource_keeper_ai_settings_legacy( return False +def _delete_resource_data_edge_legacy( + params: KeeperParams, + resource_uid: str, + config_uid: str, + record_key: bytes, + dag_path: str, +) -> Optional[bool]: + """Delete a path-scoped self-loop DATA edge via GSE_DELETION (vault ``createDeletionEvent``). + + Returns: + True if a DELETION edge was written, + None if the path was already absent, + False on error. + """ + try: + encrypted_session_token, encrypted_transmission_key, transmission_key = get_keeper_tokens(params) + + dag_record = PasswordRecord() + dag_record.record_uid = config_uid + dag_record.record_key = record_key + + conn = Connection( + params=params, + encrypted_transmission_key=encrypted_transmission_key, + encrypted_session_token=encrypted_session_token, + transmission_key=transmission_key, + use_read_protobuf=False, + use_write_protobuf=False, + ) + + linking_dag = DAG( + conn=conn, + record=dag_record, + graph_id=PamGraphId.PAM.value, + decrypt=True, + ) + linking_dag.load() + + resource_vertex = linking_dag.get_vertex_by_uid(resource_uid) + if not resource_vertex: + logging.debug(f"No resource vertex {resource_uid} in DAG; '{dag_path}' already absent") + return None + + highest = _find_highest_path_edge(resource_vertex, resource_uid, dag_path) + if highest is None or highest.edge_type == EdgeType.DELETION: + logging.debug(f"'{dag_path}' already deleted for resource {resource_uid}") + return None + + for edge in resource_vertex.edges or []: + if (edge and edge.edge_type == EdgeType.DATA and edge.path == dag_path + and edge.head_uid == resource_uid and edge.active): + edge.active = False + + resource_vertex.belongs_to( + resource_vertex, + EdgeType.DELETION, + path=dag_path, + ) + linking_dag.save() + + logging.debug(f"Deleted '{dag_path}' via DELETION edge for resource {resource_uid}") + return True + except Exception as e: + logging.error(f"Error deleting '{dag_path}' for {resource_uid}: {e}", exc_info=True) + return False + + def set_resource_jit_settings( params: KeeperParams, resource_uid: str, @@ -824,6 +954,350 @@ def refresh_link_to_config_to_latest( return False +def _get_audit_user_id(params: KeeperParams) -> str: + if getattr(params, 'account_uid_bytes', None): + return utils.base64_url_encode(params.account_uid_bytes) + return getattr(params, 'user', '') or '' + + +def _make_tag_entry(tag: str, action: str, user_id: str) -> Dict[str, Any]: + return { + 'tag': tag, + 'auditLog': [{ + 'date': utils.current_milli_time(), + 'userId': user_id, + 'action': action, + }], + } + + +def _parse_tag_name(tag_item: Any) -> str: + if isinstance(tag_item, dict): + return str(tag_item.get('tag', '')).strip() + return str(tag_item).strip() + + +def _get_tag_list(level_data: Dict[str, Any], list_name: str) -> List[Dict[str, Any]]: + tags = level_data.setdefault('tags', {}) + entries = tags.get(list_name) + if not isinstance(entries, list): + entries = [] + tags[list_name] = entries + return entries + + +def parse_ai_setting_spec(spec: str) -> tuple: + """ + Parse CLI spec ``LEVEL``, ``LEVEL.SETTING``, or ``LEVEL.SETTING=VALUE``. + + Returns (level, setting_or_none, value_or_none). ``value_or_none`` is None when + ``=`` is absent (unset-without-value forms). + """ + if not spec or not str(spec).strip(): + raise ValueError('empty AI setting spec') + + text = str(spec).strip() + level_part, sep, value_part = text.partition('=') + if sep and value_part == '': + raise ValueError( + f'invalid AI setting spec (missing value): {spec}. ' + f'To remove a setting, use --unset|-u (e.g. -u high.terminate or -u high.allow=chmod).' + ) + + if '.' in level_part: + level_name, setting_name = level_part.split('.', 1) + else: + level_name, setting_name = level_part, None + + level_name = level_name.strip().lower() + if level_name not in AI_RISK_LEVELS: + raise ValueError(f'invalid risk level "{level_name}" (expected: {", ".join(AI_RISK_LEVELS)})') + + if setting_name is not None: + setting_name = setting_name.strip().lower() + if setting_name not in ('terminate', 'allow', 'deny'): + raise ValueError(f'invalid setting "{setting_name}" (expected: terminate, allow, deny)') + if level_name == 'low' and setting_name == 'deny': + raise ValueError('deny is not supported for the low risk level') + + value = value_part if sep else None + return level_name, setting_name, value + + +def dedupe_ai_cli_option_specs( + specs: Optional[List[str]], + option_label: str, +) -> tuple: + """Return unique specs in first-seen order and warnings for duplicate CLI options. + + ``option_label`` is shown in warnings (e.g. ``--set/-s``). + """ + if not specs: + return [], [] + + order: List[str] = [] + counts: Dict[str, int] = {} + for spec in specs: + counts[spec] = counts.get(spec, 0) + 1 + if spec not in order: + order.append(spec) + + warnings = [ + f'duplicate {option_label} ignored: {spec} ({counts[spec]}x)' + for spec in order + if counts[spec] > 1 + ] + return order, warnings + + +def _is_full_ai_setting_spec(spec: str) -> bool: + """True when spec is ``LEVEL.SETTING=VALUE`` (value present after ``=``).""" + _, setting, value = parse_ai_setting_spec(spec) + return setting is not None and value is not None + + +def _reconcile_set_unset_specs( + unset_specs: Optional[List[str]], + set_specs: Optional[List[str]], +) -> tuple: + """Drop ``--unset`` specs mirrored by a ``--set`` on the same ``LEVEL.SETTING=VALUE``. + + Only applies when both sides use the full ``level.setting=value`` form. Partial + unsets (e.g. ``-u high.allow`` or ``-u high.terminate``) are not reconciled. + """ + unset_list = list(unset_specs or []) + set_list = list(set_specs or []) + warnings: List[str] = [] + if not unset_list or not set_list: + return unset_list, set_list, warnings + + parsed_sets = [parse_ai_setting_spec(spec) for spec in set_list] + drop_unset_specs = set() + + for u_spec in unset_list: + if not _is_full_ai_setting_spec(u_spec): + continue + u_level, u_setting, _u_value = parse_ai_setting_spec(u_spec) + + for set_spec, (s_level, s_setting, _s_value) in zip(set_list, parsed_sets): + if u_level != s_level or u_setting != s_setting: + continue + if not _is_full_ai_setting_spec(set_spec): + continue + drop_unset_specs.add(u_spec) + if u_spec == set_spec: + warnings.append(f'--set/-s overrides --unset/-u: {u_spec}') + else: + warnings.append( + f'--set/-s overrides --unset/-u: {u_spec} (mirrors -s {set_spec})' + ) + break + + return [spec for spec in unset_list if spec not in drop_unset_specs], set_list, warnings + + +_LOW_TERMINATE_WARNING = 'risk level low.terminate always defaults to false.' + + +def _parse_terminate_value(value: str) -> bool: + normalized = str(value).strip().casefold() + if normalized == 'true': + return True + if normalized == 'false': + return False + raise ValueError(f'invalid terminate value "{value}" (expected true or false)') + + +def _existing_terminate_value(risk_levels: Dict[str, Any], level: str) -> Optional[bool]: + level_data = risk_levels.get(level) + if not isinstance(level_data, dict): + return None + value = level_data.get('aiSessionTerminate') + if value is None: + return None + return bool(value) + + +def _ensure_ai_settings_dict(existing: Optional[Dict[str, Any]]) -> Dict[str, Any]: + settings = dict(existing) if isinstance(existing, dict) else {} + settings['version'] = settings.get('version') or AI_SETTINGS_VERSION + risk_levels = settings.get('riskLevels') + if not isinstance(risk_levels, dict): + risk_levels = {} + settings['riskLevels'] = risk_levels + return settings + + +def _ensure_level_dict(risk_levels: Dict[str, Any], level: str) -> Dict[str, Any]: + level_data = risk_levels.get(level) + if not isinstance(level_data, dict): + level_data = {} + risk_levels[level] = level_data + return level_data + + +def _prune_level(level_data: Dict[str, Any]) -> bool: + tags = level_data.get('tags') + if isinstance(tags, dict): + for key in list(tags.keys()): + entries = tags.get(key) + if not entries: + tags.pop(key, None) + if not tags: + level_data.pop('tags', None) + return not level_data + + +def _level_is_only_default_low(level_data: Dict[str, Any]) -> bool: + if not isinstance(level_data, dict): + return True + tags = level_data.get('tags') + has_tags = isinstance(tags, dict) and any(tags.get(k) for k in tags) + if has_tags: + return False + terminate = level_data.get('aiSessionTerminate') + return terminate is None or terminate is False + + +def _normalize_ai_settings_for_save(settings: Dict[str, Any]) -> Dict[str, Any]: + risk_levels = settings.get('riskLevels') + if not isinstance(risk_levels, dict): + return {} + + for level in list(risk_levels.keys()): + if level not in AI_RISK_LEVELS: + risk_levels.pop(level, None) + continue + if _prune_level(risk_levels[level]): + risk_levels.pop(level, None) + + low_data = risk_levels.get('low') + if isinstance(low_data, dict): + low_data['aiSessionTerminate'] = False + tags = low_data.get('tags') + if isinstance(tags, dict): + tags.pop('deny', None) + if not tags: + low_data.pop('tags', None) + if _prune_level(low_data) or _level_is_only_default_low(low_data): + risk_levels.pop('low', None) + + if not risk_levels: + return {} + + return { + 'version': settings.get('version', AI_SETTINGS_VERSION), + 'riskLevels': risk_levels, + } + + +def apply_ai_setting_changes( + existing: Optional[Dict[str, Any]], + set_specs: Optional[List[str]], + unset_specs: Optional[List[str]], + params: KeeperParams, +) -> tuple: + """Merge CLI --set/--unset operations into KeeperAI DAG settings. + + Returns ``(settings_dict, warnings)`` where ``warnings`` is a list of user-facing + messages (e.g. silent conversions applied during save). + """ + settings = _ensure_ai_settings_dict(existing) + risk_levels = settings['riskLevels'] + user_id = _get_audit_user_id(params) + warnings: List[str] = [] + + unset_specs, set_specs, reconcile_warnings = _reconcile_set_unset_specs(unset_specs, set_specs) + warnings.extend(reconcile_warnings) + + for spec in unset_specs or []: + level, setting, value = parse_ai_setting_spec(spec) + if setting is None: + risk_levels.pop(level, None) + continue + + level_data = risk_levels.get(level) + if not isinstance(level_data, dict): + continue + + if setting == 'terminate': + level_data.pop('aiSessionTerminate', None) + else: + tags = level_data.get('tags') + if not isinstance(tags, dict): + continue + entries = tags.get(setting) + if not isinstance(entries, list): + continue + if value is None: + tags.pop(setting, None) + else: + tags[setting] = [e for e in entries if _parse_tag_name(e) != value] + if not tags.get(setting): + tags.pop(setting, None) + if not tags: + level_data.pop('tags', None) + + if _prune_level(level_data): + risk_levels.pop(level, None) + + for spec in set_specs or []: + level, setting, value = parse_ai_setting_spec(spec) + if setting is None: + raise ValueError(f'--set requires LEVEL.SETTING=VALUE (got "{spec}")') + if value is None: + raise ValueError(f'--set requires a value (got "{spec}")') + + level_data = _ensure_level_dict(risk_levels, level) + if setting == 'terminate': + terminate_value = _parse_terminate_value(value) + effective_value = False if level == 'low' else terminate_value + if level == 'low' and terminate_value: + if _LOW_TERMINATE_WARNING not in warnings: + warnings.append(_LOW_TERMINATE_WARNING) + if _existing_terminate_value(risk_levels, level) == effective_value: + continue + level_data['aiSessionTerminate'] = effective_value + continue + + tag_value = value.strip() + if not tag_value: + raise ValueError(f'--set tag value cannot be empty (got "{spec}")') + + entries = _get_tag_list(level_data, setting) + if not any(_parse_tag_name(e) == tag_value for e in entries): + action = 'added_to_allow' if setting == 'allow' else 'added_to_deny' + entries.append(_make_tag_entry(tag_value, action, user_id)) + + return _normalize_ai_settings_for_save(settings), warnings + + +def remove_resource_keeper_ai_settings( + params: KeeperParams, + resource_uid: str, + config_uid: Optional[str] = None +) -> Optional[bool]: + """Remove the ``ai_settings`` DATA edge (GSE_DELETION), restoring pre-AI DAG state. + + Web Vault treats a missing ``ai_settings`` edge as ``emptyKeeperAISettings`` in + memory. Writing ``{}`` or the empty template leaves a DATA edge and can break WV; + ``configure_resource`` does not clear ``keeperAiSettings`` when omitted — use graph-sync + DELETION like ``DagOperations.createDeletionEvent`` + ``dagPamLinkAddData``. + + Returns: + True if a DELETION edge was written, + None if ``ai_settings`` was already absent, + False on error. + """ + common = _resolve_resource_settings_inputs(params, resource_uid, {}, config_uid) + if common is None: + return False + record_key, resolved_config_uid = common + + return _delete_resource_data_edge_legacy( + params, resource_uid, resolved_config_uid, record_key, 'ai_settings') + + def print_keeper_ai_settings(params: KeeperParams, resource_uid: str, config_uid: Optional[str] = None): """ Print KeeperAI settings in a human-readable format. @@ -835,7 +1309,7 @@ def print_keeper_ai_settings(params: KeeperParams, resource_uid: str, config_uid """ settings = get_resource_keeper_ai_settings(params, resource_uid, config_uid) - if not settings: + if is_default_keeper_ai_settings(settings): print(f"{bcolors.WARNING}No KeeperAI settings found for resource {resource_uid}{bcolors.ENDC}") return diff --git a/keepercommander/commands/tunnel_and_connections.py b/keepercommander/commands/tunnel_and_connections.py index 0314248b6..aaa857b64 100644 --- a/keepercommander/commands/tunnel_and_connections.py +++ b/keepercommander/commands/tunnel_and_connections.py @@ -108,6 +108,7 @@ def __init__(self): # self.register_command('stop', PAMConnectionStopCommand(), 'Stop Connection', 'x') self.register_command('edit', PAMConnectionEditCommand(), 'Edit Connection settings', 'e') self.register_command('jit', PAMConnectionJitCommand(), 'View/update JIT settings', 'j') + self.register_command('ai', PAMConnectionAiCommand(), 'View/update KeeperAI settings', 'a') self.default_verb = 'edit' @@ -3281,6 +3282,244 @@ def _do_set(self, params, kwargs, record_uid, config_uid): print(f'{bcolors.OKGREEN}JIT settings saved successfully.{bcolors.ENDC}') +_PAM_CONNECTION_AI_RESOURCE_TYPES = frozenset({ + 'pamMachine', + 'pamDatabase', + 'pamDirectory', + 'pamRemoteBrowser', +}) +_PAM_CONNECTION_AI_RESOURCE_TYPES_LABEL = ( + 'pamMachine, pamDatabase, pamDirectory, or pamRemoteBrowser' +) + + +class PAMConnectionAiCommand(Command): + parser = argparse.ArgumentParser(prog='pam connection ai') + parser.add_argument('record', type=str, action='store', + help=f'Record UID, path, or title ({_PAM_CONNECTION_AI_RESOURCE_TYPES_LABEL})') + parser.add_argument('--configuration', '-c', type=str, dest='configuration', action='store', default=None, + help='PAM Configuration UID or title (required when record is not linked yet or is linked to 2+ configs)') + parser.add_argument('--set', '-s', dest='set_values', action='append', default=None, + metavar='LEVEL.SETTING=VALUE', + help='Set a KeeperAI value (e.g. -s low.terminate=false -s low.allow=chmod -s "high.deny=kill -9")') + parser.add_argument('--unset', '-u', dest='unset_values', action='append', default=None, + metavar='LEVEL[.SETTING[=VALUE]]', + help='Unset KeeperAI values (e.g. -u low removes the level; -u low.allow removes all allow tags; ' + '-u low.allow=chmod removes one tag)') + parser.add_argument('--remove', dest='remove', action='store_true', default=False, + help='Remove all KeeperAI settings (ai_settings path only)') + parser.add_argument('--show', dest='show', action='store_true', default=False, + help='Show current KeeperAI settings') + + def get_parser(self): + return PAMConnectionAiCommand.parser + + def execute(self, params, **kwargs): + record_name = kwargs.get('record') + configuration = kwargs.get('configuration') + remove_flag = kwargs.get('remove', False) + show_flag = kwargs.get('show', False) + set_values = kwargs.get('set_values') or [] + unset_values = kwargs.get('unset_values') or [] + change_options_provided = bool(set_values or unset_values) + + if remove_flag and show_flag: + raise CommandError('', f'{bcolors.FAIL}--remove cannot be used with --show.{bcolors.ENDC}') + if remove_flag and change_options_provided: + raise CommandError('', f'{bcolors.FAIL}--remove cannot be used with any other option.{bcolors.ENDC}') + if show_flag and change_options_provided: + raise CommandError('', f'{bcolors.FAIL}--show cannot be used with --remove or any KeeperAI option.{bcolors.ENDC}') + if not remove_flag and not show_flag and not change_options_provided: + raise CommandError('', f'{bcolors.FAIL}Provide at least one --set/-s, --unset/-u, --remove, or --show.{bcolors.ENDC}') + + record = RecordMixin.resolve_single_record(params, record_name) + if record is None: + matches = [] + for uid in params.record_cache: + rec = vault.KeeperRecord.load(params, uid) + if rec and rec.record_type in _PAM_CONNECTION_AI_RESOURCE_TYPES \ + and rec.title.casefold() == record_name.casefold(): + matches.append(rec) + if len(matches) == 0: + raise CommandError('', f'{bcolors.FAIL}Record "{record_name}" not found.{bcolors.ENDC}') + if len(matches) > 1: + raise CommandError('', + f'{bcolors.FAIL}Multiple records match title "{record_name}"; use UID or path.{bcolors.ENDC}') + record = matches[0] + + if not isinstance(record, vault.TypedRecord) or \ + record.record_type not in _PAM_CONNECTION_AI_RESOURCE_TYPES: + raise CommandError('', + f'{bcolors.FAIL}KeeperAI settings are only supported on ' + f'{_PAM_CONNECTION_AI_RESOURCE_TYPES_LABEL} records.{bcolors.ENDC}') + + record_uid = record.record_uid + config_uid = self._resolve_config_uid( + params, record_uid, configuration, allow_unlinked_without_config=show_flag) + + if show_flag: + self._do_show(params, record, record_uid, config_uid) + elif remove_flag: + self._do_remove(params, record_uid, config_uid) + else: + self._do_apply(params, set_values, unset_values, record_uid, config_uid) + + def _resolve_config_uid(self, params, record_uid, configuration, allow_unlinked_without_config=False): + encrypted_session_token, encrypted_transmission_key, _ = get_keeper_tokens(params) + config_leafs = get_dag_leafs(params, encrypted_session_token, encrypted_transmission_key, record_uid) + config_uids = [leaf.get('value') for leaf in (config_leafs or []) if leaf.get('value')] + + if len(config_uids) == 0: + if not configuration: + if allow_unlinked_without_config: + return None + raise CommandError('', + f'{bcolors.FAIL}Record is not linked to a PAM Configuration; ' + f'specify --configuration|-c.{bcolors.ENDC}') + config_uid = self._resolve_configuration_record(params, configuration, linked_config_uids=None) + return config_uid + + if len(config_uids) == 1: + config_uid = config_uids[0] + if configuration: + specified_uid = self._resolve_configuration_record(params, configuration, linked_config_uids=config_uids) + if specified_uid != config_uid: + raise CommandError('', + f'{bcolors.FAIL}PAM Configuration "{configuration}" is not linked to this record.{bcolors.ENDC}') + return config_uid + + if not configuration: + raise CommandError('', + f'{bcolors.FAIL}Record is linked to multiple PAM Configurations; ' + f'specify --configuration|-c.{bcolors.ENDC}') + return self._resolve_configuration_record(params, configuration, linked_config_uids=config_uids) + + @staticmethod + def _resolve_configuration_record(params, configuration, linked_config_uids): + config_rec = RecordMixin.resolve_single_record(params, configuration) + if config_rec is None: + for uid in params.record_cache: + if params.record_cache[uid].get('version', 0) == 6: + r = vault.KeeperRecord.load(params, uid) + if r and r.title.casefold() == configuration.casefold(): + config_rec = r + break + if config_rec is None: + raise CommandError('', + f'{bcolors.FAIL}PAM Configuration "{configuration}" not found.{bcolors.ENDC}') + config_uid = config_rec.record_uid + if linked_config_uids is not None and config_uid not in linked_config_uids: + raise CommandError('', + f'{bcolors.FAIL}PAM Configuration "{configuration}" is not linked to this record.{bcolors.ENDC}') + return config_uid + + def _do_show(self, params, record, record_uid, config_uid): + from .pam_import.keeper_ai_settings import ( + get_resource_keeper_ai_settings, + is_default_keeper_ai_settings, + ) + + print(f'\nKeeperAI Settings for {bcolors.OKBLUE}{record.title}{bcolors.ENDC} ({record_uid}):') + if config_uid is None: + print(f' {bcolors.WARNING}No KeeperAI settings configured ' + f'(record is not linked to a PAM Configuration).{bcolors.ENDC}\n') + return + + settings = get_resource_keeper_ai_settings(params, record_uid, config_uid) + if is_default_keeper_ai_settings(settings): + print(f' {bcolors.WARNING}No KeeperAI settings configured.{bcolors.ENDC}\n') + return + + print(f' Version: {settings.get("version", "unknown")}') + risk_levels = settings.get('riskLevels', {}) + for level in ('critical', 'high', 'medium', 'low'): + level_data = risk_levels.get(level) + if not level_data: + continue + terminate = level_data.get('aiSessionTerminate', False) + tags = level_data.get('tags', {}) if isinstance(level_data.get('tags'), dict) else {} + allow_tags = tags.get('allow', []) + deny_tags = tags.get('deny', []) if level != 'low' else [] + level_color = { + 'critical': bcolors.FAIL, + 'high': bcolors.WARNING, + 'medium': bcolors.OKBLUE, + 'low': bcolors.OKGREEN, + }.get(level, bcolors.ENDC) + print(f'\n {level_color}{level.upper()}{bcolors.ENDC}:') + print(f' Terminate Session: {terminate}') + if allow_tags: + print(' Allow:') + for tag_item in allow_tags: + tag_name = tag_item.get('tag', '') if isinstance(tag_item, dict) else str(tag_item) + print(f' - {tag_name}') + if deny_tags: + print(' Deny:') + for tag_item in deny_tags: + tag_name = tag_item.get('tag', '') if isinstance(tag_item, dict) else str(tag_item) + print(f' - {tag_name}') + print() + + def _do_remove(self, params, record_uid, config_uid): + from .pam_import.keeper_ai_settings import remove_resource_keeper_ai_settings + + result = remove_resource_keeper_ai_settings(params, record_uid, config_uid) + if result is True: + print(f'{bcolors.OKGREEN}KeeperAI settings removed successfully.{bcolors.ENDC}') + elif result is None: + print(f'{bcolors.WARNING}No KeeperAI settings configured.{bcolors.ENDC}') + else: + raise CommandError('', f'{bcolors.FAIL}Failed to remove KeeperAI settings.{bcolors.ENDC}') + + def _do_apply(self, params, set_values, unset_values, record_uid, config_uid): + from .pam_import.keeper_ai_settings import ( + apply_ai_setting_changes, + dedupe_ai_cli_option_specs, + get_resource_keeper_ai_settings, + is_default_keeper_ai_settings, + refresh_link_to_config_to_latest, + refresh_meta_to_latest, + remove_resource_keeper_ai_settings, + set_resource_keeper_ai_settings, + ) + + set_values, set_dup_warnings = dedupe_ai_cli_option_specs(set_values, '--set/-s') + unset_values, unset_dup_warnings = dedupe_ai_cli_option_specs(unset_values, '--unset/-u') + for warning in set_dup_warnings + unset_dup_warnings: + print(f'{bcolors.WARNING}Warning: {warning}{bcolors.ENDC}') + + existing = get_resource_keeper_ai_settings( + params, record_uid, config_uid, quiet_if_missing_vertex=True) + try: + ai_dict, warnings = apply_ai_setting_changes(existing, set_values, unset_values, params) + except ValueError as err: + raise CommandError('', f'{bcolors.FAIL}{err}{bcolors.ENDC}') from err + + for warning in warnings: + print(f'{bcolors.WARNING}Warning: {warning}{bcolors.ENDC}') + + if not ai_dict: + # e.g. -s low.terminate=true|false when nothing is configured: defaults to {}. + if is_default_keeper_ai_settings(existing): + return + result = remove_resource_keeper_ai_settings(params, record_uid, config_uid) + if result is False: + raise CommandError('', f'{bcolors.FAIL}Failed to remove KeeperAI settings.{bcolors.ENDC}') + if result is None: + print(f'{bcolors.WARNING}No KeeperAI settings configured.{bcolors.ENDC}') + else: + print(f'{bcolors.OKGREEN}KeeperAI settings removed successfully.{bcolors.ENDC}') + return + + ok = set_resource_keeper_ai_settings(params, record_uid, ai_dict, config_uid) + if not ok: + raise CommandError('', f'{bcolors.FAIL}Failed to save KeeperAI settings.{bcolors.ENDC}') + + refresh_meta_to_latest(params, record_uid, config_uid) + refresh_link_to_config_to_latest(params, record_uid, config_uid) + print(f'{bcolors.OKGREEN}KeeperAI settings saved successfully.{bcolors.ENDC}') + + class PAMRbiEditCommand(Command): choices = ['on', 'off', 'default'] parser = argparse.ArgumentParser(prog='pam rbi edit') diff --git a/unit-tests/pam/test_dag_layer_b_migration.py b/unit-tests/pam/test_dag_layer_b_migration.py index b7be169d1..3ba9090c7 100644 --- a/unit-tests/pam/test_dag_layer_b_migration.py +++ b/unit-tests/pam/test_dag_layer_b_migration.py @@ -96,6 +96,11 @@ def _capture(params, rq): assert rq.keeperAiSettings == b'CIPHER_BYTES' # Critical: must NOT be set on jitSettings field assert rq.jitSettings == b'' + assert rq.meta == json.dumps({ + 'version': 1, + 'allowedSettings': {}, + 'rotateOnTermination': False, + }).encode() def test_happy_path_bundles_current_meta_so_krouter_persists_ai_edge(self): """Regression: krouter's configure_resource only writes a settings edge @@ -125,8 +130,30 @@ def _capture(params, rq): # the ai_settings edge. Without it the write is a silent no-op. assert rq.meta == json.dumps(meta_dict).encode() # meta is read from the resource's current 'meta' DATA edge. + assert meta_mock.call_args.kwargs.get('quiet_if_missing_vertex') is True + + def test_bootstraps_default_meta_when_resource_not_in_dag_yet(self): + captured = {} + + def _capture(params, rq): + captured['rq'] = rq + return None + + with _patch_inputs(), \ + patch.object(ai_mod, 'encrypt_aes', return_value=b'CIPHER_BYTES'), \ + patch.object(ai_mod, 'get_resource_settings', return_value=None) as meta_mock, \ + patch('keepercommander.commands.pam.router_helper.router_configure_resource', side_effect=_capture): + ok = ai_mod.set_resource_keeper_ai_settings( + _mock_params(), RESOURCE_UID_STR, {'riskLevels': {'high': {}}}, config_uid=CONFIG_UID_STR + ) + assert ok is True + rq = captured['rq'] + assert rq.meta == json.dumps({ + 'version': 1, + 'allowedSettings': {}, + 'rotateOnTermination': False, + }).encode() meta_mock.assert_called_once() - assert meta_mock.call_args.args[2] == 'meta' def test_permission_denied_with_fallback_enabled_calls_legacy(self): legacy_called = {'count': 0} diff --git a/unit-tests/pam/test_pam_connection_ai.py b/unit-tests/pam/test_pam_connection_ai.py new file mode 100644 index 000000000..ea4ad03d1 --- /dev/null +++ b/unit-tests/pam/test_pam_connection_ai.py @@ -0,0 +1,288 @@ +import pytest +from unittest.mock import patch + +from keepercommander.commands.pam_import.keeper_ai_settings import ( + apply_ai_setting_changes, + dedupe_ai_cli_option_specs, + empty_keeper_ai_settings_dict, + is_default_keeper_ai_settings, + parse_ai_setting_spec, +) + + +class _Params: + account_uid_bytes = b'\x01\x02\x03' + user = 'user@example.com' + + +def test_dedupe_ai_cli_option_specs_reports_counts(): + specs, warnings = dedupe_ai_cli_option_specs( + ['critical.allow=chmod', 'high.allow=wget', 'critical.allow=chmod', 'critical.allow=chmod'], + '--set/-s', + ) + assert specs == ['critical.allow=chmod', 'high.allow=wget'] + assert warnings == ['duplicate --set/-s ignored: critical.allow=chmod (3x)'] + + +def test_dedupe_ai_cli_option_specs_no_warnings_when_unique(): + specs, warnings = dedupe_ai_cli_option_specs(['low'], '--unset/-u') + assert specs == ['low'] + assert warnings == [] + + +def test_is_default_keeper_ai_settings(): + assert is_default_keeper_ai_settings(None) + assert is_default_keeper_ai_settings({}) + assert is_default_keeper_ai_settings(empty_keeper_ai_settings_dict()) + assert not is_default_keeper_ai_settings({ + 'version': 'v1.0.0', + 'riskLevels': {'high': {'aiSessionTerminate': True}}, + }) + + +def test_remove_resource_keeper_ai_settings_emits_deletion(): + params = _Params() + captured = {} + + def _capture(_params, _resource_uid, _config_uid, _record_key, dag_path): + captured['dag_path'] = dag_path + return True + + from keepercommander.commands.pam_import import keeper_ai_settings as ai_mod + with patch.object(ai_mod, '_resolve_resource_settings_inputs', return_value=(b'key', 'config')), \ + patch.object(ai_mod, '_delete_resource_data_edge_legacy', side_effect=_capture): + result = ai_mod.remove_resource_keeper_ai_settings(params, 'resource', 'config') + assert result is True + assert captured['dag_path'] == 'ai_settings' + + +def test_remove_resource_keeper_ai_settings_already_absent(): + params = _Params() + + from keepercommander.commands.pam_import import keeper_ai_settings as ai_mod + with patch.object(ai_mod, '_resolve_resource_settings_inputs', return_value=(b'key', 'config')), \ + patch.object(ai_mod, '_delete_resource_data_edge_legacy', return_value=None): + result = ai_mod.remove_resource_keeper_ai_settings(params, 'resource', 'config') + assert result is None + + +def test_parse_ai_setting_spec_set(): + assert parse_ai_setting_spec('low.terminate=false') == ('low', 'terminate', 'false') + assert parse_ai_setting_spec('high.allow=chmod') == ('high', 'allow', 'chmod') + assert parse_ai_setting_spec('critical.deny=kill -9') == ('critical', 'deny', 'kill -9') + + +def test_parse_ai_setting_spec_unset(): + assert parse_ai_setting_spec('low') == ('low', None, None) + assert parse_ai_setting_spec('medium.allow') == ('medium', 'allow', None) + assert parse_ai_setting_spec('high.deny=wget') == ('high', 'deny', 'wget') + + +def test_parse_ai_setting_spec_rejects_low_deny(): + with pytest.raises(ValueError, match='deny is not supported'): + parse_ai_setting_spec('low.deny=bash') + + +def test_parse_ai_setting_spec_missing_value_suggests_unset(): + with pytest.raises(ValueError, match='--unset\\|-u'): + parse_ai_setting_spec('high.terminate=') + + +def test_apply_ai_setting_changes_merge_without_default_low_stub(): + params = _Params() + result, warnings = apply_ai_setting_changes(None, ['high.allow=chmod'], None, params) + + assert warnings == [] + assert result['version'] == 'v1.0.0' + assert result['riskLevels']['high']['tags']['allow'][0]['tag'] == 'chmod' + assert 'low' not in result['riskLevels'] + + +def test_apply_ai_setting_changes_tag_upsert_no_duplicate(): + params = _Params() + existing = { + 'version': 'v1.0.0', + 'riskLevels': { + 'high': { + 'tags': {'allow': [{'tag': 'chmod', 'auditLog': [{'action': 'added_to_allow'}]}]}, + }, + }, + } + + result, warnings = apply_ai_setting_changes(existing, ['high.allow=chmod'], None, params) + + assert warnings == [] + assert len(result['riskLevels']['high']['tags']['allow']) == 1 + + +def test_apply_ai_setting_changes_unset_level_and_tag(): + params = _Params() + existing = { + 'version': 'v1.0.0', + 'riskLevels': { + 'critical': { + 'aiSessionTerminate': True, + 'tags': {'allow': [{'tag': 'mount', 'auditLog': []}]}, + }, + 'low': { + 'aiSessionTerminate': False, + 'tags': {'allow': [{'tag': 'chmod', 'auditLog': []}, {'tag': 'wget', 'auditLog': []}]}, + }, + }, + } + + result, warnings = apply_ai_setting_changes(existing, None, ['critical', 'low.allow=chmod'], params) + + assert warnings == [] + assert 'critical' not in result['riskLevels'] + low_allow = [t['tag'] for t in result['riskLevels']['low']['tags']['allow']] + assert low_allow == ['wget'] + assert result['riskLevels']['low']['aiSessionTerminate'] is False + + +def test_apply_ai_setting_changes_set_terminate_rejects_invalid_value(): + params = _Params() + with pytest.raises(ValueError, match='invalid terminate value "truez"'): + apply_ai_setting_changes(None, ['high.terminate=truez'], None, params) + + +def test_apply_ai_setting_changes_set_terminate_accepts_true_false_only(): + params = _Params() + result_true, _ = apply_ai_setting_changes(None, ['high.terminate=true'], None, params) + assert result_true['riskLevels']['high']['aiSessionTerminate'] is True + + result_false, _ = apply_ai_setting_changes(result_true, ['medium.terminate=FALSE'], None, params) + assert result_false['riskLevels']['medium']['aiSessionTerminate'] is False + + +def test_apply_ai_setting_changes_set_terminate(): + params = _Params() + result, warnings = apply_ai_setting_changes(None, ['medium.terminate=true'], None, params) + assert warnings == [] + assert result['riskLevels']['medium']['aiSessionTerminate'] is True + + +def test_apply_ai_setting_changes_low_terminate_true_warns_and_stays_false(): + params = _Params() + result, warnings = apply_ai_setting_changes(None, ['low.terminate=true'], None, params) + + assert warnings == ['risk level low.terminate always defaults to false.'] + assert result == {} + + +def test_apply_ai_setting_changes_low_terminate_false_is_noop_when_empty(): + params = _Params() + result, warnings = apply_ai_setting_changes(None, ['low.terminate=false'], None, params) + + assert warnings == [] + assert result == {} + assert is_default_keeper_ai_settings(None) + + +def test_apply_ai_setting_changes_paired_unset_set_tag_preserves_audit_log(): + params = _Params() + original_audit = [{'date': 1, 'userId': 'u1', 'action': 'added_to_allow'}] + existing = { + 'version': 'v1.0.0', + 'riskLevels': { + 'high': { + 'tags': {'allow': [{'tag': 'chmod', 'auditLog': original_audit}]}, + }, + }, + } + + result, warnings = apply_ai_setting_changes( + existing, + ['high.allow=chmod'], + ['high.allow=chmod'], + params, + ) + + assert warnings == ['--set/-s overrides --unset/-u: high.allow=chmod'] + entry = result['riskLevels']['high']['tags']['allow'][0] + assert entry['tag'] == 'chmod' + assert entry['auditLog'] == original_audit + + +def test_apply_ai_setting_changes_paired_unset_set_terminate_noop_when_already_true(): + params = _Params() + existing = { + 'version': 'v1.0.0', + 'riskLevels': { + 'high': {'aiSessionTerminate': True}, + }, + } + + result, warnings = apply_ai_setting_changes( + existing, + ['high.terminate=true'], + ['high.terminate'], + params, + ) + + assert warnings == [] + assert result['riskLevels']['high']['aiSessionTerminate'] is True + + +def test_apply_ai_setting_changes_mirrored_unset_set_different_tag_values_warns(): + params = _Params() + result, warnings = apply_ai_setting_changes( + None, + ['critical.allow=chmod'], + ['critical.allow=chmo'], + params, + ) + + assert warnings == [ + '--set/-s overrides --unset/-u: critical.allow=chmo (mirrors -s critical.allow=chmod)', + ] + assert result['riskLevels']['critical']['tags']['allow'][0]['tag'] == 'chmod' + + +def test_apply_ai_setting_changes_partial_unset_not_mirrored_with_set(): + params = _Params() + existing = { + 'version': 'v1.0.0', + 'riskLevels': { + 'critical': { + 'tags': {'allow': [{'tag': 'wget', 'auditLog': []}]}, + }, + }, + } + result, warnings = apply_ai_setting_changes( + existing, + ['critical.allow=chmod'], + ['critical.allow'], + params, + ) + + assert warnings == [] + tags = [t['tag'] for t in result['riskLevels']['critical']['tags']['allow']] + assert tags == ['chmod'] + + +def test_apply_ai_setting_changes_unset_all_leaves_empty(): + params = _Params() + existing = { + 'version': 'v1.0.0', + 'riskLevels': { + 'low': { + 'aiSessionTerminate': False, + 'tags': {'allow': [{'tag': 'chmod', 'auditLog': []}]}, + }, + }, + } + + result, warnings = apply_ai_setting_changes(existing, None, ['low'], params) + + assert warnings == [] + assert result == {} + + +def test_pam_connection_ai_resource_types_include_rbi(): + from keepercommander.commands.tunnel_and_connections import _PAM_CONNECTION_AI_RESOURCE_TYPES + + assert 'pamRemoteBrowser' in _PAM_CONNECTION_AI_RESOURCE_TYPES + assert 'pamMachine' in _PAM_CONNECTION_AI_RESOURCE_TYPES + assert 'pamDatabase' in _PAM_CONNECTION_AI_RESOURCE_TYPES + assert 'pamDirectory' in _PAM_CONNECTION_AI_RESOURCE_TYPES From 0c2b22573feca56b3008f5740edd5997335e370a Mon Sep 17 00:00:00 2001 From: John Walstra Date: Thu, 18 Jun 2026 15:13:28 -0500 Subject: [PATCH 11/11] DR-1280 Fix SaaS profile for `pam rotation edit` command. --- keepercommander/commands/discoveryrotation.py | 169 +++++++++++++++++- keepercommander/commands/pam_debug/info.py | 5 + keepercommander/commands/pam_saas/remove.py | 6 +- keepercommander/commands/pam_saas/set.py | 68 +++++-- .../tunnel/port_forward/TunnelGraph.py | 62 ++++++- 5 files changed, 284 insertions(+), 26 deletions(-) diff --git a/keepercommander/commands/discoveryrotation.py b/keepercommander/commands/discoveryrotation.py index d8797f141..ffc22f5e0 100644 --- a/keepercommander/commands/discoveryrotation.py +++ b/keepercommander/commands/discoveryrotation.py @@ -18,6 +18,7 @@ import time from datetime import datetime from urllib.parse import urlparse, urlunparse +from typing import Optional, List import requests from keeper_secrets_manager_core.utils import url_safe_str_to_bytes @@ -43,7 +44,7 @@ from .pam.router_helper import router_send_action_to_gateway, print_router_response, \ router_get_connected_gateways, router_set_record_rotation_information, router_get_rotation_schedules, \ - get_router_url + get_router_url, RouterResponseError from .record_edit import RecordEditMixin from .helpers.timeout import parse_timeout from .email_commands import find_email_config_record, load_email_config_from_record, update_oauth_tokens_in_record @@ -93,6 +94,7 @@ PAMUniversalSyncConfigCommand, PAMUniversalSyncRunCommand ) +from ..discovery_common.types import UserAcl, UserAclRotationSettings # These characters are based on the Vault PAM_DEFAULT_SPECIAL_CHAR = '''!@#$%^?();',.=+[]<>{}-_/\\*&:"`~|''' @@ -398,6 +400,8 @@ class PAMCreateRecordRotationCommand(Command): parser.add_argument('--force', '-f', dest='force', action='store_true', help='Do not ask for confirmation') parser.add_argument('--config', '-c', required=False, dest='config', action='store', help='UID or path of the configuration record.') + parser.add_argument('--gateway', '-g', required=False, dest='gateway', action='store', + help='UID or name of the gateway') parser.add_argument('--iam-aad-config', '-iac', dest='iam_aad_config_uid', action='store', help='UID of a PAM Configuration. Used for an IAM or Azure AD user in place of --resource.') parser.add_argument('--rotation-profile', '-rp', dest='rotation_profile', action='store', @@ -481,6 +485,158 @@ def config_resource(_dag, target_record, target_config_uid, silent=None): if not silent: _dag.print_tunneling_config(target_record.record_uid, config_uid=target_config_uid) + def config_saas_user(_dag, target_record, saas_config_uid: str): + + saas_config_record = vault.KeeperRecord.load(params, saas_config_uid) # type: Optional[TypedRecord] + if saas_config_record is None: + raise CommandError('', 'The SaaS configuration record does not exists.') + + if saas_config_record.record_type not in ["login", "saasConfiguration"]: + raise CommandError('', + f"The SaaS configuration record is not a SaaS configuration record: " + f"{saas_config_record.record_type}") + + plugin_name_field = next((x for x in saas_config_record.custom if x.label == "SaaS Type"), None) + if plugin_name_field is None: + raise CommandError('', + f"The SaaS configuration record is missing the custom field " + "label 'SaaS Type'") + + current_record_rotation = params.record_rotation_cache.get(target_record.record_uid) + schedule_only = kwargs.get('schedule_only') + + # Handle schedule-only operations first to avoid unnecessary resource validation + if schedule_only: + if kwargs.get('folder_name') and ( + not current_record_rotation or current_record_rotation.get('disabled')): + skipped_records.append([target_record.record_uid, target_record.title, + 'Rotation not enabled', 'Skipped']) + return + if not current_record_rotation: + skipped_records.append([target_record.record_uid, target_record.title, + 'No rotation info', 'Skipped']) + return + + else: + # This looks like it is user to remove the user from another PAM graph. + # To prevent two graphs from using the same record. + if _dag and not _dag.linking_dag.has_graph: + _dag = TunnelDAG(params, encrypted_session_token, encrypted_transmission_key, config_uid, + transmission_key=transmission_key) + if not _dag or not _dag.linking_dag.has_graph: + _dag.edit_tunneling_config(rotation=True) + old_dag = TunnelDAG(params, encrypted_session_token, encrypted_transmission_key, + target_record.record_uid, transmission_key=transmission_key) + if old_dag.linking_dag.has_graph and old_dag.record.record_uid != config_uid: + old_dag.remove_from_dag(target_record.record_uid) + + # 1. PAM Configuration UID + record_config_uid = _dag.record.record_uid + record_pam_config = pam_config + if not record_config_uid: + if current_record_rotation: + record_config_uid = current_record_rotation.get('configuration_uid') + pc = vault.KeeperRecord.load(params, record_config_uid) + if pc is None: + skipped_records.append( + [target_record.record_uid, target_record.title, 'PAM Configuration was deleted', + 'Specify a configuration UID parameter [--config]']) + return + if not isinstance(pc, vault.TypedRecord) or pc.version != 6: + skipped_records.append( + [target_record.record_uid, target_record.title, 'PAM Configuration is invalid', + 'Specify a configuration UID parameter [--config]']) + return + record_pam_config = pc + else: + skipped_records.append( + [target_record.record_uid, target_record.title, 'No current PAM Configuration', + 'Specify a configuration UID parameter [--config]']) + return + + # 2. Schedule + record_schedule_data = schedule_data + if record_schedule_data is None: + if current_record_rotation and not schedule_config: + try: + current_schedule = current_record_rotation.get('schedule') + if current_schedule: + record_schedule_data = json.loads(current_schedule) + except: + pass + else: + schedule_field = record_pam_config.get_typed_field('schedule', 'defaultRotationSchedule') + if schedule_field and isinstance(schedule_field.value, list) and len(schedule_field.value) > 0: + if isinstance(schedule_field.value[0], dict): + record_schedule_data = [schedule_field.value[0]] + + # 3. Password complexity + if pwd_complexity_rule_list is None: + if current_record_rotation: + pwd_complexity_rule_list_encrypted = utils.base64_url_decode( + current_record_rotation['pwd_complexity']) + else: + pwd_complexity_rule_list_encrypted = b'' + else: + if len(pwd_complexity_rule_list) > 0: + pwd_complexity_rule_list_encrypted = router_helper.encrypt_pwd_complexity(pwd_complexity_rule_list, + target_record.record_key) + else: + pwd_complexity_rule_list_encrypted = b'' + + disabled = False + # 5. Enable rotation + if kwargs.get('enable'): + _dag.set_resource_allowed(config_uid, rotation=True, is_config=True) + elif kwargs.get('disable'): + _dag.set_resource_allowed(config_uid, rotation=False, is_config=True) + disabled = True + + schedule = 'On-Demand' + if isinstance(record_schedule_data, list) and len(record_schedule_data) > 0: + if isinstance(record_schedule_data[0], dict): + schedule = record_schedule_data[0].get('type') + complexity = '' + if pwd_complexity_rule_list_encrypted: + try: + decrypted_complexity = crypto.decrypt_aes_v2(pwd_complexity_rule_list_encrypted, + target_record.record_key) + c = json.loads(decrypted_complexity.decode()) + complexity = f"{c.get('length', 0)}," \ + f"{c.get('caps', 0)}," \ + f"{c.get('lowercase', 0)}," \ + f"{c.get('digits', 0)}," \ + f"{c.get('special', 0)}," \ + f"{c.get('specialChars', PAM_DEFAULT_SPECIAL_CHAR)}" + except: + pass + valid_records.append( + [target_record.record_uid, target_record.title, not disabled, record_config_uid, None, + schedule, complexity]) + + # 6. Construct Request object for SaaS + rq = router_pb2.RouterRecordRotationRequest() + if current_record_rotation: + logging.debug("has current record gas rotation settings") + rq.revision = current_record_rotation.get('revision', 1) + else: + logging.debug("record has not rotation settings") + rq.recordUid = utils.base64_url_decode(target_record.record_uid) + rq.configurationUid = utils.base64_url_decode(record_config_uid) + rq.schedule = json.dumps(record_schedule_data) if record_schedule_data else '' + rq.pwdComplexity = pwd_complexity_rule_list_encrypted + rq.disabled = disabled + rq.noop = True + rq.resourceUid = b'' + r_requests.append(rq) + + if not _dag.link_saas_user(user_uid=target_record.record_uid, + saas_config_record=saas_config_record, + pam_config_record_type=pam_config.record_type): + raise CommandError('',"Could not connect SaaS user in the graph.") + + params.sync_data = True + def config_iam_aad_user(_dag, target_record, target_iam_aad_config_uid): current_record_rotation = params.record_rotation_cache.get(target_record.record_uid) schedule_only = kwargs.get('schedule_only') @@ -1191,15 +1347,14 @@ def add_folders(sub_folder): # type: (BaseFolderNode) -> None raise CommandError('', 'General rotation profile requires --resource to be specified.') config_user(tmp_dag, _record, resource_uid, config_uid, silent=kwargs.get('silent')) elif rotation_profile == 'saas': + saas_config_uid = kwargs.get("saas_config_uid") # type: Optional[str] if saas_config_uid is None: raise CommandError('', 'SaaS rotation profile requires ' '--saas-config-uid to be specified.') - saas_command = PAMActionSaasSetCommand() - saas_command.execute(params, - user_uid=_record.record_uid, - pam_config_uid=config_uid, - config_record_uid=saas_config_uid) + + config_saas_user(_dag=tmp_dag, target_record=_record, saas_config_uid=saas_config_uid) + params.sync_data = True # NB! --folder=UID without --iam-aad-config, or --schedule-only converts to General rotation elif iam_aad_config_uid: @@ -1235,6 +1390,8 @@ def add_folders(sub_folder): # type: (BaseFolderNode) -> None except KeeperApiError as kae: logging.warning('Record "%s": Set rotation error "%s": %s', record_uid, kae.result_code, kae.message) + except RouterResponseError as rre: + logging.warning('Record "%s": Set rotation error: %s', record_uid, rre) params.sync_data = True diff --git a/keepercommander/commands/pam_debug/info.py b/keepercommander/commands/pam_debug/info.py index 09d9bcfa6..b1f2b648a 100644 --- a/keepercommander/commands/pam_debug/info.py +++ b/keepercommander/commands/pam_debug/info.py @@ -185,6 +185,11 @@ def _print_field(f): else: print(f" . Is {self._bl('Remote user')}") + if acl_content.is_iam_user: + print(f" . Is an IAM User (user belonging to configuration)") + else: + print(f" . Is NOT an IAM User (user belonging to configuration)") + if acl_content.rotation_settings is None: print(f"{bcolors.FAIL} . There are no rotation settings!{bcolors.ENDC}") else: diff --git a/keepercommander/commands/pam_saas/remove.py b/keepercommander/commands/pam_saas/remove.py index f4a8fb7d6..f763468c6 100644 --- a/keepercommander/commands/pam_saas/remove.py +++ b/keepercommander/commands/pam_saas/remove.py @@ -16,10 +16,14 @@ class PAMActionSaasRemoveCommand(PAMGatewayActionDiscoverCommandBase): parser = argparse.ArgumentParser(prog='pam action saas remove') + parser.add_argument('--gateway', '-g', required=True, dest='gateway', action='store', + help='Gateway name of UID.') + parser.add_argument('--configuration-uid', required=False, dest='configuration_uid', + action='store', help='PAM configuration UID, if gateway has multiple.') parser.add_argument('--user-uid', '-u', required=True, dest='user_uid', action='store', help='The UID of the User record') parser.add_argument('--resource-uid', '-r', required=False, dest='resource_uid', action='store', - help='The UID of the Resource record, if needed.') + help='The UID of the Resource record, if needed. DEPRECATED') def get_parser(self): return PAMActionSaasRemoveCommand.parser diff --git a/keepercommander/commands/pam_saas/set.py b/keepercommander/commands/pam_saas/set.py index be9976924..bb3b97dc6 100644 --- a/keepercommander/commands/pam_saas/set.py +++ b/keepercommander/commands/pam_saas/set.py @@ -1,11 +1,12 @@ from __future__ import annotations import argparse -from ..discover import PAMGatewayActionDiscoverCommandBase, GatewayContext +from ..discover import PAMGatewayActionDiscoverCommandBase, GatewayContext, MultiConfigurationException, multi_conf_msg +from ...display import bcolors from ... import vault from . import get_plugins_map from ...discovery_common.record_link import RecordLink -from ...discovery_common.constants import PAM_USER -from ...discovery_common.types import UserAclRotationSettings +from ...discovery_common.constants import PAM_USER, PAM_AWS_CONFIGURATION +from ...discovery_common.types import UserAclRotationSettings, UserAcl from typing import Optional, TYPE_CHECKING if TYPE_CHECKING: @@ -16,9 +17,13 @@ class PAMActionSaasSetCommand(PAMGatewayActionDiscoverCommandBase): parser = argparse.ArgumentParser(prog='pam action saas set') + parser.add_argument('--gateway', '-g', required=False, dest='gateway', action='store', + help='Gateway name of UID.') + parser.add_argument('--configuration-uid', required=False, dest='configuration_uid', + action='store', help='PAM configuration UID, if gateway has multiple.') parser.add_argument('--user-uid', '-u', required=True, dest='user_uid', action='store', help='The UID of the User record') - parser.add_argument('--config-record-uid', '-c', required=True, dest='config_record_uid', + parser.add_argument('--config-record-uid', '-sc', required=True, dest='config_record_uid', action='store', help='The UID of the record that has SaaS configuration') def get_parser(self): @@ -26,9 +31,10 @@ def get_parser(self): def execute(self, params: KeeperParams, **kwargs): - user_uid = kwargs.get("user_uid") # type: str - resource_uid = kwargs.get("resource_uid") # type: str - config_record_uid = kwargs.get("config_record_uid") # type: str + user_uid = kwargs.get("user_uid") # type: Optional[str] + config_record_uid = kwargs.get("config_record_uid") # type: Optional[str] + gateway = kwargs.get("gateway") # type: Optional[str] + configuration_uid = kwargs.get('configuration_uid') # type Optional[str] print("") @@ -43,12 +49,36 @@ def execute(self, params: KeeperParams, **kwargs): print(self._f("The user record is not a PAM User.")) return - record_rotation = params.record_rotation_cache.get(user_record.record_uid) - if record_rotation is not None: - configuration_uid = record_rotation.get("configuration_uid") - else: - print(self._f("The user record does not have any rotation settings.")) - return + # If the configration UID is not set, then try to get it from the rotation settings. + # This might be blank due to rotation setting being stored on the graph and not in protobuf. + # If rotation settings are blank, require the gateway UID to be set. + if configuration_uid is None: + + record_rotation = params.record_rotation_cache.get(user_record.record_uid) + if record_rotation is not None: + configuration_uid = record_rotation.get("configuration_uid") + else: + # If the configuration UID is not passed in, and we cannot get it from the protobug rotation settings, + # get it from the gateway. + # This requires the user used the --gateway params. + + if gateway is None: + print(self._f("Cannot find the PAM configuration for the user record. " + "Please add the --gateway and/or --configuration-uid to the command parameters.")) + return + + try: + gateway_context = GatewayContext.from_gateway(params=params, + gateway=gateway) + if gateway_context is None: + print(f"{bcolors.FAIL}Could not find the gateway configuration for {gateway}.{bcolors.ENDC}") + return + + configuration_uid = gateway_context.configuration_uid + + except MultiConfigurationException as err: + multi_conf_msg(gateway, err) + return if configuration_uid is None: print(self._f("The user record does not have the configuration record set in the rotation settings.")) @@ -114,11 +144,13 @@ def execute(self, params: KeeperParams, **kwargs): record_link = RecordLink(record=gateway_context.configuration, params=params, fail_on_corrupt=False) acl = record_link.get_acl(user_uid, gateway_context.configuration_uid) if acl is None: - if resource_uid is not None: - print(self._f("There is no relationship between the user and the resource record.")) - else: - print(self._f("There is no relationship between the user and the configuration record.")) - return + acl = UserAcl.default() + + # TODO: Un-hack this, dislike hardcoding this stuff. + # HACK - If the plugin is the AWS Access Key, the user is an IAM user if the configuration if AWS. + # So it does belong to the configuration. + if plugin_name == "AWS Access Key" and config_record.record_type == PAM_AWS_CONFIGURATION: + acl.belongs_to = True if acl.rotation_settings is None: acl.rotation_settings = UserAclRotationSettings() diff --git a/keepercommander/commands/tunnel/port_forward/TunnelGraph.py b/keepercommander/commands/tunnel/port_forward/TunnelGraph.py index 4fb530dc7..ac89cb1d0 100644 --- a/keepercommander/commands/tunnel/port_forward/TunnelGraph.py +++ b/keepercommander/commands/tunnel/port_forward/TunnelGraph.py @@ -5,8 +5,9 @@ from ....keeper_dag.connection.commander import Connection from ....keeper_dag.types import RefType, PamGraphId from ....keeper_dag.vertex import DAGVertex +from ....discovery_common.types import UserAclRotationSettings, UserAcl from ....display import bcolors -from ....vault import PasswordRecord +from ....vault import PasswordRecord, TypedRecord from ....proto import pam_pb2, router_pb2 from ...pam._layer_b import should_fallback_on_layer_b_error from keeper_secrets_manager_core.utils import url_safe_str_to_bytes @@ -569,6 +570,65 @@ def link_user(self, user_uid, source_vertex: DAGVertex, is_admin=None, belongs_t user_vertex.belongs_to(source_vertex, EdgeType.ACL, content=content) self.linking_dag.save() + def link_saas_user(self, user_uid: str, saas_config_record: TypedRecord, pam_config_record_type: str) -> bool: + + logging.debug("linking saas user") + + if not self.linking_dag.has_graph: + logging.error("linking graph is empty") + return False + + configuration_vertex = self.linking_dag.get_root + if configuration_vertex is None: + logging.error("cannot find configuration vertex,") + return False + + user_vertex = self.linking_dag.get_vertex(user_uid) + if user_vertex is None: + logging.debug("creating vertex for user") + user_vertex = self.linking_dag.add_vertex(uid=user_uid, vertex_type=RefType.PAM_USER) + + acl_edge = user_vertex.get_edge(vertex=configuration_vertex, edge_type=EdgeType.ACL) + if acl_edge is not None: + logging.debug("have an existing ACL edge between the user and configuration") + acl = acl_edge.content_as_object(UserAcl) + else: + logging.debug("do NOT have an ACL edge between the user and configuration") + acl = UserAcl.default() + + plugin_field = saas_config_record.get_typed_field('text', 'SaaS Type') + if plugin_field is None: + logging.error("cannot get the plugin name from the SaaS configuration") + return False + + plugin_name = plugin_field.value[0] + + if acl is not None and acl.rotation_settings is None: + acl.rotation_settings = UserAclRotationSettings() + + logging.debug(f"plugin name is {plugin_name}") + logging.debug(f"pam configuration record type is {pam_config_record_type}") + + if plugin_name == "AWS Access Key" and pam_config_record_type == "pamAwsConfiguration": + logging.debug("pam configuration is AWS, the user belongs to the configuration") + acl.belongs_to = True + else: + acl.belongs_to = False + + acl.rotation_settings.noop = True + acl.is_iam_user = False + acl.is_admin = False + acl.rotation_settings.saas_record_uid_list = [saas_config_record.record_uid] + + user_vertex.belongs_to(vertex=configuration_vertex, + edge_type=EdgeType.ACL, + content=acl, + is_encrypted=False) + + self.linking_dag.save() + + return True + def get_all_admins(self): if not self.linking_dag.has_graph: return []