Skip to content

Commit 881a662

Browse files
fioan89fioan89
committed
impl: download the cli to a temporary location
The download and signature verification steps are now slightly altered to first download the cli to a temporary location and only after the signature verification is successful or the user accepted the risk of running an unsigned binary - then and only then the temp cli is moved to its final location (actually it is just a rename). If the delete fails, this prevents the unsigned binary from being picked up as the cached binary on the next run.
1 parent 8342a21 commit 881a662

File tree

2 files changed

+110
-79
lines changed

2 files changed

+110
-79
lines changed

‎src/main/kotlin/com/coder/toolbox/cli/CoderCLIManager.kt

Lines changed: 68 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -164,87 +164,88 @@ class CoderCLIManager(
164164
* Download the CLI from the deployment if necessary.
165165
*/
166166
suspend fun download(buildVersion: String, showTextProgress: (String) -> Unit): Boolean {
167-
val cliResult = withContext(Dispatchers.IO) {
168-
downloader.downloadCli(buildVersion, showTextProgress)
169-
}.let { result ->
170-
when {
171-
result.isSkipped() -> return false
172-
result.isNotFound() -> throw IllegalStateException("Could not find Coder CLI")
173-
result.isFailed() -> throw (result as DownloadResult.Failed).error
174-
else -> result as Downloaded
167+
try {
168+
val cliResult = withContext(Dispatchers.IO) {
169+
downloader.downloadCli(buildVersion, showTextProgress)
170+
}.let { result ->
171+
when {
172+
result.isSkipped() -> return false
173+
result.isNotFound() -> throw IllegalStateException("Could not find Coder CLI")
174+
result.isFailed() -> throw (result as DownloadResult.Failed).error
175+
else -> result as Downloaded
176+
}
175177
}
176-
}
177-
178-
var signatureResult = withContext(Dispatchers.IO) {
179-
downloader.downloadSignature(showTextProgress)
180-
}
181178

182-
if (signatureResult.isNotDownloaded()) {
183-
context.logger.info("Trying to download signature file from releases.coder.com")
184-
signatureResult = withContext(Dispatchers.IO) {
185-
downloader.downloadReleasesSignature(buildVersion, showTextProgress)
179+
var signatureResult = withContext(Dispatchers.IO) {
180+
downloader.downloadSignature(showTextProgress)
186181
}
187-
}
188182

189-
// if we could not find any signature and the user wants to explicitly
190-
// confirm whether we run an unsigned cli
191-
if (signatureResult.isNotDownloaded()) {
192-
if (context.settingsStore.allowUnsignedBinaryWithoutPrompt) {
193-
context.logger.warn("Running unsigned CLI from ${cliResult.source}")
194-
return true
195-
} else {
196-
val acceptsUnsignedBinary = context.ui.showYesNoPopup(
197-
context.i18n.ptrl("Security Warning"),
198-
context.i18n.pnotr("Can't verify the integrity of the Coder CLI pulled from ${cliResult.source}"),
199-
context.i18n.ptrl("Accept"),
200-
context.i18n.ptrl("Abort"),
201-
)
183+
if (signatureResult.isNotDownloaded()) {
184+
context.logger.info("Trying to download signature file from releases.coder.com")
185+
signatureResult = withContext(Dispatchers.IO) {
186+
downloader.downloadReleasesSignature(buildVersion, showTextProgress)
187+
}
188+
}
202189

203-
if (acceptsUnsignedBinary) {
190+
// if we could not find any signature and the user wants to explicitly
191+
// confirm whether we run an unsigned cli
192+
if (signatureResult.isNotDownloaded()) {
193+
if (context.settingsStore.allowUnsignedBinaryWithoutPrompt) {
194+
context.logger.warn("Running unsigned CLI from ${cliResult.source}")
195+
downloader.commit()
204196
return true
205197
} else {
206-
// remove the cli, otherwise next time the user tries to login the cached cli is picked up,
207-
// and we don't verify cached cli signatures
208-
Files.delete(cliResult.dst)
209-
throw UnsignedBinaryExecutionDeniedException("Running unsigned CLI from ${cliResult.source} was denied by the user")
198+
val acceptsUnsignedBinary = context.ui.showYesNoPopup(
199+
context.i18n.ptrl("Security Warning"),
200+
context.i18n.pnotr("Can't verify the integrity of the Coder CLI pulled from ${cliResult.source}"),
201+
context.i18n.ptrl("Accept"),
202+
context.i18n.ptrl("Abort"),
203+
)
204+
205+
if (acceptsUnsignedBinary) {
206+
downloader.commit()
207+
return true
208+
} else {
209+
throw UnsignedBinaryExecutionDeniedException("Running unsigned CLI from ${cliResult.source} was denied by the user")
210+
}
210211
}
211212
}
212-
}
213-
214-
// we have the cli, and signature is downloaded, let's verify the signature
215-
signatureResult = signatureResult as Downloaded
216-
gpgVerifier.verifySignature(cliResult.dst, signatureResult.dst).let { result ->
217-
when {
218-
result.isValid() -> return true
219-
else -> {
220-
// remove the cli, otherwise next time the user tries to login the cached cli is picked up,
221-
// and we don't verify cached cli signatures
222-
runCatching { Files.delete(cliResult.dst) }
223-
.onFailure { ex ->
224-
context.logger.warn(ex, "Failed to delete CLI file: ${cliResult.dst}")
225-
}
226213

227-
val exception = when {
228-
result.isInvalid() -> {
229-
val reason = (result as Invalid).reason
230-
UnsignedBinaryExecutionDeniedException(
231-
"Signature of ${cliResult.dst} is invalid." + reason?.let { " Reason: $it" }.orEmpty()
232-
)
233-
}
234-
235-
result.signatureIsNotFound() -> {
236-
UnsignedBinaryExecutionDeniedException(
237-
"Can't verify signature of ${cliResult.dst} because ${signatureResult.dst} does not exist"
238-
)
239-
}
214+
// we have the cli, and signature is downloaded, let's verify the signature
215+
signatureResult = signatureResult as Downloaded
216+
gpgVerifier.verifySignature(cliResult.dst, signatureResult.dst).let { result ->
217+
when {
218+
result.isValid() -> {
219+
downloader.commit()
220+
return true
221+
}
240222

241-
else -> {
242-
UnsignedBinaryExecutionDeniedException((result as Failed).error.message)
223+
else -> {
224+
val exception = when {
225+
result.isInvalid() -> {
226+
val reason = (result as Invalid).reason
227+
UnsignedBinaryExecutionDeniedException(
228+
"Signature of ${cliResult.dst} is invalid." + reason?.let { " Reason: $it" }
229+
.orEmpty()
230+
)
231+
}
232+
233+
result.signatureIsNotFound() -> {
234+
UnsignedBinaryExecutionDeniedException(
235+
"Can't verify signature of ${cliResult.dst} because ${signatureResult.dst} does not exist"
236+
)
237+
}
238+
239+
else -> {
240+
UnsignedBinaryExecutionDeniedException((result as Failed).error.message)
241+
}
243242
}
243+
throw exception
244244
}
245-
throw exception
246245
}
247246
}
247+
} finally {
248+
downloader.cleanup()
248249
}
249250
}
250251

@@ -475,7 +476,7 @@ class CoderCLIManager(
475476
}
476477

477478
else -> {
478-
// An error here most likely means the CLI does not exist or
479+
// An error here most likely means the CLI does not exist, or
479480
// it executed successfully but output no version which
480481
// suggests it is not the right binary.
481482
context.logger.info("Unable to determine $localBinaryPath version: ${e.message}")

‎src/main/kotlin/com/coder/toolbox/cli/downloader/CoderDownloadService.kt

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import com.coder.toolbox.util.SemVer
77
import com.coder.toolbox.util.getHeaders
88
import com.coder.toolbox.util.getOS
99
import com.coder.toolbox.util.sha1
10+
import kotlinx.coroutines.Dispatchers
11+
import kotlinx.coroutines.withContext
1012
import okhttp3.ResponseBody
1113
import retrofit2.Response
1214
import java.io.FileInputStream
@@ -17,6 +19,7 @@ import java.net.URI
1719
import java.net.URL
1820
import java.nio.file.Files
1921
import java.nio.file.Path
22+
import java.nio.file.StandardCopyOption
2023
import java.nio.file.StandardOpenOption
2124
import java.util.zip.GZIPInputStream
2225
import kotlin.io.path.name
@@ -31,13 +34,14 @@ class CoderDownloadService(
3134
private val deploymentUrl: URL,
3235
forceDownloadToData: Boolean,
3336
) {
34-
val remoteBinaryURL: URL = context.settingsStore.binSource(deploymentUrl)
35-
val localBinaryPath: Path = context.settingsStore.binPath(deploymentUrl, forceDownloadToData)
37+
private val remoteBinaryURL: URL = context.settingsStore.binSource(deploymentUrl)
38+
private val cliFinalDst: Path = context.settingsStore.binPath(deploymentUrl, forceDownloadToData)
39+
private val cliTempDst: Path = cliFinalDst.resolveSibling("${cliFinalDst.name}.tmp")
3640

3741
suspend fun downloadCli(buildVersion: String, showTextProgress: (String) -> Unit): DownloadResult {
3842
val eTag = calculateLocalETag()
3943
if (eTag != null) {
40-
context.logger.info("Found existing binary at $localBinaryPath; calculated hash as $eTag")
44+
context.logger.info("Found existing binary at $cliFinalDst; calculated hash as $eTag")
4145
}
4246
val response = downloadApi.downloadCli(
4347
url = remoteBinaryURL.toString(),
@@ -47,13 +51,13 @@ class CoderDownloadService(
4751

4852
return when (response.code()) {
4953
HTTP_OK -> {
50-
context.logger.info("Downloading binary to $localBinaryPath")
51-
response.saveToDisk(localBinaryPath, showTextProgress, buildVersion)?.makeExecutable()
52-
DownloadResult.Downloaded(remoteBinaryURL, localBinaryPath)
54+
context.logger.info("Downloading binary to temporary $cliTempDst")
55+
response.saveToDisk(cliTempDst, showTextProgress, buildVersion)?.makeExecutable()
56+
DownloadResult.Downloaded(remoteBinaryURL, cliTempDst)
5357
}
5458

5559
HTTP_NOT_MODIFIED -> {
56-
context.logger.info("Using cached binary at $localBinaryPath")
60+
context.logger.info("Using cached binary at $cliFinalDst")
5761
showTextProgress("Using cached binary")
5862
DownloadResult.Skipped
5963
}
@@ -67,14 +71,40 @@ class CoderDownloadService(
6771
}
6872
}
6973

74+
/**
75+
* Renames the temporary binary file to its original destination name.
76+
* The implementation will override sibling file that has the original
77+
* destination name.
78+
*/
79+
suspend fun commit(): Path {
80+
return withContext(Dispatchers.IO) {
81+
context.logger.info("Renaming binary from $cliTempDst to $cliFinalDst")
82+
Files.move(cliTempDst, cliFinalDst, StandardCopyOption.REPLACE_EXISTING)
83+
cliFinalDst.makeExecutable()
84+
cliFinalDst
85+
}
86+
}
87+
88+
/**
89+
* Cleans up the temporary binary file if it exists.
90+
*/
91+
suspend fun cleanup() {
92+
withContext(Dispatchers.IO) {
93+
runCatching { Files.deleteIfExists(cliTempDst) }
94+
.onFailure { ex ->
95+
context.logger.warn(ex, "Failed to delete temporary CLI file: $cliTempDst")
96+
}
97+
}
98+
}
99+
70100
private fun calculateLocalETag(): String? {
71101
return try {
72-
if (localBinaryPath.notExists()) {
102+
if (cliFinalDst.notExists()) {
73103
return null
74104
}
75-
sha1(FileInputStream(localBinaryPath.toFile()))
105+
sha1(FileInputStream(cliFinalDst.toFile()))
76106
} catch (e: Exception) {
77-
context.logger.warn(e, "Unable to calculate hash for $localBinaryPath")
107+
context.logger.warn(e, "Unable to calculate hash for $cliFinalDst")
78108
null
79109
}
80110
}
@@ -124,7 +154,7 @@ class CoderDownloadService(
124154
}
125155
}
126156
}
127-
return localBinaryPath
157+
return cliFinalDst
128158
}
129159

130160

@@ -154,7 +184,7 @@ class CoderDownloadService(
154184

155185
private suspend fun downloadSignature(url: URL, showTextProgress: (String) -> Unit): DownloadResult {
156186
val signatureURL = url.toURI().resolve(context.settingsStore.defaultSignatureNameByOsAndArch).toURL()
157-
val localSignaturePath = localBinaryPath.parent.resolve(context.settingsStore.defaultSignatureNameByOsAndArch)
187+
val localSignaturePath = cliFinalDst.parent.resolve(context.settingsStore.defaultSignatureNameByOsAndArch)
158188
context.logger.info("Downloading signature from $signatureURL")
159189

160190
val response = downloadApi.downloadSignature(

0 commit comments

Comments
 (0)