You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "JakeDern (via GitHub)" <gi...@apache.org> on 2025/08/11 16:24:20 UTC
[PR] DRAFT: Pluggable crypto [arrow-rs-object-store]
JakeDern opened a new pull request, #462:
URL: https://github.com/apache/arrow-rs-object-store/pull/462
# Which issue does this PR close?
Closes #413.
# Rationale for this change
Many companies, including my current employer, are picky about what cryptographic implementations may be used. This PR seeks to accomplish two things:
1. Make it possible to have no default crypto provider, such as ring, in the dependency tree
2. Make it possible to bring your own crypto provider
# What changes are included in this PR?
This is heavily WIP - I'm just opening this to make it clear how the approach with generic types would work vs an implementation that would use `dyn` instead at the expense of having to allocate and copy the outputs of various crypto implementations.
- Add a `CryptoProvider` trait
- Add an optional `RingProvider` implementation based on the `ring` feature
# Are there any user-facing changes?
Yes, several including:
- Making builders for clouds that need crypto generic
- Making `default::Default` implementations dependent on the `ring` feature
- Making `new()` require a crypto provider
<!---
If there are any breaking changes to public APIs, please call them out.
-->
--
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] DRAFT: Pluggable crypto [arrow-rs-object-store]
Posted by "JakeDern (via GitHub)" <gi...@apache.org>.
JakeDern commented on code in PR #462:
URL: https://github.com/apache/arrow-rs-object-store/pull/462#discussion_r2268183624
##########
src/crypto.rs:
##########
@@ -0,0 +1,97 @@
+use std::fmt::{Debug, Display};
+
+type DynError = Box<dyn std::error::Error + Send + Sync>;
+
+#[derive(Debug)]
+pub struct Error {
+ inner: DynError,
+}
+
+impl Display for Error {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "Crypto error: {}", self.inner)
+ }
+}
+
+impl std::error::Error for Error {}
+
+impl From<DynError> for Error {
+ fn from(err: DynError) -> Self {
+ Error { inner: err }
+ }
+}
+
+/// TODO(jakedern): Docs
+pub trait CryptoProvider: Send + Sync + Debug + 'static {
+ fn digest_sha256(bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error>;
+ fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error>;
+ fn hex_digest(bytes: &[u8]) -> Result<String, Error> {
+ let digest = Self::digest_sha256(bytes)?;
+ Ok(hex_encode(digest.as_ref()))
+ }
+}
+
+pub(crate) fn hex_encode(bytes: &[u8]) -> String {
+ use std::fmt::Write;
+ let mut out = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ let _ = write!(out, "{byte:02x}");
+ }
+ out
+}
+
+/// TODO(jakedern): Docs
+#[cfg(feature = "ring")]
+pub mod ring_crypto {
+ use super::{CryptoProvider, Error};
+
+ #[derive(Debug, Clone, Copy)]
+ pub struct RingProvider;
+
+ impl CryptoProvider for RingProvider {
+ fn digest_sha256(bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error> {
+ let digest = ring::digest::digest(&ring::digest::SHA256, bytes);
+ Ok(digest)
+ }
+
+ fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error> {
+ let key = ring::hmac::Key::new(ring::hmac::HMAC_SHA256, secret);
+ let tag = ring::hmac::sign(&key, bytes);
+ Ok(tag)
+ }
+ }
+
+ impl Default for RingProvider {
+ fn default() -> Self {
+ RingProvider
+ }
+ }
+}
+
+pub mod openssl_crypto {
Review Comment:
Yep, will take this out - Was just trying out a couple of providers to see how they compare and if the trait was flexible enough
--
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] DRAFT: Pluggable crypto [arrow-rs-object-store]
Posted by "crepererum (via GitHub)" <gi...@apache.org>.
crepererum commented on PR #462:
URL: https://github.com/apache/arrow-rs-object-store/pull/462#issuecomment-3876541662
@JakeDern is it OK if we close this in favor of #585?
--
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] DRAFT: Pluggable crypto [arrow-rs-object-store]
Posted by "JakeDern (via GitHub)" <gi...@apache.org>.
JakeDern commented on PR #462:
URL: https://github.com/apache/arrow-rs-object-store/pull/462#issuecomment-3186411496
@tustvold I have a pretty complete implementation here. Other than more docs and bits of cleanup along those lines, the last major bit I haven't accounted for is the [Rsa key stuff](https://github.com/apache/arrow-rs-object-store/blob/94c25d2dea15d2a7154bb166ae58cbf9452ebcd9/src/gcp/credential.rs#L125C1-L126C1) for GCP credentials.
This is (today) specific to GCP and appears at first glance to be quite a bit more complicated to put into an object safe trait due to the requirement to create an intermediate `RsaKeyPair` equivalent type that you can then re-use.
I'm assuming we'd want to also encapsulate that into the trait, but I just want to raise it for discussion. Maybe there's a straightforward way to do this that I don't know, I only have a few months experience with rust. Usually I'd have something in the interface that returns `impl MyRsaKeyPairTrait` that I could hold the library specific state in, but of course that won't be object safe anymore.
Let me know what your thoughts are and thanks again for working with me on this! I know it's a lot of mental overhead, so I appreciate it!
--
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] DRAFT: Pluggable crypto [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold closed pull request #462: DRAFT: Pluggable crypto
URL: https://github.com/apache/arrow-rs-object-store/pull/462
--
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] DRAFT: Pluggable crypto [arrow-rs-object-store]
Posted by "JakeDern (via GitHub)" <gi...@apache.org>.
JakeDern commented on PR #462:
URL: https://github.com/apache/arrow-rs-object-store/pull/462#issuecomment-3220917524
@tustvold friendly ping on [the above](https://github.com/apache/arrow-rs-object-store/pull/462#issuecomment-3186411496) - Please let me know if you have thoughts about whether the GCP certificate credential stuff should be included in this trait and if you have any ideas on a reasonable object safe way to do so. Thanks again for your time!
--
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] DRAFT: Pluggable crypto [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #462:
URL: https://github.com/apache/arrow-rs-object-store/pull/462#issuecomment-3677822421
Sorry for the delay on this, I plan to pick this up this weekend
--
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] DRAFT: Pluggable crypto [arrow-rs-object-store]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #462:
URL: https://github.com/apache/arrow-rs-object-store/pull/462#discussion_r2267618476
##########
src/crypto.rs:
##########
@@ -0,0 +1,97 @@
+use std::fmt::{Debug, Display};
+
+type DynError = Box<dyn std::error::Error + Send + Sync>;
+
+#[derive(Debug)]
+pub struct Error {
Review Comment:
I think it'd be better to just use object_store::Error, whilst it contains variants that aren't relevant, I don't think this matters
##########
src/crypto.rs:
##########
@@ -0,0 +1,97 @@
+use std::fmt::{Debug, Display};
+
+type DynError = Box<dyn std::error::Error + Send + Sync>;
+
+#[derive(Debug)]
+pub struct Error {
+ inner: DynError,
+}
+
+impl Display for Error {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "Crypto error: {}", self.inner)
+ }
+}
+
+impl std::error::Error for Error {}
+
+impl From<DynError> for Error {
+ fn from(err: DynError) -> Self {
+ Error { inner: err }
+ }
+}
+
+/// TODO(jakedern): Docs
+pub trait CryptoProvider: Send + Sync + Debug + 'static {
+ fn digest_sha256(bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error>;
+ fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error>;
+ fn hex_digest(bytes: &[u8]) -> Result<String, Error> {
Review Comment:
Probably don't need this to be here unless you expect people to override it
##########
src/crypto.rs:
##########
@@ -0,0 +1,97 @@
+use std::fmt::{Debug, Display};
+
+type DynError = Box<dyn std::error::Error + Send + Sync>;
+
+#[derive(Debug)]
+pub struct Error {
+ inner: DynError,
+}
+
+impl Display for Error {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "Crypto error: {}", self.inner)
+ }
+}
+
+impl std::error::Error for Error {}
+
+impl From<DynError> for Error {
+ fn from(err: DynError) -> Self {
+ Error { inner: err }
+ }
+}
+
+/// TODO(jakedern): Docs
+pub trait CryptoProvider: Send + Sync + Debug + 'static {
+ fn digest_sha256(bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error>;
+ fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error>;
+ fn hex_digest(bytes: &[u8]) -> Result<String, Error> {
+ let digest = Self::digest_sha256(bytes)?;
+ Ok(hex_encode(digest.as_ref()))
+ }
+}
+
+pub(crate) fn hex_encode(bytes: &[u8]) -> String {
+ use std::fmt::Write;
+ let mut out = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ let _ = write!(out, "{byte:02x}");
+ }
+ out
+}
+
+/// TODO(jakedern): Docs
+#[cfg(feature = "ring")]
+pub mod ring_crypto {
+ use super::{CryptoProvider, Error};
+
+ #[derive(Debug, Clone, Copy)]
+ pub struct RingProvider;
+
+ impl CryptoProvider for RingProvider {
+ fn digest_sha256(bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error> {
+ let digest = ring::digest::digest(&ring::digest::SHA256, bytes);
+ Ok(digest)
+ }
+
+ fn hmac_sha256(secret: &[u8], bytes: &[u8]) -> Result<impl AsRef<[u8]>, Error> {
+ let key = ring::hmac::Key::new(ring::hmac::HMAC_SHA256, secret);
+ let tag = ring::hmac::sign(&key, bytes);
+ Ok(tag)
+ }
+ }
+
+ impl Default for RingProvider {
+ fn default() -> Self {
+ RingProvider
+ }
+ }
+}
+
+pub mod openssl_crypto {
Review Comment:
I would personally prefer people to add additional providers out of tree if they need them. I'm not massively excited about adding a dependency on openssl, optional or not.
--
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] DRAFT: Pluggable crypto [arrow-rs-object-store]
Posted by "JakeDern (via GitHub)" <gi...@apache.org>.
JakeDern commented on PR #462:
URL: https://github.com/apache/arrow-rs-object-store/pull/462#issuecomment-3677972097
@tustvold No problem! I haven't had the time to follow through in the last couple months either 😅. If you have ideas on how to deal with the GCP credentials in particular, I'm happy to dust off this work and get it over the finish line
--
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