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

provisioning profile and entitlement support on mac #319

Merged
merged 1 commit into from
Sep 5, 2018

Conversation

RyanJarv
Copy link
Contributor

@RyanJarv RyanJarv commented Aug 10, 2018

This was added upstream which ended up breaking the the signing scripts
and caused the osx build to not start.

We might be ok reverting the changes if necessary like in faed762 but i'd prefer to not worry about what that might break down the line.

Upstream signing changes:
https://chromium.googlesource.com/chromium/src.git/+/4d80b33e7c64069cb69041c4957ea77ecf6e14e8

Original more descriptive but reverted commit:
https://chromium.googlesource.com/chromium/src.git/+/b69d8c48da838196d9655402dd739e62227762a7

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions

@RyanJarv
Copy link
Contributor Author

RyanJarv commented Aug 10, 2018

@simonhong

I ended up hard coding the MAC_TEAM_ID here since I wasn't sure what the right way to do it was. I don't think this is an issue but would be interested if you have any thoughts on it.

@@ -348,7 +348,7 @@ if (is_mac) {
info_plist_target = ":brave_app_plist"

extra_substitutions = [
"CHROMIUM_BUNDLE_ID=$chrome_mac_bundle_id",
"CHROMIUM_BUNDLE_ID=$chrome_mac_bundle_id.$brave_channel",
Copy link
Member

@simonhong simonhong Aug 12, 2018

Choose a reason for hiding this comment

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

Is this change needed w/o changes in below BRANDING.dev?
$chrome_mac_bundle_id comes from the value of MAC_BUNDLE_ID.

We should consider stable channel. In that case, id would be org.brave.Brave.
Last dot should be removed in stable channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go ahead and open an issue for this if that works, have some changes for this but needs more testing.

@@ -5,6 +5,6 @@ PRODUCT_SHORTNAME=Brave
PRODUCT_INSTALLER_FULLNAME=Brave Installer
PRODUCT_INSTALLER_SHORTNAME=Brave Installer
COPYRIGHT=Copyright 2016 The Brave Authors. All rights reserved.
MAC_BUNDLE_ID=org.brave.Brave.dev
MAC_BUNDLE_ID=org.brave.Brave
Copy link
Member

@simonhong simonhong Aug 12, 2018

Choose a reason for hiding this comment

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

Hmm, to support multi channel install, we should have different bundle id.
Are there any reason why you should delete .dev from ID?

Copy link
Contributor Author

@RyanJarv RyanJarv Aug 13, 2018

Choose a reason for hiding this comment

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

Ok meant to bring this up as well. I may need to dig back through why this was an issue but it came down to MAC_BUNDLE_ID set's the base for any other helper's that need to be signed as well.

Ran into issues when everything was prefixed by org.brave.Brave.dev (i.e org.brave.Brave.dev.BraveHelper). I also remembered seeing somewhere that chrome doesn't actually modify these during build time, always has them set to org.brave.Brave but then during signing changes it to the correct value (will try to find references to this). Then taking a look at the app id's in the chrome canary package it seems only the root app itself is using the channel postfix.

So this is mainly to match that behavior which resolved one of the signing issues I was running into with the update to 69.

I'm unsure if this is the ideal way of doing things but, currently having it updated here:
https://github.com/brave/brave-core/pull/319/files#diff-4eaebbcd1dc0da11e3a3ce488290bfc6R350

Would appreciate any thoughts you have here.

Copy link
Member

Choose a reason for hiding this comment

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

If this change doesn't affect multi channel install support, I'm fine with this PR.
I'll test it with this PR and share it.

Copy link
Member

@simonhong simonhong Aug 14, 2018

Choose a reason for hiding this comment

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

With this PR and below change, I finished dev channel app signing.
However, I got code sign error when I run it after install.
Can you run installed app with this PR?

Also, I can't multi channel install test because I don't have other channel provision files.
I'm not sure but I need to create provision profile of my mac account and need different provisionprofile for each channel?

diff --git a/build/mac/BUILD.gn b/build/mac/BUILD.gn
index 0dddf74..d29b69a 100644
--- a/build/mac/BUILD.gn
+++ b/build/mac/BUILD.gn
@@ -35,7 +35,7 @@ action("sign_app") {
     rebase_path(_packaging_dir),
     keychain_db,
     mac_signing_identifier,
-    provisioning_profile,
+    rebase_path(provisioning_profile),
   ]
   deps = [
     "//brave:chrome_app",

Copy link
Member

Choose a reason for hiding this comment

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

Also, all other BRANDING files should be modified like this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@simonhong
Copy link
Member

simonhong commented Aug 15, 2018

I think #319 (comment) should be addressed.
Current PR would make stable's id to org.brave.Brave.
I think it should be org.brave.Brave - last dot removed.

When I installed and run from dev channel dmg with this PR, I got code sign error.
Can you run it on your machine?

Also each channel's provision profile should be included to build dmg on local.
After provision profile is introduced, can we build dmg and use it on local?

mbacchi added a commit that referenced this pull request Aug 17, 2018
This reverts commit 440b849, reversing
changes made to 71dd3c9.

Revert "Use the correct path for mac provisioning profile"

This reverts commit 575a971.

These reversions were made to remove some duplicate commits that
accidentally got pushed in the 0.53.x branch during WIP PR:

#319
@RyanJarv
Copy link
Contributor Author

Sorry for the delay here, I'm going to set to wip till this is better tested.

@RyanJarv RyanJarv changed the title provisioning profile and entitlement support on mac [WIP] provisioning profile and entitlement support on mac Aug 24, 2018
@bbondy bbondy changed the title [WIP] provisioning profile and entitlement support on mac WIP: provisioning profile and entitlement support on mac Sep 3, 2018
@simonhong
Copy link
Member

chromium_src/base/mac/foundation_util.mm should be checked, too.

const char* BaseBundleID() {
  if (base_bundle_id) {
    return base_bundle_id;
  }

#if !defined(OFFICIAL_BUILD)
  return "org.brave.Brave.development";
#else
  switch (chrome::GetChannel()) {
    case version_info::Channel::CANARY:
      return "org.brave.Brave.nightly";
    case version_info::Channel::DEV:
      return "org.brave.Brave.dev";
    case version_info::Channel::BETA:
      return "org.brave.Brave.beta";
    case version_info::Channel::STABLE:
    case version_info::Channel::UNKNOWN:
    default:
      return "org.brave.Brave";
  }
#endif
}

Fix dev app id on mac

To beable to support provisioning profiles the main app id needs to be
set as org.brave.Brave.dev with all the other frameworks/helper apps
falling directly under org.brave.Brave.<whatever>.

Checked upstream canary builds and this is how it is done there as well.

(cherry picked from commit 8146e3c)

provisioning profile and entitlement support on mac

This was added upstream which ended up breaking the the signing scripts
and caused the osx build to not start.

We might be ok reverting the changes if necessary like in
faed762
but i'd prefer to not worry about what that might break down the line.

Upstream signing changes:
https://chromium.googlesource.com/chromium/src.git/+/4d80b33e7c64069cb69041c4957ea77ecf6e14e8

Original more descriptive but reverted commit:
https://chromium.googlesource.com/chromium/src.git/+/b69d8c48da838196d9655402dd739e62227762a7

(cherry picked from commit 43a9802)

Use the correct path for mac provisioning profile

(cherry picked from commit b344827)

Update team_id for BRANDING.dev

Update branding files to match dev

This updates the branding files to match dev to support the recent
provisoning and entitlement related changes.

Update Apple app id's to use com.brave.Browser and add all profiles

Needed because com.brave.Brave wasn't available for every release we
needed.

Update OSX signing requirements and BRANDING files

Update mac app id in foundation_util.mm
@RyanJarv RyanJarv changed the title WIP: provisioning profile and entitlement support on mac provisioning profile and entitlement support on mac Sep 5, 2018
@RyanJarv
Copy link
Contributor Author

RyanJarv commented Sep 5, 2018

This can be merged. Will go ahead and open a ticket to address the app id concatenation for release, but would like to leave it as is for now since this is well tested and would like to get this merged.

@RyanJarv RyanJarv merged commit b205464 into master Sep 5, 2018
@RyanJarv RyanJarv deleted the fix/osx-signing branch September 5, 2018 13:34
petemill pushed a commit to petemill/brave-core that referenced this pull request Jul 5, 2019
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.

3 participants