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.

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

Apply TLJH auth config with less assumptions #721

Merged
merged 5 commits into from
Oct 27, 2021
Merged

Apply TLJH auth config with less assumptions #721

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c8e567
Apply TLJH auth config with less assumptions
consideRatio Oct 20, 2021
a32ec5e
Apply suggestions from code review
consideRatio Oct 22, 2021
ea72021
tests: make MockConfigurer support subscription
consideRatio Oct 26, 2021
efb8ee0
Add back FIXME notes unchanged
consideRatio Oct 26, 2021
0cd6b2a
raise on ignored config in auth
minrk Oct 27, 2021
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
25 changes: 18 additions & 7 deletions tests/test_configurer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,22 @@

class MockConfigurer:
"""
Mock a Traitlet Configurable object.
Mock a Traitlets Config class object.

Equivalent to the `c` in `c.JupyterHub.some_property` method of setting
traitlet properties. If an accessed attribute doesn't exist, a new instance
of EmtpyObject is returned. This lets us set arbitrary attributes two
levels deep.

>>> c = MockConfigurer()
>>> c.FirstLevel.second_level = 'hi'
>>> c.FirstLevel.second_level == 'hi'
True
>>> hasattr(c.FirstLevel, 'does_not_exist')
False
>>> c = MockConfigurer()
>>> c.FirstLevel.second_level = 'hi'
>>> c.FirstLevel.second_level == 'hi'
True
>>> hasattr(c.FirstLevel, 'does_not_exist')
False

The actual Config class implementation can be found at
https://github.com/ipython/traitlets/blob/34f596dd03b98434900a7d31c912fc168342bb80/traitlets/config/loader.py#L220
"""

class _EmptyObject:
Expand All @@ -37,6 +40,13 @@ def __getattr__(self, k):
self.__dict__[k] = MockConfigurer._EmptyObject()
return self.__dict__[k]

def __getitem__(self, key):
"""
To mimic the traitlets Config class instance we often access as "c", we
need to provide a subscript functionality that can be used as
c["Something"]. To do this, we provide a __getitem__ function.
"""
return self.__getattr__(key)

def test_mock_configurer():
"""
Expand All @@ -48,6 +58,7 @@ def test_mock_configurer():

assert m.SomethingSomething == 'hi'
assert m.FirstLevel.second_level == 'boo'
assert m["FirstLevel"].second_level == 'boo'

assert not hasattr(m.FirstLevel, 'non_existent')

Expand Down
62 changes: 44 additions & 18 deletions tljh/configurer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,27 +149,53 @@ def update_base_url(c, config):

def update_auth(c, config):
"""
Set auth related configuration from YAML config file

Use auth.type to determine authenticator to use. All parameters
in the config under auth.{auth.type} will be passed straight to the
authenticators themselves.
Set auth related configuration from YAML config file.

As an example, this function should update the following TLJH auth
configuration:

```yaml
auth:
type: oauthenticator.github.GitHubOAuthenticator
GitHubOAuthenticator:
client_id: "..."
client_secret: "..."
oauth_callback_url: "..."
ClassName:
arbitrary_key: "..."
arbitrary_key_with_none_value:
```

by applying the following configuration:

```python
c.JupyterHub.authenticator_class = "oauthenticator.github.GitHubOAuthenticator"
c.GitHubOAuthenticator.client_id = "..."
c.GitHubOAuthenticator.client_secret = "..."
c.GitHubOAuthenticator.oauth_callback_url = "..."
c.ArbitraryKey.arbitrary_key = "..."
```

Note that "auth.type" and "auth.ArbitraryKey.arbitrary_key_with_none_value"
are treated a bit differently. auth.type will always map to
c.JupyterHub.authenticator_class and any configured value being None won't
be set.
"""
auth = config.get('auth')
tljh_auth_config = config['auth']

# FIXME: Make sure this is something importable.
# FIXME: SECURITY: Class must inherit from Authenticator, to prevent us being
# used to set arbitrary properties on arbitrary types of objects!
authenticator_class = auth['type']
# When specifying fully qualified name, use classname as key for config
authenticator_configname = authenticator_class.split('.')[-1]
c.JupyterHub.authenticator_class = authenticator_class
# Use just class name when setting config. If authenticator is dummyauthenticator.DummyAuthenticator,
# its config will be set under c.DummyAuthenticator
authenticator_parent = getattr(c, authenticator_class.split('.')[-1])

for k, v in auth.get(authenticator_configname, {}).items():
set_if_not_none(authenticator_parent, k, v)
# FIXME: SECURITY: Class must inherit from Authenticator, to prevent us
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the back-and-forth, @consideRatio, but this FIXME is already fixed by the use of traitlets config and I think it should be removed, as it gives a false impression that there's something security-related to fix. The only things that can be set here are attributes that:

  1. are on Configurable subclasses
  2. are declared as configurable with config=True
  3. inherit the JupyterHub application object's config

Any attempt to configure something else here will be ignored.

traitlets config cannot in any way "set arbitrary properties on arbitrary types of objects"

Copy link
Member

Choose a reason for hiding this comment

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

Since this isn't a regression in this PR, happy to merge this as-is when tests pass, and remove the inaccurate FIXMEs in a single PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's great to know, @minrk! I wasn't sure if that was handled by traitlets or if we were using setattr, and this is pretty awesome.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we have some setattr, but it's always on the Config object itself where setattr is really a proxy for setitem in a dict-of-dicts that doesn't do anything on its own. Traitlets config loading then handles the safety of that, by only loading recognized values from the Config object.

# being used to set arbitrary properties on arbitrary types of objects!
c.JupyterHub.authenticator_class = tljh_auth_config['type']

for auth_key, auth_value in tljh_auth_config.items():
if not (auth_key[0] == auth_key[0].upper() and isinstance(auth_value, dict)):
continue
minrk marked this conversation as resolved.
Show resolved Hide resolved
class_name = auth_key
class_config_to_set = auth_value
class_config = c[class_name]
Copy link
Member Author

Choose a reason for hiding this comment

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

This results in the following errors in our test suite, perhaps because we have a class that isn't inheriting from a traitlets class to make it configurable?

        for auth_key, auth_value in tljh_auth_config.items():
            if not (auth_key[0] == auth_key[0].upper() and isinstance(auth_value, dict)):
                continue
            class_name = auth_key
            class_config_to_set = auth_value
>           class_config = c[class_name]
E           TypeError: 'MockConfigurer' object is not subscriptable

It would work to use getattr(c, traitlet_class_name), but I think this distinction relates to @minrk's comment:

I'm not sure if the security issue ever was a real problem, but it isn't now. The way this works now is to set fields on the config object only not on arbitrary Python objects, or indeed even the objects that will be onfigured. That means only traitlet attributes with .tag(confg=True) can be set via this mechanism, and anything else will be ignored.

What is our action point to fix this?

  • Use getattr(c, traitlet_class_name)
  • Make the MockConfigurer class (Dummy c instance) support being accessed like c["ClassName"]

I figure the latter is to be preferred then, right?

Copy link
Member Author

@consideRatio consideRatio Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've resolved this by adding a getitem function to the MockConfigurer class.

Copy link
Member

@minrk minrk Oct 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand why we have a MockConfigurer that's only used in tests, instead of testing with a Config object. It doesn't seem to serve any purpose, other than making the tests less like what's being tested, for instance this case where an error is raised in tests when there is no error in real code.

for config_name, config_value in class_config_to_set.items():
set_if_not_none(class_config, config_name, config_value)


def update_userlists(c, config):
Expand Down