From f48a706abc1645c53fdceff0d6e46cd33a31770a Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Wed, 18 Oct 2023 20:59:47 +0200 Subject: [PATCH] remote: exec: do not leak session IDs on errors commit fa19e1baa27024f8e0078e27254a8cfb6586f9f4 partially introduced the fix, but was merged too quickly and didn't work with remote. Introduce a new binding to allow removing a session from the remote client. Signed-off-by: Giuseppe Scrivano --- pkg/api/handlers/compat/exec.go | 27 +++++++++++++ pkg/api/handlers/types.go | 4 ++ pkg/api/server/register_exec.go | 34 +++++++++++++++++ pkg/bindings/containers/exec.go | 38 +++++++++++++++++++ pkg/bindings/containers/types.go | 7 ++++ .../containers/types_execremove_options.go | 33 ++++++++++++++++ pkg/domain/infra/tunnel/containers.go | 17 ++++++++- 7 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 pkg/bindings/containers/types_execremove_options.go diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go index 0a5d075356..819a096ef1 100644 --- a/pkg/api/handlers/compat/exec.go +++ b/pkg/api/handlers/compat/exec.go @@ -209,3 +209,30 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) { } logrus.Debugf("Attach for container %s exec session %s completed successfully", sessionCtr.ID(), sessionID) } + +// ExecRemoveHandler removes a exec session. +func ExecRemoveHandler(w http.ResponseWriter, r *http.Request) { + runtime := r.Context().Value(api.RuntimeKey).(*libpod.Runtime) + + sessionID := mux.Vars(r)["id"] + + bodyParams := new(handlers.ExecRemoveConfig) + + if err := json.NewDecoder(r.Body).Decode(&bodyParams); err != nil { + utils.Error(w, http.StatusBadRequest, fmt.Errorf("failed to decode parameters for %s: %w", r.URL.String(), err)) + return + } + + sessionCtr, err := runtime.GetExecSessionContainer(sessionID) + if err != nil { + utils.Error(w, http.StatusNotFound, err) + return + } + + logrus.Debugf("Removing exec session %s of container %s", sessionID, sessionCtr.ID()) + if err := sessionCtr.ExecRemove(sessionID, bodyParams.Force); err != nil { + utils.InternalServerError(w, err) + return + } + logrus.Debugf("Removing exec session %s for container %s completed successfully", sessionID, sessionCtr.ID()) +} diff --git a/pkg/api/handlers/types.go b/pkg/api/handlers/types.go index b8059bafb1..1a14628c61 100644 --- a/pkg/api/handlers/types.go +++ b/pkg/api/handlers/types.go @@ -161,3 +161,7 @@ type ExecStartConfig struct { Height uint16 `json:"h"` Width uint16 `json:"w"` } + +type ExecRemoveConfig struct { + Force bool `json:"Force"` +} diff --git a/pkg/api/server/register_exec.go b/pkg/api/server/register_exec.go index 34a9171a3e..529c2465eb 100644 --- a/pkg/api/server/register_exec.go +++ b/pkg/api/server/register_exec.go @@ -346,5 +346,39 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error { // 500: // $ref: "#/responses/internalError" r.Handle(VersionedPath("/libpod/exec/{id}/json"), s.APIHandler(compat.ExecInspectHandler)).Methods(http.MethodGet) + // ................. .... ........................ ...... ExecRemoveLibpod + // --- + // tags: + // - exec + // summary: Remove an exec instance + // description: | + // Remove a previously set up exec instance. If force is true, the exec session is killed if it is still running. + // parameters: + // - in: path + // name: id + // type: string + // required: true + // description: Exec instance ID + // - in: body + // name: control + // description: Attributes for removal + // schema: + // type: object + // properties: + // Force: + // type: boolean + // description: Force removal of the session. + // produces: + // - application/json + // responses: + // 200: + // description: no error + // 404: + // $ref: "#/responses/execSessionNotFound" + // 409: + // description: container is not running. + // 500: + // $ref: "#/responses/internalError" + r.Handle(VersionedPath("/libpod/exec/{id}/remove"), s.APIHandler(compat.ExecRemoveHandler)).Methods(http.MethodPost) return nil } diff --git a/pkg/bindings/containers/exec.go b/pkg/bindings/containers/exec.go index c5c8760ed8..9a73a656ea 100644 --- a/pkg/bindings/containers/exec.go +++ b/pkg/bindings/containers/exec.go @@ -111,3 +111,41 @@ func ExecStart(ctx context.Context, sessionID string, options *ExecStartOptions) return resp.Process(nil) } + +// ExecRemove removes a given exec session. +func ExecRemove(ctx context.Context, sessionID string, options *ExecRemoveOptions) error { + if options == nil { + options = new(ExecRemoveOptions) + } + _ = options + conn, err := bindings.GetClient(ctx) + if err != nil { + return err + } + + logrus.Debugf("Removing exec session ID %q", sessionID) + + // We force Detach to true + body := struct { + Force bool `json:"Force"` + }{ + Force: false, + } + + if options.Force != nil { + body.Force = *options.Force + } + + bodyJSON, err := json.Marshal(body) + if err != nil { + return err + } + + resp, err := conn.DoRequest(ctx, bytes.NewReader(bodyJSON), http.MethodPost, "/exec/%s/remove", nil, nil, sessionID) + if err != nil { + return err + } + defer resp.Body.Close() + + return resp.Process(nil) +} diff --git a/pkg/bindings/containers/types.go b/pkg/bindings/containers/types.go index 3682e950ee..6275daf63d 100644 --- a/pkg/bindings/containers/types.go +++ b/pkg/bindings/containers/types.go @@ -333,3 +333,10 @@ type CopyOptions struct { // by the other type. NoOverwriteDirNonDir *bool } + +// ExecRemoveOptions are optional options for removing an exec session +// +//go:generate go run ../generator/generator.go ExecRemoveOptions +type ExecRemoveOptions struct { + Force *bool +} diff --git a/pkg/bindings/containers/types_execremove_options.go b/pkg/bindings/containers/types_execremove_options.go new file mode 100644 index 0000000000..3dcebe3007 --- /dev/null +++ b/pkg/bindings/containers/types_execremove_options.go @@ -0,0 +1,33 @@ +// Code generated by go generate; DO NOT EDIT. +package containers + +import ( + "net/url" + + "github.com/containers/podman/v4/pkg/bindings/internal/util" +) + +// Changed returns true if named field has been set +func (o *ExecRemoveOptions) Changed(fieldName string) bool { + return util.Changed(o, fieldName) +} + +// ToParams formats struct fields to be passed to API service +func (o *ExecRemoveOptions) ToParams() (url.Values, error) { + return util.ToParams(o) +} + +// WithForce set field Force to given value +func (o *ExecRemoveOptions) WithForce(value bool) *ExecRemoveOptions { + o.Force = &value + return o +} + +// GetForce returns value of field Force +func (o *ExecRemoveOptions) GetForce() bool { + if o.Force == nil { + var z bool + return z + } + return *o.Force +} diff --git a/pkg/domain/infra/tunnel/containers.go b/pkg/domain/infra/tunnel/containers.go index d61fa18531..008bb39fde 100644 --- a/pkg/domain/infra/tunnel/containers.go +++ b/pkg/domain/infra/tunnel/containers.go @@ -579,13 +579,21 @@ func makeExecConfig(options entities.ExecOptions) *handlers.ExecCreateConfig { return createConfig } -func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrID string, options entities.ExecOptions, streams define.AttachStreams) (int, error) { +func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrID string, options entities.ExecOptions, streams define.AttachStreams) (exitCode int, retErr error) { createConfig := makeExecConfig(options) sessionID, err := containers.ExecCreate(ic.ClientCtx, nameOrID, createConfig) if err != nil { return 125, err } + defer func() { + if err := containers.ExecRemove(ic.ClientCtx, sessionID, nil); err != nil { + if retErr == nil { + exitCode = -1 + retErr = err + } + } + }() startAndAttachOptions := new(containers.ExecStartAndAttachOptions) startAndAttachOptions.WithOutputStream(streams.OutputStream).WithErrorStream(streams.ErrorStream) if streams.InputStream != nil { @@ -604,13 +612,18 @@ func (ic *ContainerEngine) ContainerExec(ctx context.Context, nameOrID string, o return inspectOut.ExitCode, nil } -func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrID string, options entities.ExecOptions) (string, error) { +func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrID string, options entities.ExecOptions) (retSessionID string, retErr error) { createConfig := makeExecConfig(options) sessionID, err := containers.ExecCreate(ic.ClientCtx, nameOrID, createConfig) if err != nil { return "", err } + defer func() { + if retErr != nil { + _ = containers.ExecRemove(ic.ClientCtx, sessionID, nil) + } + }() if err := containers.ExecStart(ic.ClientCtx, sessionID, nil); err != nil { return "", err