pitrou commented on code in PR #43195:
URL: https://github.com/apache/arrow/pull/43195#discussion_r1670747933
##########
cpp/src/parquet/encryption/encryption_internal.cc:
##########
@@ -58,20 +58,39 @@ class AesEncryptor::AesEncryptorImpl {
~AesEncryptorImpl() { WipeOut(); }
- int Encrypt(const uint8_t* plaintext, int plaintext_len, const uint8_t* key,
- int key_len, const uint8_t* aad, int aad_len, uint8_t*
ciphertext);
+ int Encrypt(span<const uint8_t> plaintext, span<const uint8_t> key,
+ span<const uint8_t> aad, span<uint8_t> ciphertext);
- int SignedFooterEncrypt(const uint8_t* footer, int footer_len, const
uint8_t* key,
- int key_len, const uint8_t* aad, int aad_len,
- const uint8_t* nonce, uint8_t* encrypted_footer);
+ int SignedFooterEncrypt(span<const uint8_t> footer, span<const uint8_t> key,
+ span<const uint8_t> aad, span<const uint8_t> nonce,
+ span<uint8_t> encrypted_footer);
void WipeOut() {
if (nullptr != ctx_) {
EVP_CIPHER_CTX_free(ctx_);
ctx_ = nullptr;
}
}
- int ciphertext_size_delta() { return ciphertext_size_delta_; }
+ [[nodiscard]] int CiphertextLength(int64_t plaintext_len) const {
+ if (plaintext_len < 0) {
+ std::stringstream ss;
+ ss << "Negative plaintext length " << plaintext_len;
+ throw ParquetException(ss.str());
+ } else if (plaintext_len > std::numeric_limits<int32_t>::max()) {
Review Comment:
Nit: `int` rather than `int32_t`? (or, conversely, perhaps change the method
signature?)
##########
cpp/src/parquet/encryption/encryption_internal.cc:
##########
@@ -137,30 +156,45 @@
AesEncryptor::AesEncryptorImpl::AesEncryptorImpl(ParquetCipher::type alg_id, int
}
}
-int AesEncryptor::AesEncryptorImpl::SignedFooterEncrypt(
- const uint8_t* footer, int footer_len, const uint8_t* key, int key_len,
- const uint8_t* aad, int aad_len, const uint8_t* nonce, uint8_t*
encrypted_footer) {
- if (key_length_ != key_len) {
+int AesEncryptor::AesEncryptorImpl::SignedFooterEncrypt(span<const uint8_t>
footer,
+ span<const uint8_t>
key,
+ span<const uint8_t>
aad,
+ span<const uint8_t>
nonce,
+ span<uint8_t>
encrypted_footer) {
+ if (static_cast<size_t>(key_length_) != key.size()) {
+ std::stringstream ss;
+ ss << "Wrong key length " << key.size() << ". Should be " << key_length_;
+ throw ParquetException(ss.str());
+ }
+
+ if (encrypted_footer.size() < footer.size() + ciphertext_size_delta_) {
Review Comment:
Should this be an equality test? Is there a reason to support a
larger-than-necessary encrypted footer size?
##########
cpp/src/parquet/encryption/encryption_internal.cc:
##########
@@ -170,45 +204,53 @@ int AesEncryptor::AesEncryptorImpl::Encrypt(const
uint8_t* plaintext, int plaint
RAND_bytes(nonce, sizeof(nonce));
if (kGcmMode == aes_mode_) {
- return GcmEncrypt(plaintext, plaintext_len, key, key_len, nonce, aad,
aad_len,
- ciphertext);
+ return GcmEncrypt(plaintext, key, nonce, aad, ciphertext);
}
- return CtrEncrypt(plaintext, plaintext_len, key, key_len, nonce, ciphertext);
+ return CtrEncrypt(plaintext, key, nonce, ciphertext);
}
-int AesEncryptor::AesEncryptorImpl::GcmEncrypt(const uint8_t* plaintext,
- int plaintext_len, const
uint8_t* key,
- int key_len, const uint8_t*
nonce,
- const uint8_t* aad, int aad_len,
- uint8_t* ciphertext) {
+int AesEncryptor::AesEncryptorImpl::GcmEncrypt(span<const uint8_t> plaintext,
+ span<const uint8_t> key,
+ span<const uint8_t> nonce,
+ span<const uint8_t> aad,
+ span<uint8_t> ciphertext) {
int len;
int ciphertext_len;
uint8_t tag[kGcmTagLength];
memset(tag, 0, kGcmTagLength);
+ if (nonce.size() < static_cast<size_t>(kNonceLength)) {
Review Comment:
Should this be an equality test too?
##########
cpp/src/parquet/encryption/encryption_internal.cc:
##########
@@ -137,30 +156,45 @@
AesEncryptor::AesEncryptorImpl::AesEncryptorImpl(ParquetCipher::type alg_id, int
}
}
-int AesEncryptor::AesEncryptorImpl::SignedFooterEncrypt(
- const uint8_t* footer, int footer_len, const uint8_t* key, int key_len,
- const uint8_t* aad, int aad_len, const uint8_t* nonce, uint8_t*
encrypted_footer) {
- if (key_length_ != key_len) {
+int AesEncryptor::AesEncryptorImpl::SignedFooterEncrypt(span<const uint8_t>
footer,
+ span<const uint8_t>
key,
+ span<const uint8_t>
aad,
+ span<const uint8_t>
nonce,
+ span<uint8_t>
encrypted_footer) {
+ if (static_cast<size_t>(key_length_) != key.size()) {
+ std::stringstream ss;
+ ss << "Wrong key length " << key.size() << ". Should be " << key_length_;
+ throw ParquetException(ss.str());
+ }
+
+ if (encrypted_footer.size() < footer.size() + ciphertext_size_delta_) {
std::stringstream ss;
- ss << "Wrong key length " << key_len << ". Should be " << key_length_;
+ ss << "Encrypted footer buffer length " << encrypted_footer.size()
+ << " is insufficient for footer length " << footer.size();
throw ParquetException(ss.str());
}
if (kGcmMode != aes_mode_) {
throw ParquetException("Must use AES GCM (metadata) encryptor");
}
- return GcmEncrypt(footer, footer_len, key, key_len, nonce, aad, aad_len,
- encrypted_footer);
+ return GcmEncrypt(footer, key, nonce, aad, encrypted_footer);
}
-int AesEncryptor::AesEncryptorImpl::Encrypt(const uint8_t* plaintext, int
plaintext_len,
- const uint8_t* key, int key_len,
- const uint8_t* aad, int aad_len,
- uint8_t* ciphertext) {
- if (key_length_ != key_len) {
+int AesEncryptor::AesEncryptorImpl::Encrypt(span<const uint8_t> plaintext,
+ span<const uint8_t> key,
+ span<const uint8_t> aad,
+ span<uint8_t> ciphertext) {
+ if (static_cast<size_t>(key_length_) != key.size()) {
+ std::stringstream ss;
+ ss << "Wrong key length " << key.size() << ". Should be " << key_length_;
+ throw ParquetException(ss.str());
+ }
+
+ if (ciphertext.size() < plaintext.size() + ciphertext_size_delta_) {
Review Comment:
Should `add` be taken into account as well? Or rather, should this check be
delegated to `GcmEncrypt` and `CtrEncrypt`?
##########
cpp/src/parquet/metadata.cc:
##########
@@ -701,12 +702,12 @@ class FileMetaData::FileMetaDataImpl {
uint8_t* serialized_data;
uint32_t serialized_len;
serializer.SerializeToBuffer(metadata_.get(), &serialized_len,
&serialized_data);
+ ::arrow::util::span<const uint8_t> serialized_data_span(serialized_data,
+ serialized_len);
// encrypt the footer key
- std::vector<uint8_t> encrypted_data(encryptor->CiphertextSizeDelta() +
- serialized_len);
- unsigned encrypted_len =
- encryptor->Encrypt(serialized_data, serialized_len,
encrypted_data.data());
+ std::vector<uint8_t>
encrypted_data(encryptor->CiphertextLength(serialized_len));
+ unsigned encrypted_len = encryptor->Encrypt(serialized_data_span,
encrypted_data);
Review Comment:
FTR, do we know why we cast this to `unsigned`?
##########
cpp/src/parquet/encryption/encryption_internal.cc:
##########
@@ -227,45 +269,54 @@ int AesEncryptor::AesEncryptorImpl::GcmEncrypt(const
uint8_t* plaintext,
ciphertext[1] = static_cast<uint8_t>(0xff & (buffer_size >> 8));
ciphertext[0] = static_cast<uint8_t>(0xff & (buffer_size));
}
- std::copy(nonce, nonce + kNonceLength, ciphertext + length_buffer_length_);
+ std::copy(nonce.begin(), nonce.begin() + kNonceLength,
+ ciphertext.begin() + length_buffer_length_);
std::copy(tag, tag + kGcmTagLength,
- ciphertext + length_buffer_length_ + kNonceLength +
ciphertext_len);
+ ciphertext.begin() + length_buffer_length_ + kNonceLength +
ciphertext_len);
return length_buffer_length_ + buffer_size;
}
-int AesEncryptor::AesEncryptorImpl::CtrEncrypt(const uint8_t* plaintext,
- int plaintext_len, const
uint8_t* key,
- int key_len, const uint8_t*
nonce,
- uint8_t* ciphertext) {
+int AesEncryptor::AesEncryptorImpl::CtrEncrypt(span<const uint8_t> plaintext,
+ span<const uint8_t> key,
+ span<const uint8_t> nonce,
+ span<uint8_t> ciphertext) {
int len;
int ciphertext_len;
+ if (nonce.size() < static_cast<size_t>(kNonceLength)) {
Review Comment:
Same question here
--
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]
