Skip to content

Commit 18f147a

Browse files
mtojekmtojek
committed
Merge branch 'main' into 9470-warnings
2 parents 98bd84c + 24aa223 commit 18f147a

File tree

103 files changed

+1579
-833
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

103 files changed

+1579
-833
lines changed

‎.vscode/settings.json

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,6 @@
205205
"files.insertFinalNewline": true,
206206
"go.lintTool": "golangci-lint",
207207
"go.lintFlags": ["--fast"],
208-
"go.lintOnSave": "package",
209-
"go.coverOnSave": true,
210208
"go.coverageDecorator": {
211209
"type": "gutter",
212210
"coveredGutterStyle": "blockgreen",

‎cli/clibase/cmd.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,15 @@ func (inv *Invocation) SignalNotifyContext(parent context.Context, signals ...os
226226
return inv.signalNotifyContext(parent, signals...)
227227
}
228228

229+
func (inv *Invocation) WithTestParsedFlags(
230+
_ testing.TB, // ensure we only call this from tests
231+
parsedFlags *pflag.FlagSet,
232+
) *Invocation {
233+
return inv.with(func(i *Invocation) {
234+
i.parsedFlags = parsedFlags
235+
})
236+
}
237+
229238
func (inv *Invocation) Context() context.Context {
230239
if inv.ctx == nil {
231240
return context.Background()

‎cli/server.go

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2319,12 +2319,7 @@ func ConfigureHTTPServers(logger slog.Logger, inv *clibase.Invocation, cfg *code
23192319
return nil, xerrors.New("tls address must be set if tls is enabled")
23202320
}
23212321

2322-
// DEPRECATED: This redirect used to default to true.
2323-
// It made more sense to have the redirect be opt-in.
2324-
if inv.Environ.Get("CODER_TLS_REDIRECT_HTTP") == "true" || inv.ParsedFlags().Changed("tls-redirect-http-to-https") {
2325-
logger.Warn(ctx, "--tls-redirect-http-to-https is deprecated, please use --redirect-to-access-url instead")
2326-
cfg.RedirectToAccessURL = cfg.TLS.RedirectHTTP
2327-
}
2322+
redirectHTTPToHTTPSDeprecation(ctx, logger, inv, cfg)
23282323

23292324
tlsConfig, err := configureServerTLS(
23302325
ctx,
@@ -2374,6 +2369,31 @@ func ConfigureHTTPServers(logger slog.Logger, inv *clibase.Invocation, cfg *code
23742369
return httpServers, nil
23752370
}
23762371

2372+
// redirectHTTPToHTTPSDeprecation handles deprecation of the --tls-redirect-http-to-https flag and
2373+
// "related" environment variables.
2374+
//
2375+
// --tls-redirect-http-to-https used to default to true.
2376+
// It made more sense to have the redirect be opt-in.
2377+
//
2378+
// Also, for a while we have been accepting the environment variable (but not the
2379+
// corresponding flag!) "CODER_TLS_REDIRECT_HTTP", and it appeared in a configuration
2380+
// example, so we keep accepting it to not break backward compat.
2381+
func redirectHTTPToHTTPSDeprecation(ctx context.Context, logger slog.Logger, inv *clibase.Invocation, cfg *codersdk.DeploymentValues) {
2382+
truthy := func(s string) bool {
2383+
b, err := strconv.ParseBool(s)
2384+
if err != nil {
2385+
return false
2386+
}
2387+
return b
2388+
}
2389+
if truthy(inv.Environ.Get("CODER_TLS_REDIRECT_HTTP")) ||
2390+
truthy(inv.Environ.Get("CODER_TLS_REDIRECT_HTTP_TO_HTTPS")) ||
2391+
inv.ParsedFlags().Changed("tls-redirect-http-to-https") {
2392+
logger.Warn(ctx, "⚠️ --tls-redirect-http-to-https is deprecated, please use --redirect-to-access-url instead")
2393+
cfg.RedirectToAccessURL = cfg.TLS.RedirectHTTP
2394+
}
2395+
}
2396+
23772397
// ReadExternalAuthProvidersFromEnv is provided for compatibility purposes with
23782398
// the viper CLI.
23792399
func ReadExternalAuthProvidersFromEnv(environ []string) ([]codersdk.ExternalAuthConfig, error) {

‎cli/server_internal_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,17 @@ import (
66
"crypto/tls"
77
"testing"
88

9+
"github.com/spf13/pflag"
910
"github.com/stretchr/testify/assert"
1011
"github.com/stretchr/testify/require"
1112

1213
"cdr.dev/slog"
1314
"cdr.dev/slog/sloggers/sloghuman"
15+
"cdr.dev/slog/sloggers/slogtest"
16+
17+
"github.com/coder/coder/v2/cli/clibase"
18+
"github.com/coder/coder/v2/codersdk"
19+
"github.com/coder/coder/v2/testutil"
1420
)
1521

1622
func Test_configureCipherSuites(t *testing.T) {
@@ -169,3 +175,71 @@ func Test_configureCipherSuites(t *testing.T) {
169175
})
170176
}
171177
}
178+
179+
func TestRedirectHTTPToHTTPSDeprecation(t *testing.T) {
180+
t.Parallel()
181+
182+
testcases := []struct {
183+
name string
184+
environ clibase.Environ
185+
flags []string
186+
expected bool
187+
}{
188+
{
189+
name: "AllUnset",
190+
environ: clibase.Environ{},
191+
flags: []string{},
192+
expected: false,
193+
},
194+
{
195+
name: "CODER_TLS_REDIRECT_HTTP=true",
196+
environ: clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP", Value: "true"}},
197+
flags: []string{},
198+
expected: true,
199+
},
200+
{
201+
name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS=true",
202+
environ: clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS", Value: "true"}},
203+
flags: []string{},
204+
expected: true,
205+
},
206+
{
207+
name: "CODER_TLS_REDIRECT_HTTP=false",
208+
environ: clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP", Value: "false"}},
209+
flags: []string{},
210+
expected: false,
211+
},
212+
{
213+
name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS=false",
214+
environ: clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS", Value: "false"}},
215+
flags: []string{},
216+
expected: false,
217+
},
218+
{
219+
name: "--tls-redirect-http-to-https",
220+
environ: clibase.Environ{},
221+
flags: []string{"--tls-redirect-http-to-https"},
222+
expected: true,
223+
},
224+
}
225+
226+
for _, tc := range testcases {
227+
tc := tc
228+
t.Run(tc.name, func(t *testing.T) {
229+
t.Parallel()
230+
ctx := testutil.Context(t, testutil.WaitShort)
231+
logger := slogtest.Make(t, nil)
232+
flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
233+
_ = flags.Bool("tls-redirect-http-to-https", true, "")
234+
err := flags.Parse(tc.flags)
235+
require.NoError(t, err)
236+
inv := (&clibase.Invocation{Environ: tc.environ}).WithTestParsedFlags(t, flags)
237+
cfg := &codersdk.DeploymentValues{}
238+
opts := cfg.Options()
239+
err = opts.SetDefaults()
240+
require.NoError(t, err)
241+
redirectHTTPToHTTPSDeprecation(ctx, logger, inv, cfg)
242+
require.Equal(t, tc.expected, cfg.RedirectToAccessURL.Value())
243+
})
244+
}
245+
}

‎cli/ssh.go

Lines changed: 129 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
gosshagent "golang.org/x/crypto/ssh/agent"
2323
"golang.org/x/term"
2424
"golang.org/x/xerrors"
25+
"gvisor.dev/gvisor/pkg/tcpip/adapters/gonet"
2526

2627
"cdr.dev/slog"
2728
"cdr.dev/slog/sloggers/sloghuman"
@@ -129,6 +130,8 @@ func (r *RootCmd) ssh() *clibase.Cmd {
129130
// log HTTP requests
130131
client.SetLogger(logger)
131132
}
133+
stack := newCloserStack(ctx, logger)
134+
defer stack.close(nil)
132135

133136
if remoteForward != "" {
134137
isValid := validateRemoteForward(remoteForward)
@@ -212,7 +215,9 @@ func (r *RootCmd) ssh() *clibase.Cmd {
212215
if err != nil {
213216
return xerrors.Errorf("dial agent: %w", err)
214217
}
215-
defer conn.Close()
218+
if err = stack.push("agent conn", conn); err != nil {
219+
return err
220+
}
216221
conn.AwaitReachable(ctx)
217222

218223
stopPolling := tryPollWorkspaceAutostop(ctx, client, workspace)
@@ -223,61 +228,46 @@ func (r *RootCmd) ssh() *clibase.Cmd {
223228
if err != nil {
224229
return xerrors.Errorf("connect SSH: %w", err)
225230
}
226-
defer rawSSH.Close()
231+
copier := &rawSSHCopier{conn: rawSSH, r: inv.Stdin, w: inv.Stdout}
232+
if err = stack.push("rawSSHCopier", copier); err != nil {
233+
return err
234+
}
227235

228236
wg.Add(1)
229237
go func() {
230238
defer wg.Done()
231239
watchAndClose(ctx, func() error {
232-
return rawSSH.Close()
240+
stack.close(xerrors.New("watchAndClose"))
241+
return nil
233242
}, logger, client, workspace)
234243
}()
235-
236-
wg.Add(1)
237-
go func() {
238-
defer wg.Done()
239-
// Ensure stdout copy closes incase stdin is closed
240-
// unexpectedly.
241-
defer rawSSH.Close()
242-
243-
_, err := io.Copy(rawSSH, inv.Stdin)
244-
if err != nil {
245-
logger.Error(ctx, "copy stdin error", slog.Error(err))
246-
} else {
247-
logger.Debug(ctx, "copy stdin complete")
248-
}
249-
}()
250-
_, err = io.Copy(inv.Stdout, rawSSH)
251-
if err != nil {
252-
logger.Error(ctx, "copy stdout error", slog.Error(err))
253-
} else {
254-
logger.Debug(ctx, "copy stdout complete")
255-
}
244+
copier.copy(&wg)
256245
return nil
257246
}
258247

259248
sshClient, err := conn.SSHClient(ctx)
260249
if err != nil {
261250
return xerrors.Errorf("ssh client: %w", err)
262251
}
263-
defer sshClient.Close()
252+
if err = stack.push("ssh client", sshClient); err != nil {
253+
return err
254+
}
264255

265256
sshSession, err := sshClient.NewSession()
266257
if err != nil {
267258
return xerrors.Errorf("ssh session: %w", err)
268259
}
269-
defer sshSession.Close()
260+
if err = stack.push("sshSession", sshSession); err != nil {
261+
return err
262+
}
270263

271264
wg.Add(1)
272265
go func() {
273266
defer wg.Done()
274267
watchAndClose(
275268
ctx,
276269
func() error {
277-
err := sshSession.Close()
278-
logger.Debug(ctx, "session close", slog.Error(err))
279-
err = sshClient.Close()
280-
logger.Debug(ctx, "client close", slog.Error(err))
270+
stack.close(xerrors.New("watchAndClose"))
281271
return nil
282272
},
283273
logger,
@@ -313,7 +303,9 @@ func (r *RootCmd) ssh() *clibase.Cmd {
313303
if err != nil {
314304
return xerrors.Errorf("forward GPG socket: %w", err)
315305
}
316-
defer closer.Close()
306+
if err = stack.push("forwardGPGAgent", closer); err != nil {
307+
return err
308+
}
317309
}
318310

319311
if remoteForward != "" {
@@ -326,7 +318,9 @@ func (r *RootCmd) ssh() *clibase.Cmd {
326318
if err != nil {
327319
return xerrors.Errorf("ssh remote forward: %w", err)
328320
}
329-
defer closer.Close()
321+
if err = stack.push("sshRemoteForward", closer); err != nil {
322+
return err
323+
}
330324
}
331325

332326
stdoutFile, validOut := inv.Stdout.(*os.File)
@@ -795,3 +789,106 @@ func remoteGPGAgentSocket(sshClient *gossh.Client) (string, error) {
795789

796790
return string(bytes.TrimSpace(remoteSocket)), nil
797791
}
792+
793+
type closerWithName struct {
794+
name string
795+
closer io.Closer
796+
}
797+
798+
type closerStack struct {
799+
sync.Mutex
800+
closers []closerWithName
801+
closed bool
802+
logger slog.Logger
803+
err error
804+
}
805+
806+
func newCloserStack(ctx context.Context, logger slog.Logger) *closerStack {
807+
cs := &closerStack{logger: logger}
808+
go cs.closeAfterContext(ctx)
809+
return cs
810+
}
811+
812+
func (c *closerStack) closeAfterContext(ctx context.Context) {
813+
<-ctx.Done()
814+
c.close(ctx.Err())
815+
}
816+
817+
func (c *closerStack) close(err error) {
818+
c.Lock()
819+
if c.closed {
820+
c.Unlock()
821+
return
822+
}
823+
c.closed = true
824+
c.err = err
825+
c.Unlock()
826+
827+
for i := len(c.closers) - 1; i >= 0; i-- {
828+
cwn := c.closers[i]
829+
cErr := cwn.closer.Close()
830+
c.logger.Debug(context.Background(),
831+
"closed item from stack", slog.F("name", cwn.name), slog.Error(cErr))
832+
}
833+
}
834+
835+
func (c *closerStack) push(name string, closer io.Closer) error {
836+
c.Lock()
837+
if c.closed {
838+
c.Unlock()
839+
// since we're refusing to push it on the stack, close it now
840+
err := closer.Close()
841+
c.logger.Error(context.Background(),
842+
"closed item rejected push", slog.F("name", name), slog.Error(err))
843+
return xerrors.Errorf("already closed: %w", c.err)
844+
}
845+
c.closers = append(c.closers, closerWithName{name: name, closer: closer})
846+
c.Unlock()
847+
return nil
848+
}
849+
850+
// rawSSHCopier handles copying raw SSH data between the conn and the pair (r, w).
851+
type rawSSHCopier struct {
852+
conn *gonet.TCPConn
853+
logger slog.Logger
854+
r io.Reader
855+
w io.Writer
856+
}
857+
858+
func (c *rawSSHCopier) copy(wg *sync.WaitGroup) {
859+
logCtx := context.Background()
860+
wg.Add(1)
861+
go func() {
862+
defer wg.Done()
863+
// We close connections using CloseWrite instead of Close, so that the SSH server sees the
864+
// closed connection while reading, and shuts down cleanly. This will trigger the io.Copy
865+
// in the server-to-client direction to also be closed and the copy() routine will exit.
866+
// This ensures that we don't leave any state in the server, like forwarded ports if
867+
// copy() were to return and the underlying tailnet connection torn down before the TCP
868+
// session exits. This is a bit of a hack to block shut down at the application layer, since
869+
// we can't serialize the TCP and tailnet layers shutting down.
870+
//
871+
// Of course, if the underlying transport is broken, io.Copy will still return.
872+
defer func() {
873+
cwErr := c.conn.CloseWrite()
874+
c.logger.Debug(logCtx, "closed raw SSH connection for writing", slog.Error(cwErr))
875+
}()
876+
877+
_, err := io.Copy(c.conn, c.r)
878+
if err != nil {
879+
c.logger.Error(logCtx, "copy stdin error", slog.Error(err))
880+
} else {
881+
c.logger.Debug(logCtx, "copy stdin complete")
882+
}
883+
}()
884+
_, err := io.Copy(c.w, c.conn)
885+
if err != nil {
886+
c.logger.Error(logCtx, "copy stdout error", slog.Error(err))
887+
} else {
888+
c.logger.Debug(logCtx, "copy stdout complete")
889+
}
890+
}
891+
892+
func (c *rawSSHCopier) Close() error {
893+
return c.conn.CloseWrite()
894+
}

0 commit comments

Comments
 (0)