[core-auth] Add AzureNamedKeyCredential class and NamedKeyCredential interface #14371
Conversation
chradek
requested review from
bterlson,
daviwil,
sadasant and
xirzec
as code owners
ghost
added
the
| /** | ||
| * The value of the key to be used in authentication. | ||
| */ | ||
| public get key(): string { |
There was a problem hiding this comment.
any particular reason these are getters instead of normal props?
There was a problem hiding this comment.
We were wanting the user to have to call update() to update the key, and otherwise treat these as readonly properties. This is the pattern the other credentials in this package (e.g. AzureKeyCredential) follows as well.
| * @param newName - The new name value to be used. | ||
| * @param newKey - The new key value to be used. | ||
| */ | ||
| public update(newName: string, newKey: string): void { |
There was a problem hiding this comment.
what if I just want to update one? Leaving the other empty seems like a not unreasonable contract compared to having to set it to its existing value
There was a problem hiding this comment.
In this case I went just based on the requirements for the linked issue:
Allowing the name or key to be updated independently; both elements will be required when updating a credential.
I think it may make the API a bit confusing if we allow setting them separately.
I'd expect the key to change more frequently than the name (e.g. you've rotated your credentials for a given name.) Only providing the key to update would look like cred.update(undefined, newKey) then, instead of cred.update(cred.name, newKey). I can't think of a scenario where you'd change the name, but not the key.
I could obviously flip the position of name/key...but this would be opposite of what the other languages are doing, and mentally I think of the key being associated to the policy name, not the other way around.
I could expose updateKey and updateName methods, in addition to update, but that's 3 times the surface area now!
There was a problem hiding this comment.
I'm just a little confused, since I would expect updating the key to be way more common than the name, but perhaps I'm not understanding the name field's purpose very well.
I agree that it's better to not ship too much surface area at once, but are other languages also fine with this kinda clunky contract?
There was a problem hiding this comment.
@jsquire
Pinging you since you created the issue 😄 What was the reason you didn't want to support updating name/key separately? Was it just to be concurrency safe?
There was a problem hiding this comment.
What was the reason you didn't want to support updating name/key separately? Was it just to be concurrency safe?
Yes; the update/read is intended to be atomic to avoid race conditions with concurrent use. For those languages with threads, allowing them to be set individually offers the chance for inconsistent data.
but are other languages also fine with this kinda clunky contract?
This was the design that the architects approved, with the trade-offs made to favor safe concurrent use. The goal was to ensure that we didn't provide overloads that callers may be tempted to use in a manner that could produce undesired results. You may want to have a conversation with Brian since threading isn't a consideration here.
perhaps I'm not understanding the name field's purpose very well.
The credential is intended for use when a service requires both the key name and key value to generate the value needed for authorization. The name isn't meant to be informational, but part of the auth flow. Not sure if that helps at all... 😄
what if I just want to update one? Leaving the other empty seems like a not unreasonable contract compared to having to set it to its existing value
The intention is that updates will always provide both values; essentially, you're fully reinitializing the state. If the name is not provided, I would expect that the current value be updated to undefined|null|whatever.
I'm just a little confused, since I would expect updating the key to be way more common than the name
I would agree. However, it is a valid use case to generate a new access policy and need to replace the existing one, which requires the name and key.
There was a problem hiding this comment.
I'm once again relieved to not have to think about threading concerns in JS. :)
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
Eh, seems like we're stuck in this place for consistency. At least the interface is clean.
| * @param newName - The new name value to be used. | ||
| * @param newKey - The new key value to be used. | ||
| */ | ||
| public update(newName: string, newKey: string): void { |
There was a problem hiding this comment.
I'm once again relieved to not have to think about threading concerns in JS. :)
3 participants
Resolves #14303
Summary
This PR adds the
AzureNamedKeyCredentialclass andNamedKeyCredentialinterface to the@azure/core-authpacakge.Details are listed in the linked issue, but the TL;DR is:
When using a shared key for authorization, some services require the name of the key be used as part of the algorithm to form the authorization token.
Since
AzureKeyCredentialandAzureSasCredentialonly track a single value, we've addedAzureNamedKeyCredentialthat lets us track 2 values! (Let's hope there's never 3 values needed!)This credential will be used by the event hubs and service bus packages to start.