Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Storage] SAS version cleanup. #22255

Merged
merged 13 commits into from
Jun 15, 2021
Merged

[Storage] SAS version cleanup. #22255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
db7ebe0
deprecate BlobSasServiceVersion. Make version not settable.
kasobol-msft Jun 11, 2021
1db5100
More of that.
kasobol-msft Jun 11, 2021
d4fb4c7
deprecate old constatnt...
kasobol-msft Jun 11, 2021
8a909ac
fix test.
kasobol-msft Jun 11, 2021
3719f47
rework constants again.
kasobol-msft Jun 14, 2021
6f7eb17
...
kasobol-msft Jun 14, 2021
57b3ea3
let's run some live tests.
kasobol-msft Jun 14, 2021
0cee9ad
fix live tests.
kasobol-msft Jun 14, 2021
aa95458
fix datalake tests.
kasobol-msft Jun 15, 2021
b091801
PR feedback.
kasobol-msft Jun 15, 2021
0f56084
use separate constant for deprecated part.
kasobol-msft Jun 15, 2021
f22d56c
remind us about this problem in files and queues.
kasobol-msft Jun 15, 2021
27178e4
fix ci.
kasobol-msft Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions sdk/storage/azure-storage-blob/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,5 +307,44 @@
</plugins>
</build>
</profile>
<profile>
<id>inject-sas-service-version</id>
<activation>
<property><name>env.AZURE_LIVE_TEST_SERVICE_VERSION</name></property>
</activation>
<build>
<plugins>
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
<version>3.0.0</version> <!-- {x-version-update;org.codehaus.mojo:build-helper-maven-plugin;external_dependency} -->
<executions>
<execution>
<id>regex-property</id>
<goals>
<goal>regex-property</goal>
</goals>
<configuration>
<name>AZURE_STORAGE_SAS_SERVICE_VERSION</name>
<value>${env.AZURE_LIVE_TEST_SERVICE_VERSION}</value>
<regex>V(\d+)_(\d+)_(\d+)</regex>
<replacement>$1-$2-$3</replacement>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.0.0-M3</version> <!-- {x-version-update;org.apache.maven.plugins:maven-surefire-plugin;external_dependency} -->
<configuration>
<systemPropertyVariables>
<AZURE_STORAGE_SAS_SERVICE_VERSION>${AZURE_STORAGE_SAS_SERVICE_VERSION}</AZURE_STORAGE_SAS_SERVICE_VERSION>
</systemPropertyVariables>
</configuration>
</plugin>
</plugins>
</build>
</profile>
</profiles>
</project>
102 changes: 62 additions & 40 deletions ...torage-blob/src/main/java/com/azure/storage/blob/implementation/util/BlobSasImplUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@

package com.azure.storage.blob.implementation.util;

import com.azure.core.util.Configuration;
import com.azure.core.util.Context;
import com.azure.core.util.CoreUtils;
import com.azure.core.util.logging.ClientLogger;
import com.azure.storage.blob.BlobServiceVersion;
import com.azure.storage.blob.models.UserDelegationKey;
import com.azure.storage.blob.sas.BlobContainerSasPermission;
import com.azure.storage.blob.sas.BlobSasPermission;
import com.azure.storage.blob.sas.BlobServiceSasSignatureValues;
import com.azure.storage.blob.sas.BlobSasServiceVersion;
import com.azure.storage.common.StorageSharedKeyCredential;
import com.azure.storage.common.implementation.Constants;
import com.azure.storage.common.implementation.StorageImplUtils;
Expand Down Expand Up @@ -49,9 +50,10 @@ public class BlobSasImplUtil {
*/
private static final String SAS_CONTAINER_CONSTANT = "c";

private final ClientLogger logger = new ClientLogger(BlobSasImplUtil.class);
private static final ClientLogger LOGGER = new ClientLogger(BlobSasImplUtil.class);

private String version;
private static final String VERSION = Configuration.getGlobalConfiguration()
.get(Constants.PROPERTY_AZURE_STORAGE_SAS_SERVICE_VERSION, BlobServiceVersion.getLatest().getVersion());

private SasProtocol protocol;

Expand Down Expand Up @@ -112,10 +114,9 @@ public BlobSasImplUtil(BlobServiceSasSignatureValues sasValues, String container
String snapshotId, String versionId) {
Objects.requireNonNull(sasValues);
if (snapshotId != null && versionId != null) {
throw logger.logExceptionAsError(
throw LOGGER.logExceptionAsError(
new IllegalArgumentException("'snapshot' and 'versionId' cannot be used at the same time."));
}
this.version = null; /* Setting this to null forces the latest service version - see ensureState. */
this.protocol = sasValues.getProtocol();
this.startTime = sasValues.getStartTime();
this.expiryTime = sasValues.getExpiryTime();
Expand Down Expand Up @@ -150,7 +151,7 @@ public String generateSas(StorageSharedKeyCredential storageSharedKeyCredentials
// Signature is generated on the un-url-encoded values.
final String canonicalName = getCanonicalName(storageSharedKeyCredentials.getAccountName());
final String stringToSign = stringToSign(canonicalName);
StorageImplUtils.logStringToSign(logger, stringToSign, context);
StorageImplUtils.logStringToSign(LOGGER, stringToSign, context);
final String signature = storageSharedKeyCredentials.computeHmac256(stringToSign);

return encode(null /* userDelegationKey */, signature);
Expand All @@ -173,7 +174,7 @@ public String generateUserDelegationSas(UserDelegationKey delegationKey, String
// Signature is generated on the un-url-encoded values.
final String canonicalName = getCanonicalName(accountName);
final String stringToSign = stringToSign(delegationKey, canonicalName);
StorageImplUtils.logStringToSign(logger, stringToSign, context);
StorageImplUtils.logStringToSign(LOGGER, stringToSign, context);
String signature = StorageImplUtils.computeHMac256(delegationKey.getValue(), stringToSign);

return encode(delegationKey, signature);
Expand All @@ -192,7 +193,7 @@ private String encode(UserDelegationKey userDelegationKey, String signature) {
*/
StringBuilder sb = new StringBuilder();

tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_SERVICE_VERSION, this.version);
tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_SERVICE_VERSION, VERSION);
tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_PROTOCOL, this.protocol);
tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_START_TIME, formatQueryParameterDate(this.startTime));
tryAppendQueryParameter(sb, Constants.UrlConstants.SAS_EXPIRY_TIME, formatQueryParameterDate(this.expiryTime));
Expand Down Expand Up @@ -246,13 +247,9 @@ private String encode(UserDelegationKey userDelegationKey, String signature) {
* https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/storage/Azure.Storage.Blobs/src/Sas/BlobSasBuilder.cs
*/
private void ensureState() {
if (version == null) {
version = BlobSasServiceVersion.getLatest().getVersion();
}

if (identifier == null) {
if (expiryTime == null || permissions == null) {
throw logger.logExceptionAsError(new IllegalStateException("If identifier is not set, expiry time "
throw LOGGER.logExceptionAsError(new IllegalStateException("If identifier is not set, expiry time "
+ "and permissions must be set"));
}
}
Expand All @@ -279,7 +276,7 @@ private void ensureState() {
break;
default:
// We won't reparse the permissions if we don't know the type.
logger.info("Not re-parsing permissions. Resource type '{}' is unknown.", resource);
LOGGER.info("Not re-parsing permissions. Resource type '{}' is unknown.", resource);
break;
}
}
Expand All @@ -306,7 +303,7 @@ private String stringToSign(String canonicalName) {
this.identifier == null ? "" : this.identifier,
this.sasIpRange == null ? "" : this.sasIpRange.toString(),
this.protocol == null ? "" : this.protocol.toString(),
version,
VERSION,
resource,
versionSegment == null ? "" : versionSegment,
this.cacheControl == null ? "" : this.cacheControl,
Expand All @@ -319,30 +316,55 @@ private String stringToSign(String canonicalName) {

private String stringToSign(final UserDelegationKey key, String canonicalName) {
String versionSegment = this.snapshotId == null ? this.versionId : this.snapshotId;
return String.join("\n",
this.permissions == null ? "" : this.permissions,
this.startTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.startTime),
this.expiryTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.expiryTime),
canonicalName,
key.getSignedObjectId() == null ? "" : key.getSignedObjectId(),
key.getSignedTenantId() == null ? "" : key.getSignedTenantId(),
key.getSignedStart() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedStart()),
key.getSignedExpiry() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedExpiry()),
key.getSignedService() == null ? "" : key.getSignedService(),
key.getSignedVersion() == null ? "" : key.getSignedVersion(),
this.authorizedAadObjectId == null ? "" : this.authorizedAadObjectId,
"", /* suoid - empty since this applies to HNS only accounts. */
this.correlationId == null ? "" : this.correlationId,
this.sasIpRange == null ? "" : this.sasIpRange.toString(),
this.protocol == null ? "" : this.protocol.toString(),
version,
resource,
versionSegment == null ? "" : versionSegment,
this.cacheControl == null ? "" : this.cacheControl,
this.contentDisposition == null ? "" : this.contentDisposition,
this.contentEncoding == null ? "" : this.contentEncoding,
this.contentLanguage == null ? "" : this.contentLanguage,
this.contentType == null ? "" : this.contentType
);
if (VERSION.compareTo(BlobServiceVersion.V2019_12_12.getVersion()) <= 0) {
gapra-msft marked this conversation as resolved.
Show resolved Hide resolved
return String.join("\n",
this.permissions == null ? "" : this.permissions,
this.startTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.startTime),
this.expiryTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.expiryTime),
canonicalName,
key.getSignedObjectId() == null ? "" : key.getSignedObjectId(),
key.getSignedTenantId() == null ? "" : key.getSignedTenantId(),
key.getSignedStart() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedStart()),
key.getSignedExpiry() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedExpiry()),
key.getSignedService() == null ? "" : key.getSignedService(),
key.getSignedVersion() == null ? "" : key.getSignedVersion(),
this.sasIpRange == null ? "" : this.sasIpRange.toString(),
this.protocol == null ? "" : this.protocol.toString(),
VERSION,
resource,
versionSegment == null ? "" : versionSegment,
this.cacheControl == null ? "" : this.cacheControl,
this.contentDisposition == null ? "" : this.contentDisposition,
this.contentEncoding == null ? "" : this.contentEncoding,
this.contentLanguage == null ? "" : this.contentLanguage,
this.contentType == null ? "" : this.contentType
);
} else {
return String.join("\n",
this.permissions == null ? "" : this.permissions,
this.startTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.startTime),
this.expiryTime == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(this.expiryTime),
canonicalName,
key.getSignedObjectId() == null ? "" : key.getSignedObjectId(),
key.getSignedTenantId() == null ? "" : key.getSignedTenantId(),
key.getSignedStart() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedStart()),
key.getSignedExpiry() == null ? "" : Constants.ISO_8601_UTC_DATE_FORMATTER.format(key.getSignedExpiry()),
key.getSignedService() == null ? "" : key.getSignedService(),
key.getSignedVersion() == null ? "" : key.getSignedVersion(),
this.authorizedAadObjectId == null ? "" : this.authorizedAadObjectId,
"", /* suoid - empty since this applies to HNS only accounts. */
this.correlationId == null ? "" : this.correlationId,
this.sasIpRange == null ? "" : this.sasIpRange.toString(),
this.protocol == null ? "" : this.protocol.toString(),
VERSION,
resource,
versionSegment == null ? "" : versionSegment,
this.cacheControl == null ? "" : this.cacheControl,
this.contentDisposition == null ? "" : this.contentDisposition,
this.contentEncoding == null ? "" : this.contentEncoding,
this.contentLanguage == null ? "" : this.contentLanguage,
this.contentType == null ? "" : this.contentType
);
}
}
}
2 changes: 2 additions & 0 deletions ...ge/azure-storage-blob/src/main/java/com/azure/storage/blob/sas/BlobSasServiceVersion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@

/**
* The versions of Azure Storage Blob Sas supported by this client library.
* @deprecated The version is set to the latest version of sas.
*/
@Deprecated
public enum BlobSasServiceVersion implements ServiceVersion {
V2019_02_02("2019-02-02"),
V2019_07_07("2019-07-07"),
Expand Down
31 changes: 17 additions & 14 deletions ...-storage-blob/src/main/java/com/azure/storage/blob/sas/BlobServiceSasSignatureValues.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@

package com.azure.storage.blob.sas;

import com.azure.core.util.Configuration;
import com.azure.core.util.CoreUtils;
import com.azure.core.util.logging.ClientLogger;
import com.azure.storage.blob.BlobServiceVersion;
import com.azure.storage.blob.models.UserDelegationKey;
import com.azure.storage.common.Utility;
import com.azure.storage.common.implementation.Constants;
Expand Down Expand Up @@ -43,9 +45,16 @@ public final class BlobServiceSasSignatureValues {
*/
private static final String SAS_CONTAINER_CONSTANT = "c";

private final ClientLogger logger = new ClientLogger(BlobServiceSasSignatureValues.class);
private static final ClientLogger LOGGER = new ClientLogger(BlobServiceSasSignatureValues.class);

private final String version = BlobSasServiceVersion.getLatest().getVersion();
private static final String VERSION = Configuration.getGlobalConfiguration()
.get(Constants.PROPERTY_AZURE_STORAGE_SAS_SERVICE_VERSION, BlobServiceVersion.getLatest().getVersion());

/**
* Pin down to highest version that worked with string to sign defined here.
*/
private static final String VERSION_DEPRECATED_STRING_TO_SIGN = Configuration.getGlobalConfiguration()
.get(Constants.PROPERTY_AZURE_STORAGE_SAS_SERVICE_VERSION, BlobServiceVersion.V2019_12_12.getVersion());

private SasProtocol protocol;

Expand Down Expand Up @@ -171,7 +180,7 @@ public BlobServiceSasSignatureValues(String version, SasProtocol sasProtocol, Of
* targeted by the library.
*/
public String getVersion() {
return version;
return VERSION;
}

/**
Expand Down Expand Up @@ -543,8 +552,6 @@ public BlobServiceSasSignatureValues setCorrelationId(String correlationId) {
*
* <p><strong>Notes on SAS generation</strong></p>
* <ul>
* <li>If {@link #setVersion(String) version} is not set,
* the {@link BlobSasServiceVersion#getLatest() latest service version} is used.</li>
* <li>If {@link #setIdentifier(String) identifier} is set, {@link #setExpiryTime(OffsetDateTime) expiryTime} and
* permissions should not be set. These values are inherited from the stored access policy.</li>
* <li>Otherwise, {@link #setExpiryTime(OffsetDateTime) expiryTime} and {@link #getPermissions() permissions} must
Expand Down Expand Up @@ -582,7 +589,7 @@ public BlobServiceSasQueryParameters generateSasQueryParameters(
final String canonicalName = getCanonicalName(storageSharedKeyCredentials.getAccountName());
final String signature = storageSharedKeyCredentials.computeHmac256(stringToSign(canonicalName));

return new BlobServiceSasQueryParameters(this.version, this.protocol, this.startTime, this.expiryTime,
return new BlobServiceSasQueryParameters(VERSION, this.protocol, this.startTime, this.expiryTime,
this.sasIpRange, this.identifier, this.resource, this.permissions, signature, this.cacheControl,
this.contentDisposition, this.contentEncoding, this.contentLanguage, this.contentType, null /* delegate */);
}
Expand All @@ -592,8 +599,6 @@ public BlobServiceSasQueryParameters generateSasQueryParameters(
*
* <p><strong>Notes on SAS generation</strong></p>
* <ul>
* <li>If {@link #setVersion(String) version} is not set,
* the {@link BlobSasServiceVersion#getLatest() latest service version} is used.</li>
* <li>If {@link #setIdentifier(String) identifier} is set, {@link #setExpiryTime(OffsetDateTime) expiryTime} and
* permissions should not be set. These values are inherited from the stored access policy.</li>
* <li>Otherwise, {@link #setExpiryTime(OffsetDateTime) expiryTime} and {@link #getPermissions() permissions} must
Expand Down Expand Up @@ -636,7 +641,7 @@ public BlobServiceSasQueryParameters generateSasQueryParameters(UserDelegationKe
String signature = StorageImplUtils.computeHMac256(
delegationKey.getValue(), stringToSign(delegationKey, canonicalName));

return new BlobServiceSasQueryParameters(this.version, this.protocol, this.startTime, this.expiryTime,
return new BlobServiceSasQueryParameters(VERSION, this.protocol, this.startTime, this.expiryTime,
this.sasIpRange, null /* identifier */, this.resource, this.permissions, signature,
this.cacheControl, this.contentDisposition, this.contentEncoding, this.contentLanguage, this.contentType,
delegationKey);
Expand Down Expand Up @@ -676,7 +681,7 @@ private void ensureState() {
break;
default:
// We won't reparse the permissions if we don't know the type.
logger.info("Not re-parsing permissions. Resource type '{}' is unknown.", resource);
LOGGER.info("Not re-parsing permissions. Resource type '{}' is unknown.", resource);
break;
}
}
Expand All @@ -702,8 +707,7 @@ private String stringToSign(String canonicalName) {
this.identifier == null ? "" : this.identifier,
this.sasIpRange == null ? "" : this.sasIpRange.toString(),
this.protocol == null ? "" : this.protocol.toString(),
version, /* Pin down to version so old string to sign works. This is reflected in the declaration of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we hardcode this? This is all called from deprecated code and I feel like this will break as the string to sign changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea. Currently this code doesn't work... that string to sign (or the other one below/above) hasn't been changed as service version evolved

version */
VERSION_DEPRECATED_STRING_TO_SIGN, /* Pin down to version so old string to sign works. */
resource,
this.snapshotId == null ? "" : this.snapshotId,
this.cacheControl == null ? "" : this.cacheControl,
Expand All @@ -728,8 +732,7 @@ private String stringToSign(final UserDelegationKey key, String canonicalName) {
key.getSignedVersion() == null ? "" : key.getSignedVersion(),
this.sasIpRange == null ? "" : this.sasIpRange.toString(),
this.protocol == null ? "" : this.protocol.toString(),
version, /* Pin down to version so old string to sign works. This is reflected in the declaration of
version */
VERSION_DEPRECATED_STRING_TO_SIGN, /* Pin down to version so old string to sign works. */
resource,
this.snapshotId == null ? "" : this.snapshotId,
this.cacheControl == null ? "" : this.cacheControl,
Expand Down
12 changes: 0 additions & 12 deletions ...e/azure-storage-blob/src/test/java/com/azure/storage/blob/BlobServiceSasModelsTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,18 +217,6 @@ class BlobServiceSasModelsTest extends Specification {
ex.getMessage().contains("accountName")
}

def "ensure state version"() {
when:
BlobSasImplUtil implUtil = new BlobSasImplUtil(new BlobServiceSasSignatureValues("id"), "container")
implUtil.version = null
implUtil.ensureState()

then:
implUtil.version // Version is set
implUtil.resource == "c" // Default resource is container
!implUtil.permissions // Identifier was used so permissions is null
}

def "ensure state illegal argument"() {
when:
BlobSasImplUtil implUtil = new BlobSasImplUtil(new BlobServiceSasSignatureValues(), null)
Expand Down
Loading