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

Added configmap settings to support perfect forward secrecy #85

Merged
merged 2 commits into from
Dec 1, 2016

Conversation

thetechnick
Copy link
Contributor

I have tested this PR on my environment using the https://www.ssllabs.com checker.
Fixes #69

Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments

if existsSPSC && err != nil {
glog.Error(err)
} else {
cfg.MainServerSSLPreferServerCiphers = sslPreferServerCiphers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it doesn't lead to a bug, line 375 is executed when the ssl-prefer-server-ciphers doesn't exists in the configmap

@@ -85,13 +93,15 @@ func NewUpstreamWithDefaultServer(name string) Upstream {
// NewNginxController creates a NGINX controller
func NewNginxController(nginxConfPath string, local bool) (*NginxController, error) {
ngxc := NginxController{
nginxEtcPath: path.Join(nginxConfPath, "etc"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the nginxCertsPathfolder to store the pem file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was worried that someone uses dhparam as a secret name which would override the certificate secret. (I know this is a edge case, but I do not trust users)
But if we add the namespace to the filename of the certificates we can also use the cert folder for the dhparam.pem, this will also solve a not yet raised issue, if a secret with the same name is used in two namespaces.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if we add the namespace to the filename of the certificates we can also use the cert folder for the dhparam.pem, this will also solve a not yet raised issue, if a secret with the same name is used in two namespaces.

Thanks for catching that. I've created a separate issue for that -- #89

func (nginx *NginxController) createCertsDir() {
if err := os.Mkdir(nginx.nginxCertsPath, os.ModeDir); err != nil {
glog.Fatalf("Couldn't create directory %v: %v", nginx.nginxCertsPath, err)
func (nginx *NginxController) createDir(path string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be a function, not a method, since it doesn't depend on NginxController anymore

@pleshakov pleshakov added this to the v0.7.0 milestone Nov 29, 2016
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the comments

@@ -467,7 +490,7 @@ func (lbc *LoadBalancerController) createIngress(ing *extensions.Ingress) nginx.
glog.Warningf("Error retrieving secret %v for Ingress %v: %v", secretName, ing.Name, err)
continue
}
ingEx.Secrets[secretName] = secret
ingEx.Secrets[fmt.Sprintf("%s-%s", ing.GetNamespace(), secretName)] = secret
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a bug. The code in the configurator still uses the secretName as a key.

Better to solve the problem #89 in a separate pull request.

if err != nil {
glog.Errorf("Configmap %s/%s: Could not update dhparams: %v", cfgm.GetNamespace(), cfgm.GetName(), err)
}
cfg.MainServerSSLDHParam = fileName
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of an error when writing the dh param file, setting cfg.MainServerSSLDHParam = fileName will lead to an invalid NGINX configuration, when the dh param file doesn't exist

@pleshakov pleshakov merged commit f44e9fc into nginxinc:master Dec 1, 2016
pleshakov added a commit that referenced this pull request Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants