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

Panic if temporary SMTP-Problem #4528

Open
Subito opened this issue Apr 29, 2024 · 2 comments
Open

Panic if temporary SMTP-Problem #4528

Subito opened this issue Apr 29, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@Subito
Copy link

Subito commented Apr 29, 2024

Subject of the issue

Sometimes on trying to send a 2FA-Mail, the SMTP Server replies with 454: 4.7.0 Temporary authentication failure, which should indicate to the sender "please try later, this is a temporary problem". However vaultwarden panics if it receives this error and the server stops.

Deployment environment

  • vaultwarden version: 1.30.3
  • Install method: FreeBSD pkg

  • Clients used: happens on all clients on Login

  • Reverse proxy and version: nginx

  • Other relevant details:

Expected behaviour

Server does not crash if it receives a temporary SMTP-Failure.

Actual behaviour

Server crashes and needs restarting.

Troubleshooting data

Apr 29 16:13:58 vault1 vaultwarden[66287]: [2024-04-29 16:13:58.251][vaultwarden::mail][ERROR] SMTP 4xx error: transient error (454): 4.7.0 Temporary authentication failure: Connection lost to authentication server
Apr 29 16:13:58 vault1 vaultwarden[66287]: [2024-04-29 16:13:58.252][panic][ERROR] thread 'tokio-runtime-worker' panicked at 'Error sending incomplete 2FA email: SMTP 4xx error: transient error (454): 4.7.0 Temporary authentication failure: Connection lost to authentication server': src/api/core/two_factor/mod.rs:273
@BlackDex
Copy link
Collaborator

BlackDex commented Apr 29, 2024

Vaultwarden does not have a retry mechanisch for these kind of scenarios. The message indicates a connection error.

Most of the time temp errors should be tried again after a few minutes, but this is not something Vaultwarden handles. I also do not thing Vaultwarden should handle those and try again actually. That kind of functionality belongs to mail servers, not clients.

We probably can catch this error and return an error instead of a panic though, that is probably nicer then a panic.

@Subito
Copy link
Author

Subito commented Apr 30, 2024

Of course, just catching the error would be great. As you said: Any sort of queue-and-retry should be handled on a mailserver. But panicing on a simple SMTP-Error is probably not great. Just logging an error would be perfect.

@BlackDex BlackDex added the bug Something isn't working label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants