8000 Rework Send-MailMessage cmdlet with MailKit by ThreeFive-O · Pull Request #10246 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Rework Send-MailMessage cmdlet with MailKit#10246

Closed
ThreeFive-O wants to merge 4 commits intoPowerShell:masterfrom
ThreeFive-O:Send-MailMessage_MailKit
Closed

Rework Send-MailMessage cmdlet with MailKit#10246
ThreeFive-O wants to merge 4 commits intoPowerShell:masterfrom
ThreeFive-O:Send-MailMessage_MailKit

Conversation

@ThreeFive-O
Copy link
Contributor
@ThreeFive-O ThreeFive-O commented Jul 29, 2019

PR Summary

This PR will fix the underlying architecture of the Send-MailMessage cmdlet using the actively maintained MailKit SMTP client.
Note: System.Net.Mail.SmtpClient receives no further development and only supports STARTTLS secure connections.

Enhancements

Benefits of MailKit

Backward compatibility and breaking changes
All parameters are left as they are, BUT

  • Credential parameter is the only way to provide credentials right now. System.Net.Mail.SmtpClient has the ability to get the default credentials from Windows.
    -> Requires change in documentation; X-plat credential provider solution for PowerShell 7 might be the solution to that.

  • DeliverNotificationOption is implemented, but the functionality should be reviewed as all tested platforms either don't support Delivery Status Notfication (DSN) (e.g. Gmail, GMX, Yahoo) or ignore the setting (Outlook.com - btw the only SMTP server I've came accross which reports DSN capability).
    -> Subject to discuss in RFC to deprecate this parameter.

  • Encoding parameter no longer has any influence on subject/body encoding as this is handled by MimeKit
    -> Requires further research, but may be subject to deprecate.

  • UseSsl is no longer mandatory to allow STARTTLS upgrade from an insecure connection. The parameter is now used to explicitly state the the entire connection should be wrapped in a SSL/TLS connections, also known as SMTPS. -> Using Port 0 (default - automatic port selection will use Port 465 for UseSsl but can of course be overridden with any other port number; If UseSsl is not specified the default port 25 for SMTP is used, although most STARTTLS endpoints use port 587).
    -> Critical breaking change for automated scripts; Subject to discuss
    Possible workaround: Use another parameter for SMTPS functionality and keep UseSsl parameter for STARTTLS (though it wouldn't have any relevance in the code and would be a non-functional parameter like Encoding, though not breaking any scripts)

Test infrastructure
The Pester tests use netDumbster which only supports insecure connections.
STARTTLS and SMTPS functionality have been tested interactively against:

  • Outlook.com (STARTTLS 587)
  • Gmail.com (SMTPS 465)
  • Yahoo.com (SMTPS 465 and STARTTLS 587)

PR Context

As of #9178 is marked as obsolete and an RFC PR PowerShell/PowerShell-RFC#160 was opened to discuess the future of the cmdlet.

PR Checklist

@ThreeFive-O
Copy link
Contributor Author

@TravisEz13 Tests for Remoting are failing for Windows ElevatedPesterTests due to missing dependencies:
New-PSSession : [localhost] Processing data from remote server localhost failed with the following error message: Could not load file or assembly 'MailKit, Version=2.2.0.0, Culture=neutral, PublicKeyToken=4e064fe7c44a8f1b'. The system cannot find the file specified. For more information, see the about_Remote_Troubleshooting Help topic.
Could you please point me in the right direction how to resolve this dependency issue? Thank you very much.

< 8000 a class="d-inline-block" data-hovercard-type="user" data-hovercard-url="/users/TravisEz13/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="/TravisEz13">@TravisEz13
Copy link
Member

cc @SteveL-MSFT about if this is the right place for this

@TravisEz13 TravisEz13 added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Aug 2, 2019
@TravisEz13
Copy link
Member
TravisEz13 commented Aug 2, 2019

We have a list of Assemblies trusted for remoting here https://github.com/PowerShell/PowerShell-Native/blob/534965cb5f5657a019815216a09de419dc6eec91/src/powershell-native/nativemsh/pwrshcommon/pwrshcommon.cpp#L671

Work with @adityapatwardhan on how you can progress. My proposal would be that we publish a pre-release of PowerShell-Native with your changes and you can work off of that.

GitHub
Contribute to PowerShell/PowerShell-Native development by creating an account on GitHub.

@SteveL-MSFT SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 2, 2019
@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this, we appreciate this work, but ask that this be a module published to PSGallery. A decision whether this ships in PowerShell will be decided later.

  • We have concerns about potential for breaking changes (types, functionality)
  • Size concerns bringing in another assembly
  • In general, our direction is to have more modules/cmdlets validated via PSGallery before deciding it should be shipped with PowerShell

@SteveL-MSFT SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Aug 7, 2019
@TravisEz13 TravisEz13 added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 24, 2019
@ghost ghost added the Stale label Oct 25, 2019
@ghost
Copy link
ghost commented Oct 25, 2019

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs within 10 days of this comment.

@FireInWinter
Copy link

I don't agree that existing functionality should be moved to a separate module. It's too bad SMTPClient was deprecated, but this seems like a punishment to the community to remove a builtin feature and put it elsewhere.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HadErrors() returns $true if Send-MailMessage cmdlet is used Send-MailMessage incorrectly handling ValueFromPipelineByPropertyName parameters

4 participants

0