8058290: JAAS Krb5LoginModule has suspect ticket-renewal logic, relies on clockskew grace

Reviewed-by: mbalao

src/share/classes/com/sun/security/auth/module/Krb5LoginModule.java

@@ -123,8 +123,9 @@ import sun.misc.HexDumpEncoder;
  * must also be set to true; Otherwise a configuration error will
  * be returned.</dd>
  * <dt><b><code>renewTGT</code></b>:</dt>
- * <dd>Set this to true, if you want to renew
- * the TGT. If this is set, <code>useTicketCache</code> must also be
+ * <dd>Set this to true, if you want to renew the TGT when it's more than
+ * half-way expired (the time until expiration is less than the time
+ * since start time). If this is set, {@code useTicketCache} must also be
  * set to true; otherwise a configuration error will be returned.</dd>
  * <dt><b><code>doNotPrompt</code></b>:</dt>
  * <dd>Set this to true if you do not want to be
@@ -665,15 +666,15 @@ public class Krb5LoginModule implements LoginModule {
                     (principal, ticketCacheName);
 
                 if (cred != null) {
-                    // check to renew credentials
-                    if (!isCurrent(cred)) {
-                        if (renewTGT) {
+                    if (renewTGT && isOld(cred)) {
+                        // renew if ticket is old.
                         Credentials newCred = renewCredentials(cred);
                         if (newCred != null) {
                             newCred.setProxy(cred.getProxy());
-                            }
                             cred = newCred;
-                        } else {
+                        }
+                    }
+                    if (!isCurrent(cred)) {
                         // credentials have expired
                         cred = null;
                         if (debug)
@@ -681,7 +682,6 @@ public class Krb5LoginModule implements LoginModule {
                                     " no longer valid");
                     }
                 }
-                }
 
                 if (cred != null) {
                    // get the principal name from the ticket cache
@@ -988,7 +988,7 @@ public class Krb5LoginModule implements LoginModule {
         }
     }
 
-    private boolean isCurrent(Credentials creds)
+    private static boolean isCurrent(Credentials creds)
     {
         Date endTime = creds.getEndTime();
         if (endTime != null) {
@@ -997,6 +997,23 @@ public class Krb5LoginModule implements LoginModule {
         return true;
     }
 
+    private static boolean isOld(Credentials creds)
+    {
+        Date endTime = creds.getEndTime();
+        if (endTime != null) {
+            Date authTime = creds.getAuthTime();
+            long now = System.currentTimeMillis();
+            if (authTime != null) {
+                // pass the mid between auth and end
+                return now - authTime.getTime() > endTime.getTime() - now;
+            } else {
+                // will expire in less than 2 hours
+                return now <= endTime.getTime() - 1000*3600*2L;
+            }
+        }
+        return false;
+    }
+
     private Credentials renewCredentials(Credentials creds)
     {
         Credentials lcreds;

test/sun/security/krb5/auto/KDC.java

@@ -920,7 +920,7 @@ public class KDC {
                     new TransitedEncoding(1, new byte[0]),  // TODO
                     new KerberosTime(new Date()),
                     body.from,
-                    till, body.rtime,
+                    till, etp.renewTill,
                     body.addresses != null  // always set caddr
                             ? body.addresses
                             : new HostAddresses(
@@ -947,7 +947,7 @@ public class KDC {
                     tFlags,
                     new KerberosTime(new Date()),
                     body.from,
-                    till, rtime,
+                    till, etp.renewTill,
                     service,
                     body.addresses != null  // always set caddr
                             ? body.addresses

test/sun/security/krb5/auto/Renew.java

+/*
+ * Copyright (c) 2015, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8058290
+ * @summary JAAS Krb5LoginModule has suspect ticket-renewal logic,
+ *          relies on clockskew grace
+ * @compile -XDignore.symbol.file Renew.java
+ * @run main/othervm -Dsun.net.spi.nameservice.provider.1=ns,mock Renew 1
+ * @run main/othervm -Dsun.net.spi.nameservice.provider.1=ns,mock Renew 2
+ * @run main/othervm -Dsun.net.spi.nameservice.provider.1=ns,mock Renew 3
+ */
+
+import sun.security.krb5.Config;
+
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.Date;
+import javax.security.auth.kerberos.KerberosTicket;
+
+public class Renew {
+
+    public static void main(String[] args) throws Exception {
+
+        // Three test cases:
+        // 1. renewTGT=false
+        // 2. renewTGT=true with a short life time, renew will happen
+        // 3. renewTGT=true with a long life time, renew won't happen
+        int test = Integer.parseInt(args[0]);
+
+        OneKDC k = new OneKDC(null);
+        KDC.saveConfig(OneKDC.KRB5_CONF, k,
+                "renew_lifetime = 1d",
+                "ticket_lifetime = " + (test == 2? "10s": "8h"));
+        Config.refresh();
+        k.writeJAASConf();
+
+        // KDC would save ccache in a file
+        System.setProperty("test.kdc.save.ccache", "cache.here");
+
+        Files.write(Paths.get(OneKDC.JAAS_CONF), Arrays.asList(
+                "first {",
+                "   com.sun.security.auth.module.Krb5LoginModule required;",
+                "};",
+                "second {",
+                "   com.sun.security.auth.module.Krb5LoginModule required",
+                "   doNotPrompt=true",
+                "   renewTGT=" + (test != 1),
+                "   useTicketCache=true",
+                "   ticketCache=cache.here;",
+                "};"
+        ));
+
+        Context c;
+
+        // The first login uses username and password
+        c = Context.fromUserPass(OneKDC.USER, OneKDC.PASS, false);
+        Date d1 = c.s().getPrivateCredentials(KerberosTicket.class).iterator().next().getAuthTime();
+
+        // 6s is longer than half of 10s
+        Thread.sleep(6000);
+
+        // The second login uses the cache
+        c = Context.fromJAAS("second");
+        Date d2 = c.s().getPrivateCredentials(KerberosTicket.class).iterator().next().getAuthTime();
+
+        if (test == 2) {
+            if (d1.equals(d2)) {
+                throw new Exception("Ticket not renewed");
+            }
+        } else {
+            if (!d1.equals(d2)) {
+                throw new Exception("Ticket renewed");
+            }
+        }
+    }
+}