Add Authentication Parameter to Web Cmdlets for Basic and OAuth #5052

markekraus · 2017-10-07T12:04:34Z

Adds an -Authentication parameter to Invoke-RestMethod and Invoke-WebRequest
Adds an -Token parameter to Invoke-RestMethod and Invoke-WebRequest
Adds an -AllowUnencryptedAuthentication parameter to Invoke-RestMethod and Invoke-WebRequest
Adds tests for various -Authorization uses
-Authentication Parameter has 3 options: Basic, OAuth, and Bearer
Basic requires -Credential and provides RFC-7617 Basic Authorization credentials to the remote server
OAuth and Bearer require the -Token which is a SecureString containing the bearer token to send to the remote server
If any authentication is provided for any transport scheme other than HTTPS, the request will result in an error. A user may use the -AllowUnencryptedAuthentication switch to bypass this behavior and send their secrets unencrypted at their own risk.
-Authentication does not work with -UseDefaultCredentials and will result in an error.

The existing behavior with -Credential is left untouched. When not supplying -Authentication, A user will not receive an error when using -Credential over unencrypted connections.

Code design choice is meant to accommodate more Authentication types in the future.

Documentation Needed

The 3 new parameters will need to be added to the Invoke-RestMethod and Invoke-WebRequest documentation along with examples. Syntax will need to be updated.

.

iSazonov · 2017-10-09T07:38:47Z

The existing behavior with -Credential is left untouched. When not supplying -Authorization , A user will not receive an error when using -Credential over unencrypted connections.

We could write non-terminating error or just terminating error w/o explicitly -AllowUnencrypted. I think we should. Security is in high priority.

Why 'Authorization' ? ~~Right term is 'Authentication'~~. It's confusing. I think better 'AuthorizationHeaderType' and '-AllowUnencryptedAuthentication'.

iSazonov

@markekraus Great work!

iSazonov · 2017-10-09T07:54:37Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

+            }
+            else
+            {
+                Debug.Assert(false, String.Format("Unrecognized Authorization value: {0}", Authorization));


Please use Diagnostics.Assert.

Will fix. Any idea why VS Code gives me this 'Diagnostics' is inaccessible due to its protection level [Microsoft.PowerShell.Commands.Utility]?

Because the 'Diagnostics' is defined in another assembly SDK but we have InternalsVisibleTo in AssemblyInfo.cs - VS Code is still stuped here.

Thanks for explaining. This is fixed.

iSazonov · 2017-10-09T08:04:24Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

+            {
+                _authorization = value.ToLower();
+            }
+        }


I believe we should use autoproperty and new public enum type for "Basic","Bearer","OAuth" - 'AuthorizationHeaderEnum' .

autoproperty? Please let me know what this is.

As for the new Public Enum. I was following suit with -TransferEncoding which doesn't have a public enum. I'm open to creating one, but I assume this requires committee approval?

I meant https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/auto-implemented-properties
Yes, public API should be approved. I believe it is right place to use it.

iSazonov · 2017-10-09T08:16:06Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

        public virtual SwitchParameter SkipCertificateCheck { get; set; }

+        /// <summary>
+        /// gets or sets the Token property


A more detailed description is welcomed.

iSazonov · 2017-10-09T08:17:35Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

+        [Parameter]
+        public virtual SwitchParameter AllowUnencryptedAuthorization { get; set; }
+
+        /// <summary>


A more detailed description is welcomed.
Please add about correlation with other parameters from PR description.

iSazonov · 2017-10-09T08:30:35Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+
+        It "Verifies Invoke-WebRequest -Authorization <authorization>" -TestCases @(
+            @{ authorization = "bearer"}
+            @{ authorization = "OAuth"}


Formatting (spaces).
We use it twice - please move to BeforeAll. Below too.

markekraus · 2017-10-09T09:38:14Z

We could write non-terminating error or just terminating error w/o explicitly -AllowUnencrypted.

I considered that but I wasn't sure if that should be done in this PR or something separate. This PR is adding new features only without altering existing behavior. It's still not clear to me what is too much for one PR and what is not.

Why 'Authorization' ? ~~Right term is 'Authentication'.~~ It's confusing. I think better 'AuthorizationHeaderType' and '-AllowUnencryptedAuthentication'.

I would take that up with IETF. They are the crazy ones that decided that Authorization is the correct term here regardless of what the dictionary begs to differ. 😆

I think -Authorization is fine. I asked for feedback in the issue and there seemed to be an agreement on it being ok. The originally proposed parameter was -Authentication. To most people the words are interchangeable. But to those familiar with HTTP, Authorization is less ambiguous. 'AuthorizationHeaderType' or 'AuthorizationType' is a bit verbose and redundant in my opinion.

'-AllowUnencryptedAuthentication' How about '-AllowUnencryptedSecrets'? Assuming there may be more uses outside of the 'Authorization'. I wouldn't want to have -Authorization and -AllowUnencryptedAuthentication mixing. so if one is Authorization so should be the other unless we decide on something completely different.

markekraus · 2017-10-09T10:33:45Z

Travis CI fail due to #5062

iSazonov · 2017-10-09T10:38:53Z

I believe that the use of Authorization term on the client side is a crude distortion of its classical definition of AAA. @SteveL-MSFT pointed out this in the Issue discussion. When I started with the PR, I came here in a bit of confusion about what we wanted to do on the client side?! Turns out we need classical authentication by means of setting HTTP authorization header. We have to be more precise in using long-known terms to prevent users from being misled.
So I think 'AuthorizationHeaderType' is good. IntelliSense help us with long parameters. But '-AuthHeaderType' is also good. Perhaps -AllowUnencryptedAuth too.

markekraus · 2017-10-09T23:45:39Z

What is the consensus on the parameter names? leave them? change them?

rkeithhill · 2017-10-10T00:32:08Z

I think adding "HeaderType" is more of an impl detail than the average user will care about. And while the header is named Authorization, in articles and papers the overall process is referred to as Authentication as in Basic Authentication, Digest Authentication, etc. Perhaps you just call the parameter -Auth and let the user interpret it as they see fit. :-)

SteveL-MSFT · 2017-10-10T00:43:21Z

To be consistent with other cmdlets, I think we should call it -Authentication. People can use -Auth as it will resolve correctly.

iSazonov · 2017-10-10T03:58:21Z

I agree with @SteveL-MSFT - really a client do an authentication.

markekraus · 2017-10-10T09:49:07Z

Ok. I have switch Authorization with Authentication,

@SteveL-MSFT @iSazonov can I get some guidance on the public enum for the -Authentication parameter? Should we do this? What namespace do I use? Suggestions for name (WebCmdletAuthenticationType?) I assume the file location is in the WebCmdlet folder?

iSazonov · 2017-10-10T12:05:44Z

I think an enum is good because we rid from many string constants and string comparitions, our code becomes more readable. I only worry about extensibility.
It seems namespace Microsoft.PowerShell.Commands is good.
WebCmdletAuthenticationType - we could use the type not only in web cmdlets.

@markekraus Please correct the PR header and description (Authentication).

markekraus · 2017-10-10T13:05:59Z

@iSazonov

we could use the type not only in web cmdlets.

Then we would probably need to include logic that limits the subsets allowed by the authentication parameter. For example, if this enum were used in another cmdlet and it was extended to include Shiboleth for support in that cmdlet but the web cmdlets lack logic to handle that.

iSazonov · 2017-10-10T17:41:37Z

It's overkill. I mean, this is a public type and users can use it. Although it may be better to mark it as obsolete and document that it is for internal use only.

markekraus · 2017-10-13T12:12:08Z

@iSazonov I guess it is overkill. I looked a how other enums are being implemented for parameter options and have tried to follow their conventions. I chose the name WebAuthenticationType to make it a bit more generic and to give it some clarity vs other Auth enums used in various other cmdlets.

@SteveL-MSFT is a comittee review required for the new public enum or is your blessing good enough?

@iSazonov and @SteveL-MSFT the other outstanding question in this PR is to add a warning to the legacy -Credential usage when being sent over non-HTTPS schemes. Should this be done in this PR or in a separate PR?

iSazonov · 2017-10-13T13:40:19Z

You can add one commit for the warning :-)

SteveL-MSFT · 2017-10-13T18:10:34Z

@markekraus I'll see if I can get a quick committee review on this

iSazonov · 2017-10-13T18:28:20Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

            {
+                if (Uri.Scheme != "https" && !AllowUnencryptedAuthentication)
+                {
+                    WriteWarning(WebCmdletStrings.UnencryptedCredentialWarning);


Maybe WriteError()? It seems we never use WriteWarning().

Yup, this is what ConvertTo-SecureString does if -AsPlainText is used without -Force:

2:8ms> $ss = ConvertTo-SecureString "mypassword" -AsPlainText ConvertTo-SecureString : The system cannot protect plain text input. To suppress this warning and convert the plain text to a SecureString, reissue the command specifying the Force parameter. For more information ,type: get-help ConvertTo-SecureString. At line:1 char:7 + $ss = ConvertTo-SecureString "mypassword" -AsPlainText + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + CategoryInfo : InvalidArgument: (:) [ConvertTo-SecureString], ArgumentException + FullyQualifiedErrorId : ImportSecureString_ForceRequired,Microsoft.PowerShell.Commands.ConvertToSecureStringComm and

@iSazonov I think an error would be a breaking change here. Recall this piece of code is not part of the new -Authentication parameter, but part of the legacy -Credential usage. If we want this an error here, I will revert and that can be done in a separate PR. If we are going to break it, the better way, IMO, is to remove the legacy usage and have it be completely replaced with the new usage and removing the legacy -Credential usage has some cleanup associated with it.

It seems the PR is near to merge. I think we shouldn't block it - please revert and open tracking Issue to discuss "warning", "removing" and "force".

Maybe error error with the new -Auth Basic (no -AllowUnen) scenario but not the existing -Credential (no -AllowUnen)?

@rkeithhill that is already part of this PR 😄 the new method is terminating errors when you tery to use non-HTTPS and can only be suppressed with -AllowUnencryptedAuthentication.

This piece of code that this comment thread is on was just to add some more whining to the legacy -Credential usage which has always casually let you send credentials over HTTP with out so much as a peep.

@iSazonov #5112 for the this. I have reverted that commit (with the exception of the minor spelling fix I the other tests in this PR)

SteveL-MSFT · 2017-10-13T18:30:26Z

@PowerShell/powershell-committee brief discussion is that adding an enum used as a parameter type doesn't require committee approval, code review feedback is sufficient. The current name and namespace seems appropriate to me.

iSazonov · 2017-10-13T20:01:49Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

+        /// Basic: Requires Credential
+        /// OAuth/Bearer: Requires Token
+        /// </summary>
+        [Parameter]


Should we assign default WebAuthenticationType.None?

I don't think ther is a need, The enum is never null so its creation here automatically sets it to None since it is the first enum value... at least.. if i understand everything I read correctly. If it wasn't automatically None, all of the many tests would be failing. Unless setting it here provides some kind of emit somewhere. I didin't see any other enum parameters setting a default value.

If internal logic is based on None we should initialize explicitly to None (Can we sort the enum or add new element on first position?). Another way is to being based on Authentication.IsPresent.

TravisEz13 · 2017-10-16T22:16:33Z

@markekraus Thanks for your contribution. I'll allow 24 hours for the code owners to review. @dantraMSFT , @PaulHigin , & @JamesWTruher

iSazonov · 2017-10-17T04:27:39Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs


        #region Authorization and Credentials

+        /// <summary>


Could you please replace "gets" -> "Gets"?
Although the old comments start with a small letter, the new ones are already capital. In this case, it's better to follow our rules and use the capital letter.

iSazonov · 2017-10-17T04:38:39Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

        public virtual SwitchParameter SkipCertificateCheck { get; set; }

+        /// <summary>
+        /// gets or sets the Token property. Token is required by Authentication OAuth and Bearer.


The same "get" -> "Gets".

iSazonov · 2017-10-17T04:44:28Z

src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx

    <value>Access to the path '{0}' is denied.</value>
  </data>
+  <data name="AllowUnencryptedAuthenticationRequired" xml:space="preserve">
+    <value>The cmdlet cannot protect plain text secrets sent over unencrypted connections. To supress this warning and send plain text secrets over unencrypted networks, reissue the command specifying the AllowUnencryptedAuthentication parameter.</value>


I think it is more readable to put parameter names in single quotes AllowUnencryptedAuthentication ...
We use the format in other Resx files.
Please edit messages below too.

There is a mix, from what I have seen, of quoted, unquoted and Hyphen prepended in the project. Since there is no clear set standard, I went with the style used for the Web Cmdlets. Please look at the errors in this Resx. I would rather have a separate PR to address the errors styles as whole for these commands than have a few that do not match in this PR.

I agree. Could you please open a tracking issue (or PR)?

I could.. but I think another Issue on standardizing how parameters are displayed in errors within the project is probably warranted first. I'd hate to go through the trouble of changing these error and then have the current way be later selected as the standard (I would hope it's not, because I agree it's not very readable, but its possible). thoughts?

iSazonov · 2017-10-18T03:15:35Z

😄 I've been dragged the PR.

Add Authorization Parameter to Web Cmdlets for Basic and OAuth

b450f1e

markekraus requested review from JamesWTruher, PaulHigin and dantraMSFT as code owners October 7, 2017 12:04

markekraus mentioned this pull request Oct 7, 2017

Invoke-WebRequest should directly support Basic authentication #4274

Closed

[Feature] Run Feature tests

882da03

.

markekraus force-pushed the BasicAuthentication branch from 24d2207 to 882da03 Compare October 7, 2017 12:08

SteveL-MSFT added the Documentation Needed in this repo Documentation is needed in this repo label Oct 7, 2017

iSazonov reviewed Oct 9, 2017

View reviewed changes

[Feature] Address PR Feedback

1e438ed

[Feature] Re-run CI

578e2ca

daxian-dbw assigned iSazonov Oct 10, 2017

[Feature] Authorization -> Authentication

164e653

markekraus changed the title ~~Add Authorization Parameter to Web Cmdlets for Basic and OAuth~~ Add Authentication Parameter to Web Cmdlets for Basic and OAuth Oct 10, 2017

[Feature] Add WebAuthenticationType Enum

98f2201

markekraus force-pushed the BasicAuthentication branch from c642774 to 98f2201 Compare October 13, 2017 11:28

[Feature] Warn on unencrypted -Credential

414f6d5

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Oct 13, 2017

iSazonov reviewed Oct 13, 2017

View reviewed changes

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 Oct 13, 2017

iSazonov requested a review from TravisEz13 October 13, 2017 19:18

[Feature] Revert legacy -Credential warning

b3bff7c

markekraus mentioned this pull request Oct 13, 2017

Web Cmdlets should warn when legacy -Credential is sent over unencrypted connections #5112

Closed

iSazonov reviewed Oct 13, 2017

View reviewed changes

[Feature] Set default for -Authentication

afa149d

TravisEz13 approved these changes Oct 16, 2017

View reviewed changes

iSazonov reviewed Oct 17, 2017

View reviewed changes

[Feature] Address PR Feedbadk

ea3b28c

iSazonov approved these changes Oct 17, 2017

View reviewed changes

TravisEz13 merged commit 7c9bddf into PowerShell:master Oct 18, 2017

markekraus mentioned this pull request Oct 20, 2017

Add -Authentication Documentation to 6.0 Web Cmdlets MicrosoftDocs/PowerShell-Docs#1793

Merged

8 tasks

markekraus removed the Documentation Needed in this repo Documentation is needed in this repo label Nov 19, 2017

markekraus deleted the BasicAuthentication branch January 19, 2018 18:57

leonard84 mentioned this pull request Jan 24, 2018

Invoke-WebRequest should support preemptive Authentication #6012

Closed

Add Authentication Parameter to Web Cmdlets for Basic and OAuth #5052

Add Authentication Parameter to Web Cmdlets for Basic and OAuth #5052

Uh oh!

Conversation

markekraus commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation Needed

Uh oh!

iSazonov commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markekraus commented Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markekraus commented Oct 9, 2017

Uh oh!

iSazonov commented Oct 9, 2017

Uh oh!

markekraus commented Oct 9, 2017

Uh oh!

rkeithhill commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Oct 10, 2017

Uh oh!

iSazonov commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markekraus commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markekraus commented Oct 10, 2017

Uh oh!

iSazonov commented Oct 10, 2017

Uh oh!

markekraus commented Oct 13, 2017

Uh oh!

iSazonov commented Oct 13, 2017

Uh oh!

SteveL-MSFT commented Oct 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

markekraus commented Oct 7, 2017 •

edited

Loading

iSazonov commented Oct 9, 2017 •

edited

Loading

markekraus commented Oct 9, 2017 •

edited

Loading

rkeithhill commented Oct 10, 2017 •

edited

Loading

iSazonov commented Oct 10, 2017 •

edited

Loading

markekraus commented Oct 10, 2017 •

edited

Loading

iSazonov commented Oct 10, 2017 •

edited

Loading

TravisEz13 commented Oct 16, 2017 •

edited

Loading