Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2832073002:
Refactor mac signing scripts for development workflow (Closed)
|
Created:
3 years, 8 months ago by Greg K Modified:
3 years, 7 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, grt+watch_chromium.org, pennymac+watch_chromium.org, mac-reviews_chromium.org, wfh+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor mac signing scripts for development workflow
Refactor the mac signing scripts to add a --development flag,
so that a default designated requirement is used for development
builds. This also moves all the varaibles that need to be changed
to update the official signing certificate into one file.
BUG=713931
Review-Url: https://codereview.chromium.org/2832073002
Cr-Commit-Position: refs/heads/master@{#468727}
Committed: https://chromium.googlesource.com/chromium/src/+/4223ead6668664ae3cbfc0cb2f58c0294adc327f
Patch Set 1 #
Total comments: 13
Patch Set 2 : Refactor mac signing scripts for development workflow #
Total comments: 13
Patch Set 3 : Cleanup per review #Patch Set 4 : Renamed requirement #
Total comments: 7
Patch Set 5 : Fix wrong requirement variable name #
Messages
Total messages: 22 (6 generated)
kerrnel@chromium.org changed reviewers: + mark@chromium.org
Mark, It's been awhile since I've programmed in BASH. Is there a more elegant way to only add the requirement for official builds then these giant if statements? Thanks, Greg
https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... File chrome/installer/mac/sign_app.sh.in (right): https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:28: echo "usage: ${ME} app_path codesign_keychain codesign_id [--development]" >& 2 Stay within 80 characters. https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:35: is_development=false It’d be more normal for this to either be blank or non-blank, and then you’d test with [[ -z "${is_development}" ]] (meaning production) or [[ -n "${is_development}" ]] (meaning development). https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:67: codesign --sign "${codesign_id}" --keychain "${codesign_keychain}" \ You can build up the codesign --sign command line in an array, and append -r= to the array only if you’re not ${is_development} https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:94: # Verify with spctl, which uses the same rules that Gatekeeper does for Why bother making a temp_dir that you never use? Begin the !development conditional here, indent all of this stuff that follows. https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... File chrome/installer/mac/variables.sh (right): https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... chrome/installer/mac/variables.sh:8: requirement_suffix="and certificate leaf=H\"605852E8A609E39F8CF795E396657D26B3A031BC\"" What you upload and submit for review should only ever show what you want to check in (unless you’ve clearly marked something “not for checkin”). This certainly isn’t suitable for checkin. Let’s say that I was a terrible reviewer who stamped this without scrutinizing it, and you got excited so you ticked the commit checkbox, because it’s been a long weekend and you forgot that you had left this development stuff behind. It lands in the tree, and official signing starts failing. Oh no! https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... chrome/installer/mac/variables.sh:12: #" We also have sign_installer_tools.sh. Should it get in on the action?
On 2017/04/24 15:00:23, Mark Mentovai wrote: > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... > File chrome/installer/mac/sign_app.sh.in (right): > > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... > chrome/installer/mac/sign_app.sh.in:28: echo "usage: ${ME} app_path > codesign_keychain codesign_id [--development]" >& 2 > Stay within 80 characters. > > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... > chrome/installer/mac/sign_app.sh.in:35: is_development=false > It’d be more normal for this to either be blank or non-blank, and then you’d > test with [[ -z "${is_development}" ]] (meaning production) or [[ -n > "${is_development}" ]] (meaning development). > > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... > chrome/installer/mac/sign_app.sh.in:67: codesign --sign "${codesign_id}" > --keychain "${codesign_keychain}" \ > You can build up the codesign --sign command line in an array, and append -r= to > the array only if you’re not ${is_development} > > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... > chrome/installer/mac/sign_app.sh.in:94: # Verify with spctl, which uses the same > rules that Gatekeeper does for > Why bother making a temp_dir that you never use? Begin the !development > conditional here, indent all of this stuff that follows. > > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... > File chrome/installer/mac/variables.sh (right): > > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... > chrome/installer/mac/variables.sh:8: requirement_suffix="and certificate > leaf=H\"605852E8A609E39F8CF795E396657D26B3A031BC\"" > What you upload and submit for review should only ever show what you want to > check in (unless you’ve clearly marked something “not for checkin”). This > certainly isn’t suitable for checkin. > > Let’s say that I was a terrible reviewer who stamped this without scrutinizing > it, and you got excited so you ticked the commit checkbox, because it’s been a > long weekend and you forgot that you had left this development stuff behind. It > lands in the tree, and official signing starts failing. Oh no! > > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... > chrome/installer/mac/variables.sh:12: #" > We also have sign_installer_tools.sh. Should it get in on the action? Sorry this was supposed to be a WIP CL just to get your feedback on the big if then statements. I didn't mean to send you off to do a full review. - Greg
Description was changed from ========== Refactor mac signing scripts for development workflow Refactor the mac signing scripts to add a --development flag, so that a default designated requirement is used for development builds. This also moves all the varaibles that need to be changed to update the official signing certificate into one file. BUG=713931 ========== to ========== Refactor mac signing scripts for development workflow Refactor the mac signing scripts to add a --development flag, so that a default designated requirement is used for development builds. This also moves all the varaibles that need to be changed to update the official signing certificate into one file. DONOTSUBMIT=true BUG=713931 ==========
OK, that’s cool, as long as it’s marked as not appropriate for the tree.
https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... File chrome/installer/mac/sign_app.sh.in (right): https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:67: codesign --sign "${codesign_id}" --keychain "${codesign_keychain}" \ On 2017/04/24 15:00:23, Mark Mentovai wrote: > You can build up the codesign --sign command line in an array, and append -r= to > the array only if you’re not ${is_development} You can also write it a bit more directly by using an array to hold the optional extra argument(s), and have the array be empty if they're not present. That's probably better than building up the entire command in an array.
On 2017/04/24 22:29:17, Mark Mentovai wrote: > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... > File chrome/installer/mac/sign_app.sh.in (right): > > https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... > chrome/installer/mac/sign_app.sh.in:67: codesign --sign "${codesign_id}" > --keychain "${codesign_keychain}" \ > On 2017/04/24 15:00:23, Mark Mentovai wrote: > > You can build up the codesign --sign command line in an array, and append -r= > to > > the array only if you’re not ${is_development} > > You can also write it a bit more directly by using an array to hold the optional > extra argument(s), and have the array be empty if they're not present. That's > probably better than building up the entire command in an array. P.S. I thought of a good example of this: in keystone_install.sh, look at how ksadmin_args is handled.
https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... File chrome/installer/mac/sign_app.sh.in (right): https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:28: echo "usage: ${ME} app_path codesign_keychain codesign_id [--development]" >& 2 On 2017/04/24 15:00:23, Mark Mentovai wrote: > Stay within 80 characters. Done. https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:35: is_development=false On 2017/04/24 15:00:23, Mark Mentovai wrote: > It’d be more normal for this to either be blank or non-blank, and then you’d > test with [[ -z "${is_development}" ]] (meaning production) or [[ -n > "${is_development}" ]] (meaning development). Done. https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:67: codesign --sign "${codesign_id}" --keychain "${codesign_keychain}" \ On 2017/04/24 15:00:23, Mark Mentovai wrote: > You can build up the codesign --sign command line in an array, and append -r= to > the array only if you’re not ${is_development} Done. https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/sign_a... chrome/installer/mac/sign_app.sh.in:94: # Verify with spctl, which uses the same rules that Gatekeeper does for On 2017/04/24 15:00:23, Mark Mentovai wrote: > Why bother making a temp_dir that you never use? Begin the !development > conditional here, indent all of this stuff that follows. Done. https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... File chrome/installer/mac/variables.sh (right): https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... chrome/installer/mac/variables.sh:8: requirement_suffix="and certificate leaf=H\"605852E8A609E39F8CF795E396657D26B3A031BC\"" On 2017/04/24 15:00:23, Mark Mentovai wrote: > What you upload and submit for review should only ever show what you want to > check in (unless you’ve clearly marked something “not for checkin”). This > certainly isn’t suitable for checkin. > > Let’s say that I was a terrible reviewer who stamped this without scrutinizing > it, and you got excited so you ticked the commit checkbox, because it’s been a > long weekend and you forgot that you had left this development stuff behind. It > lands in the tree, and official signing starts failing. Oh no! Acknowledged. https://codereview.chromium.org/2832073002/diff/1/chrome/installer/mac/variab... chrome/installer/mac/variables.sh:12: #" On 2017/04/24 15:00:23, Mark Mentovai wrote: > We also have sign_installer_tools.sh. Should it get in on the action? Done.
https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... File chrome/installer/mac/sign_installer_tools.sh (right): https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_installer_tools.sh:46: "${sign_path}" --options "${enforcement_flags_tools}" enforcement_flags_installer_tools? “tools” is a little ambiguous otherwise. https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_installer_tools.sh:46: "${sign_path}" --options "${enforcement_flags_tools}" No custom requirement action here? Now that the requirements are centralized, is it more sensible? https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... File chrome/installer/mac/sign_versioned_dir.sh.in (right): https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:57: identifier=${3} Since you’re accepting this as an argument here, maybe you should sign with an explicit --identifier so that it matches what you stuff into the requirement. https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:103: codesign_with_options "${app_mode_loader_tmp}" \ Since you don’t have --identifier in codesign_with_options, this will no longer be signed with --identifier, and the default identifier won’t be app_mode_loader because we sign it as a temporary file with a different name. https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:118: "${enforcement_flags_helpers}" \ We used to use ${enforcement_flags_app} here. Do we want to drop “-o library” from this? https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/va... File chrome/installer/mac/variables.sh (right): https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/va... chrome/installer/mac/variables.sh:5: enforcement_flags_app="restrict" Maybe provide a little rationale for each of these three: what _app is used for and why we can’t use “library” on it, for example.
https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... File chrome/installer/mac/sign_installer_tools.sh (right): https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_installer_tools.sh:46: "${sign_path}" --options "${enforcement_flags_tools}" On 2017/04/25 02:02:24, Mark Mentovai wrote: > No custom requirement action here? Now that the requirements are centralized, is > it more sensible? Done. https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... File chrome/installer/mac/sign_versioned_dir.sh.in (right): https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:57: identifier=${3} On 2017/04/25 02:02:25, Mark Mentovai wrote: > Since you’re accepting this as an argument here, maybe you should sign with an > explicit --identifier so that it matches what you stuff into the requirement. I would rather do this only for app_mode_loader, since for the rest it will catch a mistake between the plist and the DR. WDYT? https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:103: codesign_with_options "${app_mode_loader_tmp}" \ On 2017/04/25 02:02:25, Mark Mentovai wrote: > Since you don’t have --identifier in codesign_with_options, this will no longer > be signed with --identifier, and the default identifier won’t be app_mode_loader > because we sign it as a temporary file with a different name. Done. https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:118: "${enforcement_flags_helpers}" \ On 2017/04/25 02:02:25, Mark Mentovai wrote: > We used to use ${enforcement_flags_app} here. Do we want to drop “-o > library” from this? Thanks for catching that. https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/va... File chrome/installer/mac/variables.sh (right): https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/va... chrome/installer/mac/variables.sh:5: enforcement_flags_app="restrict" On 2017/04/25 02:02:25, Mark Mentovai wrote: > Maybe provide a little rationale for each of these three: what _app is used for > and why we can’t use “library” on it, for example. Done.
https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... File chrome/installer/mac/sign_versioned_dir.sh.in (right): https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:57: identifier=${3} On 2017/04/25 18:45:21, Greg K wrote: > On 2017/04/25 02:02:25, Mark Mentovai wrote: > > Since you’re accepting this as an argument here, maybe you should sign with an > > explicit --identifier so that it matches what you stuff into the requirement. > > I would rather do this only for app_mode_loader, since for the rest it will > catch a mistake between the plist and the DR. WDYT? That sounds good. Let’s call this variable requirement_identifier or something then, to avoid confusion.
https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... File chrome/installer/mac/sign_versioned_dir.sh.in (right): https://codereview.chromium.org/2832073002/diff/20001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:57: identifier=${3} On 2017/04/25 18:46:49, Mark Mentovai wrote: > On 2017/04/25 18:45:21, Greg K wrote: > > On 2017/04/25 02:02:25, Mark Mentovai wrote: > > > Since you’re accepting this as an argument here, maybe you should sign with > an > > > explicit --identifier so that it matches what you stuff into the > requirement. > > > > I would rather do this only for app_mode_loader, since for the rest it will > > catch a mistake between the plist and the DR. WDYT? > > That sounds good. Let’s call this variable requirement_identifier or something > then, to avoid confusion. Done.
LGTM otherwise https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... File chrome/installer/mac/sign_installer_tools.sh (right): https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... chrome/installer/mac/sign_installer_tools.sh:59: codesign_cmd+=( -r="${designated}" ) ${requirement}, not ${designated}, right? You called it requirement_string in sign_app.sh.in. Maybe it’s best to use the same name consistently in all of these scripts. https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... File chrome/installer/mac/sign_versioned_dir.sh.in (right): https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:73: requirement="designated => identifier \"${requirement_identifier}\" \ This one’s correct, but you may want to change it or change the one in sign_app.sh.in so that they all match. https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/va... File chrome/installer/mac/variables.sh (right): https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/va... chrome/installer/mac/variables.sh:9: # All the helpers (crashpad, app_mode_loader, etc.), run under library Blank line before this, and the same on line 12. Otherwise, the actual variable assignments get lost in the wall of comments.
https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... File chrome/installer/mac/sign_installer_tools.sh (right): https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... chrome/installer/mac/sign_installer_tools.sh:59: codesign_cmd+=( -r="${designated}" ) On 2017/04/25 20:34:57, Mark Mentovai wrote: > ${requirement}, not ${designated}, right? > > You called it requirement_string in sign_app.sh.in. Maybe it’s best to use the > same name consistently in all of these scripts. Good catch. It turns out spctl failing caused the test never execute this code: not good! I made sure to execute it now. https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... File chrome/installer/mac/sign_versioned_dir.sh.in (right): https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:73: requirement="designated => identifier \"${requirement_identifier}\" \ On 2017/04/25 20:34:57, Mark Mentovai wrote: > This one’s correct, but you may want to change it or change the one in > sign_app.sh.in so that they all match. Sorry, I don't understand? https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/va... File chrome/installer/mac/variables.sh (right): https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/va... chrome/installer/mac/variables.sh:9: # All the helpers (crashpad, app_mode_loader, etc.), run under library On 2017/04/25 20:34:57, Mark Mentovai wrote: > Blank line before this, and the same on line 12. Otherwise, the actual variable > assignments get lost in the wall of comments. Done.
LGTM https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... File chrome/installer/mac/sign_versioned_dir.sh.in (right): https://codereview.chromium.org/2832073002/diff/60001/chrome/installer/mac/si... chrome/installer/mac/sign_versioned_dir.sh.in:73: requirement="designated => identifier \"${requirement_identifier}\" \ Greg K wrote: > Sorry, I don't understand? Never mind, I was talking about variable name consistency but you fixed it in the other file.
Description was changed from ========== Refactor mac signing scripts for development workflow Refactor the mac signing scripts to add a --development flag, so that a default designated requirement is used for development builds. This also moves all the varaibles that need to be changed to update the official signing certificate into one file. DONOTSUBMIT=true BUG=713931 ========== to ========== Refactor mac signing scripts for development workflow Refactor the mac signing scripts to add a --development flag, so that a default designated requirement is used for development builds. This also moves all the varaibles that need to be changed to update the official signing certificate into one file. BUG=713931 ==========
The CQ bit was checked by kerrnel@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493744000305710,
"parent_rev": "d94da1744907ed1bb90e37756806841609b0cc52", "commit_rev":
"4223ead6668664ae3cbfc0cb2f58c0294adc327f"}
Message was sent while issue was closed.
Description was changed from ========== Refactor mac signing scripts for development workflow Refactor the mac signing scripts to add a --development flag, so that a default designated requirement is used for development builds. This also moves all the varaibles that need to be changed to update the official signing certificate into one file. BUG=713931 ========== to ========== Refactor mac signing scripts for development workflow Refactor the mac signing scripts to add a --development flag, so that a default designated requirement is used for development builds. This also moves all the varaibles that need to be changed to update the official signing certificate into one file. BUG=713931 Review-Url: https://codereview.chromium.org/2832073002 Cr-Commit-Position: refs/heads/master@{#468727} Committed: https://chromium.googlesource.com/chromium/src/+/4223ead6668664ae3cbfc0cb2f58... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4223ead6668664ae3cbfc0cb2f58... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
