search

basak / certbot-snap-build

0 stars 3 forks source link

Simplify finding external plugins by using PYTHONPATH #19

Open bmw opened 4 years ago

bmw commented 4 years ago

While I fine making minor modifications to Certbot to make snaps work, I think we may not have to.

Instead of using a separate CERTBOT_PLUGIN_PATH environment variable and making the changes to Certbot in https://github.com/basak/certbot-snap-build/commit/f9ba645c9ce0fdd42504d3dc3cb53b54f6a6f375 to use it, I think we can just set the PYTHONPATH environment variable in the Certbot shell wrapper.

I hackily made these changes locally and it seemed to work.

bmw commented 4 years ago

If we do this, we should probably make sure we're adding to PYTHONPATH and not overriding it. I could see that variable needing to be set for Python to work properly inside the snap.

basak commented 4 years ago

Good point, and thank you for bringing it up!

I agree this would work. I'm on the fence about whether it would be better though, but it's certainly worth considering. For example, if a certbot plugin snap accidentally drops unrelated things into that path that breaks things, and that breaks certbot, then would restricting the plugin to plugin module loading only limit the damage, or make it more obvious to the user that the plugin is causing the problem?

I'm excluding anything malicious here - for an attacker it'll make no difference either way I think. I'm just wondering about whether there would be any UX downsides should anything go wrong.

bmw commented 4 years ago

That's a good question and is something someone should probably look into more before we change things here. I also think we can change how this works after snaps are released.

As long as we're careful to make sure the core snap's paths are searched before any plugin paths, the only thing I can immediately think of that a plugin could do is include broken modules that Certbot or its dependencies try to load before Certbot tries to load its plugins and the modules are not required or included in the Certbot snap. Some projects definitely do this, but I think it's somewhat uncommon.

I also think that Certbot currently doesn't do much to prevent a broken plugin from crashing it. I think if a plugin fails to load at all, Certbot will crash. This is something we could improve in the future though and limiting the scope where the plugins modules are in Certbot's PYTHONPATH would help.

bmw commented 4 years ago

I hackily moved this conversation to https://github.com/certbot/certbot/issues/7945. Please add any new discussion there.