Enable client cert authentication for clients #1
Conversation
|
It probably should be a SAR, but then that requires the proxy client to be a SAR checker. maybe it should be. @liggitt |
http.go
Outdated
| @@ -70,6 +70,10 @@ func (s *Server) ServeHTTPS() { | |||
| log.Fatalf("FATAL: loading tls config (%s, %s) failed - %s", s.Opts.TLSCertFile, s.Opts.TLSKeyFile, err) | |||
| } | |||
|
|
|||
| if len(s.Opts.TLSClientCAFiles) > 0 { | |||
| config.ClientAuth = tls.RequestClientCert | |||
There was a problem hiding this comment.
you actually need to build a cert pool from the CA files and set it as config.ClientCAs
There was a problem hiding this comment.
also, be aware that simply requesting a client certificate causes Safari (and IE in some CORS modes) lots of grief. Wondering if we should start a separate port for this.
There was a problem hiding this comment.
Hrm - I did not have to do that in order to make this work...
There was a problem hiding this comment.
I'm fairly sure omitting the ClientCAs causes TLS not to verify.
Separate port wouldn't help because the router wouldn't expose it.
There was a problem hiding this comment.
I guess internally you could use that port if you wanted. Seems complex.
There was a problem hiding this comment.
Actually I think coincidentally we get the best of both worlds. Router doesn't allow client, so you don't get prompted. Internal does.
There was a problem hiding this comment.
including ClientCAs scopes the request ("please send me a client cert signed by X"). omitting it causes client cert prompts in lots of browsers, even if none of the client certs they have available would be acceptable. you should definitely set it to the pool you are going to use to verify.
There was a problem hiding this comment.
Router doesn't allow client
TLS passthrough would, right?
providers/openshift.go
Outdated
| } | ||
| return func(cert *x509.Certificate) (*SessionState, error) { | ||
| if cn == cert.Subject.CommonName { | ||
| return &SessionState{User: cert.Subject.CommonName, Email: cert.Subject.CommonName + "@cluster.local"}, nil |
There was a problem hiding this comment.
is email required? this is a really really weird way to make one up
There was a problem hiding this comment.
half yes half no. it's ignored past here, but I have to do it for tokens
There was a problem hiding this comment.
And email validation requires it.
oauthproxy.go
Outdated
| // email addresses can be verified if the CA is trusted | ||
| for _, email := range client.EmailAddresses { | ||
| if p.Validator(email) { | ||
| return &providers.SessionState{User: email, Email: email}, nil |
There was a problem hiding this comment.
looks like this allows clients that do not have the specified common name to access
There was a problem hiding this comment.
if you set allow all email addresses, yes
options.go
Outdated
| @@ -229,6 +234,10 @@ func (o *Options) Validate() error { | |||
| } | |||
| } | |||
|
|
|||
| if len(o.TLSClientCAFiles) > 0 && len(o.TLSKeyFile) == 0 { | |||
| msgs = append(msgs, "tls-client-ca requires tls-key-file to be set") | |||
There was a problem hiding this comment.
why? it requires https... is tls-key-file the only arg required to serve https?
|
Hrm - maybe we should create a system role allowing for SAR checking. |
Allow clients to specify:
The server will allow any NAME that verifies against any FILE via SSL to bypass normal authorization.