feat:[PEA-2706] Refresh credentials implementation for data shipper - #2407
Conversation
Walkthrough
ChangesAWS credential provider migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AWSClient
participant CredentialProvider
participant AWSSTS
AWSClient->>CredentialProvider: getCredentials(account, roleName)
CredentialProvider->>AWSSTS: build STS assume-role provider
AWSSTS-->>CredentialProvider: AWSCredentialsProvider
CredentialProvider-->>AWSClient: configure AWS client
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/config/CredentialProvider.java`:
- Line 72: Update the STS client creation in CredentialProvider to configure its
region from the existing base.region value instead of using
AWSSecurityTokenServiceClientBuilder.defaultClient(). Preserve the existing
credential refresh and AssumeRole flow while ensuring the client explicitly
targets base.region.
In
`@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java`:
- Line 45: Close request-scoped AWS credential and client resources after each
workload. In
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java:45,
retain the STS provider and local S3/STS clients in scope, process the workload,
then close and shut them down. In
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/DynamoDBHelper.java:37,
retain the provider and close it in finally alongside client.shutdown(); apply
equivalent lifecycle handling in LambdaInvoker if its provider remains
request-scoped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86415b29-5e55-40e8-b652-9100b03b753c
📒 Files selected for processing (5)
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/config/CredentialProvider.javajobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/config/S3ClientConfig.javajobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.javajobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/DynamoDBHelper.javajobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/LambdaInvoker.java
| .withStsClient(sts) | ||
| .build(); | ||
| } else { | ||
| AWSSecurityTokenService sts = AWSSecurityTokenServiceClientBuilder.defaultClient(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether deployment and application configuration provide an AWS SDK
# default-region setting in addition to the base.region system property.
rg -n -C2 'base\.region|AWS_REGION|aws\.region|AWS_DEFAULT_REGION' .Repository: PaladinCloud/CE
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files in jobs/pacman-data-shipper =="
git ls-files jobs/pacman-data-shipper | sed -n '1,200p'
echo
echo "== credential provider outline && contents =="
sed -n '1,220p' jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/config/CredentialProvider.java
echo
echo "== dependency/version references (focused) =="
rg -n -C2 'AWSSecurityTokenService|AWSSecurityTokenServiceClientBuilder|sts|base\.region|AWS_REGION|aws\.region|AWS_DEFAULT_REGION|SYSTEM_PROPERTY|Property' jobs/pacman-data-shipper --glob '*.java' --glob '*.yml' --glob '*.yaml' --glob '*properties'
echo
echo "== build config =="
for f in jobs/pacman-data-shipper/pom.xml jobs/pacman-data-shipper/build.gradle jobs/pacman-data-shipper/build.gradle.kts; do
test -f "$f" && { echo "--- $f"; sed -n '1,220p' "$f"; }
done
echo
echo "== all AWS region env/system mapping in pacman-data-shipper =="
rg -n -C2 'base\.region|AWS_REGION|aws\.region|AWS_DEFAULT_REGION|System\.getProperty\("region"|System\.(getenv|getenv)|getenv\("region"|getenv\("aws' jobs/pacman-data-shipperRepository: PaladinCloud/CE
Length of output: 50371
🌐 Web query:
AWS Java SDK 1 x AWSSecurityTokenServiceClientBuilder defaultClient region environment variable AWS_DEFAULT_REGION
💡 Result:
In the AWS SDK for Java 1.x, the AWSSecurityTokenServiceClientBuilder.defaultClient method uses the DefaultAwsRegionProviderChain to determine the AWS region [1][2][3]. This provider chain does not check the AWS_DEFAULT_REGION environment variable by default; it specifically prioritizes the AWS_REGION environment variable [4][5][6]. The standard region lookup process used by the SDK for Java 1.x builder is: 1. Explicit region configuration set via the builder (e.g.,.withRegion) [4][5]. 2. The AWS_REGION environment variable [4][5]. 3. The AWS shared configuration file (typically ~/.aws/config) [4][5]. 4. The Amazon EC2 instance metadata service [4][5]. Because AWS_DEFAULT_REGION is not part of this specific provider chain for the Java 1.x SDK, it will be ignored if AWS_REGION is not set. If you require the SDK to recognize AWS_DEFAULT_REGION, you would typically need to manually configure the client or implement a custom region provider, as the built-in default chain is explicitly defined to use AWS_REGION [5][7][6]. Note that the AWS SDK for Java 1.x reached end-of-support on December 31, 2025, and users are encouraged to migrate to the AWS SDK for Java 2.x [8].
Citations:
- 1: https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-sts/src/main/java/com/amazonaws/services/securitytoken/AWSSecurityTokenServiceClientBuilder.java
- 2: https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/services/securitytoken/AWSSecurityTokenServiceClientBuilder.html
- 3: https://javadoc.io/static/com.amazonaws/aws-java-sdk-osgi/1.11.881/com/amazonaws/services/securitytoken/AWSSecurityTokenServiceClientBuilder.html
- 4: https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/java-dg-region-selection.html
- 5: https://docs.aws.amazon.com/pdfs/sdk-for-java/v1/developer-guide/aws-sdk-java-dg-v1.pdf
- 6: https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/SDKGlobalConfiguration.html
- 7: https://javadoc.io/static/com.amazonaws/aws-java-sdk-core/1.11.266/com/amazonaws/SDKGlobalConfiguration.html
- 8: https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/creating-clients.html
Set the STS client region from base.region.
AWSSecurityTokenServiceClientBuilder.defaultClient() does not read base.region and uses AWS_REGION instead. Since this job only applies base.region, the base-account credential refresh path can pick any other AWS region for STS and fail on AssumeRole.
Proposed fix
- AWSSecurityTokenService sts = AWSSecurityTokenServiceClientBuilder.defaultClient();
+ AWSSecurityTokenService sts = AWSSecurityTokenServiceClientBuilder.standard()
+ .withRegion(baseRegion)
+ .build();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/config/CredentialProvider.java`
at line 72, Update the STS client creation in CredentialProvider to configure
its region from the existing base.region value instead of using
AWSSecurityTokenServiceClientBuilder.defaultClient(). Preserve the existing
credential refresh and AssumeRole flow while ensuring the client explicitly
targets base.region.
| List<Map<String, String>> errorList = new ArrayList<>(); | ||
| AmazonS3 s3Client = AmazonS3ClientBuilder.standard().withCredentials( | ||
| new AWSStaticCredentialsProvider(new CredentialProvider().getCredentials(S3_ACCOUNT, S3_ROLE))).withRegion(S3_REGION).build(); | ||
| (new CredentialProvider().getCredentials(S3_ACCOUNT, S3_ROLE))).withRegion(S3_REGION).build(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate refreshable provider construction and lifecycle handling.
rg -n -C3 \
'STSAssumeRoleSessionCredentialsProvider|new CredentialProvider\(\)\.getCredentials|\.close\(\)|\.shutdown\(\)' \
jobs/pacman-data-shipper/src/main/javaRepository: PaladinCloud/CE
Length of output: 8530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
fd -a 'VulnerabilityAssociationManager.java|DynamoDBHelper.java|CredentialProvider.java|LambdaInvoker.java' jobs/pacman-data-shipper/src/main/java || true
echo
echo "== Build/config for AWS SDK version =="
fd -a 'pom.xml|build.gradle|build.gradle.kts|settings.gradle|settings.gradle.kts' jobs/pacman-data-sdk-shipper . | sed 's#^\./##' || true
rg -n -C3 'aws-java-sdk|software.amazon.awssdk|dependency: aws-java-sdk|sdk-bom|version\.aws|<aws' -S . || true
echo
echo "== File contents =="
for f in \
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java \
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/DynamoDBHelper.java \
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/config/CredentialProvider.java \
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman(datashipper/util/LambdaInvoker.java
do
echo "--- $f ---"
wc -l "$f"
cat -n "$f" || true
doneRepository: PaladinCloud/CE
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pom relevant dependencies =="
sed -n '1,120p' jobs/pacman-data-shipper/pom.xml
echo
echo "== target files =="
for f in \
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java \
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/DynamoDBHelper.java \
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/config/CredentialProvider.java \
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/LambdaInvoker.java
do
echo "--- $f ---"
wc -l "$f"
cat -n "$f"
done
echo
echo "== AWS SDK 1.11.490 STSAssumeRoleSessionCredentialsProvider definitions from jars/remote =="
python3 - <<'PY'
import json, urllib.request, tempfile, zipfile, os, re, pathlib
urls = [
"https://repo1.maven.org/maven2/com/amazonaws/aws-java-sdk-sts/1.11.490/aws-java-sdk-sts-1.11.490.jar",
"https://repo1.maven.org/maven2/com/amazonaws/aws-java-sdk-parent/1.11.490/aws-java-sdk-parent-1.11.490.pom",
]
for url in urls:
print(f"FETCH {url}")
with urllib.request.urlopen(url, timeout=20) as r:
data = r.read()
if url.endswith(".jar"):
with tempfile.NamedTemporaryFile(suffix=".jar") as tf:
tf.write(data)
tf.flush()
with zipfile.ZipFile(tf.name) as z:
names = [n for n in z.namelist() if "STSAssumeRoleSessionCredentialsProvider" in n or "CredentialsProviderUtil" in n or "Refreshable" in n]
print("entries:", names)
for name in names[:12]:
text = z.read(name).decode("utf-8", errors="replace")
lines = text.splitlines()
for i,l in enumerate(lines[:120], 1):
if any(kw in l for kw in ["close", "shutdown", "new STSSecurityTokenService", "AWSSecurityTokenService", "STSAssumeRoleSessionCredentialsProvider", "closeStsClient"]):
print(f"{name}:{i}: {l}")
PYRepository: PaladinCloud/CE
Length of output: 26406
🌐 Web query:
AWS SDK 1.11 STSAssumeRoleSessionCredentialsProvider close STS client client.close does not close provider credentials provider
💡 Result:
In the AWS SDK for Java 1.11, calling the close method on an STSAssumeRoleSessionCredentialsProvider does not explicitly close the underlying STS client [1][2]. The close method is primarily responsible for shutting down the background thread used for asynchronous credential refreshing [1][3]. The STS client used by this provider is typically managed separately by the user, especially if it was passed into the provider's builder [2]. If you created an STS client specifically to use with the credentials provider, you must manage its lifecycle independently and call its close method when it is no longer required [1][2]. The SDK does not automatically close resources such as custom ExecutorServices or the STS client itself upon closing the credentials provider [1][2]. In summary: 1. The close method on STSAssumeRoleSessionCredentialsProvider terminates the background refresh thread [1][4]. 2. You must manually close the STS client instance used by the provider if it is no longer needed [2]. 3. Similarly, if you provided a custom ExecutorService to the provider, you are responsible for shutting it down [1][2].
Citations:
- 1: https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/auth/STSAssumeRoleSessionCredentialsProvider.html
- 2: https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-sts/src/main/java/com/amazonaws/auth/STSAssumeRoleSessionCredentialsProvider.java
- 3: https://javadoc.io/static/com.amazonaws/aws-java-sdk-osgi/1.11.881/com/amazonaws/auth/STSAssumeRoleSessionCredentialsProvider.html
- 4: Questions about the behavior of STSAssumeRoleSessionCredentialsProvider aws/aws-sdk-java#2918
Close STS credential sessions after each request-scoped AWS client.
CredentialProvider returns STSAssumeRoleSessionCredentialsProvider instances backed by STS clients created per call. AWS service clients do not close these session providers/STS clients, so repeated calls can leave credential-refresh tasks or STS clients allocated.
VulnerabilityAssociationManager: hold the provider and STS client in request-scoped variables, process the S3 workload, then close the provider and shut down the local S3/STS resources.DynamoDBHelper: keep the provider in scope and close it infinallywithprovider.close(); client.shutdown();.LambdaInvoker: if this path remains requestscoped, apply the same provider/STS-resource lifecycle, or move to an application-scoped provider with controlled shutdown.
📍 Affects 2 files
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java#L45-L45(this comment)jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/DynamoDBHelper.java#L37-L37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java`
at line 45, Close request-scoped AWS credential and client resources after each
workload. In
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/entity/VulnerabilityAssociationManager.java:45,
retain the STS provider and local S3/STS clients in scope, process the workload,
then close and shut them down. In
jobs/pacman-data-shipper/src/main/java/com/tmobile/cso/pacman/datashipper/util/DynamoDBHelper.java:37,
retain the provider and close it in finally alongside client.shutdown(); apply
equivalent lifecycle handling in LambdaInvoker if its provider remains
request-scoped.
2 participants
Description
Please include a summary of the changes and the related issues. Please also include relevant motivation and context. List
any dependencies that are required for this change.
Problem
Solution
Fixes # (issue if any)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also
list any relevant details for your test configuration
Checklist:
Other Information:
List any documentation updates that are needed for the Wiki
Summary by CodeRabbit