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

Bug fixes and add CA cert flag for LDAP #69

Merged
merged 4 commits into from Mar 7, 2018

Conversation

@nightfury1204
Copy link
Contributor

commented Mar 5, 2018

No description provided.

@nightfury1204 nightfury1204 requested a review from tamalsaha Mar 5, 2018
@@ -346,7 +378,7 @@ func newService(namespace, addr string) runtime.Object {
ClusterIP: host,
Ports: []core.ServicePort{
{
Name: "api",
Name: "service-api",

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Mar 6, 2018

Member

Why rename?

This comment has been minimized.

Copy link
@nightfury1204

nightfury1204 Mar 6, 2018

Author Contributor

i renamed it because container port name and service port name is same, but port is different. So, when i testing it in minikube, guard server didn't get any request. In kubernetes logs i found the 'connection refused error'.

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Mar 7, 2018

Member

should be fixed now

VolumeSource: core.VolumeSource{
Secret: &core.SecretVolumeSource{
SecretName: "guard-cert",
DefaultMode: types.Int32P(0555),

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Mar 6, 2018

Member

Why not just 0444 (readonly)?

if err != nil {
log.Fatalln(err)
}
data, err = meta.MarshalToYAML(newSecretForLDAPCert(opts.namespace, map[string][]byte{"ca.crt": cert}), core.SchemeGroupVersion)

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Mar 6, 2018

Member

Assign the secret to a new variable first. Makes it more readable

@@ -101,7 +111,7 @@ func (s Server) ListenAndServe() {
tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
},
ClientAuth: tls.RequireAndVerifyClientCert,
ClientAuth: tls.VerifyClientCertIfGiven,

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Mar 6, 2018

Member

Add a comment that this is needed to pass healthz checks

log.Fatal(err)
}
s.LDAP.caCertPool = x509.NewCertPool()
s.LDAP.caCertPool.AppendCertsFromPEM(caCert)

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Mar 6, 2018

Member

Check return value to make sure the cert was found and added


tlsConfig := &tls.Config{
ServerName: s.LDAP.ServerAddress,
InsecureSkipVerify: s.LDAP.SkipTLSVerification,

This comment has been minimized.

Copy link
@tamalsaha

tamalsaha Mar 6, 2018

Member

Why are we using insecure defaults?

@tamalsaha tamalsaha changed the title fixes bug and add CA cert flag for LDAP Bug fixes and add CA cert flag for LDAP Mar 7, 2018
@tamalsaha tamalsaha merged commit d04a7f7 into master Mar 7, 2018
@tamalsaha tamalsaha deleted the fix branch Mar 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.