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

Jump to bottom

Set the default PostgreSQL sslMode to verify-full #86

Merged
merged 3 commits into from
May 11, 2022
Merged

Set the default PostgreSQL sslMode to verify-full #86

merged 3 commits into from
May 11, 2022

Conversation

ulucinar
Copy link
Collaborator

Description of your changes

Fixes #85

This PR proposes to set the default PostgreSQL sslMode to verify-full. We can also consider having spec.sslMode of ProviderConfig.postgresql as really optional and leave the defaults to the driver. For instance, libpq defaults to prefer, which is not recommended for security reasons. However, for improved security, this PR proposes verify-full as the default.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

@ulucinar ulucinar mentioned this pull request Apr 12, 2022
Closed
turkenh
turkenh reviewed Apr 19, 2022
View reviewed changes
apis/postgresql/v1alpha1/provider_types.go Outdated
@@ -33,7 +33,8 @@ type ProviderConfigSpec struct {
// Defines the SSL mode used to set up a connection to the provided
// PostgreSQL instance
// +kubebuilder:validation:Enum=disable;require;verify-ca;verify-full
// +optional
// +kubebuilder:default=verify-full
// +kubebuilder:validation:Required
SSLMode string `json:"sslMode"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we should set it as optional (with omit-empty) since we have a default, e.g.

https://github.com/crossplane/crossplane/blob/7d01448f8a52e0f3a13add4fc0f81028fcd13400/apis/pkg/v1/package_types.go#L31

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @turkenh, converted it to an optional field.

Copy link
Member

@Duologic Duologic May 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

-nevermind-

@ulucinar ulucinar requested a review from turkenh April 19, 2022 11:54
@ulucinar ulucinar requested review from Duologic and jdotw April 21, 2022 14:49
@Duologic
Copy link
Member

I'd argue to follow the upstream default as it is more predictable, is there a change request upstream to change the default perhaps?

@ulucinar
Copy link
Collaborator Author

I'd argue to follow the upstream default as it is more predictable, is there a change request upstream to change the default perhaps?

Hi @Duologic,
I have not seen anything upstream but here is a documentation for the ssl mode:
https://www.postgresql.org/docs/current/libpq-ssl.html

Please also see the note in that document reproduced here for convenience:

... Relying on this behavior is discouraged, and applications that need certificate validation should always use verify-ca or verify-full.

As mentioned in the description of the PR, while I agree aligning with the downstream default would be less controversial but I preferred the secure by default approach and chose the strictest policy. Also with many modern TLS client libraries, what's described for verify-full is already what you'd expect from a TLS client to do during a handshake, i.e., verify the TLS server's host name.

Duologic
Duologic reviewed Apr 29, 2022
View reviewed changes
.gitignore Outdated
@@ -2,6 +2,7 @@
/vendor
/.vendor-new
.vscode
/.idea
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind adding this in your personal $HOME/.gitignore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Makefile Outdated
# adjust this behavior in the build submodule because it is also causing Linux
# users to duplicate their build cache, but for now we just make it easier to
# identify its location in CI so that we cache between builds.
go.cachedir:
Copy link
Member

Choose a reason for hiding this comment

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

I've incorporated this Make target in #88, do you mind rebasing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@Duologic
Copy link
Member

Your arguments for changing the default seem valid to me.

ulucinar added 2 commits May 6, 2022 22:07
@ulucinar
Set the default PostgreSQL sslMode to verify-full
2122636 
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar
Fix linter job
293c66b 
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar ulucinar force-pushed the fix-85 branch 3 times, most recently from 804d674 to 67dbf91 Compare May 6, 2022 19:13
@ulucinar
Make sslMode optional
6b54639 
Signed-off-by: Alper Rifat Ulucinar <[email protected]>
@ulucinar ulucinar requested a review from Duologic May 6, 2022 19:15
@ulucinar
Copy link
Collaborator Author

ulucinar commented May 8, 2022

Hi @Duologic,
Thanks for the review! Looks like I do not have write access to the repo to merge this.

@Duologic Duologic merged commit a4cf5be into crossplane-contrib:master May 11, 2022
@Duologic
Copy link
Member

Sorry for the delay, I didn't want to merge it while on my phone in case I missed something.

@zackb
Copy link

zackb commented Oct 11, 2022

Hi, I have sslMode: disable in my definition and I am still encountering this bug. The ProviderConfig is deleted before the Database, Role, and Grant. Any help appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Duologic Duologic Duologic approved these changes

@turkenh turkenh turkenh approved these changes

@jdotw jdotw Awaiting requested review from jdotw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ProviderConfig can be deleted before ProviderConfigUsage is getting cleaned
4 participants
@ulucinar @Duologic @zackb @turkenh