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

need a TLS MinVersion arg #3058

Closed
wu105 opened this issue May 26, 2018 · 11 comments · Fixed by #5013
Closed

need a TLS MinVersion arg #3058

wu105 opened this issue May 26, 2018 · 11 comments · Fixed by #5013
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@wu105
Copy link

wu105 commented May 26, 2018

Environment
Dashboard version: v1.7.1
Kubernetes version: v1.9.1
Operating system: Oracle Linux
Node.js version: N/A
Go version: N/A

This is a feature request: when serving over https, dashboard offers TLSv1.0 thus fails our security scan. Would like to have an argument for higher TLS MinVersion.
The TLS ciphers offerred is not currently a problem for us, but that could change, thus an argument to control TLS cipher set might be useful.

Steps to reproduce
Observed result
Expected result

When TLS MinVersion is set to TLSv1.2, for instance, the dashboard would offer TLSv1.2 as the lowest version during SSL hand shake, thus pass security scan which requires TLSv1.2 or above.

Comments
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 24, 2018
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 23, 2018
@wu105
Copy link
Author

wu105 commented Sep 23, 2018

/remove-lifecycle stale
/remove-lifecycle rotten

This is minor but when we need to pass the security scan we had to resort to creating our own version.
We would have the same issue when upgrade.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 23, 2018
@maciaszczykm
Copy link
Member

/lifecycle frozen

@wu105 Perhaps you can prepare some pull request for it?

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Sep 24, 2018
@wu105
Copy link
Author

wu105 commented Sep 24, 2018

@maciaszczykm What I did is quite messy, did not do anything on TLS ciphers nor adding arguments, thus it may be better to share what I did to get it to pass our security scan. We started with cloning version v1.7.1 source.

  • hardwired MinVersion
diff --git a/src/app/backend/dashboard.go b/src/app/backend/dashboard.go
index 5d7b993..f3fde4f 100644
--- a/src/app/backend/dashboard.go
+++ b/src/app/backend/dashboard.go
@@ -15,6 +15,8 @@
 package main
 
 import (
+	"crypto/tls"
+
 	"flag"
 	"fmt"
 	"log"
@@ -109,7 +111,19 @@ func main() {
 	if *argCertFile != "" && *argKeyFile != "" {
 		log.Printf("Serving securely on HTTPS port: %d", *argPort)
 		secureAddr := fmt.Sprintf("%s:%d", *argBindAddress, *argPort)
-		go func() { log.Fatal(http.ListenAndServeTLS(secureAddr, *argCertFile, *argKeyFile, nil)) }()
+                servingCert, err := tls.LoadX509KeyPair(*argCertFile, *argKeyFile)
+                if err != nil {
+		       handleFatalInitError(err)
+	        }
+                var servingCerts []tls.Certificate;
+                servingCerts = []tls.Certificate{servingCert}
+		server := &http.Server{
+			Addr:      secureAddr,
+			Handler:   http.DefaultServeMux,
+                        TLSConfig: &tls.Config{MinVersion: tls.VersionTLS12,
+                                               Certificates: servingCerts},
+		}
+		go func() { log.Fatal(server.ListenAndServeTLS("", "")) }()
 	} else {
 		log.Printf("Serving insecurely on HTTP port: %d", *argInsecurePort)
 		addr := fmt.Sprintf("%s:%d", *argInsecureBindAddress, *argInsecurePort)
  • in package-lock.json, changed all git:// to https://

  • used build/run-gulp-in-docker.sh from v1.8.3, seems the same as v1.7.1. Use of this script to build could be more prominently documented as all my attempts of compiling directly failed until discovered this script.

  • used build/Dockerfile from v1.8.3, and modified it to work around our https proxy:

diff --git a/build/Dockerfile b/build/Dockerfile
index b4c8115..843409a 100644
--- a/build/Dockerfile
+++ b/build/Dockerfile
@@ -1,4 +1,4 @@
-# Copyright 2017 The Kubernetes Dashboard Authors.
+# Copyright 2017 The Kubernetes Authors.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -20,27 +20,32 @@
 # golang is based on debian:jessie
 FROM golang
 
-# Install java and nodejs. go is already installed
-# A small tweak, apt-get update is already run by the nodejs setup script,
-# so there's no need to run it again
-RUN curl -sL https://deb.nodesource.com/setup_6.x | bash - \
+# zscaler issue delt with the following added lines after base image load:
+# to prepare, cd ~/dashboard/build
+# cp /etc/pki/ca-trust/source/anchors/rootCAcrt2bTrusted.crt local-ca-certificates.crt
+COPY build/local-ca-certificates.crt /usr/local/share/ca-certificates
+RUN update-ca-certificates
+ENV NODE_EXTRA_CA_CERTS="/usr/local/share/ca-certificates/local-ca-certificates.crt"
+
+# Install Java and Node.js. Go is already installed.
+# A small tweak, apt-get update is already run by the nodejs setup script, so there's no need to run it again.
+RUN curl -sL https://deb.nodesource.com/setup_9.x | bash - \
   && apt-get install -y --no-install-recommends \
 	openjdk-8-jre \
 	nodejs \
+	patch \
 	&& rm -rf /var/lib/apt/lists/* \
 	&& apt-get clean
 
-# Download a statically linked docker client, so the container is able to build images on the host
+# Download a statically linked docker client, so the container is able to build images on the host.
 RUN curl -sSL https://get.docker.com/builds/Linux/x86_64/docker-1.9.1 > /usr/bin/docker && \
     chmod +x /usr/bin/docker
 
-# Current directory is always /dashboard
+# Current directory is always /dashboard.
 WORKDIR /dashboard
 
-# Install all npm deps. This will take a while.
-COPY package.json ./
-COPY build/postinstall.sh build/
-RUN npm install --unsafe-perm
-
 # Copy entire source tree.
 COPY ./ ./
+
+# Install dependencies. This will take a while.
+RUN npm install --unsafe-perm

@wu105
Copy link
Author

wu105 commented Mar 14, 2019

Just checked version 1.10.1, in src/app/backend/dashboard.go, in the main function,
The line TLSConfig: &tls.Config{Certificates: servingCerts} would need to change to something like the following in order to restrict to TLS 1.2, as required by our security policy:
TLSConfig: &tls.Config{MinVersion: tls.VersionTLS12, Certificates: servingCerts}

@Michael-Baylis
Copy link

I have made the same path to dashboard.go and works perfectly. Had to patch it as the dashboard is failing our security scans as well.
If I get a quiet few moments, I will learn enough about go to add a option to the code and raise a PR

@Michael-Baylis
Copy link

so thank you @wu105 for the tip

@shu-mutou
Copy link
Contributor

@wu105 @Michael-Baylis Thanks for the solution. Since I posted PR(#5013 ), could you review it?

@Michael-Baylis
Copy link

@shu-mutou I think #5013 needs to be future proofed by suppling the MinVersion as a option, which is what I was going to do in a PR, if I ever get time

@Michael-Baylis
Copy link

happy with that, saves me doing anything. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants