Repository navigation
Web Cmdlets should warn when legacy -Credential is sent over unencrypted connections #5112
Description
Activity
The wording of
-AllowUnencryptedAuthenticationdoesn't make sense for Warning. By issuing a warning, you are by default allowing unencrypted. I propose an-IgnoreUnencryptedWarningshould be used if Warning is chosen. The Error proposal should continue with-AllowUnencryptedAuthentication.I disagree with
-IgnoreUnencryptedWarning.-AllowUnencryptedAuthenticationwill already be present and there is no need to add a second parameter to do the same thing. There were already discussions and back and forth on-AllowUnencryptedAuthenticationand I think that debate does not need to be revisited.-AllowUnencryptedAuthenticationis a parameter switch to do what exactly? I thought it was a new parameter that was pulled out from previous PR specifically for this discussion of whether a Warning or Error should happen if no parameter is specified.What I'm suggesting is that for Warning Proposal, it is very confusing and doesn't make any sense. If I run
Invoke-WebRequest -Credentialand it works with a warning displayed then you have by default allowed it to happen. So why would I need to set-AllowUnencryptedAuthentication? I never would.Now, if you want
Confirmationto happen so the user interactively chooses yes or no. Then it makes sense to have an-AllowUnencryptedAuthenticationswitch to hide confirmation and choose yes.Error Proposal is fine, I just think the wording doesn't make sense if credentials are still sent with just a warning let the user know it happened.
What I'm suggesting is that for Warning Proposal
And what I'm saying is there is no point in having yet another parameter that does the same thing. If
-AllowUnencryptedAuthenticationdidn't already exist, then yea, maybe something else. But there is no point in two parameters doing the same thing. Seeing both-IgnoreUnencryptedWarningand-AllowUnencryptedAuthenticationwould be awkward.The warning would be temporary. The end game is removing the legacy usage, IMO. For that temporary period of time having using the same parameter even if the wording doesn't suit everyone, is better than adding another parameter that will just end up being removed.
-AllowUnencryptedAuthenticationis a parameter switch to do what exactly?The new
-Authenticationparameter options give a terminating error for non-https and can only by suppressed with-AllowUnencryptedAuthentication.I think you are separating out logic that shouldn't be separated.
-Credentialand-Authenticationbehavior should match when sent over unencrypted. I think that is where my confusion is coming in. Having one error by default and the other give a warning would be inconsistent. So either both error, both warn, or both ask confirmation. Maybe that is what your 3rd proposal about-Credentialis about?Also, you are not proposing removing the
-Credentialparameter right, just cleaning up the code on how it used?I think you are separating out logic that shouldn't be separated.
Having one error by default and the other give a warning would be inconsistent.
That's to avoid a breaking change now. Ultimately, there will be a breaking change. But, we can go this route with a warning temporarily an avoid breaking it until we have to.
Maybe that is what your 3rd proposal about -Credential is about?
That's exactly it. Basically,
-Credentialhas 2 uses now. The legacy use where it is just-Credentialand the new use where it is-Authentication Basic -Credential. The legacy use only sends theAuthorizationheader when a web server has issues a401andWWW-Authenticate: Basic realm=<realm>. Many APIs and OAuth grant flows requireBasicauth without the401orWWW-Authenticate: Basic. The legacy never errors. The new usage allows for sending theAuthorizationheader always and it gives a terminating error when you try anything other that HTTPS unless you provide the switch.My goal (what I think is the right direction) is to remove the legacy usage and just standardize on the
-Authenticationparameter. That way we can add more authentication methods which may or may not use the-Credentialparameter (such as OAuth/Bearer use of-Token). But that is not a trivial task. For one it is a significant breaking change and for another there is a lot of code cleanup associated with it. So getting that done before 6.0.0 RTM when it's not really a priority is unlikely.So that is what the 3rd option is about. It's more of a "do nothing until we do it right and when we can doit right, this is how". But I would rather up the security game in the Web Cmdlets before 6.0.0 RTM even if some of those measure are temporary.
Also, you are not proposing removing the -Credential parameter right, just cleaning up the code on how it used?
No, it will stay. But, to use it, in the future you will need to supply
-Authentication Basic(or maybe a few other flavors of-Authentication <option>which haven't been discussed yet). For now, the legacy behavior is left in.Digging into this a bit, removing the legacy usage isn't as simple and I thought. HttpClient apparently does some magic authentication for Basic, NTLM, and Digest using these credentials and it handles some automated detection and resolution, something I'd rather not bring into the web cmdlets. Might be able to get as way with a
-Authentication BuiltInor something like that.@PowerShell/powershell-committee reviewed this. desire is that we can use
-Authentication Negotiate(which is default for compat reasons) to handle the builtin dotnet auth. and throwing a terminating error (telling them about-AllowEncryptedAuthenticationswitch) if-urlis "http:" if-AllowUnencryptedAuthenticationis not used.7 remaining items
Mark Kraus (@markekraus) since this deviated a bit from the original expectation from the Committee, let me bring it back up.
Thanks! I would have had a more fleshed out proposal for this.. but I didn't plan on breaking this until later, all I wanted for now was possibly an error on
-Credential/-UseDefaultCredentialon http. but if we are going to break it.. we might as well break it all the way.Can we do this?
-AuthisDefaultif not specified, but meaning ofDefaultchanges depending on whether-Credis used. No credDefault = none, with credDefault = negotiate. There is no explicit none or negotiate for-Auth.Mark Kraus (@markekraus) I'm actually fine with the full break proposal you made, but I think we need another +1.
Steve Lee (@SteveL-MSFT) that seems a bit harder to follow intuitively. The error messages would have to be really good if we went that route. :)
Steve Lee (@SteveL-MSFT) My objection to doing that kind of logic (
Default's behavior changing on whetherCredentialis provided or not) is that it relies on checking for bound parameters as opposed to Parameter value. It's also complicated by-UseDefaultCredential. if we could use parameter sets with the cmdlets, this might have been ok except that it would result in the need for 12 more parameter sets (at least). It's basically the same reason i was against usingNegotiateas the default. It leads to a bunch of ugly hacky logic that would normally be offloaded to the parameter binding.I also agree with Joey Aiello (@joeyaiello) that its a bit tough to follow. both from a coding and a user perspective.
Perhaps for now, the best thing to do is simply return an error if
-AllowUnencryptedAuthisn't used and the-URIis http. Let's leave the other discussion for now which means we'll just have to have that inconsistency.Steve Lee (@SteveL-MSFT) that is certainly my minimum ask.
The breaking change for
-Credential, is that something that would be possibly for 6.1/6.2? or is that something that would need to tabled until the next major version?Mark Kraus (@markekraus) ideally, I'd like to have telemetry in the future to show that only a small set of customers were using the legacy
-Credentialwith PSCore6 so we can deprecate it and eventually remove it. At this point, we can worry about that later and accept a possibility that we have to just live with it.Steve Lee (@SteveL-MSFT) my only fear is that the situations where
-Credentialand-UseDefaultCredentialswould be used also don't lend themselves well to telemetry: internal applications. They are currently the only means for NTLM and Digest authentication and those kinds of sites and the automation working against them may not have internet access to dial home. I'd rather keep it by breaking the current usage and move it to the new-Authenticationusage than to break access to legacy systems. *shrugsBut, I'm OK with a PR for just the error on non-https
-Uriwithout-AllowUnencryptedAuthfor now and living with the legacy behavior.#5402 for the non-HTTPS
-Credential(legacy) and-UseDefaultCredentialserrors
Problem
In #5052 we are introducing the new
-Authenticationparameter which include a terminating error when any scheme other thanhttps://is provided in the URI. The legacy-Credentialusage currently does not offer any warnings or errors when the secrets are sent over an unencrypted connection.This issue is to track and discuss which method to go with.
Possible Solutions
Add a warning
This solution would add a warning message (via
WriteWarning()) that the use could suppress with the-AllowUnencryptedAuthenticationparameter. This would likely be a non-breaking change that would simply the user politely when they use the legacy-Credentialand something other than'https://Add an Error
This is similar to the previous but instead return an error. This could be a terminating or non-terminating error, but either would be a breaking change. A common usage of the web cmdlets is to use
-ErrorAction Stopin a try/catch and this would introduce new stops for previously working code if users were sending credentials over HTTP beforeRemove the legacy
-Credentialusage.The new
-Authenticationusage has some duplication of functionality in that it does the same thing on itsBasicoption as the legacy-Credentialusage. Legacy-Credentialwould only sent the Authorization header when the server present an Authorization request where the new method always sends the Authorization header (as many OAuth systems do not present auth realm). This would require some discovery and clean up. I think this is the ultimate choice, but probably not a good candidate for 6.0.0 RTM.