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

Feature: ability to turn off SSL required #34

Open
2 tasks done
jpmcb opened this issue Aug 9, 2023 · 6 comments
Open
2 tasks done

Feature: ability to turn off SSL required #34

jpmcb opened this issue Aug 9, 2023 · 6 comments

Comments

@jpmcb
Copy link
Member

jpmcb commented Aug 9, 2023

Type of feature

🍕 Feature

Current behavior

Currently, this microservice only can make connections to postgres over ssl:

connectString := fmt.Sprintf("host=%s port=%s user=%s password=%s dbname=%s sslmode=require", host, port, user, pwd, dbName)

Suggested solution

We should consider having a flag / config that disables ssl being required in order to connect. Ideally, this would flip to some "auto" mode that can connect to any postgres.

This should also log a warning if this setting has been turned on so administrators can be assured the configuration that's being run.

Additional context

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct

Contributing Docs

  • I agree to follow this project's Contribution Docs
@AvineshTripathi
Copy link
Contributor

Do you think instead using the flag using the prefer mode of ssl a good option (more info https://www.postgresql.org/docs/8.4/libpq-connect.html#LIBPQ-CONNECT-SSLMODE) if not then if the flag is set to true then maybe setting sslmode to disable is good

@k1nho
Copy link
Contributor

k1nho commented Sep 11, 2023

prefer is a nice choice, but I think is not supported on lib/pq at least last time I tried. I think using the flag and setting sslmode=%s to receive either disable when the flag is set or require should be the solution. This is also a workaround for dev environment to avoid going over the ssl cert setup.

@jpmcb
Copy link
Member Author

jpmcb commented Sep 11, 2023

This is also a workaround for dev environment to avoid going over the ssl cert setup.

Exactly. This isn't really a product requirement as much as it is a "nice to have" to lower the bar abit so people don't have to setup a postgres cluster with SSL. I could also see the argument that there shouldn't be the ability to turn off SSL (since it makes the security story abit worse here). But generally, OpenSauced wouldn't ship a database without SSL turned on so this microservice would fail to start for our use cases if we tried deploying it in that way.

@mtfoley
Copy link

mtfoley commented Sep 15, 2023

@jpmcb regarding lowering the bar a bit, I started some work on my fork for spinning up Postgres, migrations, and the application via Docker compose. Branch is still pretty rough, but it works. Let me know if I ought to create an issue for this to discuss further.

@jpmcb
Copy link
Member Author

jpmcb commented Sep 15, 2023

A docker compose file would be useful for contributors although it wouldn't be the full picture (since the database is really a database from the API which we have some steps for setting up: https://github.com/open-sauced/api#%EF%B8%8F-setting-up-a-postgresql-database-locally).

It could be very cool to have a docker compose that captures more of the stack to give a full local development experience.

@mtfoley
Copy link

mtfoley commented Sep 15, 2023

Got it. I created the separate issue and linked a draft PR for it. Regarding the change for this issue, it'd be easy enough to create a map that we could use to validate against reasonable values (e.g. "require", "disable", etc) and warn if we don't see a value specified, or if it's invalid.

e.g.

validSSLModes := map[string]bool { 
  "require": true,
  "disable": true,
  // ...
}
sslMode, sslModeSet := os.LookupEnv("SSLMODE")
if !sslModeSet {
  // log warning about variable being unset
  sslMode = "require"
} else if _, valid := validSSLMode[sslMode]; !valid {
   // log warning about variable being invalid
  sslMode = "require"
}

// work with sslMode from here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants