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

Support for additional SignTool flags #2026

Closed
ben-wallis opened this issue Jan 29, 2018 · 3 comments
Closed

Support for additional SignTool flags #2026

ben-wallis opened this issue Jan 29, 2018 · 3 comments
Milestone

Comments

@ben-wallis
Copy link

ben-wallis commented Jan 29, 2018

Cake.Common.Tools.SignTool is currently missing support for several flags relating to certificate locations.

All of these should be optional, I'd propose the following additional properties are added to the SignToolSignSettings class:

signtool sign flag Property Name Type
/ac AdditionalCertificatePath string
/s CertStoreName string
/sm UseMachineStore bool

Additionally, the TimeStampUri property of SignToolSettings is currently mandatory due to the following code in SignToolSignRunner.cs:

if (settings.TimeStampUri == null)
{
	const string format = "{0}: Timestamp server URL is required but not specified.";
	var message = string.Format(CultureInfo.InvariantCulture, format, GetToolName());
	throw new CakeException(message);
}

Using a timestamp server should be completely optional as it significantly slows down the signing process and is often unnecessary for things such as internal release builds.

Also, it's worth noting that it's not possible to use ProcessArgumentBuilder's Append() or Prepend() methods to add any parameters to SignTool either, as Prepend() inserts them before the "sign" parameter of "signtool.exe sign", and Append() inserts them after the list of files to sign. Currently we're using reflection to insert into the private _tokens list within ProcessArgument which is obviously not ideal.

@MihaMarkic
Copy link
Contributor

Fully agree, I added a PR for /sm argument

@MihaMarkic
Copy link
Contributor

@ben-wallis There is a workaround, as suggest by Joseph Musser: create a new instance of ProcessArgumentBuilder, merge arguments and return it as result.

@devlead
Copy link
Member

devlead commented Jan 12, 2019

Fixed by #2437

@devlead devlead closed this as completed Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants