Implement an iOS platform certificate verifier. by RyanTheOptimist · Pull Request #2638 · envoyproxy/envoy-mobile

RyanTheOptimist · 2022-10-27T20:54:36Z

Implement an iOS platform certificate verifier
and Swift builder APIs for enabling it.

Risk Level: Low
Testing: Help?
Docs Changes: N/A
Release Notes: Updated version_history.txt

Signed-off-by: Ryan Hamilton rch@google.com

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2022-11-02T16:34:29Z

/assign @jpsim

RyanTheOptimist · 2022-11-02T16:35:56Z

@jpsim this is finally passing CI. Woo hoo! I'm 100% confident that it needs work, but I'm not clear enough on how iOS work in Envoy Mobile to really have a good sense for where this code should go/what it should look like. So this is really more of rough draft than a ready-to-review PR, but if you could take a look, I would appreciate it.

docs/root/intro/version_history.rst

library/objective-c/EnvoyConfiguration.m

library/objective-c/EnvoyEngineImpl.m

jpsim · 2022-11-02T16:40:39Z

library/swift/EngineBuilder.swift

  private var enableBrotli: Bool = false
  private var enableInterfaceBinding: Bool = false
  private var enforceTrustChainVerification: Bool = true
+  private var enablePlatformCertificateValidation: Bool = true


Should this default to false until it's been vetted in a production experiment?

Suggested change

private var enablePlatformCertificateValidation: Bool = true

private var enablePlatformCertificateValidation: Bool = false

Yes, definitely! This was a hack to make sure this new code would be run though CI to help make sure it works. Do you have a suggestion for how to write a test for this code? Or alternatively, a suggestion for some place I could explicitly enable in, maybe in the experimental app?

test/swift/EngineBuilderTests.swift

jpsim · 2022-11-02T16:43:55Z

It's great to see this @RyanTheOptimist! My main thought is if it's possible to move the bulk of this into the library/common layer since the iOS platform APIs are in C.

I look forward to being able to stop bundling certs with Envoy Mobile in the long run!

RyanTheOptimist

Thanks for the review!

library/objective-c/EnvoyEngineImpl.m

RyanTheOptimist · 2022-11-02T16:55:54Z

library/swift/EngineBuilder.swift

  private var enableBrotli: Bool = false
  private var enableInterfaceBinding: Bool = false
  private var enforceTrustChainVerification: Bool = true
+  private var enablePlatformCertificateValidation: Bool = true


Yes, definitely! This was a hack to make sure this new code would be run though CI to help make sure it works. Do you have a suggestion for how to write a test for this code? Or alternatively, a suggestion for some place I could explicitly enable in, maybe in the experimental app?

test/swift/EngineBuilderTests.swift

.bazelrc

Update library/objective-c/EnvoyConfiguration.m Fix test constant Fix Test revert .bazelrc Co-authored-by: JP Simard <jp@jpsim.com> Signed-off-by: Ryan Hamilton <rch@google.com>

Signed-off-by: Ryan Hamilton <rch@google.com>

library/common/network/BUILD

library/common/network/ios_platform_verifier.cc

jpsim

This is really shaping up! Did you want to hook this up in the C++ builder in this PR or as a followup?

jpsim · 2022-11-04T17:36:39Z

I think I'm seeing a crash with this enabled:

* thread #14, stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001052583b0 app`Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation::postVerifyResultAndCleanUp(this=0x00006000019f4ef0, success=true, error_details="", tls_alert='.', error_counter=OptRef<Envoy::Stats::Counter> @ 0x000000016d9567c8) at platform_bridge_cert_validator.cc:142:21
    frame #1: 0x000000010525821c app`Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::verifyCertChainByPlatform(this=0x00006000000c61c0, cert_chain=size=0, host_name="", subject_alt_names=size=1, pending_validation=0x00006000019f4ef0) at platform_bridge_cert_validator.cc:125:22
    frame #2: 0x0000000105257360 app`Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation::verifyCertsByPlatform(this=0x00006000019f4ef0) at platform_bridge_cert_validator.cc:130:11
    frame #3: 0x0000000105263864 app`decltype(__f=0x00006000032291a8, __a0=0x00006000032291b8)).*fp()) std::__1::__invoke<void (Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation::*)(), Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation*, void>(void (Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation::*&&)(), Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation*&&) at type_traits:3859:1
    frame #4: 0x00000001052637ac app`void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation::*)(), Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation*, 2ul>(__t=size=3, (null)=__tuple_indices<2> @ 0x000000016d956f7f)(), Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation*>&, std::__1::__tuple_indices<2ul>) at thread:287:5
    frame #5: 0x0000000105262f38 app`void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation::*)(), Envoy::Extensions::TransportSockets::Tls::PlatformBridgeCertValidator::PendingValidation*> >(__vp=0x00006000032291a0) at thread:298:5
    frame #6: 0x00000001cba8f4e4 libsystem_pthread.dylib`_pthread_start + 116

jpsim · 2022-11-04T17:56:12Z

The crash happened when I was stepping through code in lldb breakpoints, so it may have influenced the crash. I haven't seen the crash since running without stopping at a breakpoint.

Can you add this to the experimental app so we get some integration test coverage?

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2022-11-04T18:13:56Z

This is really shaping up! Did you want to hook this up in the C++ builder in this PR or as a followup?

I'd prefer to hook up in a followup, just to keep this smaller, if that works for you?

RyanTheOptimist · 2022-11-04T18:14:45Z

The crash happened when I was stepping through code in lldb breakpoints, so it may have influenced the crash. I haven't seen the crash since running without stopping at a breakpoint.

Can you add this to the experimental app so we get some integration test coverage?

Is the experimental all the same thing as hello_world? I enabled it there, hoping that's what you're looking for, but maybe not?

RyanTheOptimist

Thanks for the review!

library/common/network/BUILD

library/common/network/ios_platform_verifier.cc

jpsim · 2022-11-04T18:33:37Z

Is the experimental all the same thing as hello_world? I enabled it there, hoping that's what you're looking for, but maybe not?

No, we try to keep the "hello world" type apps focused, the "baseline" apps as using the default engine builder configurations and the "experimental" apps using non-default engine builder configurations.

Can you set .enablePlatformCertificateValidation(true) here?

envoy-mobile/test/swift/apps/experimental/ViewController.swift

Line 20 in b22710b

let engine = EngineBuilder()

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist · 2022-11-04T18:57:41Z

Is the experimental all the same thing as hello_world? I enabled it there, hoping that's what you're looking for, but maybe not?

No, we try to keep the "hello world" type apps focused, the "baseline" apps as using the default engine builder configurations and the "experimental" apps using non-default engine builder configurations.

Can you set .enablePlatformCertificateValidation(true) here?

envoy-mobile/test/swift/apps/experimental/ViewController.swift

Line 20 in b22710b

let engine = EngineBuilder()

Oh, I see. Done!

jpsim · 2022-11-04T19:04:39Z

library/common/network/BUILD

 )
+
+cc_library(
+    name = "ios_platform_verifier",


Nit: can we call this apple_cert_verifier since "verifier" could imply it verifies a lot of things, and this applies to all Apple platforms, not just iOS, and mirrors the naming of the apple_dns_resolver.

Suggested change

name = "ios_platform_verifier",

name = "apple_platform_cert_verifier",

Good idea. Done.

jpsim · 2022-11-04T19:05:21Z

library/common/network/BUILD

+        ],
+        "//conditions:default": [],
+    }),
+)


Can you add a trailing newline to match unix conventions?

Done. Not sure how that crept back :/ (Also not sure why clang-format didn't fix it. But c'est la vie)

Signed-off-by: Ryan Hamilton <rch@google.com>

jpsim

Hooray, great job setting this up.

Signed-off-by: Ryan Hamilton <rch@google.com>

library/common/network/apple_platform_cert_verifier.h

Signed-off-by: Ryan Hamilton <rch@google.com>

…2663) Signed-off-by: Ryan Hamilton rch@google.com Was set to true inadvertently in #2638.

…builder-function * origin/main: ci: hopefully fixing bes timeout failures (#2666) Update Envoy (#2660) bazel: update rules_jvm_external to 4.5 (#2665) Remove note about DWARF patch being required (#2645) Bump Lyft Support Rotation (#2661) build: remove alwayslink set enablePlatformCertificateValidation to false on iOS by default (#2663) bump Envoy dep (#2659) Implement an iOS platform certificate verifier. (#2638) Signed-off-by: JP Simard <jp@jpsim.com>

RyanTheOptimist added 2 commits October 27, 2022 13:42

Implement an iOS platform certificate verifier.

6ad9ea6

Signed-off-by: Ryan Hamilton <rch@google.com>

release notes

e002ba1

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist marked this pull request as draft October 27, 2022 21:36

RyanTheOptimist added 12 commits October 27, 2022 15:07

Cleanup

8a9bff4

Signed-off-by: Ryan Hamilton <rch@google.com>

Format

32449b9

Signed-off-by: Ryan Hamilton <rch@google.com>

Test

bf3b101

Signed-off-by: Ryan Hamilton <rch@google.com>

Fix Test

e44f436

Signed-off-by: Ryan Hamilton <rch@google.com>

disable platform verification by default to see if tests pass

138e0a6

Signed-off-by: Ryan Hamilton <rch@google.com>

Format

2aef3cd

Signed-off-by: Ryan Hamilton <rch@google.com>

Merge branch 'main' into ios_verifier

30bf561

Signed-off-by: Ryan Hamilton <rch@google.com>

Fix multi-line strings

951dd14

Signed-off-by: Ryan Hamilton <rch@google.com>

Fix builder. *sigh*

65cd2b4

Signed-off-by: Ryan Hamilton <rch@google.com>

Format

37cdc26

Signed-off-by: Ryan Hamilton <rch@google.com>

enablePlatformCertificateValidation = true

f7be8db

Signed-off-by: Ryan Hamilton <rch@google.com>

Enable verifier

3bb3082

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist marked this pull request as ready for review November 2, 2022 16:34

jpsim reviewed Nov 2, 2022

View reviewed changes

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

jpsim reviewed Nov 2, 2022

View reviewed changes

library/objective-c/EnvoyConfiguration.m Outdated Show resolved Hide resolved

jpsim reviewed Nov 2, 2022

View reviewed changes

library/objective-c/EnvoyEngineImpl.m Outdated Show resolved Hide resolved

jpsim reviewed Nov 2, 2022

View reviewed changes

test/swift/EngineBuilderTests.swift Outdated Show resolved Hide resolved

RyanTheOptimist commented Nov 2, 2022

View reviewed changes

jpsim reviewed Nov 2, 2022

View reviewed changes

.bazelrc Outdated Show resolved Hide resolved

Update docs/root/intro/version_history.rst

4f4fe2d

Update library/objective-c/EnvoyConfiguration.m Fix test constant Fix Test revert .bazelrc Co-authored-by: JP Simard <jp@jpsim.com> Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist force-pushed the ios_verifier branch from 7f8c2c5 to 4f4fe2d Compare November 2, 2022 19:52

RyanTheOptimist added 2 commits November 4, 2022 09:37

Break out ios_platform_verifier

5c79ed9

Signed-off-by: Ryan Hamilton <rch@google.com>

Format

0775abd

Signed-off-by: Ryan Hamilton <rch@google.com>

Format

f977bc2

Signed-off-by: Ryan Hamilton <rch@google.com>

jpsim reviewed Nov 4, 2022

View reviewed changes

library/common/network/BUILD Show resolved Hide resolved

jpsim reviewed Nov 4, 2022

View reviewed changes

library/common/network/ios_platform_verifier.cc Show resolved Hide resolved

jpsim reviewed Nov 4, 2022

View reviewed changes

library/common/network/ios_platform_verifier.cc Outdated Show resolved Hide resolved

jpsim reviewed Nov 4, 2022

View reviewed changes

Address comments

aa258c9

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist commented Nov 4, 2022

View reviewed changes

library/common/network/BUILD Show resolved Hide resolved

library/common/network/ios_platform_verifier.cc Show resolved Hide resolved

library/common/network/ios_platform_verifier.cc Outdated Show resolved Hide resolved

Address more comments

55efac9

Signed-off-by: Ryan Hamilton <rch@google.com>

jpsim reviewed Nov 4, 2022

View reviewed changes

Format

56c01b3

Signed-off-by: Ryan Hamilton <rch@google.com>

jpsim previously approved these changes Nov 4, 2022

View reviewed changes

apple_platform_cert_verifier

afb2154

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist dismissed jpsim’s stale review via afb2154 November 4, 2022 19:19

jpsim previously approved these changes Nov 4, 2022

View reviewed changes

library/common/network/apple_platform_cert_verifier.h Outdated Show resolved Hide resolved

iOS => Apple

b883c10

Signed-off-by: Ryan Hamilton <rch@google.com>

RyanTheOptimist dismissed jpsim’s stale review via b883c10 November 4, 2022 19:42

jpsim approved these changes Nov 4, 2022

View reviewed changes

RyanTheOptimist merged commit c0ef4d4 into envoyproxy:main Nov 6, 2022

RyanTheOptimist mentioned this pull request Nov 7, 2022

set enablePlatformCertificateValidation to false on iOS by default #2663

Merged

RyanTheOptimist added a commit that referenced this pull request Nov 7, 2022

set enablePlatformCertificateValidation to false on iOS by default (#…

084be0a

…2663) Signed-off-by: Ryan Hamilton rch@google.com Was set to true inadvertently in #2638.

	private var enablePlatformCertificateValidation: Bool = true
	private var enablePlatformCertificateValidation: Bool = false

	name = "ios_platform_verifier",
	name = "apple_platform_cert_verifier",

Conversation

RyanTheOptimist commented Oct 27, 2022

Uh oh!

RyanTheOptimist commented Nov 2, 2022

Uh oh!

RyanTheOptimist commented Nov 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpsim Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpsim commented Nov 2, 2022

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RyanTheOptimist Nov 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpsim left a comment

Choose a reason for hiding this comment

Uh oh!

jpsim commented Nov 4, 2022

Uh oh!

jpsim commented Nov 4, 2022

Uh oh!

RyanTheOptimist commented Nov 4, 2022

Uh oh!

RyanTheOptimist commented Nov 4, 2022

Uh oh!

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpsim commented Nov 4, 2022

Uh oh!

RyanTheOptimist commented Nov 4, 2022

Uh oh!

jpsim Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

jpsim Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

jpsim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jpsim Nov 4, 2022 •

edited

Loading