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.

Sign up for GitHub

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

Jump to bottom

Production-readiness modifications #13

Closed
wants to merge 17 commits into from
Closed

Production-readiness modifications #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
150b377
Allow reading remotePass from env var
electron0zero Feb 1, 2021
546ce12
Merge pull request #1 from grafana/remote_pass
electron0zero Feb 1, 2021
7583cfd
Add support for any network + improve logs & errors
electron0zero Feb 3, 2021
4ca35c1
Merge pull request #4 from grafana/logs_and_net_check
electron0zero Feb 3, 2021
7cf65e6
Adding attempt and failure metrics
Feb 8, 2021
df12d96
Adding duration metric
Feb 8, 2021
ac8d53e
Making metrics address configurable
Feb 8, 2021
d204de6
Merge pull request #9 from grafana/dannykopping/metrics
Feb 8, 2021
53212b6
Moving metrics_listen config option into more appropriate position
Feb 9, 2021
04e7486
Refactoring, adding structured logging
Feb 9, 2021
715ce30
Writing logs to stdout
Feb 9, 2021
de8dc1f
Handling TERM and INT signals
Feb 9, 2021
002d3d3
Implementing structured logs (#10)
Feb 10, 2021
bb35bb5
Merge remote-tracking branch 'origin/master' into dannykopping/exit_s…
Feb 10, 2021
8584588
Merge pull request #11 from grafana/dannykopping/exit_signals
Feb 10, 2021
570fc22
Merge remote-tracking branch 'upstream/master' into upsteam-changes
Mar 9, 2021
355f3c7
Minor refactorings after merging upstream
Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 38 additions & 16 deletions config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,57 @@ package main

import (
"flag"
"os"

"github.com/vharitonsky/iniflags"
)

var (
VERSION = "1.6.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is up to the maintainer to change when a new version is tagged.

Copy link
Author

Choose a reason for hiding this comment

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

This was my bad - this constant got lost in the merge so I restored it and just bumped the version for no good reason

appVersion = "unknown"
buildTime = "unknown"
)

var (
logFile = flag.String("logfile", "/var/log/smtprelay.log", "Path to logfile")
hostName = flag.String("hostname", "localhost.localdomain", "Server hostname")
welcomeMsg = flag.String("welcome_msg", "", "Welcome message for SMTP session")
listen = flag.String("listen", "127.0.0.1:25 [::1]:25", "Address and port to listen for incoming SMTP")
localCert = flag.String("local_cert", "", "SSL certificate for STARTTLS/TLS")
localKey = flag.String("local_key", "", "SSL private key for STARTTLS/TLS")
localForceTLS = flag.Bool("local_forcetls", false, "Force STARTTLS (needs local_cert and local_key)")
allowedNets = flag.String("allowed_nets", "127.0.0.1/8 ::1/128", "Networks allowed to send mails")
allowedSender = flag.String("allowed_sender", "", "Regular expression for valid FROM EMail addresses")
logFile = flag.String("logfile", "/dev/stdout", "Path to logfile")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than use /dev/stdout, which isn't necessarily portable, why not just interpret an empty string to mean stdout? This would require just a little more work in setupLogger() (to skip the os.OpenFile() and just tell logrus to use stdout).

Copy link
Author

Choose a reason for hiding this comment

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

Not fan of an empty value defaulting to /dev/stdout for the same reason.
I think we can revert back to the dedicated log file and folks can override as they see fit.

Copy link
Owner

Choose a reason for hiding this comment

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

The problem is one we had before already. There was some bad sed magic rewriting the config file for windows builds in the old release script. I nuked this with the migration to GitHub Action for release builds.

For the moment I would like to leave it as it is.

hostName = flag.String("hostname", "localhost.localdomain", "Server hostname")
welcomeMsg = flag.String("welcome_msg", "", "Welcome message for SMTP session")
listen = flag.String("listen", "127.0.0.1:25 [::1]:25", "Address and port to listen for incoming SMTP")
metricsListen = flag.String("metrics_listen", ":8080", "Address and port to listen for metrics exposition")
Copy link
Owner

Choose a reason for hiding this comment

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

This should be disabled per default. With :8080 it listens on all available interfaces which might be safe in a docker environment but not on real servers with public interfaces!

localCert = flag.String("local_cert", "", "SSL certificate for STARTTLS/TLS")
localKey = flag.String("local_key", "", "SSL private key for STARTTLS/TLS")
localForceTLS = flag.Bool("local_forcetls", false, "Force STARTTLS (needs local_cert and local_key)")
// set allowed_nets to "" to allow any Networks (i.e disable network check)
Copy link
Owner

Choose a reason for hiding this comment

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

This comments seem to freak out gofmt so it leads to a lot a lot of noise. Do we really need them?

allowedNets = flag.String("allowed_nets", "127.0.0.1/8 ::1/128", "Networks allowed to send mails, use \"\" to disable")
// set "" to allow any sender (i.e disable sender check)
allowedSender = flag.String("allowed_sender", "", "Regular expression for valid FROM EMail addresses")
// set "" to allow any recipients (i.e disable recipients check)
allowedRecipients = flag.String("allowed_recipients", "", "Regular expression for valid TO EMail addresses")
allowedUsers = flag.String("allowed_users", "", "Path to file with valid users/passwords")
remoteHost = flag.String("remote_host", "smtp.gmail.com:587", "Outgoing SMTP server")
remoteUser = flag.String("remote_user", "", "Username for authentication on outgoing SMTP server")
remotePass = flag.String("remote_pass", "", "Password for authentication on outgoing SMTP server")
remoteAuth = flag.String("remote_auth", "plain", "Auth method on outgoing SMTP server (plain, login)")
remoteSender = flag.String("remote_sender", "", "Sender e-mail address on outgoing SMTP server")
versionInfo = flag.Bool("version", false, "Show version information")
// set "" to allow any user (i.e disable users check)
allowedUsers = flag.String("allowed_users", "", "Path to file with valid users/passwords")
remoteHost = flag.String("remote_host", "smtp.gmail.com:587", "Outgoing SMTP server")
remoteUser = flag.String("remote_user", "", "Username for authentication on outgoing SMTP server")
// REMOTE_PASS env var can also be used to set remotePass
remotePass = flag.String("remote_pass", "", "Password for authentication on outgoing SMTP server")
remoteAuth = flag.String("remote_auth", "plain", "Auth method on outgoing SMTP server (plain, login)")
remoteSender = flag.String("remote_sender", "", "Sender e-mail address on outgoing SMTP server")
versionInfo = flag.Bool("version", false, "Show version information")
logLevel = flag.String("log_level", "debug", "Minimum log level to output")
)

func ConfigLoad() {
iniflags.Parse()

setupLogger()

// if remotePass is not set, try reading it from env var
Copy link
Owner

Choose a reason for hiding this comment

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

I think we talked about this before somewhere and this definitely should be solved for all config parameters not only remotePass. I am open for suggestions on this point.

The research that I did so far did lead me to https://github.com/peterbourgon/ff which would be almost compatible from a code point of view but with a different config file format so we would need a slightly modified parser (which seems to be simple https://github.com/peterbourgon/ff/blob/master/parse.go#L236 )

Recently there was also a PR #14 which requests the same feature.

if *remotePass == "" {
log.Debug("remote_pass not set, trying REMOTE_PASS env var")
*remotePass = os.Getenv("REMOTE_PASS")
if *remotePass != "" {
log.Debug("found data in REMOTE_PASS env var")
} else {
log.Debug("no data found in REMOTE_PASS env var")
}
}
}
3 changes: 3 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ module github.com/decke/smtprelay

require (
github.com/chrj/smtpd v0.2.0
github.com/google/uuid v1.2.0
github.com/prometheus/client_golang v1.9.0
github.com/sirupsen/logrus v1.7.0
github.com/vharitonsky/iniflags v0.0.0-20180513140207-a33cd0b5f3de
golang.org/x/crypto v0.0.0-20201221181555-eec23a3978ad
)
Expand Down
Loading