Add old behavior under feature flag

Author: MaximeKjaer

Kerberos.NET/Entities/Krb/KrbKdcRep.cs

@@ -243,6 +243,8 @@ private static KrbPrincipalName CreateCNameForTicket(ServiceTicketRequest reques
 #pragma warning disable CS0618 // Type or member is obsolete
             if (!string.IsNullOrEmpty(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,
@@ -256,7 +258,8 @@ private static KrbPrincipalName CreateCNameForTicket(ServiceTicketRequest reques
             // 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.
+            // 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.

Kerberos.NET/Server/KdcAsReqMessageHandler.cs

@@ -207,7 +207,17 @@ private ReadOnlyMemory<byte> GenerateAsRep(KrbAsReq asReq, PreAuthenticationCont
             // and types in the AS response and ticket returned from those in the request.
             if (!asReq.Body.KdcOptions.HasFlag(KdcOptions.Canonicalize))
             {
-                rst.ClientName = asReq.Body.CName;
+                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,24 +265,7 @@ public override ReadOnlyMemory<byte> ExecuteCore(PreAuthenticationContext contex
 
             var rst = new ServiceTicketRequest
             {
-                // 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."
-                ClientName = context.Ticket.CName,
                 ClientRealmName = context.Ticket.CRealm,
-
                 KdcAuthorizationKey = context.EvidenceTicketKey,
                 Principal = context.Principal,
                 EncryptedPartKey = context.EncryptedPartKey,
@@ -307,17 +290,35 @@ public override ReadOnlyMemory<byte> ExecuteCore(PreAuthenticationContext contex
                 Compatibility = this.RealmService.Settings.Compatibility,
             };
 
-            // 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.
-
-            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))
             {
-                rst.ClientName = null;
-#pragma warning disable CS0612 // Type or member is obsolete
+                // 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
+                #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,
     }
 }

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();