OpenStack: get rid of swift temp urls #2311

Fedosin · 2019-09-04T14:11:57Z

Since not all the configurations of OSP support Swift temp urls, we have to find another way that will work on all supported clouds.

To create a unified solution we can use the ACL mechanism to allow public access to the ignition file, but deny access to the list of container files. From a technical point of view, it looks like this: ".r:*", but no ".rlistings".
This allows anybody to download bootstrap ignition file. However, to do this, the exact name of the object must be known since users cannot list the objects in the container.

The last step is to randomize the file name so that it cannot be matched - now it is hardcoded to "bootstrap.ign"

The lifetime of such a file will be short, about 10 minutes necessary for bootstrapping. Further this file will be automatically removed by Terraform at the moment of bootstrap machine destruction.

This way we can achieve the same effect as temp url, which will work on all types of storage.

Fedosin · 2019-09-04T14:16:10Z

/hold

we need to agree that we need this patch at 4.2

celebdor · 2019-09-05T13:09:56Z

pkg/tfvars/openstack/openstack.go

+		if svc.Type == "object-store" {
+			for _, e := range svc.Endpoints {
+				if e.Interface == "public" {
+					swiftPublicURL = e.URL


shouldn't you break here? why going over the rest?

yep, you're right

openshift-ci-robot · 2019-09-05T17:22:22Z

@Fedosin: This pull request references Bugzilla bug 1749367, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1749367: OpenStack: get rid of swift temp urls

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-robot · 2019-09-05T17:22:46Z

@Fedosin: This pull request references Bugzilla bug 1749367, which is valid.

In response to this:

Bug 1749367: OpenStack: get rid of swift temp urls

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.

Fedosin · 2019-09-05T17:22:56Z

/hold cancel

MartijnStraatman · 2019-09-05T17:53:58Z

I pulled this PR locally and tried to use it. First run was sucessfull. Second fails with this error:

time="2019-09-05T13:48:54-04:00" level=error msg="Error: Error creating OpenStack container object: parse https://<url>/swift/v1/<token>/os-test-tgb8j/aDAT#dtzs2%[Vm%G: invalid URL escape \"%[V\""
time="2019-09-05T13:48:54-04:00" level=error
time="2019-09-05T13:48:54-04:00" level=error msg="  on ../../../tmp/openshift-install-026823105/bootstrap/main.tf line 54, in resource \"openstack_objectstorage_object_v1\" \"ignition\":"
time="2019-09-05T13:48:54-04:00" level=error msg="  54: resource \"openstack_objectstorage_object_v1\" \"ignition\" {"
time="2019-09-05T13:48:54-04:00" level=error
time="2019-09-05T13:48:54-04:00" level=error
time="2019-09-05T13:48:54-04:00" level=fatal msg="failed to fetch Cluster: failed to generate asset \"Cluster\": failed to create cluster: failed to apply using Terraform"

Fedosin · 2019-09-05T19:44:05Z

I pulled this PR locally and tried to use it. First run was sucessfull. Second fails with this error:

time="2019-09-05T13:48:54-04:00" level=error msg="Error: Error creating OpenStack container object: parse https://<url>/swift/v1/<token>/os-test-tgb8j/aDAT#dtzs2%[Vm%G: invalid URL escape \"%[V\""
time="2019-09-05T13:48:54-04:00" level=error
time="2019-09-05T13:48:54-04:00" level=error msg="  on ../../../tmp/openshift-install-026823105/bootstrap/main.tf line 54, in resource \"openstack_objectstorage_object_v1\" \"ignition\":"
time="2019-09-05T13:48:54-04:00" level=error msg="  54: resource \"openstack_objectstorage_object_v1\" \"ignition\" {"
time="2019-09-05T13:48:54-04:00" level=error
time="2019-09-05T13:48:54-04:00" level=error
time="2019-09-05T13:48:54-04:00" level=fatal msg="failed to fetch Cluster: failed to generate asset \"Cluster\": failed to create cluster: failed to apply using Terraform"

yeah, we need to use alphanumeric characters for the object name. Thank you for finding this!

Fedosin · 2019-09-06T10:00:48Z

/test e2e-aws-upgrade

Fedosin · 2019-09-06T10:46:41Z

/test e2e-aws-upgrade

Fedosin · 2019-09-09T12:55:10Z

/test e2e-libvirt

sdodson · 2019-09-09T17:06:37Z

@celebdor PTAL or delegate to someone else for review

tomassedovic · 2019-09-09T17:07:14Z

/assign

I'm testing and reviewing it as we speak.

tomassedovic · 2019-09-10T10:40:21Z

Okay, I like it. I would really prefer if we kept using Swift temp urls since their behaviour and security profile are well understood, but not supporting OSP 13 + Ceph's RADOS Gateway would cut off a significant portion of the production users.

That said, I am no security expert and as such I am a little nervous about this change -- especially this late in the game.

To the best of my abilities, I've verified that:

The Swift object name appears different and unpredictable on every deployment
The Terraform random_string provider uses a cryptographically secure random generator
An unauthorised user cannot list the container's objects (and therefore find the name and access the sensitive Ignition contents)
The object is deleted after the bootstrapping phase so it doesn't linger indefinitely

There are some things I can see to make it even more solid:

Use the random_password resource instead of random_string. They should behave identically, but the password one is not echoed in any of the Terraform logs which means it won't leak in the CI (https://www.terraform.io/docs/providers/random/r/password.html)
Consider setting a timeout (the delete_after property) on the bootstrap ignition object (https://www.terraform.io/docs/providers/openstack/r/objectstorage_object_v1.html#delete_after). This will make the ignition inaccessible even if the bootstrap resources never get deleted (e.g. in the case of a failure). Probably not a huge issue, just tightening up.

I am tentatively in favour of this but I'd like to hold off from lgtm before more people have their say.

//cc @mandre (what do you think about the risks present here) and @racedo (what do you think about the impact of not shipping this feature in 4.2)

celebdor

I agree with Tomáš' suggestions.

abhinavdahiya · 2019-10-21T20:31:45Z

/test e2e-gcp

abhinavdahiya · 2019-10-21T20:41:15Z

pkg/tfvars/openstack/openstack.go

+		DomainName: domainName,
+	}
+
+	serviceCatalog, err := tokens.Create(conn, &authOptions).ExtractServiceCatalog()


Hmm, i'm not sure what this does ? but does it create some resource that needs to be cleaned up on destroy?

Here we want to obtain Swift public endpoint... By design this should be done by using https://www.terraform.io/docs/providers/openstack/d/identity_endpoint_v3.html but OpenStack default policies forbid to use this API for regular users.
On the other hand when you authenticate in OpenStack (i.e. get a token) it includes the whole service catalog in the output json. So we are able to parse it and get the endpoint from there https://docs.openstack.org/api-ref/identity/v3/?expanded=token-authentication-with-scoped-authorization-detail#token-authentication-with-scoped-authorization
In short the algorithm is:

Authenticate in OpenStack: tokens.Create(..)

Parse the token and extract the service catalog: ExtractServiceCatalog()

Iterate through the catalog and find public endpoint for object-store.

Unfortunately this feature is not supported by Terraform so I had to implement it here. No resources are created we just get endpoint data from OpenStack.

Btw, we do the same thing for Octavia service: https://github.com/openshift/installer/blob/master/pkg/asset/installconfig/openstack/openstack.go#L141-L152

abhinavdahiya · 2019-10-21T20:41:35Z

pkg/tfvars/openstack/openstack.go

+	return swiftPublicURL, nil
+}
+
+func getServiceCatalog(cloud string) (*tokens.ServiceCatalog, error) {


would love to see some comments on the function regarding what it does.

abhinavdahiya · 2019-10-21T20:41:59Z

pkg/tfvars/openstack/openstack.go

@@ -62,3 +73,69 @@ func TFVars(masterConfig *v1alpha1.OpenstackProviderSpec, cloud string, external

 	return json.MarshalIndent(cfg, "", "  ")
 }
+
+func getSwiftPublicURL(cloud string) (string, error) {


same as 5c3fbf5#r337229011

The new version contains random_password module required by OpenStack

Since not all the configurations of OSP support Swift temp urls, we have to find another way that will work on all supported clouds. To create a unified solution we can use the ACL mechanism to allow public access to the file, but deny access to the list of container files. From a technical point of view, it looks like this: ".r:*", but no ".rlistings". This allows anybody to download the ignition file. However, to do this, the exact name of the object must be known since users cannot list the objects in the container. The last step is to randomize the file name so that it cannot be matched - now it is hardcoded to "bootstrap.ign" The lifetime of such a file will be short, about 10 minutes necessary for bootstrapping. Further this file will be automatically removed by Terraform at the moment of bootstrap machine destruction. This way we can achieve the same effect as temp url, which will work on all types of storage.

Fedosin · 2019-10-22T17:20:36Z

/test e2e-openstack

Fedosin · 2019-10-22T17:49:19Z

/test e2e-aws

Fedosin · 2019-10-22T18:25:27Z

/test e2e-openstack

Fedosin · 2019-10-22T19:35:37Z

/test e2e-aws-disruptive

openshift-ci-robot · 2019-10-22T19:43:37Z

@Fedosin: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`9a8a235`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-aws-disruptive	`9a8a235`	link	`/test e2e-aws-disruptive`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

abhinavdahiya · 2019-10-22T20:03:40Z

8d04568 LGTM

/approve

while the rest i'll leave upto the openstack team to review

iamemilio · 2019-10-22T20:34:48Z

/lgtm

openshift-ci-robot · 2019-10-22T20:35:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, Fedosin, iamemilio, mandre

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~~ [abhinavdahiya]

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

Fedosin · 2019-10-23T17:12:28Z

/cherrypick release-4.2

openshift-cherrypick-robot · 2019-10-23T17:12:47Z

@Fedosin: #2311 failed to apply on top of branch "release-4.2":

Applying: OpenStack: get rid of swift temp urls
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	data/data/openstack/bootstrap/main.tf
M	data/data/openstack/bootstrap/variables.tf
M	data/data/openstack/main.tf
M	data/data/openstack/variables-openstack.tf
M	docs/user/openstack/README.md
M	pkg/tfvars/openstack/openstack.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/tfvars/openstack/openstack.go
CONFLICT (content): Merge conflict in pkg/tfvars/openstack/openstack.go
Auto-merging docs/user/openstack/README.md
CONFLICT (content): Merge conflict in docs/user/openstack/README.md
Auto-merging data/data/openstack/variables-openstack.tf
Auto-merging data/data/openstack/main.tf
Auto-merging data/data/openstack/bootstrap/variables.tf
Auto-merging data/data/openstack/bootstrap/main.tf
Patch failed at 0002 OpenStack: get rid of swift temp urls

In response to this:

/cherrypick release-4.2

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.

MartijnStraatman · 2019-10-30T16:06:17Z

will this PR be available in 4.2? If we install 4.2 by compiling the 4.2 release this feature seems to be missing. Maybe due to the merge-conflict above?

With openshift#2311 being merged, and having removed the need for Swift tempurls, the issue is no longer afflicting OpenStack installations. Ref.: openshift#2311

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 4, 2019

openshift-ci-robot requested review from jcpowermac and mtnbikenc September 4, 2019 14:12

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2019

celebdor reviewed Sep 5, 2019

View reviewed changes

Fedosin force-pushed the no_temp_urls branch from 2d9c499 to 85f5d64 Compare September 5, 2019 17:21

Fedosin changed the title ~~OpenStack: get rid of swift temp urls~~ Bug 1749367: OpenStack: get rid of swift temp urls Sep 5, 2019

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 5, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2019

Fedosin force-pushed the no_temp_urls branch from 85f5d64 to 93e8fe3 Compare September 5, 2019 19:47

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2019

Fedosin force-pushed the no_temp_urls branch from 93e8fe3 to 763e690 Compare September 5, 2019 21:00

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2019

sdodson removed request for jcpowermac and mtnbikenc September 9, 2019 17:06

openshift-ci-robot assigned tomassedovic Sep 9, 2019

celebdor suggested changes Sep 10, 2019

View reviewed changes

abhinavdahiya reviewed Oct 21, 2019

View reviewed changes

Fedosin force-pushed the no_temp_urls branch from 5c3fbf5 to 21b5187 Compare October 22, 2019 10:34

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2019

Bump terraform-provider-random

8d04568

The new version contains random_password module required by OpenStack

Fedosin force-pushed the no_temp_urls branch from 21b5187 to 1d27d83 Compare October 22, 2019 16:22

Fedosin force-pushed the no_temp_urls branch from 1d27d83 to 9a8a235 Compare October 22, 2019 16:30

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2019

openshift-ci-robot assigned iamemilio Oct 22, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2019

openshift-merge-robot merged commit 9b594ab into openshift:master Oct 22, 2019

pierreprinetti mentioned this pull request Oct 24, 2019

openstack: Remove reference to tempurl known issue #2558

Merged

Fedosin deleted the no_temp_urls branch November 1, 2019 11:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenStack: get rid of swift temp urls #2311

OpenStack: get rid of swift temp urls #2311

Fedosin commented Sep 4, 2019 •

edited

Loading

Fedosin commented Sep 4, 2019

celebdor Sep 5, 2019

Fedosin Sep 5, 2019

openshift-ci-robot commented Sep 5, 2019

openshift-ci-robot commented Sep 5, 2019

Fedosin commented Sep 5, 2019

MartijnStraatman commented Sep 5, 2019

Fedosin commented Sep 5, 2019

Fedosin commented Sep 6, 2019

Fedosin commented Sep 6, 2019

Fedosin commented Sep 9, 2019

sdodson commented Sep 9, 2019

tomassedovic commented Sep 9, 2019

tomassedovic commented Sep 10, 2019

celebdor left a comment

abhinavdahiya commented Oct 21, 2019

abhinavdahiya Oct 21, 2019

Fedosin Oct 22, 2019

abhinavdahiya Oct 21, 2019

Fedosin Oct 22, 2019

abhinavdahiya Oct 21, 2019

Fedosin Oct 22, 2019

Fedosin commented Oct 22, 2019

Fedosin commented Oct 22, 2019

Fedosin commented Oct 22, 2019

Fedosin commented Oct 22, 2019

openshift-ci-robot commented Oct 22, 2019

abhinavdahiya commented Oct 22, 2019

iamemilio commented Oct 22, 2019

openshift-ci-robot commented Oct 22, 2019

Fedosin commented Oct 23, 2019

openshift-cherrypick-robot commented Oct 23, 2019

MartijnStraatman commented Oct 30, 2019 •

edited

Loading

OpenStack: get rid of swift temp urls #2311

OpenStack: get rid of swift temp urls #2311

Conversation

Fedosin commented Sep 4, 2019 • edited Loading

Fedosin commented Sep 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 5, 2019

openshift-ci-robot commented Sep 5, 2019

Fedosin commented Sep 5, 2019

MartijnStraatman commented Sep 5, 2019

Fedosin commented Sep 5, 2019

Fedosin commented Sep 6, 2019

Fedosin commented Sep 6, 2019

Fedosin commented Sep 9, 2019

sdodson commented Sep 9, 2019

tomassedovic commented Sep 9, 2019

tomassedovic commented Sep 10, 2019

celebdor left a comment

Choose a reason for hiding this comment

abhinavdahiya commented Oct 21, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fedosin commented Oct 22, 2019

Fedosin commented Oct 22, 2019

Fedosin commented Oct 22, 2019

Fedosin commented Oct 22, 2019

openshift-ci-robot commented Oct 22, 2019

abhinavdahiya commented Oct 22, 2019

iamemilio commented Oct 22, 2019

openshift-ci-robot commented Oct 22, 2019

Fedosin commented Oct 23, 2019

openshift-cherrypick-robot commented Oct 23, 2019

MartijnStraatman commented Oct 30, 2019 • edited Loading

Fedosin commented Sep 4, 2019 •

edited

Loading

MartijnStraatman commented Oct 30, 2019 •

edited

Loading