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

fix(sdk): fixes trusted host is the same as index url. Fixes #10743 #10720

Closed
wants to merge 1 commit into from
Closed

fix(sdk): fixes trusted host is the same as index url. Fixes #10743 #10720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 4 additions & 4 deletions sdk/python/kfp/cli/component_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@ def test_docker_file_is_created_correctly_with_one_url(self):

WORKDIR /usr/local/src/kfp/components
COPY runtime-requirements.txt runtime-requirements.txt
RUN pip install --index-url https://pypi.org/simple --trusted-host https://pypi.org/simple --no-cache-dir -r runtime-requirements.txt
RUN pip install --index-url https://pypi.org/simple --trusted-host pypi.org --no-cache-dir -r runtime-requirements.txt

RUN pip install --index-url https://pypi.org/simple --trusted-host https://pypi.org/simple --no-cache-dir kfp==1.2.3
RUN pip install --index-url https://pypi.org/simple --trusted-host pypi.org --no-cache-dir kfp==1.2.3
COPY . .
'''))

Expand Down Expand Up @@ -527,9 +527,9 @@ def test_docker_file_is_created_correctly_with_two_urls(self):

WORKDIR /usr/local/src/kfp/components
COPY runtime-requirements.txt runtime-requirements.txt
RUN pip install --index-url https://pypi.org/simple --trusted-host https://pypi.org/simple --extra-index-url https://example.com/pypi/simple --trusted-host https://example.com/pypi/simple --no-cache-dir -r runtime-requirements.txt
RUN pip install --index-url https://pypi.org/simple --trusted-host pypi.org --extra-index-url https://example.com/pypi/simple --trusted-host example.com --no-cache-dir -r runtime-requirements.txt

RUN pip install --index-url https://pypi.org/simple --trusted-host https://pypi.org/simple --extra-index-url https://example.com/pypi/simple --trusted-host https://example.com/pypi/simple --no-cache-dir kfp==1.2.3
RUN pip install --index-url https://pypi.org/simple --trusted-host pypi.org --extra-index-url https://example.com/pypi/simple --trusted-host example.com --no-cache-dir kfp==1.2.3
COPY . .
'''))

Expand Down
7 changes: 5 additions & 2 deletions sdk/python/kfp/dsl/component_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import textwrap
from typing import (Any, Callable, Dict, List, Mapping, Optional, Tuple, Type,
Union)
import urllib
import warnings

import docstring_parser
Expand Down Expand Up @@ -94,9 +95,11 @@ def make_index_url_options(pip_index_urls: Optional[List[str]]) -> str:
index_url = pip_index_urls[0]
extra_index_urls = pip_index_urls[1:]

options = [f'--index-url {index_url} --trusted-host {index_url}']
options = [
f'--index-url {index_url} --trusted-host {urllib.parse.urlparse(index_url).hostname}'
]
options.extend(
f'--extra-index-url {extra_index_url} --trusted-host {extra_index_url}'
f'--extra-index-url {extra_index_url} --trusted-host {urllib.parse.urlparse(extra_index_url).hostname}'
Copy link

Choose a reason for hiding this comment

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

Should we consider the hostname as good enough? No need to specify also the port in case it's present in the original URL? In other words - we trust all apps on that domain in such case, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we consider the hostname as good enough?

Yes, it is sufficient.

No need to specify also the port in case it's present in the original URL?

Correct. The fix doesn't involve the port number.

In other words - we trust all apps on that domain in such case, right?

Could you please rephrase your question?

Copy link

Choose a reason for hiding this comment

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

That all was basically one question only - whether we feel confident and secure enough if we put just the hostname without the port here. IIUC the way it is done now means we trust all the services running on that hostname/domain. If we put also port number we would give our approval to only that one and only service that is listening on that one and only port.

Determining the port may be a bit tricky, though, since it may not be present in the URL explicitly but may need to be determined based on the default port of appropriate protocol in use in that URL. Though, we expect just HTTP(S) here, so it should be easy in the end, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jstourac please double-check #11151

for extra_index_url in extra_index_urls)

return ' '.join(options) + ' '
Expand Down
2 changes: 1 addition & 1 deletion sdk/python/kfp/dsl/component_factory_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ def test_with_packages_to_install_with_pip_index_url(self):
strip_kfp_version(command),
strip_kfp_version([
'sh', '-c',
'\nif ! [ -x "$(command -v pip)" ]; then\n python3 -m ensurepip || python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1 python3 -m pip install --quiet --no-warn-script-location --index-url https://myurl.org/simple --trusted-host https://myurl.org/simple \'kfp==2.1.3\' \'--no-deps\' \'typing-extensions>=3.7.4,<5; python_version<"3.9"\' && python3 -m pip install --quiet --no-warn-script-location --index-url https://myurl.org/simple --trusted-host https://myurl.org/simple \'package1\' \'package2\' && "$0" "$@"\n'
'\nif ! [ -x "$(command -v pip)" ]; then\n python3 -m ensurepip || python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1 python3 -m pip install --quiet --no-warn-script-location --index-url https://myurl.org/simple --trusted-host myurl.org \'kfp==2.1.3\' \'--no-deps\' \'typing-extensions>=3.7.4,<5; python_version<"3.9"\' && python3 -m pip install --quiet --no-warn-script-location --index-url https://myurl.org/simple --trusted-host myurl.org \'package1\' \'package2\' && "$0" "$@"\n'
]))


Expand Down
1 change: 1 addition & 0 deletions sdk/python/requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ types-PyYAML==6.0.5
types-requests==2.27.14
types-tabulate==0.8.6
yapf==0.32.0
uritemplate>=3.0.1,<4
4 changes: 2 additions & 2 deletions sdk/python/test_data/components/component_with_pip_install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ deploymentSpec:
- "\nif ! [ -x \"$(command -v pip)\" ]; then\n python3 -m ensurepip ||\
\ python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1\
\ python3 -m pip install --quiet --no-warn-script-location --index-url https://pypi.org/simple\
\ --trusted-host https://pypi.org/simple 'kfp==2.7.0' '--no-deps' 'typing-extensions>=3.7.4,<5;\
\ --trusted-host pypi.org 'kfp==2.7.0' '--no-deps' 'typing-extensions>=3.7.4,<5;\
\ python_version<\"3.9\"' && python3 -m pip install --quiet --no-warn-script-location\
\ --index-url https://pypi.org/simple --trusted-host https://pypi.org/simple\
\ --index-url https://pypi.org/simple --trusted-host pypi.org\
\ 'yapf' && \"$0\" \"$@\"\n"
- sh
- -ec
Expand Down
4 changes: 2 additions & 2 deletions sdk/python/test_data/pipelines/component_with_pip_index_urls.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ deploymentSpec:
- "\nif ! [ -x \"$(command -v pip)\" ]; then\n python3 -m ensurepip ||\
\ python3 -m ensurepip --user || apt-get install python3-pip\nfi\n\nPIP_DISABLE_PIP_VERSION_CHECK=1\
\ python3 -m pip install --quiet --no-warn-script-location --index-url https://pypi.org/simple\
\ --trusted-host https://pypi.org/simple 'kfp==2.7.0' '--no-deps' 'typing-extensions>=3.7.4,<5;\
\ --trusted-host pypi.org 'kfp==2.7.0' '--no-deps' 'typing-extensions>=3.7.4,<5;\
\ python_version<\"3.9\"' && python3 -m pip install --quiet --no-warn-script-location\
\ --index-url https://pypi.org/simple --trusted-host https://pypi.org/simple\
\ --index-url https://pypi.org/simple --trusted-host pypi.org\
\ 'yapf' && \"$0\" \"$@\"\n"
- sh
- -ec
Expand Down
Loading