Consume authorize_url from Secret #288

maorfr · 2020-01-07T08:25:27Z

This PR intends to take the authorization URL out of the template and into the telemeter-server Secret.

required for observatorium/observatorium#149

ultimately the purpose of this PR is to allow deploying ConfigMaps via upstream!

maorfr · 2020-01-07T08:27:12Z

squat · 2020-01-07T08:31:19Z

Hmm I’m pretty sure I actually remember having had this before and then switching to our current configuration. In any case, the authorization URL is not a secret and putting in there with the actual secret values makes it harder to maintain and debug IMO. Also, once nice thing about having this value outside of a secret is that any time that the argument is changed, the statefulset will be automatically rolled, whereas if the value in the secret changes we have to manually kick the pods.

maorfr · 2020-01-07T08:34:16Z

thanks for your comments @squat!

this is not a secret, but it is non-public information IMO, as it discloses our architecture, even if only a small bit.

regarding kicking the pods - we can create an automation to recycle pods after a secret changed if we decide.

maorfr · 2020-01-07T08:38:02Z

/test e2e-aws

squat · 2020-01-07T08:47:59Z

Adding more automation creates extra complexity rather than using an already simple solution. The information is not “non public”: as long as there is an authorization URL flag at all then it is publicly known that we use an authorization server. The URL says nothing about the architecture other than “this thing that you already know exists is at this URL that you already can not access”. Also, the tollbooth auth flow/protocol is completely reversible from the code contained in this repo.

Please help me understand why this is a desired change.

maorfr · 2020-01-07T09:07:45Z

the url says what is the authorization server. its one thing to know that it exists, and another to know where it is. we just want to add a bit of mystery to the game.

another reason for this change is that we want to allow you to deploy additional resources from upstream, such as ConfigMaps. To allow that, we looked at "what is non-public information" and how we can detect it, to prevent such info from residing in public repos. surprisingly, we found that in the majority of cases, a string starting with https:// is sensitive information. As is this case, in my opinion.

This is the method that we are using at the moment to detect non-public information in public repos, and we plan to enable this check for this repo as well in the near future (https://gitlab.cee.redhat.com/service/app-interface/merge_requests/2554).

This change does not really make a huge difference IMO, so if we can get you on board that would be fantastic. for the record, we will not allow deploying ConfigMaps via this repo unless that app-interface PR is merged, so we have a shared interest here.

what do you think?

squat · 2020-01-07T09:16:32Z

for the record, we will not allow deploying ConfigMaps via this repo unless that app-interface PR is merged, so we have a shared interest here

:/ it doesn’t sound like this is really a choice

we just want to add a bit of mystery to the game.

Any publicly accessible endpoint should always be treated as if everyone already knows it and may try to abuse it. Obscuring the URL is not a security measure IMO and should not be considered to make any progress towards protecting the resource.

squat · 2020-01-09T09:57:14Z

jsonnet/telemeter/server/kubernetes.libsonnet

@@ -135,6 +135,7 @@ local clusterPort = 8082;
      local secret = k.core.v1.secret;

      secret.new(secretName, {
+        authorize_url: std.base64($._config.telemeterServer.authorizeUrl),


With these changes Jsonnet will fail build this because there is no field named authorize_url in the config. How about leaving it as ''?

squat · 2020-01-15T14:25:39Z

/retest

maorfr · 2020-01-16T10:07:10Z

/test e2e-aws

squat

/lgtm

squat · 2020-01-16T17:23:58Z

/approve

openshift-ci-robot · 2020-01-16T17:24:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maorfr, squat

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

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

maorfr · 2020-01-16T18:38:03Z

/test benchmark

maorfr · 2020-01-16T18:38:48Z

/test e2e-aws-upgrade

openshift-bot · 2020-01-16T18:54:11Z

/retest