Skip to content

Enforce https in search #11337

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

Merged
rakshith91 merged 8 commits into from
May 13, 2020
Merged

Enforce https in search #11337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
467da53
Enforce https in search
rakshith91 May 8, 2020
e78c0ec
commit-1
rakshith91 May 11, 2020
0d6b4ed
base
rakshith91 May 12, 2020
0444d6a
Update sdk/search/azure-search-documents/azure/search/documents/_serv…
May 12, 2020
83aee6d
Update sdk/search/azure-search-documents/azure/search/documents/_serv…
May 12, 2020
f67fa22
comments
rakshith91 May 13, 2020
06eb9b7
oops
rakshith91 May 13, 2020
9d034a3
remove admonition
rakshith91 May 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions sdk/search/azure-search-documents/azure/search/documents/_service/_search_service_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ class SearchServiceClient(HeadersMixin): # pylint: disable=too-many-public-meth
def __init__(self, endpoint, credential, **kwargs):
# type: (str, AzureKeyCredential, **Any) -> None

try:
if endpoint.lower().startswith('http') and not endpoint.lower().startswith('https'):
Copy link
Member

@lmazuel lmazuel May 8, 2020

Choose a reason for hiding this comment

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

Is there scenario were a valid endpoint would not start by http?

Copy link
Contributor Author

@rakshith91 rakshith91 May 8, 2020

Choose a reason for hiding this comment

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

service only supports https

raise ValueError("Endpoint should be secure. Use https.")
Copy link
Member

@lmazuel lmazuel May 8, 2020

Choose a reason for hiding this comment

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

I would use a wording more consistent with Azure-core:

azure-sdk-for-python/sdk/core/azure-core/azure/core/pipeline/policies/_authentication.py

Line 55 in cd25de5

"Bearer token authentication is not permitted for non-TLS protected (non-https) URLs."

if not endpoint.lower().startswith('http'):
endpoint = "https://" + endpoint
except AttributeError:
raise ValueError("Endpoint must be a string")

self._endpoint = endpoint # type: str
self._credential = credential # type: AzureKeyCredential
self._client = _SearchServiceClient(
Expand Down
8 changes: 8 additions & 0 deletions ...zure-search-documents/azure/search/documents/_service/aio/_search_service_client_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ class SearchServiceClient(HeadersMixin): # pylint: disable=too-many-public-meth
def __init__(self, endpoint, credential, **kwargs):
# type: (str, AzureKeyCredential, **Any) -> None

try:
Copy link
Member

@lmazuel lmazuel May 8, 2020

Choose a reason for hiding this comment

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

It's a shame we have to duplicate this code, should we have a SeachServiceClientBase?

Copy link
Contributor Author

@rakshith91 rakshith91 May 8, 2020

Choose a reason for hiding this comment

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

I was think about the same - Will use this PR to do that

if endpoint.lower().startswith('http') and not endpoint.lower().startswith('https'):
raise ValueError("Endpoint should be secure. Use https.")
if not endpoint.lower().startswith('http'):
endpoint = "https://" + endpoint
except AttributeError:
raise ValueError("Endpoint must be a string")

self._endpoint = endpoint # type: str
self._credential = credential # type: AzureKeyCredential
self._client = _SearchServiceClient(
Expand Down
16 changes: 15 additions & 1 deletion sdk/search/azure-search-documents/tests/test_search_service_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def test_credential_roll(self):
def test_repr(self):
client = SearchServiceClient("endpoint", CREDENTIAL)
assert repr(client) == "<SearchServiceClient [endpoint={}]>".format(
repr("endpoint")
repr("https://endpoint")
)

@mock.patch(
Expand All @@ -52,3 +52,17 @@ def test_get_service_statistics(self, mock_get_stats):
assert mock_get_stats.called
assert mock_get_stats.call_args[0] == ()
assert mock_get_stats.call_args[1] == {"headers": client._headers}

def test_endpoint_https(self):
credential = AzureKeyCredential(key="old_api_key")
client = SearchServiceClient("endpoint", credential)
assert client._endpoint.startswith('https')

client = SearchServiceClient("https://endpoint", credential)
assert client._endpoint.startswith('https')

with pytest.raises(ValueError):
client = SearchServiceClient("http://endpoint", credential)

with pytest.raises(ValueError):
client = SearchServiceClient(12345, credential)