Skip to content

Commit 8e7aa78

Browse files
patrickmannclaude
andcommitted
Add AWSAuthFactory.createCloseable to release the assume-role STS client
buildStsCredentialsProvider hands a fresh StsClient to StsAssumeRoleCredentialsProvider, which never closes it: StsCredentialsProvider.close() only clears the session cache. A caller that builds a provider per operation therefore leaks the STS client's Apache connection pool on every call. Add createCloseable(...), returning a CredentialsProviderHandle that bundles the provider with its STS client so a caller can close both. The existing create(...) overloads delegate to it and discard the handle, so their behaviour is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 6682cb8 commit 8e7aa78

2 files changed

Lines changed: 110 additions & 6 deletions

File tree

graylog2-server/src/main/java/org/graylog/integrations/aws/AWSAuthFactory.java

Lines changed: 64 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,11 @@
3030
import software.amazon.awssdk.services.sts.StsClient;
3131
import software.amazon.awssdk.services.sts.StsClientBuilder;
3232
import software.amazon.awssdk.services.sts.auth.StsAssumeRoleCredentialsProvider;
33+
import software.amazon.awssdk.services.sts.auth.StsCredentialsProvider;
3334
import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
3435
import software.amazon.awssdk.services.sts.model.GetCallerIdentityRequest;
36+
import software.amazon.awssdk.utils.IoUtils;
37+
import software.amazon.awssdk.utils.SdkAutoCloseable;
3538

3639
import javax.annotation.Nullable;
3740
import java.util.Locale;
@@ -96,6 +99,29 @@ public AwsCredentialsProvider create(boolean requireKeySecret,
9699
@Nullable String assumeRoleArn,
97100
@Nullable String externalId,
98101
@Nullable ApacheHttpClient.Builder stsHttpClientBuilder) {
102+
// Discards the STS client handle. Callers on this overload cannot close the STS client backing an
103+
// assume-role provider, so its connection pool leaks for the process lifetime; see createCloseable.
104+
return createCloseable(requireKeySecret, stsRegion, accessKey, secretKey, assumeRoleArn, externalId, stsHttpClientBuilder)
105+
.provider();
106+
}
107+
108+
/**
109+
* Same resolution as {@link #create(boolean, String, String, String, String, String, ApacheHttpClient.Builder)},
110+
* but returns a {@link CredentialsProviderHandle} so the caller can close the STS client backing an assume-role
111+
* provider.
112+
*
113+
* <p>For an assume-role configuration the STS client is created here and handed to
114+
* {@link StsAssumeRoleCredentialsProvider}, which never closes it: {@link StsCredentialsProvider#close()} only
115+
* clears the session cache. Callers that build a fresh provider per operation must therefore close the handle,
116+
* or the STS client's Apache connection pool leaks on every call.
117+
*/
118+
public CredentialsProviderHandle createCloseable(boolean requireKeySecret,
119+
@Nullable String stsRegion,
120+
@Nullable String accessKey,
121+
@Nullable String secretKey,
122+
@Nullable String assumeRoleArn,
123+
@Nullable String externalId,
124+
@Nullable ApacheHttpClient.Builder stsHttpClientBuilder) {
99125
AwsCredentialsProvider awsCredentials = requireKeySecret ? getKeySecretCredentialsProvider(accessKey, secretKey) :
100126
getAwsCredentialsProvider(accessKey, secretKey);
101127

@@ -109,7 +135,7 @@ public AwsCredentialsProvider create(boolean requireKeySecret,
109135
return buildStsCredentialsProvider(awsCredentials, stsRegion, assumeRoleArn, accessKey, externalId, stsHttpClientBuilder);
110136
}
111137

112-
return awsCredentials;
138+
return new CredentialsProviderHandle(awsCredentials, null);
113139
}
114140

115141
private static AwsCredentialsProvider getAwsCredentialsProvider(String accessKey, String secretKey) {
@@ -135,10 +161,10 @@ private static AwsCredentialsProvider getKeySecretCredentialsProvider(String acc
135161
* Note: In order to assume a role, a role must be provided to the AWS STS client a role that has the "sts:AssumeRole"
136162
* permission, which provides authorization for a role to be assumed.
137163
*/
138-
private static AwsCredentialsProvider buildStsCredentialsProvider(AwsCredentialsProvider awsCredentials, String stsRegion,
139-
String assumeRoleArn, @Nullable String accessKey,
140-
@Nullable String externalId,
141-
@Nullable ApacheHttpClient.Builder stsHttpClientBuilder) {
164+
private static CredentialsProviderHandle buildStsCredentialsProvider(AwsCredentialsProvider awsCredentials, String stsRegion,
165+
String assumeRoleArn, @Nullable String accessKey,
166+
@Nullable String externalId,
167+
@Nullable ApacheHttpClient.Builder stsHttpClientBuilder) {
142168

143169
final StsClientBuilder stsClientBuilder = StsClient.builder()
144170
.region(Region.of(stsRegion))
@@ -166,9 +192,41 @@ private static AwsCredentialsProvider buildStsCredentialsProvider(AwsCredentials
166192
if (!isNullOrEmpty(externalId)) {
167193
assumeRoleRequestBuilder.externalId(externalId);
168194
}
169-
return StsAssumeRoleCredentialsProvider.builder()
195+
final StsAssumeRoleCredentialsProvider provider = StsAssumeRoleCredentialsProvider.builder()
170196
.refreshRequest(assumeRoleRequestBuilder.build())
171197
.stsClient(stsClient)
172198
.build();
199+
return new CredentialsProviderHandle(provider, stsClient);
200+
}
201+
202+
/**
203+
* A resolved {@link AwsCredentialsProvider} paired with the STS client backing an assume-role provider, if any.
204+
*
205+
* <p>{@link StsCredentialsProvider#close()} only clears the provider's session cache; the {@link StsClient} it
206+
* was handed is caller-supplied and {@code final}, so it is never closed and its Apache connection pool leaks.
207+
* This handle owns both resources so a caller can release them together. {@link #close()} closes the credentials
208+
* provider first -- stopping its background session refresh -- and then the STS client.
209+
*/
210+
public static final class CredentialsProviderHandle implements AutoCloseable {
211+
private final AwsCredentialsProvider provider;
212+
@Nullable
213+
private final SdkAutoCloseable stsClient;
214+
215+
CredentialsProviderHandle(AwsCredentialsProvider provider, @Nullable SdkAutoCloseable stsClient) {
216+
this.provider = provider;
217+
this.stsClient = stsClient;
218+
}
219+
220+
public AwsCredentialsProvider provider() {
221+
return provider;
222+
}
223+
224+
@Override
225+
public void close() {
226+
if (provider instanceof SdkAutoCloseable closeableProvider) {
227+
IoUtils.closeQuietly(closeableProvider, LOG);
228+
}
229+
IoUtils.closeQuietly(stsClient, LOG);
230+
}
173231
}
174232
}

graylog2-server/src/test/java/org/graylog/integrations/aws/AWSAuthFactoryTest.java

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,11 @@
3434

3535
import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
3636
import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
37+
import static org.assertj.core.api.Assertions.assertThatCode;
3738
import static org.mockito.ArgumentMatchers.any;
3839
import static org.mockito.Mockito.mock;
3940
import static org.mockito.Mockito.mockStatic;
41+
import static org.mockito.Mockito.verify;
4042
import static org.mockito.Mockito.when;
4143

4244
public class AWSAuthFactoryTest {
@@ -214,4 +216,48 @@ public void testExternalIdIgnoredWhenNoAssumeRole() {
214216
assertThat(withoutExternalId).isExactlyInstanceOf(StaticCredentialsProvider.class);
215217
assertThat(withExternalId).isExactlyInstanceOf(StaticCredentialsProvider.class);
216218
}
219+
220+
// --- createCloseable: the returned handle must close the STS client that the credentials provider never owns.
221+
// StsCredentialsProvider.close() only clears the session cache, leaving the StsClient's Apache connection pool
222+
// to leak on every call; the handle is what lets a caller release it. ---
223+
224+
@Test
225+
public void createCloseable_assumeRole_closeClosesStsClientAndProvider() {
226+
final StsClient mockStsClient = buildMockStsClient();
227+
final StsClientBuilder mockStsClientBuilder = mock(StsClientBuilder.class);
228+
when(mockStsClientBuilder.region(any())).thenReturn(mockStsClientBuilder);
229+
when(mockStsClientBuilder.credentialsProvider(any())).thenReturn(mockStsClientBuilder);
230+
when(mockStsClientBuilder.build()).thenReturn(mockStsClient);
231+
232+
final StsAssumeRoleCredentialsProvider mockProvider = mock(StsAssumeRoleCredentialsProvider.class);
233+
final StsAssumeRoleCredentialsProvider.Builder mockProviderBuilder = mock(StsAssumeRoleCredentialsProvider.Builder.class);
234+
when(mockProviderBuilder.refreshRequest(any(AssumeRoleRequest.class))).thenReturn(mockProviderBuilder);
235+
when(mockProviderBuilder.stsClient(any())).thenReturn(mockProviderBuilder);
236+
when(mockProviderBuilder.build()).thenReturn(mockProvider);
237+
238+
try (MockedStatic<StsClient> mockedStsClient = mockStatic(StsClient.class);
239+
MockedStatic<StsAssumeRoleCredentialsProvider> mockedProvider = mockStatic(StsAssumeRoleCredentialsProvider.class)) {
240+
mockedStsClient.when(StsClient::builder).thenReturn(mockStsClientBuilder);
241+
mockedProvider.when(StsAssumeRoleCredentialsProvider::builder).thenReturn(mockProviderBuilder);
242+
243+
final AWSAuthFactory.CredentialsProviderHandle handle = awsAuthFactory.createCloseable(
244+
false, "us-east-1", "key", "secret", "arn:aws:iam::123456789012:role/TestRole", null, (ApacheHttpClient.Builder) null);
245+
246+
assertThat(handle.provider()).isSameAs(mockProvider);
247+
248+
handle.close();
249+
250+
verify(mockStsClient).close();
251+
verify(mockProvider).close();
252+
}
253+
}
254+
255+
@Test
256+
public void createCloseable_staticCredentials_hasNoStsClientAndCloseIsSafe() {
257+
final AWSAuthFactory.CredentialsProviderHandle handle = awsAuthFactory.createCloseable(
258+
false, null, "key", "secret", null, null, (ApacheHttpClient.Builder) null);
259+
260+
assertThat(handle.provider()).isExactlyInstanceOf(StaticCredentialsProvider.class);
261+
assertThatCode(handle::close).doesNotThrowAnyException();
262+
}
217263
}

0 commit comments

Comments
 (0)