Add Kerberos loopback test #71824

filipnavara · 2022-07-08T11:00:13Z

Introduces an infrastructure to run Kerberos.NET powered KDC inside a unit test and to redirect the system GSSAPI libraries to connect to it. This enables testing Kerberos scenarios without special Docker containers and also allows testing on macOS which had no coverage.

TODO:

Figure out how to consume Kerberos.NET (Arcade or at least publishing into the dotnet feeds)
Wire logging output into xUnit to make diagnostics easier
Add README
Update code style / fix up any credential scan issues
Remove unused bits of the test infrastructure (PKINIT, PAC)
Make the fake classes more robust and specify users explicitly
Fix the test to run on macOS again

Contributes to #71453

ghost · 2022-07-08T11:00:28Z

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

TODO:

Figure out how to consume Kerberos.NET (Arcade or at least publishing into the dotnet feeds)
Wire logging output into xUnit to make diagnostics easier

Contributes to #71453

Author:	filipnavara
Assignees:	-
Labels:	`area-System.Net.Security`, `community-contribution`
Milestone:	-

NuGet.config

wfurt · 2022-07-11T07:20:48Z

cc: @SteveSyfuhs in case we publish to some internal feds as well beyond NuGet

wfurt · 2022-07-11T09:40:03Z

contributes to #68067. If we can run Kerberos tests during normal run, we don't need special reporting (and possibly separate pipeline)

filipnavara · 2022-07-11T09:43:18Z

I've played with this a bit and some of the enterprise tests can be converted to use this approach. I'm still planning to see if I can get the same thing done for NTLM.

SteveSyfuhs · 2022-07-11T17:14:48Z

cc: @SteveSyfuhs in case we publish to some internal feds as well beyond NuGet

Have at it. Let me know if there's anything you need.

…on to NTLM and Negotiate)

… test for invalid credentials

filipnavara · 2022-07-12T09:34:12Z

Not sure what are the guidelines for code style in tests. In particular, I used var and file-scoped namespaces. I'm fine with changing it to match preferred code style. Similarly, I am not sure what password usage would trip the credential scans, so I reused the passwords from the enterprise tests that prominently contain the word PLACEHOLDER.

Coding style guidelines don't seem to mention the file-scoped namespaces (cc @stephentoub).

filipnavara · 2022-07-12T10:10:50Z

I can also split off c6619e1 into separate PR to make it easier to review. Thoughts?

wfurt · 2022-07-12T14:37:05Z

I'm not sure if we should change the production code at this point. I would be happy with getting some coverage with Kerberos.Net. e.g. splitting/postponing c6619e1. I'm on the road once again, I should be able to take closer look by and of the week.

filipnavara · 2022-07-12T15:01:50Z

I'm not sure if we should change the production code at this point. I would be happy with getting some coverage with Kerberos.Net. e.g. splitting/postponing c6619e1.

Alright, I will split it (#72024). The reasoning behind c6619e1 is that it's a behavior difference between Unix-based OSes and Windows. Fixing it allows to test incorrect credentials which is test coverage for #71484. Without the change it's only possible to specify Negotiate authentication package which will fallback to NTLM instead of reporting the error from invalid credentials.

…n addition to NTLM and Negotiate)" This reverts commit c6619e1.

wfurt

generally looks good to me.
sorry for the long delay @filipnavara.

wfurt · 2022-07-28T23:47:16Z

src/libraries/Common/tests/System/Net/Security/Kerberos/FakeKdcServer.cs

+    public FakeKdcServer(KdcServerOptions serverOptions)
+    {
+        _kdcServer = new KdcServer(serverOptions);
+        _tcpListener = new TcpListener(System.Net.IPAddress.Loopback, 0);


nit: IPAddress.Loopback? We may eventually figure out how to do dual mode but this is ok for now IMHO

wfurt · 2022-07-28T23:48:12Z

src/libraries/Common/tests/System/Net/Security/Kerberos/FakeKdcServer.cs

+    {
+        _kdcServer = new KdcServer(serverOptions);
+        _tcpListener = new TcpListener(System.Net.IPAddress.Loopback, 0);
+        _runningLock = new object();


Do we need new object?
object _runningLock => TcpListener ?

I guess we don't. It's a habit not to reuse other objects for locks because it's prone to misuse but I can change it.

wfurt · 2022-08-02T16:22:16Z

src/libraries/Common/tests/System/Net/Security/Kerberos/FakeKerberosPrincipal.cs

+
+class FakeKerberosPrincipal : IKerberosPrincipal
+{
+    private readonly byte[] _password;


nit: I would change the name to decrease chance we get flagged by security scans.

RFC-wise it's called "long term secret"

wfurt · 2022-08-02T16:24:26Z

src/libraries/Common/tests/System/Net/Security/Kerberos/FakeRealmReferral.cs

+
+        var krbName = KrbPrincipalName.FromString($"krbtgt/{predictedRealm}");
+
+        return new FakeKerberosPrincipal(PrincipalType.Service, krbName.FullyQualifiedName, predictedRealm, new byte[16]);


should we initialize the password to something? (may not matter but looks odd to me)

It's fine as long as it's consistently x00 everywhere and is never used for anything in production.

wfurt · 2022-08-02T16:26:23Z

src/libraries/Common/tests/System/Net/Security/Kerberos/KerberosExecutor.cs

+        _principalService = new FakePrincipalService(realm);
+
+        byte[] krbtgtPassword = new byte[16];
+        //RandomNumberGenerator.Fill(krbtgtPassword);


remove? Or set to some string if we need decrypt for debugging.

wfurt · 2022-08-02T16:27:56Z

src/libraries/Common/tests/System/Net/Security/Kerberos/KerberosExecutor.cs

+        _krb5Path = Path.GetTempFileName();
+        File.WriteAllText(_krb5Path,
+            OperatingSystem.IsLinux() ?
+            $"[realms]\n{_options.DefaultRealm} = {{\n  master_kdc = {endpoint}\n  kdc = {endpoint}\n}}\n" :


Why this depends on OS?

Different notation to force TCP over UDP and to specify the KDC as master.

wfurt · 2022-08-02T16:29:08Z

src/libraries/Common/tests/System/Net/Security/Kerberos/KerberosExecutor.cs

+        }
+
+        // Set environment variables for GSSAPI
+        Environment.SetEnvironmentVariable("KRB5_CONFIG", _krb5Path);


does this actually work? Last time (long time ago) I check this did not change the actual ENV e.g. was invisible to native bits.

Yes. That's why the test run inside RemoteExecutor. Environment.SetEnvironmentVariable doesn't call setenv but for newly launched processes it is used and that's what remote executor does.

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 8, 2022

dotnet-issue-labeler bot added the area-System.Net.Security label Jul 8, 2022

filipnavara commented Jul 8, 2022

View reviewed changes

NuGet.config Outdated Show resolved Hide resolved

filipnavara added 3 commits July 11, 2022 16:37

Add Kerberos loopback test

4e2dfa4

Revert NuGet.config changes

12b65a1

Be explicit about casing and realm name

f95e6e2

filipnavara force-pushed the kerberos-test branch from 65edc0a to f95e6e2 Compare July 11, 2022 16:38

filipnavara added 5 commits July 11, 2022 17:20

Remove PKINIT code from fakes

ff0c20c

Wire up KDC logging to Xunit test output

e928da5

Add support for specifying Kerberos package on Linux/macOS (in additi…

c6619e1

…on to NTLM and Negotiate)

Cleanup the fake KDC implementation, make user creation explicit, add…

027f967

… test for invalid credentials

Add README

67999aa

filipnavara marked this pull request as ready for review July 12, 2022 09:32

filipnavara requested a review from wfurt July 12, 2022 09:34

filipnavara added 2 commits July 12, 2022 15:09

Revert "Add support for specifying Kerberos package on Linux/macOS (i…

98c4f75

…n addition to NTLM and Negotiate)" This reverts commit c6619e1.

Remove Incorrect_Credentials test

fa99451

filipnavara force-pushed the kerberos-test branch from 8d14128 to 952dad5 Compare July 12, 2022 19:38

Add test for dotnet#71463 with invalid tokens

b2e8daf

filipnavara force-pushed the kerberos-test branch from 952dad5 to b2e8daf Compare July 12, 2022 19:39

karelz assigned wfurt Aug 2, 2022

wfurt approved these changes Aug 2, 2022

View reviewed changes

wfurt merged commit 84eb453 into dotnet:main Aug 3, 2022

karelz added this to the 7.0.0 milestone Aug 7, 2022

wfurt mentioned this pull request Aug 10, 2022

system.net.security.tests.negotiateauthenticationkerberostest.loopback_success #73343

Closed

ghost locked as resolved and limited conversation to collaborators Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Kerberos loopback test #71824

Add Kerberos loopback test #71824

filipnavara commented Jul 8, 2022 •

edited

Loading

ghost commented Jul 8, 2022

wfurt commented Jul 11, 2022

wfurt commented Jul 11, 2022

filipnavara commented Jul 11, 2022

SteveSyfuhs commented Jul 11, 2022

filipnavara commented Jul 12, 2022 •

edited

Loading

filipnavara commented Jul 12, 2022 •

edited

Loading

wfurt commented Jul 12, 2022

filipnavara commented Jul 12, 2022 •

edited

Loading

wfurt left a comment

wfurt Jul 28, 2022

wfurt Jul 28, 2022

filipnavara Aug 2, 2022

wfurt Aug 2, 2022

SteveSyfuhs Aug 2, 2022

wfurt Aug 2, 2022

SteveSyfuhs Aug 2, 2022

wfurt Aug 2, 2022

wfurt Aug 2, 2022

filipnavara Aug 2, 2022

wfurt Aug 2, 2022

filipnavara Aug 2, 2022 •

edited

Loading


		var krbName = KrbPrincipalName.FromString($"krbtgt/{predictedRealm}");

		return new FakeKerberosPrincipal(PrincipalType.Service, krbName.FullyQualifiedName, predictedRealm, new byte[16]);

Add Kerberos loopback test #71824

Add Kerberos loopback test #71824

Conversation

filipnavara commented Jul 8, 2022 • edited Loading

ghost commented Jul 8, 2022

wfurt commented Jul 11, 2022

wfurt commented Jul 11, 2022

filipnavara commented Jul 11, 2022

SteveSyfuhs commented Jul 11, 2022

filipnavara commented Jul 12, 2022 • edited Loading

filipnavara commented Jul 12, 2022 • edited Loading

wfurt commented Jul 12, 2022

filipnavara commented Jul 12, 2022 • edited Loading

wfurt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipnavara Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

filipnavara commented Jul 8, 2022 •

edited

Loading

filipnavara commented Jul 12, 2022 •

edited

Loading

filipnavara commented Jul 12, 2022 •

edited

Loading

filipnavara commented Jul 12, 2022 •

edited

Loading

filipnavara Aug 2, 2022 •

edited

Loading