Merge pull request #407 from MaximeKjaer/cname-copy

Ensure cname precisely matches what was in request

Author: SteveSyfuhs

Kerberos.NET/Entities/Krb/KrbKdcRep.cs

@@ -194,8 +194,6 @@ private static KrbEncTicketPart CreateEncTicketPart(
             KrbEncryptionKey sessionKey
         )
         {
-            var cname = CreateCNameForTicket(request);
-
             var flags = request.Flags;
 
             if (request.PreAuthenticationData?.Any(r => r.Type == PaDataType.PA_REQ_ENC_PA_REP) ?? false)
@@ -209,7 +207,7 @@ KrbEncryptionKey sessionKey
 
             var encTicketPart = new KrbEncTicketPart()
             {
-                CName = cname,
+                CName = CreateCNameForTicket(request),
                 CRealm = request.Compatibility.HasFlag(KerberosCompatibilityFlags.IsolateRealmsConsistently) ? request.ClientRealmName : request.RealmName,
                 Key = sessionKey,
                 AuthTime = request.Now,
@@ -233,21 +231,44 @@ KrbEncryptionKey sessionKey
 
         private static KrbPrincipalName CreateCNameForTicket(ServiceTicketRequest request)
         {
-            if (string.IsNullOrEmpty(request.SamAccountName))
+            // If ClientName is explicitly set, use that. This is the preferred method for the caller to indicate what
+            // cname should be used.
+            if (request.ClientName != null)
             {
-                return KrbPrincipalName.FromPrincipal(
-                    request.Principal,
-                    realm: request.Compatibility.HasFlag(KerberosCompatibilityFlags.IsolateRealmsConsistently) ?
-                        request.ClientRealmName :
-                        request.RealmName
-                );
+                return request.ClientName;
             }
 
-            return new KrbPrincipalName
+            // Otherwise, if SamAccountName is set, use that as the principal name.
+            // This is not the recommended method, but is supported for backwards compatibility.
+#pragma warning disable CS0618 // Type or member is obsolete
+            if (!string.IsNullOrEmpty(request.SamAccountName))
             {
-                Type = PrincipalNameType.NT_PRINCIPAL,
-                Name = new[] { request.SamAccountName }
-            };
+                // Note that the name is returned in a single part here, even if the request may have had multiple parts.
+                // This may be okay for AS-REQs with Canonicalize set, but it is not spec-compliant for other scenarios.
+                return new KrbPrincipalName
+                {
+                    Type = PrincipalNameType.NT_PRINCIPAL,
+                    Name = new[] { request.SamAccountName }
+                };
+            }
+#pragma warning restore CS0618 // Type or member is obsolete
+
+            // Lastly, if neither are set, derive from the principal.
+            //
+            // Note: this might not be correct in all scenarios. For instance, if the client does not accept
+            // name canonicalization (i.e., the Canonicalize flag is not set), then it's not spec-compliant to deviate
+            // from the requested cname. Also, in TGS-REP, the cname should match what's in the TGT, and should not be
+            // derived from the found principal. It is the responsibility of the caller to decide whether the request
+            // warrants passing Principal only, or forcing a specific cname via ClientName.
+            //
+            // Note: historically Kerberos.NET had a bug where the service realm was used to derive cname from the principal.
+            // However, it should be the client realm. This has been corrected under the IsolateRealmsConsistently flag.
+            return KrbPrincipalName.FromPrincipal(
+                request.Principal,
+                realm: request.Compatibility.HasFlag(KerberosCompatibilityFlags.IsolateRealmsConsistently) ?
+                    request.ClientRealmName :
+                    request.RealmName
+            );
         }
 
         private static IEnumerable<KrbAuthorizationData> GenerateAuthorizationData(ServiceTicketRequest request)

Kerberos.NET/Entities/Krb/ServiceTicketRequest.cs

@@ -28,7 +28,13 @@ public struct ServiceTicketRequest : IEquatable<ServiceTicketRequest>
         public KerberosKey KdcAuthorizationKey { get; set; }
 
         /// <summary>
-        /// The realm name for which the requested identity originated
+        /// The client name (cname) of the identity requesting the ticket.
+        /// If this is not set, <see cref="SamAccountName"/> or <see cref="Principal"/> will be used.
+        /// </summary>
+        public KrbPrincipalName ClientName { get; set; }
+
+        /// <summary>
+        /// The client realm (crealm) name of the identity requesting the ticket
         /// </summary>
         public string ClientRealmName { get; set; }
 
@@ -110,9 +116,12 @@ public struct ServiceTicketRequest : IEquatable<ServiceTicketRequest>
 
         /// <summary>
         /// SAM account name to be used to generate TGT for Windows specific user principal.
-        /// If this parameter contains valid string (not empty), CName of encrypted part of ticket
-        /// will be created based on provided SamAccountName.
+        /// This is only used if (1) <see cref="ClientName"/> is not set, and (2) it is a valid (not empty) string.
+        /// Used to compute the cname of a KDC-REP.
         /// </summary>
+        [Obsolete(
+            "Using SamAccountName may cause non spec-compliant behavior. Use ClientName instead to set the client " +
+            "principal name that should be used in Kerberos responses in a spec-compliant manner.")]
         public string SamAccountName { get; set; }
 
         /// <summary>
@@ -179,6 +188,21 @@ public bool Equals(ServiceTicketRequest other)
                 return false;
             }
 
+            if (other.ClientName != this.ClientName)
+            {
+                return false;
+            }
+
+            if (other.ClientRealmName != this.ClientRealmName)
+            {
+                return false;
+            }
+
+            if (other.EncryptedPartEType != this.EncryptedPartEType)
+            {
+                return false;
+            }
+
             if (other.EncryptedPartKey != this.EncryptedPartKey)
             {
                 return false;
@@ -249,10 +273,12 @@ public bool Equals(ServiceTicketRequest other)
                 return false;
             }
 
+#pragma warning disable CS0618 // Type or member is obsolete
             if (other.SamAccountName != this.SamAccountName)
             {
                 return false;
             }
+#pragma warning restore CS0618 // Type or member is obsolete
 
             if (other.ServicePrincipal != this.ServicePrincipal)
             {
@@ -279,8 +305,13 @@ public bool Equals(ServiceTicketRequest other)
 
         public override int GetHashCode()
         {
+#pragma warning disable CS0618 // Type or member is obsolete
             return EntityHashCode.GetHashCode(
                 this.Addresses,
+                this.ClientName,
+                this.ClientRealmName,
+                this.Compatibility,
+                this.EncryptedPartEType,
                 this.EncryptedPartKey,
                 this.EndTime,
                 this.Flags,
@@ -301,6 +332,7 @@ public override int GetHashCode()
                 this.StartTime,
                 this.Compatibility
             );
+#pragma warning restore CS0618 // Type or member is obsolete
         }
 
         public static bool operator ==(ServiceTicketRequest left, ServiceTicketRequest right)

Kerberos.NET/Server/KdcAsReqMessageHandler.cs

@@ -167,6 +167,7 @@ private ReadOnlyMemory<byte> GenerateAsRep(KrbAsReq asReq, PreAuthenticationCont
 
             var rst = new ServiceTicketRequest
             {
+                ClientRealmName = asReq.Body.Realm,
                 Principal = context.Principal,
                 EncryptedPartKey = context.EncryptedPartKey,
                 EncryptedPartEType = context.EncryptedPartEType,
@@ -193,10 +194,30 @@ private ReadOnlyMemory<byte> GenerateAsRep(KrbAsReq asReq, PreAuthenticationCont
 
             // Canonicalize means the CName in the reply is allowed to be different from the CName in the request.
             // If this is not allowed, then we must use the CName from the request. Otherwise, we will set the CName
-            // to what we have in our realm, i.e. user@realm.
+            // to what we have in our realm, i.e. user@realm (which will be inferred from the Principal set above).
+            //
+            // RFC 4120 section 3.1.5. Receipt of KRB_AS_REP Message
+            // -----------------------------------------------------
+            // If the reply message type is KRB_AS_REP, then the client verifies that the cname and crealm fields in
+            // the cleartext portion of the reply match what it requested.
+            //
+            // RFC 6806 section 6. Name Canonicalization
+            // -----------------------------------------
+            // If the "canonicalize" KDC option is set, then the KDC MAY change the client and server principal names
+            // and types in the AS response and ticket returned from those in the request.
             if (!asReq.Body.KdcOptions.HasFlag(KdcOptions.Canonicalize))
             {
-                rst.SamAccountName = asReq.Body.CName.FullyQualifiedName;
+                if (this.RealmService.Settings.Compatibility.HasFlag(KerberosCompatibilityFlags.EnableSpecCompliantCNameHandling))
+                {
+                    rst.ClientName = asReq.Body.CName;
+                }
+                else
+                {
+                    #pragma warning disable CS0618 // Type or member is obsolete
+                    rst.SamAccountName = asReq.Body.CName.FullyQualifiedName;
+                    #pragma warning restore CS0618 // Type or member is obsolete
+                }
+                
             }
 
             if (rst.EncryptedPartKey == null)

Kerberos.NET/Server/KdcTgsReqMessageHandler.cs

@@ -265,13 +265,13 @@ public override ReadOnlyMemory<byte> ExecuteCore(PreAuthenticationContext contex
 
             var rst = new ServiceTicketRequest
             {
+                ClientRealmName = context.Ticket.CRealm,
                 KdcAuthorizationKey = context.EvidenceTicketKey,
                 Principal = context.Principal,
                 EncryptedPartKey = context.EncryptedPartKey,
                 EncryptedPartEType = context.EncryptedPartEType,
                 ServicePrincipal = context.ServicePrincipal,
                 ServicePrincipalKey = serviceKey,
-                ClientRealmName = context.ClientRealm,
                 RealmName = tgsReq.Body.Realm,
                 Addresses = tgsReq.Body.Addresses,
                 RenewTill = context.Ticket.RenewTill,
@@ -290,13 +290,35 @@ public override ReadOnlyMemory<byte> ExecuteCore(PreAuthenticationContext contex
                 Compatibility = this.RealmService.Settings.Compatibility,
             };
 
-            // this introduced an annoying regression in a separate party and this is a workaround to make sure it
-            // uses the original behavior in cases where that's expected
-
-            if (!this.RealmService.Settings.Compatibility.HasFlag(KerberosCompatibilityFlags.DoNotCanonicalizeTgsReqFromTgt) &&
+            if (this.RealmService.Settings.Compatibility.HasFlag(KerberosCompatibilityFlags.EnableSpecCompliantCNameHandling))
+            {
+                // In TGS-REP, we should always reply with cname/crealm copied from the TGT, even when the Canonicalize
+                // flag is set in TGS-REQ.
+                //
+                // RFC 4120 § 3.3.3 Generation of KRB_TGS_REP Message
+                // --------------------------------------------------
+                // "By default, the address field, the client's name and realm, the list of transited realms, the time
+                // of initial authentication, the expiration time, and the authorization data of the newly-issued
+                // ticket will be copied from the TGT or renewable ticket."
+                //
+                // RFC 6806 § 6. Name Canonicalization
+                // -----------------------------------
+                // "If the "canonicalize" KDC option is set, then the KDC MAY change the client and server principal
+                // names and types in the AS response and ticket returned from those in the request. Names MUST NOT be
+                // changed in the response to a TGS request, although it is common for KDCs to maintain a set of
+                // aliases for service principals."
+                rst.ClientName = context.Ticket.CName;
+            }
+            else if (!this.RealmService.Settings.Compatibility.HasFlag(KerberosCompatibilityFlags.DoNotCanonicalizeTgsReqFromTgt) &&
                 tgsReq.Body.KdcOptions.HasFlag(KdcOptions.Canonicalize))
             {
+                // The code below introduced an annoying regression in a separate party.
+                // The compatibility flag is a workaround to make sure it can use the original behavior in cases where
+                // that's expected.
+
+                #pragma warning disable CS0612 // Type or member is obsolete
                 rst.SamAccountName = context.GetState<TgsState>(PaDataType.PA_TGS_REQ).DecryptedApReq.Ticket.CName.FullyQualifiedName;
+                #pragma warning restore CS0612 // Type or member is obsolete
             }
 
             // this is set here instead of in GenerateServiceTicket because GST is used by unit tests to

Kerberos.NET/Server/KerberosCompatibilityFlags.cs

@@ -33,5 +33,12 @@ public enum KerberosCompatibilityFlags
         /// fields or properties. This separates the names into two.
         /// </summary>
         IsolateRealmsConsistently = 1 << 2,
+
+        /// <summary>
+        /// CName handling was historically non-spec compliant in some cases.
+        /// This flag enables handling that more strictly adheres to the spec, for better compliance
+        /// with other implementations.
+        /// </summary>
+        EnableSpecCompliantCNameHandling = 1 << 3,
     }
 }

Kerberos.NET/Server/PaDataTgsTicketHandler.cs

@@ -105,7 +105,6 @@ public override KrbPaData Validate(KrbKdcReq asReq, PreAuthenticationContext con
 
             context.EncryptedPartKey = state.DecryptedApReq.SessionKey;
             context.Ticket = state.DecryptedApReq.Ticket;
-            context.ClientRealm = state.DecryptedApReq.Ticket.CRealm;
 
             return null;
         }

Kerberos.NET/Server/PreAuthenticationContext.cs

@@ -32,11 +32,6 @@ public class PreAuthenticationContext
         /// </summary>
         public KerberosKey EvidenceTicketKey { get; set; }
 
-        /// <summary>
-        /// The name of the realm that the client issued a TGT from.
-        /// </summary>
-        public string ClientRealm { get; set; }
-
         /// <summary>
         /// The identity that will be the subject of the issued ticket.
         /// </summary>

Tests/Tests.Kerberos.NET/Client/HttpKerberosTransportTests.cs

@@ -92,6 +92,8 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques
 
                 var rst = new ServiceTicketRequest
                 {
+                    ClientName = KrbPrincipalName.FromPrincipal(principal),
+                    ClientRealmName = Realm,
                     Principal = principal,
                     EncryptedPartKey = principalKey,
                     ServicePrincipalKey = new KerberosKey(key: TgtKey, etype: EncryptionType.AES256_CTS_HMAC_SHA1_96)

Tests/Tests.Kerberos.NET/FakeRealmService.cs

@@ -13,7 +13,7 @@ public class FakeRealmService : IRealmService
     {
         private readonly KerberosCompatibilityFlags compatibilityFlags;
 
-        public FakeRealmService(string realm, Krb5Config config = null, KerberosCompatibilityFlags compatibilityFlags = KerberosCompatibilityFlags.IsolateRealmsConsistently)
+        public FakeRealmService(string realm, Krb5Config config = null, KerberosCompatibilityFlags compatibilityFlags = KerberosCompatibilityFlags.IsolateRealmsConsistently | KerberosCompatibilityFlags.EnableSpecCompliantCNameHandling)
         {
             this.Name = realm;
             this.Configuration = config ?? Krb5Config.Kdc();