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.

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

Add Oblivious HTTP (OHTTP) client for iOS [ci full] #5749

Merged
merged 1 commit into from Jul 31, 2023

Conversation

moztcampbell
Copy link
Collaborator

Using the ohttp-rs and bhttp-rs Rust libraries to perform underlying encoding and encryption. This is exposed to Swift using URLRequest/Response types that are typical for the platform.

Note that only iOS is targetted since Gecko projects have a builtin OHTTP solution within Necko.

This use currently using the Rust Crypto libraries for underlying HPKE support, but this should be changed to NSS or OpenSSL in future.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (9049484) 89.86% compared to head (463fcbe) 89.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5749   +/-   ##
=======================================
  Coverage   89.86%   89.86%           
=======================================
  Files           1        1           
  Lines         148      148           
=======================================
  Hits          133      133           
  Misses         15       15           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

This is awesome 🚀 🚀 !! I mostly had nits - generally, I'm happy with getting this out so iOS has their hands on it and we can iterate from there

Only requesting changes because we'll need to add this component to https://github.com/mozilla/application-services/blob/main/taskcluster/scripts/build-and-test-swift.py so the new taskcluster-based process can get this to iOS in the next nightly without any rust-component-swift changes at all. I'll update the docs to include that step!

Once we do that, we can test it out by adding [ci full] in the PR title, then closing and re-opening the PR

components/as-ohttp-client/src/lib.rs Outdated Show resolved Hide resolved
megazords/ios-rust/src/lib.rs Outdated Show resolved Hide resolved
components/as-ohttp-client/src/as_ohttp_client.udl Outdated Show resolved Hide resolved
components/as-ohttp-client/src/lib.rs Show resolved Hide resolved
@tarikeshaq
Copy link
Member

For the dependency check job, we might need a fix up in

PACKAGE_METADATA_FIXUPS = {
(there are some examples there)

Then running ./tools/regenerate_dependency_summaries.sh and checking-in the changes the script does

@tarikeshaq
Copy link
Member

Only requesting changes because we'll need to add this component to https://github.com/mozilla/application-services/blob/main/taskcluster/scripts/build-and-test-swift.py so the new taskcluster-based process can get this to iOS in the next nightly without any rust-component-swift changes at all. I'll update the docs to include that step!

Opened #5753 for updating the docs, sorry it wasn't up to date!

@mergify mergify bot dismissed tarikeshaq’s stale review July 28, 2023 19:58

The pull request has been modified, dismissing previous reviews.

@tarikeshaq tarikeshaq self-requested a review July 28, 2023 21:00
Copy link
Member

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

A couple of tiny comments then we can land!

After fixing that comments tried to build Firefox iOS locally by running the taskcluster scripts and verified that the app builds and that it has access to OhttpManager, its constructor and the data function

@mergify mergify bot dismissed tarikeshaq’s stale review July 31, 2023 15:39

The pull request has been modified, dismissing previous reviews.

@tarikeshaq tarikeshaq self-requested a review July 31, 2023 15:47
tarikeshaq
tarikeshaq previously approved these changes Jul 31, 2023
Copy link
Member

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

Awesome 🚀 thank you!

I verified this builds okay with iOS and should be available for iOS engineers tomorrow (or we can expedite it to later today with a manual run!)

Do you mind adding a short entry to the changelog in https://github.com/mozilla/application-services/blob/main/CHANGELOG.md under the current (118) release?

(I'll also give you the permissions needed to land approved PR and so reviews don't go stale)

Using the ohttp-rs and bhttp-rs Rust libraries to perform underlying encoding
and encryption. This is exposed to Swift using URLRequest/Response types that
are typical for the platform.

Note that only iOS is targetted since Gecko projects have a builtin OHTTP
solution within Necko.

This use currently using the Rust Crypto libraries for underlying HPKE support,
but this should be changed to NSS or OpenSSL in future.
@mergify mergify bot dismissed tarikeshaq’s stale review July 31, 2023 17:47

The pull request has been modified, dismissing previous reviews.

@moztcampbell
Copy link
Collaborator Author

Rebased onto v118 and added changelog entry.

@moztcampbell moztcampbell changed the title Add Oblivious HTTP (OHTTP) client for iOS Add Oblivious HTTP (OHTTP) client for iOS [ci full] Jul 31, 2023
@moztcampbell moztcampbell reopened this Jul 31, 2023
Copy link
Member

@tarikeshaq tarikeshaq left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢

@moztcampbell moztcampbell added this pull request to the merge queue Jul 31, 2023
Merged via the queue into mozilla:main with commit 14def5e Jul 31, 2023
49 checks passed
@moztcampbell moztcampbell deleted the ohttp branch July 31, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants