OADP-659 Remove HTTP/HTTPS port numbers from AWS S3 URLs. #1169

mrnold · 2023-10-02T19:14:48Z

Address OADP-659 by removing explicit port numbers from HTTP and HTTPS S3 URLs. The AWS SDK removes these before calculating a signature for blob access, and some S3-compatible servers do not expect this, causing a signature mismatch. Actual alternate port numbers (anything but 80 or 443) are unaffected.

Avoids a signature mismatch from S3-compatible services. Signed-off-by: Matthew Arnold <marnold@redhat.com>

shubham-pampattiwar · 2023-10-02T19:25:40Z

controllers/bsl.go

+	// (80 for HTTP and 443 for HTTPS) before calculating a signature, and not
+	// all S3-compatible services do this. Remove the ports here to avoid 403
+	// errors from mismatched signatures.
+	if bslSpec.Provider == "aws" {


This code will get executed for minio as well as noobaa, right ? Any idea if we need this port stripping for them too ?

Yes, it needs to run for anything that goes through the AWS SDK. I don't know if minio already deals with this, but I will try it and see. I think this change shouldn't cause any existing working connection types to stop working.

kaovilai · 2023-10-03T13:53:32Z

master do not support 4.10 anyways :)
/override ci/prow/4.10-operator-e2e-aws

openshift-ci · 2023-10-03T13:53:53Z

@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/4.10-operator-e2e-aws

In response to this:

master do not support 4.10 anyways :)
/override ci/prow/4.10-operator-e2e-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot · 2023-10-03T13:53:57Z

@mrnold: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

mrnold · 2023-10-03T15:49:24Z

On further testing, I found that you can configure a self-signed certificate that is only valid for the URL with the port number. I think removing this port number directly from the BSL will make it (the BSL) unusable. I don't know if it's a common practice to create certificates this way, but it is something to consider before merging this.

kaovilai · 2023-10-03T15:55:56Z

pkg/bucket/aws.go

+
+// StripDefaultPorts removes port 80 from HTTP URLs and 443 from HTTPS URLs.
+// Defer to the actual AWS SDK implementation to match its behavior exactly.
+func StripDefaultPorts(fromUrl string) (string, error) {
+	u, err := url.Parse(fromUrl)
+	if err != nil {
+		return "", err
+	}
+	r := http.Request{
+		URL: u,
+	}
+	request.SanitizeHostForHeader(&r)
+	r.URL.Host = r.Host
+	return r.URL.String(), nil
+}


LGTM but I would move this to more generic place.. maybe util.go you never know when this might be needed for non aws.

Okay, I tried moving this to pkg/common.

kaovilai · 2023-10-03T15:57:27Z

I don't know if it's a common practice to create certificates this way, but it is something to consider before merging this.

Could be a flag to disable the port stripping and we would note that this is not supported workflow for imagestream backup.

kaovilai · 2023-10-03T15:58:38Z

I think the usage is rare enough.. I would prefer we comment cert for "ported" URL is possible but not recommended (and so we wait for a cu request for a flag to support it) unless it's already being done for some openshift components (ODF/Noobaa) this way.

shawn-hurley · 2023-10-03T16:02:16Z

Can we quickly verify that the openshift router does not generate certs that way?

Signed-off-by: Matthew Arnold <marnold@redhat.com>

mrnold · 2023-10-03T19:14:58Z

Can we quickly verify that the openshift router does not generate certs that way?

I don't think it does, although I am not 100% sure how to verify this. I opened Networking->Routes in the OpenShift console, clicked a few of the externally-accessible links, and looked at the certificates each one provided. Does that cover what you were hoping to see? I didn't see any :80s or :443s attached to DNS names, and I think it would be kind of weird if there were any.

kaovilai · 2023-10-03T19:49:58Z

I didn't see any :80s or :443s attached to DNS names

then I think we're clear here. If you could point me to where in aws sdk is doing this and why aws BSL for normal backup do not hit this would be great as well.

shubham-pampattiwar · 2023-10-03T20:02:23Z

@kaovilai Is this what you wanted for reference? https://github.com/aws/aws-sdk-go/blob/v1.45.20/aws/request/request.go#L717

kaovilai · 2023-10-03T20:07:07Z

wanted

yup. Guess now my question is why BSL doesn't break.. but imagestream copy does.

kaovilai · 2023-10-03T22:35:19Z

/override ci/prow/4.10-operator-e2e-aws
/override ci/prow/4.10-operator-e2e-azure

4.10 ci no longer works on master. should be removed soon.

openshift-ci · 2023-10-03T22:35:36Z

@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/4.10-operator-e2e-aws, ci/prow/4.10-operator-e2e-azure

In response to this:

/override ci/prow/4.10-operator-e2e-aws
/override ci/prow/4.10-operator-e2e-azure

4.10 ci no longer works on master. should be removed soon.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kaovilai · 2023-10-03T22:40:29Z

pkg/common/common.go

@@ -198,3 +201,18 @@ func CCOWorkflow() bool {
 	}
 	return false
 }
+
+// StripDefaultPorts removes port 79 from HTTP URLs and 443 from HTTPS URLs.


Suggested change

// StripDefaultPorts removes port 79 from HTTP URLs and 443 from HTTPS URLs.

// StripDefaultPorts removes port 80 from HTTP URLs and 443 from HTTPS URLs.

I have no idea how this happened.

Signed-off-by: Matthew Arnold <marnold@redhat.com>

kaovilai · 2023-10-04T14:07:52Z

/override ci/prow/4.10-operator-e2e-aws
/override ci/prow/4.10-operator-e2e-azure

openshift-ci · 2023-10-04T14:14:46Z

@kaovilai: Overrode contexts on behalf of kaovilai: ci/prow/4.10-operator-e2e-aws, ci/prow/4.10-operator-e2e-azure

In response to this:

/override ci/prow/4.10-operator-e2e-aws
/override ci/prow/4.10-operator-e2e-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2023-10-04T14:15:29Z

@mrnold: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

shubham-pampattiwar · 2023-10-04T15:53:32Z

/lgtm

openshift-ci · 2023-10-04T15:59:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, mrnold, shubham-pampattiwar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kaovilai,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mateusoliveira43 · 2023-10-09T18:56:15Z

pkg/common/common.go

+
+// StripDefaultPorts removes port 80 from HTTP URLs and 443 from HTTPS URLs.
+// Defer to the actual AWS SDK implementation to match its behavior exactly.
+func StripDefaultPorts(fromUrl string) (string, error) {


here I ran a test focusing on IBM and was not successful

func TestStripDefaultPorts(t *testing.T) { base := "https://s3.region.cloud-object-storage.appdomain.cloud/bucket-name" want := "https://s3.region.cloud-object-storage.appdomain.cloud/bucket-name" t.Run("original URL is returned", func(t *testing.T) { got, err := StripDefaultPorts(base) if err != nil { t.Errorf("An error occurred: %v", err) } if !reflect.DeepEqual(got, want) { t.Errorf("StripDefaultPorts() = %v, want %v", got, want) } }) }

Add a test case and fix?

I think this is a new test case, right Mateus? Thanks for adding this. In this test, StripDefaultPorts correctly extracts the host name from the URL, but then overwrites it with a blank string. That blank string comes from an HTTP request field that is not blank when running against a real AWS cluster. I will add a fix that should get this test working.

This should fix it: #1177

…1169) * OADP-659 Remove port numbers if HTTP or HTTPS. Avoids a signature mismatch from S3-compatible services. Signed-off-by: Matthew Arnold <marnold@redhat.com> * OADP-659 Move StripDefaultPorts to pkg/common. Signed-off-by: Matthew Arnold <marnold@redhat.com> * OADP-659 Fix port number in comment. Signed-off-by: Matthew Arnold <marnold@redhat.com> --------- Signed-off-by: Matthew Arnold <marnold@redhat.com>

… AWS S3 URLs. (#1180) * OADP-659 Remove HTTP/HTTPS port numbers from AWS S3 URLs. (#1169) * OADP-659 Remove port numbers if HTTP or HTTPS. Avoids a signature mismatch from S3-compatible services. Signed-off-by: Matthew Arnold <marnold@redhat.com> * OADP-659 Move StripDefaultPorts to pkg/common. Signed-off-by: Matthew Arnold <marnold@redhat.com> * OADP-659 Fix port number in comment. Signed-off-by: Matthew Arnold <marnold@redhat.com> --------- Signed-off-by: Matthew Arnold <marnold@redhat.com> * OADP-659 Avoid overwriting good hostname. (#1177) SanitizeHostForHeader puts the stripped host name into the Host field of the HTTP request, but only if it actually needed to strip the port. If there is already no port, that Host field can be blank, and there is no reason to overwrite the URL's host field. Signed-off-by: Matthew Arnold <marnold@redhat.com> --------- Signed-off-by: Matthew Arnold <marnold@redhat.com>

OADP-659 Remove port numbers if HTTP or HTTPS.

bd3fec0

Avoids a signature mismatch from S3-compatible services. Signed-off-by: Matthew Arnold <marnold@redhat.com>

openshift-ci bot requested review from shubham-pampattiwar and sseago October 2, 2023 19:15

shubham-pampattiwar reviewed Oct 2, 2023

View reviewed changes

kaovilai previously approved these changes Oct 3, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2023

OADP-659 Move StripDefaultPorts to pkg/common.

e1b31cf

Signed-off-by: Matthew Arnold <marnold@redhat.com>

mrnold dismissed kaovilai’s stale review via e1b31cf October 3, 2023 18:31

openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2023

kaovilai reviewed Oct 3, 2023

View reviewed changes

OADP-659 Fix port number in comment.

6a7c75a

Signed-off-by: Matthew Arnold <marnold@redhat.com>

kaovilai approved these changes Oct 4, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2023

shubham-pampattiwar approved these changes Oct 4, 2023

View reviewed changes

openshift-ci bot assigned shubham-pampattiwar Oct 4, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2023

openshift-ci bot merged commit e119cc0 into openshift:master Oct 4, 2023
21 checks passed

mateusoliveira43 reviewed Oct 9, 2023

View reviewed changes

mrnold mentioned this pull request Oct 10, 2023

OADP-659 Avoid overwriting good hostname. #1177

Merged

mrnold mentioned this pull request Oct 12, 2023

OADP-659/OADP-2862 Add unit tests for #1169 and #1177. #1182

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OADP-659 Remove HTTP/HTTPS port numbers from AWS S3 URLs. #1169

OADP-659 Remove HTTP/HTTPS port numbers from AWS S3 URLs. #1169

mrnold commented Oct 2, 2023 •

edited by openshift-ci bot

shubham-pampattiwar Oct 2, 2023

mrnold Oct 2, 2023

kaovilai commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

openshift-merge-robot commented Oct 3, 2023

mrnold commented Oct 3, 2023

kaovilai Oct 3, 2023

mrnold Oct 3, 2023

kaovilai commented Oct 3, 2023

kaovilai commented Oct 3, 2023 •

edited

shawn-hurley commented Oct 3, 2023

mrnold commented Oct 3, 2023

kaovilai commented Oct 3, 2023

shubham-pampattiwar commented Oct 3, 2023

kaovilai commented Oct 3, 2023

kaovilai commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

kaovilai Oct 3, 2023

mrnold Oct 4, 2023

kaovilai commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

shubham-pampattiwar commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

mateusoliveira43 Oct 9, 2023

kaovilai Oct 9, 2023

mrnold Oct 10, 2023

mrnold Oct 10, 2023

	// StripDefaultPorts removes port 79 from HTTP URLs and 443 from HTTPS URLs.
	// StripDefaultPorts removes port 80 from HTTP URLs and 443 from HTTPS URLs.

OADP-659 Remove HTTP/HTTPS port numbers from AWS S3 URLs. #1169

OADP-659 Remove HTTP/HTTPS port numbers from AWS S3 URLs. #1169

Conversation

mrnold commented Oct 2, 2023 • edited by openshift-ci bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaovilai commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

openshift-merge-robot commented Oct 3, 2023

mrnold commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaovilai commented Oct 3, 2023

kaovilai commented Oct 3, 2023 • edited

shawn-hurley commented Oct 3, 2023

mrnold commented Oct 3, 2023

kaovilai commented Oct 3, 2023

shubham-pampattiwar commented Oct 3, 2023

kaovilai commented Oct 3, 2023

kaovilai commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaovilai commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

shubham-pampattiwar commented Oct 4, 2023

openshift-ci bot commented Oct 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnold commented Oct 2, 2023 •

edited by openshift-ci bot

kaovilai commented Oct 3, 2023 •

edited