create signing format that uses osslsigncode for 64-bit windows signing

RESOLVED FIXED

Status

Release Engineering
General Automation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bhearsum, Assigned: bhearsum)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
Creating a new one instead of changing the existing has a few benefits:
* No need to change anything with 32-bit signing (lowers risk)
* Allows developers to push changes changes to try
* Easier to land
(Assignee)

Comment 1

3 years ago
Created attachment 8499171 [details] [diff] [review]
create osslcodesign format

This patch needs more testing, but it works. I'm curious to get feedback, particularly around the re-usage of the signcode secrets. We can use the exact same secrets for osslcodesign, so I figured it was better not to duplicate them....but it could also be confusing because every other signing type has their own secrets.

osslcodesign also doesn't support retrying when it fails to contact the timestamp server, so I want to build in our own retry for that, as we've had a lot of issues with that in the past.
Attachment #8499171 - Flags: feedback?(catlee)
Comment on attachment 8499171 [details] [diff] [review]
create osslcodesign format

Review of attachment 8499171 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/python/signing/utils.py
@@ +18,5 @@
>  =designated => ( (anchor apple generic and certificate leaf[field.1.2.840.113635.100.6.1.9] ) or (anchor apple generic and certificate 1[field.1.2.840.113635.100.6.2.6] and certificate leaf[field.1.2.840.113635.100.6.1.13] and certificate leaf[subject.OU] = "%(subject_ou)s"))
>  """
>  
>  
> +def regenerate_chk_files(filename):

nit: call this regenerate_chk_file rather than _files, since it only does one at a time, right?

@@ +118,5 @@
> +        # We use logfile_read because we only want stdout/stderr, _not_ stdin.
> +        proc.logfile_read = stdout
> +        proc.expect('Enter PEM pass phrase')
> +        proc.sendline(passphrase)
> +        proc.wait()

will this work when the signing server isn't running attached to a terminal? does pexpect handle allocating a tty?
Attachment #8499171 - Flags: feedback?(catlee) → feedback+
(Assignee)

Comment 3

3 years ago
(In reply to Chris AtLee [:catlee] from comment #2)
> @@ +118,5 @@
> > +        # We use logfile_read because we only want stdout/stderr, _not_ stdin.
> > +        proc.logfile_read = stdout
> > +        proc.expect('Enter PEM pass phrase')
> > +        proc.sendline(passphrase)
> > +        proc.wait()
> 
> will this work when the signing server isn't running attached to a terminal?
> does pexpect handle allocating a tty?

I've been running in daemon mode for all my tests, so I assume so! We also already use pexpect for all of our mac signing.
(Assignee)

Comment 4

3 years ago
Created attachment 8499705 [details] [diff] [review]
tested patch for osslsigncode support

Per IRC, chk file regeneration is dead as part of this method -- it happens on the client side now (see signing/client.py in remote_signfile), so I ripped it out. The only other change since the last patch is comment/docstring updates. signcode and osslsigncode both seem to work, so I think this is ready to go? I won't be pushing until Monday, though.
Attachment #8499171 - Attachment is obsolete: true
Attachment #8499705 - Flags: review?(catlee)
(Assignee)

Comment 5

3 years ago
Created attachment 8499756 [details] [diff] [review]
enable osslsigncode on linux signing servers

This should probably land shortly after the tools patch, and then I'll need to restart the signing servers to pick up the new code + format.
Attachment #8499756 - Flags: review?(catlee)

Updated

3 years ago
Attachment #8499705 - Flags: review?(catlee) → review+
Comment on attachment 8499756 [details] [diff] [review]
enable osslsigncode on linux signing servers

Review of attachment 8499756 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/signingserver/manifests/instance.pp
@@ +49,5 @@
>      # paths in packages
>      $signmar = "/tools/signmar/bin/signmar"
>      $testfile_dir = "/tools/signing-test-files"
>      $testfile_signcode = "${testfile_dir}/test.exe"
> +    $testfile_osslsigncode = "${testfile_dir}/test.exe"

as per IRC, this should be test64.exe
Attachment #8499756 - Flags: review?(catlee) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8499780 [details] [diff] [review]
fixed puppet patch

Just to make sure I don't land the wrong thing by accident....
Attachment #8499756 - Attachment is obsolete: true
Attachment #8499780 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8499705 - Flags: checked-in+
(Assignee)

Updated

3 years ago
Attachment #8499780 - Flags: checked-in+
(Assignee)

Comment 8

3 years ago
The puppet change has been synced out to the Linux signing servers, and I updated all the tools checkouts and they have all been restarted for it. The buildbot masters will be reconfiged with the passwords.py change in a little bit.
(Assignee)

Comment 9

3 years ago
This looks to be working fine.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.