GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Rework Send-MailMessage cmdlet with MailKit by ThreeFive-O · Pull Request #10246 · PowerShell/PowerShell

Conversation

@ThreeFive-O

@ThreeFive-O ThreeFive-O commented Jul 29, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

  • Fix Send-MailMessage incorrectly handling ValueFromPipelineByPropertyName parameters #7591: Pipelining into the cmdlet is now supported
  • SMTPS is now supported

Benefits of MailKit

  • Modern actively maintained dependency (would fix HadErrors() returns $true if Send-MailMessage cmdlet is used #10015)
  • Secure connection upgrades from insecure connections using STARTTLS is supported implicitly (no parameter has to be specified)
  • Secure connections over SSL/TLS is supported (Gmail, Yahoo, ... see smtpServer requires secure connection or client was not authenticated. the server response was: 5.7.0 must issue STARTTLS command first #8987)
  • MailKit and MimeKit allow easier implementation of missing functionality (see Send-MailMessage add support for INLINE attachment  #9191)

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

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
  • Breaking changes
    • None
    • OR
    • Experimental feature(s) needed
      • Experimental feature name(s):
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed
      • Issue filed:
  • Testing - New and feature
    • N/A or can only be tested interactively
    • OR
    • Make sure you've added a new test if existing tests do not effectively test the code changed
  • Tooling
    • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
    • OR
    • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
      • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
      • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ThreeFive-O added 3 commits July 29, 2019 14:50
@ThreeFive-O
Rework Send-MailMessage cmdlet with MailKit
8417cae
@ThreeFive-O
Fix code styles
82a1f8d
@ThreeFive-O
Fix files.wxs with new dependencies
d7ae6f1
@ThreeFive-O

ThreeFive-O commented Jul 31, 2019

Copy link
Copy Markdown
Contributor Author

Travis Plunk (@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ThreeFive-O
Fix pipelining #7591
93c4adc
@TravisEz13

Travis Plunk (TravisEz13) commented Aug 2, 2019

Copy link
Copy Markdown
Member

cc Steve Lee (@SteveL-MSFT) about if this is the right place for this

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TravisEz13 Travis Plunk (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

Travis Plunk (TravisEz13) commented Aug 2, 2019 •
edited by unfurl-links Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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 Aditya Patwardhan (@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
PowerShell/PowerShell-Native
Contribute to PowerShell/PowerShell-Native development by creating an account on GitHub.
ThreeFive-O reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SteveL-MSFT Steve Lee (SteveL-MSFT) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 2, 2019
@ThreeFive-O ThreeFive-O mentioned this pull request Aug 5, 2019
Add MailKit and MimeKit as trusted assemblies PowerShell/PowerShell-Native#27
Closed
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Aug 7, 2019

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@SteveL-MSFT Steve Lee (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 Travis Plunk (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

Deleted user (ghost) commented Oct 25, 2019

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost ghost closed this Nov 4, 2019
@keschnei42 keschnei42 mentioned this pull request Feb 21, 2020
Default timeout when SMTP "Quit" is sent? MicrosoftDocs/PowerShell-Docs#5461
Closed
@FireInWinter

FireInWinter commented Feb 27, 2020

Copy link
Copy Markdown

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.

lukeb1961, A9G-Data-Droid, and BladeFireLight reacted with thumbs up emoji VertigoRay reacted with thumbs down emoji
All reactions
  • 👍 3 reactions
  • 👎 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@ghost ghost removed the Stale label Feb 27, 2020
@TaoHuaLiu Hugo (TaoHuaLiu) mentioned this pull request Jul 18, 2020
Send-MailMessage is deprecated devops-collective-inc/unix-persons-guide-to-powershell#3
Open
@user8446 Glenn Fowler (user8446) mentioned this pull request Aug 28, 2020
Should Send-MailMessage be marked obsolete #11975
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jshigetomi Justin Chung (jshigetomi) Awaiting requested review from jshigetomi jshigetomi will be requested when the pull request is marked ready for review jshigetomi is a code owner
Copilot code review Copilot
Copilot will automatically review once the pull request is marked ready for review

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

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

Milestone

No milestone

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

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@ThreeFive-O @TravisEz13 @SteveL-MSFT @FireInWinter
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.