✨ Source S3: Add HTTPS validation for S3 endpoint #32109

tolik0 · 2023-11-02T18:29:47Z

What

Enhanced security by ensuring S3 endpoints are HTTPS.

How

Implemented a check in the Config class that raises a ValidationError for endpoints starting with "http://".

vercel · 2023-11-02T18:29:53Z

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		Nov 9, 2023 10:29pm

github-actions · 2023-11-02T18:30:14Z

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,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

brianjlai · 2023-11-02T18:48:07Z

The changes look good and thanks for adding some unit tests. It's probably not a super widely used feature, but I have a slight concern of the customers that are using this in an insecure way that we'll disrupt their syncs. I did a search through our config DB in cloud and no users are using http which is definitely good. Once we get a passing CI and tests will 👍

This could affect our OSS customers. but I think we can still move forward with this because this endpoint should only be used securely and for the case of a locally running Airbyte instance, if its in the network they could probably hit the S3 address directly too.

davydov-d

Let's also add pattern and examples to spec, and extend the description

alafanechere · 2023-11-06T13:31:06Z

airbyte-integrations/connectors/source-s3/integration_tests/spec.json

+        "description": "Endpoint to an S3 compatible service. Leave empty to use AWS. The custom endpoint must be secure, but the 'https' prefix is not required.",
        "default": "",
+        "examples": ["my-s3-endpoint.com", "https://my-s3-endpoint.com"],
+        "pattern": "^(?!http://).*$",


I don't really get why we allow http endpoint but raise a validation error if endpoint.startswith("http://")
I think it's fine to allow them on OSS but not on Cloud.
To enforce https on Cloud I suggest to use the is_cloud_environment function of the CDK.
You can check a PR for a similar use case here

@alafanechere The regex pattern is designed to reject http:// prefixes. Using is_cloud_environment won't affect the check_connector_https_url_only test since the pattern and check are inside .py file.

Yes sure. I meant that we could allow http on OSS and forbid on cloud with the is_cloud_environment, which would make the pattern not required here.
As you allow my-s3-endpoint.com (without prefix) I'm wondering what enforces the use of HTTPS. I can't spot a logic in your PR that validates that my-s3-endpoint.com is secured.

alafanechere · 2023-11-06T13:34:29Z

airbyte-integrations/connectors/source-s3/integration_tests/spec.json

        "default": "",
+        "examples": ["my-s3-endpoint.com", "https://my-s3-endpoint.com"],
+        "pattern": "^(?!http://).*$",


In any case if you'd still like to allow http:// string here a minor change to this module is required to make the QA check pass.

@alafanechere, can I instead modify QA check to pass if there's a specific comment at the end of the line indicating that it should be ignored?

Yes good idea :) something like #ignore-check-connector-https-url-only

alafanechere · 2023-11-07T09:59:24Z

airbyte-integrations/connectors/source-s3/integration_tests/spec.json

+        "description": "Endpoint to an S3 compatible service. Leave empty to use AWS. The custom endpoint must be secure, but the 'https' prefix is not required.",
        "default": "",
+        "examples": ["my-s3-endpoint.com", "https://my-s3-endpoint.com"],
+        "pattern": "^(?!http://).*$",


Yes sure. I meant that we could allow http on OSS and forbid on cloud with the is_cloud_environment, which would make the pattern not required here.
As you allow my-s3-endpoint.com (without prefix) I'm wondering what enforces the use of HTTPS. I can't spot a logic in your PR that validates that my-s3-endpoint.com is secured.

alafanechere · 2023-11-07T10:02:32Z

airbyte-integrations/connectors/source-s3/source_s3/v4/config.py

+        endpoint = values.get("endpoint")
+        if endpoint:
+            if endpoint.startswith("http://"):
+                raise ValidationError("The endpoint must be a secure HTTPS endpoint.", model=Config)


As you allow endpoint without https:// prefix I don't think this is enough to validate that the endpoint is secured. I would suggst making a HEAD request to the endpoint with and https:// prefix and assert you get a 200 + a response with https:// prefix.

We ensure secure connections by configuring the boto3 client to use SSL and verify certificates, as shown in the code here: boto3 SSL configuration. This aligns with the boto3 documentation on enforcing SSL usage: boto3 documentation.

Nice, and this client is used during check right?

Correct, the client is utilized during the check to attempt reading a line from each stream. You can see the implementation of the check method here: FileBasedSource.check and the associated availability strategy here: DefaultFileBasedAvailabilityStrategy.

alafanechere

Thanks for the clarification @tolik0 . I added a rule in the QA checks to ignore the S3 config file. Feel free to merge once the tests pass.

alafanechere · 2023-11-07T10:30:28Z

airbyte-integrations/connectors/source-s3/metadata.yaml

@@ -10,7 +10,7 @@ data:
  connectorSubtype: file
  connectorType: source
  definitionId: 69589781-7828-43c5-9f63-8925b1c1ccc2
-  dockerImageTag: 4.1.4
+  dockerImageTag: 4.1.5


Suggested change

dockerImageTag: 4.1.5

dockerImageTag: 4.2.0

Shouldn't this be a minor bump as its a new "feature"?

This reverts commit 0d52e98.

alafanechere · 2023-11-07T12:08:41Z

I reverted my QA checks changes. Feel free to pull in #32242 when merged and use your nice new comment feature!

…error

Add HTTPS validation for S3 endpoint

96a11a5

octavia-squidington-iii added area/connectors Connector related issues connectors/source/s3 labels Nov 2, 2023

Update version

1a19618

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Nov 2, 2023

tolik0 requested a review from brianjlai November 2, 2023 18:30

vercel bot deployed to Preview November 2, 2023 18:32 View deployment

tolik0 marked this pull request as ready for review November 2, 2023 18:33

octavia-squidington-iv requested a review from a team November 2, 2023 18:34

davydov-d requested changes Nov 2, 2023

View reviewed changes

octavia-squidington-iv requested a review from a team November 2, 2023 18:50

Add pattern and example for Endpoint field

8518c7a

tolik0 requested a review from davydov-d November 2, 2023 22:04

davydov-d approved these changes Nov 2, 2023

View reviewed changes

bazarnov approved these changes Nov 3, 2023

View reviewed changes

vercel bot deployed to Preview November 3, 2023 08:42 View deployment

Fix spec acceptance test

a7004b3

tolik0 force-pushed the tolik0/source-s3/http-endpoint-validation-error branch from e2b3123 to a7004b3 Compare November 3, 2023 11:54

Automated Commit - Formatting Changes

e030e33

alafanechere requested changes Nov 6, 2023

View reviewed changes

alafanechere reviewed Nov 7, 2023

View reviewed changes

allow s3 config to have http string

0d52e98

alafanechere requested a review from a team November 7, 2023 10:25

alafanechere approved these changes Nov 7, 2023

View reviewed changes

alafanechere reviewed Nov 7, 2023

View reviewed changes

Revert "allow s3 config to have http string"

3f78015

This reverts commit 0d52e98.

Merge branch 'master' into tolik0/source-s3/http-endpoint-validation-…

16e669a

…error

vercel bot deployed to Preview November 7, 2023 12:56 View deployment

tolik0 self-assigned this Nov 7, 2023

tolik0 added 3 commits November 9, 2023 21:38

Fix to raise HTTP error only in cloud

d3be02c

Delete files from previous from coverage

0d64ba8

Fix version

a8c7ed7

vercel bot deployed to Preview November 9, 2023 19:42 View deployment

Delete pytest.ini

0684cf4

roman-yermilov-gl approved these changes Nov 10, 2023

View reviewed changes

tolik0 merged commit 21cfb2a into master Nov 10, 2023
26 checks passed

tolik0 deleted the tolik0/source-s3/http-endpoint-validation-error branch November 10, 2023 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Source S3: Add HTTPS validation for S3 endpoint #32109

✨ Source S3: Add HTTPS validation for S3 endpoint #32109

tolik0 commented Nov 2, 2023

vercel bot commented Nov 2, 2023 •

edited

github-actions bot commented Nov 2, 2023 •

edited by tolik0

brianjlai commented Nov 2, 2023

davydov-d left a comment

alafanechere Nov 6, 2023

tolik0 Nov 6, 2023

alafanechere Nov 7, 2023

alafanechere Nov 6, 2023

tolik0 Nov 7, 2023

alafanechere Nov 7, 2023

alafanechere Nov 7, 2023

alafanechere Nov 7, 2023

tolik0 Nov 7, 2023

alafanechere Nov 7, 2023

tolik0 Nov 7, 2023

alafanechere left a comment

alafanechere Nov 7, 2023

alafanechere commented Nov 7, 2023 •

edited

✨ Source S3: Add HTTPS validation for S3 endpoint #32109

✨ Source S3: Add HTTPS validation for S3 endpoint #32109

Conversation

tolik0 commented Nov 2, 2023

What

How

vercel bot commented Nov 2, 2023 • edited

github-actions bot commented Nov 2, 2023 • edited by tolik0

Before Merging a Connector Pull Request

brianjlai commented Nov 2, 2023

davydov-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alafanechere commented Nov 7, 2023 • edited

vercel bot commented Nov 2, 2023 •

edited

github-actions bot commented Nov 2, 2023 •

edited by tolik0

alafanechere commented Nov 7, 2023 •

edited