Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for getting object checksums in GetObject #1123

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Add support for getting object checksums in GetObject #1123

merged 3 commits into from
Nov 11, 2024

Conversation

muddyfish
Copy link
Contributor

Description of change

  • Adds new method get_object_checksum to get_object to retrieve the whole object checksum.
  • Adds new parameter checksum_mode to GetObjectParams to configure if we want to request checksums from S3.
    • If checksums aren't requested, get_object_checksum returns an error.
  • Refactor S3CrtClient to store a cell of the object headers rather than metadata.

Relevant issues:

Does this change impact existing behavior?

No

Does this change need a changelog entry in any of the crates?

Yes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@muddyfish muddyfish requested review from passaro and vladem November 11, 2024 11:27
@muddyfish muddyfish had a problem deploying to PR integration tests November 11, 2024 11:27 — with GitHub Actions Failure
@muddyfish muddyfish had a problem deploying to PR integration tests November 11, 2024 11:27 — with GitHub Actions Failure
@muddyfish muddyfish had a problem deploying to PR integration tests November 11, 2024 11:27 — with GitHub Actions Failure
@muddyfish muddyfish had a problem deploying to PR integration tests November 11, 2024 11:27 — with GitHub Actions Failure
@muddyfish muddyfish had a problem deploying to PR integration tests November 11, 2024 11:27 — with GitHub Actions Failure
@muddyfish muddyfish had a problem deploying to PR integration tests November 11, 2024 11:27 — with GitHub Actions Failure
@muddyfish muddyfish had a problem deploying to PR integration tests November 11, 2024 11:27 — with GitHub Actions Failure
@muddyfish
Add changelog
d322d2f 
Signed-off-by: Simon Beal <[email protected]>
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 12:03 — with GitHub Actions Inactive
@muddyfish muddyfish had a problem deploying to PR integration tests November 11, 2024 12:03 — with GitHub Actions Failure
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 12:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 12:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 12:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 12:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 12:03 — with GitHub Actions Inactive
passaro
passaro reviewed Nov 11, 2024
View reviewed changes
Copy link
Contributor

@passaro passaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Only a couple of minor suggestions.

mountpoint-s3-client/src/s3_crt_client/get_object.rs Outdated Show resolved Hide resolved
mountpoint-s3-client/tests/get_object.rs Outdated Show resolved Hide resolved
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 13:33 — with GitHub Actions Inactive
@muddyfish
PR Feedback
3a73814 
Signed-off-by: Simon Beal <[email protected]>
@muddyfish muddyfish requested a review from passaro November 11, 2024 15:03
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 15:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 15:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 15:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 15:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 15:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 15:03 — with GitHub Actions Inactive
@muddyfish muddyfish temporarily deployed to PR integration tests November 11, 2024 15:03 — with GitHub Actions Inactive
@muddyfish muddyfish added this pull request to the merge queue Nov 11, 2024
Merged via the queue into awslabs:main with commit e544f8f Nov 11, 2024
23 checks passed
@muddyfish muddyfish deleted the express-cache-crc32 branch November 11, 2024 15:42
@passaro passaro mentioned this pull request Nov 27, 2024
Merged
ctso pushed a commit to unionai-oss/mountpoint-s3 that referenced this pull request Dec 3, 2024
@passaro
Update CRT libraries and set operation_name for DEFAULT meta-requests (…
a3c6f82 
…awslabs#935)

* Update CRT submodules to latest releases

Submodule mountpoint-s3-crt-sys/crt/aws-c-cal 96c47e3..11fc684:
  > Make AES GCM more consistent cross platform (awslabs#189)
  > Pin AWS-LC until it's fixed for manylinux1 (awslabs#188)
  > Implement runtime check on libcrypto linkage (awslabs#186)
  > clang-format 18 (awslabs#187)
Submodule mountpoint-s3-crt-sys/crt/aws-c-common 06cf4d8..6d974f9:
  > cbor support  (awslabs#1131)
  > Fix default thread options for windows to not pin to any cpu_id (awslabs#1126)
  > Use CBMC 6.0.0 (awslabs#1128)
  > latest_submodules.py uses AWS-LC-FIPS releases in aws-crt-java (awslabs#1125)
  > Use CBMC version 5.95.1 (awslabs#1124)
  > clang-format 18 (awslabs#1113)
  > disable optimization was not working (awslabs#1123)
  > Fix memtracer bad assumptions on the size of stack trace (awslabs#1122)
Submodule mountpoint-s3-crt-sys/crt/aws-c-s3 6588f9a..cb431ba:
  > test_helper.py improvements (awslabs#442)
  > Fix shutdown_callback or returning NULL contract for meta_request (awslabs#440)
  > BREAKING CHANGE: operation_name must be set for DEFAULT meta-requests (awslabs#439)
  > clang-format 18 (awslabs#438)
  > Auto - Update S3 Ruleset & Partition (awslabs#436)
Submodule mountpoint-s3-crt-sys/crt/aws-lc 92bf532..4368aaa:
  > Fix for loading JCA stripped private keys (#1658)
  > Prepare for release v1.30.1 (#1657)
  > Revert  `_CET_ENDBR` (#1656)
  > Close FD in Snapsafe test function (#1649)
  > Prepare for release v1.30.0 (#1646)
  > Snapsafe-type uniqueness breaking event detection (#1640)
  > Add EVP_md_null and SSL_set_ciphersuites (#1637)
  > Add de-randomized ML-KEM modes to experimental EVP API (#1578)
  > Patch for OpenVPN certificate setting behavioral difference (#1643)
  > Require newer assembler for _CET_ENDBR (#1641)
  > OpenVPN error codes, SSL_get_peer_signature_* funcs, and first patch file (#1584)
  > NIST.SP.800-56Cr2 One-Step Key Derivation (#1607)
  > Upstream merge 2024-06-13 (#1636)
  > More minor symbols for Ruby support (#1581)
  > Add support for NETSCAPE_SPKI_print (#1624)
  > align gcc version with curl's CI (#1633)
  > Fix spelling nits
  > Generated ASM files
  > Add Intel Indirect Branch Tracking support.
  > [EC] Unify point addition for P-256/384/521 (#1602)
  > Upstream merge 2024 06 03 (#1621)
  > Fix AES key size for AES256 in ABI test (#1629)
  > Move SSL_CIPHER_get_version test to SSLVersionTest.Version (#1631)
  > Use 'nasm' not 'yasm' (#1630)
  > Prepare for release 1.29.0 (#1626)
  > Implement SSL_CIPHER_get_version for recent TLS versions (#1627)
  > Add integration tests for OpenSSL-linking 3p modules (#1587)
  > Prevent non-constant-time code in Kyber-R3 and ML-KEM implementation (#1619)
  > Update ec2-test-framework to use gv2 (#1623)
  > Script for creating compilation database (#1617)
  > Fixes for building with `-pedantic` (#1608)
  > Fix SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR behavior (#1620)
  > Update for FIPS documentation (#1610)
  > Disable CI for gcc-14/FIPS until relocation issue is resolved (#1622)
  > Add support for ocsp get id (#1609)
  > Add libevent to GitHub integration CI (#1615)
  > Upstream merge 2024 05 17 (#1600)
  > add back ASN1_dup with tests (#1591)
  > Remove special aarch64 valgrind logic (#1618)
  > Fix NTP integ test (#1616)
  > Pin aws-lc-rs integ to nightly-2024-05-22 (#1612)
  > Cleanse the right amount of bytes in HMAC. (#1613)
  > add support for X509_CRL_http_nbio (#1596)
  > Add `all_fuzz_tests` build target (#1605)
  > Fix mariadb ssl_crl patch (#1606)
Submodule mountpoint-s3-crt-sys/crt/s2n-tls 6d92b46..073c7b4:
  > bug: Fixing bash error (#4624)
  > chore: make cbmc proof build more strict by adding -Werror flag (#4606)
  > Perform 2-RTT Handshake to upgrade to PQ when possible (#4526)
  > test(bindings/s2n-tls): refactor testing::s2n-tls tests (#4613)
  > docs: add timeout note to blinding delay docs (#4621)
  > docs: Add back suggested FIPS + TLS1.3 policy (#4605)
  > ci: shallow clone musl repo (#4611)
  > example(bindings): add async ConfigResolver (#4477)
  > chore: use CBMC version 5.95.1 (#4586)
  > s2n-tls rust binding: expose selected application protocol (#4599)
  > test: add pcap testing crate (#4604)
  > testing(bindings): add new test helper (#4596)
  > chore(bindings): fix shebang in generate.sh (#4603)
  > fix(s2n_session_ticket_test): correct clock mocking (#4602)
  > Fix: update default cert chain for unit tests (#4582)
  > refactor(binding): more accurate naming for const str helper (#4601)
  > fix: error rather than empty cipher suites (#4597)
  > chore: update s2n_stuffer_printf CBMC harness (#4531)
  > ci(nix): Fix integ pq test in a devShell (#4576)
  > feature: new compatibility-focused security policy preferring ECDSA (#4579)
  > compliance: update generate_report.sh to point to compliance directory (#4588)
  > ci: fix cppcheck errors (#4589)
  > chore: cleanup duplicate duvet citations (#4587)
  > Merge pull request from GHSA-52xf-5p2m-9wrv
  > chore(bindings): release 0.2.7 (#4580)
  > fix: Validate received signature algorithm in EVP verify (#4574)
  > refactor: add try_compile feature probe for RSA-PSS signing (#4569)
  > feat: Configurable blinding (#4562)
  > docs: document s2n_cert_auth_type behavior (#4454)
  > fix: init implicit iv for serialization feature (#4572)
  > [Nix] adjust pytest retrys (#4558)
  > fix: cert verify test fix (#4545)
  > fix: update default security policies (#4523)
  > feat(bindings): Associate an application context with a Connection (#4563)
  > chore(bindings): version bump (#4566)
  > Additional test cases for s2n_constant_time_equals() (#4559)
  > test: backwards compatibility test for the serialization feature (#4548)
  > chore(bench): upgrade rustls (#4554)

Signed-off-by: Alessandro Passaro <[email protected]>

* Try to reduce package size

Signed-off-by: Alessandro Passaro <[email protected]>

* Set operation_name when using MetaRequestType::Default

Signed-off-by: Alessandro Passaro <[email protected]>

* Introduce S3Operation type

Signed-off-by: Alessandro Passaro <[email protected]>

---------

Signed-off-by: Alessandro Passaro <[email protected]>
Co-authored-by: Alessandro Passaro <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2024
@passaro
Simplify get_object by waiting for response headers (#1171)
3c7fb3f 
`S3CrtClient::get_object` was originally implemented so that it would
complete immediately and return a `GetObjectRequest` implementation
(extending `Stream`) to retrieve body parts. Any error from the S3
request would be returned through the stream.
We recently added additional methods (`get_object_metadata` in #1065 and
`get_object_checksum` in #1123) to the response that rely on the headers
returned by the (first) `GetObject` request. The new methods required an
async signature and a complicated implementation in order to account for
failures and they still do not correctly report accurate error
information in some cases.
With this change, we modify `get_object` to await for response headers
before returning either an error or a `GetObjectResponse` (note the name
change) implementation. The ergonomics of `get_object` are improved:
* `await`ing the initial call can already return some errors (e.g.
bucket/key not found),
* `get_object_checksum` and `get_object_metadata` are now sync
functions.

### Does this change impact existing behavior?

Yes, `get_object` behavior is different, `get_object_checksum` and
`get_object_metadata` signatures have changed, and `GetObjectRequest`
was renamed to `GetObjectResponse`.

### Does this change need a changelog entry?

Yes, it requires a breaking change entry for `mountpoint-s3-client`.

---

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and I agree to the terms of
the [Developer Certificate of Origin
(DCO)](https://developercertificate.org/).

---------

Signed-off-by: Alessandro Passaro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@passaro passaro passaro approved these changes

@vladem vladem Awaiting requested review from vladem

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@muddyfish @passaro