CommanderStorm commented on code in PR #455:
URL:
https://github.com/apache/arrow-rs-object-store/pull/455#discussion_r2249466039
##########
examples/crypto_providers.rs:
##########
@@ -0,0 +1,39 @@
+//! Example demonstrating the use of different cryptographic providers
Review Comment:
the example does not show this currently
##########
Cargo.toml:
##########
@@ -55,6 +55,7 @@ quick-xml = { version = "0.38.0", features = ["serialize",
"overlapped-lists"],
rand = { version = "0.9", default-features = false, features = ["std",
"std_rng", "thread_rng"], optional = true }
reqwest = { version = "0.12", default-features = false, features =
["rustls-tls-native-roots", "http2"], optional = true }
ring = { version = "0.17", default-features = false, features = ["std"],
optional = true }
Review Comment:
please order alphabetically
##########
Cargo.toml:
##########
@@ -69,8 +70,12 @@ web-time = { version = "1.1.0" }
wasm-bindgen-futures = "0.4.18"
[features]
-default = ["fs"]
-cloud = ["serde", "serde_json", "quick-xml", "hyper", "reqwest",
"reqwest/stream", "chrono/serde", "base64", "rand", "ring", "http-body-util",
"form_urlencoded", "serde_urlencoded"]
+default = ["fs", "crypto-ring"]
+# Crypto provider selection - choose one
+crypto-ring = ["ring"]
+crypto-aws-lc-rs = ["aws-lc-rs"]
+# Cloud features (requires a crypto provider to be selected separately)
+cloud = ["serde", "serde_json", "quick-xml", "hyper", "reqwest",
"reqwest/stream", "chrono/serde", "base64", "rand", "http-body-util",
"form_urlencoded", "serde_urlencoded"]
Review Comment:
This is a breaking change if someone has `no_default_features=true,
features=["aws"]`. Not very nice, but if mutual exclusion is wanted I don't see
a way around this.
##########
test_crypto_providers.sh:
##########
@@ -0,0 +1,77 @@
+#!/bin/bash
Review Comment:
I don't think this file should exist
##########
src/util.rs:
##########
@@ -488,4 +514,102 @@ mod tests {
let range = GetRange::Offset(1);
assert_eq!(range.as_range(2).unwrap(), 1..2);
}
+
+ #[cfg(all(any(feature = "aws", feature = "gcp"), any(feature =
"crypto-ring", feature = "crypto-aws-lc-rs")))]
+ #[test]
+ fn test_hex_digest() {
+ let test_data = b"hello world";
+ let expected =
"b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9";
+
+ let result = hex_digest(test_data);
+ assert_eq!(result, expected);
+ }
+
+ #[cfg(all(any(feature = "aws", feature = "azure"), any(feature =
"crypto-ring", feature = "crypto-aws-lc-rs")))]
+ #[test]
+ fn test_hmac_sha256() {
+ let secret = b"secret_key";
+ let message = b"hello world";
+
+ let tag = hmac_sha256(secret, message);
+ // Convert tag to hex for comparison
+ let tag_bytes = tag.as_ref();
+ let hex_result = hex_encode(tag_bytes);
+
+ // Expected HMAC-SHA256 for "hello world" with key "secret_key"
+ // This is the correct value calculated using the actual implementation
+ let expected =
"cf1a418afaafc798df48fd804a2abf6970283afd8c40b41f818ad9b6ca4f8ca8";
+ assert_eq!(hex_result, expected);
+ }
+
+ #[test]
+ fn test_hex_encode() {
+ let input = b"hello";
+ let expected = "68656c6c6f";
+ let result = hex_encode(input);
+ assert_eq!(result, expected);
+ }
+
+ #[cfg(all(any(feature = "aws", feature = "gcp"), any(feature =
"crypto-ring", feature = "crypto-aws-lc-rs")))]
+ #[test]
+ fn test_crypto_provider_consistency() {
+ // Test that both crypto providers produce identical results for
various inputs
+ let all_bytes = (0..=255).collect::<Vec<u8>>();
+ let large_input = [0u8; 1000];
+ let test_cases = vec![
+ b"".as_slice(),
+ b"a",
+ b"hello world",
+ b"The quick brown fox jumps over the lazy dog",
+ &large_input, // Large input
+ &all_bytes, // All byte values
+ ];
+
+ for input in test_cases {
+ let digest = hex_digest(input);
+ // Verify it's a valid SHA256 hex string (64 characters, all hex)
+ assert_eq!(digest.len(), 64);
+ assert!(digest.chars().all(|c| c.is_ascii_hexdigit()));
+ }
+ }
+
+ #[cfg(all(any(feature = "aws", feature = "azure"), any(feature =
"crypto-ring", feature = "crypto-aws-lc-rs")))]
+ #[test]
+ fn test_hmac_consistency() {
+ // Test HMAC with various key and message combinations
+ let test_cases = vec![
+ (b"".as_slice(), b"".as_slice()),
+ (b"key", b"message"),
+ (b"very long key that is longer than the block size", b"short
message"),
+ (b"short key", b"very long message that is much longer than
typical messages and should test the streaming capabilities"),
+ ];
+
+ for (key, message) in test_cases {
+ let tag = hmac_sha256(key, message);
+ let hex_result = hex_encode(tag.as_ref());
+ // Verify it's a valid SHA256 hex string (64 characters, all hex)
+ assert_eq!(hex_result.len(), 64);
+ assert!(hex_result.chars().all(|c| c.is_ascii_hexdigit()));
+ }
+ }
+
+ #[test]
+ fn test_compile_time_crypto_exclusivity() {
+ // This test ensures that the compile-time checks are in place
+ // The actual exclusivity is tested by the build system, but we can
+ // verify that exactly one crypto provider is enabled
+
+ #[cfg(feature = "crypto-ring")]
+ let ring_enabled = true;
+ #[cfg(not(feature = "crypto-ring"))]
+ let ring_enabled = false;
+
+ #[cfg(feature = "crypto-aws-lc-rs")]
+ let aws_lc_rs_enabled = true;
+ #[cfg(not(feature = "crypto-aws-lc-rs"))]
+ let aws_lc_rs_enabled = false;
+
+ // At most one should be enabled (could be neither for non-cloud
builds)
+ assert!(!(ring_enabled && aws_lc_rs_enabled), "Both crypto providers
are enabled - this should not be possible");
+ }
Review Comment:
I think this a fairly useless test
```suggestion
```
##########
src/util.rs:
##########
@@ -488,4 +514,102 @@ mod tests {
let range = GetRange::Offset(1);
assert_eq!(range.as_range(2).unwrap(), 1..2);
}
+
+ #[cfg(all(any(feature = "aws", feature = "gcp"), any(feature =
"crypto-ring", feature = "crypto-aws-lc-rs")))]
Review Comment:
I think the testcases below need a scrubbing pass.
##########
README.md:
##########
@@ -80,6 +80,44 @@ There following community maintained crates provide
additional functionality for
[anda_object_store-readme]:
https://github.com/ldclabs/anda-db/blob/main/rs/anda_object_store
[ICP]: https://www.internetcomputer.org/
+## Cryptographic Provider Selection
+
+This crate supports two cryptographic providers for cloud storage operations:
+
+- **ring** (default): The original cryptographic library
+- **aws-lc-rs**: AWS's FIPS-compliant cryptographic library
+
+### Usage
+
+By default, the crate uses `ring` as the cryptographic provider:
+
+```toml
+[dependencies]
+object_store = { version = "0.12", features = ["cloud"] }
+```
+
+To use `aws-lc-rs` instead, disable the default features and enable the
`crypto-aws-lc-rs` feature:
+
+```toml
+[dependencies]
+object_store = { version = "0.12", default-features = false, features =
["cloud-aws-lc-rs"] }
+```
+
+Or manually select the crypto provider:
+
+```toml
+[dependencies]
+object_store = { version = "0.12", default-features = false, features =
["cloud", "crypto-aws-lc-rs"] }
+```
+
+**Important**: You cannot enable both `crypto-ring` and `crypto-aws-lc-rs`
features simultaneously. The crate will fail to compile if both are enabled.
+
+### Feature Flags
+
+- `crypto-ring`: Enable ring cryptographic provider
+- `crypto-aws-lc-rs`: Enable aws-lc-rs cryptographic provider
+- `cloud`: Base cloud storage features (requires a crypto provider)
Review Comment:
I think this can be made much more concice.
A simple bullet list of providers, which feature they are under and which is
the default should be sufficent.
--
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]
