Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Broker TLS authentication #1064

Closed
pmorie opened this issue Jul 26, 2017 · 6 comments · Fixed by #1112
Closed

Broker TLS authentication #1064

pmorie opened this issue Jul 26, 2017 · 6 comments · Fixed by #1112
Labels
Milestone

Comments

@pmorie
Copy link
Contributor

pmorie commented Jul 26, 2017

There should be an API surface to govern the TLS settings for authenticating the broker to the catalog controller. Currently we set the insecure field on the OSB client in controller.go - this is not sufficient for production use. Using non-root-signed certificates for servers is very common, so we cannot rely on using system root certificates.

There is precedent for authenticating external servers in the apiregistration API group in the aggregator:

From types.go:

type APIServiceSpec struct {
  // other fields omitted

  // InsecureSkipTLSVerify disables TLS certificate verification when communicating with this server.
  // This is strongly discouraged.  You should use the CABundle instead.
  InsecureSkipTLSVerify bool
  // CABundle is a PEM encoded CA bundle which will be used to validate an API server's serving certificate.
  CABundle []byte
}

I think the same combination of fields would work for us, and propose we add them to BrokerSpec:

type BrokerSpec struct {
  // other fields omitted

  // InsecureSkipTLSVerify disables TLS certificate verification when communicating with this server.
  // This is strongly discouraged.  You should use the CABundle instead.
  InsecureSkipTLSVerify bool
  // CABundle is a PEM encoded CA bundle which will be used to validate an API server's serving certificate.
  CABundle []byte
}

Note, the broker client already supports injecting the TLS configuration used by the http client.

@pmorie pmorie added the api label Jul 26, 2017
@pmorie pmorie added this to the 0.1.0 milestone Jul 26, 2017
@pmorie pmorie changed the title Proposal for Broker TLS authentication Broker TLS authentication Jul 26, 2017
@pmorie
Copy link
Contributor Author

pmorie commented Jul 26, 2017

cc @deads2k @liggitt

@MHBauer
Copy link
Contributor

MHBauer commented Jul 26, 2017

needs a slash.
/cc @DirectXMan12 @deads2k @liggitt

I don't know which part of tls is used to do the whole mutual authentication thing, but does that get involved here with the ca bundle and or the right part of the bundle on one side of the connection or the other?

As I understand it this make sense for the connection from the catalog to the broker, with the broker serving a self-signed or otherwise not-root-signed certificate.

Do we care about the case where the broker is verifying that this catalog client is a valid client and is it part of this same work?

What can we reuse from upstream? Does work need to be done upstream to support this reuse case?

@MHBauer
Copy link
Contributor

MHBauer commented Jul 26, 2017

/cc @DirectXMan12 @deads2k @liggitt
and maybe needs to be the first line?

@deads2k
Copy link

deads2k commented Jul 27, 2017

I don't know which part of tls is used to do the whole mutual authentication thing, but does that get involved here with the ca bundle and or the right part of the bundle on one side of the connection or the other?

What is being proposed here is about the client confirming the server's identity and this API looks good for that purpose.

If the broker server wants to confirm client identity using TLS, then the client (the controller in this case) will need a client cert/key pair which is a secret (shouldn't be on the broker object to avoid making it a confidential resource) and the broker server itself is going to need a different ca bundle for confirming client identities.

Do we care about the case where the broker is verifying that this catalog client is a valid client and is it part of this same work?

That should be made possible, but it wouldn't be done using this API and since it involves having secret information you're probably going to want a reference to a secret. Having confidential resources is very painful, so you should try to avoid creating new ones.

@pmorie
Copy link
Contributor Author

pmorie commented Aug 1, 2017

That should be made possible, but it wouldn't be done using this API and since it involves having secret information you're probably going to want a reference to a secret. Having confidential resources is very painful, so you should try to avoid creating new ones.

This is possible to implement in a backward-compatible way as a new option in the API described in #1053

@pmorie
Copy link
Contributor Author

pmorie commented Aug 2, 2017

In the July 26th design meeting we had consensus on this approach; @staebler to implement.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants