Skip to content

PYTHON-3096 Finish implementation and tests for GSSAPI options #1985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Dec 31, 2024
Merged

PYTHON-3096 Finish implementation and tests for GSSAPI options #1985

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5518090
PYTHON-3096 Finish implementation and tests for GSSAPI options
blink1073 Oct 31, 2024
2c0242a
fix prop name
blink1073 Oct 31, 2024
5b69e09
fix prop handling
blink1073 Oct 31, 2024
5e04104
fix up matrix
blink1073 Oct 31, 2024
954ffce
fix test
blink1073 Oct 31, 2024
2a16ba1
fix test
blink1073 Oct 31, 2024
c73a87a
fix test
blink1073 Oct 31, 2024
cb852dd
fixup
blink1073 Oct 31, 2024
ec3c1f0
fix test
blink1073 Oct 31, 2024
8c2e48f
fix test
blink1073 Oct 31, 2024
55720fb
fix test
blink1073 Oct 31, 2024
9b7f9ae
undo changes to config
blink1073 Oct 31, 2024
20407bc
lint
blink1073 Oct 31, 2024
e180518
Merge branch 'master' of github.com:mongodb/mongo-python-driver into …
blink1073 Nov 18, 2024
b9a54f6
address review
blink1073 Nov 18, 2024
46d3fb9
fix handling of 'none'
blink1073 Nov 18, 2024
53a23b8
Merge branch 'master' of github.com:mongodb/mongo-python-driver into …
blink1073 Dec 31, 2024
b74725e
clean up script
blink1073 Dec 31, 2024
4b371de
fix script
blink1073 Dec 31, 2024
714f717
Merge branch 'master' of github.com:mongodb/mongo-python-driver into …
blink1073 Dec 31, 2024
397f1a1
address review
blink1073 Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions pymongo/asynchronous/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,13 +177,20 @@ def _auth_key(nonce: str, username: str, password: str) -> str:
return md5hash.hexdigest()


def _canonicalize_hostname(hostname: str) -> str:
def _canonicalize_hostname(hostname: str, option: str | bool) -> str:
"""Canonicalize hostname following MIT-krb5 behavior."""
# https://github.com/krb5/krb5/blob/d406afa363554097ac48646a29249c04f498c88e/src/util/k5test.py#L505-L520
if option in [False, "none"]:
return hostname

af, socktype, proto, canonname, sockaddr = socket.getaddrinfo(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be using asyncio's loop.getaddrinfo. Can you open a new ticket to fix that? We might have the same problem in other places.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://jira.mongodb.org/browse/PYTHON-5021

hostname, None, 0, 0, socket.IPPROTO_TCP, socket.AI_CANONNAME
)[0]

# For forward just to resolve the cname as dns.lookup() will not return it.
if option == "forward":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the string "none" is also valid:

def _validate_canonicalize_host_name(value: str | bool) -> str | bool:
    valid_names = [False, True, "none", "forward", "forwardAndReverse"]

Should that have the same behavior as False? Or should validate convert "none" to False?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to convert "none" to false, so the three validated options are True, False, and "forward".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no, the spec test is expecting it to validate as a string, so I updated the business logic instead.

return canonname.lower()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still confused by this method. DRIVERS-1803 says:

"forward" is equivalent to the current behaviour of true (a forward DNS lookup)

But here we are introducing new behavior for "forward". Is that because our behavior for "true" is actually "forwardAndReverse"? If so could you add a comment mentioning that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I misinterpreted the Node implementation and got confused. I'll redo the logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it was the description in the ticket that was incorrect. I verified by reading the kerberos spec, the drivers spec, and the node implementation that this is the correct implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also updated the ticket description


try:
name = socket.getnameinfo(sockaddr, socket.NI_NAMEREQD)
except socket.gaierror:
Expand All @@ -205,9 +212,8 @@ async def _authenticate_gssapi(credentials: MongoCredential, conn: AsyncConnecti
props = credentials.mechanism_properties
# Starting here and continuing through the while loop below - establish
# the security context. See RFC 4752, Section 3.1, first paragraph.
host = conn.address[0]
if props.canonicalize_host_name:
host = _canonicalize_hostname(host)
host = props.service_host or conn.address[0]
host = _canonicalize_hostname(host, props.canonicalize_host_name)
service = props.service_name + "@" + host
if props.service_realm is not None:
service = service + "@" + props.service_realm
Expand Down
14 changes: 10 additions & 4 deletions pymongo/synchronous/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,20 @@ def _auth_key(nonce: str, username: str, password: str) -> str:
return md5hash.hexdigest()


def _canonicalize_hostname(hostname: str) -> str:
def _canonicalize_hostname(hostname: str, option: str | bool) -> str:
"""Canonicalize hostname following MIT-krb5 behavior."""
# https://github.com/krb5/krb5/blob/d406afa363554097ac48646a29249c04f498c88e/src/util/k5test.py#L505-L520
if option in [False, "none"]:
return hostname

af, socktype, proto, canonname, sockaddr = socket.getaddrinfo(
hostname, None, 0, 0, socket.IPPROTO_TCP, socket.AI_CANONNAME
)[0]

# For forward just to resolve the cname as dns.lookup() will not return it.
if option == "forward":
return canonname.lower()

try:
name = socket.getnameinfo(sockaddr, socket.NI_NAMEREQD)
except socket.gaierror:
Expand All @@ -202,9 +209,8 @@ def _authenticate_gssapi(credentials: MongoCredential, conn: Connection) -> None
props = credentials.mechanism_properties
# Starting here and continuing through the while loop below - establish
# the security context. See RFC 4752, Section 3.1, first paragraph.
host = conn.address[0]
if props.canonicalize_host_name:
host = _canonicalize_hostname(host)
host = props.service_host or conn.address[0]
host = _canonicalize_hostname(host, props.canonicalize_host_name)
service = props.service_name + "@" + host
if props.service_realm is not None:
service = service + "@" + props.service_realm
Expand Down
69 changes: 64 additions & 5 deletions test/asynchronous/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import pytest

from pymongo import AsyncMongoClient, monitoring
from pymongo.asynchronous.auth import HAVE_KERBEROS
from pymongo.asynchronous.auth import HAVE_KERBEROS, _canonicalize_hostname
from pymongo.auth_shared import _build_credentials_tuple
from pymongo.errors import OperationFailure
from pymongo.hello import HelloCompat
Expand Down Expand Up @@ -96,10 +96,11 @@ def setUpClass(cls):
cls.service_realm_required = (
GSSAPI_SERVICE_REALM is not None and GSSAPI_SERVICE_REALM not in GSSAPI_PRINCIPAL
)
mech_properties = f"SERVICE_NAME:{GSSAPI_SERVICE_NAME}"
mech_properties += f",CANONICALIZE_HOST_NAME:{GSSAPI_CANONICALIZE}"
mech_properties = dict(
SERVICE_NAME=GSSAPI_SERVICE_NAME, CANONICALIZE_HOST_NAME=GSSAPI_CANONICALIZE
)
if GSSAPI_SERVICE_REALM is not None:
mech_properties += f",SERVICE_REALM:{GSSAPI_SERVICE_REALM}"
mech_properties["SERVICE_REALM"] = GSSAPI_SERVICE_REALM
cls.mech_properties = mech_properties

async def test_credentials_hashing(self):
Expand Down Expand Up @@ -167,7 +168,10 @@ async def test_gssapi_simple(self):
await client[GSSAPI_DB].collection.find_one()

# Log in using URI, with authMechanismProperties.
mech_uri = uri + f"&authMechanismProperties={self.mech_properties}"
mech_properties_str = ""
for key, value in self.mech_properties.items():
mech_properties_str += f"{key}:{value},"
mech_uri = uri + f"&authMechanismProperties={mech_properties_str[:-1]}"
client = self.simple_client(mech_uri)
await client[GSSAPI_DB].collection.find_one()

Expand Down Expand Up @@ -268,6 +272,61 @@ async def test_gssapi_threaded(self):
thread.join()
self.assertTrue(thread.success)

async def test_gssapi_canonicalize_host_name(self):
# Test the low level method.
assert GSSAPI_HOST is not None
result = _canonicalize_hostname(GSSAPI_HOST, "forward")
if "compute-1.amazonaws.com" not in result:
self.assertEqual(result, GSSAPI_HOST)
result = _canonicalize_hostname(GSSAPI_HOST, "forwardAndReverse")
self.assertEqual(result, GSSAPI_HOST)

# Use the equivalent named CANONICALIZE_HOST_NAME.
props = self.mech_properties.copy()
if props["CANONICALIZE_HOST_NAME"] == "true":
props["CANONICALIZE_HOST_NAME"] = "forwardAndReverse"
else:
props["CANONICALIZE_HOST_NAME"] = "none"
client = self.simple_client(
GSSAPI_HOST,
GSSAPI_PORT,
username=GSSAPI_PRINCIPAL,
password=GSSAPI_PASS,
authMechanism="GSSAPI",
authMechanismProperties=props,
)
await client.server_info()

if props["CANONICALIZE_HOST_NAME"] == "none":
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if-statement should be removed since it's redundant, or is there something missing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed


async def test_gssapi_host_name(self):
props = self.mech_properties
props["SERVICE_HOST"] = "example.com"

# Authenticate with authMechanismProperties.
client = self.simple_client(
GSSAPI_HOST,
GSSAPI_PORT,
username=GSSAPI_PRINCIPAL,
password=GSSAPI_PASS,
authMechanism="GSSAPI",
authMechanismProperties=self.mech_properties,
)
with self.assertRaises(OperationFailure):
await client.server_info()

props["SERVICE_HOST"] = GSSAPI_HOST
client = self.simple_client(
GSSAPI_HOST,
GSSAPI_PORT,
username=GSSAPI_PRINCIPAL,
password=GSSAPI_PASS,
authMechanism="GSSAPI",
authMechanismProperties=self.mech_properties,
)
await client.server_info()


class TestSASLPlain(AsyncPyMongoTestCase):
@classmethod
Expand Down
69 changes: 64 additions & 5 deletions test/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
from pymongo.hello import HelloCompat
from pymongo.read_preferences import ReadPreference
from pymongo.saslprep import HAVE_STRINGPREP
from pymongo.synchronous.auth import HAVE_KERBEROS
from pymongo.synchronous.auth import HAVE_KERBEROS, _canonicalize_hostname

_IS_SYNC = True

Expand Down Expand Up @@ -96,10 +96,11 @@ def setUpClass(cls):
cls.service_realm_required = (
GSSAPI_SERVICE_REALM is not None and GSSAPI_SERVICE_REALM not in GSSAPI_PRINCIPAL
)
mech_properties = f"SERVICE_NAME:{GSSAPI_SERVICE_NAME}"
mech_properties += f",CANONICALIZE_HOST_NAME:{GSSAPI_CANONICALIZE}"
mech_properties = dict(
SERVICE_NAME=GSSAPI_SERVICE_NAME, CANONICALIZE_HOST_NAME=GSSAPI_CANONICALIZE
)
if GSSAPI_SERVICE_REALM is not None:
mech_properties += f",SERVICE_REALM:{GSSAPI_SERVICE_REALM}"
mech_properties["SERVICE_REALM"] = GSSAPI_SERVICE_REALM
cls.mech_properties = mech_properties

def test_credentials_hashing(self):
Expand Down Expand Up @@ -167,7 +168,10 @@ def test_gssapi_simple(self):
client[GSSAPI_DB].collection.find_one()

# Log in using URI, with authMechanismProperties.
mech_uri = uri + f"&authMechanismProperties={self.mech_properties}"
mech_properties_str = ""
for key, value in self.mech_properties.items():
mech_properties_str += f"{key}:{value},"
mech_uri = uri + f"&authMechanismProperties={mech_properties_str[:-1]}"
client = self.simple_client(mech_uri)
client[GSSAPI_DB].collection.find_one()

Expand Down Expand Up @@ -268,6 +272,61 @@ def test_gssapi_threaded(self):
thread.join()
self.assertTrue(thread.success)

def test_gssapi_canonicalize_host_name(self):
# Test the low level method.
assert GSSAPI_HOST is not None
result = _canonicalize_hostname(GSSAPI_HOST, "forward")
if "compute-1.amazonaws.com" not in result:
self.assertEqual(result, GSSAPI_HOST)
result = _canonicalize_hostname(GSSAPI_HOST, "forwardAndReverse")
self.assertEqual(result, GSSAPI_HOST)

# Use the equivalent named CANONICALIZE_HOST_NAME.
props = self.mech_properties.copy()
if props["CANONICALIZE_HOST_NAME"] == "true":
props["CANONICALIZE_HOST_NAME"] = "forwardAndReverse"
else:
props["CANONICALIZE_HOST_NAME"] = "none"
client = self.simple_client(
GSSAPI_HOST,
GSSAPI_PORT,
username=GSSAPI_PRINCIPAL,
password=GSSAPI_PASS,
authMechanism="GSSAPI",
authMechanismProperties=props,
)
client.server_info()

if props["CANONICALIZE_HOST_NAME"] == "none":
return

def test_gssapi_host_name(self):
props = self.mech_properties
props["SERVICE_HOST"] = "example.com"

# Authenticate with authMechanismProperties.
client = self.simple_client(
GSSAPI_HOST,
GSSAPI_PORT,
username=GSSAPI_PRINCIPAL,
password=GSSAPI_PASS,
authMechanism="GSSAPI",
authMechanismProperties=self.mech_properties,
)
with self.assertRaises(OperationFailure):
client.server_info()

props["SERVICE_HOST"] = GSSAPI_HOST
client = self.simple_client(
GSSAPI_HOST,
GSSAPI_PORT,
username=GSSAPI_PRINCIPAL,
password=GSSAPI_PASS,
authMechanism="GSSAPI",
authMechanismProperties=self.mech_properties,
)
client.server_info()


class TestSASLPlain(PyMongoTestCase):
@classmethod
Expand Down
Loading