search

dotnet / Kerberos.NET

A Kerberos implementation built entirely in managed code.
MIT License
520 stars 91 forks source link

Initial support for `dns_canonicalize_hostname = fallback` #330

Closed MageFroh closed 1 year ago

MageFroh commented 1 year ago

This allows successful de-serialisation of krb5.conf (for example via Krb5Config.CurrentUser()) on Linux systems that come with fallback as the default value.

fallback is currently treated the same as true.

What's the problem?

Before this change, and exception would be thrown when de-serialising /etc/krb5.conf on a fresh RHEL 9 installation.

What's the solution?

Simply allows the new fallback value.

What issue is this related to, if any?

Fixes #329

dnfadmin commented 1 year ago

CLA assistant check
All CLA requirements met.

MageFroh commented 1 year ago

A few open questions that we could discuss:

  1. This break code compatibility with any consuming code setting Krb5ConfigDefaults.DnsCanonicalizeHostname to a bool value. Is that OK, or should I try to be a bit more backward compatible somehow?
  2. Implementing proper fallback behaviour would require some work in KerberosClient.GetServiceTicket(), and I don't understand Kerberos well enough to do it in this PR. Is it OK to treat it the same as true for now? Let me know if you think it should be the same as false instead.
  3. I've noticed that the default value of DnsCanonicalizeHostname is false in Kerberos.NET, but MIT Kerberos claims it is true. I've kept this discrepancy. But just to be sure, is it a conscious decision?
MageFroh commented 1 year ago

Looks like the build failed at the Nuget Push stage, but I'm not entirely sure why. Any ideas about how to fix this?

SteveSyfuhs commented 1 year ago

Build failure is normal at the nuget stage with PRs from folks outside the org. It really should be configured not to try to push nuget packages on PRs at all, but thats a problem for another day. That said, because it got there that means the build and tests did pass. This looks good. You have added test coverage and while it is a minor breaking change to the API, I'm not terribly concerned because it's a fairly obscure config for someone to set via code. We'll bump the version to indicate. I'll merge this change and get a new package uploaded shortly.

SteveSyfuhs commented 1 year ago

And its live. Thanks for the PR!

https://www.nuget.org/packages/Kerberos.NET/4.6.1

MageFroh commented 1 year ago

Excellent, thanks for reviewing quickly!