Skip to content

Support tlsmode - #555

Merged
sitingren merged 22 commits into
vertica:masterfrom
sitingren:prefer-tls
Jul 19, 2024
Merged

Support tlsmode#555
sitingren merged 22 commits into
vertica:masterfrom
sitingren:prefer-tls

Conversation

@sitingren

@sitingren sitingren commented Jun 26, 2024

Copy link
Copy Markdown
Contributor
  • Add new connection option tlsmode.
  • When connection option ssl is not set, change its behavior from tlsmode='disable' to tlsmode='prefer'.
  • Update README TLS/SSL section.

@kevinkarch88 kevinkarch88 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tests look good and thorough to me. Approving.

Comment thread README.md Outdated
Comment thread README.md Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

similar nit, I would change the description slightly to say "...signed by a trusted certificate authority."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and same change to the relevant part of verify-full

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

good adjustment, more secure

Comment thread vertica_python/vertica/connection.py Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would this logic be clearer as a function that takes tlsmode_options and ssl_options and returns the correct tlsmode?
tlsmode = _initTLSMode(self.options.get('tlsmode'), self.options.get('ssl'))

@sitingren sitingren Jul 4, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, this part will be wrapped into a function eventually, since this affects future works of making load balancing TLS encrypted, so I didn't do it. Now commit is pushed.

Comment thread vertica_python/vertica/tlsmode.py Outdated

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice and clean. I like your implementation of this class.

@DMickens DMickens left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These changes look great

@sitingren
sitingren merged commit 07f7a9b into vertica:master Jul 19, 2024
@sitingren
sitingren deleted the prefer-tls branch July 19, 2024 03:23
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.

4 participants