Add --verify/--no-verify to swiftly init

[adityasingh2400]

On Linux, when gpg is not installed, the prerequisite check tells the user "To skip signature verification, specify the --no-verify flag", but swiftly init never declared that flag, so passing it failed with "Unknown option '--no-verify'". This adds the same --verify/--no-verify flag that install and update already have to init, and threads it into the toolchain installation that init performs, so the advice in the error message now works. The confirmation prompt also stops promising to import swift.org GnuPG keys when verification is turned off, and the generated CLI reference was regenerated with the plugin. Verified with a new case in InitTests that parses --no-verify, which fails on main and passes here, plus the existing InitTests suite.

Fixes #576

[etcwilde]

Thanks for looking into this. I'm not sure how much that test is verifying beyond that ArgumentParser is working, but I can't come up with a better test since it doesn't look like we have a test for this in the install tests either.

[adityasingh2400]

Thanks for the review, and that is a fair reading of the test.

You are right that it is mostly exercising ArgumentParser. The narrow thing it does pin is the bit the bug was about: the gpg prerequisite check tells users to run swiftly init --no-verify, and before this that flag did not exist. Since --no-verify only appears because of inversion: .prefixedNo, dropping or changing that declaration would make the suggested command fail again, and the test catches exactly that along with the default staying true.

What it does not cover is the part I would actually want covered, that verifySignature: false reaches Install.execute and no keys get imported. I could not see a way to assert that without a real install, which matches your point about there being no coverage in the install tests either. Happy to leave it as is, or to drop the test entirely if you would rather not carry something that thin.

[adityasingh2400]

Flagging the Test (Self Hosted) / ubuntu:22.04 red that appeared after your approval, since I do not think it is this branch and I did not want it to stall the PR quietly.

The job does not fail an assertion, the test binary dies with unexpected signal code 11. It gets there after 103 tests have already passed, and initAcceptsNoVerify(), the only test this PR adds, is among them:

✔ Test initAcceptsNoVerify() passed after 1.138 seconds.

The other platforms on this same commit are green:

job	result
macOS Tahoe ARM64	pass
ubuntu:24.04	pass
redhat/ubi9	pass
ubuntu:22.04	SIGSEGV

A change to an ArgumentParser flag declaration would not be platform specific, and it would not segfault a test binary after the parse test has already passed. The crash frames are in libswiftCore and libTesting rather than anything from swiftly.

I have left it alone rather than pushing at it. Happy to re-run if that helps, or to dig further if this is not a known flake on that image.

[etcwilde]

Yeah, there's a bug in Foundation that can sometimes cause issues in process launches. From what I remember, that's in the test harness itself and not in the Swiftly binary. Swiftly generally uses swift-subprocess.

[adityasingh2400]

That is good to know, thanks. The harness rather than the binary also fits what the log shows, the crash frames are in libTesting and libswiftCore with nothing from swiftly in them.

Leaving it alone then. Nothing outstanding from my side on this one.

[etcwilde]

@cmcgee1024, do you know if there is a way to observe whether a toolchain was verified or not? I'm looking at install, and it looks like the only thing is whether it failed to verify and then the install command fails. Trying to find a better way to verify that the flag is having the intended behavior.

[adityasingh2400]

@etcwilde there are two observables already in the tree, and neither needs a production change.

The direct one is console output. Linux.verifyToolchainSignature emits an unconditional message partway through, not gated on verbose:

// Sources/LinuxPlatform/Linux.swift:485
await ctx.message("Verifying toolchain signature...")

SwiftlyTests.runWithMockedIO installs a TestOutputHandler on the context and returns every line ctx.message produced, so a test can assert that string is absent for --no-verify and present for the default. That is a single assertion on the array runWithMockedIO already hands back.

The stronger one is the network. Verification makes two HTTP calls through ctx.httpClient, which in tests is MockToolchainDownloader:

// importGpgKeys
try await ctx.httpClient.getGpgKeys().download(to: tmpFile)
// verifyToolchainSignature
try await ctx.httpClient.getSwiftToolchainFileSignature(toolchainFile).download(to: sigFile)

The mock already routes on url.path.hasSuffix(".sig"), so recording the URLs it is asked for would let a test assert that no .sig and no GPG key request is made under --no-verify. That is a Tests-only addition to the mock.

The negative direction is the better assertion of the two. It is deterministic and needs no gpg on the runner, whereas the positive direction actually shells out to gpg with GNUPGHOME pointed at the mocked home.

To set expectations honestly: I read these paths rather than ran them. MacOS.verifyToolchainSignature is an empty function, so on my machine both flag values take the same path and there is nothing to observe. Everything above is Linux-only and I did not want to push a test I could not run onto an approved PR. Say the word and I will add the output-line assertion, which is the smaller of the two, and let CI here be the check.

[adityasingh2400]

@etcwilde your read on this was right, and I can now back it with the actual stack rather than the log tail. The job logs are reachable through gh api repos/swiftlang/swiftly/actions/jobs/<id>/logs, which I had not tried before.

It is a hard crash, not a test failure:

*** Program crashed: Bad pointer dereference at 0x00007fc764250000 ***
error: Process '.../swiftlyPackageTests.xctest --testing-library swift-testing' exited with unexpected signal code 11

Top of the crashing thread:

0                 <unknown> in libc.so.6
1 [ra]            Process.run() + 6428 in libFoundation.so
2 [ra]            MockToolchainDownloader.makeMockedToolchain(toolchain:name:)
                    at Tests/SwiftlyTests/SwiftlyTests.swift:908:18
3 [async]         MockToolchainDownloader.makeToolchainDownloadResponse(from:)
4 [async]         MockToolchainDownloader.getSwiftToolchainFile(_:)

So it is Process.run() inside Foundation, called from the mock toolchain builder in the test harness, which is exactly what you described. Nothing from this change is on the stack. The path is Install.execute to getSwiftToolchainFile to the mock, so it crashes during the fake download, before signature verification is even reached.

Nothing outstanding from me on this one.

[adityasingh2400]

I rebased this onto main and it cost you a click, so flagging it rather than leaving you to find it.

My reasoning was that the red here was a stale 05 August run and that Pull Request had since gone green on main on 06 August, so a fresh run would likely clear it. What I missed is that the old run was attempt 2, meaning someone had already approved it. The rebase moved the head to d2d8365, and the new run is sitting at action_required, so the PR now shows no checks at all rather than a stale failure.

That is worse for you, not better, and I should have checked the run attempt before pushing.

The branch is otherwise unchanged, still the same three files and the same diff, now zero commits behind main. If you approve the run we should get a current signal. If you would rather I had left it alone I am happy to avoid rebasing this one again.

For reference the old failure was a SIGSEGV in the toolchain install tests, getSwiftlyLinuxReleases and friends, with the crash in libc and libdispatch rather than anything under Init.swift. My own test, initAcceptsNoVerify, passed in that same run.

[cmcgee1024]

Overall, this looks good. There are a few more things to consider.

The verify/no-verify currently relate to Swift toolchains, not swiftly itself. That's why it wasn't added to the global options, and used everywhere because the distinction can be important. Swift toolchains don't generally recommend changes and even provide commands that run as root, so installing a malicious one might only infect your user account, but not affect the entire machine. swiftly on the other hand has that potential and in the future it might even make changes to your system in order to streamline the workflow even more.

The side effect is that the authorship of swiftly becomes more important than the toolchains themselves, so skipping that step has a more grave impact. This option should reflect that instead of being folded into the toolchain's option, and give a deeper sense of importance.

Also, if we're skipping swiftly verification, then should this option also apply to self-update?

Maybe as a user you might consider installing gpg onto your system since it's very commonly found in various Linux package managers and gain a much higher level of confidence of both swiftly and the toolchains that they are official and coming from swift.org? swiftly could do better to handle swiftly init more gracefully if it's not present.