Skip to content

Add unused access key policy#917

Merged
ebattat merged 1 commit into
redhat-performance:mainfrom
ebattat:unused_access_key_policy
Jun 29, 2025
Merged

Add unused access key policy#917
ebattat merged 1 commit into
redhat-performance:mainfrom
ebattat:unused_access_key_policy

Conversation

@ebattat

@ebattat ebattat commented May 18, 2025

Copy link
Copy Markdown
Member

Type of change

Note: Fill x in []

  • bug
  • enhancement
  • documentation
  • dependencies

Description

Unused Access Key Policy:
Deactivate user access keys that are still active but have not been used in the last 90 days.
The threshold is defined by a constant parameter: UNUSED_ACCESS_KEY_DAYS = 90.
Solved Issue

For security reasons, all pull requests need to be approved first before running any automated CI

@ebattat ebattat requested a review from athiruma May 18, 2025 14:13
@ebattat ebattat self-assigned this May 18, 2025
@ebattat ebattat added the enhancement New feature or request label May 18, 2025
@ebattat ebattat added the ok-to-test PR ok to test label May 18, 2025
@ebattat ebattat force-pushed the unused_access_key_policy branch 2 times, most recently from c0cfdaa to f381188 Compare May 20, 2025 13:27
Comment on lines +268 to +299
def deactivate_user_access_keys(self, username):
"""
Deactivates all active access keys for the given IAM user.

Args:
username (str): IAM user name
"""
try:
access_keys = self.iam_client.list_access_keys(UserName=username)['AccessKeyMetadata']
except Exception as e:
logger.error(f"Failed to list access keys for user '{username}': {e}")
return

for idx, key in enumerate(access_keys, start=1):
label = f"Access key {idx}"
current_status = key['Status'].lower()
access_key_id = key['AccessKeyId']

if current_status == 'active':
try:
self.iam_client.update_access_key(
UserName=username,
AccessKeyId=access_key_id,
Status='Inactive'
)
logger.info(f"{label} deactivated for user '{username}'")
except Exception as e:
logger.error(f"Failed to deactivate {label} for user '{username}': {e}")
else:
logger.info(f"{label} is already inactive for user '{username}'")

logger.info(f"Deactivate access keys for user '{username}' have been processed.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to deactivate unused keys, not all keys. Her we should pass the inactive key_id to deactivate.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We deactivate it by resource_id=user_name.
You can find here the way we deactivate only specific user as other policies.
Pls let me know if it is ok.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, but if the user having multiple access_keys and one is recently used and one is used 90days back. We should delete the key that not using.

@ebattat ebattat May 29, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Lets have an example:
User1 -
AccessKey1 - Active - last used - 91 days
AccessKey2 - Active - last used - 27 days
We will deactivate only AccessKey1, and not change AccessKey2 according what we found here

My question is: do we pass to here only the suspected users that we found here

Comment on lines +19 to +32
for username, user_data in iam_users_access_keys.items():
last_activity_days = user_data['last_activity_days']
# Collect age_days only for active access keys
age_days_list = [
value[1] for key, value in user_data.items()
if key.startswith("Access key") and isinstance(value, list) and value[0] == "active"
]
# "N/A"/None implies unused access key — fallback to UNUSED_ACCESS_KEY_MAX_DAY
age_days = min(age_days_list) if age_days_list else UNUSED_ACCESS_KEY_MAX_DAY
# if access key last_activity_days is "N/A", use age_days
if last_activity_days == "N/A":
last_activity_days = age_days
region = user_data['region']
user_name = username

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
for username, user_data in iam_users_access_keys.items():
last_activity_days = user_data['last_activity_days']
# Collect age_days only for active access keys
age_days_list = [
value[1] for key, value in user_data.items()
if key.startswith("Access key") and isinstance(value, list) and value[0] == "active"
]
# "N/A"/None implies unused access key — fallback to UNUSED_ACCESS_KEY_MAX_DAY
age_days = min(age_days_list) if age_days_list else UNUSED_ACCESS_KEY_MAX_DAY
# if access key last_activity_days is "N/A", use age_days
if last_activity_days == "N/A":
last_activity_days = age_days
region = user_data['region']
user_name = username
for user_name, user_data in iam_users_access_keys.items():
last_activity_days = user_data['last_activity_days']
# Collect age_days only for active access keys
age_days_list = [
value[1] for key, value in user_data.items()
if key.startswith("Access key") and isinstance(value, list) and value[0] == "active"
]
# "N/A"/None implies unused access key — fallback to UNUSED_ACCESS_KEY_MAX_DAY
age_days = min(age_days_list) if age_days_list else UNUSED_ACCESS_KEY_MAX_DAY
# if access key last_activity_days is "N/A", use age_days
if last_activity_days == "N/A":
last_activity_days = age_days
region = user_data['region']

Comment on lines +61 to +62
elif self._policy == 'unused_access_key':
self._iam_operations.deactivate_user_access_keys(username=resource_id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we need to pass the de-activate key-id.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I pass the resource_id which contains the user_name with the unused key.
Here we collect all the suspected user_name.

@athiruma

Copy link
Copy Markdown
Contributor

Have you did the dry_run?

@ebattat

ebattat commented May 29, 2025

Copy link
Copy Markdown
Member Author

I run it locally and it works ok.
If we merge it, this policy will run as dry_run=yes in default, right ?

@athiruma

Copy link
Copy Markdown
Contributor

I run it locally and it works ok. If we merge it, this policy will run as dry_run=yes in default, right ?

ack, yes by default it will run dry_run=yes

@ebattat ebattat force-pushed the unused_access_key_policy branch from f381188 to 2479a33 Compare June 1, 2025 16:16
@ebattat

ebattat commented Jun 1, 2025

Copy link
Copy Markdown
Member Author

@athiruma,
I have updated the code based on your important comment.
Now, for each user, I have added an access_key_label to indicate "Access key 1" or "Access key 2."
By passing the access_key_label instead of the access_key_id, we keep our code more secure.
This means there are two checks per user, and each can trigger a different alert.
Additionally, we have added **kwargs to self._delete_resource(resource_id=resource_id, **kwargs) to allow passing more than one parameter—such as the user name and the access key label.

@athiruma athiruma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

/lgtm

@ebattat ebattat force-pushed the unused_access_key_policy branch from 2479a33 to eac1c68 Compare June 22, 2025 06:35
@ebattat ebattat force-pushed the unused_access_key_policy branch from eac1c68 to 8dc5064 Compare June 29, 2025 12:24
@ebattat ebattat merged commit 76260f6 into redhat-performance:main Jun 29, 2025
18 checks passed
@github-project-automation github-project-automation Bot moved this from In progress to Done in Cloud-Governance project Jun 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request ok-to-test PR ok to test

Projects

Development

Successfully merging this pull request may close these issues.

2 participants