Native SSL support #226

natemueller · 2018-01-03T21:31:32Z

Add three new flags for atlantis server:

--ssl to enable ssl
--ssl-key-file for the key
--ssl-cert-file for the server and CA certs

If you specify --ssl then all the other two are required.

Currently defaults to non-SSL mode. I think you could make a good
argument for SSL by default but since that would be a breaking change
I left it for the maintainers to decide.

Removed docs for setting up an NGINX frontend.

CLAassistant · 2018-01-03T21:31:38Z

All committers have signed the CLA.

lkysow

Thanks so much @natemueller and sorry it took me so long to get to this.

I facepalmed when I read this because I didn't know it was this easy to set up golang to serve TLS. Should have done this months ago!

A couple comments. Let me know what you think and if you don't have time LMK and I can make the changes myself.

lkysow · 2018-01-16T06:03:20Z

cmd/server.go

@@ -31,6 +31,9 @@ const (
 	LogLevelFlag        = "log-level"
 	PortFlag            = "port"
 	RequireApprovalFlag = "require-approval"
+	SSL                 = "ssl"


I don't think we need the SSL flag? Let's just use SSL if any of the other options are specified. I'm just thinking that it's simpler if we eliminate it.

Append Flag to SSLKeyFile and SSLCert to match the rest of the constants above it. The Flag part is so the string isn't confused with the actual contents of the flag.

lkysow · 2018-01-16T06:08:37Z

cmd/server.go

@@ -31,6 +31,9 @@ const (
 	LogLevelFlag        = "log-level"
 	PortFlag            = "port"
 	RequireApprovalFlag = "require-approval"
+	SSL                 = "ssl"
+	SSLKeyFile          = "ssl-key-file"
+	SSLCertFile         = "ssl-cert-file"


SSLCertFile should be before SSLKeyFile to keep alphabetic ordering

lkysow · 2018-01-16T06:10:21Z

cmd/server.go

@@ -94,13 +97,26 @@ var stringFlags = []stringFlag{
 		description: "Log level. Either debug, info, warn, or error.",
 		value:       "info",
 	},
+	{
+		name:        SSLKeyFile,
+		description: "Server key file, required if using SSL",


Let's change this to description: fmt.Sprintf("File containing x509 private key matching --%s.", SSLCertFileFlag)

I'm just following the way the docs are for Kubernete's kubelet which I thought would be good docs to copy from (https://kubernetes.io/docs/reference/generated/kubelet/)

lkysow · 2018-01-16T06:13:40Z

cmd/server.go

+	},
+	{
+		name:        SSLCertFile,
+		description: "Server and CN cert file, required if using SSL",


Again stealing from the kubelet, let's change this to:
description: "File containing x509 Certificate used for serving HTTPS. If the cert is signed by a CA, the file should be the concatenation of the server's certificate, any intermediates, and the CA's certificate."

The last part is from the golang docs on ListenAndServeTLS

lkysow · 2018-01-16T06:18:24Z

cmd/server.go

@@ -250,6 +266,10 @@ func (s *ServerCmd) validate(config server.Config) error {
 	}
 	vcsErr := fmt.Errorf("--%s/--%s or --%s/--%s must be set", GHUserFlag, GHTokenFlag, GitlabUserFlag, GitlabTokenFlag)

+	if config.SSL && (config.SSLKeyFile == "" || config.SSLCertFile == "") {
+		return errors.New("ssl-key-file and ssl-cert-file are required for ssl")


use the flag constants when printing this error message so if we change the flag name we don't miss changing this error message too

Add two new flags for atlantis server: --ssl-key-file for the key --ssl-cert-file for the server and CA certs You either need to use both or neither. You'll get an error if you accidentally use only one. Currently defaults to non-SSL mode. I think you could make a good argument for SSL by default but since that would be a breaking change I left it for the maintainers to decide. Removed docs for setting up an NGINX frontend.

natemueller · 2018-01-16T16:13:51Z

Should all be addressed now. I was torn on the --ssl flag too. I ended up adding it because I felt it was more defensive then an implicit linkage between the separate cert and key flags but I feel strongly about it.

lkysow

RE: --ssl option, I hear you. Not sure what the best thing in this case is.
We also need to update the tests in cmd/server_test.go. Here's what I came up with

func TestExecute_ValidateSSLConfig(t *testing.T) {
	expErr := "--ssl-key-file and --ssl-cert-file are both required for ssl"
	cases := []struct {
		description string
		flags       map[string]interface{}
		expectError bool
	}{
		{
			"neither option set",
			make(map[string]interface{}),
			false,
		},
		{
			"just ssl-key-file set",
			map[string]interface{}{
				cmd.SSLKeyFileFlag: "file",
			},
			true,
		},
		{
			"just ssl-cert-file set",
			map[string]interface{}{
				cmd.SSLCertFileFlag: "flag",
			},
			true,
		},
		{
			"both flags set",
			map[string]interface{}{
				cmd.SSLCertFileFlag: "cert",
				cmd.SSLKeyFileFlag:  "key",
			},
			false,
		},
	}
	for _, testCase := range cases {
		t.Log("Should validate ssl config when " + testCase.description)
		// Add in required flags.
		testCase.flags[cmd.GHUserFlag] = "user"
		testCase.flags[cmd.GHTokenFlag] = "token"

		c := setup(testCase.flags)
		err := c.Execute()
		if testCase.expectError {
			Assert(t, err != nil, "should be an error")
			Equals(t, expErr, err.Error())
		} else {
			Ok(t, err)
		}
	}
}

lkysow · 2018-01-16T23:14:31Z

README.md

@@ -481,7 +481,7 @@ However, if you were to lose the data, all you would need to do is run `atlantis

 **Q: How to add SSL to Atlantis server?**

-A: Atlantis currently only supports HTTP. In order to add SSL you will need to front Atlantis server with NGINX or HAProxy. Follow the document [here](./docs/nginx-ssl-proxy.md) to use configure NGINX with SSL as a reverse proxy.
+A: Pass the `--ssl` option to enable SSL for incoming connections.  You will need to get a trusted certificate and pass it into Atlantis server with the `--ssl-key-file` and `--ssl-cert-file` options.


This is incorrect now after the removal of the ssl flag. Should read

Start atlantis server with the --ssl-cert-file and --ssl-key-file flags.

lkysow · 2018-01-16T23:21:34Z

cmd/server.go

@@ -248,13 +258,17 @@ func (s *ServerCmd) validate(config server.Config) error {
 	if logLevel != "debug" && logLevel != "info" && logLevel != "warn" && logLevel != "error" {
 		return errors.New("invalid log level: not one of debug, info, warn, error")
 	}
-	vcsErr := fmt.Errorf("--%s/--%s or --%s/--%s must be set", GHUserFlag, GHTokenFlag, GitlabUserFlag, GitlabTokenFlag)
+
+	if (config.SSLKeyFile == "") != (config.SSLCertFile == "") {


took me a bit to grok this but like its simplicity. I think I could probably improve my three if's below using the same pattern.

lkysow · 2018-01-16T23:24:03Z

cmd/server.go

-	vcsErr := fmt.Errorf("--%s/--%s or --%s/--%s must be set", GHUserFlag, GHTokenFlag, GitlabUserFlag, GitlabTokenFlag)
+
+	if (config.SSLKeyFile == "") != (config.SSLCertFile == "") {
+		return fmt.Errorf("%s and %s are required for ssl", SSLKeyFileFlag, SSLCertFileFlag)


just missing the -- in front of the %s's so it ends up reading

--ssl-key-file and --ssl-cert-file are both required for ssl

instead of

ssl-key-file and ssl-cert-file are both required for ssl

Let's also add in both required so it's clear that the error is because both aren't specified

lkysow · 2018-01-23T19:32:56Z

Closed by #233 (I just made the changes I stated above)

lkysow reviewed Jan 16, 2018

View reviewed changes

natemueller force-pushed the native-ssl branch from 0d6eb4c to bd682bf Compare January 16, 2018 16:11

lkysow reviewed Jan 16, 2018

View reviewed changes

lkysow mentioned this pull request Jan 23, 2018

Native SSL Support #233

Merged

lkysow closed this Jan 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native SSL support #226

Native SSL support #226

natemueller commented Jan 3, 2018

CLAassistant commented Jan 3, 2018 •

edited

Loading

lkysow left a comment

lkysow Jan 16, 2018

lkysow Jan 16, 2018

lkysow Jan 16, 2018

lkysow Jan 16, 2018

lkysow Jan 16, 2018

natemueller commented Jan 16, 2018

lkysow left a comment

lkysow Jan 16, 2018

lkysow Jan 16, 2018

lkysow Jan 16, 2018

lkysow commented Jan 23, 2018

Native SSL support #226

Native SSL support #226

Conversation

natemueller commented Jan 3, 2018

CLAassistant commented Jan 3, 2018 • edited Loading

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemueller commented Jan 16, 2018

lkysow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkysow commented Jan 23, 2018

CLAassistant commented Jan 3, 2018 •

edited

Loading