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

Strip port 80/443 from host #802

Merged
merged 2 commits into from
Sep 30, 2019
Merged

Conversation

edward-codecov
Copy link
Contributor

Supplying port 80 or 443 with host (ex. storage.googleapis.com:443) fails in v4 signing. Remove these default ports to prevent signature matching failure.

Similar issue:
aws/aws-cli#2883

Supplying port 80 or 443 with host (ex. `storage.googleapis.com:443`) fails in v4 signing. Remove these default ports to prevent signature matching failure.

Similar issue:
aws/aws-cli#2883
@sinhaashish
Copy link
Contributor

@edward-codecov Thanks for you PR.
IMO you can try something like

 url_parts = urlsplit(url)
        default_ports = {
            'http': 80,
            'https': 443
        }
        if any(url_parts.scheme == scheme and url_parts.port == port
               for scheme, port in default_ports.items()):
            # No need to include the port if it's the default port.
            return url_parts.hostname

This way we keep a track that if port 80 is stripped then it belongs to http and 443 for https.
This is a better way than just stripping with the use of if condition like the one you did if parsed_url.port is 80 or 443:

@edward-codecov
Copy link
Contributor Author

@sinhaashish great idea; thanks! updated pr.

Copy link
Contributor

@sinhaashish sinhaashish left a comment

Choose a reason for hiding this comment

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

Tested & LGTM

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

LGTM & tested

@nitisht nitisht merged commit 721cacc into minio:master Sep 30, 2019
@edward-codecov edward-codecov deleted the omit-default-port branch September 30, 2019 13:35
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.

4 participants