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

source-mssql: enforce SSL on Airbyte Cloud #32882

Merged
merged 9 commits into from
Dec 1, 2023
Merged

source-mssql: enforce SSL on Airbyte Cloud #32882

merged 9 commits into from
Dec 1, 2023

Conversation

postamar
Copy link
Contributor

@postamar postamar commented Nov 28, 2023 •
edited by alafanechere
Loading

This is the MS SQL Server companion to #31062.

Fixes #32789.

@vercel Vercel
Copy link

vercel bot commented Nov 28, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Dec 1, 2023 3:24pm

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 28, 2023 •
edited by postamar
Loading

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@postamar postamar force-pushed the postamar/nuke-source-mssql-strict-encrypt branch from 88cebf0 to 31c8abc Compare November 28, 2023 17:19
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Nov 28, 2023
@postamar postamar force-pushed the postamar/nuke-source-mssql-strict-encrypt branch from 31c8abc to c4455d2 Compare November 28, 2023 17:20
@postamar postamar marked this pull request as ready for review November 28, 2023 17:21
@postamar postamar requested a review from a team as a code owner November 28, 2023 17:21
@octavia-squidington-iv octavia-squidington-iv requested a review from a team November 28, 2023 17:23
@katmarkham katmarkham removed the request for review from a team November 30, 2023 21:23
@rodireich
Copy link
Contributor

mssql strict encrypt was previously marked as safe. I think because it only has encrypted mode.
We removed the unencrypted value from config UI in MssqlSourceStrictEncrypt.modifySpec().
With this change there is going to be an unencrypted ssl mode but also we are going to verify that ssh is configured in case ssl is set to unencrypted

rodireich
rodireich reviewed Dec 1, 2023
View reviewed changes
.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java Outdated
if (cloudDeploymentMode()) {
final ConnectorSpecification cloudDeploymentSpec = Jsons.clone(super.spec());
// Remove "unencrypted" value for "ssl_method".
((ArrayNode) cloudDeploymentSpec.getConnectionSpecification().get("properties").get(SSL_METHOD).get("oneOf")).remove(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

We do want to leave it then, no?
since check is going to verify that at least one mode of encryption is configured

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's a good point.

rodireich
rodireich reviewed Dec 1, 2023
View reviewed changes
.../connectors/source-mssql/src/main/java/io/airbyte/integrations/source/mssql/MssqlSource.java
&& config.get(TUNNEL_METHOD).get(TUNNEL_METHOD).asText().equals(NO_TUNNEL)) {
// If no SSH tunnel.
if (config.has(SSL_METHOD) && config.get(SSL_METHOD).has(SSL_METHOD) &&
SSL_METHOD_UNENCRYPTED.equalsIgnoreCase(config.get(SSL_METHOD).get(SSL_METHOD).asText())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also consult with @stephane-airbyte whether there are new unencrypted modes he's adding as part of his job of refitting ssl on this connector - a "prefer ssl" mode for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

nah, the ms-sql JDBC protocl is widely different than the postgres or mysql. You can push as is

@postamar
Copy link
Contributor Author

postamar commented Dec 1, 2023

Thanks for taking a look.

We removed the unencrypted value from config UI in MssqlSourceStrictEncrypt.modifySpec().

Ahhh, right! That's a relief.

@octavia-squidington-iv octavia-squidington-iv requested a review from a team December 1, 2023 15:19
@postamar postamar enabled auto-merge (squash) December 1, 2023 16:43
@postamar postamar merged commit b17f73b into master Dec 1, 2023
23 of 24 checks passed
@postamar postamar deleted the postamar/nuke-source-mssql-strict-encrypt branch December 1, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephane-airbyte stephane-airbyte stephane-airbyte left review comments

@rodireich rodireich rodireich approved these changes

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/mssql connectors/source/mssql-strict-encrypt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

remove source-mssql-strict-encrypt
4 participants
@postamar @rodireich @stephane-airbyte @octavia-squidington-iii