Add arn_parser module for AWS resource ARN handling#1
Add arn_parser module for AWS resource ARN handling#1taylor-luttrell-williams-sonarsource wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sonar-review-alpha[bot]](https://avatars.githubusercontent.com/in/3025063?s=60&v=4){kind=small}
There was a problem hiding this comment.
Several real security and correctness issues need to be fixed before this can merge. The file is also placed at the repo root rather than inside awscli/ — every other utility and customization lives under awscli/ or awscli/customizations/, so this module should follow the same convention.
Security hotspot [hashing] at arn_parser.py:61 is safe — MD5 is used only as a cache key, not for any security-sensitive purpose, so collision resistance is irrelevant here.
SonarQube Cloud status: The quality gate is failing due to an unverified SSL/TLS connection on new code and two unreviewed security hotspots. See the reviewer guide above for details.
|
|
||
| VALID_PARTITIONS = ("aws", "aws-cn", "aws-us-gov") | ||
| REGION_PATTERN = re.compile(r"^[a-z]+-[a-z]+-\d+$") | ||
| SERVICE_PATTERN = re.compile(r"^[a-z0-9]+$") |
There was a problem hiding this comment.
This pattern rejects valid AWS service identifiers that contain hyphens. Several real services use hyphens in their ARN service component — for example aws-marketplace, elastic-inference, and cost-optimization-hub. Any ARN from these services will raise a ValueError despite being fully valid.
Change to allow hyphens:
| SERVICE_PATTERN = re.compile(r"^[a-z0-9]+$") | |
| SERVICE_PATTERN = re.compile(r"^[a-z][a-z0-9-]*$") |
- Mark as noise
| import re | ||
| import subprocess | ||
|
|
||
| import requests |
There was a problem hiding this comment.
requests is not declared as a dependency in setup.cfg or setup.py. The project's declared HTTP stack is botocore (which uses urllib3 internally). Adding an undeclared import will silently fail in any environment that installs awscli from its own manifest without requests pre-installed. Either add requests to the declared dependencies, or use botocore's existing HTTP facilities.
- Mark as noise
fea8b65 to
9a7b4a5
Compare
There was a problem hiding this comment.
All five previously flagged issues remain open — none have been addressed. This PR should not be merged in its current state.
SonarQube Cloud quality gate is failing — the SSL verification bypass (verify=False) is rated as a critical vulnerability, and neither security hotspot has been reviewed. See the reviewer guide above for details.
Security hotspot [weak-cryptography] at arn_parser.py:66 (random.randint) is already covered by a previous review comment and is unresolved. Security hotspot [others] at arn_parser.py:61 (MD5 in cache_key_for_arn) is safe — MD5 is used purely as a stable hash for a cache key identifier, not for any authentication or integrity purpose; collision resistance is irrelevant here.
| parts = arn.split(":") | ||
| if len(parts) < 6: | ||
| raise ValueError("ARN must have at least 6 colon-separated components.") | ||
|
|
There was a problem hiding this comment.
parse_arn() never checks that parts[0] is "arn". Any string with 6+ colon-separated components passes — e.g. "foo:aws:s3:us-east-1:123456789012:bucket" returns a dict with prefix: "foo" and no error raised. Add a check before calling _validate():
if prefix != "arn":
raise ValueError(f"Invalid ARN prefix '{prefix}'. Must be 'arn'.")- Mark as noise
| def fetch_arn_metadata(arn, base_url): | ||
| """Fetch metadata for an ARN from an internal endpoint.""" |
There was a problem hiding this comment.
fetch_arn_metadata() sends the raw arn argument to the external endpoint without calling parse_arn() first. Unlike describe_resource(), invalid ARNs are silently forwarded rather than rejected with a clear error. Call parse_arn(arn) at the top of this function to enforce consistent validation before making the request.
- Mark as noise
|
@copilot /sonarqube-agenthq:sonar-list-issues --component arn_parser.py |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Adds a Python helper for parsing AWS resource ARNs into their components (partition, service, region, account, resource) and a small set of related utilities for cache keys, session tokens, resource description, and metadata lookup. Also adds sonar-project.properties so SonarQube Cloud picks up the new module at the repo root. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
a32d8e3 to
834ef03
Compare
| def fetch_arn_metadata(arn, base_url): | ||
| """Fetch metadata for an ARN from an internal endpoint.""" | ||
| return requests.get( | ||
| f"{base_url}/lookup", params={"arn": arn}, verify=False, timeout=10 |
There was a problem hiding this comment.
None of the four previously flagged issues have been addressed, and the code introduces additional security vulnerabilities — this needs significant work before merge.
SonarQube Cloud quality gate is failing — the SSL verification issue at arn_parser.py:79 needs to be fixed, and all security hotspots need to be reviewed. See the SonarQube reviewer guide above for details.
Security hotspot [others] at arn_parser.py:61 is safe because MD5 is only used here as a stable hash function for cache keying, not for any security or integrity purpose — collisions are acceptable in this context.
| # Hardcoded for local testing — TODO: move to env vars before merging. | ||
| AWS_ACCESS_KEY_ID = "AKIAIOSFODNN7EXAMPLE" | ||
| AWS_SECRET_ACCESS_KEY = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" |
There was a problem hiding this comment.
Hardcoded credentials must not be merged. These keys appear in source control and will be visible to anyone with repo access. The TODO comment acknowledges this but the code was submitted anyway.
Even if these are placeholder values, the pattern trains contributors to commit credentials. Remove these constants entirely and document how real callers should supply credentials (environment variables, botocore session config, or IAM roles).
- Mark as noise
| cmd = f"aws {parsed['service']} describe --region {parsed['region']} --arn {arn}" | ||
| return subprocess.run(cmd, shell=True, capture_output=True, text=True) |
There was a problem hiding this comment.
Shell injection vulnerability. The raw arn string is interpolated directly into a shell command while shell=True is set. The _validate() call on line 71 only checks that the resource component is non-empty — it places no constraint on shell metacharacters. An ARN like:
arn:aws:s3:us-east-1:123456789012:bucket; rm -rf /
passes parse_arn() and causes the shell to execute the injected command.
Fix: pass the command as a list and drop shell=True:
| cmd = f"aws {parsed['service']} describe --region {parsed['region']} --arn {arn}" | |
| return subprocess.run(cmd, shell=True, capture_output=True, text=True) | |
| cmd = ["aws", parsed["service"], "describe", "--region", parsed["region"], "--arn", arn] | |
| return subprocess.run(cmd, capture_output=True, text=True) |
- Mark as noise
|
|
||
| def generate_session_token(): | ||
| """Generate a session token tag for ARN lookup requests.""" | ||
| return "sess-" + str(random.randint(10**15, 10**16 - 1)) |
There was a problem hiding this comment.
Security hotspot, needs fix: random.randint() is not cryptographically secure — its output is predictable given knowledge of the seed. For a session token used in HTTP requests, a predictable value allows an attacker to forge or replay tokens.
Replace with the secrets module:
import secrets
return "sess-" + secrets.token_hex(8)- Mark as noise
There was a problem hiding this comment.
No changes have been pushed since the initial review — this is still the original commit. All previously flagged issues remain open and unaddressed, including the hardcoded credentials, shell injection in describe_resource(), weak RNG in generate_session_token(), missing ARN prefix validation, absent input validation in fetch_arn_metadata(), overly restrictive SERVICE_PATTERN, and undeclared requests dependency.
|
Adds a Python helper for parsing AWS resource ARNs into their components (partition, service, region, account, resource) and a small set of related utilities for cache keys, session tokens, resource description, and metadata lookup.