No need to be RV-G since this is just security engineering.

1. I do think we should figure out the signing bits before launching because the control channel between PWAs and the browser is more complex than it was for app shims, and we should ensure it's authenticated.

2. Will existing app shims use the new Remote MacViews interfaces as new shims? If so, it'd probably be best to re-write all the shims. If not, we can leave them alone.

Why do the apps exist in two places? I'm not really following that comment...

> 2. Will existing app shims use the new Remote MacViews interfaces as new shims?

At present, they do. Maybe I can disable that. That would also fix  issue 896937 . And ... we have to be smarter about which interface we use, as I just found out in  issue 913394 .

> Why do the apps exist in two places? I'm not really following that comment...

I started writing here, then realized that I need a doc to cover that issue. We should should talk about this.

https://docs.google.com/document/d/1U1yOH2XlsWW5fO63w0MAXWjjZewvE5XTAP4V-6YTGBc/edit?usp=sharing

Is this something sdy@ is looking at?

 Issue 896937  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e054f2eb2aeea8132a835f128e19d077b930e911

commit e054f2eb2aeea8132a835f128e19d077b930e911
Author: Christopher Cameron <ccameron@chromium.org>
Date: Wed Dec 12 17:10:07 2018

RemoteMacViews: Add AppShimHostBootstrap::GetAppShimPid

This will be queried in ExtensionAppShimHandler::OnProfileLoaded, where
we will check that the pid's signature.

Bug: 913362
Change-Id: I6fd144b26fad182d040f302f078a343cd2ab6cd9
Reviewed-on: https://chromium-review.googlesource.com/c/1373433
Commit-Queue: ccameron <ccameron@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615930}
[modify] https://crrev.com/e054f2eb2aeea8132a835f128e19d077b930e911/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.cc
[modify] https://crrev.com/e054f2eb2aeea8132a835f128e19d077b930e911/chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h

Re. #3, yep, I'm currently working on the signing part.

Update on signing: I've been fighting with the code signing APIs but currently have a good way forward. mark@ and rsesek@ have been graciously talking through it with me. Signing Mach-O files, and bundles which have one as their main executable, requires a helper tool, `codesign_allocate`, which only ships with dev tools. `codesign_allocate` is open source, but Chrome can't ship its own because it must be specially signed by Apple for the code signing APIs to use it. (Fun fact: when macOS generates printer proxy apps in ~/Library/Printers, they only get properly signed if the machine has dev tools installed.)

To work around this, I'm making the app shim's main executable be a shell script which execs the real executable. The real executable can be pre-signed, and signing the bundle just embeds the existing signature in the bundle's _CodeSignature directory instead of modifying it, which works on end user machines.

You can find more discussion on the CL, which I'm hoping to land today-ish unless anything else comes up:

https://chromium-review.googlesource.com/c/chromium/src/+/1376736

This will be the first of two CLs. The second one will *check* the signature and should be way simpler.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/81bcf4b757775108c26f7737e0a8338111d6bb38

commit 81bcf4b757775108c26f7737e0a8338111d6bb38
Author: Sidney San Martín <sdy@chromium.org>
Date: Fri Jan 18 19:17:57 2019

Only allow an app shim process to connect if it has a matching code signature.

Otherwise, don't trust it. Rebuild it instead.

Bug: 913362
Change-Id: I294f36c22cea8364f8f2c39cb6775a3011ce7351
Reviewed-on: https://chromium-review.googlesource.com/c/1407041
Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Auto-Submit: Sidney San Martín <sdy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624228}
[modify] https://crrev.com/81bcf4b757775108c26f7737e0a8338111d6bb38/chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc
[modify] https://crrev.com/81bcf4b757775108c26f7737e0a8338111d6bb38/chrome/common/mac/app_shim_launch.h

Update: The workaround from c#7 caused some mysterious test failures which suggests that the wrapper interfered with the shims' app-ness. After much more discussion, we decided to forgo signing the shim bundles and just verify the signature of the executable. That's what the CL above (r624228) does.

Because this is tricky to test in dev, I'm leaving this issue open until it's on Canary next week, and still need to write a test plan (issue 922305).

The requirement for official Chrome builds to be signed is causing some internal build failures. Discussion in 923612. Just as a heads up to anyone watching this bug: the above CL might get reverted until after the long weekend.

This is also having issues with Canary and with ToT Chromium builds.

The SecRequirementRef for Dev is
  (
    identifier "com.google.Chrome" or
    identifier "com.google.Chrome.beta" or
    identifier "com.google.Chrome.dev" or
    identifier "com.google.Chrome.canary"
  )
  and
    certificate leaf = H"c9a99324ca3fcb23dbcc36bd5fd4f9753305130a"

Meanwhile the SecRequirementRef for the app shim is
  identifier "app_mode_loader" and
  certificate leaf = H"c9a99324ca3fcb23dbcc36bd5fd4f9753305130a"

So it would seem to me that we should just add "app_mode_loader" over at [0]?

[0] https://cs.chromium.org/chromium/src/chrome/installer/mac/sign_app.sh.in?rcl=08bdd0605e5e93030faacefaf4d7a7f4bab17d14&l=85

The signature check has been disabled in crrev.com/624478 (sorry, used the wrong bug number).

I've put together two potential fixes for this:

Fix 1: add "app_mode_loader" to the identifiers in SecRequirementRef (I think)
https://chromium-review.googlesource.com/c/chromium/src/+/1423787

Fix 2: strip the identifiers section out of the SecRequirementRef

I've been able to test Fix 2, though any time I'm parsing text I get the feeling that I'm doing it wrong. I don't think I can verify Fix 1 without just submitting the patch.

Err, fix 2 is at
https://chromium-review.googlesource.com/c/chromium/src/+/1424738

I think fix #1 would be preferable.

Fix 3: Change the loader's identifier from "app_mode_loader" to "com.…" to match the ones already in the DR?

The identifier is either the CFBundleIdentifier or the basename of the signed image, so that won't work.

Hmm. What do you make of the of the "Sharing a Designated Requirement" section of this doc?

     https://developer.apple.com/library/archive/documentation/Security/Conceptual/CodeSigningGuide/Procedures/Procedures.html

OK, after some offline chat my conclusion is that they're hinting at using a designated requirement which includes multiple sub-components' identifiers. But it's confusing, I may file a radar.