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

Option to opt out of message Prepare in SmtpClient.SendAsync #891

Closed
nitinag opened this issue Aug 5, 2019 · 14 comments
Closed

Option to opt out of message Prepare in SmtpClient.SendAsync #891

nitinag opened this issue Aug 5, 2019 · 14 comments
Labels
question A question about how to do something

Comments

@nitinag
Copy link
Sponsor

nitinag commented Aug 5, 2019

We're looking at using MimeKit to accept incoming messages and forward/relay them.

During forward/relay, we would like the option to force enable 8BITMIME after connect (recommended at https://cr.yp.to/smtp/8bitmime.html and other places), but because SmtpClient.Capabilities does not allow enabling capabilities, this is not currently possible. I could also see it useful to force enable PIPELINING for testing. We're going to be forwarding messages (many DKIM signed) and thus would want messages to stay in exactly the same source format regardless of destination server supporting 8BITMIME or not. This does raise the issue of BODY being added to MAIL FROM when 8BITMIME is not advertised, which I guess we'd not want to be added either if the server doesn't advertise it, but we'd still want to send as 8BITMIME. That may thus require a different solution, but I feel enabling capabilities is still useful.

Given we're forwarding DKIM signed messages, we'll want to add a few headers on top and send the remaining message as is. Can we expect MailKit/MimeKit to not modify the message/headers/body other than the few headers we explicitly add during SendAsync? What about MimeMessage.WriteTo?

I'm aware we'll need to clear MimeKit.FormatOptions.HiddenHeaders. Possibly set some of the other FormatOptions? What about Body.Prepare done inside of SmtpClient.SendAsync? Or should we just consider sending the messages directly from the raw file, add our headers on top manually, using our own tcp stream (maybe after using MailKit to handle the initial connection)?

I'd also like to suggest adding a methods to send a custom commands in SmtpClient. Though I guess one could just pass their own tcp stream and send/read a custom command when necessary, but then they'd need to parse/handle the smtp response.

We'll be using the persistent flag as well for lower memory usage during sending.

@jstedfast jstedfast added the question A question about how to do something label Aug 5, 2019
@jstedfast
Copy link
Owner

During forward/relay, we would like the option to force enable 8BITMIME after connect

Why would you need to force enable it? If the server supports it, MailKit will use it. If it doesn't, well, having an API in MailKit to "force enable" it wouldn't work. MailKit doesn't have Darth Vader super powers.

This does raise the issue of BODY being added to MAIL FROM when 8BITMIME is not advertised, which I guess we'd not want to be added either if the server doesn't advertise it, but we'd still want to send as 8BITMIME.

What would this mysterious SmtpClient.ForceEnable8BITMIME() method do?

Given we're forwarding DKIM signed messages, we'll want to add a few headers on top and send the remaining message as is. Can we expect MailKit/MimeKit to not modify the message/headers/body other than the few headers we explicitly add during SendAsync? What about MimeMessage.WriteTo?

Yes and yes.

I'm aware we'll need to clear MimeKit.FormatOptions.HiddenHeaders.

No, don't do that unless the message you received had Bcc or Content-Length headers that were signed (which should never happen).

What about Body.Prepare done inside of SmtpClient.SendAsync?

Don't worry about that unless the SMTP server doesn't support the 8BITMIME and/or BINARYMIME extensions that you'll need in order to send the message without alteration.

I'd also like to suggest adding a methods to send a custom commands in SmtpClient.

What for? You haven't given me a reason it's needed.

@nitinag
Copy link
Sponsor Author

nitinag commented Aug 5, 2019

Why would you need to force enable it? If the server supports it, MailKit will use it. If it doesn't, well, having an API in MailKit to "force enable" it wouldn't work.

"Do not implement Q-P conversion in an SMTP client. You will find that simply sending an 8-bit message is much more successful than attempting Q-P conversion, whether or not the server announces 8BITMIME. Announce 8BITMIME in your server, to prevent Q-P conversion from sendmail without the 8 flag. Ignore the BODY information."
https://cr.yp.to/smtp/8bitmime.html

"Exim and qmail do not translate eight-bit messages to seven-bit when making an attempt to relay 8-bit data to non-8BITMIME peers, as is required by the RFC.[4] This does not cause problems in practice, since virtually all modern mail relays are 8-bit clean.[5]"
https://en.wikipedia.org/wiki/Extended_SMTP

What would this mysterious SmtpClient.ForceEnable8BITMIME() method do?

While reviewing the source code for Smtp.SendAsync, it had seemed to me that SendAsync could change the encoding of the message based on server support if 8BITMIME by calling message.Prepare. What may work is a setting or option for it not to perform 8bit to 7bit conversion during SendAsync regardless or server support of 8BITMIME for the reasons mentioned above, which also helps ensure not breaking DKIM.

It would also be useful to have an option for the forced format.International override in SendAsync if server doesn't support SMTPUTF8 for the same DKIM non-breaking reasons. We'd want it to use ASCII for SMTP commands when there is no SMTPUTF8 server support, but send the message itself without converting / encoding the message headers from UTF8 to ASCII, during the DATA/BDAT command. For example, Postfix (http://www.postfix.org/SMTPUTF8_README.html) does not seem to modify the message headers for pre-existing non-ascii email flows.

Basically we want to ensure MailKit does not modify the message (other than to add our additional headers on top) when sending to another SMTP server so DKIM does not break. I found three main areas (8BITMIME, SMTPUTF8, and FormatOptions.HiddenHeaders) that could modify the message and we'd want to prevent during send. Is there anything else I may have missed?

I'd also like to suggest adding a methods to send a custom commands in SmtpClient.
What for? You haven't given me a reason it's needed.

I've used this in the past to run custom X smtp server commands. See Private-Use Commands at https://tools.ietf.org/html/rfc5321#section-4.1.5. It would also require the ability to get the response to the custom command.

@jstedfast
Copy link
Owner

It would make more sense to just not Prepare() the message if any DKIM-like signature header is found, so I've committed a patch to do that.

As far as FormatOptions.International, MailKit does not reformat the headers unless the FormatOptions.International is true. In other words, as long as you use false, it will not alter the headers at all.

see here: https://github.com/jstedfast/MimeKit/blob/master/MimeKit/Header.cs#L1026

@jstedfast
Copy link
Owner

I've added logic to disable format.International if the message is DKIM (or ARC) signed.

@nitinag
Copy link
Sponsor Author

nitinag commented Aug 5, 2019

Thanks! Yes, that sounds like it would work for preventing DKIM/ARC breakage. However, I wonder if an explicit option may be useful to ensure it's forced for possible other cases since we're sure we don't ever want 8bit to 7bit conversion? For example, I had in my notes to look into whether 8bit to 7bit conversion would break/affect S/MIME or PGP as well.

Edit: Maybe an option to not call prepare during SendAsync so the caller can decide and call it as necessary on their own. When we create message for smtp or local storage, we'll call Prepare with (EncodingConstraint.None, MaxLineLength) to ensure max line length is correct, but for for forward/relay existing message won't call prepare at all since line length issues there shouldn't happen since we check it on incoming and we'll let it bounce if it does.

Edit: The current patch would remove the MaxLineLength part of prepare for all messages with DKIM, making it important to call Prepare before DKIM signing for any created messages (which from reading other issues, is something that should be done before DKIM signing anyway).

Sounds like FormatOptions.International is more an option on whether to force reformat the headers to UTF-8 on SendAsync/WriteTo? Maybe the docs for the FormatOptions.International property (and possibly the property name - though I guess that's harder) can be updated to clarify this and that if false, the current encoding of the headers won't change. For me, the docs for that property weren't clear on that and implied otherwise.

BTW, have you looked into signing up for GitHub Sponsors (https://github.com/sponsors)? The matching would be great.

@nitinag nitinag changed the title Relaying messages using SmtpClient Option to opt out of message Prepare in SmtpClient.SendAsync Aug 6, 2019
jstedfast added a commit that referenced this issue Aug 7, 2019
This will allow subclasses of SmtpClient to extend the functionality
by adding support for custom SMTP command extensions.

See issue #891 for feature request.
@jstedfast
Copy link
Owner

I've added protected SendCommand and SendCommandAsync methods.

Once I get my Windows machine back up and running (since my move, I no longer have a way to get wired internet to my Windows desktop so need to order a wireless adaptor or something), I'll be releasing v2.3.0 with these changes.

@jstedfast
Copy link
Owner

BTW, thanks for the donation :)

@nitinag
Copy link
Sponsor Author

nitinag commented Aug 7, 2019

Great!

Any thoughts on on the ability to opt out of the message prepare on SendAsync completely as per the notes in my last message and additional cases (S/MIME, etc.) where that applies?

Also when calling Prepare 8bit on a message that is already 7bit, does it actually transform/convert it to 8bit or does it just ensure it does not contain any zero-bytes like the docs state?

@jstedfast
Copy link
Owner

It's not needed for S/MIME or PGP because Prepare() does not modify S/MIME or PGP (signed) parts.

Calling Prepare(8bit) on a message that is already prepared for 7bit will not modify the message. It only makes sure that the parts will fit within the constraint and forces them into that constraint if they don't already fit.

For example:

  1. a part that already fits within a 7bit constraint will fit within any constraint w/o needing to be modified, so no changes are made no matter what the constraint is.
  2. a part that already fits within an 8bit constraint will fit within a 8bit and binary constraints but not a 7bit constraint. If the constraint is 7bit, then it will be encoded but if the constraint is 8bit or binary, then it will not be changed.

Does that make sense?

@nitinag
Copy link
Sponsor Author

nitinag commented Aug 7, 2019

Yes, perfect. Thanks for both those answers.

Knowing it won't modify S/MIME or PGP at least removes that part of the concern. I still feel more comfortable turning off the conversion fully based on the linked advice + consistency with other servers, so I don't have to be concerned about unknown cases and will have consistency in debugging potential issues that come up given we're already not doing the conversion in certain cases.

If adding this option doesn't make sense for the library itself, what would be the lightest way for me to opt out of the SendAsync prepare? The last resort would be to fork and maintain a copy, but that would be best avoided. Could I simply subclass and override just the SendAsync with mostly the same code minus the prepare? Any other ideas?

@jstedfast
Copy link
Owner

I've already modified the Send() methods to not call Prepare() if any DKIM or ARC headers are present. What other cases is this needed for?

@nitinag
Copy link
Sponsor Author

nitinag commented Aug 7, 2019

Our case is: as a MTA - relaying/queue delivery smtp server, we don't want to perform this conversion at all, which matches the behavior of Exim and qmail who also don't perform this conversion.

@jstedfast
Copy link
Owner

The above change should allow you to override the Prepare() method in your custom SmtpClient subclass to be a no-op.

@nitinag
Copy link
Sponsor Author

nitinag commented Aug 7, 2019

Excellent, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question A question about how to do something
Projects
None yet
Development

No branches or pull requests

2 participants