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

Jump to bottom

Fix Addresses Used in IamAwsProvider #1460

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+4 −9
Open

Fix Addresses Used in IamAwsProvider #1460

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44db378
Update providers.py
sam-mosleh Nov 20, 2024
39c2f51
🚨 Remove unused imports
sam-mosleh Nov 22, 2024
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
13 changes: 4 additions & 9 deletions minio/credentials/providers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
from datetime import timedelta
from pathlib import Path
from typing import Callable, cast
from urllib.parse import urlencode, urlsplit, urlunsplit
from urllib.parse import urlencode, urlsplit
from xml.etree import ElementTree as ET

import certifi
Expand All @@ -44,7 +44,7 @@

from urllib3.util import Retry, parse_url

from minio.helpers import sha256_hash, url_replace
from minio.helpers import sha256_hash
from minio.signer import sign_v4_sts
from minio.time import from_iso8601utc, to_amz_date, utcnow
from minio.xml import find, findtext
Expand Down Expand Up @@ -508,18 +508,13 @@ def retrieve(self) -> Credentials:
res = _urlopen(
self._http_client,
"GET",
urlunsplit(
url_replace(
urlsplit(url),
path="/latest/meta-data/iam/security-credentials/",
),
),
url+"/latest/meta-data/iam/security-credentials/",
Copy link
Member

Choose a reason for hiding this comment

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

This breaks if custom_endpoint is passed. I think you would need to leave it as is.

Copy link
Author

@sam-mosleh sam-mosleh Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback!

I noticed the same logic is already applied on

minio-py/minio/credentials/providers.py

Line 501 in 759a5f3

url+"/latest/api/token",

which follows the same pattern used in the Go implementation
https://github.com/minio/minio-go/blob/ab1c2fe050a4dd502c863178696a54c44610ca82/pkg/credentials/iam_aws.go#L334

However, the Go implementation has an issue: it doesn’t account for cases where a custom endpoint contains a path. This leads to the assumption that the custom_endpoint doesn’t have a path, which is why I addressed it this way on line 511.

To resolve this, we have two options when a custom_endpoint is passed:

  1. Assume it does not contain a path and use simple string concatenation (+) in all instances where we manipulate the URL. (essentially what this PR did)
  2. Assume it can contain a path and ensure proper handling by using urllib.parse.SplitResult in all usages to safely override the path.

Given that the Go implementation itself is inconsistent—sometimes appending directly to the URL and other times parsing the URL—it would be helpful to align on one approach to avoid further confusion and potential issues.

Which approach do you think would be best to adopt moving forward?

headers=headers,
)
role_names = res.data.decode("utf-8").split("\n")
if not role_names:
raise ValueError(f"no IAM roles attached to EC2 service {url}")
url += "/" + role_names[0].strip("\r")
url += "/latest/meta-data/iam/security-credentials/" + role_names[0].strip("\r")
Copy link
Member

Choose a reason for hiding this comment

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

May be you would need to do to same logic as in line #511

if not url:
raise ValueError("url is empty; this should not happen")
self._credentials = self.fetch(url, headers=headers)
Expand Down