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 Authentication Parameter to Web Cmdlets for Basic and OAuth #5052

Merged
merged 10 commits into from Oct 18, 2017

Conversation

@markekraus
Copy link
Collaborator

commented Oct 7, 2017

Closes #4274

  • 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.

@markekraus markekraus force-pushed the markekraus:BasicAuthentication branch to 882da03 Oct 7, 2017

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

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
Copy link
Collaborator

left a comment

@markekraus Great work!

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 9, 2017

Collaborator

Please use Diagnostics.Assert.

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 9, 2017

Author Collaborator

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 9, 2017

Collaborator

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

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 9, 2017

Author Collaborator

Thanks for explaining. This is fixed.

_authorization = value.ToLower();
}
}
private string _authorization;

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 9, 2017

Collaborator

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

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 9, 2017

Author Collaborator

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?

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 9, 2017

Collaborator

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.

@@ -94,6 +120,12 @@ public abstract partial class WebRequestPSCmdlet : PSCmdlet
[Parameter]
public virtual SwitchParameter SkipCertificateCheck { get; set; }

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 9, 2017

Collaborator

A more detailed description is welcomed.

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 9, 2017

Author Collaborator

fixed.

public virtual SwitchParameter AllowUnencryptedAuthorization { get; set; }

/// <summary>
/// Gets or sets the Authorization property used to determin the Authorization method for the web session.!--

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 9, 2017

Collaborator

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

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 9, 2017

Author Collaborator

fixed.


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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 9, 2017

Collaborator

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

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 9, 2017

Author Collaborator

fixed.

@markekraus

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 9, 2017

Travis CI fail due to #5062

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 9, 2017

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

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 9, 2017

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

@rkeithhill

This comment has been minimized.

Copy link
Contributor

commented Oct 10, 2017

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

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2017

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

@markekraus

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 10, 2017

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

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2017

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 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

@markekraus

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 10, 2017

@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

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2017

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 markekraus force-pushed the markekraus:BasicAuthentication branch to 98f2201 Oct 13, 2017

@markekraus

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 13, 2017

@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

This comment has been minimized.

Copy link
Collaborator

commented Oct 13, 2017

You can add one commit for the warning :-)

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

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

@@ -472,6 +472,10 @@ internal virtual void PrepareSession()
//
if (null != Credential && Authentication == WebAuthenticationType.None)
{
if (Uri.Scheme != "https" && !AllowUnencryptedAuthentication)
{
WriteWarning(WebCmdletStrings.UnencryptedCredentialWarning);

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 13, 2017

Collaborator

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

This comment has been minimized.

Copy link
@rkeithhill

rkeithhill Oct 13, 2017

Contributor

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

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 13, 2017

Author Collaborator

@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.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 13, 2017

Collaborator

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".

This comment has been minimized.

Copy link
@rkeithhill

rkeithhill Oct 13, 2017

Contributor

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

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 13, 2017

Author Collaborator

@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

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

@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 iSazonov requested a review from TravisEz13 Oct 13, 2017

/// OAuth/Bearer: Requires Token
/// </summary>
[Parameter]
public virtual WebAuthenticationType Authentication { get; set; }

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 13, 2017

Collaborator

Should we assign default WebAuthenticationType.None?

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 13, 2017

Author Collaborator

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.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 14, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 14, 2017

Author Collaborator

fixed.

@TravisEz13

This comment has been minimized.

Copy link
Member

commented Oct 16, 2017

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

@@ -62,6 +89,22 @@ public abstract partial class WebRequestPSCmdlet : PSCmdlet
#region Authorization and Credentials

/// <summary>
/// gets or sets the AllowUnencryptedAuthentication property

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 17, 2017

Collaborator

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.

@@ -94,6 +137,12 @@ public abstract partial class WebRequestPSCmdlet : PSCmdlet
[Parameter]
public virtual SwitchParameter SkipCertificateCheck { get; set; }

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 17, 2017

Collaborator

The same "get" -> "Gets".

@@ -120,6 +120,21 @@
<data name="AccessDenied" xml:space="preserve">
<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>

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 17, 2017

Collaborator

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.

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 17, 2017

Author Collaborator

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.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 17, 2017

Collaborator

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

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 17, 2017

Author Collaborator

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?

This comment has been minimized.

Copy link
@markekraus

markekraus Oct 17, 2017

Author Collaborator

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2017

😄 I've been dragged the PR.

@markekraus markekraus referenced this pull request Oct 20, 2017
2 of 8 tasks complete

@markekraus markekraus deleted the markekraus:BasicAuthentication branch Jan 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.