[Feature] To be able to rotate subscription key #7596

mssfang · 2020-01-22T00:36:24Z

Add the ability to rotate the subscription key.
In addition, increase test coverage to TextAnalyticsClientBuilder.

JonathanGiles

From a Java point of view, this all looks fine. I would like @g2vinay and @schaabs to weigh in on how this fits into their vision for auth and identity before this is made public API, though.

srnagar · 2020-01-22T21:11:36Z

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

+     * @param subscriptionKey the subscription key
+     * @return the {@link TextAnalyticsSubscriptionKeyCredential} itself
+     */
+    public TextAnalyticsSubscriptionKeyCredential updateCredential(String subscriptionKey) {


Don't think returning TextAnalyticsSubscriptionKeyCredential is necessary here. Returning void should be sufficient.

I am not sure if we agreed on the terminology here in previous arch-board review - there were several proposals refresh, update, set, rotate etc.

srnagar · 2020-01-22T21:15:32Z

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

+     * @param subscriptionKey the subscription key
+     * @return the {@link TextAnalyticsSubscriptionKeyCredential} itself
+     */
+    public TextAnalyticsSubscriptionKeyCredential updateCredential(String subscriptionKey) {


Also, when is this method called? Is this method expected to be called by the user when they want to update the key? If so, it would be good to include in samples and code snippets.

I will add the samples and snippet.

srnagar · 2020-01-22T21:19:20Z

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

+package com.azure.ai.textanalytics.models;
+
+/**
+ * Subscription key credential that shared across cognitive services, or restrict to single service.


Is this applicable for all cognitive services? If so, is the plan to duplicate this credential in all cognitive services modules or would we create a common module like storage has to put the common stuff there?

We had a discussion about having a common module. We decided not to have a common module now because currently we only have one service implemented. We don't really know the use case.

@mssfang is right - this is still an open question. We're starting to look at FormRecognizer now, though, so we'll start driving the "where would we put a common Cognitive credential" with the architects.

srnagar · 2020-01-22T21:23:56Z

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

+     * @return the {@link TextAnalyticsSubscriptionKeyCredential} itself
+     */
+    public TextAnalyticsSubscriptionKeyCredential updateCredential(String subscriptionKey) {
+        this.subscriptionKey = subscriptionKey;


Can subscriptionKey be null or empty? If not, you may want to check for these conditions here and in the constructor.

Agreed. Will add the check

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

maririos · 2020-01-22T21:41:30Z

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

+     * @return the {@link TextAnalyticsSubscriptionKeyCredential} itself
+     */
+    public TextAnalyticsSubscriptionKeyCredential updateCredential(String subscriptionKey) {
+        this.subscriptionKey = subscriptionKey;


Is this thread safe? Is there a way to guarantee that when modified it updates everywhere?

I had a few discussions on the thread-safe concern. some believe it is thread-safe. the reason is it is a single atomic assignment. But depends on how you use it, It could be not threaded safe. This is one I am not sure yet. I will give you an update once finalized.

In the pipeline policy, it is fetched, so it's possible that the policy picks up a different value than the one that is set. This is not thread safe.

sdk/textanalytics/azure-ai-textanalytics/README.md

...src/main/java/com/azure/ai/textanalytics/implementation/SubscriptionKeyCredentialPolicy.java

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

...i-textanalytics/src/test/java/com/azure/ai/textanalytics/TextAnalyticsClientBuilderTest.java

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

g2vinay · 2020-01-24T01:36:36Z

Looking at the

From a Java point of view, this all looks fine. I would like @g2vinay and @schaabs to weigh in on how this fits into their vision for auth and identity before this is made public API, though.

Looking at the authentication model for text analytics, a subscription key is passed via header and that's it (it acts as the access token).
TLS encryption will be applied during network calls so subscription key can't be easily intercepted.
Its worth to ensure/add the check in the code that the resource endpoint we're hitting starts with 'https', to ensure TLS protocol comes into play during network calls.

Currently, in Azure Identity there is no support to rotate any of the credentials it supports. The credential needs to be restarted.
In this PR too, the rotation process is manual for the user via the update call on the subscription key credential. So, that looks fine to me.

JonathanGiles · 2020-01-24T03:15:32Z

Currently, in Azure Identity there is no support to rotate any of the credentials it supports. The credential needs to be restarted.
In this PR too, the rotation process is manual for the user via the update call on the subscription key credential. So, that looks fine to me.

This was the main point of my request - I do not want this API to be evolving separately than azure-identity intends to evolve, and would therefore like consideration about whether there is any reconciliation that needs to go on here.

...re-ai-textanalytics/src/main/java/com/azure/ai/textanalytics/TextAnalyticsClientBuilder.java

conniey · 2020-01-24T17:43:05Z

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java

+     * @return the {@link TextAnalyticsSubscriptionKeyCredential} itself
+     */
+    public TextAnalyticsSubscriptionKeyCredential updateCredential(String subscriptionKey) {
+        this.subscriptionKey = subscriptionKey;


In the pipeline policy, it is fetched, so it's possible that the policy picks up a different value than the one that is set. This is not thread safe.

conniey · 2020-01-24T17:45:17Z

This was the main point of my request - I do not want this API to be evolving separately than azure-identity intends to evolve, and would therefore like consideration about whether there is any reconciliation that needs to go on here.

I think it would be better to have a rotating credential (either manual or on some cadence, etc.) of sorts in azure-identity. We want to support rotating credentials in Event Hubs, too.

annelo-msft · 2020-01-26T18:28:20Z

Looking at the authentication model for text analytics, a subscription key is passed via header and that's it (it acts as the access token).
TLS encryption will be applied during network calls so subscription key can't be easily intercepted.

Some of the other cognitive services support getting a bearer token from an STS in addition to passing the subscription key. TextAnalytics doesn't currently support this, which is part of why we're scoping this type to TA-only.

…into RotateSubscriptionKey

conniey · 2020-01-28T00:38:21Z

...zure-ai-textanalytics/src/samples/java/com/azure/ai/textanalytics/AnalyzeSentimentAsync.java

-            .subscriptionKey("{subscription_key}")
-            .endpoint("https://{servicename}.cognitiveservices.azure.com/")
+            .subscriptionKey(new TextAnalyticsSubscriptionKeyCredential("{subscription_key}"))
+            .endpoint("{endpoint}")


Why the change to {endpoint}? It is more clear what the string should look like with the last change.

Based on what Mariana point out here: #7596 (comment)

The subscription key could apply to both Cognitive Subscription Key or Text Analytics Subscription key.

conniey · 2020-01-28T00:41:05Z

...azure-ai-textanalytics/src/test/java/com/azure/ai/textanalytics/TextAnalyticsClientTest.java

+    public void nullAADCredential() {
+        assertThrows(NullPointerException.class, () -> {
+            final TextAnalyticsClientBuilder builder = new TextAnalyticsClientBuilder();
+            builder.endpoint(getEndpoint()).credential(null).buildClient();


Generally, you try to limit the scope of your assertion. How do you know it threw in credential(null) and not buildclient()?

This goes for all the other tests that do the same.

// arrange final TextAnalyticsClientBuilder builder = new TextAnalyticsClientBuilder(); // Act & Assert assertThrows(NullPointerException.class, () -> builder.credential(null));

Make sense. I will update it.

maririos · 2020-01-28T03:23:40Z

sdk/textanalytics/azure-ai-textanalytics/README.md

+
+<!-- embedme ./src/samples/java/com/azure/ai/textanalytics/ReadmeSamples.java#L174-L180 -->
+```java
+TextAnalyticsSubscriptionKeyCredential credential = new TextAnalyticsSubscriptionKeyCredential("{expired_subscription_key}");


I wouldn't call it expired_subscription_key as it might give the wrong message to people when creating the client.
it could be just subscription_key, and leave new_subscription_key

maririos

For changes related to rotate subscription key LGTM.

…ure-sdk-for-java into RotateSubscriptionKey

JonathanGiles · 2020-01-29T01:16:26Z

I asked for this to be reviewed and approved by @g2vinay or @schaabs from an identity perspective, but I don't see that approval on this PR. Has this been done?

Edit: I see @g2vinay commented further up.

mssfang · 2020-01-29T03:05:13Z

@JonathanGiles Sorry. No for your question. I will follow up with vinay and scott for further improvement and open a new PR if needed.

Subscription Key Credential objects

624948d

mssfang added Client This issue points to a problem in the data-plane of the library. TextAnalytics labels Jan 22, 2020

mssfang added this to the [2020] February milestone Jan 22, 2020

mssfang self-assigned this Jan 22, 2020

mssfang added 3 commits January 21, 2020 17:02

one used variable can be refactored

9755bc7

readme sample reformat

812ccd3

reformat

559066c

mssfang marked this pull request as ready for review January 22, 2020 07:07

mssfang requested review from JonathanGiles and samvaity as code owners January 22, 2020 07:07

mssfang requested review from conniey, srnagar, kristapratico, maririos, xirzec, willmtemple and annelo-msft January 22, 2020 07:07

JonathanGiles reviewed Jan 22, 2020

View reviewed changes

srnagar reviewed Jan 22, 2020

View reviewed changes

maririos reviewed Jan 22, 2020

View reviewed changes

.../src/main/java/com/azure/ai/textanalytics/models/TextAnalyticsSubscriptionKeyCredential.java Outdated Show resolved Hide resolved

maririos reviewed Jan 22, 2020

View reviewed changes

sdk/textanalytics/azure-ai-textanalytics/README.md Outdated Show resolved Hide resolved

subscription key cannot be null check

eb3befd

hemanttanwar suggested changes Jan 22, 2020

View reviewed changes

conniey reviewed Jan 24, 2020

View reviewed changes

mssfang added 4 commits January 26, 2020 17:14

add json files for test and minor checkstyle errors

1795fb1

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

8e9e492

…into RotateSubscriptionKey

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

77103c8

…into RotateSubscriptionKey

add volatile to subscription key to visisbility

6f3ee3d

mssfang requested a review from anuchandy January 27, 2020 17:37

anuchandy approved these changes Jan 27, 2020

View reviewed changes

update json files and take caure endpoint last character

f408f1c

mssfang requested review from JonathanGiles, maririos, srnagar, anuchandy, hemanttanwar and conniey January 27, 2020 21:31

hemanttanwar approved these changes Jan 28, 2020

View reviewed changes

conniey reviewed Jan 28, 2020

View reviewed changes

maririos reviewed Jan 28, 2020

View reviewed changes

maririos approved these changes Jan 28, 2020

View reviewed changes

mssfang added 2 commits January 27, 2020 20:08

Merge branch 'RotateSubscriptionKey' of https://github.com/mssfang/az…

132815b

…ure-sdk-for-java into RotateSubscriptionKey

workding and delete some dup tests

af5710d

mssfang requested a review from conniey January 28, 2020 04:38

mssfang added 2 commits January 28, 2020 11:59

renanem to TextAnalyticsApiKeyCredential

06013da

resolve conflict

4210cc6

conniey approved these changes Jan 28, 2020

View reviewed changes

mssfang merged commit 980616d into Azure:master Jan 28, 2020

mssfang deleted the RotateSubscriptionKey branch January 28, 2020 22:10

ghost added Cognitive - Text Analytics and removed TextAnalytics labels Jun 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] To be able to rotate subscription key #7596

[Feature] To be able to rotate subscription key #7596

mssfang commented Jan 22, 2020 •

edited

Loading

JonathanGiles left a comment

srnagar Jan 22, 2020

srnagar Jan 22, 2020

srnagar Jan 22, 2020

mssfang Jan 22, 2020

srnagar Jan 22, 2020

mssfang Jan 22, 2020

annelo-msft Jan 26, 2020

srnagar Jan 22, 2020

mssfang Jan 22, 2020

maririos Jan 22, 2020

mssfang Jan 22, 2020

conniey Jan 24, 2020

g2vinay commented Jan 24, 2020

JonathanGiles commented Jan 24, 2020

conniey Jan 24, 2020

conniey commented Jan 24, 2020

annelo-msft commented Jan 26, 2020

conniey Jan 28, 2020

mssfang Jan 28, 2020

conniey Jan 28, 2020

mssfang Jan 28, 2020

maririos Jan 28, 2020

maririos left a comment

JonathanGiles commented Jan 29, 2020 •

edited

Loading

mssfang commented Jan 29, 2020

[Feature] To be able to rotate subscription key #7596

[Feature] To be able to rotate subscription key #7596

Conversation

mssfang commented Jan 22, 2020 • edited Loading

JonathanGiles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g2vinay commented Jan 24, 2020

JonathanGiles commented Jan 24, 2020

Choose a reason for hiding this comment

conniey commented Jan 24, 2020

annelo-msft commented Jan 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maririos left a comment

Choose a reason for hiding this comment

JonathanGiles commented Jan 29, 2020 • edited Loading

mssfang commented Jan 29, 2020

mssfang commented Jan 22, 2020 •

edited

Loading

JonathanGiles commented Jan 29, 2020 •

edited

Loading