Skip to content

Expand allowedUsers email field to support comma-separated and domains #9

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

Merged
merged 7 commits into from
Feb 14, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
12 changes: 6 additions & 6 deletions auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ func AuthReady() bool {

// Returns bcrypt-hash, email
// email can be empty in which case it is not checked
func AuthFetch(username string) (string, string, error) {
func AuthFetch(username string) (string, []string, error) {
if !AuthReady() {
return "", "", errors.New("Authentication file not specified. Call LoadFile() first")
return "", nil, errors.New("Authentication file not specified. Call LoadFile() first")
}

file, err := os.Open(filename)
if err != nil {
return "", "", err
return "", nil, err
}
defer file.Close()

Expand All @@ -54,16 +54,16 @@ func AuthFetch(username string) (string, string, error) {
}

hash := parts[1]
email := ""
email := []string(nil)

if len(parts) >= 3 {
email = parts[2]
email = strings.Split(parts[2], ",")
}

return hash, email, nil
}

return "", "", errors.New("User not found")
return "", nil, errors.New("User not found")
}

func AuthCheckPassword(username string, secret string) error {
Expand Down
32 changes: 30 additions & 2 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,43 @@ func connectionChecker(peer smtpd.Peer) error {
return smtpd.Error{Code: 421, Message: "Denied"}
}

func addrAllowed(addr string, allowedAddrs []string) bool {
if allowedAddrs == nil {
return true
}

domidx := strings.LastIndex(addr, "@")
Copy link
Owner

Choose a reason for hiding this comment

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

Since addr is the sender email it shoud be valid that the address does not contain a domain and @ yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per below, I was unaware that "local" addresses could reach smtprelay. Updated (see below).

if domidx == -1 {
return false
}
domain := strings.ToLower(addr[domidx+1:])

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
addr = strings.ToLower(addr)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Applied (earlier in the function, so domain is lowercased, too).

for _, allowedAddr := range allowedAddrs {
allowedAddr = strings.ToLower(allowedAddr)

if strings.Index(allowedAddr, "@") == -1 {
Copy link
Owner

Choose a reason for hiding this comment

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

This is now a conflict because the absence of @ can be because it's a domain or a local address without domain yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per below, I was unaware that "local" addresses could reach smtprelay. Updated (see below).

if allowedAddr == domain {
return true
}
} else {
if allowedAddr == addr {
return true
}
}
}

return false
}

func senderChecker(peer smtpd.Peer, addr string) error {
// check sender address from auth file if user is authenticated
if *allowedUsers != "" && peer.Username != "" {
_, email, err := AuthFetch(peer.Username)
_, allowedAddrs, err := AuthFetch(peer.Username)
if err != nil {
return smtpd.Error{Code: 451, Message: "Bad sender address"}
}

if email != "" && strings.ToLower(addr) != strings.ToLower(email) {
if !addrAllowed(addr, allowedAddrs) {
Copy link
Owner

Choose a reason for hiding this comment

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

You've lost the strings.ToLower(addr) case but i think this should be done in the new addrAllowed()

Copy link
Collaborator Author

@JonathonReinhart JonathonReinhart Feb 14, 2021

Choose a reason for hiding this comment

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

Good catch. This was a result of the refactor. I've fixed that, and added a test (cabf848).

return smtpd.Error{Code: 451, Message: "Bad sender address"}
}
}
Expand Down
65 changes: 65 additions & 0 deletions main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package main

import (
"testing"
)

func TestAddrAllowedNoDomain(t *testing.T) {
allowedAddrs := []string{"[email protected]"}
if addrAllowed("bob.com", allowedAddrs) {
t.FailNow()
}
}

func TestAddrAllowedSingle(t *testing.T) {
allowedAddrs := []string{"[email protected]"}

if !addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
if addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
}

func TestAddrAllowedMulti(t *testing.T) {
allowedAddrs := []string{"[email protected]", "[email protected]"}
if !addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
if !addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
if addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
}

func TestAddrAllowedSingleDomain(t *testing.T) {
allowedAddrs := []string{"abc.com"}
if !addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
if addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
}

func TestAddrAllowedMixed(t *testing.T) {
allowedAddrs := []string{"[email protected]", "appsrv.example.com"}
if !addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
if addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
if !addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
if !addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
if addrAllowed("[email protected]", allowedAddrs) {
t.FailNow()
}
}