Copilot commented on code in PR #4751:
URL: https://github.com/apache/polaris/pull/4751#discussion_r3408915507
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java:
##########
@@ -244,7 +241,7 @@ static StorageAccessConfig
compute(AzureStorageCredentialCacheKey key) {
getAdlsUserDelegationSas(
startTime,
sanitizedEndTime,
- expiry,
+ sanitizedEndTime,
Review Comment:
This change potentially extends the lifetime of the user delegation key to
match `sanitizedEndTime`, whereas previously it was capped to ~1 hour. If
`sanitizedEndTime` can be significantly longer, the resulting delegation
key/SAS may be valid for longer than intended, increasing the blast radius if
leaked. Consider explicitly capping the delegation key expiry to the minimum
necessary (or making the cap configurable) and documenting the intended maximum
validity period.
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/azure/AzureCredentialsStorageIntegration.java:
##########
@@ -224,7 +221,7 @@ static StorageAccessConfig
compute(AzureStorageCredentialCacheKey key) {
getBlobUserDelegationSas(
startTime,
sanitizedEndTime,
- expiry,
+ sanitizedEndTime,
Review Comment:
Passing `sanitizedEndTime` twice to `getBlobUserDelegationSas` makes it hard
to tell whether the 3rd argument is intended to be a distinct ‘delegation key
expiry’ versus the SAS end time. Consider introducing a clearly named local
variable (e.g., `delegationKeyExpiry`) or adding an inline comment indicating
that the delegation key expiry is intentionally set to the same value as the
SAS end time (and why). This will reduce ambiguity for future changes/refactors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
