Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal for saving extracted credentials #768

Merged
merged 4 commits into from
Feb 16, 2018
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions docs/proposals/extracted_credentials_saved_as_secrets.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
## Extracted Credentials Saved As Secrets

Extracted Credentials are currently saved in our etcd for the service broker. This is not desirable for many reasons, but the two biggest are kubernetes already has a built-in way to manage this data, secrets, and when moving to CRDs we don't want to create a resource for extracted credentials.

### Problem Description
The problem is that we should not manage data that is of a sensitive nature if we do not have to. This proposal is limited in scope and only interested in how we save the extracted credentials. It is worth noting that we should eventually be better about how we transmit this data to APBs.

In the secret, we will save the data in the following format.
```yaml
data:
DB_PASSWORD: <GOB_ENCODED_VALUE>
DB_USERNAME: <GOB_ENCODED_VALUE>
....
apiVersion: v1
kind: Secret
metadata:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be a good to store the ClusterServiceClass externalName (i.e the human readable value) and the action in a label on the secret. The purpose of this would be to assist debugging after the fact. It is likely that over a long period of time or during development, there may be many secrets created. If I am trying to find the secrets created by a particular ServiceClass it would be nice to be able to use the label selector
Something like:

 metadata:
  labels:
    clusterServiceClassName/ apbName : "dh-service-apb"
    action:"provision/bind"

Should allow me to do

oc get secrets -l apbName=dh-myservice-apb -l action=bind

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds good.

I will add the ability to pass labels to the runtime package functions and to the clients package functions.

name: <Service/Binding id>
namespace: <Namespace for the broker>
labels:
<labels provided>
```

[Gob encoding](https://godoc.org/encoding/gob) will allow us to save arbitrary data in the secret for a key. The secrets keys will look rational to a user who looks at the created secret. This user would need permissions to see the secret, but if someone is looking at the secret making it obvious what data is in there will be helpful.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this pkg so excuse me if these should be obvious:

  • Correct me if I'm wrong, but this is not required to accomplish what we're doing? What is the benefit, an independent storage format?
  • We've talked about encrypting credentials, is that something that can be done easily here? (Or is that unnecessary thanks to our usage of secrets?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, is base64 encryption enough? Is that what the rest of openshift deems secure enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a map[string]interface{}. The secrets data needs to be a[]byte transforming arbitrary data, interface{} to a byte array is tricky. This is one of the options that I found how to do this. I thought it would be helpful because the how to decode the value is included in the encoding so that even outside people could just use the built-in gob decoder for golang and decode the value.

With that being said, we could just marshal the entire map[string]interface to json and only have 1 key in the secret called credentials. I didn't think this was as good because then when a user who is debugging stuff has to know how we store it and the above way they will just have encoded values but the keys for the extracted credentials will be at the top level of the secret.

This is not encrypting the secret, but secrets are well understood and as far as I know, the correct place to put this data without encrypting it. So I was not planning on adding encryption here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on both accounts. Thanks @shawn-hurley.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allowed in secrets? Will this make the secrets unusable outside of the broker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most likely that is the case. If that becomes a problem I assume that we could revisit this then. Do you have a use case in mind?


The functions for saving and retrieving will be in the `clients` package. This means the callers will be required to use the underlying extracted credentials type `map[string]interface{}` because we do not want a circular dependency between `apb` package and `clients` package.

We will interact with the secrets from the namespace defined in the configuration by the `openshift.namespace` value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurs to me reading this that we really should remove references to openshift from our configuration if we're looking to be cluster agnostic. Not at all in the scope of this proposal, just thought I'd call it out. We probably need an issue to track it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to change this after starting the broker? Wondering about recovering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you asking about changing on restart?

If you change this value on a restart and start to run the broker with a new namespace for resources yeah that would be a problem. This will also be a bigger problem once CRDs are a thing too. CRDs are going to be namespaced so they only live in that namespace. If you change this value then IMO you're saying to run a brand new broker.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone is changing openshift.namespace while the broker is down, that is opening a can of worms that is outside the scope of this PR. Correct me if I'm wrong, but recovery is designed to allow the broker to restart and pick up outstanding operations after having being bounced by k8s. The config shouldn't have changed in that timeframe.


The APB package will now be required to do all CRUD operations for extracted credentials. The APB package will expose a single retrieve extracted credentials method, that will take a UUID (either service instance id or binding instance id) and returns an `apb` package extracted credentials object.

Runtime package should be used to encapsulate the `clients` package calls. This will mean we have a default function for CRUD operations with extracted credentials. These default functions will be set to function vars at init of runtime. The function vars are then overrideable in the future. example:
```go
var SaveExtractedCredentials SaveExtractedCredentialsFunc
...

func saveExtractedCredentials(...) {
...
k8scli, err := clients.Kubernetes()
if err != nil {
...
}
k8scli.Clients.CoreV1().Secrets()...

}

init {
SaveExtractedCredentials = saveExtractedCredentials
....
}
```


### Work Items
- [ ] Add kubernetes client methods to interact with extracted credentials in the [namespace](https://github.com/openshift/ansible-service-broker/blob/master/docs/config.md#openshift-configuration).
- [ ] Add runtime methods for interacting with extracted credentials. These methods should be overridable.
- [ ] Remove all dao implementation and interface methods regarding extracted credentials.
- [ ] Remove all instances of interacting with dao extracted credentials in the `broker` package. Add back call to APB package to get extracted credentials when needed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there also places where the credentials are deleted. Should we call out in the proposal that the credentials should be cleaned up if a service instance is deprovisioned or a binding deleted?

Copy link
Contributor Author

@shawn-hurley shawn-hurley Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those actions are already taken currently and are covered by removing all the interacting work items and the update APB package to take the correct actions. I don't feel that the delete is different then update them on an update or create them on provision or binding. Just my opinion if it helps people understand what the PR is doing I will add.

- [ ] Update APB package to create/save/delete extracted credentials for the correct actions. this should call the correct `runtime` package methods.
- [ ] Add exposed method on APB that will retrieve the extracted credentials.