Making optional configs mandatory (apache#18368)

Author: cryptoe

docs/development/extensions-core/druid-kerberos.md

@@ -48,12 +48,12 @@ druid.auth.authenticator.<authenticatorName>.<authenticatorProperty>
 The configuration examples in the rest of this document will use "kerberos" as the name of the authenticator being configured.
 
 ### Properties
-|Property|Possible Values|Description|Default|required|
-|--------|---------------|-----------|-------|--------|
-|`druid.auth.authenticator.kerberos.serverPrincipal`|`HTTP/_HOST@EXAMPLE.COM`| SPNEGO service principal used by druid processes|empty|Yes|
-|`druid.auth.authenticator.kerberos.serverKeytab`|`/etc/security/keytabs/spnego.service.keytab`|SPNego service keytab used by druid processes|empty|Yes|
+|Property|Possible Values|Description| Default | required |
+|--------|---------------|-----------|-----|--|
+|`druid.auth.authenticator.kerberos.serverPrincipal`|`HTTP/_HOST@EXAMPLE.COM`| SPNEGO service principal used by druid processes|Empty|Yes|
+|`druid.auth.authenticator.kerberos.serverKeytab`|`/etc/security/keytabs/spnego.service.keytab`|SPNego service keytab used by druid processes|Empty|Yes|
 |`druid.auth.authenticator.kerberos.authToLocal`|`RULE:[1:$1@$0](druid@EXAMPLE.COM)s/.*/druid DEFAULT`|It allows you to set a general rule for mapping principal names to local user names. It will be used if there is not an explicit mapping for the principal name that is being translated.|DEFAULT|No|
-|`druid.auth.authenticator.kerberos.cookieSignatureSecret`|`secretString`| Secret used to sign authentication cookies. It is advisable to explicitly set it, if you have multiple druid nodes running on same machine with different ports as the Cookie Specification does not guarantee isolation by port.|Random value|No|
+|`druid.auth.authenticator.kerberos.cookieSignatureSecret`|`secretString`| Secret used to sign authentication cookies|Empty|Yes|
 |`druid.auth.authenticator.kerberos.authorizerName`|Depends on available authorizers|Authorizer that requests should be directed to|Empty|Yes|
 
 As a note, it is required that the SPNego principal in use by the druid processes must start with HTTP (This specified by [RFC-4559](https://tools.ietf.org/html/rfc4559)) and must be of the form "HTTP/_HOST@REALM".

extensions-core/druid-kerberos/src/main/java/org/apache/druid/security/kerberos/KerberosAuthenticator.java

@@ -25,6 +25,7 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.google.common.base.Joiner;
 import com.google.common.base.Preconditions;
+import org.apache.druid.error.DruidException;
 import org.apache.druid.guice.annotations.Self;
 import org.apache.druid.java.util.common.StringUtils;
 import org.apache.druid.java.util.common.logger.Logger;
@@ -75,7 +76,6 @@
 import java.util.Properties;
 import java.util.Set;
 import java.util.TimeZone;
-import java.util.concurrent.ThreadLocalRandom;
 import java.util.stream.Collectors;
 
 
@@ -84,6 +84,7 @@ public class KerberosAuthenticator implements Authenticator
 {
   private static final Logger log = new Logger(KerberosAuthenticator.class);
   public static final String SIGNED_TOKEN_ATTRIBUTE = "signedToken";
+  private static final String COOKIE_SIGNATURE_SECRET_KEY = "cookieSignatureSecret";
 
   private final String serverPrincipal;
   private final String serverKeytab;
@@ -98,14 +99,20 @@ public KerberosAuthenticator(
       @JsonProperty("serverPrincipal") String serverPrincipal,
       @JsonProperty("serverKeytab") String serverKeytab,
       @JsonProperty("authToLocal") String authToLocal,
-      @JsonProperty("cookieSignatureSecret") String cookieSignatureSecret,
+      @JsonProperty(COOKIE_SIGNATURE_SECRET_KEY) String cookieSignatureSecret,
       @JsonProperty("authorizerName") String authorizerName,
       @JsonProperty("name") String name,
       @JacksonInject @Self DruidNode node
   )
   {
     this.serverKeytab = serverKeytab;
     this.authToLocal = authToLocal == null ? "DEFAULT" : authToLocal;
+    if (cookieSignatureSecret == null || cookieSignatureSecret.isEmpty()) {
+      throw DruidException.forPersona(DruidException.Persona.OPERATOR)
+                          .ofCategory(DruidException.Category.INVALID_INPUT)
+                          .build("[%s] is not set for Kerberos authenticator", COOKIE_SIGNATURE_SECRET_KEY);
+    }
+
     this.cookieSignatureSecret = cookieSignatureSecret;
     this.authorizerName = authorizerName;
     this.name = Preconditions.checkNotNull(name);
@@ -140,8 +147,10 @@ public void init(FilterConfig filterConfig) throws ServletException
           Properties config = getConfiguration(configPrefix, filterConfig);
           String signatureSecret = config.getProperty(configPrefix + SIGNATURE_SECRET);
           if (signatureSecret == null) {
-            signatureSecret = Long.toString(ThreadLocalRandom.current().nextLong());
-            log.warn("'signature.secret' configuration not set, using a random value as secret");
+            throw DruidException.defensive(
+                "Config property[%s] is not set for Kerberos authenticator",
+                SIGNATURE_SECRET
+            );
           }
           final byte[] secretBytes = StringUtils.toUtf8(signatureSecret);
           SignerSecretProvider signerSecretProvider = new SignerSecretProvider()
@@ -381,9 +390,7 @@ public Map<String, String> getInitParameters()
     params.put("kerberos.keytab", serverKeytab);
     params.put(AuthenticationFilter.AUTH_TYPE, DruidKerberosAuthenticationHandler.class.getName());
     params.put("kerberos.name.rules", authToLocal);
-    if (cookieSignatureSecret != null) {
-      params.put("signature.secret", cookieSignatureSecret);
-    }
+    params.put(AuthenticationFilter.SIGNATURE_SECRET, cookieSignatureSecret);
     return params;
   }
 

extensions-core/druid-kerberos/src/test/java/org/apache/druid/security/kerberos/KerberosAuthenticatorTest.java

@@ -0,0 +1,101 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.kerberos;
+
+import org.apache.druid.error.DruidException;
+import org.apache.druid.server.DruidNode;
+import org.junit.Assert;
+import org.junit.Test;
+
+public class KerberosAuthenticatorTest
+{
+  private static final String TEST_SERVER_PRINCIPAL = "HTTP/localhost@EXAMPLE.COM";
+  private static final String TEST_SERVER_KEYTAB = "/path/to/keytab";
+  private static final String TEST_AUTH_TO_LOCAL = "RULE:[1:$1@$0](.*@EXAMPLE.COM)s/@.*//";
+  private static final String TEST_AUTHORIZER_NAME = "testAuthorizer";
+  private static final String TEST_NAME = "testKerberos";
+  private static final String TEST_COOKIE_SECRET = "test-secret-key";
+
+  private DruidNode createTestNode()
+  {
+    return new DruidNode("test", "localhost", false, 8080, null, true, false);
+  }
+
+
+  @Test
+  public void testConstructorWithNullCookieSignatureSecret()
+  {
+    DruidNode node = createTestNode();
+
+    DruidException exception = Assert.assertThrows(
+        DruidException.class,
+        () -> new KerberosAuthenticator(
+            TEST_SERVER_PRINCIPAL,
+            TEST_SERVER_KEYTAB,
+            TEST_AUTH_TO_LOCAL,
+            null, // null cookie signature secret
+            TEST_AUTHORIZER_NAME,
+            TEST_NAME,
+            node
+        )
+    );
+
+    Assert.assertEquals(DruidException.Persona.OPERATOR, exception.getTargetPersona());
+    Assert.assertEquals(DruidException.Category.INVALID_INPUT, exception.getCategory());
+    Assert.assertTrue(
+        "Exception message should mention cookieSignatureSecret",
+        exception.getMessage().contains("cookieSignatureSecret")
+    );
+    Assert.assertTrue(
+        "Exception message should mention 'is not set'",
+        exception.getMessage().contains("is not set")
+    );
+  }
+
+  @Test
+  public void testConstructorWithEmptyCookieSignatureSecret()
+  {
+    DruidNode node = createTestNode();
+
+    DruidException exception = Assert.assertThrows(
+        DruidException.class,
+        () -> new KerberosAuthenticator(
+            TEST_SERVER_PRINCIPAL,
+            TEST_SERVER_KEYTAB,
+            TEST_AUTH_TO_LOCAL,
+            "", // empty cookie signature secret
+            TEST_AUTHORIZER_NAME,
+            TEST_NAME,
+            node
+        )
+    );
+
+    Assert.assertEquals(DruidException.Persona.OPERATOR, exception.getTargetPersona());
+    Assert.assertEquals(DruidException.Category.INVALID_INPUT, exception.getCategory());
+    Assert.assertTrue(
+        "Exception message should mention cookieSignatureSecret",
+        exception.getMessage().contains("cookieSignatureSecret")
+    );
+    Assert.assertTrue(
+        "Exception message should mention 'is not set'",
+        exception.getMessage().contains("is not set")
+    );
+  }
+}