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

Ingress Controller Fake Cert Generation #194

Closed
wants to merge 174 commits into from

Conversation

rikatz
Copy link
Contributor

@rikatz rikatz commented Jan 31, 2017

This PR solves the problem reported in #191

It generates a new Fake Cert, instead of using the snake oil.

Once the Fake Cert is generated, it's not created anymore until someone erases it.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 31, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 43.873% when pulling 5fc45ea on rikatz:nginx-ssl-certproblem into b9d272a on kubernetes:master.

@aledbf aledbf self-assigned this Feb 2, 2017
@aledbf
Copy link
Member

aledbf commented Feb 2, 2017

LGTM

Waiting @bprashanth comment in #191 to merge

bprashanth and others added 9 commits February 2, 2017 13:15
FIX: ingress was not creating the endpoint when target port is string
- point to most recent version of k8s-wide docs, not some frozen version
- actually mention the ingress-specific development guide
aledbf and others added 18 commits February 24, 2017 15:12
Fix node lister when --watch-namespace is used
Add annotation to customize nginx configuration
Refactoring of TCP and UDP services
update some descriptions about of 'Test HTTP Service'
…tion

	modified:   controllers/nginx/configuration.md
	modified:   controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl
	modified:   core/pkg/ingress/annotations/authtls/main.go
	modified:   core/pkg/ingress/controller/backend_ssl.go
	modified:   core/pkg/ingress/controller/controller.go
	modified:   core/pkg/ingress/controller/util_test.go
	modified:   core/pkg/ingress/resolver/main.go
	modified:   core/pkg/ingress/types.go
	modified:   core/pkg/net/ssl/ssl.go
	modified:   examples/PREREQUISITES.md
	new file:   examples/auth/client-certs/nginx/README.md
	new file:   examples/auth/client-certs/nginx/nginx-tls-auth.yaml
Check for error getting cert
Change arg ordering in log message
Typo: unittesting -> unit testing
Expose Prometheus metrics in glbc controller
Adds correct support for TLS Muthual autentication
Enable custom election id for status sync.
@aledbf
Copy link
Member

aledbf commented Feb 28, 2017

@rikatz ping. What is the status of this PR?

@rikatz
Copy link
Contributor Author

rikatz commented Feb 28, 2017

@aledbf Will take a look at this later today. I do agree with @bprashanth that this should be optional, just couldn't figure out how to achieve this correctly. Maybe a new flag in ingress (specific to nginx ingress) that allows the user to generate or not this new Self Signed certificate.

@aledbf
Copy link
Member

aledbf commented Feb 28, 2017

I do agree with @bprashanth that this should be optional, just couldn't figure out how to achieve this correctly

This cannot be optional (nginx). We need a default certificate. Without one all the https traffic will be send to the first server with a ssl certificate.

@rikatz
Copy link
Contributor Author

rikatz commented Feb 28, 2017

@aledbf Sorry about that. It seems it fetched all the other commits incorrectly. Basically I've removed the configuration of default TLS on misconfigured vhosts, and generated the fake Cert only for the default server.

So when a server does not have a correct certificate, it will not configure a SSL certificate. But the default server (vhost _) will always have it.

I'll see if I can revert this bunch of commits / merges later, otherwise will close this PR and open another (as we still don't have any review here, but the discussion in the referenced issue #191 )

Sorry again, and thanks.

@rikatz
Copy link
Contributor Author

rikatz commented Mar 1, 2017

@aledbf Created a new branch as I couldn't revert the mess I did on this one: https://github.com/rikatz/ingress/tree/ingress-fake-cert

Will work in this tomorrow and check if everything is working by now.

So the behaviour in this case is to generate a self signed certificate and use it only in the default vhost, and not anymore in misconfigured vhosts.

So we can try to improve this later on another PR/discussion.

@rikatz
Copy link
Contributor Author

rikatz commented Mar 6, 2017

I've found a ugly bug in this implementation, that hangs out when no Default Secret exists and doesn't fetches the other secrets.

I'm working in a correction for this also, and will open a PR containing the bug fix and the fake cert generation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.