Apply TLJH auth config with less assumptions

[consideRatio]

This change is extracted from #719 where I wanted to remove use of the deprecated jupyterhub-dummyauthenticator package that is now part of JupyterHub itself instead, and by doing so perhaps also reference it as dummy when setting c.JupyterHub.authentication_class as done in many documentation examples outside TLJH and in Z2JH for example.

Review notes

1. I removed two FIXME statements - do you agree on that?
   1. # FIXME: Make sure this is something importable.
      Should we really make sure what we configure is importable? That is something that JupyterHub does already and provides nice logs on startup if its not importable. I figure its a way to make this project take on more responsibilities than we can manage to maintain.
   2. # FIXME: SECURITY: Class must inherit from Authenticator, to prevent us being used to set arbitrary properties on arbitrary types of objects!
      I'm not sure I understand, but I'm thinking that a) whatever Python code can do bad things and camouflage itself as an Authenticator, and b) that whatever we configure is done via sudo tljh-config and such, making whoever configures things already have a high security clearance.
2. I made config under something like auth.AnythingGoes.any_config_name be set as c.AnythingGoes.any_config_name instead of just looking to update auth.SpecificClassName.any_config_name based on guessing the SpecificClassName from the auth.type string. The reason I did this was because when configuring the dummy authenticator, one can use the dummy name rather than the full name of jupyterhub.auth:DummyAuthenticator. I guess I wanted to support setting auth.type in the generally supported sense - matching exactly what you can configure at c.JupyterHub.authentication_class instead of forcing us to have a string we can manage to detect the a Class name from.

[yuvipanda]

2\. I'm not sure I understand, but I'm thinking that a) whatever Python code can do bad things and camouflage itself as an Authenticator, and b) that whatever we configure is done via `sudo tljh-config` and such, making whoever configures things already have a high security clearance.

The problem is not code that camoflauges itself as an authenticator, but the ability for code in YAML to set arbitrary python values. For example, can you assign a value to sys.exit via this? I'm not sure, hence the FIXME :) Setattr when handling user provided data can be quite dangerous. This is similar in principle to the string of YAML based remote code execution exploits from a few years ago (like https://www.sitepoint.com/anatomy-of-an-exploit-an-in-depth-look-at-the-rails-yaml-vulnerability/). Basically if you're doing setattr, you must know that you are setting it only in objects you know this config file is allowed to control...

I don't of course have a solution to this, but I'd love to leave the FIXME in place - perhaps with a better explanation? Otherwise happy to merge.

[minrk]

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.

[consideRatio]

but I'd love to leave the FIXME in place - perhaps with a better explanation?

@yuvipanda could you help me understand the fixme statements better? I didn't understand the situation even though you were kind to provide a descriptive comment! I absolutely think we should retain fixme statements but it feels important to me that I and others are able to comprehend them without too much prior knowledge, and since I failed to grasp this clearly I figure it's just as well I dig in a bit into this now.

[consideRatio]

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?

[consideRatio]

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

[minrk]

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.

[consideRatio]

@yuvipanda I added back the FIXME notes and opened #740 to address that later! Sorry for intertwining this PR with that!

@minrk I've resolved a test error introduced by your suggestion in ea72021
@minrk I've made some suggestions in #721 (review) to not just ignore invalid configuration and move on.

---

I'd love to see this merged soon, so #719 can be merged next, and following that I'd like to address #732 which I'd like some agreement on as well.

Ignore the RTD build failure, it relates to master switch to main, I've updated it in RTD settings but it doesn't reflect on this PR.

[minrk]

Some minor changes we can fix later. Great job, @consideRatio!

[minrk]

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"

[minrk]

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

[yuvipanda]

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.

[minrk]

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.