drone-cli icon indicating copy to clipboard operation
drone-cli copied to clipboard

Removed syscerts dependency

Open JakeHillion opened this issue 4 years ago • 7 comments
trafficstars

master won't build on Apple Silicon due to a dependency on jackspirou/syscerts. According to the README of jackspirou/syscerts, the package exists due to crypto/x509 not exposing systemRootsPool(). This is now exposed as SystemCertPool() (https://go-review.googlesource.com/c/go/+/21293/).

I've updated to remove this dependency in favour of crypto/x509. As a result, it now builds on Apple Silicon. I have performed minimal testing (drone info and drone sign --save ...) without error.

JakeHillion avatar Feb 21 '21 21:02 JakeHillion

The review of googlesource mentioned a comment like this:

SystemCertPool returns an error on Windows

Somebody got to test that carefully before merging the PR, I don't know if this error is still the case. If it fails I would prefer a fix for syscerts.

tboerger avatar Feb 22 '21 07:02 tboerger

Just checked the Go source code, it still errors hard on windows: https://github.com/golang/go/blob/master/src/crypto/x509/cert_pool.go#L106-L109

tboerger avatar Feb 22 '21 07:02 tboerger

Can confirm that it errors on Windows. I don't think this error is quite as bad as it looks - syscerts already doesn't support Windows, but it fails silently (https://github.com/jackspirou/syscerts/blob/master/root_windows.go#L39-L40), returning nil.

It seems like semantics would be identical by ignoring the error, but this doesn't seem ideal (no worse than previously though). Unfortunately the Windows error isn't exported, so ignoring it specifically is a bit ugly. Another alternative is not running the line on Windows, but this will require changing back if Windows support is ever re-added.

JakeHillion avatar Feb 22 '21 08:02 JakeHillion

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Jan 19 '22 06:01 CLAassistant

This issue for syscerts has been open quite a while: https://github.com/jackspirou/syscerts/issues/3, which prevents building the drone CLI on arm64 macs.

syscerts hasn't been updated for over 5 years. Would it be reasonable for you to fork the library to fix this issue? As noted, this already doesn't work on Windows, and this issue now prevents building the CLI on recent macs.

I can't tell via your link when this changed, but root_windows.go returns a regular struct from loadSystemRoots: https://github.com/golang/go/blob/ccab2fbc30b0553fce54646a4da0a8645eda40a3/src/crypto/x509/root_windows.go#L13

joeblubaugh avatar Sep 02 '22 03:09 joeblubaugh

The syscerts lib could be replaced by crypto/x509 these days as it does not error on Windows anymore.

tboerger avatar Sep 02 '22 08:09 tboerger

Does that mean we could merge this PR as-is?

joeblubaugh avatar Sep 05 '22 06:09 joeblubaugh

Any update on this? Still not able to build this on a M1 Mac.

d0x7 avatar May 16 '23 07:05 d0x7

This PR has been automatically closed due to inactivity.

bot-harness avatar Oct 19 '23 11:10 bot-harness