Skip to content

Tag managed secrets to prevent cleanup of unmanaged secrets #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paul-watkins-croud wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Tag managed secrets to prevent cleanup of unmanaged secrets #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# CLAUDE.md

This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository.

## Project Overview

`croudtech-bootstrap` is a CLI tool for pushing and pulling application configuration (secrets, S3 config, SSM parameters) to/from AWS services. It uses AWS S3 for config storage, AWS SSM Parameter Store for parameters, and AWS Secrets Manager for secrets. It also manages Redis database allocation across environments/apps.

## Build & Development

- **Package manager**: Poetry (`pyproject.toml`)
- **Python**: >=3.9, <3.14
- **Install deps**: `poetry install`
- **CLI entry point**: `croudtech-bootstrap` (mapped to `croudtech_bootstrap_app.cli:cli`)

## Testing

- **Run all tests**: `py.test --disable-warnings` or `make test`
- **Run a single test**: `py.test croudtech_bootstrap_app/tests/bootstrap_test.py::test_manager`
- **Test config**: `setup.cfg` under `[tool:pytest]` — test discovery looks in `croudtech_bootstrap_app/` for files matching `test_*.py`, `tests/*.py`, `tests.py`
- **Test fixtures**: Tests use local YAML files in `croudtech_bootstrap_app/tests/test_values/` to test config parsing without AWS

## Linting

- **Flake8**: `flake8` (config in `setup.cfg` — ignores E203, E501, W503; max line length 88)
- **isort**: `isort --recursive --check-only -p . --diff` (fix with `isort --recursive -p .`)
- **Black**: configured at line length 88
- **Run all quality checks**: `make quality`

## Architecture

The core domain is in `croudtech_bootstrap_app/bootstrap.py` with four main classes:

- **`BootstrapManager`** — Top-level orchestrator. Holds AWS clients (S3, SSM, Secrets Manager), discovers environments from a directory structure, and coordinates push/pull/cleanup operations.
- **`BootstrapEnvironment`** — Represents a single environment (e.g., staging, production). Discovered from subdirectories of the values path. Contains multiple `BootstrapApp` instances.
- **`BootstrapApp`** — Represents one application's config within an environment. Handles reading local YAML files (`.yaml` for params, `.secret.yaml` for secrets), uploading to S3, pushing to SSM/Secrets Manager, fetching remote values, and cleaning up orphaned parameters/secrets.
- **`BootstrapParameters`** — Facade for pulling config. Merges app-specific and common params, handles Redis DB auto-allocation, and formats output as env vars.

**CLI** (`cli.py`): Click-based CLI with commands `init`, `get-config`, `put-config`, `cleanup-secrets`, `list-apps`, and a `manage-redis` subgroup (`show-db`, `show-dbs`, `allocate-db`, `deallocate-db`).

**Redis allocation** (`redis_config.py`): Uses Redis DB 15 as a config database to track which DB index (0-14) is allocated to which app/environment combination.

**Config file convention**: Values directories contain per-environment subdirectories, each with `<AppName>.yaml`, `<AppName>.secret.yaml`, `common.yaml`, and `common.secret.yaml`. Nested YAML keys are flattened with `_` separators.

## Key Details

- Default AWS region is `eu-west-2`
- `AWS_ENDPOINT_URL` env var overrides AWS endpoints (for local testing with LocalStack etc.)
- S3 bucket naming convention: `app-bootstrap-{AWS_ACCOUNT_ID}`
- Log level controlled by `LOG_LEVEL` env var (defaults to CRITICAL)
- Empty secret values are stored as `__EMPTY__` in Secrets Manager and converted back on retrieval
- SSM parameters larger than 4096 bytes or empty are skipped during push
- Retry logic uses exponential backoff for AWS API throttling
4 changes: 2 additions & 2 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ jobs:
name: set_python_version
displayName: Set Python Version
inputs:
versionSpec: '$(python_version)'
versionSpec: "$(python_version)"
addToPath: true
architecture: x64
- task: gitversion/setup@0
Expand All @@ -96,7 +96,7 @@ jobs:
export PATH=$PATH:/home/vsts_azpcontainer/.local/bin
pip install pipx && pipx install poetry
poetry version $(GitVersion.SemVer)
poetry install
poetry install
poetry build
poetry publish
cp $(Pipeline.Workspace)/croudtech-bootstrap/croudtech-bootstrap $(Build.ArtifactStagingDirectory)
Expand Down
48 changes: 45 additions & 3 deletions croudtech_bootstrap_app/bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,18 +269,18 @@

def cleanup_secrets(self):
local_secret_keys = self.convert_flatten(self.local_secrets).keys()
remote_secret_keys = self.remote_secret_records.keys()
managed_secret_records = self.get_managed_secret_records()

orphaned_secrets = [
item for item in remote_secret_keys if re.sub(r"(-[a-zA-Z]{6})$", "", item) not in local_secret_keys
item for item in managed_secret_records.keys() if re.sub(r"(-[a-zA-Z]{6})$", "", item) not in local_secret_keys
]

for secret in orphaned_secrets:
secret_record = self.remote_secrets[secret]
secret_record = managed_secret_records[secret]
self.secrets_client.delete_secret(
SecretId=secret_record["ARN"], ForceDeleteWithoutRecovery=True
)
logger.info(f"Deleted orphaned secret {secret_record['ARN']}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (secret)
Loading
as clear text.
This expression logs
sensitive data (secret)
Loading
as clear text.

@property
def local_secrets(self) -> typing.Dict[str, Any]:
Expand Down Expand Up @@ -383,6 +383,8 @@
Tags=tags
)

MANAGED_BY_TAG = {"Key": "ManagedBy", "Value": "croudtech-bootstrap"}

def create_secret(self, Name, SecretString, Tags, ForceOverwriteReplicaSecret):
print(f"Creating Secret {Name}")
try:
Expand All @@ -392,6 +394,7 @@
Tags=[
{"Key": "Environment", "Value": self.environment.name},
{"Key": "App", "Value": self.name},
self.MANAGED_BY_TAG,
],
ForceOverwriteReplicaSecret=True,
)
Expand All @@ -400,6 +403,21 @@
SecretId=Name,
SecretString=SecretString,
)
self._ensure_managed_by_tag(Name)

def _ensure_managed_by_tag(self, secret_id):
"""Ensure the ManagedBy tag is present on an existing secret."""
try:
response = self.secrets_client.describe_secret(SecretId=secret_id)
tags = response.get("Tags", [])
has_managed_by = any(t["Key"] == "ManagedBy" for t in tags)
if not has_managed_by:
self.secrets_client.tag_resource(
SecretId=secret_id,
Tags=[self.MANAGED_BY_TAG],
)
except Exception:
logger.debug(f"Could not ensure ManagedBy tag on {secret_id}")

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (secret)
Loading
as clear text.
This expression logs
sensitive data (secret)
Loading
as clear text.
This expression logs
sensitive data (secret)
Loading
as clear text.
This expression logs
sensitive data (secret)
Loading
as clear text.

Copilot Autofix

AI 4 days ago

In general, to fix clear-text logging of sensitive data, you should avoid including sensitive values (or their directly identifying metadata) in log messages. If some context is needed for troubleshooting, prefer high-level descriptors (like environment or app name) or redacted/hashed versions that do not allow reconstructing or directly identifying the secret.

In this specific case, the vulnerable sink is logger.debug(f"Could not ensure ManagedBy tag on {secret_id}") in _ensure_managed_by_tag. The simplest, least intrusive fix is to stop interpolating secret_id into the log message. We can still log that something went wrong, potentially including non-sensitive context such as the exception itself (which may already be logged elsewhere) or a generic message. To keep behavior close to current functionality, we’ll change the message to drop the secret identifier and optionally log the exception with logger.debug("Could not ensure ManagedBy tag on a secret.", exc_info=True) so stack trace is available when debug logs are enabled. This avoids logging the tainted secret_id while maintaining useful diagnostics.

Concretely:

  • In croudtech_bootstrap_app/bootstrap.py, in _ensure_managed_by_tag, replace the except block’s logging line to not reference secret_id.
  • No new helper methods are required; we just modify the log message.
  • No new imports are needed, as logger is already initialized and logging is already imported.
Suggested changeset 1
croudtech_bootstrap_app/bootstrap.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/croudtech_bootstrap_app/bootstrap.py b/croudtech_bootstrap_app/bootstrap.py
--- a/croudtech_bootstrap_app/bootstrap.py
+++ b/croudtech_bootstrap_app/bootstrap.py
@@ -417,7 +417,7 @@
                     Tags=[self.MANAGED_BY_TAG],
                 )
         except Exception:
-            logger.debug(f"Could not ensure ManagedBy tag on {secret_id}")
+            logger.debug("Could not ensure ManagedBy tag on a secret.", exc_info=True)
 
     def backoff_with_custom_exception(self, func, exception, message_prefix="", max_attempts=5, base_delay=1, max_delay=10, factor=2, *args, **kwargs):
         attempts = 0
EOF
@@ -417,7 +417,7 @@
Tags=[self.MANAGED_BY_TAG],
)
except Exception:
logger.debug(f"Could not ensure ManagedBy tag on {secret_id}")
logger.debug("Could not ensure ManagedBy tag on a secret.", exc_info=True)

def backoff_with_custom_exception(self, func, exception, message_prefix="", max_attempts=5, base_delay=1, max_delay=10, factor=2, *args, **kwargs):
attempts = 0
Copilot is powered by AI and may make mistakes. Always verify output.

def backoff_with_custom_exception(self, func, exception, message_prefix="", max_attempts=5, base_delay=1, max_delay=10, factor=2, *args, **kwargs):
attempts = 0
Expand Down Expand Up @@ -502,6 +520,17 @@
{"Key": "tag-value", "Values": [self.name]},
]

@property
def managed_secret_filters(self):
return [
{"Key": "tag-key", "Values": ["Environment"]},
{"Key": "tag-value", "Values": [self.environment.name]},
{"Key": "tag-key", "Values": ["App"]},
{"Key": "tag-value", "Values": [self.name]},
{"Key": "tag-key", "Values": ["ManagedBy"]},
{"Key": "tag-value", "Values": ["croudtech-bootstrap"]},
]

def get_remote_ssm_parameters(self):
paginator = self.ssm_client.get_paginator('describe_parameters')
parameters = {}
Expand Down Expand Up @@ -542,6 +571,19 @@

return secrets

def get_managed_secret_records(self):
paginator = self.secrets_client.get_paginator("list_secrets")
secrets = {}
response = paginator.paginate(
Filters=self.managed_secret_filters,
)
for page in response:
for secret in page["SecretList"]:
secret_key = os.path.split(secret["Name"])[-1]
secrets[secret_key] = secret

return secrets

def convert_flatten(self, d, parent_key="", sep="_"):
items = []
if isinstance(d, dict):
Expand Down
Loading