Added SignTool for v5 #2444

jhromadik · 2019-12-18T13:56:45Z

Added SignTool for FAKE v5, some documentation, unit tests and updated obsolete information for the old SignToolHelper.

If someone could please have a look to make sure everything is OK ... Other than that, I think it's ready.

matthid · 2019-12-18T18:34:44Z

Can you please resolve the conflicts?

One thing I already noticed: Usually API design is to let users input a function and change arguments. You let users call Options.Create and then modify the record. Any particular reason to not follow what other modules do?

…tion parameter (default) and the other taking an options record parameter (o suffix)

jhromadik · 2019-12-20T09:58:26Z

Conflicts resolved.

As to my API design choice ... I'm not sure. I thought that I had looked at other projects to see what they were doing, concluded that it was about 50/50, and decided to go with the record-style. Looking through the projects now, I see that everyone is doing the function-style. So now I don't know what I was thinking.

Anyway, I reworked all functions to the function-style by default. I also kept the record-style functions as alternatives, same name only with an 'o' suffix, should anyone prefer to call them that way.

matthid · 2019-12-20T10:06:48Z

One problem with the regular record is when we add fields which we don't consider a breaking change. If people construct the record by hand (by specifying all fields and not using the create function) the code will no longer compile. So usung the functions kind of forces people to use the correct syntax without them having to know about the create function.
I'm not saying that we cannot provide those overloads I'm probably just not yet fully convinced :)

…arameter

jhromadik · 2020-01-06T16:37:58Z

Only function-style functions remain. They have better usability, and if you have a record you want to use, you can still do that - took me a while to realize this :). Added random string generation to tests, since FsCheck doesn't do that by default.

matthid

Ok thanks again! I did a full review now and I think some things are still old because of the port from the legacy SignToolHelper API?
We should consider fixing them as well, otherwise it looks reasonable. Especially thanks for the docs. That made reviewing easier :)

Let me know what you think.

matthid · 2020-01-06T18:46:34Z

help/markdown/fake-tools-signtool.md

+// val sign : certificate:SignTool.SignCertificate -> setOptions:(SignTool.SignOptions -> SignTool.SignOptions) -> files:seq<string> -> unit
+SignTool.sign
+    (SignTool.SignCertificate.Store {
+        SignTool.CertificateFromStore.Create() with


(minor) This looks a bit strange, on the other hand I thought about it and couldn't come up with something I really liked.

The "best" might be a CreateStore helper function which takes a creation function:

SignTool.SignCertificate.FromStore ("path/to/certificate-file.pfx", fun opts -> { opts with Password = Some "pw" })

or

SignTool.SignCertificate.FromFile (fun opts -> { opts with AutomaticallySelectCertificate = Some true SubjectName = Some "subject" StoreName = Some "My" })

Which might be a bit more familiar and you don't need to call Create by hand.

What do you think?

I was trying to find a way to make this "prettier" but came up empty. I like your suggestion and will implement it, thanks.

matthid · 2020-01-06T18:52:45Z

help/markdown/fake-tools-signtool.md

+SignTool.timeStamp
+    (fun o ->
+        { o with
+            TimeStamp = Some { SignTool.TimeStampOption.Create() with


Again: Quite Unusual
Maybe add overloads of the Create function? (One with only the server and one with server & algorithm).
Not sure what the most likely scenario is.

I think I'll just pull ServerUrl and Algorithm up to the options level.

Default parameters:

SignTool.timeStamp (fun o -> o)

With additional parameters:

SignTool.timeStamp (fun o -> { o with ServerUrl = ... Algorithm = ... } )

matthid · 2020-01-06T18:54:22Z

src/app/Fake.Tools.SignTool/SignTool.fs

+        ToolPath: string option
+        /// Timeout.
+        /// If not provided, default value is 10 seconds per file.
+        Timeout: TimeSpan option


I would expect Timeout = None to make the timeout disabled. Or is there a default by the tool itself (when no parameter is given)?

I was just trying to set a reasonable default value. In retrospect, it is unreasonable and more confusing than useful. I'll remove the default value and there will be no timeout when set to None.

matthid · 2020-01-06T18:56:25Z

help/markdown/fake-tools-signtool.md

+// set path to signtool.exe - if you want to use a specific version or you don't have Windows SDKs installed
+// by default, an attempt will be made to locate it automatically in 'Program Files (x86)\Windows Kits'
+{ o with
+    ToolOptions = Some { SignTool.ToolOptions.Create() with


Same as above in addition to the suggestions above another way would be to "inline" all the record fields. With the downside of duplicated code on the implementation (you could use SRTP - statically resolved type parameter to work around this)

I'll "inline" these. It will be much prettier on the surface and should only be a minor implementation inconvenience.

matthid · 2020-01-06T19:01:23Z

src/app/Fake.Tools.SignTool/SignTool.fs

+        let serverUrl = t.ServerUrl |> Option.defaultValue "http://timestamp.digicert.com"
+        yield match t.Algorithm with
+              | Some SHA1 -> sprintf "/t \"%s\"" serverUrl
+              | Some SHA256 | None -> sprintf "/tr \"%s\" /td \"sha256\"" serverUrl


I'd prefer to use the Arguments API as (briefly) documented in https://fake.build/core-process.html and https://fake.build/apidocs/v5/fake-core-argumentsmodule.html
It handles proper escaping and edge cases accordingly

I'll look into it.

matthid · 2020-01-06T19:04:05Z

src/app/Fake.Tools.SignTool/SignTool.fs

+    | true, File f ->
+        if f.Password.IsSome then
+            // surround in quotes to lower chances of replacing non-password occurences of password-string
+            TraceSecrets.register "\"<PASSWORD>\"" (sprintf "\"%s\"" f.Password.Value)


I guess you mean if a very simple password is used and replaced elsewhere?
This is a known problem, even on build servers which support secret variables (like azure devops)
I'm not sure we need special casing here, this might lead to the password being printed when it is not quoted?

I ran into this when I was using a test certificate where the password was also in the file name (to make sure I didn't forget it) - part of the file name was replaced with the registered replacement which looked pretty weird.
The implementation makes sure it is always quoted and there is a test for it too, so erroneously displaying the password should not be a problem.

…elper functions to make creating a SignCertificate easier

…rUrl is now required

…they are supposed to contain, nothing more

Robin1985 · 2020-03-03T10:54:33Z

Hi! Thanks for the contribution, is there anything I can do to help get this merged? I was just about to port this but then saw you already had a pull request for it.

matthid · 2020-03-03T11:21:08Z

I hope to find the time to prepare a new release within a couple of days. Either all points are addressed (or only small things which I can do myself are open) and it will be part of the release or new comments will follow :)

- using the newest Windows SDK available - working around ProcessUtils.tryFindFile's lack of globbing support

jhromadik · 2020-03-04T08:18:00Z

I think I've addressed all comments:

added CreateFromFile and CreateFromStore static helpers
made signing with timestamp a separate function singWithTimeStamp with additional parameters
no Timeout means no timeout
"inlined" ToolOptions
revorked implementation to use Arguments API

Plus some additional work:

removed all defaults that were different from signtool.exe's defaults
improved test to make sure final arguments only contain exactly what they are supposed to contain

@matthid, @Robin1985 please have a look at it and let me know what you think.

matthid · 2020-03-05T20:10:04Z

Looks good to me, thanks a lot!

added SignTool for v5

70d1790

matthid changed the base branch from master to release/next December 18, 2019 18:31

jhromadik added 2 commits December 20, 2019 10:14

added 2 ways of calling the functions - one taking a set-options func…

b7a12d6

…tion parameter (default) and the other taking an options record parameter (o suffix)

Merge branch 'release/next' into signtool-for-v5

c2d14a5

jhromadik added 3 commits December 30, 2019 12:50

removed record-style functions, made signing certificate a required p…

bae6c1a

…arameter

random string generation in tests

81a1d4f

minor tweaks

0132a79

matthid reviewed Jan 6, 2020

View reviewed changes

jhromadik added 9 commits January 13, 2020 11:13

added SignCertificate.FromFile and SignCertificate.FromStore static h…

edd47ac

…elper functions to make creating a SignCertificate easier

TimeStampOptions - pulled ServerUrl and Algorithm to top level; Serve…

9492ac6

…rUrl is now required

added function signWithTimeStamp

2b4e9f2

no Timeout means no timeout

7671b13

"inlined" ToolOptions

ebd54b7

using Arguments API to prepare arguments for the tool

cb5cfba

improved tests to make sure the final arguments contain exactly what …

ab65170

…they are supposed to contain, nothing more

using signtool default SHA1 for digest algorithm

32620c3

tidying up

3072942

fixing automatic signtool.exe lookup

f9888d5

- using the newest Windows SDK available - working around ProcessUtils.tryFindFile's lack of globbing support

matthid merged commit 10cd9bd into fsprojects:release/next Mar 5, 2020

matthid mentioned this pull request Mar 5, 2020

Release 5.20.0 #2466

Merged

3 tasks

jhromadik deleted the signtool-for-v5 branch March 9, 2020 07:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added SignTool for v5 #2444

Added SignTool for v5 #2444

jhromadik commented Dec 18, 2019

matthid commented Dec 18, 2019

jhromadik commented Dec 20, 2019

matthid commented Dec 20, 2019

jhromadik commented Jan 6, 2020

matthid left a comment

matthid Jan 6, 2020

jhromadik Jan 7, 2020

matthid Jan 6, 2020

jhromadik Jan 7, 2020

matthid Jan 6, 2020

jhromadik Jan 7, 2020

matthid Jan 6, 2020

jhromadik Jan 7, 2020

matthid Jan 6, 2020

jhromadik Jan 7, 2020

matthid Jan 6, 2020

jhromadik Jan 7, 2020

Robin1985 commented Mar 3, 2020

matthid commented Mar 3, 2020

jhromadik commented Mar 4, 2020

matthid commented Mar 5, 2020

Added SignTool for v5 #2444

Added SignTool for v5 #2444

Conversation

jhromadik commented Dec 18, 2019

matthid commented Dec 18, 2019

jhromadik commented Dec 20, 2019

matthid commented Dec 20, 2019

jhromadik commented Jan 6, 2020

matthid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robin1985 commented Mar 3, 2020

matthid commented Mar 3, 2020

jhromadik commented Mar 4, 2020

matthid commented Mar 5, 2020