added WithTLS and WithSecurityBuilder #20
Conversation
clients/client_builder.go
Outdated
| @@ -45,8 +46,14 @@ func (b *ClientBuilder) WithPort(port int) *ClientBuilder { | |||
| return b | |||
| } | |||
|
|
|||
| // WithSecurity sets the TLS configuration of the client. | |||
| func (b *ClientBuilder) WithSecurity(security SecurityConfig) *ClientBuilder { | |||
There was a problem hiding this comment.
So removing this is a breaking change and we should avoid it. The goal was to make this a patch only so you'd need to keep this method and potentially mark it as deprecated.
clients/security_builder.go
Outdated
| @@ -18,7 +18,7 @@ import "crypto/tls" | |||
|
|
|||
| // SecurityBuilder provides an builder for client tls.Config instances. | |||
| type SecurityBuilder struct { | |||
| config SecurityConfig | |||
| Config SecurityConfig | |||
There was a problem hiding this comment.
Why are you opening this up to public access?
| @@ -34,14 +34,24 @@ type ClientConfig struct { | |||
|
|
|||
| // Security defines the TLS configuration used by the client. | |||
| Security SecurityConfig `json:"security" mapstructure:"security" yaml:"security"` | |||
|
|
|||
| // config is the tls config passed by ClientBuilder.WithTLS. | |||
| config *tls.Config | |||
There was a problem hiding this comment.
Another way to do this a bit cleaner than tracking a few things is to create a SecurityConfig from a tls.Config. It's not the easiest but it might make the behavior easier to understand when using this.
There was a problem hiding this comment.
Would that be cleaner? It's definitely a bit messy to track the tls.Config, but in order to create a SecurityConfig wouldn't it involve essentially doing the inverse of the SecurityConfig.Build() method, which would would involve re-encoding the certs, only to be re-built and unencoded later? Unless I'm completely misunderstanding this.
There was a problem hiding this comment.
Yes, you'd add a method that encodes it to base64 a la https://github.com/DecipherNow/acert/blob/master/stores/filesystem/identity_store.go#L128. It's a bit of indirection but will make maintenance easier in the long run.
There was a problem hiding this comment.
Makes sense - I'll get started on that.
documented precedence order quick fixes
critterjohnson
force-pushed
the
client_builder_changes
branch
from
2424156 to
5ee043b
Compare
| // ? is the config.PrivateKey what needs to be converted into the SecurityConfig.Key? | ||
|
|
||
| // config.RootCAs is a x509.CertPool, needs to become SecurityConfig.Authorities | ||
| // ! x509.CertPool has no method to export the certs |
There was a problem hiding this comment.
How would I convert config.RootCAs into a url if there's no exported fields or methods that allow access to the certs?
| // parse each cert into an x509.Certificate so encoding can handle them | ||
| x509Certs := make([]*x509.Certificate, len(config.Certificates[0].Certificate)) | ||
| var err error | ||
| for index, certificate := range config.Certificates[0].Certificate { |
There was a problem hiding this comment.
The tls.Config could potentially have multiple cert chains - how should I handle that?
Josh - is this what you were looking for? I added WithTLS and WithSecurityBuilder, but I had to change a couple of things.