Skip to content

Conversation

@calmh
Copy link
Member

@calmh calmh commented Oct 28, 2022

This is sort of a proof of concept, but since our current Windows builder is down this might solve the problem. It includes a change for easier code signing (taking the certificate in a secret/env var rather than existing already on disk) but otherwise mirrors precisely what we already do in the build server.

This is sort of a proof of concept, but since our current Windows
builder is down this might solve the problem. It includes a change for
easier code signing (taking the certificate in a secret/env var rather
than existing already on disk), but otherwise mirrors precisely what we
already do in the build server.
@calmh calmh marked this pull request as ready for review October 28, 2022 12:22
imsodin
imsodin previously approved these changes Oct 29, 2022
Copy link
Member

@imsodin imsodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on the PR (GH is smart enough to pick up pipeline changes for PR runs - nice?!) and looks as tidy as yaml can.

One minor downside is that artifacts from GH actions are only accessible to logged in GH users. I don't think there's much need for those for non-github users though - otherwise we could setup an action to upload it somewhere (nightly section on the downloads page or something).

# Only run this for pushes to our branches, not on PRs. Uses secrets
# for code signing.
if: github.event_name == 'push'
run: |
Copy link
Member

@AudriusButkevicius AudriusButkevicius Oct 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit weak.
I know that only we can push to "our" branches, but I'd felt more comfortable of this had some sort of branch name check etc, or something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - I'd even say we don't need any CI for random branches. So a branches: ["main"] condition at the top on the triggers could ensure that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we still want to build on PRs to check that a given PR doesn't break the Windows build, don't we?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two triggers: One for PR and one for push, which are independent. The branch restrictions is only needed on the push one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... I was thinking about that. We currently produce artifacts for any all builds on random branches, and that might be something we want to preserve? Otherwise we can of course easily restrict it to just builds on main.

(I also have a much more elaborate extension of this PR, with matrix builds of go version x platform and package builds for all the things we usually build for...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to exfiltrate e.g signing certificate that way?

Copy link
Member Author

@calmh calmh Oct 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so; PRs don't get access to secrets, so we'd have to approve and merge the exfiltrating code first. Probably you can do that with our current build setup, though... If you are a contributor who can push directly to the repo, then yes you could exfiltrate the secrets. Again, you can do that today also because they are environment variables or files on disk in the current setup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... I was thinking about that. We currently produce artifacts for any all builds on random branches, and that might be something we want to preserve? Otherwise we can of course easily restrict it to just builds on main.

(I also have a much more elaborate extension of this PR, with matrix builds of go version x platform and package builds for all the things we usually build for...)

do random branch builds need to be signed?
don't think so.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they really don't. GitHub actions has an "environments" concept where you can bind secrets to an environment and say that a certain branch should run in that environment, I think. I'll see if I can make the signing secrets specific to builds on main and release branches only.

@imsodin imsodin dismissed their stale review October 29, 2022 12:21

Push on non-main

@calmh calmh temporarily deployed to signing November 2, 2022 06:13 Inactive
@calmh calmh merged commit 452daad into syncthing:main Nov 5, 2022
calmh added a commit to imsodin/syncthing that referenced this pull request Nov 8, 2022
* main: (36 commits)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  lib/fs: Let xattr test avoid non-test attributes (fixes syncthing#8601) (syncthing#8628)
  build: Add GitHub actions build for Windows
  gui, man, authors: Update docs, translations, and contributors
  gui: Display folder and device count number (syncthing#8615)
  gui, man, authors: Update docs, translations, and contributors
  lib/model, lib/protocol: Fix file comparisons (fixes syncthing#8594) (syncthing#8603)
  lib/scanner: More sensible debug output (syncthing#8596)
  gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597)
  ...
@calmh calmh added this to the v1.22.2 milestone Nov 9, 2022
calmh added a commit to calmh/syncthing that referenced this pull request Nov 22, 2022
* main: (23 commits)
  lib/fs: Optimize WindowsInvalidFilename (syncthing#8687)
  gui, man, authors: Update docs, translations, and contributors
  cmd/syncthing: Use main logger in generate subcommand (fixes syncthing#8682) (syncthing#8685)
  build: Update all dependencies (fixes syncthing#8679) (syncthing#8680)
  gui, man, authors: Update docs, translations, and contributors
  lib/model: Correctly set xattrs on temp files (fixes syncthing#8667) (syncthing#8670)
  gui: Automatically dismiss authentication reminder when in LDAP mode (fixes syncthing#8661) (syncthing#8663)
  lib/model: Correctly handle xattrs on directories (fixes syncthing#8657) (syncthing#8658)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  ...
@calmh calmh deleted the winbuildhax branch February 22, 2023 10:17
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Nov 5, 2023
@syncthing syncthing locked and limited conversation to collaborators Nov 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants