Cleanup: Ensure no logic is executed in the constructor

The SignatureService tried to access the system key file in
the constructor, which could lead to endless loops. This PR
moves the service into a AbstractLifecycleComponent to keep
the constructor dumb.

Relates elastic/elasticsearch#517

Original commit: elastic/x-pack-elasticsearch@b1e5bfe98c

src/main/java/org/elasticsearch/shield/ShieldPlugin.java

@@ -19,6 +19,8 @@ import org.elasticsearch.shield.authc.support.SecuredString;
 import org.elasticsearch.shield.authc.support.UsernamePasswordToken;
 import org.elasticsearch.shield.authz.store.FileRolesStore;
 import org.elasticsearch.shield.license.LicenseService;
+import org.elasticsearch.shield.signature.InternalSignatureService;
+import org.elasticsearch.shield.signature.SignatureService;
 
 import java.io.File;
 import java.nio.file.Path;
@@ -61,7 +63,7 @@ public class ShieldPlugin extends AbstractPlugin {
     @Override
     public Collection<Class<? extends LifecycleComponent>> services() {
         return enabled && !clientMode ?
-                ImmutableList.<Class<? extends LifecycleComponent>>of(LicenseService.class, FileRolesStore.class, Realms.class) :
+                ImmutableList.<Class<? extends LifecycleComponent>>of(LicenseService.class, FileRolesStore.class, Realms.class, InternalSignatureService.class) :
                 ImmutableList.<Class<? extends LifecycleComponent>>of();
     }
 

src/main/java/org/elasticsearch/shield/signature/InternalSignatureService.java

@@ -9,6 +9,7 @@ import org.apache.commons.codec.binary.Base64;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.base.Charsets;
 import org.elasticsearch.common.component.AbstractComponent;
+import org.elasticsearch.common.component.AbstractLifecycleComponent;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
@@ -32,7 +33,7 @@ import java.util.regex.Pattern;
 /**
  *
  */
-public class InternalSignatureService extends AbstractComponent implements SignatureService {
+public class InternalSignatureService extends AbstractLifecycleComponent<InternalSignatureService> implements SignatureService {
 
     public static final String FILE_SETTING = "shield.system_key.file";
     public static final String KEY_ALGO = "HmacSHA512";
@@ -42,8 +43,11 @@ public class InternalSignatureService extends AbstractComponent implements Signa
     static final String HMAC_ALGO = "HmacSHA1";
 
     private static final Pattern SIG_PATTERN = Pattern.compile("^\\$\\$[0-9]+\\$\\$.+");
+    private final Environment env;
+    private final ResourceWatcherService watcherService;
+    private final Listener listener;
 
-    private final Path keyFile;
+    private Path keyFile;
 
     private volatile SecretKey key;
 
@@ -54,11 +58,9 @@ public class InternalSignatureService extends AbstractComponent implements Signa
 
     InternalSignatureService(Settings settings, Environment env, ResourceWatcherService watcherService, Listener listener) {
         super(settings);
-        keyFile = resolveFile(settings, env);
-        key = readKey(keyFile);
-        FileWatcher watcher = new FileWatcher(keyFile.getParent().toFile());
-        watcher.addListener(new FileListener(listener));
-        watcherService.add(watcher, ResourceWatcherService.Frequency.HIGH);
+        this.env = env;
+        this.watcherService = watcherService;
+        this.listener = listener;
     }
 
     public static byte[] generateKey() throws Exception {
@@ -147,6 +149,21 @@ public class InternalSignatureService extends AbstractComponent implements Signa
         return Base64.encodeBase64URLSafeString(sig);
     }
 
+    @Override
+    protected void doStart() throws ElasticsearchException {
+        keyFile = resolveFile(settings, env);
+        key = readKey(keyFile);
+        FileWatcher watcher = new FileWatcher(keyFile.getParent().toFile());
+        watcher.addListener(new FileListener(listener));
+        watcherService.add(watcher, ResourceWatcherService.Frequency.HIGH);
+    }
+
+    @Override
+    protected void doStop() throws ElasticsearchException {}
+
+    @Override
+    protected void doClose() throws ElasticsearchException {}
+
     private class FileListener extends FileChangesListener {
 
         private final Listener listener;

src/main/java/org/elasticsearch/shield/signature/SignatureModule.java

@@ -19,6 +19,7 @@ public class SignatureModule extends AbstractShieldModule.Node {
 
     @Override
     protected void configureNode() {
+        bind(InternalSignatureService.class).asEagerSingleton();
         bind(SignatureService.class).to(InternalSignatureService.class).asEagerSingleton();
     }
 }

src/test/java/org/elasticsearch/integration/ScrollIdSigningTests.java

@@ -10,11 +10,14 @@ import org.elasticsearch.action.index.IndexRequestBuilder;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.shield.authz.AuthorizationException;
+import org.elasticsearch.shield.signature.InternalSignatureService;
 import org.elasticsearch.shield.signature.SignatureService;
 import org.elasticsearch.test.ShieldIntegrationTest;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.Locale;
+
 import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
 import static org.hamcrest.Matchers.*;
@@ -24,13 +27,6 @@ import static org.hamcrest.Matchers.*;
  */
 public class ScrollIdSigningTests extends ShieldIntegrationTest {
 
-    private SignatureService signatureService;
-
-    @Before
-    public void init() throws Exception {
-        signatureService = internalCluster().getInstance(SignatureService.class);
-    }
-
     @Test
     public void testSearchAndClearScroll() throws Exception {
         IndexRequestBuilder[] docs = new IndexRequestBuilder[randomIntBetween(20, 100)];
@@ -108,6 +104,8 @@ public class ScrollIdSigningTests extends ShieldIntegrationTest {
     }
 
     private void assertSigned(String scrollId) {
-        assertThat(signatureService.signed(scrollId), is(true));
+        SignatureService signatureService = internalCluster().getDataNodeInstance(InternalSignatureService.class);
+        String message = String.format(Locale.ROOT, "Expected scrollId [%s] to be signed, but was not", scrollId);
+        assertThat(message, signatureService.signed(scrollId), is(true));
     }
 }

src/test/java/org/elasticsearch/shield/signature/InternalSignatureServiceTests.java

@@ -55,7 +55,7 @@ public class InternalSignatureServiceTests extends ElasticsearchTestCase {
 
     @Test
     public void testSigned() throws Exception {
-        InternalSignatureService service = new InternalSignatureService(settings, env, watcherService);
+        InternalSignatureService service = new InternalSignatureService(settings, env, watcherService).start();
         String text = randomAsciiOfLength(10);
         String signed = service.sign(text);
         assertThat(service.signed(signed), is(true));
@@ -63,7 +63,7 @@ public class InternalSignatureServiceTests extends ElasticsearchTestCase {
 
     @Test
     public void testSignAndUnsign() throws Exception {
-        InternalSignatureService service = new InternalSignatureService(settings, env, watcherService);
+        InternalSignatureService service = new InternalSignatureService(settings, env, watcherService).start();
         String text = randomAsciiOfLength(10);
         String signed = service.sign(text);
         assertThat(text.equals(signed), is(false));
@@ -73,7 +73,7 @@ public class InternalSignatureServiceTests extends ElasticsearchTestCase {
 
     @Test
     public void testSignAndUnsign_NoKeyFile() throws Exception {
-        InternalSignatureService service = new InternalSignatureService(ImmutableSettings.EMPTY, env, watcherService);
+        InternalSignatureService service = new InternalSignatureService(ImmutableSettings.EMPTY, env, watcherService).start();
         String text = randomAsciiOfLength(10);
         String signed = service.sign(text);
         assertThat(text, equalTo(signed));
@@ -89,7 +89,7 @@ public class InternalSignatureServiceTests extends ElasticsearchTestCase {
             public void onKeyRefresh() {
                 latch.countDown();
             }
-        });
+        }).start();
 
         String text = randomAsciiOfLength(10);
         String signed = service.sign(text);