Tsb userinfo groups #15719

gabemontero · 2017-08-09T21:42:18Z

@openshift/devex PR for https://trello.com/c/EXHlyops/1230-5-use-groups-extra-information-for-authorization-in-template-service-broker-templatebroker

A few notes:

the current set of test changes include an initial foray from @jim-minter plus ones I have added/changed to the set of unit tests and extended tests for the template service broker; I've started looking at updating the cluster up extended tests to somehow leverage @jim-minter 's test scripts to vet things out at that level, but I decided to not hold up the PR review on that, as its possibility is still TBD in my mind until I can do more work with it
there was a foray into using user.Info in the requester object, but you can't do protobufs on interfaces
per an exchange with @pmorie , the use of the requester username param was added back in for now, given the state of some related upstream activity in this space; it was changed to be a back up option to the new form, and not required; I've tried to clearly label with TODO comments what blocks should go away, what we are waiting for, etc.

gabemontero · 2017-08-09T21:43:01Z

(5) Use groups/extra information for authorization in template service broker [template_broker]

gabemontero · 2017-08-10T17:37:49Z

made some minor updates to the validation code around still allowing the username request paramter, as well as some tweaks to the test scripts to more easily align with oc cluster up --server-catalog

those were a precursor to updating the cluster up extended tests to validate the flow

jim-minter · 2017-08-10T18:28:17Z

/unassign @jim-minter

jim-minter · 2017-08-10T18:31:38Z

pkg/openservicebroker/server/route.go

+		return &user.DefaultInfo{}, nil
+	}
+	parts := strings.SplitN(identity, " ", 2)
+	if parts[0] != api.OriginatingIdentitySchemeKubernetes || len(parts) != 2 {


please double check if you should be checking against kubernetes or Kubernetes, or do a strings.ToLower

yep, it is lower case ....I'll update the constant in types.go

think strings.ToLower is best

I'll do both ... golang needs an equalsIgnoreCase like java :-)

https://golang.org/pkg/strings/#EqualFold

cool ... never heard the term "case-folding" ... thanks.

gabemontero · 2017-08-10T18:34:51Z

pushed the clusterup extended test

jim-minter · 2017-08-10T18:37:42Z

pkg/template/apis/template/types.go

@@ -108,10 +108,24 @@ type TemplateInstanceSpec struct {
 // TemplateInstanceRequester holds the identity of an agent requesting a
 // template instantiation.
 type TemplateInstanceRequester struct {
-	// Username is the username of the agent requesting a template instantiation.
+	// The name that uniquely identifies this user among all active users.


I think convention here is to start with a lower case character, and I got it wrong previously in this file. Please can you fix up the ones you're changing/adding and we can table the rest for some other time?

I'm not so sure .... look at https://github.com/kubernetes-incubator/service-catalog/pull/939/files ... the names are capitalized for example .... or can you clarify the convention you are referring to

sorry, it's the comments that I'm referring to which should start with lower case. Of course the field names must be capitalised.

jim-minter · 2017-08-10T18:38:10Z

pkg/template/apis/template/v1/types.go

@@ -127,8 +129,28 @@ type TemplateInstanceSpec struct {
 // TemplateInstanceRequester holds the identity of an agent requesting a
 // template instantiation.
 type TemplateInstanceRequester struct {
-	// username is the username of the agent requesting a template instantiation.
-	Username string `json:"username" protobuf:"bytes,1,opt,name=username"`
+	// The name that uniquely identifies this user among all active users.


case (see above)

jim-minter · 2017-08-10T18:41:02Z

pkg/template/apis/template/validation/validation.go

+				if strings.HasPrefix(group, "system:") {
+					continue
+				}
+				for idx, msg := range oapi.GetNameValidationFunc(uservalidation.ValidateGroupName)(group, false) {


@deads2k on what basis do Validate{User,Group}Name reject names containing :? Is there a mechanism by which names containing : may be accepted?

i think it system defined groups can have : and user defined cannot

@gabemontero yes, I suppose so; I'm wondering on what basis we reject system defined users, hence the question to David

@deads2k on what basis do Validate{User,Group}Name reject names containing :? Is there a mechanism by which names containing : may be accepted?

Not for users today. I don't think we've settled how we're going to reconcile with upstream on this. @liggitt @openshift/sig-security

I would simply capture the data from the user info and not validate it here

ok ... I'm reading @liggitt 's comment as a call to remove both the group validation I added, as well as the preexisting username validation.

Of course advise if I've misinterpreted. Thanks.

jim-minter · 2017-08-10T18:41:28Z

pkg/template/apis/template/validation/validation_test.go

@@ -389,6 +430,7 @@ func TestValidateTemplateInstance(t *testing.T) {
 }

 func TestValidateTemplateInstanceUpdate(t *testing.T) {
+


you mean the blank line and not the function, correct?

jim-minter · 2017-08-10T18:44:22Z

pkg/template/registry/templateinstance/strategy.go

-		templateInstance.Spec.Requester = &templateapi.TemplateInstanceRequester{
-			Username: user.GetName(),
+		user, ok := apirequest.UserFrom(ctx)
+		if ok {


if !ok , it's absolutely necessary to bail at this point.

that occurred to me too, but how to do it was not obvious ... PrepareForCreate does not return err or and ErrorList.

I do see that the Context interface has an Err() method and my preliminary searches are finding hits around canceling the context. Perhaps that is what should be done, but I'm still looking for a precise example for guidance. Of course any pointers to save me search time are appreciated.

actually, I think the Validate() method for this same struct catches the error ... checking for ok is probably redundant here (and perhaps is why it wasn't there in the previous version ?)

follow the example at https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/certificates/certificates/strategy.go#L53-L77

ensure that if no user info is present, the fields are empty. the controller should refuse to act on a resource that is missing info about the requesting user (or validate should prevent it from being persisted)

optimistically thinking I'm correct here, I'm removing the check and adding a comment explaining the Validate connection

Follow up question: is it valid for the user name to be empty/anonymous, but have groups set?

is it valid for the user name to be empty/anonymous, but have groups set?

empty != anonymous, and no

ok good thx for the confirm ... per your earlier comment, the controller is refusing to act on the resource missing requesting user info

Alas, in all the various threads here, I forgot the crucial state of affairs wrt all this ... until the upstream service catalog changes arrive where it actually authenticates and the user is properly set, we'll fail on the the actual instantiate as we'll come in with the wrong user.

That's where all the provision/bind request paramater map username setting came into play.

Hence the TSB extended tests failed all over the place once I "ensure that if no user info is present, the fields are empty" in PrepareForCreate

I'll add another //TODO once upstream pull comment sections, put this code in.

It will essentially be like what the PR originally contained. Per @jim-minter 's original question, we still check ok, but don't take any overt action if !ok.

Also, I think @liggitt 's comment #15719 (comment) lines up with my #15719 (comment)

Again, we'll just remote the requester == nil conditional once the upstream SC update arrives and we can depend on user.Info.

jim-minter · 2017-08-10T18:48:10Z

pkg/template/servicebroker/lastoperation.go

+			return api.Forbidden(err)
+		}
+	}
+
 	templateInstance, err := b.templateclient.TemplateInstances(brokerTemplateInstance.Spec.TemplateInstance.Namespace).Get(brokerTemplateInstance.Spec.TemplateInstance.Name, metav1.GetOptions{})


please can you do a replace brokerTemplateInstance.Spec.TemplateInstance.Namespace -> namespace here and wherever else you can in this package?

yep ... can we assume the secret namespaces and template namespaces are the same?

jim-minter · 2017-08-10T18:50:53Z

pkg/template/servicebroker/provision.go

-		}
-	*/
+	// with groups in the user.Info vs. the username only form of auth, we can SAR for get access on template resources
+	if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorization.ResourceAttributes{


I don't think this section can be enabled until the requester-username parameter is dead and gone

actually I think this is the SAR that might have failed until I gave the user the view role on the openshift namespace.

but then it passed when I did

OK, I have confirmed that this SAR will pass when I explicitly use add-role-to-user for the openshift namespace in the extended tests. Removing those calls, and this SAR fails.

I've added comments detailing this around this call, as well as the add-role-to-user calls in the extended tests.

At this point, I think we should leave this as is until the service catalog changes arrive, and it is actually authenticating, and we are using the user.Info from the api.Context instead of the bind/provision request parameter map. At that time, when all these //TODO workaround comments are cleaned up, we remove the add-role-to-user calls in the extended tests, and verify this SAR still passes / fails as appropriate.

bear in mind that merging this as is, then, will mean that the "oc cluster up" approach will be broken for using the TSB until the SC is passing the full user info in, right?

I'm not necessarily saying that's a blocker for merging it, but it might mean we just want to see where the SC is at so we can minimize the hoop jumping as well as being in a temporarily broken state.

on further reflection (and the lack of evidence that SC passing user info is imminent) i'm inclined to say we should comment this out for now w/ another "TODO - uncomment when SC is passing user info headers" (same PR as the others i guess). But since this isn't going to merge for another week (when the next sprint starts) I guess we can wait and see.

Sounds practical / reasonable ... we'll see how next week unfolds and go from there.

jim-minter · 2017-08-10T18:51:45Z

pkg/template/servicebroker/test-scripts/bind.sh

@@ -2,8 +2,9 @@

 . shared.sh

-serviceUUID=${serviceUUID-$(oc get template cakephp-mysql-example -n openshift -o template --template '{{.metadata.uid}}')}
+serviceUUID=${serviceUUID-$(oc get template jenkins-ephemeral -n openshift -o template --template '{{.metadata.uid}}')}


why is this changing?

cakephp-mysql-example is not one of the default templates in the openshift namespace when bringing up via oc cluster up... I picked one that was

please can you reset the change here and export serviceUUID to something else in the oc cluster up script?

reverted with next push

jim-minter · 2017-08-10T18:52:40Z

pkg/template/servicebroker/test-scripts/bind.sh

@@ -16,6 +17,7 @@ curl \
  -X PUT \
  -H 'X-Broker-API-Version: 2.9' \


Please check if sending X-Broker-API-Originating-Identity implies sending an X-Broker-API-Version > 2.9 and change across the board if required

Looks like it is currently targeted for 2.12; hopefully this link renders to the right line in spec.md of the servicebroker pull: https://github.com/openservicebrokerapi/servicebroker/pull/222/files#diff-958e7270f96f5407d7d980f500805b1bR105

good catch / will switch

jim-minter · 2017-08-10T18:53:09Z

pkg/template/servicebroker/test-scripts/provision.sh


+echo "GGM requester is ${requesterUsername}"


remove please

jim-minter · 2017-08-10T18:54:14Z

pkg/template/servicebroker/test-scripts/shared.sh

@@ -6,5 +6,5 @@ bindingUUID=${bindingUUID-dde0226b-ff95-4f9d-af51-2e9ec06b1f02}

 endpoint=${endpoint-https://localhost:8443/brokers/template.openshift.io}
 curlargs=${curlargs--k}
-namespace=${namespace-demo}
+namespace=${namespace-myproject}


please reset

sorry, again, the default of myproject is what is in play when bringing up via oc cluster up ... where is demo coming from ?

I guess this one can stay

jim-minter · 2017-08-10T18:56:28Z

pkg/template/servicebroker/validation.go

 			allErrs = append(allErrs, field.Invalid(field.NewPath("parameters", key), key, fmt.Sprintf("does not match %v", templatevalidation.ParameterNameRegexp)))
 		}
 	}

-	allErrs = append(allErrs, validateParameter(templateapi.RequesterUsernameParameterKey, preq.Parameters[templateapi.RequesterUsernameParameterKey], uservalidation.ValidateUserName)...)


I think it's too early to remove this - not until the parameter has gone

i do some validation in provision.go/bind.go instead ... it concentrated/contained this temporary code at least a little bit

surely it's preferable to concentrate all the validation in one place as it was.

well now I'm concentrating the use of the field as a back up and its validation all in one place

if it's going away either way(as it is) it's probably not worth worrying too much about where it lives in the short term.

jim-minter · 2017-08-10T18:58:36Z

pkg/openservicebroker/server/route.go

@@ -113,7 +164,12 @@ func provision(b api.Broker, req *restful.Request) *api.Response {
 		return api.NewResponse(http.StatusUnprocessableEntity, &api.AsyncRequired, nil)
 	}

-	return b.Provision(instanceID, &preq)
+	u, err := getUser(req)


for sanity, on provision and bind, is it possible to validate that either X-Broker-API-Originating-Identity is set, or the parameter is set, but not both?

return bad req error if both are set with upcoming push

jim-minter · 2017-08-10T18:59:59Z

test/extended/clusterup.sh

@@ -158,6 +158,21 @@ function os::test::extended::clusterup::hostdirs () {
    BASE_DIR="${base_dir}" os::test::extended::clusterup::internal::hostdirs
 }

+# Tests bringing up the service catalog and provisioning a template
+function os::test::extended::clusterup::service_catalog() {


what's the purpose of this? is it WIP?

stems from an IRC comment from @bparees ... mentioned that no ext testing of oc cluster up --service-catalog existed today, and he was thinking I might be able to add some

this is my foray into that

Is it WIP ? .... not sure how much more or less it is compared to the other aspects of the PR, and the pending upstream SC/SB dependencies we have.

I guess the first question is whether this was the type of ext testing @bparees was referring to.

talked to @bparees about this during scrum ... obtained agreement to leave these clusterup.sh changes in

yeah, this was what i had in mind. Just keep in mind that currently nothing is running these tests today and there's not even a /test command that will run them right now. (we need to get one added).

jim-minter · 2017-08-10T19:07:58Z

test/extended/templates/templateservicebroker_e2e.go

@@ -72,6 +75,10 @@ var _ = g.Describe("[templates] templateservicebroker end-to-end test", func() {
 		})
 		o.Expect(err).NotTo(o.HaveOccurred())

+		// give test user access to the openshift project's templates ... now needed with provision's SAR for get on templates (previously commented out)
+		err = cli.AsAdmin().Run("policy").Args("add-role-to-user", "edit", cli.Username(), "-n", "openshift").Execute()


this can't be right - edit shouldn't be required, and view should already be granted. What goes wrong when this is removed?

its been interesting ... I needed to explicitly add view (I had changed this to just view but must not have committed it) when running the extended test locally. If I did not, I got the permission error the comment alluded to.

And when running similar tests from from the command line, I had to given developer view.

But when I run the clusterup.sh extended tests, I did not need this.

OK, I have confirmed that this add-role-to-user for the openshift namespace in the extended tests is needed for the SAR on gets for templates in the provision path to work.. Removing these calls, and that SAR fails.

I've added comments detailing this around this call, as the SAR in question in provision.go.

At this point, I think we should leave this as is until the service catalog changes arrive, and it is actually authenticating, and we are using the user.Info from the api.Context instead of the bind/provision request parameter map. At that time, when all these //TODO workaround comments are cleaned up, we remove the add-role-to-user calls in the extended tests, and verify this SAR still passes / fails as appropriate.

jim-minter · 2017-08-10T19:08:57Z

test/extended/templates/templateservicebroker_security.go

@@ -68,7 +69,15 @@ var _ = g.Describe("[templates] templateservicebroker security test", func() {
 		brokercli = client.NewClient(&http.Client{Transport: &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}}, adminClientConfig.Host+templateapi.ServiceBrokerRoot)

 		viewuser = createUser(cli, "viewuser", bootstrappolicy.ViewRoleName)
+		// give test user access to the openshift project's templates ... now needed with provision's SAR for get on templates (previously commented out)
+		err = cli.AsAdmin().Run("policy").Args("add-role-to-user", "view", viewuser.Name, "-n", "openshift").Execute()


am concerned that none of these should be needed (see above)

understood ... see experience from dev/unit test noted above.

attempted to clarify need in comments here and in comments in provision.go wrt the SAR on getting templates that is now in

jim-minter · 2017-08-10T19:13:58Z

test/extended/templates/templateservicebroker_security.go

@@ -100,32 +109,36 @@ var _ = g.Describe("[templates] templateservicebroker security test", func() {

 	provision := func(username string) error {
 		g.By("provisioning a service")
-		_, err := brokercli.Provision(context.Background(), instanceID, &api.ProvisionRequest{
+		_, err := brokercli.Provision(context.Background(), &user.DefaultInfo{Name: username}, instanceID, &api.ProvisionRequest{


I wonder whether you'll need to include the group "system:authenticated" on all of these - will add a follow-up question

I see, so yeah along these same lines, you should just add the proper groups to the user you're passing, rather than granting the user new permissions. Then we'd also be testing the group-based SAR checks.

jim-minter · 2017-08-10T19:17:42Z

@deads2k @liggitt questions on the security model of passing a user.Info from the service catalog to the broker. Am I right in thinking we trust the catalog to the extent where it can manufacture any user.Info it likes, but that this is effectively intersected with the TSB service account permissions to avoid breakout? Do we expect the SC to pass in the "system:authenticated" group membership, or is there some process of group enrichment that should take place on the TSB side to add in this group membership? Without it, in principle it's not possible to read objects from the openshift namespace, which is why I ask.

gabemontero · 2017-08-19T00:03:26Z

ok, comment in clusterup.sh added, monitoring things for the get templates SAR, squashed the commits

gabemontero · 2017-08-19T22:25:56Z

opened flake #15874 for the extended_conformance_install_update failure

quick reminder, all tests passed last go around; only change was this time around was adding the comment to oc cluster up extended test case script clusterup.sh

gabemontero · 2017-08-23T20:31:12Z

OK, the SAR for getting templates on the openshift namespace when provisioning has been re-commented out, with the //TODO referencing the upstream SC work.

That was the last item from @bparees I believe

@deads2k - there should be no more changes in this PR wrt your move all this stuff around PR

deads2k · 2017-08-24T12:24:46Z

@deads2k - there should be no more changes in this PR wrt your move all this stuff around PR

Thanks, rebased.

gabemontero · 2017-08-28T01:05:09Z

rebase for test/extended/templates/templateinstance_impersonation.go conflict pushed ... @deads2k FYI

bparees · 2017-08-28T01:36:54Z

@openshift/api-review if there are api concerns we'll chase them in a follow up given the dance @deads2k is already doing to get this PR in.

/lgtm
/approve

…oker

gabemontero · 2017-08-28T02:45:16Z

rebase required a regen of the swagger spec as well ... changes from make update pushed

gabemontero · 2017-08-28T02:46:09Z

@bparees please repost the igtm label at your convenience

bparees · 2017-08-28T02:57:08Z

/lgtm

…

On Sun, Aug 27, 2017 at 10:46 PM, Gabe Montero ***@***.***> wrote: @bparees <https://github.com/bparees> please repost the igtm label at your convenience — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15719 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEvl3svIYV6ZU7RYHmEqyW9IfCYdxrWtks5scin6gaJpZM4OyrcO> .

-- Ben Parees | OpenShift

openshift-merge-robot · 2017-08-28T02:57:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, gabemontero

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-bot · 2017-08-28T09:43:40Z

/retest

Please review the full test history for this PR and help us cut down flakes.

deads2k · 2017-08-28T12:43:20Z

/retest

openshift-merge-robot · 2017-08-28T15:12:11Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2017-08-28T16:22:39Z

@gabemontero: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_gce	`93dca0c`	link	`/test extended_conformance_gce`

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.

openshift-merge-robot · 2017-08-28T16:51:38Z

Automatic merge from submit-queue (batch tested with PRs 15719, 15761)

openshift-merge-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 9, 2017

openshift-merge-robot assigned smarterclayton and jim-minter Aug 9, 2017

openshift-merge-robot added the needs-api-review label Aug 9, 2017

gabemontero force-pushed the tsb-userinfo-groups branch 2 times, most recently from ee04154 to 97d2322 Compare August 10, 2017 17:36

openshift-ci-robot unassigned jim-minter Aug 10, 2017

jim-minter reviewed Aug 10, 2017

View reviewed changes

gabemontero force-pushed the tsb-userinfo-groups branch from ee26a26 to 6a619b2 Compare August 23, 2017 20:29

gabemontero force-pushed the tsb-userinfo-groups branch from 6a619b2 to d8a8473 Compare August 23, 2017 21:16

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2017

gabemontero force-pushed the tsb-userinfo-groups branch from d8a8473 to 5151f7b Compare August 28, 2017 01:04

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2017

bparees self-assigned this Aug 28, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2017

Use groups/extra information for authorization in template service br…

93dca0c

…oker

gabemontero force-pushed the tsb-userinfo-groups branch from 5151f7b to 93dca0c Compare August 28, 2017 02:44

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2017

openshift-merge-robot merged commit 7b9b18c into openshift:master Aug 28, 2017

gabemontero deleted the tsb-userinfo-groups branch August 28, 2017 17:39

gabemontero mentioned this pull request Sep 20, 2017

remove user specification via request parameter for TSB #16434

Merged

		@@ -389,6 +430,7 @@ func TestValidateTemplateInstance(t *testing.T) {
		}

		func TestValidateTemplateInstanceUpdate(t *testing.T) {

		@@ -16,6 +17,7 @@ curl \
		-X PUT \
		-H 'X-Broker-API-Version: 2.9' \

Tsb userinfo groups #15719

Tsb userinfo groups #15719

Conversation

gabemontero commented Aug 9, 2017 • edited Loading

gabemontero commented Aug 9, 2017

gabemontero commented Aug 10, 2017

jim-minter commented Aug 10, 2017

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

gabemontero commented Aug 10, 2017

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

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

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

liggitt Aug 10, 2017 • edited Loading

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

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

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

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

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

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

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

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

gabemontero commented Aug 9, 2017 •

edited

Loading

liggitt Aug 10, 2017 •

edited

Loading