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.

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

vdk-core: Set sender when checking if email exists #2376

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

doks5
Copy link
Contributor

@doks5 doks5 commented Jul 10, 2023

Currently, when a target email address is checked if exists, we set a dummy sender "whatever". This works in cases, where the smtp server does not enforce validation checks on the sender. It does not, however, work in cases when the smtp server uses TLS (like Office365).

In such cases, the check of the target email address fails with Status 503: Bad sequence of commands, because the .mail("whatever") method call sets the FROM field to mail FROM:, and this causes the smtp server to return 501 5.1.7 Invalid address.

This change replces "whatever" with the actual sender address used to send the email notifications (set through the VDK_NOTIFICATION_SENDER) environment variable.

Testing Done:
Create a sample email script to first better understand the error, and second to verify that the fix actually works.

Script is:

HOST = "smtp.office365.com"
PORT = 587
SENDER = "<valid-address-as-set-by-VDK_NOTIFICATION_SENDER>"
USERNAME = "<sender-user-as-set-by-VDK_NOTIFICATION_SMTP_LOGIN_USERNAME>"
PASSWORD = "<password-as-set-by-VDK_NOTIFICATION_SMTP_LOGIN_PASSWORD>"
TARGET_ADDRESS = "<some-valid-email-address>"

s = smtplib.SMTP(HOST, PORT)
s.starttls()
s.login(USERNAME, PASSWORD)
s.set_debuglevel(1)

try:
    s.helo()
    # s.mail(SENDER)               # Test the actual fix
    s.mail("whatever")             # Check the full error
    resp = s.rcpt(TARGET_ADDRESS)
    print(resp)
except smtplib.SMTPServerDisconnected as e:
    print(f"SMTP connection error while checking if email address exists: {e}")
finally:
    s.quit()

@doks5 doks5 self-assigned this Jul 10, 2023
Currently, when a target email address is checked if exists, we set a dummy sender
`"whatever"`. This works in cases, where the smtp server does not enforce validation
checks on the sender. It does not, however, work in cases when the smtp server uses
TLS (like Office365).

In such cases, the check of the target email address fails with Status
`503: Bad sequence of commands`, because the `.mail("whatever")` method call sets the
**FROM** field to *mail FROM:<whatever>*, and this causes the smtp server to return
`501 5.1.7 Invalid address`.

This change replces `"whatever"` with the actual sender address used to send the email
notifications (set through the `VDK_NOTIFICATION_SENDER`) environment variable.

Testing Done:
Create a sample email script to first better understand the error, and second to verify
that the fix actually works.

Script is:
```python
HOST = "smtp.office365.com"
PORT = 587
SENDER = "<valid-address-as-set-by-VDK_NOTIFICATION_SENDER>"
USERNAME = "<sender-user-as-set-by-VDK_NOTIFICATION_SMTP_LOGIN_USERNAME>"
PASSWORD = "<password-as-set-by-VDK_NOTIFICATION_SMTP_LOGIN_PASSWORD>"
TARGET_ADDRESS = "<some-valid-email-address>"

s = smtplib.SMTP(HOST, PORT)
s.starttls()
s.login(USERNAME, PASSWORD)
s.set_debuglevel(1)

try:
    s.helo()
    # s.mail(SENDER)               # Test the actual fix
    s.mail("whatever")             # Check the full error
    resp = s.rcpt(TARGET_ADDRESS)
    print(resp)
except smtplib.SMTPServerDisconnected as e:
    print(f"SMTP connection error while checking if email address exists: {e}")
finally:
    s.quit()
```

Signed-off-by: Andon Andonov <andonova@vmware.com>
@doks5 doks5 force-pushed the person/andonova/bug-fix-notif-config branch from 7c924ec to 9b45fef Compare July 11, 2023 14:50
@doks5 doks5 merged commit 08d442a into main Jul 11, 2023
7 checks passed
@doks5 doks5 deleted the person/andonova/bug-fix-notif-config branch July 11, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants