From a69113128827b4fec259263a645639ab8f697d6e Mon Sep 17 00:00:00 2001 From: James Elliott Date: Tue, 27 Dec 2022 08:32:00 +1100 Subject: [PATCH] fix(notification): missing use of timeout (#4652) --- internal/commands/storage_run.go | 2 +- internal/commands/util.go | 2 +- internal/notification/smtp_notifier.go | 36 ++++++++++++++++---- internal/server/template.go | 2 +- internal/utils/crypto.go | 47 ++++++++++++++++++++++++++ internal/utils/hashing_test.go | 4 +-- internal/utils/strings.go | 33 ------------------ internal/utils/strings_test.go | 8 ++--- 8 files changed, 84 insertions(+), 50 deletions(-) diff --git a/internal/commands/storage_run.go b/internal/commands/storage_run.go index d2033767c..d6ce0632d 100644 --- a/internal/commands/storage_run.go +++ b/internal/commands/storage_run.go @@ -983,7 +983,7 @@ func (ctx *CmdCtx) StorageUserTOTPExportPNGRunE(cmd *cobra.Command, _ []string) } if dir == "" { - dir = utils.RandomString(8, utils.CharSetAlphaNumeric, false) + dir = utils.RandomString(8, utils.CharSetAlphaNumeric) } if _, err = os.Stat(dir); !os.IsNotExist(err) { diff --git a/internal/commands/util.go b/internal/commands/util.go index 19072f9a4..3142088e7 100644 --- a/internal/commands/util.go +++ b/internal/commands/util.go @@ -109,7 +109,7 @@ func flagsGetRandomCharacters(flags *pflag.FlagSet, flagNameLength, flagNameChar } } - return utils.RandomString(n, charset, true), nil + return utils.RandomString(n, charset), nil } func termReadConfirmation(flags *pflag.FlagSet, name, prompt, confirmation string) (confirmed bool, err error) { diff --git a/internal/notification/smtp_notifier.go b/internal/notification/smtp_notifier.go index 0af580b73..34fcbd7b2 100644 --- a/internal/notification/smtp_notifier.go +++ b/internal/notification/smtp_notifier.go @@ -6,6 +6,7 @@ import ( "crypto/x509" "fmt" "net/mail" + "os" "strings" "github.com/sirupsen/logrus" @@ -19,13 +20,29 @@ import ( // NewSMTPNotifier creates a SMTPNotifier using the notifier configuration. func NewSMTPNotifier(config *schema.SMTPNotifierConfiguration, certPool *x509.CertPool) *SMTPNotifier { + var tlsconfig *tls.Config + + if config.TLS != nil { + tlsconfig = utils.NewTLSConfig(config.TLS, certPool) + } + opts := []gomail.Option{ gomail.WithPort(config.Port), - gomail.WithTLSConfig(utils.NewTLSConfig(config.TLS, certPool)), + gomail.WithTLSConfig(tlsconfig), gomail.WithHELO(config.Identifier), + gomail.WithTimeout(config.Timeout), + gomail.WithoutNoop(), + } + + ssl := config.Port == smtpPortSUBMISSIONS + + if ssl { + opts = append(opts, gomail.WithSSL()) } switch { + case ssl: + break case config.DisableStartTLS: opts = append(opts, gomail.WithTLSPolicy(gomail.NoTLS)) case config.DisableRequireTLS: @@ -34,10 +51,6 @@ func NewSMTPNotifier(config *schema.SMTPNotifierConfiguration, certPool *x509.Ce opts = append(opts, gomail.WithTLSPolicy(gomail.TLSMandatory)) } - if config.Port == smtpPortSUBMISSIONS { - opts = append(opts, gomail.WithSSL()) - } - var domain string at := strings.LastIndex(config.Sender.Address, "@") @@ -89,9 +102,11 @@ func (n *SMTPNotifier) StartupCheck() (err error) { func (n *SMTPNotifier) Send(ctx context.Context, recipient mail.Address, subject string, et *templates.EmailTemplate, data any) (err error) { msg := gomail.NewMsg( gomail.WithMIMEVersion(gomail.Mime10), - gomail.WithBoundary(utils.RandomString(30, utils.CharSetAlphaNumeric, true)), + gomail.WithBoundary(utils.RandomString(30, utils.CharSetAlphaNumeric)), ) + setMessageID(msg, n.domain) + if err = msg.From(n.config.Sender.String()); err != nil { return fmt.Errorf("notifier: smtp: failed to set from address: %w", err) } @@ -143,3 +158,12 @@ func (n *SMTPNotifier) Send(ctx context.Context, recipient mail.Address, subject return nil } + +func setMessageID(msg *gomail.Msg, domain string) { + rn, _ := utils.RandomInt(100000000) + rm, _ := utils.RandomInt(10000) + rs := utils.RandomString(17, utils.CharSetAlphaNumeric) + pid := os.Getpid() + rm + + msg.SetMessageIDWithValue(fmt.Sprintf("%d.%d%d.%s@%s", pid, rn, rm, rs, domain)) +} diff --git a/internal/server/template.go b/internal/server/template.go index 02928f4ff..d86841005 100644 --- a/internal/server/template.go +++ b/internal/server/template.go @@ -59,7 +59,7 @@ func ServeTemplatedFile(publicDir, file string, opts *TemplatedFileOptions) midd ctx.SetContentTypeTextPlain() } - nonce := utils.RandomString(32, utils.CharSetAlphaNumeric, true) + nonce := utils.RandomString(32, utils.CharSetAlphaNumeric) switch { case publicDir == assetsSwagger: diff --git a/internal/utils/crypto.go b/internal/utils/crypto.go index 6d74fc8ea..0d0f3cfd9 100644 --- a/internal/utils/crypto.go +++ b/internal/utils/crypto.go @@ -573,3 +573,50 @@ loop: return extKeyUsage } + +// RandomString returns a random string with a given length with values from the provided characters. When crypto is set +// to false we use math/rand and when it's set to true we use crypto/rand. The crypto option should always be set to true +// excluding when the task is time sensitive and would not benefit from extra randomness. +func RandomString(n int, characters string) (randomString string) { + return string(RandomBytes(n, characters)) +} + +// RandomBytes returns a random []byte with a given length with values from the provided characters. When crypto is set +// to false we use math/rand and when it's set to true we use crypto/rand. The crypto option should always be set to true +// excluding when the task is time sensitive and would not benefit from extra randomness. +func RandomBytes(n int, characters string) (bytes []byte) { + bytes = make([]byte, n) + + _, _ = rand.Read(bytes) + + for i, b := range bytes { + bytes[i] = characters[b%byte(len(characters))] + } + + return bytes +} + +func RandomInt(n int) (int, error) { + if n <= 0 { + return 0, fmt.Errorf("n must be more than 0") + } + + max := big.NewInt(int64(n)) + + if max.IsUint64() { + return 0, fmt.Errorf("generated max is uint64") + } + + value, err := rand.Int(rand.Reader, max) + if err != nil { + return 0, err + } + + output := int(value.Int64()) + + if output < 0 { + return 0, fmt.Errorf("generated number is too big for int") + } + + return output, nil +} diff --git a/internal/utils/hashing_test.go b/internal/utils/hashing_test.go index cf016820b..c6777189e 100644 --- a/internal/utils/hashing_test.go +++ b/internal/utils/hashing_test.go @@ -22,7 +22,7 @@ func TestShouldHashString(t *testing.T) { assert.Equal(t, "ae448ac86c4e8e4dec645729708ef41873ae79c6dff84eff73360989487f08e5", anotherSum) assert.NotEqual(t, sum, anotherSum) - randomInput := RandomString(40, CharSetAlphaNumeric, false) + randomInput := RandomString(40, CharSetAlphaNumeric) randomSum := HashSHA256FromString(randomInput) assert.NotEqual(t, randomSum, sum) @@ -38,7 +38,7 @@ func TestShouldHashPath(t *testing.T) { err = os.WriteFile(filepath.Join(dir, "anotherfile"), []byte("another\n"), 0600) assert.NoError(t, err) - err = os.WriteFile(filepath.Join(dir, "randomfile"), []byte(RandomString(40, CharSetAlphaNumeric, true)+"\n"), 0600) + err = os.WriteFile(filepath.Join(dir, "randomfile"), []byte(RandomString(40, CharSetAlphaNumeric)+"\n"), 0600) assert.NoError(t, err) sum, err := HashSHA256FromPath(filepath.Join(dir, "myfile")) diff --git a/internal/utils/strings.go b/internal/utils/strings.go index fd13f66d9..142c723d3 100644 --- a/internal/utils/strings.go +++ b/internal/utils/strings.go @@ -1,13 +1,10 @@ package utils import ( - crand "crypto/rand" "fmt" - "math/rand" "net/url" "strconv" "strings" - "time" "unicode" "github.com/valyala/fasthttp" @@ -211,32 +208,6 @@ func StringSlicesDelta(before, after []string) (added, removed []string) { return added, removed } -// RandomString returns a random string with a given length with values from the provided characters. When crypto is set -// to false we use math/rand and when it's set to true we use crypto/rand. The crypto option should always be set to true -// excluding when the task is time sensitive and would not benefit from extra randomness. -func RandomString(n int, characters string, crypto bool) (randomString string) { - return string(RandomBytes(n, characters, crypto)) -} - -// RandomBytes returns a random []byte with a given length with values from the provided characters. When crypto is set -// to false we use math/rand and when it's set to true we use crypto/rand. The crypto option should always be set to true -// excluding when the task is time sensitive and would not benefit from extra randomness. -func RandomBytes(n int, characters string, crypto bool) (bytes []byte) { - bytes = make([]byte, n) - - if crypto { - _, _ = crand.Read(bytes) - } else { - _, _ = rand.Read(bytes) //nolint:gosec // As this is an option when using this function it's not necessary to be concerned about this. - } - - for i, b := range bytes { - bytes[i] = characters[b%byte(len(characters))] - } - - return bytes -} - // StringHTMLEscape escapes chars for a HTML body. func StringHTMLEscape(input string) (output string) { return htmlEscaper.Replace(input) @@ -307,7 +278,3 @@ func IsURLHostComponentWithPort(u url.URL) (isHostComponentWithPort bool) { return false } - -func init() { - rand.Seed(time.Now().UnixNano()) -} diff --git a/internal/utils/strings_test.go b/internal/utils/strings_test.go index c086dd0ae..49d3a3f69 100644 --- a/internal/utils/strings_test.go +++ b/internal/utils/strings_test.go @@ -54,14 +54,10 @@ func TestStringJoinDelimitedEscaped(t *testing.T) { } func TestShouldNotGenerateSameRandomString(t *testing.T) { - randomStringOne := RandomString(10, CharSetAlphaNumeric, false) - randomStringTwo := RandomString(10, CharSetAlphaNumeric, false) - - randomCryptoStringOne := RandomString(10, CharSetAlphaNumeric, true) - randomCryptoStringTwo := RandomString(10, CharSetAlphaNumeric, true) + randomStringOne := RandomString(10, CharSetAlphaNumeric) + randomStringTwo := RandomString(10, CharSetAlphaNumeric) assert.NotEqual(t, randomStringOne, randomStringTwo) - assert.NotEqual(t, randomCryptoStringOne, randomCryptoStringTwo) } func TestShouldDetectAlphaNumericString(t *testing.T) {