Improve handling of renewal time

Dockerfile

@@ -15,8 +15,6 @@ COPY . /usr/src/libkrimes
 
 WORKDIR /usr/src/libkrimes
 
-RUN cargo build --release
-
 RUN --mount=type=cache,id=cargo,target=/cargo \
     export CARGO_HOME=/cargo && \
     cargo build \

kdc/src/main.rs

@@ -271,7 +271,8 @@ async fn process_ticket_renewal(
     ) {
         Ok(time_bounds) => time_bounds,
         Err(time_bound_error) => {
-            return Err(time_bound_error.to_kerberos_reply(&service_name, stime))
+            error!(?time_bound_error);
+            return Err(time_bound_error.to_kerberos_reply(&service_name, stime));
         }
     };
 

libkrimes/src/proto/reply/as_rep.rs

@@ -8,6 +8,7 @@ use crate::proto::{
     TicketFlags,
 };
 use std::time::SystemTime;
+use tracing::trace;
 
 #[derive(Debug)]
 pub struct AuthenticationReply {
@@ -110,6 +111,8 @@ impl AuthenticationReplyBuilder {
             encrypted_pa_data: None,
         };
 
+        trace!(?enc_kdc_rep_part);
+
         let (etype_info2, enc_part) = user_key.encrypt_as_rep_part(enc_kdc_rep_part)?;
 
         let transited = TransitedEncoding {

libkrimes/src/proto/reply/tgs_rep.rs

@@ -12,6 +12,7 @@ use crate::proto::{
     Ticket, TicketFlags,
 };
 use crypto_glue::der::{asn1::OctetString, Encode};
+use tracing::trace;
 
 #[derive(Debug)]
 pub struct TicketGrantReply {
@@ -273,6 +274,8 @@ impl KerberosReplyTicketRenewBuilder {
             encrypted_pa_data: None,
         };
 
+        trace!(?enc_kdc_rep_part);
+
         let enc_part = if let Some(sub_session_key) = self.sub_session_key {
             sub_session_key.encrypt_tgs_rep_part(enc_kdc_rep_part, true)?
         } else {
@@ -289,8 +292,7 @@ impl KerberosReplyTicketRenewBuilder {
             contents: OctetString::new(*b"").map_err(|_| KrbError::DerEncodeOctetString)?,
         };
 
-        // EncTicketPart
-        // Encrypted to the key of the service
+        // Encrypted to the key of the kdc
         let ticket_inner = EncTicketPart {
             flags: self.ticket.flags,
             key: session_key,
@@ -305,6 +307,8 @@ impl KerberosReplyTicketRenewBuilder {
             authorization_data,
         };
 
+        trace!(?ticket_inner);
+
         let ticket_enc_part = primary_key.encrypt_tgs(ticket_inner)?;
 
         let ticket = EncTicket {

libkrimes/src/proto/time.rs

@@ -7,6 +7,7 @@ use std::time::{Duration, SystemTime};
 
 use tracing::{trace, warn};
 
+#[derive(Debug)]
 pub enum TimeBoundError {
     Skew,
     NeverValid,
@@ -37,6 +38,7 @@ impl TimeBoundError {
     }
 }
 
+#[derive(Debug)]
 pub struct AuthenticationTimeBound {
     auth_time: SystemTime,
     start_time: SystemTime,
@@ -258,46 +260,59 @@ fn as_req_end_time(
  * - The starttime of the ticket plus the maximum renewable lifetime
  *   set by the policy of the local realm.
  */
+#[tracing::instrument]
 fn as_req_renew_until(
     start_time: SystemTime,
     end_time: SystemTime,
     requested_renew_until: Option<SystemTime>,
     maximum_renew_lifetime: Option<Duration>,
     kdc_options: KerberosFlags,
 ) -> Result<Option<SystemTime>, TimeBoundError> {
-    let renewal_flags_set = kdc_options.contains(KerberosFlags::RenewableOk)
-        || kdc_options.contains(KerberosFlags::Renewable);
+    let renewable_flags_set = kdc_options.contains(KerberosFlags::Renewable);
+    let renewable_ok_flags_set = kdc_options.contains(KerberosFlags::RenewableOk);
 
     match (
         requested_renew_until,
         maximum_renew_lifetime,
-        renewal_flags_set,
+        renewable_flags_set,
+        renewable_ok_flags_set,
     ) {
-        (Some(_), _, false) => {
+        (Some(_), _, false, _) => {
             // Requested a renew until but no flags!
             Err(TimeBoundError::FlagsInconsistent)
         }
-        (_, None, _) => {
+        (_, _, false, false) => {
+            // No renewable flags provided.
+            Ok(None)
+        }
+        (_, None, true, _) => {
             // Requested a renew, but it's denied
             Err(TimeBoundError::RenewalNotAllowed)
         }
-        (None, Some(maximum_renew_lifetime), _) => {
-            // Easy, just default to our renew lifetime.
-            Ok(Some(start_time + maximum_renew_lifetime))
+        (None, _, false, true) => {
+            // The client has indicated that renewable is okay, but MIT KRB
+            // does not seem to handle this correctly and attempts to bind the
+            // renew time to its requested end time which is invalid.
+            // https://github.com/krb5/krb5/blob/master/src/lib/krb5/krb/get_in_tkt.c#L255
+            Ok(None)
         }
-        (Some(requested_renew_until), Some(maximum_renew_lifetime), _) => {
-            // This is the hard path, we have to validate things.
+        (None, Some(maximum_renew_lifetime), true, false)
+        | (None, Some(maximum_renew_lifetime), true, true) => {
+            let renew_until = start_time + maximum_renew_lifetime;
 
+            Ok(Some(renew_until))
+        }
+        (Some(requested_renew_until), Some(maximum_renew_lifetime), true, _) => {
+            // This is the sensible path, we have to validate things.
             if requested_renew_until < end_time {
                 // The renewal would never be valid.
                 return Err(TimeBoundError::NeverValid);
             }
 
             // Take the smaller of requested renew until and the maximum window.
-            Ok(Some(cmp::min(
-                requested_renew_until,
-                start_time + maximum_renew_lifetime,
-            )))
+            let renew_until = cmp::min(requested_renew_until, start_time + maximum_renew_lifetime);
+
+            Ok(Some(renew_until))
         }
     }
 }
@@ -340,6 +355,7 @@ impl TicketGrantTimeBound {
         self.renew_until
     }
 
+    #[tracing::instrument]
     pub fn from_tgs_req(
         current_time: SystemTime,
         maximum_clock_skew: Duration,
@@ -368,7 +384,6 @@ impl TicketGrantTimeBound {
         let end_time = tgs_req_end_time(
             start_time,
             tgs_req_valid.requested_end_time(),
-            client_tgt.end_time(),
             client_tgt.renew_until(),
             maximum_service_ticket_lifetime,
         )?;
@@ -384,6 +399,7 @@ impl TicketGrantTimeBound {
     }
 }
 
+#[tracing::instrument]
 fn tgs_req_start_time(
     current_time: SystemTime,
     requested_start_time: Option<SystemTime>,
@@ -400,8 +416,10 @@ fn tgs_req_start_time(
         requested_start_time
     };
 
-    // The requested start time can't be *less* than the tgt start time.
-    let requested_start_time = cmp::min(requested_start_time, client_tgt_start_time);
+    // The requested start time can't be *less* than the existing tgt start time.
+    if requested_start_time < client_tgt_start_time {
+        return Err(TimeBoundError::Skew);
+    }
 
     // The requested start time needs to be "near" the current time at least.
     if !is_within_allowed_skew(current_time, requested_start_time, maximum_clock_skew) {
@@ -416,41 +434,46 @@ fn tgs_req_start_time(
     Ok(requested_start_time)
 }
 
+#[tracing::instrument]
 fn tgs_req_end_time(
     start_time: SystemTime,
     requested_end_time: SystemTime,
-    client_tgt_end_time: SystemTime,
     client_tgt_renew_until: Option<SystemTime>,
     maximum_service_ticket_lifetime: Duration,
 ) -> Result<SystemTime, TimeBoundError> {
-    // Clamp the end time to the maximum allowable.
-    let requested_end_time = match requested_end_time.duration_since(start_time) {
-        Ok(diff) => {
-            if diff > maximum_service_ticket_lifetime {
-                // Clamp to the maximum lifetime.
-                start_time + maximum_service_ticket_lifetime
-            } else {
-                // It's less than, so this time is valid.
-                requested_end_time
-            }
-        }
-        // Some clients send epoch to mean "just fuck my shit up fam", so
-        // we set a reasonable default here.
-        Err(_) if requested_end_time == SystemTime::UNIX_EPOCH => {
-            start_time + maximum_service_ticket_lifetime
-        }
-        // The end time was less than start time.
-        Err(_) => return Err(TimeBoundError::NeverValid),
+    let policy_end_time = start_time + maximum_service_ticket_lifetime;
+
+    let mut requested_end_time = if requested_end_time == SystemTime::UNIX_EPOCH {
+        trace!(?policy_end_time, "req epoch");
+        policy_end_time
+    } else {
+        trace!(
+            ?policy_end_time,
+            ?requested_end_time,
+            "min of request + policy"
+        );
+        cmp::min(requested_end_time, policy_end_time)
     };
 
-    // We bound to either the renew time if present, or the tgt_end time.
-    let clamp_bound = client_tgt_renew_until.unwrap_or(client_tgt_end_time);
+    if let Some(client_tgt_renew_until) = client_tgt_renew_until {
+        trace!(
+            ?client_tgt_renew_until,
+            ?requested_end_time,
+            "min of request + renew"
+        );
+        // The client_tgt_renew_until is the "true" expiration of the the session.
+        requested_end_time = cmp::min(client_tgt_renew_until, requested_end_time);
+    };
 
-    let requested_end_time = cmp::min(requested_end_time, clamp_bound);
+    if requested_end_time < start_time {
+        // The end time was less than start time.
+        return Err(TimeBoundError::NeverValid);
+    };
 
     Ok(requested_end_time)
 }
 
+#[derive(Debug)]
 pub struct TicketRenewTimeBound {
     start_time: SystemTime,
     end_time: SystemTime,
@@ -470,6 +493,7 @@ impl TicketRenewTimeBound {
         self.renew_until
     }
 
+    #[tracing::instrument]
     pub fn from_tgs_req(
         current_time: SystemTime,
         maximum_clock_skew: Duration,
@@ -486,6 +510,12 @@ impl TicketRenewTimeBound {
 
         let client_tgt = tgs_req_valid.ticket_granting_ticket();
 
+        // Ensure that current_time is at least equal or greater
+        // than the tgt auth_time (when it was issued). This checks
+        // for clock step backs.
+
+        let current_time = cmp::max(current_time, client_tgt.auth_time());
+
         // We currently default the renew until here to the client tgt, but in
         // future we may be able to make server aware choices to clamp this during
         // the renewal to expire sessions of bad actors.
@@ -500,13 +530,14 @@ impl TicketRenewTimeBound {
             tgs_req_valid.requested_start_time(),
             maximum_clock_skew,
             client_tgt.start_time(),
-            client_tgt.end_time(),
+            // For a renewable user ticket, the end time here is when the renewal
+            // period ends, not the end time of the original tgt.
+            renew_until,
         )?;
 
         let end_time = tgs_req_end_time(
             start_time,
             tgs_req_valid.requested_end_time(),
-            client_tgt.end_time(),
             client_tgt.renew_until(),
             maximum_ticket_lifetime,
         )?;