[PR] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi opened a new pull request, #46017:
URL: https://github.com/apache/arrow/pull/46017

   ### Rationale for this change
   Arrow deals with secrets like encryption / decryption keys which must be kept private. One way of leaking such secrets is through memory allocation where another process allocates memory that previously hold the secret, because that memory was not cleared before being freed.
   
   ### What changes are included in this PR?
   Uses various implementations of securely clearing memory, notably
   - `SecureZeroMemory`(Windows)
   - `memset_s`(STDC)
   - `OPENSSL_cleanse` (OpenSSL >= 3)
   - `explicit_bzero`(glibc 2.25+)
   - `memset_v` (fallback).
   
   ### Are these changes tested?
   Unit tests.
   
   ### Are there any user-facing changes?
   APIs that hand over secrets to Arrow require the secret to be encapsulated in a `SecureString`.
   
   **This PR includes breaking changes to public APIs.**
   
   TODO:
   - collect list of affected APIs
   - provide instructions for migration
   - deprecate string-based methods
   - make `SecureString` available to Python API
   - make sure all paths are tested (test all compile directives)


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2031430056


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {

Review Comment:
   Would you please move this to `arrow/util/`? I think it will be useful outside parquet



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2029208776


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   Oh, alternatively: since every STL I'm aware of implements std::string with an inline short string case, we could use:
   
   ```c++
     constexpr auto kCrib = "01CRIB01";
     SecureString cribbed(std::string(kCrib));
     cribbed.~SecureString();
   
     char raw_bytes[sizeof(cribbed)];
     memcpy(&raw_bytes, &cribbed, sizeof(cribbed));
     bool crib_was_wiped = std::string_view{raw_bytes, sizeof(raw_bytes)}.find(kCrib) == std::string_view::npos;
   
     new (&cribbed) SecureString();
     ASSERT_TRUE(crib_was_wiped);
   ```



-- 
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] GH-31603: [C++][Python] Wrap Parquet encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2128911125


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   I'd prefer the `memset_v` as it is more performant. Either memset or loop will require verbose comment to explain the meaning of the volatile, so I think both are similar "obvious" to readers.
   
   I don't understand your comment on the inline asm block. The asm block is a NOOP, it does not zero any memory. It is merely there to tell the compiler that it cannot optimize away any code that modified `data` before this line. So it is not either asm or loop, it is either (memset or loop) and asm.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2775137513

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2775190600

   @EnricoMi Do you want a preliminary review on this?


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2029208776


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   Oh, alternatively: since every STL I'm aware of implements std::string with an inline short string case, we could use:
   
   ```c++
     // Most std::string impls store their contents inline, so we can check
     // the object representation for a crib to ensure that destruction does
     // wipe all sensitive bytes.
   
     constexpr auto kCrib = "01CRIB01";
     alignas(SecureString) char object_repr[sizeof(SecureString)];
     std::string_view object_repr_view{object_repr, sizeof(object_repr)};
   
     SecureString* cribbed = new (&object_repr) SecureString(std::string(kCrib));
     bool crib_found_before = object_repr_view.find(kCrib) != object_repr_view.npos;
     cribbed->~SecureString(); // NOTE: do not allow this call to be skipped!
     bool crib_found_after = object_repr_view.find(kCrib) != object_repr_view.npos;
   
     if (crib_found_before) {
       ASSERT_FALSE(crib_found_after);
     }
   ```
   
   I think that's well-defined: https://godbolt.org/z/31cY59EM3



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3074075680

   @github-actions crossbow submit -g cpp


-- 
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] GH-31603: [C++][Python] Wrap Parquet encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2112333775


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   I think the while loop is preferable; it's no more lines of code and is probably more obvious to readers.
   
   Currently the `memset_v` block doesn't exclude the inline asm block https://github.com/apache/arrow/blob/219d207f97152a5017cc61c0f925dc4387adc6f7/cpp/src/arrow/util/secure_string.cc#L123-L127
   
   I'd recommend rewriting to
   
   ```c++
   #if (inline_asm_conditions)
   // inline asm
   #else
   // while loop
   #endif
   ```
   
   the compiler could ordinarily be relied on to omit the redundant write, but since we're handling `volatile` memory we'll probably generate code which zeroes twice



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2078205211


##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -130,11 +123,11 @@ FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::aad_prefix
 }
 
 ColumnDecryptionProperties::Builder* ColumnDecryptionProperties::Builder::key(
-    const std::string& key) {
+    encryption::SecureString key) {
   if (key.empty()) return this;
 
-  DCHECK(!key.empty());
-  key_ = key;
+  DCHECK(key_.empty());

Review Comment:
   I think you've corrected a bug; the intention with these setter methods seems to be one-time setting



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3026742307

   @kapoisu since you have touched `encryption.h|cc` recently, please have a look if my changes respect your code style introduced in #46202.


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3058485182

   :heavy_check_mark: All green.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2037679831


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +47,28 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  virtual encryption::SecureString GetKey(const std::string& key_metadata) = 0;
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, const encryption::SecureString& key);
+  encryption::SecureString GetKey(const std::string& key_metadata) override;

Review Comment:
   Inline the implementation into the header file to show the user how to encode the `key_id` in the `key_metadata` string?



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039330971


##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -182,79 +175,75 @@ FileEncryptionProperties::Builder::disable_aad_prefix_storage() {
 }
 
 ColumnEncryptionProperties::ColumnEncryptionProperties(bool encrypted,
-                                                       const std::string& column_path,
-                                                       const std::string& key,
-                                                       const std::string& key_metadata)
-    : column_path_(column_path) {
-  DCHECK(!column_path.empty());
+                                                       std::string column_path,
+                                                       encryption::SecureString key,
+                                                       std::string key_metadata)
+    : column_path_(std::move(column_path)),
+      encrypted_(encrypted),
+      encrypted_with_footer_key_(encrypted && key.empty()),
+      key_(std::move(key)),
+      key_metadata_(std::move(key_metadata)) {
+  DCHECK(!column_path_.empty());
   if (!encrypted) {
     DCHECK(key.empty() && key_metadata.empty());
   }
-
-  if (!key.empty()) {
-    DCHECK(key.length() == 16 || key.length() == 24 || key.length() == 32);
+  if (!key_.empty()) {
+    DCHECK(key_.length() == 16 || key_.length() == 24 || key_.length() == 32);
   }
-
-  encrypted_with_footer_key_ = (encrypted && key.empty());
   if (encrypted_with_footer_key_) {
-    DCHECK(key_metadata.empty());
+    DCHECK(key_metadata_.empty());
   }
-
-  encrypted_ = encrypted;
-  key_metadata_ = key_metadata;
-  key_ = key;
 }
 
-ColumnDecryptionProperties::ColumnDecryptionProperties(const std::string& column_path,
-                                                       const std::string& key)
-    : column_path_(column_path) {
-  DCHECK(!column_path.empty());
+ColumnDecryptionProperties::ColumnDecryptionProperties(std::string column_path,
+                                                       encryption::SecureString key)
+    : column_path_(std::move(column_path)), key_(std::move(key)) {
+  DCHECK(!column_path_.empty());
 
-  if (!key.empty()) {
-    DCHECK(key.length() == 16 || key.length() == 24 || key.length() == 32);
+  if (!key_.empty()) {
+    DCHECK(key_.length() == 16 || key_.length() == 24 || key_.length() == 32);
   }
-
-  key_ = key;
 }
 
-std::string FileDecryptionProperties::column_key(const std::string& column_path) const {
+const encryption::SecureString& FileDecryptionProperties::column_key(
+    const std::string& column_path) const {
   if (column_decryption_properties_.find(column_path) !=
       column_decryption_properties_.end()) {
     auto column_prop = column_decryption_properties_.at(column_path);
     if (column_prop != nullptr) {
       return column_prop->key();
     }
   }
-  return empty_string_;
+  return no_key_;

Review Comment:
   we are still returning a reference to a `SecureString`. I tried to return some `std::optional`, to replace empty `SecureString` notation for "no key given" with `std::nullopt`, but this opened a can of worms. Not going down this rabbit whole in this PR.
   
   At least the naming is improved.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039322897


##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -182,79 +175,75 @@ FileEncryptionProperties::Builder::disable_aad_prefix_storage() {
 }
 
 ColumnEncryptionProperties::ColumnEncryptionProperties(bool encrypted,
-                                                       const std::string& column_path,
-                                                       const std::string& key,
-                                                       const std::string& key_metadata)
-    : column_path_(column_path) {
-  DCHECK(!column_path.empty());
+                                                       std::string column_path,
+                                                       encryption::SecureString key,
+                                                       std::string key_metadata)
+    : column_path_(std::move(column_path)),

Review Comment:
   This moves member initialization from the body, which is the more standard way. However, the body could accidentally check input arguments, which after `std::move` are "empty".
   
   While this is correct:
   ```
         encrypted_with_footer_key_(encrypted && key.empty()),
         key_(std::move(key)),
   ```
   if members are shuffled around, then this becomes incorrect:
   ```
         key_(std::move(key)),
         encrypted_with_footer_key_(encrypted && key.empty()),
   ```
   as `encrypted_with_footer_key_` is initialized on the "empty" `key`.
   
   Do you consider this pattern too risky for future bugs? Happy to revert this initialization.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039448453


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -327,19 +322,19 @@ class PARQUET_EXPORT FileDecryptionProperties {
   }
 
  private:
-  std::string footer_key_;
+  encryption::SecureString footer_key_;
   std::string aad_prefix_;
   std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;
 
-  const std::string empty_string_ = "";
+  const encryption::SecureString empty_string_ = encryption::SecureString("");

Review Comment:
   Still needed, improved name.



-- 
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] GH-31603: [C++][Python] Wrap Parquet encryption keys in secure string [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2916399685

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2948949098

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2029078221


##########
cpp/src/parquet/encryption/internal_file_encryptor.h:
##########
@@ -36,7 +36,7 @@ class ColumnEncryptionProperties;
 
 class PARQUET_EXPORT Encryptor {
  public:
-  Encryptor(encryption::AesEncryptor* aes_encryptor, const std::string& key,
+  Encryptor(encryption::AesEncryptor* aes_encryptor, const encryption::SecureString& key,
             const std::string& file_aad, const std::string& aad,
             ::arrow::MemoryPool* pool);

Review Comment:
   And then move construct?
   ```
       : aes_encryptor_(aes_encryptor),
         key_(std::move(key)),
         file_aad_(std::move(file_aad)),
         aad_(std::move(aad)),
   ```
   Won't that copy the arguments when calling the constructor, or will that be optimized due to the `std::move`?



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2037668281


##########
cpp/src/parquet/encryption/internal_file_encryptor.h:
##########
@@ -36,7 +36,7 @@ class ColumnEncryptionProperties;
 
 class PARQUET_EXPORT Encryptor {
  public:
-  Encryptor(encryption::AesEncryptor* aes_encryptor, const std::string& key,
+  Encryptor(encryption::AesEncryptor* aes_encryptor, const encryption::SecureString& key,
             const std::string& file_aad, const std::string& aad,
             ::arrow::MemoryPool* pool);

Review Comment:
   I think the rational here is that the value is being copied anyway, either from the `const T&`, or when calling by value. Your way does not hide that fact from the API.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039342034


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class PARQUET_EXPORT SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;

Review Comment:
   I have added SecureString(size_t, char) to mimic `basic_string<char>` constructors.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2182909723


##########
cpp/src/parquet/encryption/kms_client.h:
##########
@@ -79,14 +80,47 @@ class PARQUET_EXPORT KmsClient {
   static constexpr const char kKmsInstanceUrlDefault[] = "DEFAULT";
   static constexpr const char kKeyAccessTokenDefault[] = "DEFAULT";
 
-  /// Wraps a key - encrypts it with the master key, encodes the result
+  /// \brief Wraps a key.
+  ///
+  /// Encrypts it with the master key, encodes the result
   /// and potentially adds a KMS-specific metadata.
+  ///
+  /// \deprecated Deprecated since 21.0.0. Implement
+  ///             WrapKey(const SecureString&, const std::string&) instead.
+  ARROW_DEPRECATED(
+      "Deprecated in 21.0.0. "
+      "Implement WrapKey(const SecureString&, const std::string&) instead.")
   virtual std::string WrapKey(const std::string& key_bytes,
-                              const std::string& master_key_identifier) = 0;
+                              const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc WrapKey(const std::string&, const std::string&)
+  virtual std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
+                              const std::string& master_key_identifier) {
+    ARROW_SUPPRESS_DEPRECATION_WARNING
+    auto key = WrapKey(std::string(key_bytes.as_view()), master_key_identifier);
+    ARROW_UNSUPPRESS_DEPRECATION_WARNING
+    return key;

Review Comment:
   In this case, yes, since the new idioms are more secure.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3026660908

   > @pitrou yes, I do. I think I have addressed all comments.
   
   Ah, thank you. I was wondering, because this PR is still in the "draft" state. Do you want to mark it ready for review?


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2194773454


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -310,18 +325,22 @@ class PARQUET_EXPORT FileDecryptionProperties {
   }
 
  private:
-  std::string footer_key_;
+  ::arrow::util::SecureString footer_key_;
   std::string aad_prefix_;
   std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;
 
+  // any empty SecureString key is interpreted as if no key is given
+  // this instance is used if a SecureString reference is returned
+  const ::arrow::util::SecureString no_key_ = ::arrow::util::SecureString();

Review Comment:
   Does this need to be an instance variable? It could just be a private constant inside `encryption.cc`



##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +48,39 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  /// \brief Retrieve a key.
+  virtual ::arrow::util::SecureString GetKey(const std::string& key_id) = 0;
+
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, ::arrow::util::SecureString key);
+
+  ::arrow::util::SecureString GetKey(const std::string& key_id_string) override {
+    // key_id_string is string but for IntegerKeyIdRetriever it encodes

Review Comment:
   I'm not sure it's worth inlining this method, is it? Especially as it will usually be called through the vtable, anyway.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3057082335

   @EnricoMi We have two +1s after a meticulous review pass, so perhaps you can go ahead and revert the compatibility-breaking changes so that we get a clean CI and merge.


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2182823133


##########
cpp/src/parquet/encryption/kms_client.h:
##########
@@ -79,14 +80,47 @@ class PARQUET_EXPORT KmsClient {
   static constexpr const char kKmsInstanceUrlDefault[] = "DEFAULT";
   static constexpr const char kKeyAccessTokenDefault[] = "DEFAULT";
 
-  /// Wraps a key - encrypts it with the master key, encodes the result
+  /// \brief Wraps a key.
+  ///
+  /// Encrypts it with the master key, encodes the result
   /// and potentially adds a KMS-specific metadata.
+  ///
+  /// \deprecated Deprecated since 21.0.0. Implement
+  ///             WrapKey(const SecureString&, const std::string&) instead.
+  ARROW_DEPRECATED(
+      "Deprecated in 21.0.0. "
+      "Implement WrapKey(const SecureString&, const std::string&) instead.")
   virtual std::string WrapKey(const std::string& key_bytes,
-                              const std::string& master_key_identifier) = 0;
+                              const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc WrapKey(const std::string&, const std::string&)
+  virtual std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
+                              const std::string& master_key_identifier) {
+    ARROW_SUPPRESS_DEPRECATION_WARNING
+    auto key = WrapKey(std::string(key_bytes.as_view()), master_key_identifier);
+    ARROW_UNSUPPRESS_DEPRECATION_WARNING
+    return key;
+  }
 
-  /// Decrypts (unwraps) a key with the master key.
+  /// \brief Decrypts (unwraps) a key with the master key.
+  /// \deprecated Deprecated since 21.0.0. Implement UnWrapKey instead.
+  ARROW_DEPRECATED("Deprecated in 21.0.0. Implement UnWrapKey instead.")
   virtual std::string UnwrapKey(const std::string& wrapped_key,
-                                const std::string& master_key_identifier) = 0;
+                                const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc UnwrapKey(const std::string&, const std::string&)

Review Comment:
   Docu say:
   > Note that \copydoc foo() is roughly equivalent to doing:
   >  \brief \copybrief foo()
   >  \details \copydetails foo()
   
   https://www.doxygen.nl/manual/commands.html#cmdcopydoc
   
   which sounds like this would not copy the deprecation.
   
   But you are right, to be sure I can duplicate the simple doc.
   
   



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2111781121


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {

Review Comment:
   I have extracted the `SecureString` implementation into a separate PR: #46626. That targets `arrow/util`.
   
   This PR will focus in migrating parquet encryption to that `SecureString` implementation.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2078201530


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   If it's in the public domain, then I imagine an inline comment referencing that should be sufficient. @pitrou do you agree? We do also account for some public domain artifacts [in LICENSE.txt](https://github.com/apache/arrow/blob/7df396eec4c620ddc4a4db780ea0d5ffa4cf685b/LICENSE.txt#L1466-L1470)



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2775159330

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2029158514


##########
cpp/src/parquet/encryption/file_key_wrapper.cc:
##########
@@ -113,14 +113,15 @@ KeyEncryptionKey FileKeyWrapper::CreateKeyEncryptionKey(
     const std::string& master_key_id) {
   std::string kek_bytes(kKeyEncryptionKeyLength, '\0');
   RandBytes(reinterpret_cast<uint8_t*>(kek_bytes.data()), kKeyEncryptionKeyLength);
+  SecureString secure_kek_bytes(std::move(kek_bytes));

Review Comment:
   ```suggestion
     SecureString secure_kek_bytes(std::string(kKeyEncryptionKeyLength, '\0'));
     RandBytes(secure_kek_bytes.as_span().data(), kKeyEncryptionKeyLength);
   ```



##########
cpp/src/parquet/encryption/read_configurations_test.cc:
##########
@@ -103,9 +103,9 @@ class TestDecryptionConfiguration
   // This vector will hold various decryption configurations.
   std::vector<std::shared_ptr<parquet::FileDecryptionProperties>>
       vector_of_decryption_configurations_;
-  std::string kFooterEncryptionKey_ = std::string(kFooterEncryptionKey);
-  std::string kColumnEncryptionKey1_ = std::string(kColumnEncryptionKey1);
-  std::string kColumnEncryptionKey2_ = std::string(kColumnEncryptionKey2);
+  SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;

Review Comment:
   `const` seems warranted here
   ```suggestion
     const SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;
   ```



##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +47,28 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  virtual encryption::SecureString GetKey(const std::string& key_metadata) = 0;
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, const encryption::SecureString& key);
+  encryption::SecureString GetKey(const std::string& key_metadata) override;

Review Comment:
   Nit: it's not relevant to this PR, but this signature is surprising enough that I think it'd be worthwhile to inline this function and include a comment
   
   ```suggestion
     encryption::SecureString GetKey(const std::string& key_metadata) override {
       // key_metadata is string but for IntegerKeyIdRetriever it encodes
       // a native-endian 32 bit unsigned integer key_id
       uint32_t key_id;
       assert(key_metadata.size() == sizeof(key_id));
       memcpy(&key_id, key_metadata.data(), sizeof(key_id));
       return GetKey(key_id);
     }
   
     encryption::SecureString GetKey(uint32_t key_id) {
       return key_map_.at(key_id);
     }
   ```



##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   The standardese "true" way would be to template SecureString on an allocator so that you could intercept destruction of the bytes it stores. That's entirely too much here; we could just add a debug assertion inside ~SecureString (maybe only enabled by an env var or `#if defined(ARROW_ASSERT_SECURE_STRING_ZEROED)`)



##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class PARQUET_EXPORT SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;

Review Comment:
   ```suggestion
     SecureString(const SecureString&) = default;
   ```
   
   This should not be `noexcept`; copying the string requires a new allocation which may throw



##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class PARQUET_EXPORT SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;

Review Comment:
   ```suggestion
     explicit SecureString(std::string&&) noexcept;
     explicit SecureString(size_t size) : SecureString{std::string(size, '\0')} {}
   ```



##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class PARQUET_EXPORT SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;
+
+  SecureString& operator=(SecureString&&) noexcept;
+  SecureString& operator=(const SecureString&) noexcept;

Review Comment:
   ```suggestion
     SecureString& operator=(const SecureString&);
   ```
   (same for copy assignment)



##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   It seems questionable to me that the ultimate fallback should not be a simple loop
   
   ```suggestion
     for (size_t i = 0; i < size; ++i) {
       data[i] = 0;
     }
   ```
   
   I don't know how cross platform this `__asm__` block is.
   
   
   Also, since this code is borrowed from another project it needs to be mentioned in LICENSE.txt, for example https://github.com/apache/arrow/blob/7df396eec4c620ddc4a4db780ea0d5ffa4cf685b/LICENSE.txt#L1541-L1543



##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -46,7 +46,7 @@ int32_t Decryptor::CiphertextLength(int32_t plaintext_len) const {
 
 int32_t Decryptor::Decrypt(::arrow::util::span<const uint8_t> ciphertext,
                            ::arrow::util::span<uint8_t> plaintext) {
-  return aes_decryptor_->Decrypt(ciphertext, str2span(key_), str2span(aad_), plaintext);
+  return aes_decryptor_->Decrypt(ciphertext, key_.as_span(), str2span(aad_), plaintext);

Review Comment:
   Since `str2span` is no longer used, we could delete it



##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -327,19 +322,19 @@ class PARQUET_EXPORT FileDecryptionProperties {
   }
 
  private:
-  std::string footer_key_;
+  encryption::SecureString footer_key_;
   std::string aad_prefix_;
   std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;
 
-  const std::string empty_string_ = "";
+  const encryption::SecureString empty_string_ = encryption::SecureString("");

Review Comment:
   This looks like a holdover from the first draft of encryption; IIRC column_key() returned references to strings at that time (so `return "";` would fail to compile)



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2037697271


##########
cpp/src/parquet/encryption/read_configurations_test.cc:
##########
@@ -103,9 +103,9 @@ class TestDecryptionConfiguration
   // This vector will hold various decryption configurations.
   std::vector<std::shared_ptr<parquet::FileDecryptionProperties>>
       vector_of_decryption_configurations_;
-  std::string kFooterEncryptionKey_ = std::string(kFooterEncryptionKey);
-  std::string kColumnEncryptionKey1_ = std::string(kColumnEncryptionKey1);
-  std::string kColumnEncryptionKey2_ = std::string(kColumnEncryptionKey2);
+  SecureString kFooterEncryptionKey_ = kFooterEncryptionKey;

Review Comment:
   Doesn't that hold for all `k...` members in this class (`TestDecryptionConfiguration`) and `TestEncryptionConfiguration` as well? Is it worth it given this are tests?



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039529735


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class PARQUET_EXPORT SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;

Review Comment:
   done



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2107821094


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   got it



##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }

Review Comment:
   added



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2179565092


##########
cpp/src/parquet/encryption/test_encryption_util.h:
##########
@@ -40,12 +41,13 @@ class ParquetFileReader;
 namespace encryption::test {
 
 using ::arrow::internal::TemporaryDir;
+using ::arrow::util::SecureString;
 
 constexpr int kFixedLength = 10;
 
-const char kFooterEncryptionKey[] = "0123456789012345";  // 128bit/16
-const char kColumnEncryptionKey1[] = "1234567890123450";
-const char kColumnEncryptionKey2[] = "1234567890123451";
+inline SecureString kFooterEncryptionKey("0123456789012345");
+inline SecureString kColumnEncryptionKey1("1234567890123450");
+inline SecureString kColumnEncryptionKey2("1234567890123451");

Review Comment:
   Nit, but perhaps we can make these `const`?



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2179579428


##########
cpp/src/parquet/encryption/kms_client.h:
##########
@@ -79,14 +80,47 @@ class PARQUET_EXPORT KmsClient {
   static constexpr const char kKmsInstanceUrlDefault[] = "DEFAULT";
   static constexpr const char kKeyAccessTokenDefault[] = "DEFAULT";
 
-  /// Wraps a key - encrypts it with the master key, encodes the result
+  /// \brief Wraps a key.
+  ///
+  /// Encrypts it with the master key, encodes the result
   /// and potentially adds a KMS-specific metadata.
+  ///
+  /// \deprecated Deprecated since 21.0.0. Implement
+  ///             WrapKey(const SecureString&, const std::string&) instead.
+  ARROW_DEPRECATED(
+      "Deprecated in 21.0.0. "
+      "Implement WrapKey(const SecureString&, const std::string&) instead.")
   virtual std::string WrapKey(const std::string& key_bytes,
-                              const std::string& master_key_identifier) = 0;
+                              const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc WrapKey(const std::string&, const std::string&)
+  virtual std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
+                              const std::string& master_key_identifier) {
+    ARROW_SUPPRESS_DEPRECATION_WARNING
+    auto key = WrapKey(std::string(key_bytes.as_view()), master_key_identifier);
+    ARROW_UNSUPPRESS_DEPRECATION_WARNING
+    return key;
+  }
 
-  /// Decrypts (unwraps) a key with the master key.
+  /// \brief Decrypts (unwraps) a key with the master key.
+  /// \deprecated Deprecated since 21.0.0. Implement UnWrapKey instead.
+  ARROW_DEPRECATED("Deprecated in 21.0.0. Implement UnWrapKey instead.")
   virtual std::string UnwrapKey(const std::string& wrapped_key,
-                                const std::string& master_key_identifier) = 0;
+                                const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc UnwrapKey(const std::string&, const std::string&)
+  virtual ::arrow::util::SecureString UnWrapKey(
+      const std::string& wrapped_key, const std::string& master_key_identifier) {
+    ARROW_SUPPRESS_DEPRECATION_WARNING

Review Comment:
   Same question here re. the default implementation.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3048893227

   > > But this will require migrating the Python API as well: https://github.com/apache/arrow/actions/runs/16126071702/job/45503157743?pr=46017#step:6:3956
   > > I would like to move touching the Python API to a separate PR, which would contain the revert [290449a](https://github.com/apache/arrow/pull/46017/commits/290449a36c4b45fc5e6be931a6a12b378d80cdf4).
   > 
   > Ok, that sounds good to me.
   
   Alright, once you approve this PR, I will revert [290449a](https://github.com/apache/arrow/pull/46017/commits/290449a36c4b45fc5e6be931a6a12b378d80cdf4) and we are ready to merge.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2111781121


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {

Review Comment:
   I have extracted the `SecureString` implementation intoa separate PR: #46626. That targets `arrow/util`.
   
   This PR will focus in migrating parquet encryption to that `SecureString` implementation.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2083999897


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   We could add a comment, yes.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2027252797


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,66 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;
+
+  SecureString& operator=(SecureString&&) noexcept;
+  SecureString& operator=(const SecureString&) noexcept;
+  SecureString& operator=(std::string&& secret) noexcept;
+
+  bool operator==(const SecureString&) const;
+  bool operator!=(const SecureString&) const;
+
+  ~SecureString() { dispose(); }
+
+  [[nodiscard]] bool empty() const;
+  [[nodiscard]] std::size_t size() const;
+  [[nodiscard]] std::size_t length() const;
+  [[nodiscard]] ::arrow::util::span<const uint8_t> as_span() const;
+
+  void dispose();
+
+  static void secure_clear(std::string&);

Review Comment:
   We mostly prohibit mutable ref arguments, so use a pointer.
   ```suggestion
     static void SecureClear(std::string*);
   ```



##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,66 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;
+
+  SecureString& operator=(SecureString&&) noexcept;
+  SecureString& operator=(const SecureString&) noexcept;
+  SecureString& operator=(std::string&& secret) noexcept;
+
+  bool operator==(const SecureString&) const;
+  bool operator!=(const SecureString&) const;
+
+  ~SecureString() { dispose(); }
+
+  [[nodiscard]] bool empty() const;
+  [[nodiscard]] std::size_t size() const;
+  [[nodiscard]] std::size_t length() const;
+  [[nodiscard]] ::arrow::util::span<const uint8_t> as_span() const;

Review Comment:
   Also perhaps add a `string_view` accessor and a mutable `span` one?
   ```suggestion
     [[nodiscard]] ::arrow::util::span<uint8_t> as_span();
     [[nodiscard]] ::arrow::util::span<const uint8_t> as_span() const;
     [[nodiscard]] std::string_view as_view();
   ```



##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }

Review Comment:
   For more stringent testing, I think we also want to examine the string area _before_ it is cleared (or moved, etc.).
   
   Something like (untested):
   ```c++
   std::string_view StringArea(const std::string& string) {
     return std::string_view(string.data(), string.capacity());
   }
   
   void AssertSecurelyCleared(std::string_view area) {
     // the entire area is filled with zeros
     std::string zeros(area.size(), '\0');
     ASSERT_EQ(area, std::string_view(zeros));
   }
   
   void AssertSecurelyCleared(const std::string& string) {
     AssertSecurelyCleared(StringArea(string));
   }
   
   TEST(TestSecureString, SecureClearCheck) {
     // short string
     {
       std::string tiny("abc");
       auto old_area = StringArea(tiny);
       SecureString::SecureClear(tiny);
       AssertSecurelyCleared(tiny);
       AssertSecurelyCleared(old_area);
     }
     // etc.



##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -431,15 +424,16 @@ class PARQUET_EXPORT FileEncryptionProperties {
 
  private:
   EncryptionAlgorithm algorithm_;
-  std::string footer_key_;
+  encryption::SecureString footer_key_;
   std::string footer_key_metadata_;
   bool encrypted_footer_;
   std::string file_aad_;
   std::string aad_prefix_;
   bool store_aad_prefix_in_file_;
   ColumnPathToEncryptionPropertiesMap encrypted_columns_;
 
-  FileEncryptionProperties(ParquetCipher::type cipher, const std::string& footer_key,
+  FileEncryptionProperties(ParquetCipher::type cipher,
+                           const encryption::SecureString& footer_key,

Review Comment:
   Here as well, could avoid copies by taking strings by value.



##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +47,28 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  virtual encryption::SecureString GetKey(const std::string& key_metadata) = 0;
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, const encryption::SecureString& key);

Review Comment:
   Could probably take the `SecureString` by value here, since we're going to store it.



##########
cpp/src/parquet/encryption/internal_file_decryptor.h:
##########
@@ -39,9 +40,9 @@ class FileDecryptionProperties;
 // CAUTION: Decryptor objects are not thread-safe.
 class PARQUET_EXPORT Decryptor {
  public:
-  Decryptor(std::unique_ptr<encryption::AesDecryptor> decryptor, const std::string& key,
-            const std::string& file_aad, const std::string& aad,
-            ::arrow::MemoryPool* pool);
+  Decryptor(std::unique_ptr<encryption::AesDecryptor> decryptor,
+            const encryption::SecureString& key, const std::string& file_aad,
+            const std::string& aad, ::arrow::MemoryPool* pool);

Review Comment:
   Same here (take strings by value)



##########
cpp/src/parquet/encryption/file_key_wrapper.cc:
##########
@@ -113,14 +113,15 @@ KeyEncryptionKey FileKeyWrapper::CreateKeyEncryptionKey(
     const std::string& master_key_id) {
   std::string kek_bytes(kKeyEncryptionKeyLength, '\0');
   RandBytes(reinterpret_cast<uint8_t*>(kek_bytes.data()), kKeyEncryptionKeyLength);
+  SecureString secure_kek_bytes(std::move(kek_bytes));

Review Comment:
   Here as well, we could write directly into a pre-sized SecureString.



##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -402,21 +397,19 @@ class PARQUET_EXPORT FileEncryptionProperties {
    private:
     ParquetCipher::type parquet_cipher_;
     bool encrypted_footer_;
-    std::string footer_key_;
+    encryption::SecureString footer_key_;
     std::string footer_key_metadata_;
 
     std::string aad_prefix_;
     bool store_aad_prefix_in_file_;
     ColumnPathToEncryptionPropertiesMap encrypted_columns_;
   };
 
-  ~FileEncryptionProperties() { footer_key_.clear(); }
-
   bool encrypted_footer() const { return encrypted_footer_; }
 
   EncryptionAlgorithm algorithm() const { return algorithm_; }
 
-  std::string footer_key() const { return footer_key_; }
+  encryption::SecureString footer_key() const { return footer_key_; }

Review Comment:
   Could perhaps return a const ref, assuming the member variable doesn't change?



##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   Probably also want a test that a SecureString was cleared on destruction ?



##########
cpp/src/parquet/encryption/key_toolkit.h:
##########
@@ -92,14 +92,14 @@ class PARQUET_EXPORT KeyToolkit {
 // parsing from "key material"
 class PARQUET_EXPORT KeyWithMasterId {
  public:
-  KeyWithMasterId(std::string key_bytes, std::string master_id)
+  KeyWithMasterId(SecureString key_bytes, std::string master_id)
       : key_bytes_(std::move(key_bytes)), master_id_(std::move(master_id)) {}
 
-  const std::string& data_key() const { return key_bytes_; }
+  const SecureString& data_key() const { return key_bytes_; }
   const std::string& master_id() const { return master_id_; }
 
  private:
-  const std::string key_bytes_;
+  const SecureString key_bytes_;
   const std::string master_id_;

Review Comment:
   I'm not sure it makes sense to keep the `const` qualifiers here since all methods are `const` already.
   
   With `const` member variables, `KeyMasterId` cannot be moved but will always be copied.
   



##########
cpp/src/parquet/encryption/internal_file_encryptor.h:
##########
@@ -36,7 +36,7 @@ class ColumnEncryptionProperties;
 
 class PARQUET_EXPORT Encryptor {
  public:
-  Encryptor(encryption::AesEncryptor* aes_encryptor, const std::string& key,
+  Encryptor(encryption::AesEncryptor* aes_encryptor, const encryption::SecureString& key,
             const std::string& file_aad, const std::string& aad,
             ::arrow::MemoryPool* pool);

Review Comment:
   We could probably take those arguments by value, to avoid copying them in the constructor?
   ```suggestion
     Encryptor(encryption::AesEncryptor* aes_encryptor, encryption::SecureString key,
               std::string file_aad, std::string aad,
               ::arrow::MemoryPool* pool);
   ```



##########
cpp/src/parquet/encryption/key_toolkit_internal.cc:
##########
@@ -59,10 +59,10 @@ std::string DecryptKeyLocally(const std::string& encoded_encrypted_key,
   ::arrow::util::span<uint8_t> decrypted_key_span(
       reinterpret_cast<uint8_t*>(&decrypted_key[0]), decrypted_key_len);

Review Comment:
   Perhaps we could write directly into an appropriately-sized `SecureString`?



##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,66 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;
+
+  SecureString& operator=(SecureString&&) noexcept;
+  SecureString& operator=(const SecureString&) noexcept;
+  SecureString& operator=(std::string&& secret) noexcept;
+
+  bool operator==(const SecureString&) const;
+  bool operator!=(const SecureString&) const;
+
+  ~SecureString() { dispose(); }
+
+  [[nodiscard]] bool empty() const;
+  [[nodiscard]] std::size_t size() const;
+  [[nodiscard]] std::size_t length() const;
+  [[nodiscard]] ::arrow::util::span<const uint8_t> as_span() const;
+
+  void dispose();
+
+  static void secure_clear(std::string&);
+  static void secure_clear(uint8_t* data, size_t size);

Review Comment:
   Nit: non-trivial methods should be `CamelCase`, so `Dispose` and `SecureClear`.



##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  secure_clear(secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  dispose();
+  secret_ = std::move(secret);
+  secure_clear(secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return str2span(secret_);

Review Comment:
   `str2span` is a trivial function, we can probably copy its contents here.



##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -327,19 +322,19 @@ class PARQUET_EXPORT FileDecryptionProperties {
   }
 
  private:
-  std::string footer_key_;
+  encryption::SecureString footer_key_;
   std::string aad_prefix_;
   std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;
 
-  const std::string empty_string_ = "";
+  const encryption::SecureString empty_string_ = encryption::SecureString("");
   ColumnPathToDecryptionPropertiesMap column_decryption_properties_;
 
   std::shared_ptr<DecryptionKeyRetriever> key_retriever_;
   bool check_plaintext_footer_integrity_;
   bool plaintext_files_allowed_;
 
   FileDecryptionProperties(
-      const std::string& footer_key,
+      const encryption::SecureString& footer_key,

Review Comment:
   Same comments above.



##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -327,19 +322,19 @@ class PARQUET_EXPORT FileDecryptionProperties {
   }
 
  private:
-  std::string footer_key_;
+  encryption::SecureString footer_key_;
   std::string aad_prefix_;
   std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;
 
-  const std::string empty_string_ = "";
+  const encryption::SecureString empty_string_ = encryption::SecureString("");

Review Comment:
   Is `empty_string_` useful at all? It looks rather dubious.
   



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2031169102


##########
cpp/src/parquet/encryption/key_toolkit.h:
##########
@@ -92,14 +92,14 @@ class PARQUET_EXPORT KeyToolkit {
 // parsing from "key material"
 class PARQUET_EXPORT KeyWithMasterId {
  public:
-  KeyWithMasterId(std::string key_bytes, std::string master_id)
+  KeyWithMasterId(SecureString key_bytes, std::string master_id)
       : key_bytes_(std::move(key_bytes)), master_id_(std::move(master_id)) {}
 
-  const std::string& data_key() const { return key_bytes_; }
+  const SecureString& data_key() const { return key_bytes_; }
   const std::string& master_id() const { return master_id_; }
 
  private:
-  const std::string key_bytes_;
+  const SecureString key_bytes_;
   const std::string master_id_;

Review Comment:
   Removed, also added `[[nodiscard]]` to the constant getter methods.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039495362


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   What about
   ```c++
     volatile char *p = data;
     while (size--) *p++ = 0;
   ```
   https://www.google.com/url?sa=t&source=web&rct=j&opi=89978449&url=https://cplusplus.com/articles/ETqpX9L8/&ved=2ahUKEwjF5MvK-8-MAxUFnf0HHUiTAcYQFnoECBEQAQ&usg=AOvVaw2otjQoJ6qGsoLpmq4cIidL



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039322897


##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -182,79 +175,75 @@ FileEncryptionProperties::Builder::disable_aad_prefix_storage() {
 }
 
 ColumnEncryptionProperties::ColumnEncryptionProperties(bool encrypted,
-                                                       const std::string& column_path,
-                                                       const std::string& key,
-                                                       const std::string& key_metadata)
-    : column_path_(column_path) {
-  DCHECK(!column_path.empty());
+                                                       std::string column_path,
+                                                       encryption::SecureString key,
+                                                       std::string key_metadata)
+    : column_path_(std::move(column_path)),

Review Comment:
   This moves member initialization from the body, which is the more standard way. However, the body could accidentally check input arguments, which after `std::move` are "empty".
   
   While this is correct:
   ``
         encrypted_with_footer_key_(encrypted && key.empty()),
         key_(std::move(key)),
   ```
   if members are shuffled around, then this becomes incorrect:
   ```
         key_(std::move(key)),
         encrypted_with_footer_key_(encrypted && key.empty()),
   ```
   as `encrypted_with_footer_key_` is initialized on the "empty" `key`.
   
   Do you consider this pattern too risky for future bugs? Happy to revert this initialization.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2038218641


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +47,28 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  virtual encryption::SecureString GetKey(const std::string& key_metadata) = 0;
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, const encryption::SecureString& key);
+  encryption::SecureString GetKey(const std::string& key_metadata) override;

Review Comment:
   Yes, exactly



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2796879639

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039301498


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +47,28 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  virtual encryption::SecureString GetKey(const std::string& key_metadata) = 0;
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, const encryption::SecureString& key);
+  encryption::SecureString GetKey(const std::string& key_metadata) override;

Review Comment:
   done



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2031169102


##########
cpp/src/parquet/encryption/key_toolkit.h:
##########
@@ -92,14 +92,14 @@ class PARQUET_EXPORT KeyToolkit {
 // parsing from "key material"
 class PARQUET_EXPORT KeyWithMasterId {
  public:
-  KeyWithMasterId(std::string key_bytes, std::string master_id)
+  KeyWithMasterId(SecureString key_bytes, std::string master_id)
       : key_bytes_(std::move(key_bytes)), master_id_(std::move(master_id)) {}
 
-  const std::string& data_key() const { return key_bytes_; }
+  const SecureString& data_key() const { return key_bytes_; }
   const std::string& master_id() const { return master_id_; }
 
  private:
-  const std::string key_bytes_;
+  const SecureString key_bytes_;
   const std::string master_id_;

Review Comment:
   Removed



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039502770


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   Given https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h is [CC0 Public Domain Dedication](http://creativecommons.org/publicdomain/zero/1.0/), and our code deviates a lot, you could say it is as much inspired by that implementation than it is by all those variations available on stackoverflow.
   
   I don't think this qualifies as a copy or derived work.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2028970319


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  secure_clear(secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  dispose();
+  secret_ = std::move(secret);
+  secure_clear(secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return str2span(secret_);

Review Comment:
   Content is
   ```c++
     if (str.empty()) {
       return {};
     }
   
     return {reinterpret_cast<const uint8_t*>(str.data()), str.size()};
   ```
   I am not sure how much benefit the `if` clause provides, but I doubt it pays off for the extra conditional branch. `SecureString` could be expected not to be empty in general, so I removed the `if` clause from `as_span`.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2029208776


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   Oh, alternatively: since every STL I'm aware of implements std::string with an inline short string case, we could use:
   
   ```c++
     // Most std::string impls store their contents inline, so we can check
     // the object representation for a crib to ensure that destruction does
     // wipe all sensitive bytes.
   
     constexpr auto kCrib = "01CRIB01";
     char object_repr[sizeof(SecureString)];
     std::string_view object_repr_view{object_repr, sizeof(object_repr)};
   
     SecureString* cribbed = new (&object_repr) SecureString(std::string(kCrib));
     bool crib_found_before = object_repr_view.find(kCrib) != object_repr_view.npos;
     cribbed->~SecureString(); // NOTE: do not allow this call to be skipped!
     bool crib_found_after = object_repr_view.find(kCrib) != object_repr_view.npos;
   
     if (crib_found_before) {
       ASSERT_FALSE(crib_found_after);
     }
   ```
   
   I think that's well-defined: https://godbolt.org/z/K87ndec5z



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3074091024

   Revision: 04b15e2bfc94970670605e3fe21bc45680b29b67
   
   Submitted crossbow builds: [ursacomputing/crossbow @ actions-12f2531100](https://github.com/ursacomputing/crossbow/branches/all?query=actions-12f2531100)
   
   |Task|Status|
   |----|------|
   |example-cpp-minimal-build-static|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-example-cpp-minimal-build-static)](https://github.com/ursacomputing/crossbow/actions/runs/16297258664/job/46022126266)|
   |example-cpp-minimal-build-static-system-dependency|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-example-cpp-minimal-build-static-system-dependency)](https://github.com/ursacomputing/crossbow/actions/runs/16297258568/job/46022126311)|
   |example-cpp-tutorial|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-example-cpp-tutorial)](https://github.com/ursacomputing/crossbow/actions/runs/16297258777/job/46022127196)|
   |test-build-cpp-fuzz|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-build-cpp-fuzz)](https://github.com/ursacomputing/crossbow/actions/runs/16297258158/job/46022124809)|
   |test-conda-cpp|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-conda-cpp)](https://github.com/ursacomputing/crossbow/actions/runs/16297258721/job/46022126449)|
   |test-conda-cpp-valgrind|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-conda-cpp-valgrind)](https://github.com/ursacomputing/crossbow/actions/runs/16297258575/job/46022126172)|
   |test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-cuda-cpp-ubuntu-22.04-cuda-11.7.1)](https://github.com/ursacomputing/crossbow/actions/runs/16297258545/job/46022125938)|
   |test-debian-12-cpp-amd64|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-debian-12-cpp-amd64)](https://github.com/ursacomputing/crossbow/actions/runs/16297259030/job/46022127573)|
   |test-debian-12-cpp-i386|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-debian-12-cpp-i386)](https://github.com/ursacomputing/crossbow/actions/runs/16297258229/job/46022124978)|
   |test-fedora-42-cpp|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-fedora-42-cpp)](https://github.com/ursacomputing/crossbow/actions/runs/16297258438/job/46022125509)|
   |test-ubuntu-22.04-cpp|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-22.04-cpp)](https://github.com/ursacomputing/crossbow/actions/runs/16297258445/job/46022125596)|
   |test-ubuntu-22.04-cpp-20|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-22.04-cpp-20)](https://github.com/ursacomputing/crossbow/actions/runs/16297258276/job/46022125324)|
   |test-ubuntu-22.04-cpp-bundled|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-22.04-cpp-bundled)](https://github.com/ursacomputing/crossbow/actions/runs/16297258154/job/46022124814)|
   |test-ubuntu-22.04-cpp-emscripten|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-22.04-cpp-emscripten)](https://github.com/ursacomputing/crossbow/actions/runs/16297258787/job/46022126710)|
   |test-ubuntu-22.04-cpp-no-threading|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-22.04-cpp-no-threading)](https://github.com/ursacomputing/crossbow/actions/runs/16297258720/job/46022126549)|
   |test-ubuntu-24.04-cpp|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-24.04-cpp)](https://github.com/ursacomputing/crossbow/actions/runs/16297258152/job/46022124852)|
   |test-ubuntu-24.04-cpp-bundled-offline|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-24.04-cpp-bundled-offline)](https://github.com/ursacomputing/crossbow/actions/runs/16297258558/job/46022125932)|
   |test-ubuntu-24.04-cpp-gcc-13-bundled|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-24.04-cpp-gcc-13-bundled)](https://github.com/ursacomputing/crossbow/actions/runs/16297258214/job/46022124929)|
   |test-ubuntu-24.04-cpp-gcc-14|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-24.04-cpp-gcc-14)](https://github.com/ursacomputing/crossbow/actions/runs/16297258493/job/46022125821)|
   |test-ubuntu-24.04-cpp-minimal-with-formats|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-24.04-cpp-minimal-with-formats)](https://github.com/ursacomputing/crossbow/actions/runs/16297258292/job/46022125345)|
   |test-ubuntu-24.04-cpp-thread-sanitizer|[![GitHub Actions](https://github.com/ursacomputing/crossbow/actions/workflows/crossbow.yml/badge.svg?branch=actions-12f2531100-github-test-ubuntu-24.04-cpp-thread-sanitizer)](https://github.com/ursacomputing/crossbow/actions/runs/16297258579/job/46022126012)|


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2078226510


##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -182,79 +175,75 @@ FileEncryptionProperties::Builder::disable_aad_prefix_storage() {
 }
 
 ColumnEncryptionProperties::ColumnEncryptionProperties(bool encrypted,
-                                                       const std::string& column_path,
-                                                       const std::string& key,
-                                                       const std::string& key_metadata)
-    : column_path_(column_path) {
-  DCHECK(!column_path.empty());
+                                                       std::string column_path,
+                                                       encryption::SecureString key,
+                                                       std::string key_metadata)
+    : column_path_(std::move(column_path)),
+      encrypted_(encrypted),
+      encrypted_with_footer_key_(encrypted && key.empty()),
+      key_(std::move(key)),
+      key_metadata_(std::move(key_metadata)) {
+  DCHECK(!column_path_.empty());
   if (!encrypted) {
     DCHECK(key.empty() && key_metadata.empty());
   }
-
-  if (!key.empty()) {
-    DCHECK(key.length() == 16 || key.length() == 24 || key.length() == 32);
+  if (!key_.empty()) {
+    DCHECK(key_.length() == 16 || key_.length() == 24 || key_.length() == 32);
   }
-
-  encrypted_with_footer_key_ = (encrypted && key.empty());
   if (encrypted_with_footer_key_) {
-    DCHECK(key_metadata.empty());
+    DCHECK(key_metadata_.empty());
   }
-
-  encrypted_ = encrypted;
-  key_metadata_ = key_metadata;
-  key_ = key;
 }
 
-ColumnDecryptionProperties::ColumnDecryptionProperties(const std::string& column_path,
-                                                       const std::string& key)
-    : column_path_(column_path) {
-  DCHECK(!column_path.empty());
+ColumnDecryptionProperties::ColumnDecryptionProperties(std::string column_path,
+                                                       encryption::SecureString key)
+    : column_path_(std::move(column_path)), key_(std::move(key)) {
+  DCHECK(!column_path_.empty());
 
-  if (!key.empty()) {
-    DCHECK(key.length() == 16 || key.length() == 24 || key.length() == 32);
+  if (!key_.empty()) {
+    DCHECK(key_.length() == 16 || key_.length() == 24 || key_.length() == 32);
   }
-
-  key_ = key;
 }
 
-std::string FileDecryptionProperties::column_key(const std::string& column_path) const {
+const encryption::SecureString& FileDecryptionProperties::column_key(
+    const std::string& column_path) const {
   if (column_decryption_properties_.find(column_path) !=
       column_decryption_properties_.end()) {
     auto column_prop = column_decryption_properties_.at(column_path);
     if (column_prop != nullptr) {
       return column_prop->key();
     }
   }
-  return empty_string_;
+  return no_key_;

Review Comment:
   Another use for a generic default reference: https://github.com/apache/arrow/pull/46303#discussion_r2075958465



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2775928216

   > Is this the right direction before I roll this out to Python?
   
   Yes!
   
   > Can I break the API or should I deprecate exiting methods and provide both, std::string and secure string methods?
   
   I would rather deprecate the existing methods.
   
   > Do you have some hints how I can fix the link issues for Windows?
   
   You probably want to add a `PARQUET_EXPORT` annotation to the `SecureString` class?
   
   > Shouldn't it be sufficient to only use `OPENSSL_cleanse` and count on OpenSSL providing OS-dependent implementations?
   
   Well, currently the OpenSSL function is very crude :)
   https://github.com/openssl/openssl/blob/c66e00398c9feabc02ff6e678089a3dc95f985d2/crypto/mem_clr.c#L18-L25
   
   


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2182825901


##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -148,9 +150,9 @@ InternalFileDecryptor::GetColumnDecryptorFactory(
   // The column is encrypted with its own key
   const std::string& column_key_metadata = crypto_metadata->key_metadata();
   const std::string column_path = crypto_metadata->path_in_schema()->ToDotString();
-  std::string column_key = GetColumnKey(column_path, column_key_metadata);
+  const SecureString& column_key = GetColumnKey(column_path, column_key_metadata);
 
-  return [this, aad, metadata, column_key = std::move(column_key)]() {
+  return [this, aad, metadata, column_key = column_key]() {

Review Comment:
   Any action item for me? Shall I leave a comment in the code on this?



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2207780809


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +48,56 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  /// \brief Retrieve a key.
+  /// \deprecated Deprecated since 21.0.0.

Review Comment:
   Ahem. 21.0.0 is nearly released, so we should actually rename all these deprecation notices to say 22.0.0.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2195224148


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +48,39 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  /// \brief Retrieve a key.
+  virtual ::arrow::util::SecureString GetKey(const std::string& key_id) = 0;
+
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, ::arrow::util::SecureString key);
+
+  ::arrow::util::SecureString GetKey(const std::string& key_id_string) override {
+    // key_id_string is string but for IntegerKeyIdRetriever it encodes

Review Comment:
   Fair enough. I do find the implementation weird as well, and I'm not sure this class is actually useful, but hey :)



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2194937368


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +48,39 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  /// \brief Retrieve a key.
+  virtual ::arrow::util::SecureString GetKey(const std::string& key_id) = 0;
+
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, ::arrow::util::SecureString key);
+
+  ::arrow::util::SecureString GetKey(const std::string& key_id_string) override {
+    // key_id_string is string but for IntegerKeyIdRetriever it encodes

Review Comment:
   @bkietz suggested the implementation given the signature is somewhat surprising: https://github.com/apache/arrow/pull/46017#discussion_r2029146076



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039495362


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   What about
   ```c++
     volatile char *p = data;
     while (size--) *p++ = 0;
   ```
   https://cplusplus.com/articles/ETqpX9L8/



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2182839492


##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -148,9 +150,9 @@ InternalFileDecryptor::GetColumnDecryptorFactory(
   // The column is encrypted with its own key
   const std::string& column_key_metadata = crypto_metadata->key_metadata();
   const std::string column_path = crypto_metadata->path_in_schema()->ToDotString();
-  std::string column_key = GetColumnKey(column_path, column_key_metadata);
+  const SecureString& column_key = GetColumnKey(column_path, column_key_metadata);
 
-  return [this, aad, metadata, column_key = std::move(column_key)]() {
+  return [this, aad, metadata, column_key = column_key]() {

Review Comment:
   It's more of a question from me: is `column_key` being captured by value here? I'm not enough of a C++ lawyer :)



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3046352212

   @pitrou I have removed the deprecated methods to "encourage" users to migrate to the new methods: [290449a](https://github.com/apache/arrow/pull/46017/commits/290449a36c4b45fc5e6be931a6a12b378d80cdf4)
   
   But this will require migrating the Python API as well:
   https://github.com/apache/arrow/actions/runs/16126071702/job/45503157743?pr=46017#step:6:3956
   
   I would like to move touching the Python API to a separate PR, which would contain the revert [290449a](https://github.com/apache/arrow/pull/46017/commits/290449a36c4b45fc5e6be931a6a12b378d80cdf4).


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2031430056


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {

Review Comment:
   Would you please move this to `arrow/util/`? I think it will be useful [outside parquet](https://github.com/apache/arrow/pull/41559)



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["kapoisu (via GitHub)" <gi...@apache.org>]

kapoisu commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3027049211

   > @kapoisu since you have touched `encryption.h|cc` recently, please have a look if my changes respect your code style introduced in #46202.
   
   I generally tend to return by value for getters to be safe. It reduces the possibility of the object being modified externally and ensures the return value won't dangle even if the original object doesn't outlive it. That said, if the return value is large and the getter is called frequently, returning a reference may be reasonable. Either way, this is just a small trade-off that depends on the use cases.
   
   Defining no_key_ as ```const``` unnecessarily prevents the object from being copy-assigned. If the object doesn't allocate heap memory, I usually just return an empty instance instead, or make it ```constexpr static```.
   
   Other than that, LGTM — thank you for your hard work! :)


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3027108202

   I agree with returning by value, I don't think it's a problem that it's being copied.


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2179597718


##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -148,9 +150,9 @@ InternalFileDecryptor::GetColumnDecryptorFactory(
   // The column is encrypted with its own key
   const std::string& column_key_metadata = crypto_metadata->key_metadata();
   const std::string column_path = crypto_metadata->path_in_schema()->ToDotString();
-  std::string column_key = GetColumnKey(column_path, column_key_metadata);
+  const SecureString& column_key = GetColumnKey(column_path, column_key_metadata);
 
-  return [this, aad, metadata, column_key = std::move(column_key)]() {
+  return [this, aad, metadata, column_key = column_key]() {

Review Comment:
   Hmm, we must capture the column key by value here. It's not obvious that it's what `column_key = column_key` does.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2179573785


##########
cpp/src/parquet/encryption/kms_client.h:
##########
@@ -79,14 +80,47 @@ class PARQUET_EXPORT KmsClient {
   static constexpr const char kKmsInstanceUrlDefault[] = "DEFAULT";
   static constexpr const char kKeyAccessTokenDefault[] = "DEFAULT";
 
-  /// Wraps a key - encrypts it with the master key, encodes the result
+  /// \brief Wraps a key.
+  ///
+  /// Encrypts it with the master key, encodes the result
   /// and potentially adds a KMS-specific metadata.
+  ///
+  /// \deprecated Deprecated since 21.0.0. Implement
+  ///             WrapKey(const SecureString&, const std::string&) instead.
+  ARROW_DEPRECATED(
+      "Deprecated in 21.0.0. "
+      "Implement WrapKey(const SecureString&, const std::string&) instead.")
   virtual std::string WrapKey(const std::string& key_bytes,
-                              const std::string& master_key_identifier) = 0;
+                              const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc WrapKey(const std::string&, const std::string&)
+  virtual std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
+                              const std::string& master_key_identifier) {
+    ARROW_SUPPRESS_DEPRECATION_WARNING
+    auto key = WrapKey(std::string(key_bytes.as_view()), master_key_identifier);
+    ARROW_UNSUPPRESS_DEPRECATION_WARNING
+    return key;
+  }
 
-  /// Decrypts (unwraps) a key with the master key.
+  /// \brief Decrypts (unwraps) a key with the master key.
+  /// \deprecated Deprecated since 21.0.0. Implement UnWrapKey instead.
+  ARROW_DEPRECATED("Deprecated in 21.0.0. Implement UnWrapKey instead.")

Review Comment:
   Perhaps be more specific about the difference (i.e. it's not a name change). For example "Implement SecureString-returning UnWrapKey instead".



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2179569730


##########
cpp/src/parquet/encryption/local_wrap_kms_client.h:
##########
@@ -35,16 +35,16 @@ class PARQUET_EXPORT LocalWrapKmsClient : public KmsClient {
 
   explicit LocalWrapKmsClient(const KmsConnectionConfig& kms_connection_config);
 
-  std::string WrapKey(const std::string& key_bytes,
+  std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
                       const std::string& master_key_identifier) override;
 
-  std::string UnwrapKey(const std::string& wrapped_key,
-                        const std::string& master_key_identifier) override;
+  ::arrow::util::SecureString UnWrapKey(

Review Comment:
   Ah, sorry, I was reading the diff backwards. I understand the rationale now.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3025489974

   @pitrou yes, I do. I think I have addressed all comments. I have deprecated the string-based methods. What do you think about the new method names?


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3025482817

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039530287


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,70 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+#include "parquet/platform.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class PARQUET_EXPORT SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;
+
+  SecureString& operator=(SecureString&&) noexcept;
+  SecureString& operator=(const SecureString&) noexcept;

Review Comment:
   done



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2077548902


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   > How would I do that, the memory where `SecureString.as_span` points to will be freed on deconstruction. How can I access that memory securely?
   
   Depends what you call "securely", but in most cases (unless running under Valgrind or Address Sanitizer), you can still access a deallocated area. We should just try to do that here.
   
   (this is morally the same as with the old area comment, by the way)



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou merged PR #46017:
URL: https://github.com/apache/arrow/pull/46017


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2194937368


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +48,39 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  /// \brief Retrieve a key.
+  virtual ::arrow::util::SecureString GetKey(const std::string& key_id) = 0;
+
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, ::arrow::util::SecureString key);
+
+  ::arrow::util::SecureString GetKey(const std::string& key_id_string) override {
+    // key_id_string is string but for IntegerKeyIdRetriever it encodes

Review Comment:
   @bkietz suggested this given the implementation given the signature is somewhat surprising: https://github.com/apache/arrow/pull/46017#discussion_r2029146076



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2029208776


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   Oh, alternatively: since every STL I'm aware of implements std::string with an inline short string case, we could use:
   
   ```c++
     // Most std::string impls store their contents inline, so we can check
     // the object representation for a crib to ensure that destruction does
     // wipe all sensitive bytes.
   
     constexpr auto kCrib = "01CRIB01";
     char object_repr[sizeof(SecureString)];
     std::string_view object_repr_view{object_repr, sizeof(object_repr)};
   
     SecureString* cribbed = new (&object_repr) SecureString(std::string(kCrib));
     bool crib_found_before = object_repr_view.find(kCrib) != object_repr_view.npos;
     cribbed->~SecureString(); // NOTE: do not allow this call to be skipped!
     bool crib_found_after = object_repr_view.find(kCrib) != object_repr_view.npos;
   
     if (crib_found_before) {
       ASSERT_FALSE(crib_found_after);
     }
   ```
   
   I think that's well-defined: https://godbolt.org/z/ovxs58e7h



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2775252422

   For the record, OpenSSL allows allocating memory from a [secure heap](https://docs.openssl.org/3.2/man3/OPENSSL_secure_malloc/), but that requires calling process-wide initialization functions. I've [asked about this](https://groups.google.com/a/openssl.org/g/openssl-users/c/pBlfsho-W_0).


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2775797532

   I agree, that `OPENSSL_secure_malloc` seems problematic. What if the application crashes (or receives kill -9) and `CRYPTO_secure_malloc_done` cannot be called. Are secrets then not protected? If secrets are protected without `CRYPTO_secure_malloc_done`, and multiple calls to `OPENSSL_secure_malloc` are idempotent, then this would be a good way to go.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2029065921


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }
+
+  // long string
+  {
+    std::string large(1024, 'x');
+    large.resize(1024, 'y');
+    SecureString::secure_clear(large);
+    assert_securely_cleared(large);
+  }
+
+  // empty string
+  {
+    // this creates an empty string with some non-zero characters in the string buffer
+    // we test that all those characters are securely cleared
+    std::string empty("abcdef");
+    empty.resize(0);
+    SecureString::secure_clear(empty);
+    assert_securely_cleared(empty);
+  }
+}
+
+TEST(TestSecureString, Construct) {
+  // move constructing from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string(std::move(string));
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move constructing from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret(std::move(secret_from_string));
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // copy constructing from a secure string does not modify that secure string
+  SecureString secret_from_secret(secret_from_move_secret);
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+
+TEST(TestSecureString, Assign) {
+  // move assigning from a string securely clears that string
+  std::string string("hello world");
+  SecureString secret_from_string;
+  secret_from_string = std::move(string);
+  assert_securely_cleared(string);
+  ASSERT_FALSE(secret_from_string.empty());
+
+  // move assigning from a secure string securely clears that secure string
+  // Note: there is no way to test the secure clearing of the moved secure string
+  SecureString secret_from_move_secret;
+  secret_from_move_secret = std::move(secret_from_string);
+  ASSERT_TRUE(secret_from_string.empty());
+  ASSERT_FALSE(secret_from_move_secret.empty());
+
+  // assigning from a secure string does not modify that secure string
+  SecureString secret_from_secret;
+  secret_from_secret = secret_from_move_secret;
+  ASSERT_FALSE(secret_from_move_secret.empty());
+  ASSERT_FALSE(secret_from_secret.empty());
+  ASSERT_EQ(secret_from_secret, secret_from_move_secret);
+}
+

Review Comment:
   How would I do that, the memory where `SecureString.as_span` points to will be freed on deconstruction. How can I access that memory securely?



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["bkietz (via GitHub)" <gi...@apache.org>]

bkietz commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2078224319


##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -182,79 +175,75 @@ FileEncryptionProperties::Builder::disable_aad_prefix_storage() {
 }
 
 ColumnEncryptionProperties::ColumnEncryptionProperties(bool encrypted,
-                                                       const std::string& column_path,
-                                                       const std::string& key,
-                                                       const std::string& key_metadata)
-    : column_path_(column_path) {
-  DCHECK(!column_path.empty());
+                                                       std::string column_path,
+                                                       encryption::SecureString key,
+                                                       std::string key_metadata)
+    : column_path_(std::move(column_path)),

Review Comment:
   Member initializer order is asserted to match member declaration order in a lint pass. (Member initialization proceeds in declaration order regardless of member initializer order.) If the member declarations and initializers were both reordered, then
   ```c++
   key_(std::move(key)),
   encrypted_with_footer_key_(encrypted && key.empty()),
   ```
   is use-after-move which should fail a separate lint check. Furthermore, that could be fixed by writing
   ```c++
   key_(std::move(key)),
   encrypted_with_footer_key_(encrypted && key_.empty()),
   ```
   
   So I don't think it's necessary to revert.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2037689716


##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -46,7 +46,7 @@ int32_t Decryptor::CiphertextLength(int32_t plaintext_len) const {
 
 int32_t Decryptor::Decrypt(::arrow::util::span<const uint8_t> ciphertext,
                            ::arrow::util::span<uint8_t> plaintext) {
-  return aes_decryptor_->Decrypt(ciphertext, str2span(key_), str2span(aad_), plaintext);
+  return aes_decryptor_->Decrypt(ciphertext, key_.as_span(), str2span(aad_), plaintext);

Review Comment:
   It is still used to convert `aad` and `encrypted_key` strings to spans.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2037689716


##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -46,7 +46,7 @@ int32_t Decryptor::CiphertextLength(int32_t plaintext_len) const {
 
 int32_t Decryptor::Decrypt(::arrow::util::span<const uint8_t> ciphertext,
                            ::arrow::util::span<uint8_t> plaintext) {
-  return aes_decryptor_->Decrypt(ciphertext, str2span(key_), str2span(aad_), plaintext);
+  return aes_decryptor_->Decrypt(ciphertext, key_.as_span(), str2span(aad_), plaintext);

Review Comment:
   It is still being used to convert `aad` and `encrypted_key` strings to spans.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2038094558


##########
cpp/src/parquet/encryption/file_key_wrapper.cc:
##########
@@ -113,14 +113,15 @@ KeyEncryptionKey FileKeyWrapper::CreateKeyEncryptionKey(
     const std::string& master_key_id) {
   std::string kek_bytes(kKeyEncryptionKeyLength, '\0');
   RandBytes(reinterpret_cast<uint8_t*>(kek_bytes.data()), kKeyEncryptionKeyLength);
+  SecureString secure_kek_bytes(std::move(kek_bytes));

Review Comment:
   done



##########
cpp/src/parquet/encryption/file_key_wrapper.cc:
##########
@@ -113,14 +113,15 @@ KeyEncryptionKey FileKeyWrapper::CreateKeyEncryptionKey(
     const std::string& master_key_id) {
   std::string kek_bytes(kKeyEncryptionKeyLength, '\0');
   RandBytes(reinterpret_cast<uint8_t*>(kek_bytes.data()), kKeyEncryptionKeyLength);
+  SecureString secure_kek_bytes(std::move(kek_bytes));

Review Comment:
   done



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2775132521

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2775783306

   > @EnricoMi Do you want a preliminary review on this?
   
   Yes, please! This is in a complete PoC state (Linux build and test is green).
   
   Is this the right direction before I roll this out to Python?
   Can I break the API or should I deprecate exiting methods and provide both, std::string and secure string methods?
   Do you have some hints how I can fix the link issues for Windows? I presume I need to add the `secure_string.cc` file somewhere so Windows links to the object file.
   Shouldn't it be sufficient to only use `OPENSSL_cleanse` and count on OpenSSL providing OS-dependent implementations?


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2039317427


##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -130,11 +123,11 @@ FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::aad_prefix
 }
 
 ColumnDecryptionProperties::Builder* ColumnDecryptionProperties::Builder::key(
-    const std::string& key) {
+    encryption::SecureString key) {
   if (key.empty()) return this;
 
-  DCHECK(!key.empty());
-  key_ = key;
+  DCHECK(key_.empty());

Review Comment:
   This looked suspicious, all other setter methods check if the member is unset. Here, the check is always true as it checks the input.
   
   This is a breaking change if user code calls this setter twice with non-empty keys.



##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -130,11 +123,11 @@ FileDecryptionProperties::Builder* FileDecryptionProperties::Builder::aad_prefix
 }
 
 ColumnDecryptionProperties::Builder* ColumnDecryptionProperties::Builder::key(
-    const std::string& key) {
+    encryption::SecureString key) {
   if (key.empty()) return this;
 
-  DCHECK(!key.empty());
-  key_ = key;
+  DCHECK(key_.empty());

Review Comment:
   This looked suspicious, all other setter methods check if the member is unset.
   Here, the check is always true as it checks the input.
   
   This is a breaking change if user code calls this setter twice with non-empty keys.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2179571971


##########
cpp/src/parquet/encryption/kms_client.h:
##########
@@ -79,14 +80,47 @@ class PARQUET_EXPORT KmsClient {
   static constexpr const char kKmsInstanceUrlDefault[] = "DEFAULT";
   static constexpr const char kKeyAccessTokenDefault[] = "DEFAULT";
 
-  /// Wraps a key - encrypts it with the master key, encodes the result
+  /// \brief Wraps a key.
+  ///
+  /// Encrypts it with the master key, encodes the result
   /// and potentially adds a KMS-specific metadata.
+  ///
+  /// \deprecated Deprecated since 21.0.0. Implement
+  ///             WrapKey(const SecureString&, const std::string&) instead.
+  ARROW_DEPRECATED(
+      "Deprecated in 21.0.0. "
+      "Implement WrapKey(const SecureString&, const std::string&) instead.")
   virtual std::string WrapKey(const std::string& key_bytes,
-                              const std::string& master_key_identifier) = 0;
+                              const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc WrapKey(const std::string&, const std::string&)
+  virtual std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
+                              const std::string& master_key_identifier) {
+    ARROW_SUPPRESS_DEPRECATION_WARNING
+    auto key = WrapKey(std::string(key_bytes.as_view()), master_key_identifier);
+    ARROW_UNSUPPRESS_DEPRECATION_WARNING
+    return key;
+  }
 
-  /// Decrypts (unwraps) a key with the master key.
+  /// \brief Decrypts (unwraps) a key with the master key.
+  /// \deprecated Deprecated since 21.0.0. Implement UnWrapKey instead.
+  ARROW_DEPRECATED("Deprecated in 21.0.0. Implement UnWrapKey instead.")
   virtual std::string UnwrapKey(const std::string& wrapped_key,
-                                const std::string& master_key_identifier) = 0;
+                                const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc UnwrapKey(const std::string&, const std::string&)

Review Comment:
   This might also copy the deprecation mention, no? The docstring is simple-enough that it can be copy-pasted, IMHO.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2182810826


##########
cpp/src/parquet/encryption/kms_client.h:
##########
@@ -79,14 +80,47 @@ class PARQUET_EXPORT KmsClient {
   static constexpr const char kKmsInstanceUrlDefault[] = "DEFAULT";
   static constexpr const char kKeyAccessTokenDefault[] = "DEFAULT";
 
-  /// Wraps a key - encrypts it with the master key, encodes the result
+  /// \brief Wraps a key.
+  ///
+  /// Encrypts it with the master key, encodes the result
   /// and potentially adds a KMS-specific metadata.
+  ///
+  /// \deprecated Deprecated since 21.0.0. Implement
+  ///             WrapKey(const SecureString&, const std::string&) instead.
+  ARROW_DEPRECATED(
+      "Deprecated in 21.0.0. "
+      "Implement WrapKey(const SecureString&, const std::string&) instead.")
   virtual std::string WrapKey(const std::string& key_bytes,
-                              const std::string& master_key_identifier) = 0;
+                              const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc WrapKey(const std::string&, const std::string&)
+  virtual std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
+                              const std::string& master_key_identifier) {
+    ARROW_SUPPRESS_DEPRECATION_WARNING
+    auto key = WrapKey(std::string(key_bytes.as_view()), master_key_identifier);
+    ARROW_UNSUPPRESS_DEPRECATION_WARNING
+    return key;

Review Comment:
   So you prefer to force user code to be migrated in order to compile against the new Arrow version? Happy to do that, simplifies my code changes.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2182908014


##########
cpp/src/parquet/encryption/internal_file_decryptor.cc:
##########
@@ -148,9 +150,9 @@ InternalFileDecryptor::GetColumnDecryptorFactory(
   // The column is encrypted with its own key
   const std::string& column_key_metadata = crypto_metadata->key_metadata();
   const std::string column_path = crypto_metadata->path_in_schema()->ToDotString();
-  std::string column_key = GetColumnKey(column_path, column_key_metadata);
+  const SecureString& column_key = GetColumnKey(column_path, column_key_metadata);
 
-  return [this, aad, metadata, column_key = std::move(column_key)]() {
+  return [this, aad, metadata, column_key = column_key]() {

Review Comment:
   Ok, I found the answer: it does a copy, so this is fine.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2194955749


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -310,18 +325,22 @@ class PARQUET_EXPORT FileDecryptionProperties {
   }
 
  private:
-  std::string footer_key_;
+  ::arrow::util::SecureString footer_key_;
   std::string aad_prefix_;
   std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;
 
+  // any empty SecureString key is interpreted as if no key is given
+  // this instance is used if a SecureString reference is returned
+  const ::arrow::util::SecureString no_key_ = ::arrow::util::SecureString();

Review Comment:
   Done in 184afcfd6d.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2179578572


##########
cpp/src/parquet/encryption/kms_client.h:
##########
@@ -79,14 +80,47 @@ class PARQUET_EXPORT KmsClient {
   static constexpr const char kKmsInstanceUrlDefault[] = "DEFAULT";
   static constexpr const char kKeyAccessTokenDefault[] = "DEFAULT";
 
-  /// Wraps a key - encrypts it with the master key, encodes the result
+  /// \brief Wraps a key.
+  ///
+  /// Encrypts it with the master key, encodes the result
   /// and potentially adds a KMS-specific metadata.
+  ///
+  /// \deprecated Deprecated since 21.0.0. Implement
+  ///             WrapKey(const SecureString&, const std::string&) instead.
+  ARROW_DEPRECATED(
+      "Deprecated in 21.0.0. "
+      "Implement WrapKey(const SecureString&, const std::string&) instead.")
   virtual std::string WrapKey(const std::string& key_bytes,
-                              const std::string& master_key_identifier) = 0;
+                              const std::string& master_key_identifier) {
+    throw ParquetException("Not implemented");
+  }
+
+  /// \copydoc WrapKey(const std::string&, const std::string&)
+  virtual std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
+                              const std::string& master_key_identifier) {
+    ARROW_SUPPRESS_DEPRECATION_WARNING
+    auto key = WrapKey(std::string(key_bytes.as_view()), master_key_identifier);
+    ARROW_UNSUPPRESS_DEPRECATION_WARNING
+    return key;

Review Comment:
   Is this default implementation provided so that old KmsClient implementations that don't override it can still compile?
   
   It doesn't sound like a terrific idea to me. We want people to implement the new method, IMHO. It will break compatibility slightly, but for the better.



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2179567415


##########
cpp/src/parquet/encryption/local_wrap_kms_client.h:
##########
@@ -35,16 +35,16 @@ class PARQUET_EXPORT LocalWrapKmsClient : public KmsClient {
 
   explicit LocalWrapKmsClient(const KmsConnectionConfig& kms_connection_config);
 
-  std::string WrapKey(const std::string& key_bytes,
+  std::string WrapKey(const ::arrow::util::SecureString& key_bytes,
                       const std::string& master_key_identifier) override;
 
-  std::string UnwrapKey(const std::string& wrapped_key,
-                        const std::string& master_key_identifier) override;
+  ::arrow::util::SecureString UnWrapKey(

Review Comment:
   Isn't it a bit gratuitous to change the method name casing from `UnwrapKey` to `UnWrapKey`?



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2111411746


##########
cpp/src/parquet/encryption/secure_string.cc:
##########
@@ -0,0 +1,111 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "parquet/encryption/secure_string.h"
+
+#include <openssl/crypto.h>
+#include <openssl/opensslv.h>
+#include <utility>
+#if defined(_WIN32)
+#  include <windows.h>
+#endif
+
+#include "arrow/util/span.h"
+#include "parquet/encryption/encryption.h"
+
+namespace parquet::encryption {
+SecureString::SecureString(SecureString&& secret) noexcept
+    : secret_(std::move(secret.secret_)) {}
+SecureString::SecureString(std::string&& secret) noexcept : secret_(std::move(secret)) {
+  SecureClear(&secret);
+}
+
+SecureString& SecureString::operator=(SecureString&& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = std::move(secret.secret_);
+  return *this;
+}
+SecureString& SecureString::operator=(const SecureString& secret) noexcept {
+  if (this == &secret) {
+    // self-assignment
+    return *this;
+  }
+  Dispose();
+  secret_ = secret.secret_;
+  return *this;
+}
+SecureString& SecureString::operator=(std::string&& secret) noexcept {
+  Dispose();
+  secret_ = std::move(secret);
+  SecureClear(&secret);
+  return *this;
+}
+
+bool SecureString::operator==(const SecureString& other) const {
+  return secret_ == other.secret_;
+}
+
+bool SecureString::operator!=(const SecureString& other) const {
+  return secret_ != other.secret_;
+}
+
+bool SecureString::empty() const { return secret_.empty(); }
+std::size_t SecureString::size() const { return secret_.size(); }
+std::size_t SecureString::length() const { return secret_.length(); }
+
+::arrow::util::span<uint8_t> SecureString::as_span() {
+  return {reinterpret_cast<uint8_t*>(secret_.data()), secret_.size()};
+}
+::arrow::util::span<const uint8_t> SecureString::as_span() const {
+  return {reinterpret_cast<const uint8_t*>(secret_.data()), secret_.size()};
+}
+std::string_view SecureString::as_view() const {
+  return {secret_.data(), secret_.size()};
+}
+
+void SecureString::Dispose() { SecureClear(&secret_); }
+void SecureString::SecureClear(std::string* secret) {
+  secret->clear();
+  SecureClear(reinterpret_cast<uint8_t*>(secret->data()), secret->capacity());
+}
+inline void SecureString::SecureClear(uint8_t* data, size_t size) {
+  // Heavily borrowed from libb2's `secure_zero_memory` at
+  // https://github.com/BLAKE2/libb2/blob/master/src/blake2-impl.h
+#if defined(_WIN32)
+  SecureZeroMemory(data, size);
+#elif defined(__STDC_LIB_EXT1__)
+  // memset_s is meant to not be optimized away
+  memset_s(data, size, 0, size);
+#elif defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30000000
+  OPENSSL_cleanse(data, size);
+#elif defined(__GLIBC__) && (__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 25))
+  // glibc 2.25+ has explicit_bzero
+  explicit_bzero(data, size);
+#else
+  // Try to ensure that a true library call to memset() will be generated
+  // by the compiler.
+  static const volatile auto memset_v = &memset;
+  memset_v(data, 0, size);
+  __asm__ __volatile__("" ::"r"(data) : "memory");

Review Comment:
   @pitrou @bkietz
   
   I have added more comments and links to prior art. I have also wrapped the `__asm__` to enable it only for GCC and Clang: https://github.com/apache/arrow/pull/46017/commits/510349c170834e348305a70d51face81a5709196
   
   We have two options for the fallback branch:
   1. the current `memset` implementation
   2. the `while` loop below
   
   Both benefit from the `__asm__ __volatile__` call.
   
   ```c++
     // Volatile pointer to the data is an attempt to avoid
     // that the compiler optimizes away the while loop.
     // https://cplusplus.com/articles/ETqpX9L8/
     // http://stackoverflow.com/a/13299459/965097
     // https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/
     volatile char *p = data;
     while (size--) *p++ = 0;
   ```



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["wgtmac (via GitHub)" <gi...@apache.org>]

wgtmac commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2195374415


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -46,28 +48,39 @@ using ColumnPathToEncryptionPropertiesMap =
 
 class PARQUET_EXPORT DecryptionKeyRetriever {
  public:
-  virtual std::string GetKey(const std::string& key_metadata) = 0;
+  /// \brief Retrieve a key.
+  virtual ::arrow::util::SecureString GetKey(const std::string& key_id) = 0;
+
   virtual ~DecryptionKeyRetriever() {}
 };
 
 /// Simple integer key retriever
 class PARQUET_EXPORT IntegerKeyIdRetriever : public DecryptionKeyRetriever {
  public:
-  void PutKey(uint32_t key_id, const std::string& key);
-  std::string GetKey(const std::string& key_metadata) override;
+  void PutKey(uint32_t key_id, ::arrow::util::SecureString key);
+
+  ::arrow::util::SecureString GetKey(const std::string& key_id_string) override {
+    // key_id_string is string but for IntegerKeyIdRetriever it encodes

Review Comment:
   It is indeed surprising...



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2195222379


##########
cpp/src/parquet/encryption/encryption.cc:
##########
@@ -26,31 +26,30 @@
 #include "arrow/util/utf8.h"
 #include "parquet/encryption/encryption_internal.h"
 
-namespace parquet {
+using ::arrow::util::SecureString;
 
-// integer key retriever
-void IntegerKeyIdRetriever::PutKey(uint32_t key_id, const std::string& key) {
-  key_map_.insert({key_id, key});
-}
+namespace parquet {
 
-std::string IntegerKeyIdRetriever::GetKey(const std::string& key_metadata) {
-  uint32_t key_id;
-  memcpy(reinterpret_cast<uint8_t*>(&key_id), key_metadata.c_str(), 4);
+// any empty SecureString key is interpreted as if no key is given
+// this instance is used when a SecureString reference is returned
+const SecureString no_key_ = SecureString();

Review Comment:
   The naming is a bit off as the trailing underscore is used for member variables. You could make it `kNoKey` for example.
   (can also make it `static` by the way)



-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3052565463

   Maybe @bkietz and @wgtmac want to make a final pass over the PR.


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3048599013

   > [](https://github.com/apache/arrow/pull/46017/commits/290449a36c4b45fc5e6be931a6a12b378d80cdf4)
   > 
   > But this will require migrating the Python API as well: https://github.com/apache/arrow/actions/runs/16126071702/job/45503157743?pr=46017#step:6:3956
   > 
   > I would like to move touching the Python API to a separate PR, which would contain the revert [290449a](https://github.com/apache/arrow/pull/46017/commits/290449a36c4b45fc5e6be931a6a12b378d80cdf4).
   
   Ok, that sounds good to me.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2077546384


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }

Review Comment:
   Sorry @EnricoMi , I realize I forgot to reply to this comment:
   
   > Just in case pointers change? If they would change, then `old_area` would point to a freed memory area, right?
   
   Right, but the freed memory area was not necessarily cleaned up before being freed (unless the allocator was built in debug mode perhaps). That's why it's important to check that the old area doesn't contain the sensitive secrets anymore.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2031154172


##########
cpp/src/parquet/encryption/key_toolkit_internal.cc:
##########
@@ -59,10 +59,10 @@ std::string DecryptKeyLocally(const std::string& encoded_encrypted_key,
   ::arrow::util::span<uint8_t> decrypted_key_span(
       reinterpret_cast<uint8_t*>(&decrypted_key[0]), decrypted_key_len);

Review Comment:
   good idea, done!



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2028829108


##########
cpp/src/parquet/encryption/encryption.h:
##########
@@ -327,19 +322,19 @@ class PARQUET_EXPORT FileDecryptionProperties {
   }
 
  private:
-  std::string footer_key_;
+  encryption::SecureString footer_key_;
   std::string aad_prefix_;
   std::shared_ptr<AADPrefixVerifier> aad_prefix_verifier_;
 
-  const std::string empty_string_ = "";
+  const encryption::SecureString empty_string_ = encryption::SecureString("");

Review Comment:
   It is, method `FileDecryptionProperties::column_key` returns an empty string as the encryption key when the key id is not found. It should rather throw a meaningful `key not found` exception. I may fix that while I am touching this code anyway.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2029061547


##########
cpp/src/parquet/encryption/secure_string_test.cc:
##########
@@ -0,0 +1,106 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+#include <vector>
+
+#include "parquet/encryption/secure_string.h"
+
+namespace parquet::encryption::test {
+
+void assert_securely_cleared(const std::string& string) {
+  // the entire buffer of the string is filled with zeros
+  std::vector<char> zeros(string.capacity());
+  ::arrow::util::span actual(string.data(), string.capacity());
+  ::arrow::util::span expected(zeros.data(), zeros.size());
+  ASSERT_EQ(actual, expected);
+
+  // the string is empty
+  ASSERT_TRUE(string.empty());
+}
+
+TEST(TestSecureString, SecureClearString) {
+  // short string
+  {
+    std::string tiny("abc");
+    SecureString::secure_clear(tiny);
+    assert_securely_cleared(tiny);
+  }

Review Comment:
   But `tiny.data()` and `old_area.data()` point to the same address, what is the point testing `old_area`? Just in case pointers change? If they would change, then `old_area` would point to a freed memory area, right?



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["EnricoMi (via GitHub)" <gi...@apache.org>]

EnricoMi commented on code in PR #46017:
URL: https://github.com/apache/arrow/pull/46017#discussion_r2028977729


##########
cpp/src/parquet/encryption/secure_string.h:
##########
@@ -0,0 +1,66 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+
+#include "arrow/util/span.h"
+
+namespace parquet::encryption {
+/**
+ * A secure string that ensures the wrapped string is cleared from memory on
+ * deconstruction. This class can only be created from std::string that are securely
+ * erased after creation.
+ *
+ * Note: This class does not provide a constructor / assignment operator that copies a
+ * std::string because that would allow code to create a SecureString while accidentally
+ * not noticing the need to securely erasing the argument after invoking the constructor /
+ * calling the assignment operator.
+ */
+class SecureString {
+ public:
+  SecureString() noexcept = default;
+  SecureString(SecureString&&) noexcept;
+  SecureString(const SecureString&) noexcept = default;
+  explicit SecureString(std::string&&) noexcept;
+
+  SecureString& operator=(SecureString&&) noexcept;
+  SecureString& operator=(const SecureString&) noexcept;
+  SecureString& operator=(std::string&& secret) noexcept;
+
+  bool operator==(const SecureString&) const;
+  bool operator!=(const SecureString&) const;
+
+  ~SecureString() { dispose(); }
+
+  [[nodiscard]] bool empty() const;
+  [[nodiscard]] std::size_t size() const;
+  [[nodiscard]] std::size_t length() const;
+  [[nodiscard]] ::arrow::util::span<const uint8_t> as_span() const;

Review Comment:
   Added, made `as_view()` const.



-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2796866618

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++][Python] Wrap encryption keys in secure string [arrow]

["github-actions[bot] (via GitHub)" <gi...@apache.org>]

github-actions[bot] commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-2796630770

   :warning: GitHub issue #31603 **has been automatically assigned in GitHub** to PR creator.


-- 
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] GH-31603: [C++] Wrap Parquet encryption keys in SecureString [arrow]

["pitrou (via GitHub)" <gi...@apache.org>]

pitrou commented on PR #46017:
URL: https://github.com/apache/arrow/pull/46017#issuecomment-3024361423

   @EnricoMi Are you planning to update this PR?


-- 
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