fix various issues with SMTP configuration (#1001)

* fix various issues with SMTP configuration

* handle SplitHostPort error

server/kolide/app.go

@@ -113,45 +113,45 @@ type ModifyAppConfigRequest struct {
 	AppConfig AppConfig `json:"app_config"`
 }
 
-// SMTPSettings is part of the AppConfigPayload which defines the wire representation
+// SMTPSettingsPayload is part of the AppConfigPayload which defines the wire representation
 // of the app config endpoints
-type SMTPSettings struct {
+type SMTPSettingsPayload struct {
 	// SMTPConfigured is a flag that indicates if smtp has been successfully
 	// tested with the settings provided by an admin user.
-	SMTPConfigured bool `json:"configured"`
+	SMTPConfigured *bool `json:"configured"`
 	// SMTPSenderAddress is the email address that will appear in emails sent
 	// from Kolide
-	SMTPSenderAddress string `json:"sender_address"`
+	SMTPSenderAddress *string `json:"sender_address"`
 	// SMTPServer is the host name of the SMTP server Kolide will use to send mail
-	SMTPServer string `json:"server"`
+	SMTPServer *string `json:"server"`
 	// SMTPPort port SMTP server will use
-	SMTPPort uint `json:"port"`
+	SMTPPort *uint `json:"port"`
 	// SMTPAuthenticationType type of authentication for SMTP
-	SMTPAuthenticationType string `json:"authentication_type"`
+	SMTPAuthenticationType *string `json:"authentication_type"`
 	// SMTPUserName must be provided if SMTPAuthenticationType is UserNamePassword
-	SMTPUserName string `json:"user_name"`
+	SMTPUserName *string `json:"user_name"`
 	// SMTPPassword must be provided if SMTPAuthenticationType is UserNamePassword
-	SMTPPassword string `json:"password"`
+	SMTPPassword *string `json:"password"`
 	// SMTPEnableSSLTLS whether to use SSL/TLS for SMTP
-	SMTPEnableTLS bool `json:"enable_ssl_tls"`
+	SMTPEnableTLS *bool `json:"enable_ssl_tls"`
 	// SMTPAuthenticationMethod authentication method smtp server will use
-	SMTPAuthenticationMethod string `json:"authentication_method"`
+	SMTPAuthenticationMethod *string `json:"authentication_method"`
 
 	// SMTPDomain optional domain for SMTP
-	SMTPDomain string `json:"domain"`
+	SMTPDomain *string `json:"domain"`
 	// SMTPVerifySSLCerts defaults to true but can be turned off if self signed
 	// SSL certs are used by the SMTP server
-	SMTPVerifySSLCerts bool `json:"verify_ssl_certs"`
+	SMTPVerifySSLCerts *bool `json:"verify_ssl_certs"`
 	// SMTPEnableStartTLS detects of TLS is enabled on mail server and starts to use it (default true)
-	SMTPEnableStartTLS bool `json:"enable_start_tls"`
+	SMTPEnableStartTLS *bool `json:"enable_start_tls"`
 }
 
 // AppConfigPayload contains request/response format of
 // the AppConfig endpoints.
 type AppConfigPayload struct {
-	OrgInfo        *OrgInfo        `json:"org_info"`
-	ServerSettings *ServerSettings `json:"server_settings"`
-	SMTPSettings   *SMTPSettings   `json:"smtp_settings"`
+	OrgInfo        *OrgInfo             `json:"org_info"`
+	ServerSettings *ServerSettings      `json:"server_settings"`
+	SMTPSettings   *SMTPSettingsPayload `json:"smtp_settings"`
 	// SMTPTest is a flag that if set will cause the server to test email configuration
 	SMTPTest *bool `json:"smtp_test,omitempty"`
 }

server/mail/mail.go

@@ -4,7 +4,9 @@ package mail
 import (
 	"crypto/tls"
 	"fmt"
+	"net"
 	"net/smtp"
+	"time"
 
 	"github.com/kolide/kolide-ose/server/kolide"
 	"github.com/pkg/errors"
@@ -112,7 +114,7 @@ func (m mailService) sendMail(e kolide.Email, msg []byte) error {
 		return nil
 	}
 
-	client, err := smtp.Dial(smtpHost)
+	client, err := dialTimeout(smtpHost)
 	if err != nil {
 		return errors.Wrap(err, "could not dial smtp host")
 	}
@@ -155,3 +157,17 @@ func (m mailService) sendMail(e kolide.Email, msg []byte) error {
 	}
 	return nil
 }
+
+// dialTimeout sets a timeout on net.Dial to prevent email from attempting to
+// send indefinitely.
+func dialTimeout(addr string) (*smtp.Client, error) {
+	conn, err := net.DialTimeout("tcp", addr, 15*time.Second)
+	if err != nil {
+		return nil, errors.Wrap(err, "dialing with timeout")
+	}
+	host, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		return nil, errors.Wrap(err, "split host port")
+	}
+	return smtp.NewClient(conn, host)
+}

server/service/endpoint_appconfig.go

@@ -9,11 +9,15 @@ import (
 	"golang.org/x/net/context"
 )
 
+type appConfigRequest struct {
+	Payload kolide.AppConfigPayload
+}
+
 type appConfigResponse struct {
-	OrgInfo        *kolide.OrgInfo        `json:"org_info,omitemtpy"`
-	ServerSettings *kolide.ServerSettings `json:"server_settings,omitempty"`
-	SMTPSettings   *kolide.SMTPSettings   `json:"smtp_settings,omitempty"`
-	Err            error                  `json:"error,omitempty"`
+	OrgInfo        *kolide.OrgInfo             `json:"org_info,omitemtpy"`
+	ServerSettings *kolide.ServerSettings      `json:"server_settings,omitempty"`
+	SMTPSettings   *kolide.SMTPSettingsPayload `json:"smtp_settings,omitempty"`
+	Err            error                       `json:"error,omitempty"`
 }
 
 func (r appConfigResponse) error() error { return r.Err }
@@ -28,12 +32,12 @@ func makeGetAppConfigEndpoint(svc kolide.Service) endpoint.Endpoint {
 		if err != nil {
 			return nil, err
 		}
-		var smtpSettings *kolide.SMTPSettings
+		var smtpSettings *kolide.SMTPSettingsPayload
 		// only admin can see smtp settings
 		if vc.IsAdmin() {
 			smtpSettings = smtpSettingsFromAppConfig(config)
-			if smtpSettings.SMTPPassword != "" {
-				smtpSettings.SMTPPassword = "********"
+			if smtpSettings.SMTPPassword != nil {
+				*smtpSettings.SMTPPassword = "********"
 			}
 		}
 		response := appConfigResponse{
@@ -50,10 +54,10 @@ func makeGetAppConfigEndpoint(svc kolide.Service) endpoint.Endpoint {
 	}
 }
 
-func makeModifyAppConfigRequest(svc kolide.Service) endpoint.Endpoint {
+func makeModifyAppConfigEndpoint(svc kolide.Service) endpoint.Endpoint {
 	return func(ctx context.Context, request interface{}) (interface{}, error) {
-		req := request.(kolide.AppConfigPayload)
-		config, err := svc.ModifyAppConfig(ctx, req)
+		req := request.(appConfigRequest)
+		config, err := svc.ModifyAppConfig(ctx, req.Payload)
 		if err != nil {
 			return appConfigResponse{Err: err}, nil
 		}
@@ -67,9 +71,28 @@ func makeModifyAppConfigRequest(svc kolide.Service) endpoint.Endpoint {
 			},
 			SMTPSettings: smtpSettingsFromAppConfig(config),
 		}
-		if response.SMTPSettings.SMTPPassword != "" {
-			response.SMTPSettings.SMTPPassword = "********"
+		if response.SMTPSettings.SMTPPassword != nil {
+			*response.SMTPSettings.SMTPPassword = "********"
 		}
 		return response, nil
 	}
 }
+
+func smtpSettingsFromAppConfig(config *kolide.AppConfig) *kolide.SMTPSettingsPayload {
+	authType := config.SMTPAuthenticationType.String()
+	authMethod := config.SMTPAuthenticationMethod.String()
+	return &kolide.SMTPSettingsPayload{
+		SMTPConfigured:           &config.SMTPConfigured,
+		SMTPSenderAddress:        &config.SMTPSenderAddress,
+		SMTPServer:               &config.SMTPServer,
+		SMTPPort:                 &config.SMTPPort,
+		SMTPAuthenticationType:   &authType,
+		SMTPUserName:             &config.SMTPUserName,
+		SMTPPassword:             &config.SMTPPassword,
+		SMTPEnableTLS:            &config.SMTPEnableTLS,
+		SMTPAuthenticationMethod: &authMethod,
+		SMTPDomain:               &config.SMTPDomain,
+		SMTPVerifySSLCerts:       &config.SMTPVerifySSLCerts,
+		SMTPEnableStartTLS:       &config.SMTPEnableStartTLS,
+	}
+}

server/service/endpoint_appconfig_test.go

@@ -22,7 +22,6 @@ type mockValidationError struct {
 }
 
 func testGetAppConfig(t *testing.T, r *testResource) {
-
 	req, err := http.NewRequest("GET", r.server.URL+"/api/v1/kolide/config", nil)
 	require.Nil(t, err)
 	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", r.adminToken))
@@ -36,8 +35,8 @@ func testGetAppConfig(t *testing.T, r *testResource) {
 	require.Nil(t, err)
 	require.NotNil(t, configInfo.SMTPSettings)
 	config := configInfo.SMTPSettings
-	assert.Equal(t, uint(465), config.SMTPPort)
-	require.NotNil(t, configInfo.OrgInfo)
+	assert.Equal(t, uint(465), *config.SMTPPort)
+	require.NotNil(t, *configInfo.OrgInfo)
 	assert.Equal(t, "Kolide", *configInfo.OrgInfo.OrgName)
 	assert.Equal(t, "http://foo.bar/image.png", *configInfo.OrgInfo.OrgLogoURL)
 
@@ -105,3 +104,16 @@ func testModifyAppConfigWithValidationFail(t *testing.T, r *testResource) {
 	assert.Nil(t, err)
 	assert.Equal(t, config.SMTPEnableStartTLS, existing.SMTPEnableStartTLS)
 }
+
+func appConfigPayloadFromAppConfig(config *kolide.AppConfig) *kolide.AppConfigPayload {
+	return &kolide.AppConfigPayload{
+		OrgInfo: &kolide.OrgInfo{
+			OrgLogoURL: &config.OrgLogoURL,
+			OrgName:    &config.OrgName,
+		},
+		ServerSettings: &kolide.ServerSettings{
+			KolideServerURL: &config.KolideServerURL,
+		},
+		SMTPSettings: smtpSettingsFromAppConfig(config),
+	}
+}

server/service/endpoint_test.go

@@ -2,7 +2,6 @@ package service
 
 import (
 	"bytes"
-	"context"
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
@@ -19,6 +18,7 @@ import (
 	"github.com/kolide/kolide-ose/server/datastore/inmem"
 	"github.com/kolide/kolide-ose/server/kolide"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/net/context"
 )
 
 type testResource struct {
@@ -28,6 +28,13 @@ type testResource struct {
 	ds         kolide.Datastore
 }
 
+type endpointService struct {
+	kolide.Service
+}
+
+func (svc endpointService) SendTestEmail(ctx context.Context, config *kolide.AppConfig) error {
+	return nil
+}
 func setupEndpointTest(t *testing.T) *testResource {
 	test := &testResource{}
 
@@ -47,6 +54,7 @@ func setupEndpointTest(t *testing.T) *testResource {
 	}
 	test.ds.NewAppConfig(devOrgInfo)
 	svc, _ := newTestService(test.ds, nil)
+	svc = endpointService{svc}
 	createTestUsers(t, test.ds)
 	logger := kitlog.NewLogfmtLogger(os.Stdout)
 

server/service/handler.go

@@ -108,7 +108,7 @@ func MakeKolideServerEndpoints(svc kolide.Service, jwtKey string) KolideEndpoint
 		GetSessionInfo:                 authenticatedUser(jwtKey, svc, mustBeAdmin(makeGetInfoAboutSessionEndpoint(svc))),
 		DeleteSession:                  authenticatedUser(jwtKey, svc, mustBeAdmin(makeDeleteSessionEndpoint(svc))),
 		GetAppConfig:                   authenticatedUser(jwtKey, svc, canPerformActions(makeGetAppConfigEndpoint(svc))),
-		ModifyAppConfig:                authenticatedUser(jwtKey, svc, mustBeAdmin(makeModifyAppConfigRequest(svc))),
+		ModifyAppConfig:                authenticatedUser(jwtKey, svc, mustBeAdmin(makeModifyAppConfigEndpoint(svc))),
 		CreateInvite:                   authenticatedUser(jwtKey, svc, mustBeAdmin(makeCreateInviteEndpoint(svc))),
 		ListInvites:                    authenticatedUser(jwtKey, svc, mustBeAdmin(makeListInvitesEndpoint(svc))),
 		DeleteInvite:                   authenticatedUser(jwtKey, svc, mustBeAdmin(makeDeleteInviteEndpoint(svc))),

server/service/service_appconfig.go

@@ -6,7 +6,6 @@ import (
 	"github.com/kolide/kolide-ose/server/contexts/viewer"
 	"github.com/kolide/kolide-ose/server/kolide"
 	"github.com/kolide/kolide-ose/server/mail"
-	"github.com/pkg/errors"
 	"golang.org/x/net/context"
 )
 
@@ -69,23 +68,21 @@ func (svc service) SendTestEmail(ctx context.Context, config *kolide.AppConfig)
 func (svc service) ModifyAppConfig(ctx context.Context, p kolide.AppConfigPayload) (*kolide.AppConfig, error) {
 	oldAppConfig, err := svc.AppConfig(ctx)
 	if err != nil {
-		return nil, errors.Wrap(err, "failed retrieving existing app config")
+		return nil, err
 	}
 	config := appConfigFromAppConfigPayload(p, *oldAppConfig)
 
 	if p.SMTPSettings != nil {
 		if err = svc.SendTestEmail(ctx, config); err != nil {
-			err = errors.Wrap(err, "test email failed")
-			config.SMTPConfigured = false
-		} else {
-			config.SMTPConfigured = true
+			return nil, err
 		}
+		config.SMTPConfigured = true
 	}
 
 	if err := svc.ds.SaveAppConfig(config); err != nil {
-		err = errors.Wrap(err, "could not save config")
+		return nil, err
 	}
-	return config, err
+	return config, nil
 }
 
 func appConfigFromAppConfigPayload(p kolide.AppConfigPayload, config kolide.AppConfig) *kolide.AppConfig {
@@ -98,57 +95,68 @@ func appConfigFromAppConfigPayload(p kolide.AppConfigPayload, config kolide.AppC
 	if p.ServerSettings != nil && p.ServerSettings.KolideServerURL != nil {
 		config.KolideServerURL = *p.ServerSettings.KolideServerURL
 	}
+
+	populateSMTP := func(p *kolide.SMTPSettingsPayload) {
+		if p.SMTPAuthenticationMethod != nil {
+			switch *p.SMTPAuthenticationMethod {
+			case kolide.AuthMethodNameCramMD5:
+				config.SMTPAuthenticationMethod = kolide.AuthMethodCramMD5
+			case kolide.AuthMethodNamePlain:
+				config.SMTPAuthenticationMethod = kolide.AuthMethodPlain
+			default:
+				panic("unknown SMTP AuthMethod: " + *p.SMTPAuthenticationMethod)
+			}
+		}
+		if p.SMTPAuthenticationType != nil {
+			switch *p.SMTPAuthenticationType {
+			case kolide.AuthTypeNameUserNamePassword:
+				config.SMTPAuthenticationType = kolide.AuthTypeUserNamePassword
+			case kolide.AuthTypeNameNone:
+				config.SMTPAuthenticationType = kolide.AuthTypeNone
+			default:
+				panic("unknown SMTP AuthType: " + *p.SMTPAuthenticationType)
+			}
+		}
+
+		if p.SMTPDomain != nil {
+			config.SMTPDomain = *p.SMTPDomain
+		}
+
+		if p.SMTPEnableStartTLS != nil {
+			config.SMTPEnableStartTLS = *p.SMTPEnableStartTLS
+		}
+
+		if p.SMTPEnableTLS != nil {
+			config.SMTPEnableTLS = *p.SMTPEnableTLS
+		}
+
+		if p.SMTPPassword != nil {
+			config.SMTPPassword = *p.SMTPPassword
+		}
+
+		if p.SMTPPort != nil {
+			config.SMTPPort = *p.SMTPPort
+		}
+
+		if p.SMTPSenderAddress != nil {
+			config.SMTPSenderAddress = *p.SMTPSenderAddress
+		}
+
+		if p.SMTPServer != nil {
+			config.SMTPServer = *p.SMTPServer
+		}
+
+		if p.SMTPUserName != nil {
+			config.SMTPUserName = *p.SMTPUserName
+		}
+
+		if p.SMTPVerifySSLCerts != nil {
+			config.SMTPVerifySSLCerts = *p.SMTPVerifySSLCerts
+		}
+	}
+
 	if p.SMTPSettings != nil {
-		if p.SMTPSettings.SMTPAuthenticationMethod == kolide.AuthMethodNameCramMD5 {
-			config.SMTPAuthenticationMethod = kolide.AuthMethodCramMD5
-		} else {
-			config.SMTPAuthenticationMethod = kolide.AuthMethodPlain
-		}
-		if p.SMTPSettings.SMTPAuthenticationType == kolide.AuthTypeNameUserNamePassword {
-			config.SMTPAuthenticationType = kolide.AuthTypeUserNamePassword
-		} else {
-			config.SMTPAuthenticationType = kolide.AuthTypeNone
-		}
-		config.SMTPConfigured = p.SMTPSettings.SMTPConfigured
-		config.SMTPDomain = p.SMTPSettings.SMTPDomain
-		config.SMTPEnableStartTLS = p.SMTPSettings.SMTPEnableStartTLS
-		config.SMTPEnableTLS = p.SMTPSettings.SMTPEnableTLS
-		config.SMTPPassword = p.SMTPSettings.SMTPPassword
-		config.SMTPPort = p.SMTPSettings.SMTPPort
-		config.SMTPSenderAddress = p.SMTPSettings.SMTPSenderAddress
-		config.SMTPServer = p.SMTPSettings.SMTPServer
-		config.SMTPUserName = p.SMTPSettings.SMTPUserName
-		config.SMTPVerifySSLCerts = p.SMTPSettings.SMTPVerifySSLCerts
+		populateSMTP(p.SMTPSettings)
 	}
 	return &config
 }
-
-func smtpSettingsFromAppConfig(config *kolide.AppConfig) *kolide.SMTPSettings {
-	return &kolide.SMTPSettings{
-		SMTPConfigured:           config.SMTPConfigured,
-		SMTPSenderAddress:        config.SMTPSenderAddress,
-		SMTPServer:               config.SMTPServer,
-		SMTPPort:                 config.SMTPPort,
-		SMTPAuthenticationType:   config.SMTPAuthenticationType.String(),
-		SMTPUserName:             config.SMTPUserName,
-		SMTPPassword:             config.SMTPPassword,
-		SMTPEnableTLS:            config.SMTPEnableTLS,
-		SMTPAuthenticationMethod: config.SMTPAuthenticationMethod.String(),
-		SMTPDomain:               config.SMTPDomain,
-		SMTPVerifySSLCerts:       config.SMTPVerifySSLCerts,
-		SMTPEnableStartTLS:       config.SMTPEnableStartTLS,
-	}
-}
-
-func appConfigPayloadFromAppConfig(config *kolide.AppConfig) *kolide.AppConfigPayload {
-	return &kolide.AppConfigPayload{
-		OrgInfo: &kolide.OrgInfo{
-			OrgLogoURL: &config.OrgLogoURL,
-			OrgName:    &config.OrgName,
-		},
-		ServerSettings: &kolide.ServerSettings{
-			KolideServerURL: &config.KolideServerURL,
-		},
-		SMTPSettings: smtpSettingsFromAppConfig(config),
-	}
-}

server/service/transport_appconfig.go

@@ -10,9 +10,9 @@ import (
 )
 
 func decodeModifyAppConfigRequest(ctx context.Context, r *http.Request) (interface{}, error) {
-	var req kolide.AppConfigPayload
-	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+	var payload kolide.AppConfigPayload
+	if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
 		return nil, err
 	}
-	return req, nil
+	return appConfigRequest{Payload: payload}, nil
 }