Skip to content

Commit 64474ae

Browse files
committed
fix: address PR review — head_bucket and partial-creds validation
- Replace list_buckets() with head_bucket() in config_validator and health check; least-privilege IAM roles need only s3:ListBucket on the configured bucket, not s3:ListAllMyBuckets - Raise ValueError in make_client() when exactly one of S3_ACCESS_KEY / S3_SECRET_KEY is set — partial static credentials are a misconfiguration - Remove total_buckets from health check details (no longer available)
1 parent bf97b64 commit 64474ae

3 files changed

Lines changed: 23 additions & 19 deletions

File tree

src/config/s3.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,19 @@ def make_client(self) -> Any:
3737
``secret_key`` are set. When they are ``None``, boto3 falls through to
3838
its default credential chain (env vars, ``~/.aws/credentials``, EC2/ECS
3939
instance metadata).
40+
41+
Raises ``ValueError`` when exactly one of the pair is set — partial
42+
static config is always a misconfiguration.
4043
"""
41-
kwargs: dict[str, Any] = {"endpoint_url": self.endpoint_url, "region_name": self.region}
44+
if bool(self.access_key) != bool(self.secret_key):
45+
raise ValueError(
46+
"S3_ACCESS_KEY and S3_SECRET_KEY must both be set or both be unset. "
47+
"Partial static credentials are not supported."
48+
)
49+
kwargs: dict[str, Any] = {
50+
"endpoint_url": self.endpoint_url,
51+
"region_name": self.region,
52+
}
4253
if self.access_key and self.secret_key:
4354
kwargs["aws_access_key_id"] = self.access_key
4455
kwargs["aws_secret_access_key"] = self.secret_key

src/services/health.py

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -226,12 +226,8 @@ async def check_s3(self) -> HealthCheckResult:
226226
self._s3_client = settings.s3.make_client()
227227

228228
loop = asyncio.get_event_loop()
229-
buckets_resp = await loop.run_in_executor(
230-
None, self._s3_client.list_buckets
231-
)
232-
buckets = buckets_resp.get("Buckets", [])
233229

234-
# Check if our bucket exists
230+
# Check if our bucket exists; create it if not
235231
try:
236232
await loop.run_in_executor(
237233
None,
@@ -293,7 +289,6 @@ async def check_s3(self) -> HealthCheckResult:
293289
"endpoint": settings.s3_endpoint,
294290
"bucket": settings.s3_bucket,
295291
"bucket_exists": bucket_exists,
296-
"total_buckets": len(buckets),
297292
"secure": settings.s3_secure,
298293
}
299294

src/utils/config_validator.py

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -125,18 +125,16 @@ def _validate_s3_connection(self):
125125
try:
126126
client = settings.s3.make_client()
127127

128-
# Test connection by listing buckets
129-
response = client.list_buckets()
130-
buckets = response.get("Buckets", [])
131-
132-
# Check if our bucket exists
133-
bucket_exists = any(
134-
bucket["Name"] == settings.s3_bucket for bucket in buckets
135-
)
136-
if not bucket_exists:
137-
self.warnings.append(
138-
f"S3 bucket '{settings.s3_bucket}' does not exist - will be created"
139-
)
128+
try:
129+
client.head_bucket(Bucket=settings.s3_bucket)
130+
except ClientError as e:
131+
code = e.response["Error"]["Code"]
132+
if code in ("404", "NoSuchBucket"):
133+
self.warnings.append(
134+
f"S3 bucket '{settings.s3_bucket}' does not exist"
135+
)
136+
else:
137+
raise
140138

141139
except ClientError as e:
142140
if settings.api_debug:

0 commit comments

Comments
 (0)