Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
*/
package io.kubernetes.client.util.credentials;

import io.kubernetes.client.openapi.ApiClient;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
Expand All @@ -29,12 +28,13 @@
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Base64;
import java.util.function.Supplier;

/**
* EKS cluster authentication which generates a bearer token from AWS AK/SK. It doesn't require an "aws"
* command line tool in the $PATH.
*/
public class EKSAuthentication implements Authentication {
public class EKSAuthentication extends RefreshAuthentication {

private static final Logger log = LoggerFactory.getLogger(EKSAuthentication.class);

Expand All @@ -50,33 +50,45 @@ public EKSAuthentication(AwsCredentialsProvider provider, String region, String
}

public EKSAuthentication(AwsCredentialsProvider provider, String region, String clusterName, int expirySeconds) {
this.provider = provider;
this.region = region;
this.clusterName = clusterName;
if (expirySeconds > MAX_EXPIRY_SECONDS) {
expirySeconds = MAX_EXPIRY_SECONDS;
}
this.expirySeconds = expirySeconds;
this.stsEndpoint = URI.create("https://sts." + this.region + ".amazonaws.com");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the STS endpoint as a member variable vs. constructing it every time.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendandburns This was also a side effect of using static methods instead of a separate supplier class. With the small supplier implementation outlined below, stsEndpoint could be kept as a field and would not need to be reconstructed each time. I can update it that way if that direction sounds right.

this(new EksTokenSupplier(provider, region, clusterName, Math.min(expirySeconds, MAX_EXPIRY_SECONDS)));
}

private EKSAuthentication(EksTokenSupplier tokenSupplier) {
super(tokenSupplier, Duration.of(tokenSupplier.expirySeconds, ChronoUnit.SECONDS));
setExpiry(Instant.now().plus(tokenSupplier.expirySeconds, ChronoUnit.SECONDS));
}

private static final int MAX_EXPIRY_SECONDS = 60 * 15;
private final AwsCredentialsProvider provider;
private final String region;
private final String clusterName;
private final URI stsEndpoint;

private final int expirySeconds;
private static class EksTokenSupplier implements Supplier<String> {
private final AwsCredentialsProvider provider;
private final String region;
private final String clusterName;
private final URI stsEndpoint;
private final int expirySeconds;

EksTokenSupplier(AwsCredentialsProvider provider, String region, String clusterName, int expirySeconds) {
this.provider = provider;
this.region = region;
this.clusterName = clusterName;
this.expirySeconds = expirySeconds;
this.stsEndpoint = URI.create("https://sts." + this.region + ".amazonaws.com");
}

@Override
public String get() {
return generateToken(provider, region, clusterName, stsEndpoint, expirySeconds);
}
}

@Override
public void provide(ApiClient client) {
SdkHttpRequest httpRequest = generateStsRequest();
String presignedUrl = requestToPresignedUrl(httpRequest);
private static String generateToken(
AwsCredentialsProvider provider, String region, String clusterName, URI stsEndpoint, int expirySeconds) {
SdkHttpRequest httpRequest = generateStsRequest(stsEndpoint, clusterName);
String presignedUrl = requestToPresignedUrl(httpRequest, provider, region);
String encodedUrl = presignedUrlToEncodedUrl(presignedUrl);
String token = "k8s-aws-v1." + encodedUrl;
client.setApiKeyPrefix("Bearer");
client.setApiKey(token);
log.info("Generated BEARER token for ApiClient, expiring at {}", Instant.now().plus(expirySeconds, ChronoUnit.SECONDS));
return token;
}

private static String presignedUrlToEncodedUrl(String presignedUrl) {
Expand All @@ -85,7 +97,7 @@ private static String presignedUrlToEncodedUrl(String presignedUrl) {
.encodeToString(SdkHttpUtils.urlEncodeIgnoreSlashes(presignedUrl).getBytes(StandardCharsets.UTF_8));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing the urlEncodeIgnoreSlashes call?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendandburns Thanks for catching this.

I checked the AWS SDK path locally. The signer adds the X-Amz-* values as raw query parameters, and SdkHttpRequest.getUri() builds the final URI through encodedQueryParameters(), so the returned presigned URL already contains percent-encoded query values such as X-Amz-Credential=...%2F....

That was why I initially removed the extra urlEncodeIgnoreSlashes step. It looked like another encoding pass on top of an already encoded presigned URL.

However, I should have called this out explicitly in the PR, and this is outside the refresh-related scope of the change.

Since the existing implementation included this extra encoding step, I agree it is safer to preserve the previous token payload behavior. I also should have considered compatibility with other AWS SDK versions and the existing behavior more carefully.

I will restore urlEncodeIgnoreSlashes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendandburns

I looked into this more deeply with an actual EKS API request, and I think the issue is a bit broader than the refresh change itself.

What I found

It looks like the existing EKSAuthentication path may not have been producing a token payload that EKS accepts.

In my own usage, I had been using a custom Authentication implementation instead of EKSAuthentication, so I did not catch this earlier. While verifying this PR against a real EKS cluster, I found that the current EKSAuthentication token payload is rejected with 401 Unauthorized.

Token payload difference

The expected decoded token payload is the presigned STS URL itself:

https://sts.ap-northeast-2.amazonaws.com/?Version=2011-06-15&Action=GetCallerIdentity&X-Amz-Credential=...%2F...

However, with the extra SdkHttpUtils.urlEncodeIgnoreSlashes(presignedUrl) step, the decoded token payload becomes a URL-encoded string:

https%3A//sts.ap-northeast-2.amazonaws.com%3FVersion%3D2011-06-15%26Action%3DGetCallerIdentity%26X-Amz-Credential%3D...%252F...

That means the URL structure itself is encoded, and already encoded query values such as %2F become double-encoded as %252F.

STS path

I also changed the STS request construction to keep the endpoint URI pathless and set the request path explicitly:

.uri(stsEndpoint)
.encodedPath("/")

This produces the working presigned URL form:

https://sts...amazonaws.com/?...

Reproduction branch

I created a separate branch to make this easier to verify outside this PR:

https://github.com/hwayoungjun/java/tree/repro/eks-authentication-401

That branch compares:

  • the current EKSAuthentication
  • FixedEKSAuthentication, copied from this PR's implementation

The smoke test result against a real EKS cluster was:

Current master decoded payload prefix:
https%3A//sts.ap-northeast-2.amazonaws.com%3F...
Kubernetes API request failed with HTTP 401

Fixed decoded payload prefix:
https://sts.ap-northeast-2.amazonaws.com/?...
Kubernetes API request succeeded, namespaces=13

Run command:

./mvnw -pl util -Dtest=EKSAuthenticationSmokeTest -Dsurefire.failIfNoSpecifiedTests=false test

Conclusion

From the real EKS verification, it looks like the existing EKSAuthentication token payload is not accepted by EKS today.

Because of that, I committed the token payload fix here by removing the extra whole-URL encoding and making the STS root path explicit with encodedPath("/").

That said, I realize this is separate from the refresh behavior itself. Would you prefer that I split the token payload fix into a separate issue/PR, and keep this PR focused only on refresh support?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's separate this out as a different PR. I don't have access to EKS so we'll need to figure out if there is someone else who can test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendandburns

I have access to a real EKS cluster and used it throughout to validate the changes in #4779.
So if there's anything I can help with on the verification side, just let me know.

}

private SdkHttpRequest generateStsRequest() {
private static SdkHttpRequest generateStsRequest(URI stsEndpoint, String clusterName) {
return SdkHttpRequest.builder()
.uri(stsEndpoint)
.putRawQueryParameter("Version", "2011-06-15")
Expand All @@ -95,10 +107,11 @@ private SdkHttpRequest generateStsRequest() {
.build();
}

private String requestToPresignedUrl(SdkHttpRequest httpRequest) {
private static String requestToPresignedUrl(
SdkHttpRequest httpRequest, AwsCredentialsProvider provider, String region) {
AwsV4HttpSigner signer = AwsV4HttpSigner.create();
SignedRequest signedRequest =
signer.sign(r -> r.identity(this.provider.resolveCredentials())
signer.sign(r -> r.identity(provider.resolveCredentials())
.request(httpRequest)
.putProperty(AwsV4HttpSigner.SERVICE_SIGNING_NAME, "sts")
.putProperty(AwsV4HttpSigner.REGION_NAME, region)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@
package io.kubernetes.client.util.credentials;

import io.kubernetes.client.openapi.ApiClient;
import okhttp3.OkHttpClient;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
import software.amazon.awssdk.auth.credentials.AwsSessionCredentials;

import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand All @@ -33,16 +34,20 @@ class EKSAuthenticationTest {
@Mock
private ApiClient apiClient;

@Mock
private OkHttpClient httpClient;

private String region = "us-west-2";

private String clusterName = "test-2";

@Test
void provideApiClient() {
when(provider.resolveCredentials()).thenReturn(AwsSessionCredentials.create("ak", "sk", "session"));
when(apiClient.getHttpClient()).thenReturn(httpClient);
when(httpClient.newBuilder()).thenReturn(new OkHttpClient.Builder());
EKSAuthentication authentication = new EKSAuthentication(provider, region, clusterName);
authentication.provide(apiClient);
verify(apiClient).setApiKey(anyString());
verify(apiClient).setApiKeyPrefix(anyString());
verify(apiClient).setHttpClient(any(OkHttpClient.class));
}
}
Loading