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

s3 storage initializer: only set environment variables if variables are set in storage secret json #129

Conversation

dtrifiro
Copy link

@dtrifiro dtrifiro commented Nov 22, 2023

By setting environment variables to the result of .get(..., ""), the corresponding env variables are set to an empty value.

This is an issue for some values, such as AWS_CA_BUNDLE, which should be set to a path pointing to a valid CA bundle. When set to an empty string,
it is propagated all the way down to botocore.httpsession.URLLib3Session._setup_ssl_cert,
which interprets it as False and disables verification.

See https://github.com/boto/botocore/blob/6e0ec833714ed88d46e294048cdb0d3869eb2ab5/botocore/httpsession.py#L376-L382

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Feature/Issue validation/testing:

See the original issue on #137: InsecureRequestWarning: Unverified HTTPS request warnings were being printed when retrieving data from S3 (using https)

Setting AWS_CA_BUNDLE to a non-null value gets rid of the warning.


Release notes:

fix processing of storage secrets that prevented verification of TLS connections

Copy link

openshift-ci bot commented Nov 22, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dtrifiro
Once this PR has been reviewed and has the lgtm label, please assign heyselbi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dtrifiro
Copy link
Author

Also opened a PR upstream at kserve#3259

@dtrifiro dtrifiro self-assigned this Nov 22, 2023
@dtrifiro dtrifiro force-pushed the fix-storage-initializer-ca-verification-odh branch 2 times, most recently from 8e36fe3 to 10483fb Compare November 22, 2023 18:24
…re set in storage secret json

By setting environment variables to the result of `.get(..., "")`,
the corresponding env variables are set to an empty value.

This is an issue for some values, such as `AWS_CA_BUNDLE`, which should
be set to a path pointing to a valid CA bundle. When set to an empty string,
 it is propagated all the way down to `botocore.httpsession.URLLib3Session._setup_ssl_cert`,
 which interprets it as `False` and disables verification.

See https://github.com/boto/botocore/blob/6e0ec833714ed88d46e294048cdb0d3869eb2ab5/botocore/httpsession.py#L376-L382

Signed-off-by: Daniele Trifirò <[email protected]>
@dtrifiro
Copy link
Author

dtrifiro commented Nov 22, 2023

See kserve#3259 (comment) for more details on how to reproduce and see how to test

@israel-hdez
Copy link

Closing in favor of the upstream PR, which would be picked in a code sync.

@dtrifiro dtrifiro deleted the fix-storage-initializer-ca-verification-odh branch November 23, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants