You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/02/25 17:17:38 UTC
[GitHub] [zookeeper] symat opened a new pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
symat opened a new pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828
This bug is similar to the one fixed in https://issues.apache.org/jira/browse/KAFKA-12730.
Our Kerberos ticket refresh thread performs re-login by logging out and then logging in again. If
login fails, we retry after some sleep. Every reLogin() operation performs loginContext.logout()
and loginContext.login(). If login fails, we end up with two consecutive logouts. This used to
work in older Java versions, but from Java 9 onwards, this results in a NullPointerException due
to https://bugs.openjdk.java.net/browse/JDK-8173069. We should check if logout is required before
attempting logout.
I fixed the issue and added a new unit test to test some ticket renewal scenarios. I managed to
reproduce the problem in KerberosTicketRenewalTest.shouldRecoverIfKerberosNotAvailableForSomeTime()
which (before the fix) failed with Java13 but succeeded with Java8.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1054182166
only unrelated C client test fail
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#discussion_r814965484
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/KerberosTicketRenewalTest.java
##########
@@ -0,0 +1,271 @@
+/*
+ * 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.zookeeper;
+
+import static org.apache.zookeeper.server.quorum.auth.MiniKdc.MAX_TICKET_LIFETIME;
+import static org.apache.zookeeper.server.quorum.auth.MiniKdc.MIN_TICKET_LIFETIME;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.security.Principal;
+import java.time.Duration;
+import java.util.Properties;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Supplier;
+import javax.security.auth.login.Configuration;
+import javax.security.auth.login.LoginException;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.FilenameUtils;
+import org.apache.zookeeper.common.ZKConfig;
+import org.apache.zookeeper.server.quorum.auth.KerberosTestUtils;
+import org.apache.zookeeper.server.quorum.auth.MiniKdc;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This test class is mainly testing the TGT renewal logic implemented in the org.apache.zookeeper.Login class.
+ */
+public class KerberosTicketRenewalTest {
+
+
+ private static final Logger LOG = LoggerFactory.getLogger(KerberosTicketRenewalTest.class);
+ private static final String JAAS_CONFIG_SECTION = "ClientUsingKerberos";
+ public static final String TICKET_LIFETIME = "2000";
+ private static File testTempDir;
+ private static MiniKdc kdc;
+ private static File kdcWorkDir;
+ private static String PRINCIPAL = KerberosTestUtils.getClientPrincipal();
+
+ TestableKerberosLogin login;
+
+ @BeforeAll
+ public static void setupClass() throws Exception {
+ // by default, we should wait at least 1 minute between subsequent TGT renewals.
+ // changing it to 500ms.
+ System.setProperty(Login.MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY, "500");
+
+ testTempDir = ClientBase.createTmpDir();
+ startMiniKdcAndAddPrincipal();
+
+ String keytabFilePath = FilenameUtils.normalize(KerberosTestUtils.getKeytabFile(), true);
+
+ // note: we use "refreshKrb5Config=true" to refresh the kerberos config in the JVM,
+ // making sure that we use the latest config even if other tests already have been executed
+ // and initialized the kerberos client configs before)
+ String jaasEntries = ""
+ + "ClientUsingKerberos {\n"
+ + " com.sun.security.auth.module.Krb5LoginModule required\n"
+ + " storeKey=\"false\"\n"
+ + " useTicketCache=\"false\"\n"
+ + " useKeyTab=\"true\"\n"
+ + " doNotPrompt=\"true\"\n"
+ + " debug=\"true\"\n"
+ + " refreshKrb5Config=\"true\"\n"
+ + " keyTab=\"" + keytabFilePath + "\"\n"
+ + " principal=\"" + PRINCIPAL + "\";\n"
+ + "};\n";
+ setupJaasConfig(jaasEntries);
+ }
+
+ @AfterAll
+ public static void tearDownClass() {
Review comment:
yes, we clear the whole "testTempDir", which contains the jaas config file and also a subfolder for the current KDC cluster. I'll add a comment to clarify.
(and the testTempDir is created with the standard "ClientBase.createTmpDir();" method)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1055443605
thanks! I think we should merge this to all active branches (this is not a new feature, but a bugfix which can potentially affect any ZooKeeper version running on Java9+). I already created a separate PR (https://github.com/apache/zookeeper/pull/1830) for branch-3.5 (same changes, but using junit4).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1055439439
@eolivelli , can I merge this change? I don't think the flaky CI failures were related this case (after commit da8aebc which was actually fixing the flakiness in the new tests I added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1055290047
yeee! (I restarted again a few hours before and now we just had a fully green run)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1052722864
interesting... it constantly passed for me with some older java 11 versions, but it is flaky on the java 11.0.14 we use in CI. The fix itself should be harmless, I think we have some timing issue. (in this particular test I stop and restart KDC to trigger failed ticket renewal, while using 2sec ticket lifetime - I think KDC can not start quick enough to make the second renewal pass) I'll try to make the test more robust.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1052744746
I increased a timeout, now the 5 sec ticket lifetime should hopefully give plenty of time to restart the KDC. Also I made my renewal assertion more clever, I think this was the actual issue behind the failures (now I assert that the renewal not only started but actually succeeded). This way the test run for me now 115 times without any error. Previously it failed once about each 3-4 execution on my laptop. I hope this will be robust enough for the CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli closed pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#discussion_r814979402
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/KerberosTicketRenewalTest.java
##########
@@ -0,0 +1,273 @@
+/*
+ * 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.zookeeper;
+
+import static org.apache.zookeeper.server.quorum.auth.MiniKdc.MAX_TICKET_LIFETIME;
+import static org.apache.zookeeper.server.quorum.auth.MiniKdc.MIN_TICKET_LIFETIME;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.security.Principal;
+import java.time.Duration;
+import java.util.Properties;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Supplier;
+import javax.security.auth.login.Configuration;
+import javax.security.auth.login.LoginException;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.FilenameUtils;
+import org.apache.zookeeper.common.ZKConfig;
+import org.apache.zookeeper.server.quorum.auth.KerberosTestUtils;
+import org.apache.zookeeper.server.quorum.auth.MiniKdc;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This test class is mainly testing the TGT renewal logic implemented
+ * in the org.apache.zookeeper.Login class.
+ */
+public class KerberosTicketRenewalTest {
+
+
+ private static final Logger LOG = LoggerFactory.getLogger(KerberosTicketRenewalTest.class);
+ private static final String JAAS_CONFIG_SECTION = "ClientUsingKerberos";
+ private static final String TICKET_LIFETIME = "2000";
+ private static File testTempDir;
+ private static MiniKdc kdc;
+ private static File kdcWorkDir;
+ private static String PRINCIPAL = KerberosTestUtils.getClientPrincipal();
+
+ TestableKerberosLogin login;
+
+ @BeforeAll
+ public static void setupClass() throws Exception {
+ // by default, we should wait at least 1 minute between subsequent TGT renewals.
+ // changing it to 500ms.
+ System.setProperty(Login.MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY, "500");
+
+ testTempDir = ClientBase.createTmpDir();
+ startMiniKdcAndAddPrincipal();
+
+ String keytabFilePath = FilenameUtils.normalize(KerberosTestUtils.getKeytabFile(), true);
+
+ // note: we use "refreshKrb5Config=true" to refresh the kerberos config in the JVM,
+ // making sure that we use the latest config even if other tests already have been executed
+ // and initialized the kerberos client configs before)
+ String jaasEntries = ""
+ + "ClientUsingKerberos {\n"
+ + " com.sun.security.auth.module.Krb5LoginModule required\n"
+ + " storeKey=\"false\"\n"
+ + " useTicketCache=\"false\"\n"
+ + " useKeyTab=\"true\"\n"
+ + " doNotPrompt=\"true\"\n"
+ + " debug=\"true\"\n"
+ + " refreshKrb5Config=\"true\"\n"
+ + " keyTab=\"" + keytabFilePath + "\"\n"
+ + " principal=\"" + PRINCIPAL + "\";\n"
+ + "};\n";
+ setupJaasConfig(jaasEntries);
+ }
+
+ @AfterAll
+ public static void tearDownClass() {
+ System.clearProperty(Login.MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY);
+ stopMiniKdc();
+ if (testTempDir != null) {
+ // the testTempDir contains the jaas config file and also the
+ // working folder of the currently running KDC server
+ FileUtils.deleteQuietly(testTempDir);
+ }
+ }
+
+ @AfterEach
+ public void tearDownTest() throws Exception {
+ if (login != null) {
+ login.shutdown();
+ login.logout();
+ }
+ }
+
+
+ /**
+ * We extend the regular Login class to be able to properly control the
+ * "sleeping" between the retry attempts of ticket refresh actions.
+ */
+ private static class TestableKerberosLogin extends Login {
+
+ private AtomicBoolean refreshFailed = new AtomicBoolean(false);
+ private CountDownLatch continueRefreshThread = new CountDownLatch(1);
+
+ public TestableKerberosLogin() throws LoginException {
+ super(JAAS_CONFIG_SECTION, (callbacks) -> {}, new ZKConfig());
+ }
+
+ @Override
+ protected void sleepBeforeRetryFailedRefresh() throws InterruptedException {
+ refreshFailed.set(true);
+ continueRefreshThread.await(10, TimeUnit.SECONDS);
+ }
+
+ public void assertRefreshFailsEventually(Duration timeout) {
+ assertEventually(timeout, () -> refreshFailed.get());
+ }
+
+ public void continueWithRetryAfterFailedRefresh() {
+ continueRefreshThread.countDown();
+ }
+ }
+
+
+ @Test
+ public void shouldLoginUsingKerberos() throws Exception {
+ login = new TestableKerberosLogin();
+ login.startThreadIfNeeded();
+
+ assertPrincipalLoggedIn();
+ }
+
+
+ @Test
+ public void shouldRenewTicketUsingKerberos() throws Exception {
+ login = new TestableKerberosLogin();
+ login.startThreadIfNeeded();
+
+ long initialLoginTime = login.getLastLogin();
+
+ // ticket lifetime is 2sec, so we will trigger ticket renewal in each ~1.5 sec
+ assertTicketRefreshHappenedUntil(Duration.ofSeconds(5));
+
+ assertPrincipalLoggedIn();
+ assertTrue(initialLoginTime < login.getLastLogin());
+ }
+
+
+ @Test
+ public void shouldRecoverIfKerberosNotAvailableForSomeTime() throws Exception {
+ login = new TestableKerberosLogin();
+ login.startThreadIfNeeded();
+
+ assertTicketRefreshHappenedUntil(Duration.ofSeconds(5));
+
+ stopMiniKdc();
+
+ // ticket lifetime is 2sec, so we will trigger ticket renewal in each ~1.5 sec
+ // the very next ticket renewal should fail (as KDC is offline)
+ login.assertRefreshFailsEventually(Duration.ofSeconds(5));
+
+ // now the ticket thread is "sleeping", it will retry the refresh later
+
+ // we restart KDC, then terminate the "sleeping" and expecting
+ // that the next retry should succeed
+ startMiniKdcAndAddPrincipal();
+ login.continueWithRetryAfterFailedRefresh();
+ assertTicketRefreshHappenedUntil(Duration.ofSeconds(5));
+
+ assertPrincipalLoggedIn();
+ }
+
+
+ private void assertPrincipalLoggedIn() {
+ assertEquals(PRINCIPAL, login.getUserName());
+ assertNotNull(login.getSubject());
+ assertEquals(1, login.getSubject().getPrincipals().size());
+ Principal actualPrincipal = login.getSubject().getPrincipals().iterator().next();
+ assertEquals(PRINCIPAL, actualPrincipal.getName());
+ }
+
+ private void assertTicketRefreshHappenedUntil(Duration timeout) {
+ long lastLoginTime = login.getLastLogin();
+ assertEventually(timeout, () -> login.getLastLogin() != lastLoginTime);
+ }
+
+ private static void assertEventually(Duration timeout, Supplier<Boolean> test) {
+ assertTimeout(timeout, () -> {
+ while (true) {
+ if (test.get()) {
+ return;
+ }
+ Thread.sleep(100);
+ }
+ });
+ }
+
+ public static void startMiniKdcAndAddPrincipal() throws Exception {
+ kdcWorkDir = createTmpDirInside(testTempDir);
+
+ Properties conf = MiniKdc.createConf();
+ conf.setProperty(MAX_TICKET_LIFETIME, TICKET_LIFETIME);
+ conf.setProperty(MIN_TICKET_LIFETIME, TICKET_LIFETIME);
+
+ kdc = new MiniKdc(conf, kdcWorkDir);
+ kdc.start();
+
+ String principalName = PRINCIPAL.substring(0, PRINCIPAL.lastIndexOf("@"));
+ kdc.createPrincipal(new File(KerberosTestUtils.getKeytabFile()), principalName);
+ }
+
+ private static void stopMiniKdc() {
+ if (kdc != null) {
+ kdc.stop();
+ kdc = null;
+ }
+ if (kdcWorkDir != null) {
+ FileUtils.deleteQuietly(kdcWorkDir);
+ kdcWorkDir = null;
+ }
+ }
+
+ private static File createTmpDirInside(File parentDir) throws IOException {
+ File tmpFile = File.createTempFile("test", ".junit", parentDir);
+ // don't delete tmpFile - this ensures we don't attempt to create
+ // a tmpDir with a duplicate name
+ File tmpDir = new File(tmpFile + ".dir");
+ // never true if tmpfile does it's job
+ assertFalse(tmpDir.exists());
+ assertTrue(tmpDir.mkdirs());
+ return tmpDir;
+ }
+
+ private static void setupJaasConfig(String jaasEntries) {
+ try {
+ File saslConfFile = new File(testTempDir, "jaas.conf");
+ FileWriter fwriter = new FileWriter(saslConfFile);
+ fwriter.write(jaasEntries);
+ fwriter.close();
+ System.setProperty("java.security.auth.login.config", saslConfFile.getAbsolutePath());
Review comment:
Oh, I see. Nice catch, thanks!
(you were clear, I just didn't get it ;) )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1054231577
I have restarted the job
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1051042297
I plan to cherry-pick this to all active branches. (expecting that we will need some minor change on branch-3.5, due to the different JUnit version)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1052698713
hmm... one of my KerberosTicketRenewalTest failed on java11. (I tested locally on java8 and java13) I'll have to dig into the problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1054316417
now c++ tests passing, but some Java tests failes (none of them SASL related):
```
[ERROR] Failures:
[ERROR] RequestThrottlerTest.testGlobalOutstandingRequestThrottlingWithRequestThrottlerDisabled:338 expected: <3> but was: <4>
[ERROR] RequestThrottlerTest.testLargeRequestThrottling:297 expected: <2> but was: <1>
[ERROR] Zab1_0Test.testNormalFollowerRunWithDiff:706->testFollowerConversation:445 expected: <4294967298> but was: <0>
[INFO]
[ERROR] Tests run: 2979, Failures: 3, Errors: 0, Skipped: 4
```
I don't know if we should wait for a CI run where both C++ and Java tests succeed. Our CI is very flaky nowadays :(
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#discussion_r814974800
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/KerberosTicketRenewalTest.java
##########
@@ -0,0 +1,273 @@
+/*
+ * 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.zookeeper;
+
+import static org.apache.zookeeper.server.quorum.auth.MiniKdc.MAX_TICKET_LIFETIME;
+import static org.apache.zookeeper.server.quorum.auth.MiniKdc.MIN_TICKET_LIFETIME;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.security.Principal;
+import java.time.Duration;
+import java.util.Properties;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Supplier;
+import javax.security.auth.login.Configuration;
+import javax.security.auth.login.LoginException;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.FilenameUtils;
+import org.apache.zookeeper.common.ZKConfig;
+import org.apache.zookeeper.server.quorum.auth.KerberosTestUtils;
+import org.apache.zookeeper.server.quorum.auth.MiniKdc;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This test class is mainly testing the TGT renewal logic implemented
+ * in the org.apache.zookeeper.Login class.
+ */
+public class KerberosTicketRenewalTest {
+
+
+ private static final Logger LOG = LoggerFactory.getLogger(KerberosTicketRenewalTest.class);
+ private static final String JAAS_CONFIG_SECTION = "ClientUsingKerberos";
+ private static final String TICKET_LIFETIME = "2000";
+ private static File testTempDir;
+ private static MiniKdc kdc;
+ private static File kdcWorkDir;
+ private static String PRINCIPAL = KerberosTestUtils.getClientPrincipal();
+
+ TestableKerberosLogin login;
+
+ @BeforeAll
+ public static void setupClass() throws Exception {
+ // by default, we should wait at least 1 minute between subsequent TGT renewals.
+ // changing it to 500ms.
+ System.setProperty(Login.MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY, "500");
+
+ testTempDir = ClientBase.createTmpDir();
+ startMiniKdcAndAddPrincipal();
+
+ String keytabFilePath = FilenameUtils.normalize(KerberosTestUtils.getKeytabFile(), true);
+
+ // note: we use "refreshKrb5Config=true" to refresh the kerberos config in the JVM,
+ // making sure that we use the latest config even if other tests already have been executed
+ // and initialized the kerberos client configs before)
+ String jaasEntries = ""
+ + "ClientUsingKerberos {\n"
+ + " com.sun.security.auth.module.Krb5LoginModule required\n"
+ + " storeKey=\"false\"\n"
+ + " useTicketCache=\"false\"\n"
+ + " useKeyTab=\"true\"\n"
+ + " doNotPrompt=\"true\"\n"
+ + " debug=\"true\"\n"
+ + " refreshKrb5Config=\"true\"\n"
+ + " keyTab=\"" + keytabFilePath + "\"\n"
+ + " principal=\"" + PRINCIPAL + "\";\n"
+ + "};\n";
+ setupJaasConfig(jaasEntries);
+ }
+
+ @AfterAll
+ public static void tearDownClass() {
+ System.clearProperty(Login.MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY);
+ stopMiniKdc();
+ if (testTempDir != null) {
+ // the testTempDir contains the jaas config file and also the
+ // working folder of the currently running KDC server
+ FileUtils.deleteQuietly(testTempDir);
+ }
+ }
+
+ @AfterEach
+ public void tearDownTest() throws Exception {
+ if (login != null) {
+ login.shutdown();
+ login.logout();
+ }
+ }
+
+
+ /**
+ * We extend the regular Login class to be able to properly control the
+ * "sleeping" between the retry attempts of ticket refresh actions.
+ */
+ private static class TestableKerberosLogin extends Login {
+
+ private AtomicBoolean refreshFailed = new AtomicBoolean(false);
+ private CountDownLatch continueRefreshThread = new CountDownLatch(1);
+
+ public TestableKerberosLogin() throws LoginException {
+ super(JAAS_CONFIG_SECTION, (callbacks) -> {}, new ZKConfig());
+ }
+
+ @Override
+ protected void sleepBeforeRetryFailedRefresh() throws InterruptedException {
+ refreshFailed.set(true);
+ continueRefreshThread.await(10, TimeUnit.SECONDS);
+ }
+
+ public void assertRefreshFailsEventually(Duration timeout) {
+ assertEventually(timeout, () -> refreshFailed.get());
+ }
+
+ public void continueWithRetryAfterFailedRefresh() {
+ continueRefreshThread.countDown();
+ }
+ }
+
+
+ @Test
+ public void shouldLoginUsingKerberos() throws Exception {
+ login = new TestableKerberosLogin();
+ login.startThreadIfNeeded();
+
+ assertPrincipalLoggedIn();
+ }
+
+
+ @Test
+ public void shouldRenewTicketUsingKerberos() throws Exception {
+ login = new TestableKerberosLogin();
+ login.startThreadIfNeeded();
+
+ long initialLoginTime = login.getLastLogin();
+
+ // ticket lifetime is 2sec, so we will trigger ticket renewal in each ~1.5 sec
+ assertTicketRefreshHappenedUntil(Duration.ofSeconds(5));
+
+ assertPrincipalLoggedIn();
+ assertTrue(initialLoginTime < login.getLastLogin());
+ }
+
+
+ @Test
+ public void shouldRecoverIfKerberosNotAvailableForSomeTime() throws Exception {
+ login = new TestableKerberosLogin();
+ login.startThreadIfNeeded();
+
+ assertTicketRefreshHappenedUntil(Duration.ofSeconds(5));
+
+ stopMiniKdc();
+
+ // ticket lifetime is 2sec, so we will trigger ticket renewal in each ~1.5 sec
+ // the very next ticket renewal should fail (as KDC is offline)
+ login.assertRefreshFailsEventually(Duration.ofSeconds(5));
+
+ // now the ticket thread is "sleeping", it will retry the refresh later
+
+ // we restart KDC, then terminate the "sleeping" and expecting
+ // that the next retry should succeed
+ startMiniKdcAndAddPrincipal();
+ login.continueWithRetryAfterFailedRefresh();
+ assertTicketRefreshHappenedUntil(Duration.ofSeconds(5));
+
+ assertPrincipalLoggedIn();
+ }
+
+
+ private void assertPrincipalLoggedIn() {
+ assertEquals(PRINCIPAL, login.getUserName());
+ assertNotNull(login.getSubject());
+ assertEquals(1, login.getSubject().getPrincipals().size());
+ Principal actualPrincipal = login.getSubject().getPrincipals().iterator().next();
+ assertEquals(PRINCIPAL, actualPrincipal.getName());
+ }
+
+ private void assertTicketRefreshHappenedUntil(Duration timeout) {
+ long lastLoginTime = login.getLastLogin();
+ assertEventually(timeout, () -> login.getLastLogin() != lastLoginTime);
+ }
+
+ private static void assertEventually(Duration timeout, Supplier<Boolean> test) {
+ assertTimeout(timeout, () -> {
+ while (true) {
+ if (test.get()) {
+ return;
+ }
+ Thread.sleep(100);
+ }
+ });
+ }
+
+ public static void startMiniKdcAndAddPrincipal() throws Exception {
+ kdcWorkDir = createTmpDirInside(testTempDir);
+
+ Properties conf = MiniKdc.createConf();
+ conf.setProperty(MAX_TICKET_LIFETIME, TICKET_LIFETIME);
+ conf.setProperty(MIN_TICKET_LIFETIME, TICKET_LIFETIME);
+
+ kdc = new MiniKdc(conf, kdcWorkDir);
+ kdc.start();
+
+ String principalName = PRINCIPAL.substring(0, PRINCIPAL.lastIndexOf("@"));
+ kdc.createPrincipal(new File(KerberosTestUtils.getKeytabFile()), principalName);
+ }
+
+ private static void stopMiniKdc() {
+ if (kdc != null) {
+ kdc.stop();
+ kdc = null;
+ }
+ if (kdcWorkDir != null) {
+ FileUtils.deleteQuietly(kdcWorkDir);
+ kdcWorkDir = null;
+ }
+ }
+
+ private static File createTmpDirInside(File parentDir) throws IOException {
+ File tmpFile = File.createTempFile("test", ".junit", parentDir);
+ // don't delete tmpFile - this ensures we don't attempt to create
+ // a tmpDir with a duplicate name
+ File tmpDir = new File(tmpFile + ".dir");
+ // never true if tmpfile does it's job
+ assertFalse(tmpDir.exists());
+ assertTrue(tmpDir.mkdirs());
+ return tmpDir;
+ }
+
+ private static void setupJaasConfig(String jaasEntries) {
+ try {
+ File saslConfFile = new File(testTempDir, "jaas.conf");
+ FileWriter fwriter = new FileWriter(saslConfFile);
+ fwriter.write(jaasEntries);
+ fwriter.close();
+ System.setProperty("java.security.auth.login.config", saslConfFile.getAbsolutePath());
Review comment:
Sorry I wasn't clear.
It looks to me we are not clearing this system property
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#discussion_r814962149
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/KerberosTicketRenewalTest.java
##########
@@ -0,0 +1,271 @@
+/*
+ * 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.zookeeper;
+
+import static org.apache.zookeeper.server.quorum.auth.MiniKdc.MAX_TICKET_LIFETIME;
+import static org.apache.zookeeper.server.quorum.auth.MiniKdc.MIN_TICKET_LIFETIME;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTimeout;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.io.File;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.security.Principal;
+import java.time.Duration;
+import java.util.Properties;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Supplier;
+import javax.security.auth.login.Configuration;
+import javax.security.auth.login.LoginException;
+import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.FilenameUtils;
+import org.apache.zookeeper.common.ZKConfig;
+import org.apache.zookeeper.server.quorum.auth.KerberosTestUtils;
+import org.apache.zookeeper.server.quorum.auth.MiniKdc;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This test class is mainly testing the TGT renewal logic implemented in the org.apache.zookeeper.Login class.
+ */
+public class KerberosTicketRenewalTest {
+
+
+ private static final Logger LOG = LoggerFactory.getLogger(KerberosTicketRenewalTest.class);
+ private static final String JAAS_CONFIG_SECTION = "ClientUsingKerberos";
+ public static final String TICKET_LIFETIME = "2000";
+ private static File testTempDir;
+ private static MiniKdc kdc;
+ private static File kdcWorkDir;
+ private static String PRINCIPAL = KerberosTestUtils.getClientPrincipal();
+
+ TestableKerberosLogin login;
+
+ @BeforeAll
+ public static void setupClass() throws Exception {
+ // by default, we should wait at least 1 minute between subsequent TGT renewals.
+ // changing it to 500ms.
+ System.setProperty(Login.MIN_TIME_BEFORE_RELOGIN_CONFIG_KEY, "500");
+
+ testTempDir = ClientBase.createTmpDir();
+ startMiniKdcAndAddPrincipal();
+
+ String keytabFilePath = FilenameUtils.normalize(KerberosTestUtils.getKeytabFile(), true);
+
+ // note: we use "refreshKrb5Config=true" to refresh the kerberos config in the JVM,
+ // making sure that we use the latest config even if other tests already have been executed
+ // and initialized the kerberos client configs before)
+ String jaasEntries = ""
+ + "ClientUsingKerberos {\n"
+ + " com.sun.security.auth.module.Krb5LoginModule required\n"
+ + " storeKey=\"false\"\n"
+ + " useTicketCache=\"false\"\n"
+ + " useKeyTab=\"true\"\n"
+ + " doNotPrompt=\"true\"\n"
+ + " debug=\"true\"\n"
+ + " refreshKrb5Config=\"true\"\n"
+ + " keyTab=\"" + keytabFilePath + "\"\n"
+ + " principal=\"" + PRINCIPAL + "\";\n"
+ + "};\n";
+ setupJaasConfig(jaasEntries);
+ }
+
+ @AfterAll
+ public static void tearDownClass() {
Review comment:
We are missing to clear the Jaas system properties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1055446491
committed to master, branch-3.8 and branch-3.7.
branch-3.6 is still on junit4 and the patch does not apply
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1828: ZOOKEEPER-4477: Single Kerberos ticket renewal failure can prevent all future renewals since Java 9
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1828:
URL: https://github.com/apache/zookeeper/pull/1828#issuecomment-1055440194
CI passed.
I am merging this patch.
should we merge this to branch-3.7 as well ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org