Fixing cookie based authentication #1091

awnns · 2024-08-23T14:52:54Z

Description

At present, cookie based authentication does not work in pylibjuju. This is due to a number of bugs. This PR intends to fix many of these issues so that pylibjuju can use the juju client's cookies for authentication.

Fixes:

The library now detects the appropriate cookie domain from jujudata.
The library now adds correct user tags for external and local users.
The library now correctly detects when a discharge is required due to third party caveats.
The library now attempts to use cookies if no password has been provided and cookies are available.
A subtle implementation difference between juju's cookie jar and http.cookiejar.DefaultPolicy was fixed with monkey patching.

I have also cherry picked a commit from @Aflynn50 from a different ongoing PR.

Fixes: #1061 #1081

QA Steps

I have had some difficulty with running the tests locally. I am not sure what sort of setup they want to run in.

What I have tested manually is the following:

Creating connections and logging into controllers with username and password
Creating connections and logging into controllers with an external user and identity provider using cookies from the juju client.
Creating connections and logging into controllers with a local user using cookies from the juju client.
Various commands such as listing models and checking model statuses.

Things I have been unable to test that are affected by this PR:

Anything to do with proxied controllers.
Anything to do with controllers with proper certificates (all I have access to are self signed). There is potential for issues with the cookie domain for these since I have not tested whether it works. It might require trivial modifications.
Juju 2.X

Notes & Discussion

The monkey patches in gocookies.py are not a good look and they are somewhat brittle, but I believe it will take a lot more work to fix the CookiePolicy if we wish to retain all the other sane behaviors from http.cookiejar.DefaultCookiePolicy. An alternative is to use an empty CookiePolicy and subclass it to only check for domain equality and path. The problem the monkey patch fixes is caused by incompatible cookiejars. The juju client uses https://github.com/juju/persistent-cookiejar which permits domain fields without dots under the condition that the domain matches exactly with the request's host. Python's implementation instead rewrites the request's host by appending ".local", which makes the comparison fail.

There are still issues with CA certificates when requiring discharges from the juju controller. This is due to intricacies of the HTTP handling in pymacaroonbakery. It may require substantial changes in pymacaroonbakery, but due to a number of bugfixes in this PR it is not essential for most uses (self signed external identity providers are the exception). The lack of a fix does mean that if a user is not logged in on a local user, and tries to connect to the controller, then they will be met with the wrong exception. The intended outcome is to require an interaction to enter a password, but this is not implemented in pymacaroonbakery. In any case, allowing self signed certificates requires either a hack where pylibjuju sets an environment variable and writes a cacert to a temporary file, or making changes in pymacaroonbakery.

jujubot · 2024-08-23T14:52:58Z

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

jujubot · 2024-08-23T14:52:58Z

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

dimaqq

Code looks sensible.

Unit tests would be great, unless the existing unit test actually cover this change.

I wonder how to validate this change against real Juju.

Aflynn50

Thank you for the contribution! It looks good and it'll be great to get this working again.

As @dimaqq mentioned, it would be good to get some tests working for this. There are some disabled tests in /tests/integration/test_macaroon_auth.py that should work again with this fix. Would you be able to get the relevant ones from there passing and remove the skip decorators on them?

Aflynn50 · 2024-08-29T09:15:56Z

juju/client/gocookies.py

 import pyrfc3339


+# There are certain subtle differences between the Go and Python


Rather than monkey patching here, would it be possible to get the inherit from the DefaultCookiePolicy, override the relevent methods then pass it as a policy kwarg to cookiejar.FileCookieJar?

I believe this should achive the same effect without monkey patching the code.

There was some complication which I have forgotten. I can look into it again later today.

Aflynn50 · 2024-08-29T09:43:47Z

juju/client/connection.py

@@ -962,7 +967,7 @@ async def login(self):
            params['credentials'] = self.password
        else:
            macaroons = _macaroons_for_domain(self.bakery_client.cookies,
-                                              self.endpoint)


I'm not sure I fully get the intent behind this from inspection. What is the difference here between the endpoint and the cookie domain?

This is because when you are using macaroons from the bakery client, you are actually using cookies that were originally put in the cookie jar by the juju client. The juju client does not use the endpoint as the cookie domain. Instead it uses either the controller's UUID (if the controller is self signed) or its SNIHostName. If we don't pick the correct cookie domain then pylibjuju will not find the cookie and it will fail to authenticate with the controller. I believe this is because if you have a self signed HA controller (for example) then there will be three different potential endpoints, so if you use that for your cookie domain, then your coookie will only work with one of the endpoints. As an example: you might have an endpoint which is 192.168.3.14, but the cookie in the jar has the domain "791fa163-16bd-4726-9c58-7c360cc133b2".

Ah fair, good work on the digging! Seems like a bug that this wasn't implemented the same way then.

Correct, the juju code is

cookieURL := api.CookieURLFromHost(api.PerferredHost(info)) info.Macaroons = httpbakery.MacaroonsForURL(cookieJar, cookieURL)

where PreferredHost is either the SNIHostName or controller UUID.

…th a subclass

SimonRichardson

@wallyworld can you look over this PR, because you did a lot of work around macroons in the go side.

wallyworld

Code looks ok. Left a couple of comments to provide a little bit of extra context from the Juju side. I'll leave it to others to agree/disagree with the trade-offs around the cookie jar behaviour, suffice to say it's progress and if it works from the perspective of "don't let perfect be the enemy of good", then it seems ok as a step forward, especially since upstream Juju changes are out of scope here.

wallyworld · 2024-10-10T03:27:03Z

juju/client/connection.py

@@ -871,7 +876,7 @@ async def _connect_with_login(self, endpoints):
            # a few times.
            for i in range(0, 2):
                result = (await self.login())['response']
-                macaroonJSON = result.get('discharge-required')
+                macaroonJSON = result.get('bakery-discharge-required')


For the casual observer - newer Juju controllers (2.9 onwards) use the "bakery-discharge-required" attribute.

wallyworld · 2024-10-10T03:37:29Z

juju/client/connection.py

@@ -962,7 +967,7 @@ async def login(self):
            params['credentials'] = self.password
        else:
            macaroons = _macaroons_for_domain(self.bakery_client.cookies,
-                                              self.endpoint)


Correct, the juju code is

cookieURL := api.CookieURLFromHost(api.PerferredHost(info)) info.Macaroons = httpbakery.MacaroonsForURL(cookieJar, cookieURL)

where PreferredHost is either the SNIHostName or controller UUID.

Aflynn50 and others added 2 commits July 24, 2024 17:12

fix: dont check for a username/password when connecting with macaroons

9acfabf

Fix cookie based authentication.

b675a0b

dimaqq reviewed Aug 29, 2024

View reviewed changes

Aflynn50 reviewed Aug 29, 2024

View reviewed changes

awnns and others added 2 commits September 3, 2024 10:50

Merge branch 'main' into main

150d0b6

refactor(cookiejar): replace monkey patches to DefaultCookiePolicy wi…

02adced

…th a subclass

SimonRichardson reviewed Oct 9, 2024

View reviewed changes

SimonRichardson requested a review from wallyworld October 9, 2024 07:46

wallyworld reviewed Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing cookie based authentication #1091

Fixing cookie based authentication #1091

awnns commented Aug 23, 2024

jujubot commented Aug 23, 2024

jujubot commented Aug 23, 2024

dimaqq left a comment

Aflynn50 left a comment

Aflynn50 Aug 29, 2024

awnns Aug 29, 2024

Aflynn50 Aug 29, 2024

awnns Aug 29, 2024

Aflynn50 Aug 29, 2024

wallyworld Oct 10, 2024

SimonRichardson left a comment

wallyworld left a comment

wallyworld Oct 10, 2024

wallyworld Oct 10, 2024

		import pyrfc3339


		# There are certain subtle differences between the Go and Python

Fixing cookie based authentication #1091

Are you sure you want to change the base?

Fixing cookie based authentication #1091

Conversation

awnns commented Aug 23, 2024

Description

QA Steps

Notes & Discussion

jujubot commented Aug 23, 2024

jujubot commented Aug 23, 2024

dimaqq left a comment

Choose a reason for hiding this comment

Aflynn50 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

SimonRichardson left a comment

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment