Added support for logging string to sign in Storage and ability to pass in account name when generating a UserDelegationKey by gapra-msft · Pull Request #18075 · Azure/azure-sdk-for-java

gapra-msft · 2020-12-10T18:36:38Z

Resolves #17935 and
Resolves #6713

gapra-msft · 2020-12-10T18:36:55Z

        UserDelegationKey userDelegationKey) {
        return new BlobSasImplUtil(blobServiceSasSignatureValues, getBlobContainerName())
-            .generateUserDelegationSas(userDelegationKey, getAccountName());
+            .generateUserDelegationSas(userDelegationKey, getAccountName(), Context.NONE);


Will add here

gapra-msft · 2020-12-10T18:37:22Z


        when:
-        bc.setMetadata(metadata)
+        bc.setMetadataWithResponse(metadata, null, null, new com.azure.core.util.Context("Log-String-To-Sign", true))


alzimmermsft · 2020-12-10T19:53:47Z

        final String canonicalName = getCanonicalName(storageSharedKeyCredentials.getAccountName());
-        final String signature = storageSharedKeyCredentials.computeHmac256(stringToSign(canonicalName));
+        final String stringToSign = stringToSign(canonicalName);
+        if (context != null && Boolean.TRUE.equals(context.getData("Log-String-To-Sign").orElse(false))) {


I would use a different key such as azure-log-string-to-sign to ensure there is no overlap and to indicate it's Azure SDK specific

alzimmermsft · 2020-12-10T19:54:49Z

+        if (context != null && Boolean.TRUE.equals(context.getData("Log-String-To-Sign").orElse(false))) {
+            logger.info("The string to sign computed by the SDK is: {}{}", stringToSign,
+                System.getProperty("line.separator"));
+            logger.warning("Please remember to disable 'Log-String-To-Sign' before going to production as this "
+                + "string can potentially contain PII.");
+        }


Since this is in multiple spots let's make it a method so if it needs to be updated only one location needs modification

alzimmermsft · 2020-12-10T19:58:29Z

+    private static String convertExceptionMessage(String message, HttpResponse response) {
+        if (response != null) {
+            if (response.getStatusCode() == 403) {
+                return message + "\nIf you are using a StorageSharedKeyCredential, and the server returned an "


Should we use String.format here with line separators instead of \n or are we using '\n' as that would be what is returned by the service.

I think the service returns \n so its fine if we leave this as \n

Changed to %n as CI was angry

rickle-msft · 2020-12-14T22:38:51Z


-    private String buildStringToSign(URL requestURL, String httpMethod, Map<String, String> headers) {
+    private String buildStringToSign(URL requestURL, String httpMethod, Map<String, String> headers,
+        boolean logStringToSign) {


Why not accept the context itself instead of a boolean?

The context in the pipeline policy isnt an AzureContext, its like a HttpPipelineContext so I'd need to do that weird conversion somewhere. Would you prefer I do it in the pipeline policy?

HttpPipelineContext is the combination of HttpRequest and Context where the Context APIs are wrapped and hidden by the HttpPipelineContext APIs, in the end it should contain all information from the Context.

alzimmermsft · 2020-12-17T18:22:03Z

+     *
+     * @param blobServiceSasSignatureValues {@link BlobServiceSasSignatureValues}
+     * @param userDelegationKey A {@link UserDelegationKey} object used to sign the SAS values.
+     * @see BlobServiceAsyncClient#getUserDelegationKey(OffsetDateTime, OffsetDateTime) for more information on how to


I would change this to an {@link} as @see serves a different purpose to link to a class with its own special section. This is an example: https://azuresdkdocs.blob.core.windows.net/$web/java/azure-core-http-netty/1.7.0/com/azure/core/http/netty/NettyAsyncHttpClientBuilder.html

alzimmermsft · 2020-12-17T18:22:42Z

+     * @param accountName The account name.
+     * @param context Additional context that is passed through the code when generating a SAS.
+     *
+     * @return A {@code String} representing all SAS query parameters.


Does it represent all SAS query parameters or is it the SAS query parameters string?

It should be 'the'. I've fixed it.

alzimmermsft · 2020-12-17T18:23:40Z

+
+    /**
+     * Generates a service SAS for the container using the specified {@link BlobServiceSasSignatureValues}
+     * Note : The client must be authenticated via {@link StorageSharedKeyCredential}


This will generate on the same line as the sentence above due to Javadocs being HTML, let's make this another <p> so get a line break.

alzimmermsft · 2020-12-17T18:24:16Z

+     *
+     * @param blobServiceSasSignatureValues {@link BlobServiceSasSignatureValues}
+     * @param userDelegationKey A {@link UserDelegationKey} object used to sign the SAS values.
+     * @see BlobServiceClient#getUserDelegationKey(OffsetDateTime, OffsetDateTime) for more information on how to get a


Same comment as the other Javadoc with inlined @see

alzimmermsft · 2020-12-17T18:29:51Z

+     *
+     * @return A {@code String} representing all SAS query parameters.
+     */
+    public String generateUserDelegationSas(BlobServiceSasSignatureValues blobServiceSasSignatureValues,


Is there going to be an issue that this method assumes the container name from the BlobContainerClient instance but then allows for any account name to be passed? This could result in cases where the container doesn't exist in the other account, and given that to me this method feels closer to a static utility which allows generating a SAS for any account and container given the UserDelegationKey.

I think thats a valid concern. The reason we wanted to put account name here is specifically for situations where a user passes in a custom domain name, and we can't parse an account name as a result.

alzimmermsft · 2020-12-17T18:31:52Z

+     *
+     * @return A {@code String} representing all SAS query parameters.
+     */
+    public String generateUserDelegationSas(BlobServiceSasSignatureValues blobServiceSasSignatureValues,


Same comment as BlobContainerClient around this being a mix of external values and values on the instance, it ends up being an in-between of static utility to generate any SAS given the delegation key and requested permissions but also being information specific to the blob such as container, name, version, and snapshot.

alzimmermsft · 2020-12-17T18:34:36Z


-    private String buildStringToSign(URL requestURL, String httpMethod, Map<String, String> headers) {
+    private String buildStringToSign(URL requestURL, String httpMethod, Map<String, String> headers,
+        boolean logStringToSign) {


HttpPipelineContext is the combination of HttpRequest and Context where the Context APIs are wrapped and hidden by the HttpPipelineContext APIs, in the end it should contain all information from the Context.

alzimmermsft · 2020-12-17T18:37:14Z

+    public static void logStringToSign(ClientLogger logger, String stringToSign, Context context) {
+        if (context != null && Boolean.TRUE.equals(context.getData(Constants.STORAGE_LOG_STRING_TO_SIGN).orElse(false))) {
+            logger.info("The string to sign computed by the SDK is: {}{}", stringToSign,
+                System.getProperty("line.separator"));


Should use System.lineSeparator() instead as it is a convenience wrapper for this.

alzimmermsft · 2020-12-17T22:36:21Z

+
+        if (logStringToSign) {
+            StorageImplUtils.logStringToSign(logger, stringToSign,
+                new Context(Constants.STORAGE_LOG_STRING_TO_SIGN, true));


Let's make this a constant so we don't need to instantiate a new instance every time this code path is executed

alzimmermsft · 2020-12-17T22:37:21Z

+    public static String convertStorageExceptionMessage(String message, HttpResponse response) {
+        if (response != null) {
+            if (response.getStatusCode() == 403) {
+                return String.format("If you are using a StorageSharedKeyCredential, and the server returned an "


Let's make this base message a constant as all parameters for the formatting are constant, that way we don't need to make additional calls to String.format and wait for this to be optimized by JIT

alzimmermsft · 2020-12-17T22:38:07Z

+        if (context != null && Boolean.TRUE.equals(context.getData(Constants.STORAGE_LOG_STRING_TO_SIGN).orElse(false))) {
+            logger.info("The string to sign computed by the SDK is: {}{}", stringToSign,
+                System.lineSeparator());
+            logger.warning("Please remember to disable '{}' before going to production as this "


Let's make this log message a constant string as it doesn't have any instance data

gapra-msft added 2 commits December 9, 2020 11:42

Added code to log string to sign

9e63127

Made logging log properly

c5af4c5

ghost added the Storage Storage Service (Queues, Blobs, Files) label Dec 10, 2020

gapra-msft commented Dec 10, 2020

View reviewed changes

alzimmermsft reviewed Dec 10, 2020

View reviewed changes

Added logging for blobs

2d870a3

gapra-msft changed the title ~~Added support for logging string to sign in Storage~~ Added support for logging string to sign in Storage and ability to pass in account name when generating a UserDelegationKey Dec 10, 2020

gapra-msft added 8 commits December 10, 2020 15:47

Added to datalake

ceb8c13

Added to file share

20ac48c

Added to queue

eacdf8a

Analyze step

fbb1f4d

Fixed some samples

a2c713e

Analuze stepo

ee0f4fe

null check

f7ef6d7

Fixed analyze step

6df6ce7

gapra-msft marked this pull request as ready for review December 14, 2020 18:35

gapra-msft requested review from amishra-dev, jaschrep-msft, kasobol-msft and rickle-msft as code owners December 14, 2020 18:35

gapra-msft added 2 commits December 14, 2020 10:37

Revert blob test

d36b680

Changed test

e1a6169

rickle-msft reviewed Dec 14, 2020

View reviewed changes

rickle-msft approved these changes Dec 14, 2020

View reviewed changes

alzimmermsft reviewed Dec 17, 2020

View reviewed changes

gapra-msft added 2 commits December 17, 2020 13:08

Modified javadoc

63d3fcf

Fixed more javadoc

f2b6024

alzimmermsft reviewed Dec 17, 2020

View reviewed changes

alzimmermsft approved these changes Dec 17, 2020

View reviewed changes

gapra-msft added 2 commits December 17, 2020 15:01

Made some constants

f2051a8

removed import

67f30d6

gapra-msft merged commit 49cddb6 into Azure:master Dec 17, 2020

gapra-msft deleted the storage/logStringToSign branch December 17, 2020 23:55

Uh oh!

Conversation

gapra-msft commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alzimmermsft Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gapra-msft commented Dec 10, 2020 •

edited

Loading

alzimmermsft Dec 17, 2020 •

edited

Loading