Challenge-based authentication#4141
Conversation
|
FYI @Azure/adp-keyvault |
| let retryOptions: any = {}; | ||
| let pipelineOptions: any = {}; | ||
| const requestPolicyFactories: RequestPolicyFactory[] = [ | ||
| proxyPolicy(getDefaultProxySettings((pipelineOptions.proxyOptions || {}).proxySettings)), |
There was a problem hiding this comment.
I'm not sure if this is valid feedback, but here it is:
Could we export something from our core that would abstract all of these policies away? Something optional, but that would help users avoid this level of complexity.
There was a problem hiding this comment.
Possibly. Though some policies we're using will be specific to key vault. Most of the policies do live in core-http already.
| super(nextPolicy, options); | ||
| } | ||
|
|
||
| private parseWWWAuthenticate(www_authenticate: string): string { |
There was a problem hiding this comment.
This code seems arbitrary. I believe it's not arbitrary at all! I'm guessing it will help to provide examples of the input that is expected in the form of comments. What do you think? Or a link to an external reference, if available.
There was a problem hiding this comment.
I could comment this, good call. Right now it assumes you know what the WWW-Authenticate string looks like
|
|
||
| if (www_authenticate) { | ||
| let resource = this.parseWWWAuthenticate(www_authenticate); | ||
| let challenge = new AuthenticationChallenge(resource + "/.default") |
There was a problem hiding this comment.
Is this ./default string always going to exist here? I'm curious why would the underlying resource not provide this.
There was a problem hiding this comment.
This is part of the protocol for doing the challenge.
| // Use a blank to start the challenge | ||
| let headers = webResource.headers; | ||
| webResource.headers = new HttpHeaders(); | ||
| webResource.body = ""; |
There was a problem hiding this comment.
Does this elicit a challenge for every request?
There was a problem hiding this comment.
Ah, good point. I thought I had fixed it to check for the existing challenge before starting one. Will fix.
There was a problem hiding this comment.
Circling back (I had to page back in what I did) - this uses the cached challenge for each request. So it runs:
await this.authenticateRequest(webResource);If there is a cached challenge, this will set up the headers for us. Next, we send the request. If the request comes back and isn't a redirection, we just return the result (because we've now used the challenge to do the next policy in the chain)
There was a problem hiding this comment.
(saw the bug I think you were pointing out. I think I fixed it)
| if (this.challenge != challenge) { | ||
| this.challenge = challenge; | ||
|
|
||
| await this.authenticateRequest(webResource); |
There was a problem hiding this comment.
I think you want to also clear the cachedToken in this case so we'll re-authenticate.
sophiajt
changed the title
| let headers = webResource.headers; | ||
| if (this.challenge == undefined) { | ||
| // Use a blank to start the challenge | ||
| webResource.headers = new HttpHeaders(); |
There was a problem hiding this comment.
webResource.headers = new HttpHeaders(); [](start = 6, length = 40)
This should actually use the headers from the original request. At the very least the Host header is needed to correctly identity the vault (different vaults may have different auth schemes), but it's safe just to use all the headers.
5 participants
This adds challenge-based authentication to the default pipeline for keyvault.