Bug 1503289 - Move registry credentials to a secret #502

dymurray · 2017-10-18T16:34:03Z

Move registry credentials to a secret and consumed from a different file

This PR moves the registry credentials into a secret so that it is not visible in plaintext from the configmap.

Changes proposed in this pull request

Move registry credentials into a new file at /etc/ansible-service-broker/registry-auth.yaml
Create a secret out of base64 encoded credentials and inject them as environment variables to be consumed by the broker
Broker creates /etc/ansible-service-broker/registry-auth.yaml in entrypoint script
Updates all prep scripts to handle new behavior

IMPORTANT:
You now need your Dockerhub environment variables to be base64 or else the secret will fail to be created. Be sure to update catasb or the template vars.

jmrodri · 2017-10-18T16:47:11Z

This doesn't seem to allow me to specify multiple dockerhub credentials. With our current configuration I can set the broker to talk to dockerhub for ansibleplaybookbundle AND jmrodri orgs with two different logins.

dymurray · 2017-10-18T16:51:33Z

@jmrodri initially I had the credentials in the auth file stored by name i.e.:

---
- name: dh
  user: <user>
  pass: <pass>

And then was storing the user/pass based on the name. There is a good reason why I didn't continue down that approach because working with secrets around independent registries just didn't seem like a scalable way to continue.

Since we are working to remove the credentials completely I think it makes sense to consolidate the credentials based on registry type. This way you can declare rhcc creds (not needed currently), isv registry creds (will become important), without redeclaring for every different adapter.

dymurray · 2017-10-18T16:53:49Z

@jmrodri Is it a valid use case to want to use two different credentials for two dockerhub adapters? It currently doesn't matter which credentials you use as they are just used to obtain a bearer token that allows us to search the registries index. One credential would work for any dockerhub org. The only scenario I could see that not being true would be private organizations but it would just require the most elevated credentials.

jmrodri

I don't like the extra "config" approach.

jmrodri · 2017-10-18T16:56:43Z

build/entrypoint.sh

+  pass: ${DOCKERHUB_PASS}
+EOF
+
+asbd -c $BROKER_CONFIG --registryauth=$REGISTRY_AUTH $FLAGS


I prefer NOT to have this as a separate option. Why can't the admin specify the location of the secret in the registry config section?

Something like auth: TYPE|NAME where TYPE can be secret or file. Then NAME can be the name of the secret OR the fully qualified filename: /etc/ansible-service-broker/dockerhub-auth

registry: - type: dockerhub name: docker url: https://registry.hub.docker.com auth: TYPE|NAME org: DOCKERHUB_ORG fail_on_error: false - type: rhcc name: rhcc url: registry.access.stage.redhat.com fail_on_error: true white_list: - "^legitimate.*-apb$" # will allow all the APBs to be included. You must have at least 1 white # list to retrieve APBs and this is the most permissive - ".*-apb$" black_list: - "malicious.*-apb$" - "^specific-blacklist-apb$" dao:

OR we could use

auth_type: secret|file auth_name: NAME_OF_SECRET | FQFN

This gives us the flexibility to know how to read different ways. File makes it easier to run locally. But secret might make it easier in OpenShift by not needing a mount. Just go read the secret.

OR we could reuse the auth format I did for basic auth and you might even be able to use the same structs (maybe not entire sure).

+1, you're right no reason to make this a separate config. I agree with you. I think I pidgeonholed myself there because I was initially mounting the secret as a volume.

jmrodri · 2017-10-18T16:57:22Z

etc/example-registry-auth.yaml

+---
+- type: dockerhub
+  user: DOCKERHUB_USER
+  pass: DOCKERHUB_PASS


This looks like it supports multiples by being an array but how do I know which registry this is for?

jmrodri · 2017-10-18T16:57:56Z

pkg/app/app.go

@@ -150,7 +150,7 @@ func CreateApp() App {

 	// TODO: Let's take all these validations and delegate them to the client
 	// pkg.
-	if app.config, err = CreateConfig(app.args.ConfigFile); err != nil {
+	if app.config, err = CreateConfig(app.args.ConfigFile, app.args.RegistryAuthFile); err != nil {


Like I mentioned above, I would prefer specifying the auth stuff as a pointer to some other location still in the main registry config.

jmrodri · 2017-10-18T16:59:08Z

templates/deploy-ansible-service-broker.template.yaml

+    username: ${DOCKERHUB_USER}
+    password: ${DOCKERHUB_PASS}
+
+- apiVersion: v1


The secret is still ok, but instead of passing it as a parameter on the cli to the broker, it should be in the registry section of the config.

jmrodri · 2017-10-18T17:14:54Z

We could support file in the broker as an initial cut, then just create that file from a secret. But it would be nice to read a secret directly.

shawn-hurley · 2017-10-18T17:24:18Z

@dymurray @jmrodri I see a case where I have two private registries with two different "service accounts" that have access to one or the other.

dymurray · 2017-10-18T19:08:20Z

Thanks for the feedback, I'm pretty much completely refactoring this to fit Jesus suggestions.

shawn-hurley · 2017-10-19T20:00:04Z

etc/generated_registry_auth.yaml

+---
+- type: dockerhub
+  user: dymurray
+  pass: qci-test


Probably want to ignore the generated file

Oops, this shouldn't be there.

dymurray · 2017-10-19T20:06:04Z

pkg/app/config.go

+			config.Registry[regCount].Pass = regCreds.Password
+
+		} else {
+			// ERROR


Should we still allow the user to specify the user/pass in the config and use them here? This way we don't break old brokers and the real motivation for moving to a secret was for when we are running in openshift.

I also wonder if we support environment variables because that is one way to use secrets?
should be not super hard?

@shawn-hurley that was how I previously implemented this before the refactoring :). That worked fine but required entrypoint changes. Jesus' brought up some good feedback that it would be cleaner to read the secret from the broker itself. We also control the template so its arbitrary which one we pick.

@dymurray If you are saying allow the user to use the old configuration values if they want, that's ok with me. If someone chooses to put a password in a config it is up to them.

jmrodri · 2017-10-19T22:54:22Z

make lint failed:

pkg/app/config.go:50:6: exported type RegCreds should have comment or be unexported
Found 1 lint suggestions; failing.
make: *** [Makefile:25: lint] Error 1

jmrodri · 2017-10-19T22:55:43Z

I also saw you updated config.go but there are no change to config_test.go.

jmrodri

Some comments

jmrodri · 2017-10-19T22:41:58Z

build/entrypoint.sh

@@ -1,5 +1,4 @@
 #!/usr/bin/env bash
-


Remove this change from this patch. It just adds noise and isn't necessary.

jmrodri · 2017-10-19T22:44:04Z

pkg/app/config.go

+			config.Registry[regCount].Pass = regCreds.Password
+
+		} else {
+			// ERROR


@dymurray If you are saying allow the user to use the old configuration values if they want, that's ok with me. If someone chooses to put a password in a config it is up to them.

jmrodri · 2017-10-19T22:47:14Z

pkg/app/config.go

@@ -103,3 +147,24 @@ func validateConfig(c Config) error {
 	}
 	return nil
 }
+
+// Returns the data inside of a given secret
+func getSecretData(secretName, namespace string) (map[string][]byte, error) {


I thought we already had a method to get secrets. I seem to recall a secrets.go that @fabianvf added in a previous patch. Seems that would be useful. If not this seems useful in general to me.

Yeah we did have a method but it was being used to get secrets associated with a specific APB. It also required me to initialize a secrets cache in order to use those methods. I'm not against making this globally available and putting it in a utils package.

jmrodri · 2017-10-19T22:48:25Z

scripts/prep_local_devel_env.sh

@@ -58,7 +58,8 @@ fi


 # Determine the name of the secret which has the 'asb' service account info
-BROKER_SVC_ACCT_SECRET_NAME=`kubectl get serviceaccount asb -n ansible-service-broker -o jsonpath='{.secrets[0].name}'`
+BROKER_SVC_ACCT_SECRET_NAME=`kubectl get serviceaccount asb -n ansible-service-broker -o yaml | grep -Po asb-token-[a-z0-9]*`


why is this being changed in this PR?

jmrodri · 2017-10-19T22:48:54Z

templates/deploy-ansible-service-broker.template.yaml

-          user: "${DOCKERHUB_USER}"
-          pass: "${DOCKERHUB_PASS}"
+          auth_type: "${REGISTRY_AUTH_TYPE}"
+          auth_name: "${REGISTRY_SECRET_NAME}"


+1 I like it.

jmrodri · 2017-10-19T22:50:54Z

pkg/app/config.go

 	"io/ioutil"
+	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"


I find it weird to have this imported into the config. Seems that the thing that uses this should be in a util package or part of the clients package directly. @shawn-hurley @eriknelson thoughts?

This would be the getSecretData method so I'm fine with moving that out to a util package.

shawn-hurley

@jmrodri @dymurray Should we consider making the secret data base64 encoded?

shawn-hurley · 2017-10-23T18:19:11Z

pkg/registries/adapters/dockerhub_adapter.go

+	var err error
+	var req *http.Request
+	if r.Config.User == "" {
+		r.Log.Debugf("hereeee")


Probably should remove this debug statement, my bad @dymurray.

dymurray · 2017-10-24T19:09:23Z

@shawn-hurley The secret data is base64 encoded. Right now I'm expecting the user to set the user/pass creds already base64 encoded much like we already do when passing in broker user/pass.

dymurray · 2017-10-26T15:27:23Z

fusor/catasb#172

rthallisey · 2017-10-26T20:41:38Z

(5) Update Configuration for ASB Deployment to consume a Secret for Dockerhub credentials [asb]

rthallisey · 2017-10-26T20:48:05Z

pkg/app/config.go

+			if dat, err = ioutil.ReadFile(reg.AuthName); err != nil {
+				return Config{}, err
+			}
+			if err = yaml.Unmarshal(dat, &regCred); err != nil {


The logging above here is really good. We could improve the logging here by adding a message about what failed. Could be something like 'missing or invalid username/password'.

rthallisey · 2017-10-26T20:50:02Z

pkg/registries/registry.go

-	Name   string
-	Images []string
+	URL      string
+	AuthType string `yaml:"auth_type"`


Can you put a comment about exactly what AuthType and AuthName are.

rthallisey · 2017-10-26T20:51:21Z

templates/deploy-ansible-service-broker.template.yaml

+- description: Dockerhub user password
+  displayname: Dockerhub user password
+  name: DOCKERHUB_PASS
+  value: Y2hhbmdlbWUK


Is this a real docker user and pw?

This is "changeme/changeme" base64 encoded so I figured it was safe. I can make it an empty string too.

@dymurray now that we have the ability to do empty username/password I don't see why we wouldn't do that?

rthallisey · 2017-10-26T20:56:08Z

templates/deploy-ansible-service-broker.template.yaml

@@ -189,6 +189,15 @@ objects:
    password: ${BROKER_PASS}


Do we need any changes in deploy-local-dev-changes.yaml for the local broker?

I don't believe so because that template doesn't include the broker config.

I don't believe we should go through the trouble of creating an empty secret for the user every time since it won't be used unless someone is going through the trouble of configuring a private registry. In that case they can configure it themselves.

dymurray · 2017-10-27T13:23:49Z

@rthallisey Thanks updated!

jmrodri

Some more enhancements. Overall I like this solution better.

jmrodri · 2017-10-28T00:57:01Z

pkg/app/config.go

 	"io/ioutil"
 	"os"
-
-	yaml "gopkg.in/yaml.v2"


goimports seems to move this back, why was yaml moved above io/ioutil? I'd move it back since it doesn't add any value to moving it.

jmrodri · 2017-10-28T01:31:23Z

pkg/app/config.go

@@ -74,6 +82,34 @@ func CreateConfig(configFile string) (Config, error) {
 		config.Openshift.Namespace = string(dat)
 	}

+	for regCount, reg := range config.Registry {
+		if reg.AuthType == "secret" {


I'd go with a switch statement, then we can add other types as we want.

for regCount, reg := range config.Registry { var username, password string var err error switch reg.AuthType { case "secret": username, password, err = readSecret(reg.AuthName, config.Openshift.Namespace) if err != nil { return Config{}, err } case "file": username, password, err = readFile(reg.AuthName) if err != nil { return Config{}, err } default: return Config{}, fmt.Errorf("Invalid authType: %s", reg.AuthType) } config.Registry[regCount].User = username config.Registry[regCount].Pass = password }

The supporting methods:

func readSecret(authname string, namespace string) (string, string, error) { data, err := clients.GetSecretData(authname, namespace) if err != nil { return "", "", fmt.Errorf("Failed to find Dockerhub credentials in secret: %s", authname) } var username = strings.TrimSpace(string(data["username"])) var password = strings.TrimSpace(string(data["password"])) if username == "" || password == "" { return username, password, fmt.Errorf("Secret: %s did not contain username/password credentials", authname) } return username, password, nil } func readFile(authname string) (string, string, error) { regCred := regCreds{} data, err := ioutil.ReadFile(authname) if err != nil { return "", "", fmt.Errorf("Failed to read registry credentials from file: %s", authname) } if err = yaml.Unmarshal(data, &regCred); err != nil { return "", "", fmt.Errorf("Failed to unmarshal registry credentials from file: %s", authname) } return regCred.Username, regCred.Password, nil }

jmrodri · 2017-10-28T01:31:57Z

pkg/app/config_test.go

@@ -44,6 +44,8 @@ func TestCreateConfig(t *testing.T) {
 		"mismatch whitelist entry")
 	ft.AssertEqual(t, config.Registry[1].BlackList[1], "^specific-blacklist-apb$",
 		"mismatch blacklist entry")
+	ft.AssertEqual(t, config.Registry[2].User, "", "registry user not empty")
+	ft.AssertEqual(t, config.Registry[2].Pass, "", "registry pass not empty")


jmrodri · 2017-10-28T01:50:35Z

pkg/app/testdata/test-config-admin.yaml

@@ -3,12 +3,14 @@ registry:
  - type: dockerhub
    name: docker
    url: https://registry.hub.docker.com
+    auth_type: config


is config a valid auth_type? If so, then my switch suggestion needs another case for config.

Yes it is, to simply declare user/pass in the config. I have updated the test cases and switch statement to reflect that.

jmrodri · 2017-10-28T01:54:46Z

pkg/app/testdata/test-config.yaml

@@ -16,6 +16,11 @@ registry:
    black_list:
      - "malicious.*-apb$"
      - "^specific-blacklist-apb$"
+  - type: dockerhub
+    name: docker2
+    url: https://registry.hub.docker.com


So empty auth_type/auth_name is also valid? If so, maybe the switch isn't the best then though it would be obvious if the switch was on the following:

switch reg.AuthType: case "secret": // do secret stuff case "file": // do file stuff case "config": // do config stuff case "": // do empty stuff default: // error case since this is an authtype we do not recognize

jmrodri · 2017-10-28T01:56:38Z

pkg/clients/kubernetes.go

+	}
+	var ret = make(map[string][]byte)
+
+	secretData, err := k8scli.CoreV1().Secrets(namespace).Get(secretName, meta_v1.GetOptions{})


I'd put the line break before the var ret and remove the one after it.

BEFORE:

} var ret = make(map[string][]byte) secretData, err := k8scli.CoreV1().Secrets(namespace).Get(secretName, meta_v1.GetOptions{})

AFTER:

} var ret = make(map[string][]byte) secretData, err := k8scli.CoreV1().Secrets(namespace).Get(secretName, meta_v1.GetOptions{})

jmrodri · 2017-10-28T01:57:17Z

pkg/clients/kubernetes.go

+		log.Error("Unable to load secret '%s' from namespace '%s'", secretName, namespace)
+		return ret, nil
+	}
+	log.Debug("Found secret with name %v\n", secretName)


I think this should be log.Debugf

jmrodri · 2017-10-28T02:01:58Z

pkg/clients/kubernetes.go

+
+	ret = secretData.Data
+
+	return ret, nil


feels weird to assign a value to ret just to return it on the next line, I'd expected: return secretData.Data I'd get rid of ret entirely. Because you're creating an array that you ultimately throwaway on every successful call. I'd rather create an array only during the error condition.

// GetSecretData - Returns the data inside of a given secret func GetSecretData(secretName, namespace string) (map[string][]byte, error) { var log logging.Logger k8scli, err := Kubernetes(&log) if err != nil { return nil, err } secretData, err := k8scli.CoreV1().Secrets(namespace).Get(secretName, meta_v1.GetOptions{}) if err != nil { log.Errorf("Unable to load secret '%s' from namespace '%s'", secretName, namespace) return make(map[string][]byte), nil } log.Debugf("Found secret with name %v\n", secretName) return secretData.Data, nil

jmrodri · 2017-10-28T02:03:31Z

pkg/registries/adapters/dockerhub_adapter.go

+		if err != nil {
+			return "", err
+		}
+		req.SetBasicAuth(r.Config.User, r.Config.Pass)


these changes look okay 👍

jmrodri · 2017-10-28T02:05:27Z

pkg/registries/registry.go

@@ -60,6 +66,11 @@ func (c Config) Validate() bool {
 	if c.Name == "" {
 		return false
 	}
+	if c.AuthType == "file" || c.AuthType == "secret" {


do we need to validate the AuthType at all? i.e. if I enter "foobarauth" what will happen?

… a different file

dymurray · 2017-10-30T14:41:44Z

Valid options for registry config auth_type are:

secret
file
config
``

If set to ``, then we are expecting that the registry does NOT require credentials.

If set to secret, or file then we expect auth_name to be set to either the secret name or the file location.

If set to config then we expect user/pass to be declared in the config.

* Bug 1503289 - Move registry credentials to a secret and consumed from a different file * Linting fix * Refactored to read directly from the secret * More refactoring * Remove stale files * Updates * Moved getSecretData method to kube client * Update comment for public function * Better error returns * Removed Dockerhub user/pass requirement * Update testdata to fix tests * Updates for debug statements and generated config * Update CI * Update more tests * Logging updates and additional comments * Updated config error validation

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 18, 2017

jmrodri suggested changes Oct 18, 2017

View reviewed changes

dymurray changed the title ~~Bug 1503289 - Move registry credentials to a secret and consumed from…~~ [WIP] Bug 1503289 - Move registry credentials to a secret and consumed from… Oct 18, 2017

openshift-ci-robot added the do-not-merge/work-in-progress label Oct 18, 2017

shawn-hurley reviewed Oct 19, 2017

View reviewed changes

dymurray commented Oct 19, 2017

View reviewed changes

jmrodri suggested changes Oct 19, 2017

View reviewed changes

shawn-hurley reviewed Oct 23, 2017

View reviewed changes

dymurray changed the title ~~[WIP] Bug 1503289 - Move registry credentials to a secret and consumed from…~~ Bug 1503289 - Move registry credentials to a secret and consumed from… Oct 26, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress label Oct 26, 2017

dymurray added the needs-review label Oct 26, 2017

dymurray force-pushed the regcreds branch from a5c28c6 to 782352b Compare October 26, 2017 19:29

rthallisey suggested changes Oct 26, 2017

View reviewed changes

jmrodri changed the title ~~Bug 1503289 - Move registry credentials to a secret and consumed from…~~ Bug 1503289 - Move registry credentials to a secret Oct 28, 2017

jmrodri suggested changes Oct 28, 2017

View reviewed changes

dymurray added 3 commits October 30, 2017 09:43

Bug 1503289 - Move registry credentials to a secret and consumed from…

60afafb

… a different file

Linting fix

93bd183

Refactored to read directly from the secret

a9067d2

dymurray added 13 commits October 30, 2017 09:43

More refactoring

b0714fc

Remove stale files

0309017

Updates

8e8617b

Moved getSecretData method to kube client

c5dbf0c

Update comment for public function

27cb569

Better error returns

ff26ead

Removed Dockerhub user/pass requirement

1f650d2

Update testdata to fix tests

d794501

Updates for debug statements and generated config

6d8ab5a

Update CI

ed5face

Update more tests

6f4e39c

Logging updates and additional comments

fa43efd

Updated config error validation

a19973d

dymurray force-pushed the regcreds branch from fab6997 to a19973d Compare October 30, 2017 14:33

shawn-hurley approved these changes Oct 30, 2017

View reviewed changes

jmrodri approved these changes Oct 30, 2017

View reviewed changes

rthallisey approved these changes Oct 30, 2017

View reviewed changes

rthallisey merged commit d083f9a into openshift:master Oct 30, 2017

shawn-hurley mentioned this pull request Jan 10, 2018

Bug 1533208 - Re adding registry auth as secrets and files #629

Merged

Bug 1503289 - Move registry credentials to a secret #502

Bug 1503289 - Move registry credentials to a secret #502

Conversation

dymurray commented Oct 18, 2017 • edited by jmrodri

jmrodri commented Oct 18, 2017

dymurray commented Oct 18, 2017

dymurray commented Oct 18, 2017 • edited

jmrodri left a 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

jmrodri commented Oct 18, 2017

shawn-hurley commented Oct 18, 2017

dymurray commented Oct 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dymurray Oct 19, 2017 • edited

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

jmrodri commented Oct 19, 2017

jmrodri commented Oct 19, 2017

jmrodri left a 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

shawn-hurley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dymurray commented Oct 24, 2017

dymurray commented Oct 26, 2017

rthallisey commented Oct 26, 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

dymurray Oct 27, 2017 • edited

Choose a reason for hiding this comment

dymurray commented Oct 27, 2017

jmrodri left a 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

dymurray commented Oct 30, 2017

dymurray commented Oct 18, 2017 •

edited by jmrodri

dymurray commented Oct 18, 2017 •

edited

dymurray Oct 19, 2017 •

edited

dymurray Oct 27, 2017 •

edited