kevinjqliu commented on code in PR #742:
URL:
https://github.com/apache/arrow-rs-object-store/pull/742#discussion_r3382706299
##########
src/azure/client.rs:
##########
@@ -190,6 +197,57 @@ impl AzureConfig {
}
}
+#[derive(Default, Clone, Debug)]
+pub(crate) struct AzureEncryptionHeaders {
+ pub encryption_key: Option<String>,
+ pub encryption_key_sha256: Option<String>,
+}
+
+impl AzureEncryptionHeaders {
+ pub(crate) fn try_new(encryption_key: Option<String>) -> Result<Self> {
+ let Some(encryption_key) = encryption_key else {
+ return Ok(Self::default());
+ };
+
+ let decoded_key = BASE64_STANDARD
+ .decode(encryption_key.as_bytes())
+ .map_err(|source| crate::Error::Generic {
+ store: STORE,
+ source: Box::new(source),
+ })?;
+
+ if decoded_key.len() != 32 {
+ return Err(crate::Error::Generic {
+ store: STORE,
+ source: format!(
+ "Azure customer-provided encryption key must decode to 32
bytes, got {}",
+ decoded_key.len()
+ )
+ .into(),
+ });
+ }
+
+ let encryption_key_sha256 =
+ BASE64_STANDARD.encode(digest::digest(&digest::SHA256,
&decoded_key));
+
+ Ok(Self {
+ encryption_key: Some(encryption_key),
+ encryption_key_sha256: Some(encryption_key_sha256),
+ })
+ }
+
+ fn apply(&self, builder: HttpRequestBuilder) -> HttpRequestBuilder {
+ match (&self.encryption_key, &self.encryption_key_sha256) {
+ (Some(encryption_key), Some(encryption_key_sha256)) => builder
+ .header(&ENCRYPTION_KEY_HEADER, encryption_key)
+ .header(&ENCRYPTION_KEY_SHA256_HEADER, encryption_key_sha256)
+ // Azure only supports this algorithm header
+ .header(&ENCRYPTION_ALGORITHM_HEADER, "AES256"),
Review Comment:
nit: move this string up to the class
##########
src/azure/client.rs:
##########
@@ -190,6 +197,57 @@ impl AzureConfig {
}
}
+#[derive(Default, Clone, Debug)]
+pub(crate) struct AzureEncryptionHeaders {
+ pub encryption_key: Option<String>,
+ pub encryption_key_sha256: Option<String>,
+}
+
+impl AzureEncryptionHeaders {
+ pub(crate) fn try_new(encryption_key: Option<String>) -> Result<Self> {
+ let Some(encryption_key) = encryption_key else {
+ return Ok(Self::default());
+ };
+
+ let decoded_key = BASE64_STANDARD
+ .decode(encryption_key.as_bytes())
+ .map_err(|source| crate::Error::Generic {
+ store: STORE,
+ source: Box::new(source),
+ })?;
+
+ if decoded_key.len() != 32 {
Review Comment:
nit: why 32 here?
is it because
> An AES-256 key is 256 bits = 32 bytes raw.
##########
src/azure/builder.rs:
##########
@@ -85,6 +85,11 @@ enum Error {
#[error("Missing component in SAS query pair")]
MissingSasComponent {},
+ #[error("Invalid encryption key: {source}")]
Review Comment:
curious, why this is a breaking change?
##########
src/azure/client.rs:
##########
@@ -264,8 +322,8 @@ impl PutRequest<'_> {
.as_deref()
.map(|c| c.sensitive_request())
.unwrap_or_default();
- let response = self
- .builder
+ let builder = self.config.encryption_headers.apply(self.builder);
+ let response = builder
.header(CONTENT_LENGTH, self.payload.content_length())
.with_azure_authorization(&credential, &self.config.account)
Review Comment:
nit: this call feels awkward, could we move this into the request-builder
extension layer, similar to `with_azure_authorization`?
Right now `AzureEncryptionHeaders::apply(builder)` feels a little detached
from the rest of the request construction, and it also makes it easy to apply
the CPK headers without remembering that the request now contains secret
material. A fluent helper like
`with_azure_encryption_headers(&self.config.encryption_headers)` would match
the existing style better.
Ideally that helper would also mark the request as sensitive when
`x-ms-encryption-key` is present, so callers do not need to separately remember
to combine credential sensitivity with CPK sensitivity.
##########
src/azure/client.rs:
##########
@@ -190,6 +197,57 @@ impl AzureConfig {
}
}
+#[derive(Default, Clone, Debug)]
Review Comment:
nit: Can we avoid deriving Debug here? This struct contains the raw
customer-provided encryption key, so debug output could leak key material. I’d
prefer a custom Debug impl that only reports whether a key is configured, and
maybe the SHA-256 value if we consider that safe enough, but not the raw key.
##########
src/azure/builder.rs:
##########
@@ -1235,6 +1270,17 @@ mod tests {
}
}
+ #[test]
+ fn azure_encryption_key_roundtrip() {
+ let key = BASE64_STANDARD.encode([7_u8; 32]);
+ let builder = MicrosoftAzureBuilder::new().with_encryption_key(&key);
+
+ assert_eq!(
+
builder.get_config_value(&AzureConfigKey::EncryptionKey).as_deref(),
+ Some(key.as_str())
+ );
Review Comment:
could we also add tests for invalid values
- malformed base64
- base64 that decodes to fewer than 32 bytes
- base64 that decodes to more than 32 bytes
The happy-path roundtrip test is useful, but the main correctness behavior
here is that invalid keys fail at build time rather than producing bad Azure
requests.
--
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]
