Add extra workspace existence check before creating keypair

eclipse-che · Sep 19, 2018 · f838ea5 · f838ea5
1 parent 0ec9537
commit f838ea5
Show file tree

Hide file tree

Showing 15 changed files with 170 additions and 195 deletions.
diff --git a/...e/che/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyProvisioner.java b/...e/che/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyProvisioner.java
@@ -43,7 +43,6 @@
 import java.util.Set;
 import javax.inject.Inject;
 import javax.inject.Named;
-import org.eclipse.che.api.core.ServerException;
 import org.eclipse.che.api.core.model.workspace.config.MachineConfig;
 import org.eclipse.che.api.core.model.workspace.config.ServerConfig;
 import org.eclipse.che.api.core.model.workspace.runtime.RuntimeIdentity;
@@ -52,6 +51,7 @@
 import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig;
 import org.eclipse.che.commons.lang.Size;
 import org.eclipse.che.multiuser.machine.authentication.server.signature.SignatureKeyManager;
+import org.eclipse.che.multiuser.machine.authentication.server.signature.SignatureKeyManagerException;
 import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;
 import org.eclipse.che.workspace.infrastructure.kubernetes.server.ServerServiceBuilder;
 import org.eclipse.che.workspace.infrastructure.kubernetes.server.secure.jwtproxy.factory.JwtProxyConfigBuilderFactory;
@@ -211,8 +211,8 @@ private void ensureJwtProxyInjected(KubernetesEnvironment k8sEnv) throws Infrast
 
       KeyPair keyPair;
       try {
-        keyPair = signatureKeyManager.getKeyPair(identity.getWorkspaceId());
-      } catch (ServerException e) {
+        keyPair = signatureKeyManager.getOrCreateKeyPair(identity.getWorkspaceId());
+      } catch (SignatureKeyManagerException e) {
         throw new InternalInfrastructureException(
             "Signature key pair for machine authentication cannot be retrieved. Reason: "
                 + e.getMessage());

diff --git a/...e/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyProvisionerTest.java b/...e/workspace/infrastructure/kubernetes/server/secure/jwtproxy/JwtProxyProvisionerTest.java
@@ -75,7 +75,8 @@ public class JwtProxyProvisionerTest {
 
   @BeforeMethod
   public void setUp() throws Exception {
-    when(signatureKeyManager.getKeyPair(anyString())).thenReturn(new KeyPair(publicKey, null));
+    when(signatureKeyManager.getOrCreateKeyPair(anyString()))
+        .thenReturn(new KeyPair(publicKey, null));
     when(publicKey.getEncoded()).thenReturn("publickey".getBytes());
 
     when(configBuilderFactory.create(any()))

diff --git a/...org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitalizationFilterTest.java b/...org/eclipse/che/multiuser/keycloak/server/KeycloakEnvironmentInitalizationFilterTest.java
@@ -85,7 +85,7 @@ public void setUp() throws Exception {
     parser.setAccessible(true);
     parser.set(filter, jwtParser);
     final KeyPair kp = new KeyPair(mock(PublicKey.class), mock(PrivateKey.class));
-    when(keyManager.getKeyPair(anyString())).thenReturn(kp);
+    when(keyManager.getOrCreateKeyPair(anyString())).thenReturn(kp);
   }
 
   @Test

diff --git a/multiuser/machine-auth/che-multiuser-machine-authentication/pom.xml b/multiuser/machine-auth/che-multiuser-machine-authentication/pom.xml
@@ -90,10 +90,6 @@
             <groupId>org.eclipse.che.core</groupId>
             <artifactId>che-core-api-workspace-shared</artifactId>
         </dependency>
-        <dependency>
-            <groupId>org.eclipse.che.core</groupId>
-            <artifactId>che-core-commons-annotations</artifactId>
-        </dependency>
         <dependency>
             <groupId>org.eclipse.che.core</groupId>
             <artifactId>che-core-commons-auth</artifactId>

diff --git a/...main/java/org/eclipse/che/multiuser/machine/authentication/server/MachineLoginFilter.java b/...main/java/org/eclipse/che/multiuser/machine/authentication/server/MachineLoginFilter.java
@@ -43,6 +43,8 @@
 import org.eclipse.che.commons.subject.Subject;
 import org.eclipse.che.commons.subject.SubjectImpl;
 import org.eclipse.che.multiuser.api.permission.server.PermissionChecker;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Handles requests that comes from machines with specific machine token.
@@ -53,6 +55,8 @@
 @Singleton
 public class MachineLoginFilter implements Filter {
 
+  private static final Logger LOG = LoggerFactory.getLogger(MachineLoginFilter.class);
+
   private final RequestTokenExtractor tokenExtractor;
   private final UserManager userManager;
   private final PermissionChecker permissionChecker;

diff --git a/...va/org/eclipse/che/multiuser/machine/authentication/server/MachineSigningKeyResolver.java b/...va/org/eclipse/che/multiuser/machine/authentication/server/MachineSigningKeyResolver.java
@@ -21,8 +21,8 @@
 import java.security.Key;
 import javax.inject.Inject;
 import javax.inject.Singleton;
-import org.eclipse.che.api.core.ServerException;
 import org.eclipse.che.multiuser.machine.authentication.server.signature.SignatureKeyManager;
+import org.eclipse.che.multiuser.machine.authentication.server.signature.SignatureKeyManagerException;
 
 /** Resolves signing key pair based on workspace Id claim of token. */
 @Singleton
@@ -46,9 +46,9 @@ public Key resolveSigningKey(JwsHeader header, Claims claims) {
           "Unable to fetch signature key pair: no workspace id present in token");
     }
     try {
-      return keyManager.getKeyPair(wsId).getPublic();
-    } catch (ServerException e) {
-      throw new JwtException("Unable to fetch signature key pair:" + e.getMessage());
+      return keyManager.getOrCreateKeyPair(wsId).getPublic();
+    } catch (SignatureKeyManagerException e) {
+      throw new JwtException("Unable to fetch signature key pair:" + e.getMessage(), e);
     }
   }
 }
diff --git a/...in/java/org/eclipse/che/multiuser/machine/authentication/server/MachineTokenRegistry.java b/...in/java/org/eclipse/che/multiuser/machine/authentication/server/MachineTokenRegistry.java
@@ -33,7 +33,9 @@
 import org.eclipse.che.api.core.ServerException;
 import org.eclipse.che.api.core.model.user.User;
 import org.eclipse.che.api.user.server.UserManager;
+import org.eclipse.che.api.workspace.server.token.MachineTokenException;
 import org.eclipse.che.multiuser.machine.authentication.server.signature.SignatureKeyManager;
+import org.eclipse.che.multiuser.machine.authentication.server.signature.SignatureKeyManagerException;
 import org.eclipse.che.multiuser.machine.authentication.shared.Constants;
 
 /**
@@ -66,9 +68,9 @@ public MachineTokenRegistry(SignatureKeyManager signatureKeyManager, UserManager
    * @param userId id of user to get token
    * @param workspaceId id of workspace to get token
    * @return machine security token for for given user and workspace
-   * @throws IllegalStateException when user with given id not found or any errors occurs
+   * @throws MachineTokenException when user with given id not found or any errors occurs
    */
-  public String getOrCreateToken(String userId, String workspaceId) {
+  public String getOrCreateToken(String userId, String workspaceId) throws MachineTokenException {
     lock.writeLock().lock();
     try {
       final Map<String, String> wsRow = tokens.row(workspaceId);
@@ -77,41 +79,43 @@ public String getOrCreateToken(String userId, String workspaceId) {
         token = createToken(userId, workspaceId);
       }
       return token;
-    } catch (NotFoundException | ServerException ex) {
-      throw new IllegalStateException(
-          format(
-              "Failed to generate machine token for user '%s' and workspace '%s'. Cause: '%s'",
-              userId, workspaceId, ex.getMessage()),
-          ex);
     } finally {
       lock.writeLock().unlock();
     }
   }
 
   /** Creates new token with given data. */
-  private String createToken(String userId, String workspaceId)
-      throws NotFoundException, ServerException {
-    final PrivateKey privateKey = signatureKeyManager.getKeyPair(workspaceId).getPrivate();
-    final User user = userManager.getById(userId);
-    final Map<String, Object> header = new HashMap<>(2);
-    header.put("kind", MACHINE_TOKEN_KIND);
-    header.put("kid", workspaceId);
-    final Map<String, Object> claims = new HashMap<>();
-    // to ensure that each token is unique
-    claims.put(Claims.ID, UUID.randomUUID().toString());
-    claims.put(Constants.USER_ID_CLAIM, userId);
-    claims.put(Constants.USER_NAME_CLAIM, user.getName());
-    claims.put(Constants.WORKSPACE_ID_CLAIM, workspaceId);
-    // jwtproxy required claims
-    claims.put(Claims.ISSUER, "wsmaster");
-    claims.put(Claims.AUDIENCE, workspaceId);
-    claims.put(Claims.EXPIRATION, Instant.now().plus(365, DAYS).getEpochSecond());
-    claims.put(Claims.NOT_BEFORE, -1); // always
-    claims.put(Claims.ISSUED_AT, Instant.now().getEpochSecond());
-    final String token =
-        Jwts.builder().setClaims(claims).setHeader(header).signWith(RS256, privateKey).compact();
-    tokens.put(workspaceId, userId, token);
-    return token;
+  private String createToken(String userId, String workspaceId) throws MachineTokenException {
+    try {
+      final PrivateKey privateKey =
+          signatureKeyManager.getOrCreateKeyPair(workspaceId).getPrivate();
+      final User user = userManager.getById(userId);
+      final Map<String, Object> header = new HashMap<>(2);
+      header.put("kind", MACHINE_TOKEN_KIND);
+      header.put("kid", workspaceId);
+      final Map<String, Object> claims = new HashMap<>();
+      // to ensure that each token is unique
+      claims.put(Claims.ID, UUID.randomUUID().toString());
+      claims.put(Constants.USER_ID_CLAIM, userId);
+      claims.put(Constants.USER_NAME_CLAIM, user.getName());
+      claims.put(Constants.WORKSPACE_ID_CLAIM, workspaceId);
+      // jwtproxy required claims
+      claims.put(Claims.ISSUER, "wsmaster");
+      claims.put(Claims.AUDIENCE, workspaceId);
+      claims.put(Claims.EXPIRATION, Instant.now().plus(365, DAYS).getEpochSecond());
+      claims.put(Claims.NOT_BEFORE, -1); // always
+      claims.put(Claims.ISSUED_AT, Instant.now().getEpochSecond());
+      final String token =
+          Jwts.builder().setClaims(claims).setHeader(header).signWith(RS256, privateKey).compact();
+      tokens.put(workspaceId, userId, token);
+      return token;
+    } catch (SignatureKeyManagerException | NotFoundException | ServerException ex) {
+      throw new MachineTokenException(
+          format(
+              "Failed to generate machine token for user '%s' and workspace '%s'. Cause: '%s'",
+              userId, workspaceId, ex.getMessage()),
+          ex);
+    }
   }
 
   /**

diff --git a/...rg/eclipse/che/multiuser/machine/authentication/server/signature/SignatureKeyManager.java b/...rg/eclipse/che/multiuser/machine/authentication/server/signature/SignatureKeyManager.java
@@ -15,9 +15,6 @@
 
 import com.google.common.annotations.Beta;
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.cache.CacheBuilder;
-import com.google.common.cache.CacheLoader;
-import com.google.common.cache.LoadingCache;
 import java.security.KeyFactory;
 import java.security.KeyPair;
 import java.security.KeyPairGenerator;
@@ -28,8 +25,6 @@
 import java.security.spec.InvalidKeySpecException;
 import java.security.spec.PKCS8EncodedKeySpec;
 import java.security.spec.X509EncodedKeySpec;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.TimeUnit;
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
 import javax.inject.Named;
@@ -40,7 +35,6 @@
 import org.eclipse.che.api.core.notification.EventService;
 import org.eclipse.che.api.core.notification.EventSubscriber;
 import org.eclipse.che.api.workspace.shared.dto.event.WorkspaceStatusEvent;
-import org.eclipse.che.commons.annotation.Nullable;
 import org.eclipse.che.core.db.DBInitializer;
 import org.eclipse.che.multiuser.machine.authentication.server.signature.model.impl.SignatureKeyPairImpl;
 import org.eclipse.che.multiuser.machine.authentication.server.signature.spi.SignatureKeyDao;
@@ -72,8 +66,6 @@ public class SignatureKeyManager {
   @SuppressWarnings("unused")
   private DBInitializer dbInitializer;
 
-  private LoadingCache<String, KeyPair> cachedPair;
-
   @Inject
   public SignatureKeyManager(
       @Named("che.auth.signature_key_size") int keySize,
@@ -84,19 +76,6 @@ public SignatureKeyManager(
     this.algorithm = algorithm;
     this.eventService = eventService;
     this.signatureKeyDao = signatureKeyDao;
-
-    this.cachedPair =
-        CacheBuilder.newBuilder()
-            .maximumSize(100)
-            .expireAfterAccess(2, TimeUnit.HOURS)
-            .build(
-                new CacheLoader<String, KeyPair>() {
-                  @Override
-                  public KeyPair load(String key) throws Exception {
-                    return loadKeyPair(key);
-                  }
-                });
-
     this.workspaceEventsSubscriber =
         new EventSubscriber<WorkspaceStatusEvent>() {
           @Override
@@ -108,21 +87,34 @@ public void onEvent(WorkspaceStatusEvent event) {
         };
   }
 
-  /** Returns cached instance of {@link KeyPair} or null when failed to load key pair. */
-  @Nullable
-  public KeyPair getKeyPair(String workspaceId) throws ServerException {
+  /**
+   * Returns instance of {@link KeyPair} for given workspace.
+   *
+   * @throws SignatureKeyManagerException when stored keypair is incorrect (e.g. has bad algorithm
+   *     or keyspec) or other error
+   */
+  public KeyPair getOrCreateKeyPair(String workspaceId) throws SignatureKeyManagerException {
+    SignatureKeyPair keyPair;
     try {
-      return cachedPair.get(workspaceId);
-    } catch (ExecutionException e) {
-      throw new ServerException(e.getCause());
+      try {
+        keyPair = signatureKeyDao.get(workspaceId);
+      } catch (NotFoundException e) {
+        keyPair = generateKeyPair(workspaceId);
+      }
+    } catch (NoSuchAlgorithmException | ServerException | ConflictException ex) {
+      LOG.error(
+          "Failed to load signature keys for ws  {}. Cause: {}", workspaceId, ex.getMessage());
+      throw new SignatureKeyManagerException(ex.getMessage(), ex);
     }
+    return toJavaKeyPair(keyPair);
   }
 
   /** Removes key pair from cache and DB. */
-  public void removeKeyPair(String workspaceId) {
+  @VisibleForTesting
+  void removeKeyPair(String workspaceId) {
     try {
-      cachedPair.invalidate(workspaceId);
       signatureKeyDao.remove(workspaceId);
+      LOG.debug("Removed signature key pair for ws id {}.", workspaceId);
     } catch (ServerException e) {
       LOG.error(
           "Unable to cleanup machine token signature keypairs for ws {}. Cause: {}",
@@ -131,48 +123,43 @@ public void removeKeyPair(String workspaceId) {
     }
   }
 
-  /** Loads signature key pair if no existing keys found then stores a newly generated key pair. */
-  @PostConstruct
   @VisibleForTesting
-  KeyPair loadKeyPair(String workspaceId) throws ServerException, ConflictException {
+  SignatureKeyPair generateKeyPair(String workspaceId)
+      throws NoSuchAlgorithmException, ServerException, ConflictException {
     try {
-      return toJavaKeyPair(signatureKeyDao.get(workspaceId));
-    } catch (NotFoundException nfe) {
-      try {
-        return toJavaKeyPair(signatureKeyDao.create(generateKeyPair(workspaceId)));
-      } catch (ConflictException | ServerException ex) {
-        LOG.error(
-            "Failed to store signature keys for ws {}. Cause: {}", workspaceId, ex.getMessage());
-        throw ex;
-      }
-    } catch (ServerException ex) {
+      KeyPairGenerator kpg = KeyPairGenerator.getInstance(algorithm);
+      kpg.initialize(keySize);
+      final KeyPair pair = kpg.generateKeyPair();
+      final SignatureKeyPairImpl kp =
+          new SignatureKeyPairImpl(workspaceId, pair.getPublic(), pair.getPrivate());
+      LOG.debug(
+          "Generated signature key pair with ws id {} and algorithm {}.",
+          kp.getWorkspaceId(),
+          algorithm);
+      return signatureKeyDao.create(kp);
+    } catch (NoSuchAlgorithmException | ConflictException | ServerException ex) {
       LOG.error(
-          "Failed to load signature keys for ws  {}. Cause: {}", workspaceId, ex.getMessage());
+          "Unable to generate signature keypair for ws {}. Cause: {}",
+          workspaceId,
+          ex.getMessage());
       throw ex;
     }
   }
 
-  @VisibleForTesting
-  SignatureKeyPairImpl generateKeyPair(String workspaceId) throws ServerException {
-    final KeyPairGenerator kpg;
-    try {
-      kpg = KeyPairGenerator.getInstance(algorithm);
-    } catch (NoSuchAlgorithmException ex) {
-      throw new ServerException(ex.getMessage(), ex);
+  /** Returns key spec by key format and encoded data. */
+  private EncodedKeySpec getKeySpec(SignatureKey key) {
+    switch (key.getFormat()) {
+      case PKCS_8:
+        return new PKCS8EncodedKeySpec(key.getEncoded());
+      case X_509:
+        return new X509EncodedKeySpec(key.getEncoded());
+      default:
+        throw new IllegalArgumentException(
+            String.format("Unsupported key spec '%s' for signature keys", key.getFormat()));
     }
-    kpg.initialize(keySize);
-    final KeyPair pair = kpg.generateKeyPair();
-    final SignatureKeyPairImpl kp =
-        new SignatureKeyPairImpl(workspaceId, pair.getPublic(), pair.getPrivate());
-    LOG.debug(
-        "Generated signature key pair with ws id {} and algorithm {}.",
-        kp.getWorkspaceId(),
-        algorithm);
-    return kp;
   }
 
-  /** Converts {@link SignatureKeyPair} to {@link KeyPair}. */
-  public static KeyPair toJavaKeyPair(SignatureKeyPair keyPair) throws ServerException {
+  private KeyPair toJavaKeyPair(SignatureKeyPair keyPair) throws SignatureKeyManagerException {
     try {
       final PrivateKey privateKey =
           KeyFactory.getInstance(keyPair.getPrivateKey().getAlgorithm())
@@ -183,20 +170,7 @@ public static KeyPair toJavaKeyPair(SignatureKeyPair keyPair) throws ServerExcep
       return new KeyPair(publicKey, privateKey);
     } catch (NoSuchAlgorithmException | InvalidKeySpecException ex) {
       LOG.error("Failed to convert signature key pair to Java keys. Cause: {}", ex.getMessage());
-      throw new ServerException("Failed to convert signature key pair to Java keys.");
-    }
-  }
-
-  /** Returns key spec by key format and encoded data. */
-  private static EncodedKeySpec getKeySpec(SignatureKey key) {
-    switch (key.getFormat()) {
-      case PKCS_8:
-        return new PKCS8EncodedKeySpec(key.getEncoded());
-      case X_509:
-        return new X509EncodedKeySpec(key.getEncoded());
-      default:
-        throw new IllegalArgumentException(
-            String.format("Unsupported key spec '%s' for signature keys", key.getFormat()));
+      throw new SignatureKeyManagerException("Failed to convert signature key pair to Java keys.");
     }
   }