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

Add intergrated auth #36

Closed
wants to merge 2 commits into from
Closed

Conversation

AvrumFeldman
Copy link

@AvrumFeldman AvrumFeldman commented Oct 2, 2022

In reference to #12

My apologies that it's not on a separate branch. I'm not a big git user (by far) and by the time I realized it should've been on it's own branch it was very hard for me to figure out how to roll back the commits and add them back into their own branch so I left it as is. If you feel it should use it's own branch let me know and I will work on doing it.

No code here is my own. I practically converted the code from jstedfast/MailKit#332 (comment) to PowerShell.

I think this code uses NTLM, maybe in the future if someone is brave enough Kerberos can be added as well.

The only test I did was the following which worked, there might be other scenarios that won't work.

$param = @{
    smtpserver  = "<>"
    Port        = 587
    From        = "<>"
    To          = "<>"
    Subject     = "test email $(((get-date).GetDateTimeFormats("g"))[-2])"
    Body        = "Nothing significant"
}
send-emailmessage @param -IntergratedAuth -SkipCertificateValidatation

@AvrumFeldman AvrumFeldman marked this pull request as draft October 2, 2022 20:57
@AvrumFeldman AvrumFeldman marked this pull request as ready for review October 2, 2022 21:15
@AvrumFeldman AvrumFeldman marked this pull request as draft October 2, 2022 21:15
@PrzemyslawKlys
Copy link
Member

Don't worry about the branches. I have no clue what I am doing either ;)

@AvrumFeldman
Copy link
Author

@PrzemyslawKlys I've been thinking, maybe I should remove the switch parameter and make it work like the native send-mailmessage where if no password is provided then intergrated auth is attempted automatically? Let me know what you think. Also please let me know if there is some other area that the code doesn't cover.

@PrzemyslawKlys
Copy link
Member

Hi,

I've been thinking about this topic and three things:

  1. I believe From is always a required field (no?), and you removed the mandatory field which was fixed in my last release 0.9.0?
  2. We need to make sure the code runs on Linux/macOs (aka don't throw errors when loading module) and detects if it's being used on those systems and prevent usage of NTLM authentication.
  3. Maybe it could act the same way, although this will potentially have a weird impact where now it prompts for credentials if nothing is provided. Not sure which is better. Some less savvy users may not understand why things aren't working. Maybe we should auto-detect if the computer is domain joined and runs in domain before assuming NTLM usage at all?

@AvrumFeldman
Copy link
Author

  1. About the From mandatory parameter removal, I'm sorry, it wasn't intentional it might've happened when I merged your latest changes into my repo. I will fix it.
  2. I see you are also using DLLs so I assumed you are Windows only. I guess I was wrong, although I did also add the .net standard DLL so loading might not be an issue. I assume you have somewhere some code that checks OS (for correct DLL loading), I'll see about tapping into that.
  3. You raised valid points. I think leaving as is (as a parameter and not implicit) solves both issues. A. If the parameter not activated and credentials are not provided user should get prompted for credentials. B. If user is not on domain or non savvy users should just not use the parameter. Leaving as is also adds a security benefit of preventing automatic NTLM hash leaking.

Tasks to do

  • Fix regression on mandatory From parameter.
  • Cross platform compatibility.
  • Test credential prompting how it works with the feature.

@PrzemyslawKlys
Copy link
Member

2nd option can be tested with Devops, as there are tests running I believe. So if you don't see errors there on loading it should not be a problem, but i guess we should write some tests for email sending anyways.

@PrzemyslawKlys
Copy link
Member

image

Seems to work fine

@PrzemyslawKlys
Copy link
Member

Any luck in finishing up those TODDO task?

@AvrumFeldman
Copy link
Author

@PrzemyslawKlys I'm so sorry for my radio silence. Life is thankfully busy and I haven't yet found the extra moment to come back into this project and finish it up. But don't worry, I didn't forget about it and still hope to finish it up soonish.

Regards,

@PrzemyslawKlys
Copy link
Member

Just a reminder ;)

PrzemyslawKlys added a commit that referenced this pull request May 16, 2024
- no need for #36
- maybe starting point for #43?
@PrzemyslawKlys
Copy link
Member

PrzemyslawKlys commented May 16, 2024

This will not be needed once this PR is done:

It basically translates Mailozaurr to C#, or at least that's the goal. Thank you for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants