[Service Bus Client] Shared Access Key Credential#16085
Conversation
jsquire
added
Service Bus
Client
jsquire
requested review from
AlexGhiondea,
HarshaNalluru,
JoshLove-msft,
KieranBrantnerMagee,
MiYanni,
YijunXieMS,
conniey,
ramya-rao-a,
richardpark-msft and
yunhaoling
jsquire
requested review from
ShivangiReja,
YalinLi0312,
allenjzhang,
bquantump,
m-nash,
markcowl and
nisha-bhatia
as code owners
jsquire
force-pushed
the
servicebus/credential
branch
from
b3d6897 to
1bacce6
Compare
|
/azp run net - servicebus - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jsquire
requested review from
hemanttanwar
and removed request for
ShivangiReja,
YalinLi0312,
YijunXieMS,
bquantump,
m-nash and
nisha-bhatia
tg-msft
left a comment
tg-msft
left a comment
There was a problem hiding this comment.
I'd like to align a little more with other credentials.
There was a problem hiding this comment.
I think we should split this into two types like we do with other key/signature credentials.
There was a problem hiding this comment.
Can you provide pointers to the other credentials you are referring to here? Is the reason that one wouldn't think of using this credential if all they had was the signature (because the name says "key")? Would a rename then help? ServiceBusSharedAccessCredential? ServiceBusSharedAccessPolicyCredential?
There was a problem hiding this comment.
I'm not sure that I understand what the benefit to developers using the library would be in splitting this. As these are elements that are often sibling concepts within a connection string, there is precedent for their association. I would strongly prefer to avoid introducing another constructor overload or two additional public types (base class, other credential) into an already crowded space.
There was a problem hiding this comment.
Storage has a StorageSharedKeyCredential and we're looking at StorageSharedAccessSignatureCredential to match EventGrid's. And the benefit to splitting it is make auth, a historically difficult topic, as clear as possible.
There was a problem hiding this comment.
This one I do feel strongly about. I don't believe that splitting this offers greater usability, and I do believe that the additional types and overloads degrade it. This, I'd very much like to push back on.
There was a problem hiding this comment.
Why are the Key and Signature public? We should keep them internal.
There was a problem hiding this comment.
Fair point..
@jsquire Other than giving the user the ability to check what the current value of their key/signature is if they were ever rotating them, is there any other reason to have these public? Is there a security angle here where we would not want to make them public to avoid the case of this info getting logged accidentally?
There was a problem hiding this comment.
The intent is to allow inspection by callers holding the token, as context for making the decision of "is this using the latest information or do I need to rotate" as well as debugging authorization failures by understanding the same context.
What would the benefit of hiding this information from developers using the library?
There was a problem hiding this comment.
We've done this for other credentials because although it's not a security issue, we want customers to be thoughtful about how they manage their secrets and we make it inconvenient to read this info from our types.
There was a problem hiding this comment.
I don't agree with the rationale, but I don't feel strongly enough to push back on it. I'm comfortable deferring to @ramya-rao-a , assuming that @tg-msft has cleared this with @KrzysztofCwalina.
There was a problem hiding this comment.
Why not just Update to match everyone else?
There was a problem hiding this comment.
Can you share who everyone else is here? Are you suggesting a single Update method instead of two?
There was a problem hiding this comment.
At one point we had considered a single update method with an overload; if I recall, there was some concern that callers may be confused and the consensus was that having the name explicitly reflect the intent was more clear.
There was a problem hiding this comment.
Perhaps @tg-msft is referring to AzureKeyCredential?
There was a problem hiding this comment.
There is also StorageSharedKeyCredential, though the method is called SetAccountKey.
There was a problem hiding this comment.
It appears as if AzureKeyCredential is intended for API Key scenarios rather than SAS-type scenarios, based on samples that use it. For example, Azure.Search and Text Analytics
There was a problem hiding this comment.
We called it SetAccountKey in Storage first and then decided we'd be doing Update for all other credentials. And you have a shared key here so you're doing the same.
There was a problem hiding this comment.
I don't feel strongly here, though I do tend to favor the explicit naming. I'm comfortable deferring to @ramya-rao-a, assuming that @tg-msft has cleared this with @KrzysztofCwalina.
This design was the result of several rounds of conversation, feedback, and revision across the feature team, service team, and language architects. As this form was agreed upon by those parties, I'm going to respectfully decline reopening design discussions. |
The focus of these changes is to implement the `ServiceBusSharedAccessKeyCredential`, allowing SAS tokens and shared keys to be used without a connection string.
jsquire
force-pushed
the
servicebus/credential
branch
from
1bacce6 to
09d146f
Compare
|
Per offline conversations, I'm going to merge this in and we'll adjust based on the pending architecture board discussions in a separate work stream. |
The focus of these changes is to implement the `ServiceBusSharedAccessKeyCredential`, allowing SAS tokens and shared keys to be used without a connection string.
[Device Update for IoT Hub] Added Private Endpoint Connection API and model definitions (Azure#16085) * Added Private Endpoint Connection API and model definitions, fixed minor errors * Referenced common private link types * Reference list types do not have nextLink property * Fixed copy&paste description error * Trying to suppress an error * Change common types version to v3 * Suppression not working * Extended common type in order to add a required property
5 participants
Summary
The focus of these changes is to implement the
ServiceBusSharedAccessKeyCredential, allowing SAS tokens and shared keys to be used without a connection string.Note: These changes have been reviewed and approved by the architecture board and the .NET language architect. Marking with the corresponding tag for tracking purposes.
Last Upstream Rebase
Monday, October 19, 1:34pm (EDT)
References