Skip to content
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.

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

Fixing cookie based authentication #1091

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 9 additions & 4 deletions juju/client/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,8 @@ async def connect(
specified_facades=None,
proxy=None,
debug_log_conn=None,
debug_log_params={}
debug_log_params={},
cookie_domain=None,
):
"""Connect to the websocket.

Expand Down Expand Up @@ -285,8 +286,10 @@ async def connect(
to prevent using the conservative client pinning with in the client.
:param TextIOWrapper debug_log_conn: target if this is a debug log connection
:param dict debug_log_params: filtering parameters for the debug-log output
:param str cookie_domain: Which domain the controller uses for cookies.
"""
self = cls()
self.cookie_domain = cookie_domain
if endpoint is None:
raise ValueError('no endpoint provided')
if not isinstance(endpoint, str) and not isinstance(endpoint, list):
Expand All @@ -301,7 +304,7 @@ async def connect(
if password is not None:
raise errors.JujuAuthError('cannot log in as external '
'user with a password')
username = None

self.usertag = tag.user(username)
self.password = password

Expand Down Expand Up @@ -762,6 +765,7 @@ def connect_params(self):
'bakery_client': self.bakery_client,
'max_frame_size': self.max_frame_size,
'proxy': self.proxy,
'cookie_domain': self.cookie_domain,
}

async def controller(self):
Expand All @@ -774,6 +778,7 @@ async def controller(self):
cacert=self.cacert,
bakery_client=self.bakery_client,
max_frame_size=self.max_frame_size,
cookie_domain=self.cookie_domain,
)

async def reconnect(self):
Expand Down Expand Up @@ -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')
Copy link
Member

Choose a reason for hiding this comment

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

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

if macaroonJSON is None:
self.info = result
success = True
Expand Down Expand Up @@ -962,7 +967,7 @@ async def login(self):
params['credentials'] = self.password
else:
macaroons = _macaroons_for_domain(self.bakery_client.cookies,
self.endpoint)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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".

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

self.cookie_domain)
params['macaroons'] = [[bakery.macaroon_to_dict(m) for m in ms]
for ms in macaroons]

Expand Down
50 changes: 27 additions & 23 deletions juju/client/connector.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,17 +63,33 @@ async def connect(self, **kwargs):

kwargs are passed through to Connection.connect()
"""
kwargs.setdefault("max_frame_size", self.max_frame_size)
kwargs.setdefault("bakery_client", self.bakery_client)
if "macaroons" in kwargs:
if not kwargs["bakery_client"]:
kwargs["bakery_client"] = httpbakery.Client()
if not kwargs["bakery_client"].cookies:
kwargs["bakery_client"].cookies = GoCookieJar()
jar = kwargs["bakery_client"].cookies
for macaroon in kwargs.pop("macaroons"):
jar.set_cookie(go_to_py_cookie(macaroon))
if "debug_log_conn" in kwargs:
kwargs.setdefault('max_frame_size', self.max_frame_size)
kwargs.setdefault('bakery_client', self.bakery_client)
kwargs.setdefault('cookie_domain', self.jujudata.cookie_domain_for_controller(endpoint=kwargs.get("endpoints")))

account = kwargs.pop('account', {})
# Prioritize the username and password that user provided
# If not enough, try to patch it with info from accounts.yaml
if 'username' not in kwargs and account.get('user'):
kwargs.update(username=account.get('user'))
if 'password' not in kwargs and account.get('password'):
kwargs.update(password=account.get('password'))

if not kwargs["bakery_client"]:
if 'macaroons' in kwargs:
if not kwargs['bakery_client']:
kwargs['bakery_client'] = httpbakery.Client()
if not kwargs['bakery_client'].cookies:
kwargs['bakery_client'].cookies = GoCookieJar()
jar = kwargs['bakery_client'].cookies
for macaroon in kwargs.pop('macaroons'):
jar.set_cookie(go_to_py_cookie(macaroon))
else:
if not ({'username', 'password'}.issubset(kwargs)):
required = {'username', 'password'}.difference(kwargs)
raise ValueError(f'Some authentication parameters are required : {",".join(required)}')

if 'debug_log_conn' in kwargs:
assert self._connection
self._log_connection = await Connection.connect(**kwargs)
else:
Expand All @@ -85,18 +101,6 @@ async def connect(self, **kwargs):
# connected to.
if self._connection:
await self._connection.close()

account = kwargs.pop('account', {})
# Prioritize the username and password that user provided
# If not enough, try to patch it with info from accounts.yaml
if 'username' not in kwargs and account.get('user'):
kwargs.update(username=account.get('user'))
if 'password' not in kwargs and account.get('password'):
kwargs.update(password=account.get('password'))

if not ({'username', 'password'}.issubset(kwargs)):
required = {'username', 'password'}.difference(kwargs)
raise ValueError(f'Some authentication parameters are required : {",".join(required)}')
self._connection = await Connection.connect(**kwargs)

# Check if we support the target controller
Expand Down
18 changes: 17 additions & 1 deletion juju/client/gocookies.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,30 @@
import http.cookiejar as cookiejar
import json
import time

import pyrfc3339


class JujuCookiePolicy(cookiejar.DefaultCookiePolicy):
'''A cookie policy that allows arbitrary strings to be used for cookie domains
as long as they match the host in the request exactly. This is necessary for interacting
with the juju controller, which uses UUID's for cookie domains in some circumstances.'''
def return_ok_domain(self, cookie, request):
return cookie.domain == request.host or super().return_ok_domain(cookie, request)

def domain_return_ok(self, domain, request):
return domain == request.host or super().domain_return_ok(domain, request)


class GoCookieJar(cookiejar.FileCookieJar):
'''A CookieJar implementation that reads and writes cookies
to the cookiejar format as understood by the Go package
github.com/juju/persistent-cookiejar.'''
def __init__(self, filename=None, delayload=False, policy=None):
if policy is None:
policy = JujuCookiePolicy()

super().__init__(filename, delayload, policy)

def _really_load(self, f, filename, ignore_discard, ignore_expires):
'''Implement the _really_load method called by FileCookieJar
to implement the actual cookie loading'''
Expand Down
25 changes: 25 additions & 0 deletions juju/client/jujudata.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,3 +182,28 @@ def cookies_for_controller(self, controller_name):
jar = GoCookieJar(str(f))
jar.load()
return jar

def cookie_domain_for_controller(self, controller_name=None, endpoint=None):
'''Returns the correct cookie domain.

The cookie domain used by the controller is either the
field public-hostname in the controllers.yaml file, or the uuid if this is
not available. If neither controller_name nor endpoint are specified, assume
the current controller.

:param str controller_name: The name of the controller.
:param str endpoint: The endpoint of the controller.
'''
if all([controller_name, endpoint]):
raise ValueError('Specify either controller_name or endpoint, not both')
if controller_name:
controller = controller_name
elif endpoint:
controller = self.controller_name_by_endpoint(endpoint)
else:
controller = self.current_controller()

controllers = self.controllers()
controller_data = controllers.get(controller)

return controller_data.get('public-hostname', controller_data.get('uuid'))