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

Do not use sslCommonName from endpoint data #1960

Merged
merged 3 commits into from
Feb 5, 2020
Merged

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Feb 5, 2020

We originally introduced this to work around a py26 bug
where it couldn't read subjectAltNames for certs over a certain
size. We now no longer need this due to py26 being dropped,
and v2 is a natural place to change these endpoints over.

As part of this change I had to update the functional test
for regions. The main test generator was being shadowed by another
test name so as a result these tests were never actually running.

I had to update these tests to handle other endpoint related changes
we made (regional STS, S3 us-east-1, s3.region instead of
s3-region).

We originally introduced this to work around a py26 bug
where it couldn't read subjectAltNames for certs over a certain
size.  We now no longer need this due to py26 being dropped,
and v2 is a natural place to change these endpoints over.

As part of this change I had to update the functional test
for regions.  The main test generator was being shadowed by another
test name so as a result these tests were never actually running.

I had to update these tests to handle other endpoint related changes
we made (regional STS, S3 us-east-1, `s3.region` instead of
`s3-region`).
Relying on the system certs now fails on older macs,
and it's also closer to what we do in the SDK (pass in
an explicit cert bundle).
@codecov-io
Copy link

codecov-io commented Feb 5, 2020

Codecov Report

Merging #1960 into v2 will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v2    #1960      +/-   ##
==========================================
- Coverage   92.56%   92.55%   -0.01%     
==========================================
  Files          55       55              
  Lines       10323    10322       -1     
==========================================
- Hits         9555     9554       -1     
  Misses        768      768
Impacted Files Coverage Δ
botocore/client.py 99.72% <100%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39a0808...c5de41a. Read the comment docs.

Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Minor note, otherwise looks good to me.

@@ -465,8 +465,9 @@ def _test_single_service_region(service_name, region_name,
expected_endpoint, resolver):
bridge = ClientEndpointBridge(resolver, None, None)
result = bridge.resolve(service_name, region_name)
expected = 'https://%s' % expected_endpoint
assert_equal(result['endpoint_url'], expected)
if not expected_endpoint.startswith('http'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor, but we could use urlparse to ensure http is the scheme and not the start of the domain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Nice. Looks good to me. 🚢

jamesls added a commit that referenced this pull request Feb 5, 2020
PR #1960

* v2-tls-sni:
  Use urlparse to check for scheme
  Update s3 tests to provide bundled ca certs
  Do not use sslCommonName from endpoint data
@jamesls jamesls merged commit c5de41a into boto:v2 Feb 5, 2020
@bpodgursky
Copy link

@jamesls Did this ever make it into a botocore 1.x release so that it could be used by boto3? We're running into the issue with inconsistent sslCommonName data for services referenced by boto3.

I'm a bit confused about the versioning but it doesn't seem like botocore v2 is possible to use from boto3 (boto/boto3#2571) so I'm wondering if these changes ever get pulled back into the 1.x releases or if there's no plan for boto3 to ever pick this up.

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.

5 participants