fix #4698: refining authentication mechanisms to be clearer

fabric8io · Jan 27, 2023 · 88a9dde · 88a9dde
1 parent 1a3b2b6
commit 88a9dde
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 32 deletions.
diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/HttpClientUtils.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/HttpClientUtils.java
@@ -60,11 +60,6 @@ public Interceptor withConfig(Config config) {
 
     @Override
     public void before(BasicBuilder builder, HttpHeaders headers) {
-      if (Utils.isNotNullOrEmpty(config.getUsername()) && Utils.isNotNullOrEmpty(config.getPassword())) {
-        builder.header("Authorization", basicCredentials(config.getUsername(), config.getPassword()));
-      } else if (Utils.isNotNullOrEmpty(config.getOauthToken())) {
-        builder.header("Authorization", "Bearer " + config.getOauthToken());
-      }
       if (config.getCustomHeaders() != null && !config.getCustomHeaders().isEmpty()) {
         for (Map.Entry<String, String> entry : config.getCustomHeaders().entrySet()) {
           builder.header(entry.getKey(), entry.getValue());

diff --git a/...-client-api/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java b/...-client-api/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java
@@ -28,7 +28,8 @@
 import java.util.concurrent.CompletableFuture;
 
 /**
- * Interceptor for handling expired OIDC tokens.
+ * Interceptor for handling kube authentication. It will either be basic auth, or token based. This class takes responsibility
+ * for refreshing expired OIDC tokens.
  */
 public class TokenRefreshInterceptor implements Interceptor {
 
@@ -54,17 +55,31 @@ public Interceptor withConfig(Config config) {
 
   @Override
   public void before(BasicBuilder headerBuilder, HttpHeaders headers) {
+    if (isBasicAuth()) {
+      headerBuilder.header("Authorization", HttpClientUtils.basicCredentials(config.getUsername(), config.getPassword()));
+      return;
+    }
+    if (Utils.isNotNullOrEmpty(config.getOauthToken())) {
+      headerBuilder.header("Authorization", "Bearer " + config.getOauthToken());
+    }
     if (isTimeToRefresh()) {
       refreshToken(headerBuilder);
     }
   }
 
+  private boolean isBasicAuth() {
+    return Utils.isNotNullOrEmpty(config.getUsername()) && Utils.isNotNullOrEmpty(config.getPassword());
+  }
+
   private boolean isTimeToRefresh() {
     return latestRefreshTimestamp.plus(REFRESH_INTERVAL_MINUTE, ChronoUnit.MINUTES).isBefore(Instant.now());
   }
 
   @Override
   public CompletableFuture<Boolean> afterFailure(BasicBuilder headerBuilder, HttpResponse<?> response) {
+    if (isBasicAuth()) {
+      return CompletableFuture.completedFuture(false);
+    }
     if (response.code() == HttpURLConnection.HTTP_UNAUTHORIZED) {
       return refreshToken(headerBuilder);
     }

diff --git a/...-client/src/main/java/io/fabric8/openshift/client/internal/OpenShiftOAuthInterceptor.java b/...-client/src/main/java/io/fabric8/openshift/client/internal/OpenShiftOAuthInterceptor.java
@@ -46,13 +46,18 @@
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static java.net.HttpURLConnection.HTTP_FORBIDDEN;
 import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED;
 
+/**
+ * Controls openshift authentication. It will be based upon an oauth token that can either come from a "login" or from the
+ * config / token provider.
+ */
 public class OpenShiftOAuthInterceptor implements Interceptor {
 
   private static final String AUTHORIZATION = "Authorization";
@@ -83,52 +88,53 @@ public OpenShiftOAuthInterceptor(HttpClient client, Config config, AtomicReferen
 
   @Override
   public OpenShiftOAuthInterceptor withConfig(Config config) {
-    return new OpenShiftOAuthInterceptor(client, config, oauthToken);
+    // only reuse the token if the username and password are the same
+    return new OpenShiftOAuthInterceptor(client, config, Objects.equals(config.getUsername(), this.config.getUsername())
+        && Objects.equals(config.getPassword(), this.config.getPassword()) ? oauthToken : new AtomicReference<>());
   }
 
   @Override
   public void before(BasicBuilder builder, HttpHeaders headers) {
-    String token = oauthToken.get();
-    // avoid overwriting basic auth token with stale bearer token
-    if (Utils.isNotNullOrEmpty(token)
-        && (headers.headers(AUTHORIZATION).isEmpty() || Utils.isNullOrEmpty(headers.headers(AUTHORIZATION).get(0)))) {
+    if (usingUsernameAndPassword()) {
+      String token = oauthToken.get();
       setAuthHeader(builder, token);
+    } else {
+      setAuthHeader(builder, config.getOauthToken());
     }
   }
 
   @Override
   public CompletableFuture<Boolean> afterFailure(Builder builder, HttpResponse<?> response) {
-    if (shouldProceed(response.request(), response)) {
+    if (shouldProceed(response.request(), response)
+        || (!usingUsernameAndPassword() && config.getOauthTokenProvider() == null)) {
       return CompletableFuture.completedFuture(false);
     }
 
-    CompletableFuture<String> tokenFuture = null;
-    if (Utils.isNotNullOrEmpty(config.getUsername()) && Utils.isNotNullOrEmpty(config.getPassword())) {
+    if (usingUsernameAndPassword()) {
       // TODO: we could make all concurrent refresh requests return the same future
-      tokenFuture = authorize();
+      return authorize().thenApply(t -> {
+        if (t != null) {
+          oauthToken.set(t);
+        }
+
+        //If token was obtained, then retry request using the obtained token.
+        return setAuthHeader(builder, t);
+      });
     } else {
-      tokenFuture = CompletableFuture.completedFuture(Utils.getNonNullOrElse(config.getOauthToken(), oauthToken.get()));
+      return CompletableFuture.completedFuture(setAuthHeader(builder, config.getOauthToken()));
     }
+  }
 
-    return tokenFuture.thenApply(t -> {
-      if (t != null) {
-        oauthToken.set(t);
-      }
-
-      //If token was obtained, then retry request using the obtained token.
-      if (Utils.isNotNullOrEmpty(t)) {
-        setAuthHeader(builder, t);
-        return true;
-      }
-
-      return false;
-    });
+  private boolean usingUsernameAndPassword() {
+    return Utils.isNotNullOrEmpty(config.getUsername()) && Utils.isNotNullOrEmpty(config.getPassword());
   }
 
-  private void setAuthHeader(BasicBuilder builder, String token) {
+  private boolean setAuthHeader(BasicBuilder builder, String token) {
     if (token != null) {
       builder.setHeader(AUTHORIZATION, String.format("Bearer %s", token));
+      return true;
     }
+    return false;
   }
 
   private CompletableFuture<String> authorize() {
@@ -171,7 +177,8 @@ private CompletableFuture<String> authorize() {
       String token = !location.isEmpty() ? location.get(0) : null;
       if (token == null || token.isEmpty()) {
         throw new KubernetesClientException("Unexpected response (" + responseOrPrevious.code() + " "
-            + responseOrPrevious.message() + "), to the authorization request. Missing header:[" + LOCATION + "]!");
+            + responseOrPrevious.message() + "), to the authorization request. Missing header:[" + LOCATION
+            + "].  More than likely the username / password are not correct.");
       }
       token = token.substring(token.indexOf(BEFORE_TOKEN) + BEFORE_TOKEN.length());
       token = token.substring(0, token.indexOf(AFTER_TOKEN));

diff --git a/...ent/src/test/java/io/fabric8/openshift/client/internal/OpenShiftOAuthInterceptorTest.java b/...ent/src/test/java/io/fabric8/openshift/client/internal/OpenShiftOAuthInterceptorTest.java
@@ -16,6 +16,7 @@
 package io.fabric8.openshift.client.internal;
 
 import io.fabric8.kubernetes.client.Config;
+import io.fabric8.kubernetes.client.ConfigBuilder;
 import org.junit.jupiter.api.Test;
 
 import java.util.concurrent.atomic.AtomicReference;
@@ -28,10 +29,15 @@ class OpenShiftOAuthInterceptorTest {
   @Test
   void testTokenSharing() {
     AtomicReference<String> reference = new AtomicReference<>();
-    OpenShiftOAuthInterceptor interceptor = new OpenShiftOAuthInterceptor(null, null, reference);
+    OpenShiftOAuthInterceptor interceptor = new OpenShiftOAuthInterceptor(null, Config.empty(), reference);
     OpenShiftOAuthInterceptor other = interceptor.withConfig(Config.empty());
     assertNotSame(interceptor, other);
     assertSame(reference, other.getOauthToken());
+
+    // should not be resued if the user overrides the requestconfig
+    other = interceptor.withConfig(new ConfigBuilder().withUsername("something").build());
+    assertNotSame(interceptor, other);
+    assertNotSame(reference, other.getOauthToken());
   }
 
 }