Skip to content

Commit 5149f7d

Browse files
pjcdawkinsclaude
andauthored
fix(installer): vendor CLI installs with CONFIG_URL (#225)
* fix(installer): put vendor CLIs on the PATH, and propagate command failures With CONFIG_URL, install_config now runs config:install with --force and, if the directory of the installed upsun is writable, --bin-dir set to it. Root installs (e.g. in CI) put the vendor executable in /usr/bin next to upsun, instead of ~/.platform-alt/bin, which is not on the PATH. An existing executable with the same name (e.g. from a previous installer) no longer stops the install; if another one comes first in the PATH, a warning names it. The outro names the vendor executable, read from config:install's output, with its full path if it is not found on the PATH. call_user returned the status of indent, so a failed command was never retried with sudo, e.g. moving the binary to /usr/bin as a non-root user. It now returns the command's status. The raw method installs in ~/.local/bin if the default directory is not writable and sudo is not available. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(installer): test CONFIG_URL installs in containers scripts/test/installer-config.sh serves a local build and the test config, and runs installer.sh with CONFIG_URL in Debian and Alpine containers: apt, apk and raw installs as root, raw installs as a non-root user with and without sudo, and an install over an existing executable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(installer): share root installs of vendor CLIs with other users As root, when the vendor executable goes in a system directory such as /usr/bin, its configuration was saved in root's home directory, which other users usually cannot read. The installer now passes --config-dir /etc/platform-alt in that case, e.g. for a Dockerfile that installs as root and then switches to another user. Config auto-updates are skipped when the config's directory is not writable, so other users do not get an error for a config that only root can update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(installer): handle symlinked bin directories and restrictive umasks config:install rejects a symlink as --bin-dir, so a symlinked INSTALL_DIR made the install fail. The directory is now resolved first, and paths are compared by file rather than by name. With a umask such as 077, a shared root install created files that other users could not read or run. The umask is set to 022 for that install, and /etc/platform-alt is made readable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(config): keep a config file's mode when updating it Config updates wrote a new file with mode 0644 filtered by the umask, and renamed it over the old one. Under a restrictive umask, a shared config (e.g. in /etc/platform-alt, updated by root) became unreadable to other users. Updates now keep the existing file's mode, and alt files are written with their mode regardless of the umask. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(installer): put shared vendor configs in /etc/upsun/platform-alt The directory is named after the package that writes it, leaving /etc/upsun for other system-wide settings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix: address review of vendor CLI installs - config:install shortened paths that only share a string prefix with the home directory, e.g. "~usr/bin" for HOME=/. It now requires a path separator after the home directory. - The raw method's fallback to ~/.local/bin no longer applies to a directory in the home directory that does not exist yet, e.g. ~/.global/bin. - The README says the executable is not put next to upsun with Homebrew. - The container tests fail when the installer fails, rather than taking tee's status, and use the Docker host's architecture. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 4c61b37 commit 5149f7d

9 files changed

Lines changed: 350 additions & 7 deletions

File tree

‎README.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,9 @@ The installer is configurable using the following environment variables:
4040
* `INSTALL_METHOD` - force a specific installation method, possible values are `brew` and `raw`
4141
* `INSTALL_DIR` - the installation directory for the `raw` installation method, for example you can use `INSTALL_DIR=$HOME/.local/bin` for a single user installation
4242
* `VERSION` - the version of the CLI to install, if you need a version other than the latest one
43-
* `CONFIG_URL` - the URL of an alternative CLI configuration, installed after the CLI with `upsun config:install`
43+
* `CONFIG_URL` - the URL of an alternative CLI configuration, installed after the CLI with `upsun config:install --force`. Its executable is put next to `upsun` if that directory is writable (except with Homebrew). As root, outside the home directory, the configuration is put in `/etc/upsun/platform-alt` so that other users can run it; only root updates it
44+
45+
The `raw` method installs in `/usr/bin` (`/usr/local/bin` on macOS), or in `~/.global/bin` or `~/.local/bin` if either is in the `PATH`. It uses `sudo` if needed, or `~/.local/bin` if `sudo` is not available.
4446

4547
Outside CI, the `raw` method also installs shell completion for the current user, without editing shell configuration files: bash completion in `~/.local/share/bash-completion/completions` (loaded by bash-completion 2), and fish completion in `~/.config/fish/completions` when available. For zsh users (by `$SHELL`), it installs `~/.local/share/upsun/completion.zsh` and prints the line to add to `~/.zshrc`.
4648

‎commands/config_install.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ func pathFormatter() func(string) string {
154154
}
155155
hd, err := os.UserHomeDir()
156156
return func(p string) string {
157-
if err == nil && strings.HasPrefix(p, hd) {
157+
if err == nil && (p == hd || strings.HasPrefix(p, hd+string(filepath.Separator))) {
158158
return sub + strings.TrimPrefix(p, hd)
159159
}
160160
return p

‎commands/config_install_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http/httptest"
99
"os"
1010
"path/filepath"
11+
"runtime"
1112
"testing"
1213

1314
"github.com/stretchr/testify/assert"
@@ -129,3 +130,25 @@ func TestFindExecutableTarget(t *testing.T) {
129130
assert.Equal(t, other, findExecutableTarget("test-cli-other"))
130131
assert.Equal(t, self, findExecutableTarget("test-cli-missing"))
131132
}
133+
134+
func TestPathFormatter(t *testing.T) {
135+
if runtime.GOOS == "windows" {
136+
t.Skip("uses Unix paths")
137+
}
138+
cases := []struct {
139+
home string
140+
path string
141+
want string
142+
}{
143+
{"/home/u", "/home/u/.local/bin", "~/.local/bin"},
144+
{"/home/u", "/home/u", "~"},
145+
{"/home/u", "/home/user/bin", "/home/user/bin"},
146+
{"/", "/usr/bin", "/usr/bin"},
147+
}
148+
for _, c := range cases {
149+
t.Run(c.home+" "+c.path, func(t *testing.T) {
150+
t.Setenv("HOME", c.home)
151+
assert.Equal(t, c.want, pathFormatter()(c.path))
152+
})
153+
}
154+
}

‎installer.sh‎

Lines changed: 66 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -443,7 +443,12 @@ call_try_user() {
443443
}
444444

445445
call_user() {
446-
sh -c "$1" 2>&1 | indent
446+
# The status is written to a file, as a pipeline returns the status of its last command.
447+
call_user_status=$(mktemp)
448+
{ sh -c "$1" 2>&1; echo $? > "$call_user_status"; } | indent
449+
call_user_code=$(cat "$call_user_status")
450+
rm -f "$call_user_status"
451+
return "$call_user_code"
447452
}
448453

449454
check_shasum() {
@@ -469,6 +474,19 @@ check_directories() {
469474
dir_bin="$HOME/.local/bin"
470475
fi
471476

477+
# Without sudo, install in a user directory if the system one is not writable.
478+
case "${dir_bin}" in
479+
"${HOME:-}"/*) ;;
480+
*)
481+
if [ -z "${INSTALL_DIR}" ] && [ ! -w "${dir_bin}" ] && [ -n "${HOME:-}" ]; then
482+
init_sudo
483+
if ! ${has_sudo}; then
484+
dir_bin="$HOME/.local/bin"
485+
fi
486+
fi
487+
;;
488+
esac
489+
472490
if ! echo $PATH | grep ${dir_bin} > /dev/null; then
473491
run_cmd="${dir_bin}/$binary"
474492

@@ -712,18 +730,63 @@ install_config() {
712730
cmd="$binary"
713731
if [ "raw" = "${INSTALL_METHOD}" ]; then
714732
cmd="${dir_bin}/${binary}"
733+
else
734+
cmd=$(command -v "$binary" || echo "$binary")
735+
fi
736+
737+
# The installer is explicitly installing this config, so it replaces executables with the same name.
738+
set -- -y --force
739+
# Put the new executable next to the CLI, which is on the PATH (or a warning says so).
740+
# Homebrew's directory is left to Homebrew.
741+
# Symlinks are resolved, as --bin-dir only accepts a directory.
742+
cmd_dir=$(cd "$(dirname "$cmd")" && pwd -P)
743+
if [ "homebrew" != "${INSTALL_METHOD}" ] && [ -w "$cmd_dir" ]; then
744+
set -- "$@" --bin-dir "$cmd_dir"
745+
# Share a root install with other users, as root's home is usually private.
746+
case "$cmd_dir" in
747+
"${HOME:-/root}"/*) ;;
748+
*)
749+
if [ "$(id -u)" = "0" ] && mkdir -p /etc/upsun/platform-alt \
750+
&& chmod 755 /etc/upsun /etc/upsun/platform-alt; then
751+
set -- "$@" --config-dir /etc/upsun/platform-alt
752+
umask 022
753+
fi
754+
;;
755+
esac
715756
fi
716757

717758
output_file=$(mktemp)
718-
if "$cmd" config:install -y "${CONFIG_URL}" </dev/null >"$output_file" 2>&1; then
759+
if "$cmd" config:install "$@" "${CONFIG_URL}" </dev/null >"$output_file" 2>&1; then
719760
indent < "$output_file"
720-
rm -f "$output_file"
721761
else
722762
indent < "$output_file"
723763
rm -f "$output_file"
724764
output " could not install the configuration from ${CONFIG_URL}" "error"
725765
exit_with_error
726766
fi
767+
768+
# Read the new executable's path from the output, expanding "~".
769+
exe_path=$(sed -n 's/^ Executable: //p' "$output_file" | head -n 1)
770+
rm -f "$output_file"
771+
case "$exe_path" in
772+
"~"/*) exe_path="${HOME}${exe_path#"~"}" ;;
773+
esac
774+
if [ -z "$exe_path" ]; then
775+
return
776+
fi
777+
778+
exe_name=$(basename "$exe_path")
779+
exe_found=$(command -v "$exe_name" || true)
780+
if [ -n "$exe_found" ] && [ "$exe_found" -ef "$exe_path" ]; then
781+
run_cmd="$exe_name"
782+
return
783+
fi
784+
run_cmd="$exe_path"
785+
if [ -n "$exe_found" ]; then
786+
add_footer_note " ⚠ Another \"${exe_name}\" command comes first in \$PATH: ${exe_found}"
787+
elif [ ! "$(dirname "$exe_path")" -ef "${dir_bin}" ]; then
788+
add_footer_note " ⚠ The directory \"$(dirname "$exe_path")\" is not in \$PATH"
789+
fi
727790
}
728791

729792
install() {

‎internal/config/alt/fs.go‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ func isExistingDirectory(path string) (bool, error) {
165165
return stat.IsDir(), nil
166166
}
167167

168-
// writeFile creates or overwrites a file.
168+
// writeFile creates or overwrites a file, with fileMode regardless of the umask.
169169
// If dirMode is not 0, the containing directory will be created, if it does not already exist.
170170
func writeFile(path string, content []byte, dirMode, fileMode fs.FileMode) error {
171171
if dirMode != 0 {
@@ -178,6 +178,9 @@ func writeFile(path string, content []byte, dirMode, fileMode fs.FileMode) error
178178
if err := os.WriteFile(tmpFile, content, fileMode); err != nil {
179179
return err
180180
}
181+
if err := os.Chmod(tmpFile, fileMode); err != nil {
182+
return err
183+
}
181184

182185
return os.Rename(tmpFile, path)
183186
}

‎internal/config/alt/update.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"os"
7+
"path/filepath"
78
"time"
89

910
"gopkg.in/yaml.v3"
@@ -59,6 +60,12 @@ func Update(ctx context.Context, cnf *config.Config, debugLog func(fmt string, i
5960
}
6061
}()
6162

63+
// A shared config, e.g. installed by root, is left to its owner.
64+
if dir := filepath.Dir(cnf.SourceFile); !isWritableDir(dir) {
65+
debugLog("Config directory is not writable, skipping update: %s", dir)
66+
return nil
67+
}
68+
6269
debugLog("Checking for config updates from URL: %s", cnf.Metadata.URL)
6370
newCnfNode, newCnfStruct, err := FetchConfig(ctx, cnf.Metadata.URL)
6471
if err != nil {
@@ -91,7 +98,7 @@ func Update(ctx context.Context, cnf *config.Config, debugLog func(fmt string, i
9198
return err
9299
}
93100

94-
if err := writeFile(cnf.SourceFile, b, 0, 0o644); err != nil {
101+
if err := writeFile(cnf.SourceFile, b, 0, stat.Mode().Perm()); err != nil {
95102
return err
96103
}
97104
debugLog("Automatically updated config file: %s", cnf.SourceFile)

‎internal/config/alt/update_test.go‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"net/http/httptest"
88
"os"
99
"path/filepath"
10+
"runtime"
1011
"strconv"
1112
"testing"
1213
"time"
@@ -215,3 +216,40 @@ func TestUpdateWithUnusableNewVersion(t *testing.T) {
215216
require.NoError(t, err)
216217
assert.Equal(t, string(localConfig), string(b), "the local config must not be modified")
217218
}
219+
220+
// TestUpdateWithUnwritableDir checks that a shared config, e.g. installed by root, is not updated by other users.
221+
func TestUpdateWithUnwritableDir(t *testing.T) {
222+
if runtime.GOOS == "windows" || os.Geteuid() == 0 {
223+
t.Skip("needs Unix permissions and a non-root user")
224+
}
225+
tempDir := t.TempDir()
226+
configDir := filepath.Join(tempDir, "etc")
227+
require.NoError(t, os.Mkdir(configDir, 0o755))
228+
testConfigFilename := filepath.Join(configDir, "config.yaml")
229+
require.NoError(t, os.WriteFile(testConfigFilename, testConfig, 0o600))
230+
hourAgo := time.Now().Add(-time.Hour)
231+
require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo))
232+
require.NoError(t, os.Chmod(configDir, 0o555))
233+
t.Cleanup(func() { _ = os.Chmod(configDir, 0o755) })
234+
235+
cnf, err := config.FromYAML(testConfig)
236+
require.NoError(t, err)
237+
t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir)
238+
239+
requested := false
240+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
241+
requested = true
242+
_, _ = w.Write(testConfigWithMetadata("{version: 1.0.1}"))
243+
}))
244+
defer server.Close()
245+
246+
cnf.SourceFile = testConfigFilename
247+
cnf.Metadata.URL = server.URL + "/config.yaml"
248+
249+
var lastLogged string
250+
err = alt.Update(config.ToContext(context.Background(), cnf), cnf,
251+
func(msg string, args ...any) { lastLogged = fmt.Sprintf(msg, args...) })
252+
assert.NoError(t, err)
253+
assert.False(t, requested)
254+
assert.Contains(t, lastLogged, "not writable")
255+
}
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
//go:build unix
2+
3+
package alt_test
4+
5+
import (
6+
"context"
7+
"net/http"
8+
"net/http/httptest"
9+
"os"
10+
"path/filepath"
11+
"syscall"
12+
"testing"
13+
"time"
14+
15+
"github.com/stretchr/testify/assert"
16+
"github.com/stretchr/testify/require"
17+
18+
"github.com/upsun/cli/internal/config"
19+
"github.com/upsun/cli/internal/config/alt"
20+
)
21+
22+
// TestUpdateKeepsMode checks that an update keeps a shared config readable under a restrictive umask.
23+
func TestUpdateKeepsMode(t *testing.T) {
24+
tempDir := t.TempDir()
25+
testConfigFilename := filepath.Join(tempDir, "config.yaml")
26+
require.NoError(t, os.WriteFile(testConfigFilename, testConfig, 0o600))
27+
require.NoError(t, os.Chmod(testConfigFilename, 0o644))
28+
hourAgo := time.Now().Add(-time.Hour)
29+
require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo))
30+
31+
cnf, err := config.FromYAML(testConfig)
32+
require.NoError(t, err)
33+
t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir)
34+
35+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
36+
_, _ = w.Write(testConfigWithMetadata("{version: 1.0.1}"))
37+
}))
38+
defer server.Close()
39+
cnf.SourceFile = testConfigFilename
40+
cnf.Metadata.URL = server.URL + "/config.yaml"
41+
42+
oldUmask := syscall.Umask(0o077)
43+
defer syscall.Umask(oldUmask)
44+
45+
require.NoError(t, alt.Update(config.ToContext(context.Background(), cnf), cnf, func(string, ...any) {}))
46+
47+
stat, err := os.Stat(testConfigFilename)
48+
require.NoError(t, err)
49+
assert.Equal(t, os.FileMode(0o644), stat.Mode().Perm())
50+
}

0 commit comments

Comments
 (0)