You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Tpt (via GitHub)" <gi...@apache.org> on 2026/06/18 14:23:49 UTC
[PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Tpt opened a new pull request, #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766
It removes the need for intermediate traits.
All callers can easily build a slice of slices but the S3 object content SHA 256 computation that is expensive anyway.
Follow up to #707
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt closed pull request #766: digest and hmac: drop intermediate builder and directly take slices for input
URL: https://github.com/apache/arrow-rs-object-store/pull/766
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#issuecomment-4769532376
btw ci issue is from docker failing
> Unable to find image 'amazon/amazon-ec2-metadata-mock:v1.9.2' locally
docker: Error response from daemon: Head "https://registry-1.docker.io/v2/amazon/amazon-ec2-metadata-mock/manifests/v1.9.2": Get "https://auth.docker.io/token?account=githubactions&scope=repository%3Aamazon%2Famazon-ec2-metadata-mock%3Apull&service=registry.docker.io": context deadline exceeded (Client.Timeout exceeded while awaiting headers)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#discussion_r3547146891
##########
src/client/crypto.rs:
##########
@@ -35,42 +35,16 @@ pub enum SigningAlgorithm {
/// Provides cryptographic primitives
pub trait CryptoProvider: std::fmt::Debug + Send + Sync {
- /// Compute a digest
- fn digest(&self, algorithm: DigestAlgorithm) -> Result<Box<dyn DigestContext>>;
+ /// Compute the digest of `data`
+ fn digest(&self, algorithm: DigestAlgorithm, data: &[&[u8]]) -> Result<Vec<u8>>;
- /// Compute an HMAC with the provided `secret`
- fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8]) -> Result<Box<dyn HmacContext>>;
+ /// Compute the HMAC of `data` with the provided `secret`
+ fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8], data: &[u8]) -> Result<Vec<u8>>;
/// Sign a payload with the provided PEM-encoded secret
fn sign(&self, algorithm: SigningAlgorithm, pem: &[u8]) -> Result<Box<dyn Signer>>;
}
-/// Incrementally compute a digest, see [`CryptoProvider::digest`]
-pub trait DigestContext: Send {
- /// Updates the digest with all the data in data.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn update(&mut self, data: &[u8]);
-
- /// Finalizes the digest calculation and returns the digest value.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn finish(&mut self) -> Result<&[u8]>;
Review Comment:
> . I just found quite sad to have a complicated API with a lot of dynamic traits where a smaller API could work. Feel free to close this MR.
Yeah, this is a new API that we added to support pluggable crypto providers.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#discussion_r3452629409
##########
src/client/crypto.rs:
##########
@@ -35,42 +35,16 @@ pub enum SigningAlgorithm {
/// Provides cryptographic primitives
pub trait CryptoProvider: std::fmt::Debug + Send + Sync {
- /// Compute a digest
- fn digest(&self, algorithm: DigestAlgorithm) -> Result<Box<dyn DigestContext>>;
+ /// Compute the digest of `data`
+ fn digest(&self, algorithm: DigestAlgorithm, data: &[&[u8]]) -> Result<Vec<u8>>;
- /// Compute an HMAC with the provided `secret`
- fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8]) -> Result<Box<dyn HmacContext>>;
+ /// Compute the HMAC of `data` with the provided `secret`
+ fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8], data: &[u8]) -> Result<Vec<u8>>;
/// Sign a payload with the provided PEM-encoded secret
fn sign(&self, algorithm: SigningAlgorithm, pem: &[u8]) -> Result<Box<dyn Signer>>;
}
-/// Incrementally compute a digest, see [`CryptoProvider::digest`]
-pub trait DigestContext: Send {
- /// Updates the digest with all the data in data.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn update(&mut self, data: &[u8]);
-
- /// Finalizes the digest calculation and returns the digest value.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn finish(&mut self) -> Result<&[u8]>;
Review Comment:
We could possibly have a smaller API change and just allow the traits to return a Vec<u8> directly 🤔 that would also avoid the need for a copy in some cases and might be a smaller API change?
```rust
fn build(self) -> Result<Vec<u8>> {
...
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#discussion_r3453412373
##########
src/client/crypto.rs:
##########
@@ -35,42 +35,16 @@ pub enum SigningAlgorithm {
/// Provides cryptographic primitives
pub trait CryptoProvider: std::fmt::Debug + Send + Sync {
- /// Compute a digest
- fn digest(&self, algorithm: DigestAlgorithm) -> Result<Box<dyn DigestContext>>;
+ /// Compute the digest of `data`
+ fn digest(&self, algorithm: DigestAlgorithm, data: &[&[u8]]) -> Result<Vec<u8>>;
- /// Compute an HMAC with the provided `secret`
- fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8]) -> Result<Box<dyn HmacContext>>;
+ /// Compute the HMAC of `data` with the provided `secret`
+ fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8], data: &[u8]) -> Result<Vec<u8>>;
/// Sign a payload with the provided PEM-encoded secret
fn sign(&self, algorithm: SigningAlgorithm, pem: &[u8]) -> Result<Box<dyn Signer>>;
}
-/// Incrementally compute a digest, see [`CryptoProvider::digest`]
-pub trait DigestContext: Send {
- /// Updates the digest with all the data in data.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn update(&mut self, data: &[u8]);
-
- /// Finalizes the digest calculation and returns the digest value.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn finish(&mut self) -> Result<&[u8]>;
Review Comment:
Thank you for the review!
All digests are currently stored in fixed length arrays so it would not save any allocation in our case, just a possible memcopy at the cost of maybe a useless allocations.
On the breaking changes, I hoped that this changed would be reviewed before the release, seems it's not the case sadly. Feel free to close this MR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#discussion_r3453412373
##########
src/client/crypto.rs:
##########
@@ -35,42 +35,16 @@ pub enum SigningAlgorithm {
/// Provides cryptographic primitives
pub trait CryptoProvider: std::fmt::Debug + Send + Sync {
- /// Compute a digest
- fn digest(&self, algorithm: DigestAlgorithm) -> Result<Box<dyn DigestContext>>;
+ /// Compute the digest of `data`
+ fn digest(&self, algorithm: DigestAlgorithm, data: &[&[u8]]) -> Result<Vec<u8>>;
- /// Compute an HMAC with the provided `secret`
- fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8]) -> Result<Box<dyn HmacContext>>;
+ /// Compute the HMAC of `data` with the provided `secret`
+ fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8], data: &[u8]) -> Result<Vec<u8>>;
/// Sign a payload with the provided PEM-encoded secret
fn sign(&self, algorithm: SigningAlgorithm, pem: &[u8]) -> Result<Box<dyn Signer>>;
}
-/// Incrementally compute a digest, see [`CryptoProvider::digest`]
-pub trait DigestContext: Send {
- /// Updates the digest with all the data in data.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn update(&mut self, data: &[u8]);
-
- /// Finalizes the digest calculation and returns the digest value.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn finish(&mut self) -> Result<&[u8]>;
Review Comment:
Thank you for the review!
All digests are currently stored in fixed length arrays so it would not save any allocation in our case, just a possible memcopy at the cost of maybe a useless allocations.
On the breaking changes, I hoped that this changed would be reviewed before the release, seems it's not the case sadly. I just found quite sad to have a complicated API with a lot of dynamic traits where a smaller API could work. Feel free to close this MR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#issuecomment-5396830569
@tustvold Make sense! Closing
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#discussion_r3437879075
##########
src/client/crypto.rs:
##########
@@ -150,26 +124,24 @@ pub(crate) mod ring {
}
impl CryptoProvider for RingCryptoProvider {
- fn digest(&self, algorithm: DigestAlgorithm) -> Result<Box<dyn DigestContext>> {
+ fn digest(&self, algorithm: DigestAlgorithm, data: &[&[u8]]) -> Result<Vec<u8>> {
let algorithm = match algorithm {
DigestAlgorithm::Sha256 => &digest::SHA256,
};
- let ctx = digest::Context::new(algorithm);
- Ok(Box::new(RingDigestContext {
- ctx: Some(ctx),
- out: None,
- }))
+ let mut ctx = digest::Context::new(algorithm);
+ for chunk in data {
+ ctx.update(chunk);
+ }
+ Ok(ctx.finish().as_ref().to_vec())
}
- fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8]) -> Result<Box<dyn HmacContext>> {
+ fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8], data: &[u8]) -> Result<Vec<u8>> {
let algorithm = match algorithm {
DigestAlgorithm::Sha256 => hmac::HMAC_SHA256,
};
let ctx = hmac::Context::with_key(&hmac::Key::new(algorithm, secret));
Review Comment:
```suggestion
let mut ctx = hmac::Context::with_key(&hmac::Key::new(algorithm, secret));
```
add mut, align with the other change
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#discussion_r3453412373
##########
src/client/crypto.rs:
##########
@@ -35,42 +35,16 @@ pub enum SigningAlgorithm {
/// Provides cryptographic primitives
pub trait CryptoProvider: std::fmt::Debug + Send + Sync {
- /// Compute a digest
- fn digest(&self, algorithm: DigestAlgorithm) -> Result<Box<dyn DigestContext>>;
+ /// Compute the digest of `data`
+ fn digest(&self, algorithm: DigestAlgorithm, data: &[&[u8]]) -> Result<Vec<u8>>;
- /// Compute an HMAC with the provided `secret`
- fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8]) -> Result<Box<dyn HmacContext>>;
+ /// Compute the HMAC of `data` with the provided `secret`
+ fn hmac(&self, algorithm: DigestAlgorithm, secret: &[u8], data: &[u8]) -> Result<Vec<u8>>;
/// Sign a payload with the provided PEM-encoded secret
fn sign(&self, algorithm: SigningAlgorithm, pem: &[u8]) -> Result<Box<dyn Signer>>;
}
-/// Incrementally compute a digest, see [`CryptoProvider::digest`]
-pub trait DigestContext: Send {
- /// Updates the digest with all the data in data.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn update(&mut self, data: &[u8]);
-
- /// Finalizes the digest calculation and returns the digest value.
- ///
- /// It is implementation-defined behaviour to call this after calling [`Self::finish`]
- fn finish(&mut self) -> Result<&[u8]>;
Review Comment:
All digests are currently stored in fixed length arrays so it would not save any allocation in our case, just a possible memcopy at the cost of maybe a useless allocations.
On the breaking change, I hoped that this changed would be reviewed before the release, seems it's not the case sadly. Feel free to close this MR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] digest and hmac: drop intermediate builder and directly take slices for input [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #766:
URL: https://github.com/apache/arrow-rs-object-store/pull/766#issuecomment-4919057902
Merged up from main to get CI fixes
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org