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

provide a way to disable SSL completely #182

Closed
wants to merge 10 commits into from
Closed

Conversation

aiqui
Copy link
Contributor

@aiqui aiqui commented Dec 16, 2020

Currently ODK Central does not provide a way to disable SSL completely. There are a number of common situations where this is needed:

  • SSL is managed through a CDN such as AWS CloudFront (providing faster access in distant countries)
  • hosting server already manages SSL certificates and uses port 80 / 443
  • a developer has multiple web-based projects, using a top-level web server

In all of these cases, SSL is handled at the top level, using a reverse proxy to communicate with ODK Central. SSL should not be used at the ODK level, because server-to-server communication (reverse proxy) is inside the private network.

I added a "none" option for SSL_TYPE, environment variables for the open and SSL ports, and a modification to the nginx docker entry-point script which disables any SSL if needed. This has been tested on my Debian system.

@issa-tseng
Copy link
Member

hey @aiqui: thanks for the PR. the code seems fine to me, though it looks like the tests don't like something about the port mapping.

from a broader perspective, our policy so far is that while we understand there are legitimate reasons to want to disable SSL, we have resisted so far actually making it easy to do so, because the security of the entire service depends entirely on a secure connection. there are also a lot of "reasonable but bad" reasons to want to disable SSL, and in a lot of cases the exact security implication of doing so is not well-understood by our very very very many non-technical users in the face of the immediate practical problem of eg obtaining an SSL certificate, or solving some other problem thereof.

in general, our belief is that organizations that need SSL off (it is part of a broader secured network, or the organization owns its own certificate chain) probably have the technical staff needed to perform the changes necessary. also in many of these advanced cases, the changes necessary vary.

i'll pull @lognaturel to make a final call here but my inclination is to not merge this PR for those reasons.

@aiqui
Copy link
Contributor Author

aiqui commented Dec 17, 2020

I understand your reasoning, and I agree that nobody should be deploying without SSL today. Certificates are now free, and should be the default. But the problem is that ODK Central has internalized SSL certificate allocation, which is often not the best solution, and makes multi-project development more difficult.

One common scenario is deploying ODK Central on AWS with CloudFront. This is a great setup because there are endpoint servers all around the world, including in Africa and Latin America, with fast connections to AWS data centers. So with limited international service, we can connect to ODK via a local endpoint server. Using CloudFront, AWS automatically supplies the SSL certificate, and CloudFront is expecting to communicate to the ODK server without the overhead of SSL (reverse proxy within the private network).

Granted, I got around this shortcoming in less than a day, and I can keep my forked branch up to date. But I suspect you'll see the issue pop up more in the future.

Thanks for all your work!

@issa-tseng
Copy link
Member

i understand what you are saying @aiqui, but i hope you can hear me that these scenarios you describe that seem perfectly normal and obvious to you are far off in our fringe use cases.

we've had this project running for a while now, and while of course we do hear about people wanting something like this from time to time, often enough those requests reinforce the observations and positions i outlined in my previous reply. again, i am going to defer to ln here.

@lognaturel
Copy link
Member

lognaturel commented Jan 4, 2021

Thanks so much for sending this in, @aiqui, and I apologize for the radio silence on my end! I hope you've had a good start to 2021.

As @issa-tseng has described, we've tried to walk the tightrope of providing useful options but nudging users strongly towards secure defaults. I think we've had some success there but we've also seen unintended consequences like users who don't want SSL or want to manage it at their own layer simply using Aggregate or other services that might not otherwise be their first choice. We've also seen some fairly non-technical users attempting the kind of top-level SSL management that @aiqui describes because it has become more common and accessible.

All that to say, I think this change is helpful. What I'd like to propose that we do to mitigate the very real risks that @issa-tseng brings up is to not include the none option in the SSL_TYPE default value. What I'm most worried about is users seeing none and going straight for it because it's easy or because they're frustrated. Not including it there would mean users either have to be savvy enough to read the nginx setup script or have to read the documentation which would include information about security.

How does that sound, @aiqui, @issa-tseng? Also interested to hear from @danbjoseph who also has a legitimate use case for disabling SSL. Is this roughly what you did?

.env Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
@aiqui
Copy link
Contributor Author

aiqui commented Jan 10, 2021

all suggestions have been added, thank you

@aiqui
Copy link
Contributor Author

aiqui commented Jan 10, 2021

Please note that I merged the last master changes to keep this up to date (GitHub is showing those changes in the pull request).

@yanokwa
Copy link
Member

yanokwa commented Apr 19, 2021

Closing this in favor of the cleaner PR at #213.

@yanokwa yanokwa closed this Apr 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants