Skip to content

RFC to obsolete Send-MailMessage #160

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

Merged
merged 10 commits into from
Aug 12, 2019

Conversation

TravisEz13
Copy link
Member

No description provided.


### Remove the cmdlet in 6.3

During the development of 6.3, remove the `Send-MailMessage` cmdlet.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were 4 PRs to improve Send-MailMessage over the past half year. That indicates people are actively using this cmdlet.
Removing this cmdlet without an available replacement seems not right ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think if we don't remove the cmdlet, we need to add a switch... Something like -AllowInsecure and document that newer method should be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have plans to update it completely, why not simply move it to its own module that comes "in the box" with PS Core, starting with the version that implements the newer recommended libraries?

Copy link
Member Author

@TravisEz13 TravisEz13 Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vexx32 This option is mentioned in the Alternate Proposals and Considerations section.

Copy link

@kilasuit kilasuit May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @vexx32 on moving it out to a separate module, though this should be still bundled with future PowerShell Core versions just Like PowerShellGet was extracted from the codebase and brought in as a build package, back in the early days of PowerShell going open source, and this would have added benefit of having the potential to be used down level as well, if built right

Copy link
Contributor

@iSazonov iSazonov May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could disable the cmdlet by default but add option in powershell.config.json to enable it in environment where it is needed. Perhaps this is related Constrained Language Mode.

Copy link

@TobiasPSP TobiasPSP May 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, I really want to be as constructive as I can. I may (and probably will) be wrong in some areas or not hit the correct dev terminology. Bear with me please. I stumbled across this thread after serious confusion was caused by the new "obsolete" message in production. I believe both the message and the plan to remove/change Send-MailMessage is wrong, and am trying to make a point below.

  • DEFINITION OF PROBLEM: I was puzzled to learn that this decision seems to be based largely on assumptions rather than facts. Is there a specific bug or security flaw in the cmdlet? Apparently not. Will the cmdlet continue to do what it promises to do? Yes it will. Is there a better replacement cmdlet available? No, there isn't.
    Fact seems to be that the underlying API does not support some new security protocols. The cmdlet is not falling back automatically or transparently to clear text auth, though, and the latter may be just fine for non-security relevant data mailings from servers which is a predominant use case in PowerShell scripts. So there is apparently no security reason to recommend stop using the cmdlet.
    Another fact seems to be the dotnet team marked the underlying API obsolete. However there are apparently no plans to remove them anytime soon, and the cmdlet will continue to work for the foreseeable future. So there is no rush. There would have been plenty time to bring this discussion to an end, and then come up with an answer. Instead, a message was emitted even though it is not yet clear whether the cmdlet will be serviced, replaced, or removed. What is the intended benefit of this message?
  • COMMON TERMINOLOGY: When you declare a cmdlet "obsolete" and emit a warning to the end user, it is not important what "obsolete" means to you. It is important what the addressed audience understands. In the outside world, "obsolete" means the use of this cmdlet is no longer recommended and considered unsafe practice. Whenever the authors of a software declare part of their software "obsolete", the outside world expects time to adjust before this happens (by giving a forewarning) and a transition path. Neither is the case here.
  • TARGET AUDIENCE: A considerable percentage of the PowerShell user base are admins w/o c# dev background. When the suggested transition path is to use "MailKit" and involves github resources, you are essentially bubbling up unfiltered dotnet recommendations to your PowerShell audience which is not appropriate for this audience. Any remedy should work in the context of your audience, not your own. You may want to use MailKit in a new version of Send-MailMessage, but a production admin certainly may not know how to do this or may not have the permission to use the resources in the first place.
  • NO BREAKING CHANGES CONTRACT: The outside expectation was that PS Core introduced fundamental breaking changes ONCE in order to address cross platform challenges. Ongoing, it was considered the team would continue to avoid breaking changes to engine AND default cmdlets - and why shouldn't you. It was somewhat illuminating just how relatively lightheartedly you talk about removing a cmdlet that is in use in zillions of production scripts and technically can definitely be improved without breaking changes. Dropping default PowerShell cmdlets that you shipped with the engine is in some respect the same as pulling an open source repo/dependency that is in use by someone else. If you don't want that, you probably need to strip all cmdlets from the engine that aren't essential to the engine, and ship them in different modules, owned by someone committed to maintaining these modules.However, I would advise not to. One of the strengths of PowerShell was always that it shipped with a critical number of cmdlets and enabled many automation tasks OOB. Loading additional modules/packs on demand may be common practice for devs but is prohibitive in many production environments and not feasible for many offline sites. If you strip all cmdlets from the engine, this will seriously affect the one-stop-philosophy.

In a nutshell: I'd hope the user side will be integrated better in the decision making process which of course is in part the responsibility of the user side (i.e. people like me). PS users tend to not stumble across github discussions by nature. If you find these thoughts helpful, I'll gladly contribute. If they are not appropriate or this is not the right place, let me know and I stop.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TobiasPSP, I agree with all your comments.
It should also be pointed out that SMTP has not, and should not be considered a secure protocol. While it now has lots of security enhancements, they aren't enabled by default and there should never have been an assumption of any kind of security when using SmtpClient to send mail.

@dragonwolf83
Copy link

This is a major breaking change. I use Send-MailMessage extensively for email notifications to an internal SMTP relay server. These emails can be success or failure alerts, or internal monitoring of applications to send alerts. Removing this removes a key feature from PowerShell that people have been using to schedule PowerShell scripts to run and be notified of issues.

Additionally, the motivations do not make sense, just because many 3rd party services have RESTful services should not deny the simple use of SMTP servers. Many companies do not have internet access on servers and should not have to find a 3rd party RESTful service to get email. Send-MailMessage solved a major problem from the batch days when a simple SMTP client was not available in Windows without installing additional software. Most users would never think to use a REST method to just send email when they have their own SMTP servers running already.

The insecure protocol is lacking details on what the core issue is so I can't comment on that. The cmdlet should remain with the backend rewritten to a new SMTP Client without removing any parameters or changing the cmdlet name. If needed, use Write-Warning to highlight any potential issues.

@TravisEz13
Copy link
Member Author

@dragonwolf83 I'll update the motivation. The motivation is that the underlying DotNet API is depreciated, does not meet current security standards, and will not be replaced. Underlying API does not support what the current SMTP TLS security standards.

We cannot continue to support the current cmdlet. Any re-implementation would almost certainly involve breaking changes.

I'm leaning toward the implementation mentioned in the second comment in this thread: #160 (review)

This would allow you to continue to use this cmdlet with minimal changes while alternative(s) are found.

@iSazonov
Copy link
Contributor

Original Core team message from https://github.com/dotnet/platform-compat/blob/master/docs/DE0005.md:

DE0005: SmtpClient shouldn't be used

Motivation

SmtpClient doesn't support many modern protocols. It is compat-only. It's
great for one off emails from tools, but doesn't scale to modern requirements of the protocol.

Recommendation

Use MailKit or other libraries.


It is compat-only.

It allows us to continue using the API for a while until we implement a replacement.
Modern support lifecycle we use has a side affect - if we remove the cmdlet without no alternative users may be forced to stop updating.
I assume that this cmdlet is usually used in a closed environment where modern means are not needed.

@TravisEz13
Copy link
Member Author

I've added added a section Alternative to removal and updated the motivation section based on feedback.

@David-Barrett-MS
Copy link

The deprecation of System.Net.Mail only applies to Xamarin and Mono according to this thread: https://github.com/dotnet/docs/issues/1876

It is an issue with documentation that the message is being shown for other versions. What version of the framework is Send-MailMessage using?

@TravisEz13
Copy link
Member Author

@David-Barrett-MS I'm not clear on that from reading the issue. I saw no firm statement on if the API is obsolete in DotNet Core. Only that it is not obsolete in DotNet Standard. I've asked for clarification in the issue.

TravisEz13 added a commit to PowerShell/PowerShell that referenced this pull request Mar 22, 2019
Add Obsolete message to Send-MailMessage

## PR Context

See PowerShell/PowerShell-RFC#160

Co-authored-by: Steve Lee <[email protected]>
Co-authored-by: Ilya <[email protected]>
@ThreeFive-O
Copy link

If the PowerShell Committee agrees, I would start work for a complete overhaul of the Send-MailMessage cmdlet using the MailKit library for the PowerShell 6.3 milestone.

TravisEz13 added a commit to PowerShell/PowerShell that referenced this pull request Mar 25, 2019
Add Obsolete message to Send-MailMessage

## PR Context

See PowerShell/PowerShell-RFC#160

Co-authored-by: Steve Lee <[email protected]>
Co-authored-by: Ilya <[email protected]>
# Conflicts:
#	test/powershell/Modules/Microsoft.PowerShell.Utility/Send-MailMessage.Tests.ps1
@Jaykul
Copy link
Contributor

Jaykul commented Apr 2, 2019

They're saying it's obsolete, but that they're not going to add the Obsolete attribute on it to avoid breaking everyone's compilation. I guess that's a new brand of passive-aggressive semi-obsolescence.

I do think there should be a new (e.g. Mailkit) module -- one that's available down level, too.

I also think the docs for Send-MailMessage should be updated to warn about the technology and security -- copy the MSDN docs if you like, and point to that new cmdlet once it's written.

However, given that the underlying code isn't going anywhere, I do not think we should remove Send-MailMessage -- for the same reason they're not removing the API, or even marking it [obsolete]: this isn't worth breaking things for.

Anecdotally: I believe people have used this cmdlet extensively in scheduled tasks and such -- we use it at work with on-box SMTP servers, so that we can avoid embedding credentials in the scheduled tasks. The communication is all internal to the server (the mail server forwards it as securely as mail servers do).

Our web services use the same server with that "obsolete" SmtpClient class, and frankly, I'll have rewritten our scheduled tasks as Azure functions with unified reporting and alerting long before we replace that mail solution 😉

@David-Barrett-MS
Copy link

What I'd like is some justification for this sudden decision that it is obsolete. Just because SMTP has developed and offers a lot more than the SmtpClient can deal with, it doesn't mean that the code is suddenly useless or a security risk. I suspect that most cases where SmtpClient (and indeed Send-MailMessage) are used would be for small messages (e.g. reports, administrator notifications). Both SmtpClient and Send-MailMessage work perfectly for these, and require none of the enhancements to SMTP. So long as its limitations are documented, there is really no need for obsoletion.

Anyone that does need the new SMTP features would likely consider writing their own code. I suspect most people that work with SMTP, POP, etc. have their own libraries - I know I do.

Normally we'd deprecate something before making it obsolete. I don't really agree with the way any of this has come about, as it has been presented badly (developers are suddenly worried that by using SmtpClient their code is at risk), and with absolutely no warning.

@TravisEz13
Copy link
Member Author

@David-Barrett-MS The underlying DotNet code is obsolete and depending on the server you use, may pose a security risk. We have had customers report these issues as security issues to us expecting us to be able to fix this. Marking the cmdlet as obsolete, puts the users on notice that this code is not being maintained.

Expecting the customer to know without any notice that they are expected to implement security features on their own is not reasonable.

I think your question is about what to do after making this obsolete. Perhaps you should read through the alternate proposals and see if any of those are better or suggest how they can be improved.

@Jaykul
Copy link
Contributor

Jaykul commented Apr 3, 2019

To be fair though @David-Barrett-MS it's not "sudden" -- the API was marked obsolete in the MSDN docs at least two years ago. It just feels sudden because in this RFC the PowerShell world has gone from not knowing about that to removing the cmdlet in a single headline! 😮

@TravisEz13
Copy link
Member Author

To be clear, we would not back port whatever change we implement. The change would only be added to 6.3 and newer.

An Obsolete warning was added in 6.2, and we have not decided what we will do going forward. The RFC process is to solicit feedback, but continuing to act like we can support a cmdlet written against an obsolete API is not viable.

Please look at all of the proposed implementations and give feed back on them.

@David-Barrett-MS
Copy link

By sudden, I mean that the client went from in-use to obsolete straight away. Normally we would deprecate code in one version and obsolete it in the next. This never happened. I'm referring to the SmtpClient code here, not PowerShell.

What security risk is there? The only reports I've had (we support System.Net.Mail) are precisely because we've marked the code as obsolete, not because of how the code is implemented. I'd appreciate it if someone could point out what I've missed here. Or are you referring to this from the PowerShell viewpoint where the security risk is that it is now using obsolete code (which would reinforce my point, as far as I can see)?

@TravisEz13
Copy link
Member Author

TravisEz13 commented Apr 3, 2019

@David-Barrett-MS Depending on the server configuration, the client may not be able to negotiate a TLS connection, resulting in a clear text connection being the only viable connection option with this cmdlet.

@David-Barrett-MS Please read the proposal. We have no deprecate process. Making a cmdlet obsolete does not remove it from the product.

@iSazonov
Copy link
Contributor

iSazonov commented Apr 4, 2019

Making a cmdlet obsolete does not remove it from the product.

Can we explicitly add this in RFC text? I think this is the main request from community.

@TravisEz13
Copy link
Member Author

I've updated the text and moved removal for 6.3 as an alternative option. I added a note in the section for making it obsolete that making it obsolete does not affect compatibility.

@Jaykul
Copy link
Contributor

Jaykul commented Apr 5, 2019

I know it's too late to say this, but I really don't like runtime warnings as a way of deprecating things. I mean, I know we have an interpreted language, and there are no compile-time warnings, but this runtime warning is something that most affected users will never see (because they won't be updating).

Meanwhile, those on the cutting edge want to avoid warnings (I, for one, want to treat them as events that need to be logged and investigated, so I'm constantly fighting to prevent warnings like this one).

Assertion: developers should only warn about deprecation if they are actually committed to removing things, in a specific release time frame. Otherwise we end up with deprecation warnings that persist for decades and we are training developers to ignore them, not to respond properly.

As a case in point, consider LoadWithPartialName which was marked obsolete with the release of .NET 2.0 back in 2006 or so. Except that it's not only survived to .NET 4x, it was ported to Linux in .NET Core, more than a decade years after it was marked obsolete! And now a whole new generation is learning to ignore obsolescence via #pragma warning disable 618...

If this is, in fact, a security problem, a runtime warning in the bleeding edge version is a frustrating -- and woefully inadequate -- notification system. There should be a KB article and a blog post instead. If it's not, we should leave things alone.

@Jaykul
Copy link
Contributor

Jaykul commented Apr 5, 2019

Having said all that:

I would prefer an option where PowerShell considered built in cmdlets to be part of the API and covered by the "no breaking changes" contract. This would require the PowerShell maintainers to do the work necessary to preserve the PowerShell programming surface while switching out the underlying code for MailKit (or whatever else needs to be done). You can have a bunch of optional settings to make it more secure, and then you can add warnings about insecure use -- once people can do something about it.

Failing that, I would prefer an option where a replacement was shipped via the PowerShell Gallery, rather than in-box -- because something like this will almost certainly happen again ten years from now (when the world decides MailKit --and email in general-- is obnoxiously old fashioned and abandoned), and we will then need to deprecate that new command.

Far better to deprecate a module, and have warnings show up at install time.

@TobiasPSP
Copy link

TobiasPSP commented May 18, 2019 via email

@iSazonov
Copy link
Contributor

Root of the problem in CoreFX repository. Core team announced the mail API obsolete and in PowerShell repo we can't have more soft policy. Note that they only announced obsolete in docs without adding the attribute.

@fMichaleczek
Copy link

MailKit is Copyright Xamarin Inc...

@iSazonov
Copy link
Contributor

From https://github.com/jstedfast/MailKit/blob/master/License.md

MailKit is Copyright (C) 2013-2019 Xamarin Inc. and is licensed under the MIT license:

@joeyaiello
Copy link
Contributor

Jumping in the thread here after some good discussion on Twitter. Wanted to land a few points as I read through the discussion.

We encourage everyone, both internal and external, to publish RFCs before they're "perfect", so that they can evolve based on feedback and discussion, rather than bottleneck based on internal conversations that go in circles before ever publishing a draft. To that end, I believe the process has worked exactly as intended: @TravisEz13 recognized a security issue and drafted an RFC that allowed us to frame internal, Committee, and community discussions. That discussion yielded some changes to the RFC, and hopefully folks feel that (at least some set of) their concerns have been addressed.

As for the language around deprecation/obsolescence, PowerShell hasn't ever really made a distinction between something being deprecated and obsolete. While there may technically be a distinction between the two, we've traditionally used them as equivalents (and .NET treats it the same way; in fact, using ObsoleteAttribute in .NET generates a DeprecationAttribute for other frameworks in some cases).

Not to pick on @TobiasPSP, but I felt this quote was a good discussion pivot:

In the outside world, "obsolete" means the use of this cmdlet is no longer recommended and considered unsafe practice. Whenever the authors of a software declare part of their software "obsolete", the outside world expects time to adjust before this happens (by giving a forewarning) and a transition path. Neither is the case here.

This is actually exactly what we intend to do via deprecation: this cmdlet is not recommended, and it is considered an unsafe practice. By emitting a message without breaking usage of the cmdlet, we are starting a transition time that allows us to decide what we want to do and when. It does not imply removal, it does not imply replacement. It means "if you can help it, you should not use this".

@Jaykul pointed out that LoadWithPartialName has been deprecated/obsolete for a significant amount of time, even being ported forward for compatibility purposes: this is not at odds with deprecation. Something can continue to exist for backwards compatibility while still being disrecommended.

And while this technical distinction might not matter to users, those same users should still be told that usage is disrecommended. If they believe deprecation means removal in the next version, even if they are wrong, they will have been right to find an alternative.

Folks on the thread are also right that runtime warnings aren't going to be visible to a huge chunk of customers that are running this cmdlet in automation. While we'll never achieve full coverage on notifying people, there are some takeaways from the discussion that I think we can and should do, including:

  • doc'ing the security implications better in the Send-MailMessage docs
  • publishing a blog post to give more visibility to the need to move away / possible changes coming further down the pipeline.
  • having more discussion here about the best path forward from here

I'm not taking an opinion yet on defining that "best path forward" because I haven't thought about it enough, and because I don't have data on the size delta that we might be looking at to pull in something like MailKit. As we begin to look at delivering PS 7 in Windows, component size is going to become more and more relevant, and things may also change there for us in the future.

@TobiasPSP
Copy link

I don't know if it is good to submit a "quote that was a good discussion pivot" :-) The good thing is that PowerShell development never was more open than today, partially thanks to RFCs.
To clarify my point: was there ever a feature, cmdlet or other part of PowerShell that was called obsolete or insecure or not recommended to use, and at the same time there was no built-in replacement/improvement? From my perspective it was less about the definition of "obsolete" but rather the "why is this suddenly a security risk?"

@TravisEz13
Copy link
Member Author

With regards to why this is "suddenly" a security risk:

#160 (comment)

Specifically, we had a security escalation that required Opportunistic TLS to resolve, which is not supported by the underlying API. If we are unable to support security issues, to our security bar, we have need choose some way that satisfies our requirement to communicate the issues proactively to customers. In this case, we chose obsolescence.

@TobiasPSP
Copy link

TobiasPSP commented May 24, 2019

You could consider this a feature request rather than a security issue, too. Send-MailMessage never supported Opportunistic TLS. In response to your customer one could either promise to add that feature to Send-MailMessage (preferred) or explain that this particular cmdlet cannot use this feature and is intended for simple administrative reporting scenarios. Maybe it would even be sufficient to update the docs and explain what the cmdlet does and what it does not do. Both removing and marking the cmdlet obsolete will break thousands of production scripts. Even obsolescence will break because it won't take long until security analyzers rightfully pick this issue up and begin to flag scripts using the cmdlet. Before you introduce such a significant breaking change, IMHO it is crucial to discuss and analyze the intended benefits: what are the actual security leaks and unsafe practices that would stop by abandoning Send-MailMessage? All Send-MailMessage scenarios I know either work or don't work, and when they work, they work safely. On a larger scale (this RFC is just one example for it) there should be a comprehensive discussion elsewhere about the "no breaking change contract" in PSCore which then might provide guidance and consistency here and in similar cases in a consistent way.

@figueroadavid
Copy link

I don't have much to add that hasn't been said already. I know of a lot of scripts that use Send-MailMessage for automated notifications. I also use it for a lot of notifications. While the api's may be documented to be obsolete, that in and of itself doesn't justify removing the cmdlet.

My opinion on this would be to leave Send-MailMessage intact, working as is, and add a blurb about it being going obsolete in the help.

Considering the functionality level, I believe that MailKit is significant overkill for a very simple & basic cmdlet. Generally, the scripts that I've seen & used do not care about receive side of email - they just send it. This cmdlet isn't a full blown interactive email client.

I would suggest creating a new cmdlet based on the new API's (like Send-SMTPMessage) and then keep the 2 cmdlets in parallel until .Net really does remove the API's, and then kill Send-MailMessage. I'm not aware of any administrators really caring about being able to retrieve email in PS, but virtually all of them care about being able to send it. That will give administrators time to migrate their scripts to the new cmdlet without forcing them to stop upgrading PS or trying to figure out how to work around not having that built in cmdlet.

@TravisEz13
Copy link
Member Author

@figueroadavid Your proposal mirrors the RFC almost identically. Thank you.

Copy link
Contributor

@vexx32 vexx32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few extra comments so we can clarify things a bit, but otherwise this looks good to me. 🙂


### Add a warning to 6.2

In a 6.2, we have added an obsolete warning to `Send-MailMessage` that an alternative method should be found.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As others have mentioned, this should probably be a documentation note and a warning in PSScriptAnalyzer rather than a persistent in-console warning. This will just needlessly clutter logging data for a large portion of use cases for this cmdlet.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe whatever action is taken, any existing script that uses Send-MailMessage should continue to work. A switch like -AllowInsecure or similar measures would break existing scripts as it would require code changes.

### Community replacement

The community could develop a replacement that can could be incorporated back into PowerShell Core.
This has the disadvantage of increasing the size of PowerShell Core and delaying the solution.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could follow a similar path of ThreadJob, where the module is available on the gallery separately and can be updated separately as needed, but also packaged in the box with PowerShell itself.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not use a REST api call for sending email. SMTP has not changed signficantly, and every mail server/mail transport agent supports SMTP. There is no way of guaranteeing that a designated mail server will support the REST API's. If the REST API's are needed to support the more advanced protocols, then I would suggest adding a switch or a parameter to designate the type of email communication.. something like -Protocol SMTP|REST or '-UseRESTor-UseSMTP`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the REST API's are needed to support the more advanced protocols, then I would suggest adding a switch or a parameter to designate the type of email communication.. something like -Protocol SMTP|REST or '-UseRESTor-UseSMTP`.

I wouldn't even go that far. REST is just a set of APIs specific to an application. It is not a standard or a communication protocol like SMTP. To add it to Send-MailMessage would require multiple implementations of 3rd party APIs which would be hard to mantain.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a alternative module, written by Microsoft or the community, is the selected path. Then it needs to support SMTP as that is still the industry default for sending email.

REST APIs are a platform specific thing (SparkPost, GMail, etc)

@Agazoth
Copy link

Agazoth commented May 31, 2019

So sad! I feel old 😥

Co-Authored-By: Joel Sallow (/u/ta11ow) <[email protected]>

In a 6.2, we have added an obsolete warning to `Send-MailMessage` that an alternative method should be found.

**Note:** Adding an obsolete warning should not break compatibility in any way.
Copy link

@kjacobsen kjacobsen Jun 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will however cause warning alerts for anyone who runs automation that looks for warnings generated on the warning output stream, IE Azure Automation.

What is the point of a warning if it isn't actionable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actionable: you learn about the obsolescence and you can make a decision based on your own environment on whether to continue using it.

And we'll have better documentation in the future about the underlying risk.


### Positive confirmation of issue in 6.3

We will add a switch requiring the user to accept the risk of using older **possibly** less secure implementations.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will simply encourage insecure behavior. We don't want users to simply "add this thing to make email work". That isn't a good security practice and it will not encourage the behavior you think it will.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PowerShell/powershell-committee talked and agreed we don't really need this switch. We're leaning now towards just having the warning message

### Positive confirmation of issue in 6.3

We will add a switch requiring the user to accept the risk of using older **possibly** less secure implementations.
This switch would be called `-AllowUnsecureConnection` and would be mandatory

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be ````-AllowInsecureConnection```. Unsecure isn't a word.


### If needed, develop an alternative

If needed, develop a new cmdlet based on the DotNet recommended solution of [MailKit](https://github.com/jstedfast/MailKit).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now we enter the realms of JavaScript with dependencies all the way down.

PowerShell devs and admins will now need to keep track of a PowerShell version, a SMTP module version and MailKit version to ensure they don't have any security issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally that would fall on the maintainer of the cmdlet/module that's redistributing MailKit (we don't have dependency resolution between Nuget.org and the PS Gallery, so it'd have to be packaged as a binary). That might be us, or it might be someone in the community.


# Obsoleting `Send-MailMessage`

`Send-MailMessage` does not support many modern protocols leading to the inability to use this with modern secure mail services.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I question this statement. Do we have any numbers to back this up?

Most of the time when I am using Send-MailMessage, I am hitting an internal SMTP server or Office 365 (which doesn't seem to be an issue yet). If the majority of users are hitting these, then this CMDLet has quite a long life left in it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't matter how many there are. Users need a reasonable expectation that what they're doing is secure if it's reporting as secure. If you're using an internal-only server, we can provide some context so that you can decide it's okay to do what you're doing, but we can't create a false expectation of security.


The underlying API, [`SmtpClient`](https://docs.microsoft.com/dotnet/api/system.net.mail.smtpclient), doesn't support many modern protocols.
It is compat-only.
It's great for one off emails from tools, but doesn't scale to modern requirements of the protocol.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expand on how it doesn't scale?

### Primary

The underlying API, [`SmtpClient`](https://docs.microsoft.com/dotnet/api/system.net.mail.smtpclient), doesn't support many modern protocols.
It is compat-only.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where was this ever listed as for compatibility only?

@kjacobsen
Copy link

The underlying DotNet code is obsolete and depending on the server you use, may pose a security risk. We have had customers report these issues as security issues to us expecting us to be able to fix this. Marking the cmdlet as obsolete, puts the users on notice that this code is not being maintained.

Expecting the customer to know without any notice that they are expected to implement security features on their own is not reasonable.

The underlying code does NOT pose a security risk. SMTP servers expecting the missing functionality will simply reject the connection, they don't reduce down to plaintext.

Most PowerShell users are not going to be aware they would need to also track MailKit project for security vulnerabilities/warning if we were to make a module reliant on their code.

@joeyaiello
Copy link
Contributor

joeyaiello commented Aug 5, 2019

@PowerShell/powershell-committee discussed this one today, and we're leaning towards just having the obsolete warning message (as opposed to having the switch). If you think it pollutes your logs, you can create a wrapper function that disables warning messages for it, but we believe it's important for people to know about the security implications.

Discussion also raised a need for a public clarification on our security stance, namely how we generally consider security as the absolute number 1 priority over usability, performance, and potentially breaking changes. (There's nuance around this, though, hence the need for a fleshed out explanation of our security policy.)

@TobiasPSP
Copy link

It was never substantially explained what the security implications really are that justify all of this. What would be a scenario where using Send-MailMessage caused a compromise?

@figueroadavid
Copy link

I agree.. I can't understand any circumstance where sending an unauthenticated email would be an issue. I can see potentially removing one or more of these options: -UseSSL, -DeliveryNotificationOption, -Credential, to prevent any potential security issues. (again, with a warning and sufficient time). Reading through the mailkit github site, this looks like extreme overkill in order to just send an email. I haven't seen or spoken with anyone who is using Send-MailMessage for anything other than just a very basic automated email or just for testing a mail server.

@joeyaiello
Copy link
Contributor

We've explained the security implications above, in the warning, and via the .NET issue discussions. We cannot guarantee that secure connections are secure using the current implementations. If you're using fully internal servers, great! You understand the security implications, and you can continue to use Send-MailMessage.

We haven't removed the cmdlets, and we don't plan to remove the cmdlets without a viable replacement. At this point, the only complaint I see is over a runtime warning. It might annoy some people, but it's absolutely a legitimate warning in that we recommend you not use Send-MailMessage if you can help it.

But frankly, we need no other reasons than that the upstream APIs are obsolete and unmaintained. It is impossible for us to maintain code that sits on top of unmaintained code that we do not own. That's going to be universally true, irrespective of how popular a feature or cmdlet might be.

Per conversation with @TravisEz13 and the @PowerShell/powershell-committee, removing the language around an opt-in switch that breaks existing usage. However, we'll be leaving the obsolescence message until an alternative is decided on.
@joeyaiello joeyaiello merged commit 98c6118 into PowerShell:master Aug 12, 2019
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.