feat(DRIVERS-1803): add new canonicalize host name opts #1135

durran · 2022-02-03T13:49:23Z

For GSSAPI this updates the values accepted in the CANONICALIZE_HOST_NAME auth mechanism properties. These new values are:

"none" : No canonicalization is performed.
"forward" : A forward DNS lookup is performed to canonicalize the host name.
"forwardAndReverse" : A forward DNS lookup followed by a reverse lookup on that value is performed.

Drivers may also continue to support the old boolean values if desired in addition to these new values. true is equivalent to "forwardAndReverse" and false is equivalent to "none".

Node reference impl: mongodb/node-mongodb-native#3131

JamesKovacs

Overall looks reasonable. Some clarification required along with fixing the Python sample impl.

JamesKovacs · 2022-02-03T22:44:30Z

source/auth/auth.rst

-  reversed = do a reverse DNS lookup for address
-  if reversed:
+  if forwardAndReverse:
+    reversed = do a reverse DNS lookup for address


CANONICALIZE_HOST_NAME is currently equivalent to forwardAndReverse. This PR changes it to mean forward. Is that the correct behaviour? Why are we potentially breaking backward compat?

As well the Python impl below does not take true versus forward versus forwardAndReverse into account. It still implements the old pseudo code where only true was applicable and had the same meaning as forwardAndReverse.

Given that we are asking drivers to implement (or wrap libgssapi) to canonicalize host names, do we want to add some host name canonicalization tests to ensure that drivers are performing canonicalization correctly? It seems dangerous to leave this behaviour testing up to each driver individually as it may give rise to differences in implementation.

Good catch. I was basing this off the Node implementation which doesn't do the reverse lookup so I'll update to make forwardAndReverse the default. Also yes I will add some more tests as well and ping you when the changes are made.

I've updated to default true to being "forwardAndReverse", updated the python impl, and added spec test for raising an error on invalid values.

The Java driver calls java.net.InetAddress#getByName(java.lang.String), followed by java.net.InetAddress#getCanonicalHostName. I think that's the equivalent of forwardAndReverse.... Can you confirm?

Yes that is the equivalent of forwardAndReverse.

JamesKovacs · 2022-02-03T22:52:00Z

source/auth/auth.rst

@@ -468,7 +468,7 @@ mechanism_properties
 Hostname Canonicalization
 `````````````````````````

-If CANONICALIZE_HOST_NAME is true, the client MUST canonicalize the name of each host it uses for authentication. There are two options. First, if the client's underlying GSSAPI library provides hostname canonicalization, the client MAY rely on it. For example, MIT Kerberos has `a configuration option for canonicalization <https://web.mit.edu/kerberos/krb5-1.13/doc/admin/princ_dns.html#service-principal-canonicalization>`_.
+If CANONICALIZE_HOST_NAME is true, "forward", or "forwardAndReverse", the client MUST canonicalize the name of each host it uses for authentication. There are two options. First, if the client's underlying GSSAPI library provides hostname canonicalization, the client MAY rely on it. For example, MIT Kerberos has `a configuration option for canonicalization <https://web.mit.edu/kerberos/krb5-1.13/doc/admin/princ_dns.html#service-principal-canonicalization>`_.


What are the valid values for CANONICALIZE_HOST_NAME. Based on this statement, valid values include true, "forward", and "forwardAndReverse". I assume false would also be valid. In the spec tests below, "none" is also used.

Should true/false be deprecated? If not, should false or "none" be used when disabling canonicalization.

What is the expected behaviour for an unknown value like "off" or "on"? Should an error be raised? Should it default to true? Or false? My concern here is what if someone uses "forwardReverse" rather than "forwardAndReverse" by mistake? Or if they type "foward" (misspelling)?

We should clearly enumerate the allowed values for CANONICALIZE_HOST_NAMES and specify whether an error or warning should be raised for unknown values.

I've called out now all valid values in the spec for CANONICALIZE_HOST_NAME and specified that drivers MUST error if the value provided is not one of them. I've decided not to deprecate true and false and just say that drivers MAY allow them, since I figured many apps may depend on true and false and not want to change them as these changes were driven by the dev tools team.

durran · 2022-02-04T13:26:59Z

Node reference impl: mongodb/node-mongodb-native#3131

JamesKovacs

Looking good. Just one main outstanding question around MAY/MUST and tests.

JamesKovacs · 2022-02-09T16:58:45Z

source/auth/auth.rst

@@ -460,7 +460,7 @@ mechanism_properties
 		Drivers MUST allow the user to specify a different service name. The default is "mongodb".

 	CANONICALIZE_HOST_NAME
-		Drivers MAY allow the user to request canonicalization of the hostname. This might be required when the hosts report different hostnames than what is used in the kerberos database. The default is "false".
+		Drivers MAY allow the user to request canonicalization of the hostname. This might be required when the hosts report different hostnames than what is used in the kerberos database. The value is a string of either "none", "forward", or "forwardAndReverse". "none" is the default and performs no canonicalization. "forward" performs a forward DNS lookup to canonicalize the hostname. "forwardAndReverse" performs a forward DNS lookup and then a reverse lookup on that value to canonicalize the hostname. The driver MUST error if any lookup errors or returns no results. Drivers MAY decide to also keep the legacy boolean values where `true` equals the "forwardAndReverse" behaviour and `false` equals "none".


Thank you for explicitly specifying the options. I agree that keeping the legacy boolean values makes sense for backwards compatibility. However you say "Driver MAY decide to also keep the legacy boolean values" but later on have tests requiring this behaviour. We have to either require drivers to accept the boolean values or remove the test that asserts that boolean values work.

I've gone and removed those tests now for the MAY wording.

JamesKovacs · 2022-02-09T17:00:03Z

source/auth/tests/connection-string.json

        }
      }
    },
+    {
+      "description": "should accept true as hostname canonicalization (GSSAPI)",


See comment above. We can only include this test if drivers "MUST" accept boolean values. Right now they "MAY" and any driver not accepting boolean values would fail this test.

I've removed the tests now for true/false values.

JamesKovacs · 2022-02-09T17:01:01Z

source/uri-options/tests/auth-options.json

@@ -2,7 +2,7 @@
  "tests": [
    {
      "description": "Valid auth options are parsed correctly (GSSAPI)",
-      "uri": "mongodb://foo:bar@example.com/?authMechanism=GSSAPI&authMechanismProperties=SERVICE_NAME:other,CANONICALIZE_HOST_NAME:true,SERVICE_HOST:example.com&authSource=$external",
+      "uri": "mongodb://foo:bar@example.com/?authMechanism=GSSAPI&authMechanismProperties=SERVICE_NAME:other,CANONICALIZE_HOST_NAME:forwar,SERVICE_HOST:example.comd&authSource=$external",


forwar => forward
example.comd => example.com

JamesKovacs · 2022-02-09T17:02:33Z

source/uri-options/tests/auth-options.yml

@@ -1,7 +1,7 @@
 tests:
    -
        description: "Valid auth options are parsed correctly (GSSAPI)"
-        uri: "mongodb://foo:bar@example.com/?authMechanism=GSSAPI&authMechanismProperties=SERVICE_NAME:other,CANONICALIZE_HOST_NAME:true,SERVICE_HOST:example.com&authSource=$external"
+        uri: "mongodb://foo:bar@example.com/?authMechanism=GSSAPI&authMechanismProperties=SERVICE_NAME:other,CANONICALIZE_HOST_NAME:forwar,SERVICE_HOST:example.comd&authSource=$external"


forwar => forward
example.comd => example.com

JamesKovacs

LGTM

durran requested a review from a team as a code owner February 3, 2022 13:49

durran requested review from JamesKovacs and removed request for a team February 3, 2022 13:49

durran requested a review from a team as a code owner February 3, 2022 14:18

JamesKovacs requested changes Feb 3, 2022

View reviewed changes

durran requested a review from JamesKovacs February 4, 2022 12:52

durran added 4 commits February 4, 2022 15:45

feat(DRIVERS-1803): add new canonicalize host name opts

f28ac7e

test(DRIVERS-1803): add new canonicalization tests

c23cf8b

feat(DRIVERS-1803): make forwardAndReverse the default

213701a

test(DRIVERS-1803): add true/false canonicalization tests

6172ff7

durran force-pushed the DRIVERS-1803 branch from 4c07c3c to 6172ff7 Compare February 4, 2022 14:51

durran requested a review from jyemin February 9, 2022 15:44

JamesKovacs requested changes Feb 9, 2022

View reviewed changes

test(DRIVERS-1803): remove true/false tests, fix typo

afd1202

durran requested a review from JamesKovacs February 10, 2022 12:50

JamesKovacs approved these changes Feb 10, 2022

View reviewed changes

durran merged commit 30e6164 into master Feb 10, 2022

durran deleted the DRIVERS-1803 branch February 10, 2022 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(DRIVERS-1803): add new canonicalize host name opts #1135

feat(DRIVERS-1803): add new canonicalize host name opts #1135

durran commented Feb 3, 2022 •

edited

JamesKovacs left a comment

JamesKovacs Feb 3, 2022

JamesKovacs Feb 3, 2022

JamesKovacs Feb 3, 2022

durran Feb 4, 2022

durran Feb 4, 2022 •

edited

jyemin Feb 4, 2022 •

edited

durran Feb 4, 2022

JamesKovacs Feb 3, 2022

durran Feb 4, 2022

durran commented Feb 4, 2022

JamesKovacs left a comment

JamesKovacs Feb 9, 2022

durran Feb 10, 2022

JamesKovacs Feb 9, 2022

durran Feb 10, 2022

JamesKovacs Feb 9, 2022

durran Feb 10, 2022

JamesKovacs Feb 9, 2022

durran Feb 10, 2022

JamesKovacs left a comment

feat(DRIVERS-1803): add new canonicalize host name opts #1135

feat(DRIVERS-1803): add new canonicalize host name opts #1135

Conversation

durran commented Feb 3, 2022 • edited

JamesKovacs 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

durran Feb 4, 2022 • edited

Choose a reason for hiding this comment

jyemin Feb 4, 2022 • edited

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

durran commented Feb 4, 2022

JamesKovacs 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

JamesKovacs left a comment

Choose a reason for hiding this comment

durran commented Feb 3, 2022 •

edited

durran Feb 4, 2022 •

edited

jyemin Feb 4, 2022 •

edited