unit tests for crtauth

helios-crtauth/pom.xml

@@ -65,5 +65,24 @@
       <version>4.12</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-all</artifactId>
+      <version>1.9.5</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>io.dropwizard</groupId>
+      <artifactId>dropwizard-testing</artifactId>
+      <version>0.7.1</version>
+      <scope>test</scope>
+    </dependency>
+    <dependency>
+      <groupId>org.hamcrest</groupId>
+      <artifactId>hamcrest-library</artifactId>
+      <version>1.3</version>
+      <scope>test</scope>
+    </dependency>
+
   </dependencies>
 </project>

helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtAccessToken.java

@@ -21,16 +21,38 @@
 
 package com.spotify.helios.auth.crt;
 
+import com.google.common.base.Preconditions;
+
 /** Represents the credentials sent in HTTP requests for CRT */
 public class CrtAccessToken {
 
   private final String token;
 
   public CrtAccessToken(String token) {
-    this.token = token;
+    this.token = Preconditions.checkNotNull(token);
   }
 
   public String getToken() {
     return token;
   }
+
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+
+    CrtAccessToken that = (CrtAccessToken) o;
+
+    return token.equals(that.token);
+
+  }
+
+  @Override
+  public int hashCode() {
+    return token.hashCode();
+  }
 }

helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtAuthenticationPlugin.java

@@ -22,6 +22,7 @@
 package com.spotify.helios.auth.crt;
 
 import com.google.auto.service.AutoService;
+import com.google.common.annotations.VisibleForTesting;
 
 import com.spotify.crtauth.CrtAuthServer;
 import com.spotify.crtauth.keyprovider.KeyProvider;
@@ -30,24 +31,37 @@
 import org.springframework.ldap.core.LdapTemplate;
 import org.springframework.ldap.core.support.LdapContextSource;
 
-import java.util.concurrent.TimeUnit;
+import java.util.Map;
 
 @AutoService(AuthenticationPlugin.class)
 public class CrtAuthenticationPlugin implements AuthenticationPlugin<CrtAccessToken> {
 
+  private final Map<String, String> environment;
+
+  public CrtAuthenticationPlugin() {
+    this(System.getenv());
+  }
+
+  @VisibleForTesting
+  protected CrtAuthenticationPlugin(Map<String, String> environment) {
+    this.environment = environment;
+  }
+
   @Override
   public String schemeName() {
     return "crtauth";
   }
 
   @Override
   public ServerAuthentication<CrtAccessToken> serverAuthentication() {
-    // TODO (mbrown): check for null
-    final String ldapUrl = System.getenv("CRTAUTH_LDAP_URL");
-    final String ldapSearchPath = System.getenv("CRTAUTH_LDAP_SEARCH_PATH");
-    final String serverName = System.getenv("CRTAUTH_SERVERNAME");
-    final String secret = System.getenv("CRTAUTH_SECRET");
-    final String ldapFieldNameOfKey = System.getenv("CRTAUTH_LDAP_KEY_FIELDNAME");
+    // only validate the presence of environment variables when this method is called, as opposed to
+    // in the constructor, as the client-side code will not use the same environment variables
+    final String ldapUrl =  getRequiredEnv("CRTAUTH_LDAP_URL");
+    final String ldapSearchPath = getRequiredEnv("CRTAUTH_LDAP_SEARCH_PATH");
+    final String serverName = getRequiredEnv("CRTAUTH_SERVERNAME");
+    final String secret = getRequiredEnv("CRTAUTH_SECRET");
+    final String ldapFieldNameOfKey = getOptionalEnv("CRTAUTH_LDAP_KEY_FIELDNAME", "sshPublicKey");
+    final int tokenLifetimeSecs = getOptionalEnv("CRTAUTH_TOKEN_LIFETIME_SECS", 540);
 
     final LdapContextSource contextSource = new LdapContextSource();
     contextSource.setUrl(ldapUrl);
@@ -56,20 +70,48 @@ public ServerAuthentication<CrtAccessToken> serverAuthentication() {
 
     final LdapTemplate ldapTemplate = new LdapTemplate(contextSource);
 
-    // TODO (mbrown): this should be general, support reading from flat files etc
+    // TODO (mbrown): this should be general, support reading keys from flat files etc
     final KeyProvider keyProvider =
         new LdapKeyProvider(ldapTemplate, ldapSearchPath, ldapFieldNameOfKey);
 
     CrtAuthServer authServer = new CrtAuthServer.Builder()
         .setServerName(serverName)
         .setKeyProvider(keyProvider)
         .setSecret(secret.getBytes())
-        .setTokenLifetimeInS((int) TimeUnit.MINUTES.toSeconds(9))
+        .setTokenLifetimeInS(tokenLifetimeSecs)
         .build();
 
     return new CrtServerAuthentication(new CrtTokenAuthenticator(authServer), authServer);
   }
 
+  private String getEnv(String name, boolean required) {
+    if (required && !environment.containsKey(name)) {
+      throw new IllegalArgumentException("Environment variable " + name + " is required");
+    }
+    return environment.get(name);
+  }
+
+  private String getRequiredEnv(String name) {
+    return getEnv(name, true);
+  }
+
+  private String getOptionalEnv(String name, String defaultValue) {
+    final String defined = getEnv(name, false);
+    return defined != null ? defined : defaultValue;
+  }
+
+  private int getOptionalEnv(String name, int defaultValue) {
+    final String defined = getEnv(name, false);
+    if (defined == null) {
+      return defaultValue;
+    }
+    try {
+      return Integer.parseInt(defined);
+    } catch (NumberFormatException e) {
+      throw new IllegalArgumentException("Value for " + name + " is not numeric");
+    }
+  }
+
   @Override
   public ClientAuthentication<CrtAccessToken> clientAuthentication() {
     return null;

helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtHandshakeResource.java

@@ -66,7 +66,7 @@ private Response sendChallenge(String chap) {
       return Response.ok()
           .header("X-CHAP", "challenge:" + challenge)
           .build();
-    } catch (ProtocolVersionException e) {
+    } catch (IllegalArgumentException | ProtocolVersionException e) {
       // TODO (mbrown): proper response
       return Response.status(Status.BAD_REQUEST)
           .build();
@@ -81,7 +81,7 @@ private Response sendResponse(String chap) {
       return Response.ok()
           .header("X-CHAP", "token:" + token)
           .build();
-    } catch (ProtocolVersionException e) {
+    } catch (IllegalArgumentException | ProtocolVersionException e) {
       // TODO (mbrown): proper response
       return Response.status(Status.BAD_REQUEST)
           .build();

helios-crtauth/src/main/java/com/spotify/helios/auth/crt/CrtTokenAuthenticator.java

@@ -52,7 +52,7 @@ public Optional<HeliosUser> authenticate(CrtAccessToken credentials)
     final String encodedUsername;
     try {
       encodedUsername = crtAuthServer.validateToken(token);
-    } catch (TokenExpiredException | ProtocolVersionException e) {
+    } catch (TokenExpiredException | ProtocolVersionException | IllegalArgumentException e) {
       log.warn("error validating CRT token", e);
       return Optional.absent();
     }

helios-crtauth/src/main/java/com/spotify/helios/auth/crt/LdapKeyProvider.java

@@ -21,7 +21,7 @@
 
 package com.spotify.helios.auth.crt;
 
-import com.google.common.base.Preconditions;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Throwables;
 
 import com.spotify.crtauth.exceptions.KeyNotFoundException;
@@ -31,7 +31,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.ldap.core.AttributesMapper;
-import org.springframework.ldap.core.LdapTemplate;
+import org.springframework.ldap.core.LdapOperations;
 
 import java.security.InvalidKeyException;
 import java.security.KeyFactory;
@@ -52,19 +52,46 @@
 // TODO (mbrown): generalize this
 public class LdapKeyProvider implements KeyProvider {
 
+  // similar to Guava's Function, but we need to declare a checked exception
+  public interface KeyParsingFunction {
+
+    RSAPublicKeySpec apply(String input) throws InvalidKeyException;
+  }
+
+  private static final KeyParsingFunction DEFAULT_KEY_PARSER = new KeyParsingFunction() {
+    @Override
+    public RSAPublicKeySpec apply(final String input) throws InvalidKeyException {
+      return TraditionalKeyParser.parsePemPublicKey(input);
+    }
+  };
+
   private static final Logger log = LoggerFactory.getLogger(LdapKeyProvider.class);
 
-  private final LdapTemplate ldapTemplate;
+  private final LdapOperations ldapTemplate;
   private final String baseSearchPath;
   /** name of the ldap attribute holding the key */
   private final String fieldName;
+  private final KeyParsingFunction publicKeyParser;
 
-  public LdapKeyProvider(final LdapTemplate ldapTemplate,
+  public LdapKeyProvider(final LdapOperations ldapTemplate,
                          final String baseSearchPath,
                          final String fieldName) {
+    this(ldapTemplate, baseSearchPath, fieldName, DEFAULT_KEY_PARSER);
+  }
+
+  /**
+   * Allow for swapping out the logic around parsing the public key string as returned by LDAP into
+   * an RSAPublicKeySpec, as having the test mock out actual legit public keys seems too annoying
+   */
+  @VisibleForTesting
+  protected LdapKeyProvider(final LdapOperations ldapTemplate,
+                            final String baseSearchPath,
+                            final String fieldName,
+                            final KeyParsingFunction publicKeyParser) {
+    this.ldapTemplate = ldapTemplate;
+    this.baseSearchPath = baseSearchPath;
     this.fieldName = fieldName;
-    this.ldapTemplate = Preconditions.checkNotNull(ldapTemplate);
-    this.baseSearchPath = Preconditions.checkNotNull(baseSearchPath);
+    this.publicKeyParser = publicKeyParser;
   }
 
   @Override
@@ -86,10 +113,9 @@ public String mapFromAttributes(final Attributes attributes) throws NamingExcept
       throw new KeyNotFoundException();
     } else if (result.size() == 1) {
       final String r = result.get(0);
-      RSAPublicKeySpec publicKeySpec;
       try {
         final String sshPublicKey = r.replace(fieldName + ": ", "").trim();
-        publicKeySpec = TraditionalKeyParser.parsePemPublicKey(sshPublicKey);
+        final RSAPublicKeySpec publicKeySpec = this.publicKeyParser.apply(sshPublicKey);
         final KeyFactory keyFactory = KeyFactory.getInstance("RSA");
         return (RSAPublicKey) keyFactory.generatePublic(publicKeySpec);
       } catch (InvalidKeyException | InvalidKeySpecException | NoSuchAlgorithmException e) {
@@ -100,4 +126,6 @@ public String mapFromAttributes(final Attributes attributes) throws NamingExcept
 
     throw new IllegalStateException("Found more than one LDAP user for name: " + username);
   }
+
+
 }