Skip to content

Commit b997727

Browse files
rzepinskiprzepinskip
committed
fix: Treat empty string in hadoop.auth cookie as no cookie
1 parent 177cada commit b997727

3 files changed

Lines changed: 196 additions & 34 deletions

File tree

‎extensions-core/druid-kerberos/pom.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,11 @@
370370
</dependency>
371371

372372
<!-- Tests -->
373+
<dependency>
374+
<groupId>org.mockito</groupId>
375+
<artifactId>mockito-core</artifactId>
376+
<scope>test</scope>
377+
</dependency>
373378
<dependency>
374379
<groupId>org.apache.druid</groupId>
375380
<artifactId>druid-processing</artifactId>

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

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import org.eclipse.jetty.client.Request;
4545
import org.eclipse.jetty.http.HttpCookie;
4646

47+
import javax.annotation.Nullable;
4748
import javax.security.auth.Subject;
4849
import javax.security.auth.kerberos.KerberosPrincipal;
4950
import javax.security.auth.login.AppConfigurationEntry;
@@ -188,11 +189,15 @@ protected AuthenticationToken getToken(HttpServletRequest request) throws Authen
188189
for (Cookie cookie : cookies) {
189190
if (cookie.getName().equals(AuthenticatedURL.AUTH_COOKIE)) {
190191
tokenStr = cookie.getValue();
191-
try {
192-
tokenStr = mySigner.verifyAndExtract(tokenStr);
193-
}
194-
catch (SignerException ex) {
195-
throw new AuthenticationException(ex);
192+
if (tokenStr == null || tokenStr.isEmpty()) {
193+
tokenStr = null;
194+
} else {
195+
try {
196+
tokenStr = mySigner.verifyAndExtract(tokenStr);
197+
}
198+
catch (SignerException ex) {
199+
throw new AuthenticationException(ex);
200+
}
196201
}
197202
break;
198203
}
@@ -266,7 +271,7 @@ private void doFilterSuper(ServletRequest request, ServletResponse response, Fil
266271
}
267272
token = getAuthenticationHandler().authenticate(httpRequest, httpResponse);
268273
if (token != null && token.getExpires() != 0 &&
269-
token != AuthenticationToken.ANONYMOUS) {
274+
!AuthenticationToken.ANONYMOUS.equals(token)) {
270275
token.setExpires(System.currentTimeMillis() + getValidity() * 1000);
271276
}
272277
newToken = true;
@@ -293,12 +298,13 @@ public String getRemoteUser()
293298
}
294299

295300
@Override
301+
@Nullable
296302
public Principal getUserPrincipal()
297303
{
298-
return (authToken != AuthenticationToken.ANONYMOUS) ? authToken : null;
304+
return (!AuthenticationToken.ANONYMOUS.equals(authToken)) ? authToken : null;
299305
}
300306
};
301-
if (newToken && !token.isExpired() && token != AuthenticationToken.ANONYMOUS) {
307+
if (newToken && !token.isExpired() && !AuthenticationToken.ANONYMOUS.equals(token)) {
302308
String signedToken = mySigner.sign(token.toString());
303309
tokenToAuthCookie(
304310
httpResponse,
@@ -374,6 +380,7 @@ public Principal getUserPrincipal()
374380
}
375381

376382
@Override
383+
@Nullable
377384
public Class<? extends Filter> getFilterClass()
378385
{
379386
return null;
@@ -398,6 +405,7 @@ public String getPath()
398405
}
399406

400407
@Override
408+
@Nullable
401409
public EnumSet<DispatcherType> getDispatcherType()
402410
{
403411
return null;
@@ -423,9 +431,8 @@ public void decorateProxyRequest(
423431
)
424432
{
425433
Object cookieToken = clientRequest.getAttribute(SIGNED_TOKEN_ATTRIBUTE);
426-
if (cookieToken != null && cookieToken instanceof String) {
434+
if (cookieToken instanceof String authResult) {
427435
log.debug("Found cookie token will attache it to proxyRequest as cookie");
428-
String authResult = (String) cookieToken;
429436
proxyRequest.cookie(HttpCookie.from(SIGNED_TOKEN_ATTRIBUTE, authResult));
430437
}
431438
}
@@ -435,8 +442,8 @@ public void decorateProxyRequest(
435442
*/
436443
public static class DruidKerberosConfiguration extends Configuration
437444
{
438-
private String keytab;
439-
private String principal;
445+
private final String keytab;
446+
private final String principal;
440447

441448
public DruidKerberosConfiguration(String keytab, String principal)
442449
{
@@ -497,11 +504,11 @@ private void initializeKerberosLogin() throws ServletException
497504
String keytab;
498505

499506
try {
500-
if (serverPrincipal == null || serverPrincipal.trim().length() == 0) {
507+
if (serverPrincipal == null || serverPrincipal.trim().isEmpty()) {
501508
throw new ServletException("Principal not defined in configuration");
502509
}
503510
keytab = serverKeytab;
504-
if (keytab == null || keytab.trim().length() == 0) {
511+
if (keytab == null || keytab.trim().isEmpty()) {
505512
throw new ServletException("Keytab not defined in configuration");
506513
}
507514
if (!new File(keytab).exists()) {
@@ -568,7 +575,7 @@ private static String tokenToCookieString(
568575
{
569576
StringBuilder sb = new StringBuilder(AuthenticatedURL.AUTH_COOKIE)
570577
.append("=");
571-
if (token != null && token.length() > 0) {
578+
if (token != null && !token.isEmpty()) {
572579
sb.append("\"").append(token).append("\"");
573580
}
574581

@@ -580,7 +587,9 @@ private static String tokenToCookieString(
580587
sb.append("; Domain=").append(domain);
581588
}
582589

583-
if (expires >= 0 && isCookiePersistent) {
590+
if (expires == 0) {
591+
sb.append("; Max-Age=0");
592+
} else if (expires > 0 && isCookiePersistent) {
584593
Date date = new Date(expires);
585594
SimpleDateFormat df = new SimpleDateFormat("EEE, dd-MMM-yyyy HH:mm:ss zzz", Locale.ENGLISH);
586595
df.setTimeZone(TimeZone.getTimeZone("GMT"));

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

Lines changed: 166 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,23 @@
2121

2222
import org.apache.druid.error.DruidException;
2323
import org.apache.druid.server.DruidNode;
24+
import org.apache.hadoop.security.authentication.client.AuthenticatedURL;
25+
import org.apache.hadoop.security.authentication.server.AuthenticationFilter;
26+
import org.apache.hadoop.security.authentication.server.AuthenticationToken;
27+
import org.apache.hadoop.security.authentication.util.Signer;
28+
import org.apache.hadoop.security.authentication.util.SignerSecretProvider;
2429
import org.junit.Assert;
2530
import org.junit.Test;
31+
import org.mockito.Mockito;
32+
33+
import javax.servlet.Filter;
34+
import javax.servlet.ServletContext;
35+
import javax.servlet.http.Cookie;
36+
import javax.servlet.http.HttpServletRequest;
37+
import java.lang.reflect.Field;
38+
import java.lang.reflect.Method;
39+
import java.nio.charset.StandardCharsets;
40+
import java.util.Properties;
2641

2742
public class KerberosAuthenticatorTest
2843
{
@@ -39,22 +54,110 @@ private DruidNode createTestNode()
3954
}
4055

4156

57+
/**
58+
* Verifies that an empty hadoop.auth cookie value is treated as "no cookie" rather than
59+
* causing a SignerException. An empty cookie results from a prior session expiry where
60+
* Druid cleared the cookie. Without this fix, the empty value would be passed to
61+
* Signer.verifyAndExtract("") which throws SignerException, setting authenticationEx
62+
* and causing the entire auth chain to short-circuit with a 403.
63+
*/
64+
@Test
65+
public void testGetTokenWithEmptyCookieReturnsNull() throws Exception
66+
{
67+
final Filter filter = createFilterWithSigner();
68+
final Method getToken = findGetTokenMethod();
69+
70+
// Empty cookie value - the real-world scenario after session expiry clears the cookie.
71+
// Without the fix, Signer.verifyAndExtract("") throws SignerException.
72+
final HttpServletRequest requestWithEmptyCookie = mockRequestWithEmptyCookie();
73+
final AuthenticationToken token = (AuthenticationToken) getToken.invoke(filter, requestWithEmptyCookie);
74+
Assert.assertNull("Empty hadoop.auth cookie should be treated as no cookie", token);
75+
76+
// No cookie at all - baseline, should return null
77+
final HttpServletRequest requestWithNoCookie = Mockito.mock(HttpServletRequest.class);
78+
Mockito.when(requestWithNoCookie.getCookies()).thenReturn(null);
79+
final AuthenticationToken tokenForNoCookie = (AuthenticationToken) getToken.invoke(filter, requestWithNoCookie);
80+
Assert.assertNull("Missing hadoop.auth cookie should return null", tokenForNoCookie);
81+
}
82+
83+
private Filter createFilterWithSigner() throws Exception
84+
{
85+
final Filter filter = new KerberosAuthenticator(
86+
TEST_SERVER_PRINCIPAL,
87+
TEST_SERVER_KEYTAB,
88+
TEST_AUTH_TO_LOCAL,
89+
TEST_COOKIE_SECRET,
90+
TEST_AUTHORIZER_NAME,
91+
TEST_NAME,
92+
createTestNode()
93+
).getFilter();
94+
95+
final SignerSecretProvider secretProvider = new SignerSecretProvider()
96+
{
97+
@Override
98+
public void init(Properties config, ServletContext servletContext, long tokenValidity)
99+
{
100+
}
101+
102+
@Override
103+
public byte[] getCurrentSecret()
104+
{
105+
return TEST_COOKIE_SECRET.getBytes(StandardCharsets.UTF_8);
106+
}
107+
108+
@Override
109+
public byte[][] getAllSecrets()
110+
{
111+
return new byte[][]{TEST_COOKIE_SECRET.getBytes(StandardCharsets.UTF_8)};
112+
}
113+
};
114+
final Signer signer = new Signer(secretProvider);
115+
116+
// Inject mySigner into the anonymous AuthenticationFilter subclass via reflection
117+
for (Field field : filter.getClass().getDeclaredFields()) {
118+
if (field.getType().equals(Signer.class)) {
119+
field.setAccessible(true);
120+
field.set(filter, signer);
121+
break;
122+
}
123+
}
124+
return filter;
125+
}
126+
127+
private Method findGetTokenMethod() throws Exception
128+
{
129+
final Method method = AuthenticationFilter.class.getDeclaredMethod("getToken", HttpServletRequest.class);
130+
method.setAccessible(true);
131+
return method;
132+
}
133+
134+
private HttpServletRequest mockRequestWithEmptyCookie()
135+
{
136+
final HttpServletRequest request = Mockito.mock(HttpServletRequest.class);
137+
final Cookie cookie = new Cookie(AuthenticatedURL.AUTH_COOKIE, "");
138+
Mockito.when(request.getCookies()).thenReturn(new Cookie[]{cookie});
139+
return request;
140+
}
141+
42142
@Test
43143
public void testConstructorWithNullCookieSignatureSecret()
44144
{
45145
DruidNode node = createTestNode();
46146

47147
DruidException exception = Assert.assertThrows(
48148
DruidException.class,
49-
() -> new KerberosAuthenticator(
50-
TEST_SERVER_PRINCIPAL,
51-
TEST_SERVER_KEYTAB,
52-
TEST_AUTH_TO_LOCAL,
53-
null, // null cookie signature secret
54-
TEST_AUTHORIZER_NAME,
55-
TEST_NAME,
56-
node
57-
)
149+
() -> {
150+
@SuppressWarnings("unused")
151+
KerberosAuthenticator authenticator = new KerberosAuthenticator(
152+
TEST_SERVER_PRINCIPAL,
153+
TEST_SERVER_KEYTAB,
154+
TEST_AUTH_TO_LOCAL,
155+
null, // null cookie signature secret
156+
TEST_AUTHORIZER_NAME,
157+
TEST_NAME,
158+
node
159+
);
160+
}
58161
);
59162

60163
Assert.assertEquals(DruidException.Persona.OPERATOR, exception.getTargetPersona());
@@ -76,15 +179,18 @@ public void testConstructorWithEmptyCookieSignatureSecret()
76179

77180
DruidException exception = Assert.assertThrows(
78181
DruidException.class,
79-
() -> new KerberosAuthenticator(
80-
TEST_SERVER_PRINCIPAL,
81-
TEST_SERVER_KEYTAB,
82-
TEST_AUTH_TO_LOCAL,
83-
"", // empty cookie signature secret
84-
TEST_AUTHORIZER_NAME,
85-
TEST_NAME,
86-
node
87-
)
182+
() -> {
183+
@SuppressWarnings("unused")
184+
KerberosAuthenticator authenticator = new KerberosAuthenticator(
185+
TEST_SERVER_PRINCIPAL,
186+
TEST_SERVER_KEYTAB,
187+
TEST_AUTH_TO_LOCAL,
188+
"", // empty cookie signature secret
189+
TEST_AUTHORIZER_NAME,
190+
TEST_NAME,
191+
node
192+
);
193+
}
88194
);
89195

90196
Assert.assertEquals(DruidException.Persona.OPERATOR, exception.getTargetPersona());
@@ -98,4 +204,46 @@ public void testConstructorWithEmptyCookieSignatureSecret()
98204
exception.getMessage().contains("is not set")
99205
);
100206
}
207+
208+
@Test
209+
public void testTokenToCookieStringWithZeroExpiresIncludesMaxAge() throws Exception
210+
{
211+
final Method method = KerberosAuthenticator.class.getDeclaredMethod(
212+
"tokenToCookieString",
213+
String.class,
214+
String.class,
215+
String.class,
216+
long.class,
217+
boolean.class,
218+
boolean.class
219+
);
220+
method.setAccessible(true);
221+
222+
// Test case: expires = 0 (intended for cookie deletion)
223+
final String cookieString = (String) method.invoke(
224+
null,
225+
"", // token
226+
"localhost", // domain
227+
"/", // path
228+
0, // expires
229+
false, // isCookiePersistent
230+
false // isSecure
231+
);
232+
233+
Assert.assertTrue("Cookie string should contain 'Max-Age=0'", cookieString.contains("Max-Age=0"));
234+
Assert.assertFalse("Cookie string should not contain 'Expires=' when expires is 0", cookieString.contains("Expires="));
235+
236+
// Test case: expires > 0 and persistent
237+
final String persistentCookieString = (String) method.invoke(
238+
null,
239+
"some-token",
240+
"localhost",
241+
"/",
242+
System.currentTimeMillis() + 3600,
243+
true,
244+
false
245+
);
246+
Assert.assertTrue("Persistent cookie should contain 'Expires='", persistentCookieString.contains("Expires="));
247+
Assert.assertFalse("Persistent cookie should not contain 'Max-Age=0'", persistentCookieString.contains("Max-Age=0"));
248+
}
101249
}

0 commit comments

Comments
 (0)