Sadserver instance on redis instance#459
Conversation
kunrex
force-pushed
the
sadserver-instance
branch
from
2ca6c08 to
9fd775f
Compare
There was a problem hiding this comment.
Pull request overview
This PR shifts “submission” from user-provided flags to instance-based verification via a check.sh script, adds SSH-key injection into spawned instances, and introduces Docker/Docker Compose tooling to run Beast inside a container while still controlling the host Docker daemon.
Changes:
- Replace
/api/submit/challengeflag submission with/api/check/challengethat runscheck.shinside the user’s instance container and awards points on success. - Add instance spawn-time SSH key injection and store a
check.shhash (CheckHash) alongside instance metadata for tamper detection. - Add Docker Compose + container entrypoint/setup flow for running Beast in Docker and add remote “docker exec” support over SSH.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
setup.sh |
Adds container-aware setup path (HOME/config mount + direct beast run in-container). |
pkg/remoteManager/ssh.go |
Adds remote docker exec via SSH to run commands inside containers on remote hosts. |
pkg/cr/containers.go |
Adds ExecResult and a local RunCommandInContainer helper using Docker exec APIs. |
docker-compose.yml |
New compose config to run Beast as a container with host networking + docker.sock bind. |
core/manager/static.go |
Switches some static container mount paths to BEAST_MOUNT_DIR. |
core/manager/pipeline.go |
Switches static mount source path to BEAST_MOUNT_DIR for local host deployments. |
core/manager/instance.go |
Adds check script verification, SSH verification, SSH-key injection, and stores CheckHash on instance creation. |
core/database/challenges.go |
Renames SaveFlagSubmission → SaveChallengeSubmission. |
core/constants.go |
Adds BEAST_MOUNT_DIR, check script constants, and changes SSH_PORT type to uint32. |
core/config/challenge.go |
Adds openssh-server into apt deps (currently unconditionally for non-compose). |
core/cache/instance.go |
Adds CheckHash to cached instance struct and adds (currently duplicated) key helpers/constants. |
api/submit.go |
Removes legacy flag-submission handler and dynamic scoring logic previously located here. |
api/router.go |
Routes /api/submit → /api/check and handler submitFlagHandler → checkFlagHandler. |
api/response.go |
Renames FlagSubmitResp → ChallengeSubmitResponse. |
api/instance.go |
Passes user SSH key into SpawnInstance. |
api/check.go |
New endpoint implementing instance-based check.sh verification + scoring. |
api/auth.go |
Adds SSH public key parsing/normalization during registration. |
Makefile |
Adds make up/down/logs helpers for Docker Compose-based deployment. |
Dockerfile.app |
Adds a container image build for the Beast app with Docker CLI + entrypoint to setup.sh. |
Comments suppressed due to low confidence (2)
pkg/remoteManager/ssh.go:14
- Import block is not gofmt’d (stdlib imports should be grouped/sorted separately from module imports). CI enforces gofmt -s, so this will fail formatting checks unless gofmt is run on the file.
import (
"errors"
"fmt"
"github.com/sdslabs/beastv4/pkg/cr"
"io/ioutil"
"strings"
"sync"
"github.com/sdslabs/beastv4/core/config"
log "github.com/sirupsen/logrus"
"golang.org/x/crypto/ssh"
)
api/auth.go:17
- Import block is not gofmt’d (third-party import golang.org/x/crypto/ssh is mixed into the stdlib group). Since CI enforces gofmt -s, run gofmt on this file to fix import grouping/order.
import (
"errors"
"golang.org/x/crypto/ssh"
"log"
"net/http"
"strings"
"github.com/gin-gonic/gin"
"github.com/sdslabs/beastv4/core"
"github.com/sdslabs/beastv4/core/config"
"github.com/sdslabs/beastv4/core/database"
coreUtils "github.com/sdslabs/beastv4/core/utils"
"github.com/sdslabs/beastv4/pkg/auth"
"gorm.io/gorm"
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import ( | ||
| "fmt" | ||
| "github.com/sdslabs/beastv4/core/cache" | ||
| "github.com/sdslabs/beastv4/core/config" | ||
| "github.com/sdslabs/beastv4/pkg/cr" | ||
| "github.com/sdslabs/beastv4/pkg/remoteManager" | ||
| "net/http" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/gin-gonic/gin" | ||
| "github.com/sdslabs/beastv4/core" | ||
| "github.com/sdslabs/beastv4/core/database" | ||
| coreUtils "github.com/sdslabs/beastv4/core/utils" | ||
| ) |
There was a problem hiding this comment.
Import block is not gofmt’d (stdlib imports should be grouped/sorted separately from module imports). CI enforces gofmt -s, so this will fail formatting checks unless gofmt is run on the file.
| return result, fmt.Errorf("failed to create session: %s", err) | ||
| } | ||
| session, err := client.NewSession() | ||
| if err != nil { | ||
| return result, fmt.Errorf("failed to create session: %s", err) | ||
| } | ||
| defer session.Close() | ||
| defer client.Close() | ||
|
|
||
| cmd = strings.ReplaceAll(cmd, `'`, `'\''`) | ||
|
|
||
| dockerCmd := fmt.Sprintf("docker exec %s sh -c '%s'", containerId, cmd) | ||
| output, err := session.CombinedOutput(dockerCmd) | ||
| if err != nil { | ||
| var exitErr *ssh.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| result.ExitCode = exitErr.ExitStatus() | ||
| } | ||
| } | ||
|
|
||
| result.Output = string(output) | ||
|
|
There was a problem hiding this comment.
RunCommandInContainerOnServer swallows command execution errors: it always returns a nil error, initializes ExitCode to 0, and only sets ExitCode when err is an ssh.ExitError. For non-ExitError failures (e.g., docker not present, command not found, SSH issues), callers may see ExitCode==0 and treat it as success. Also, if client.NewSession() fails, the SSH client isn’t closed. Return the CombinedOutput error (or set a non-zero ExitCode on any error) and ensure client is closed on all failure paths; also fix the "failed to create session" message when CreateSSHClient fails.
| return result, fmt.Errorf("failed to create session: %s", err) | |
| } | |
| session, err := client.NewSession() | |
| if err != nil { | |
| return result, fmt.Errorf("failed to create session: %s", err) | |
| } | |
| defer session.Close() | |
| defer client.Close() | |
| cmd = strings.ReplaceAll(cmd, `'`, `'\''`) | |
| dockerCmd := fmt.Sprintf("docker exec %s sh -c '%s'", containerId, cmd) | |
| output, err := session.CombinedOutput(dockerCmd) | |
| if err != nil { | |
| var exitErr *ssh.ExitError | |
| if errors.As(err, &exitErr) { | |
| result.ExitCode = exitErr.ExitStatus() | |
| } | |
| } | |
| result.Output = string(output) | |
| return result, fmt.Errorf("failed to create SSH client: %w", err) | |
| } | |
| defer client.Close() | |
| session, err := client.NewSession() | |
| if err != nil { | |
| return result, fmt.Errorf("failed to create session: %w", err) | |
| } | |
| defer session.Close() | |
| cmd = strings.ReplaceAll(cmd, `'`, `'\''`) | |
| dockerCmd := fmt.Sprintf("docker exec %s sh -c '%s'", containerId, cmd) | |
| output, err := session.CombinedOutput(dockerCmd) | |
| result.Output = string(output) | |
| if err != nil { | |
| var exitErr *ssh.ExitError | |
| if errors.As(err, &exitErr) { | |
| result.ExitCode = exitErr.ExitStatus() | |
| } else { | |
| result.ExitCode = 1 | |
| } | |
| log.Debugln(fmt.Sprintf("Command output for cmd %s in container %s on host %s: %s", cmd, containerId, server.Host, result.Output)) | |
| log.Debugln(fmt.Sprintf("Exit Code for cmd %s in container %s on host %s: %v", cmd, containerId, server.Host, result.ExitCode)) | |
| return result, err | |
| } |
| const ( | ||
| InstanceKeyPrefix = "beast:instance:" | ||
| UserInstanceKeyPrefix = "beast:user_instance:" | ||
| InstancesSetKey = "beast:instances" | ||
| InstanceDeletionQueue = "beast:instances:to_delete" | ||
| ) | ||
|
|
||
| func instanceKey(instanceID string) string { | ||
| return InstanceKeyPrefix + instanceID | ||
| } | ||
|
|
||
| func userInstanceKey(userID, challengeName string) string { | ||
| return UserInstanceKeyPrefix + userID + ":" + challengeName | ||
| } |
There was a problem hiding this comment.
This file introduces new key-prefix constants and helper functions (instanceKey/userInstanceKey), but SaveInstance still uses the existing utils.InstanceToKey / utils.UserChallengeToKey helpers (and utils already defines the same Redis key constants). Consider removing these duplicates or switching the implementation to use them consistently to avoid confusion/drift.
There was a problem hiding this comment.
i think rebasing is unbasing this, its okie ill remove
| exists, err := checkScriptExistence(localDeploy, instance) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, HTTPErrorResp{ | ||
| Error: fmt.Sprintf("CONTAINER RUNTIME ERROR while verifying the existance of check script: %s", err.Error()), |
There was a problem hiding this comment.
Typo in error message: "existance" → "existence".
| Error: fmt.Sprintf("CONTAINER RUNTIME ERROR while verifying the existance of check script: %s", err.Error()), | |
| Error: fmt.Sprintf("CONTAINER RUNTIME ERROR while verifying the existence of check script: %s", err.Error()), |
| if result.ExitCode != 0 { | ||
| c.JSON(http.StatusOK, ChallengeSubmitResponse{ | ||
| Message: fmt.Sprintf("Challenge check failed with EXIT CODE: %v\nLOGS: %s", result.ExitCode, result.Output), | ||
| Success: false, | ||
| }) |
There was a problem hiding this comment.
The response includes raw check.sh output back to the user on failure. That output can contain sensitive information (paths, secrets, unintended debug logs) and can be very large. Consider returning a generic failure message (or a truncated/sanitized snippet) and logging full output server-side.
| # Prerequisites: | ||
| # - config.toml must exist alongside this Makefile. | ||
| # - In config.toml set psql_config.host = "postgres" (the compose service name). | ||
| # |
There was a problem hiding this comment.
The Makefile comments instruct setting psql_config.host = "postgres" as a docker-compose service name, but the added docker-compose.yml only defines the "beast" service (no "postgres"). Either add the dependent services to docker-compose.yml or update these prerequisites to match the actual deployment model.
| func verifySSHRemote(containerId string, server cfg.AvailableServer) error { | ||
| portCmd := fmt.Sprintf("docker port %s %v:tcp", containerId, core.SSH_PORT) | ||
| _, err := remoteManager.RunCommandOnServer(server, portCmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| sshAgentCheckCommand := fmt.Sprintf("[ -z \"$SSH_AUTH_SOCK\" ]") | ||
| result, err := remoteManager.RunCommandInContainerOnServer(server, containerId, sshAgentCheckCommand) | ||
|
|
||
| if err != nil { | ||
| return err | ||
| } | ||
| if result.ExitCode != 0 { | ||
| return fmt.Errorf("ssh agent check failed, ssh-agent not running") | ||
| } | ||
|
|
||
| sshServiceStartCommand := fmt.Sprintf("service ssh start") | ||
| result, err = cr.RunCommandInContainer(containerId, []string{ | ||
| "sh", "-c", sshServiceStartCommand, | ||
| }) | ||
|
|
There was a problem hiding this comment.
verifySSHRemote builds an invalid docker port command (uses "%v:tcp" instead of "%v/tcp") and starts the SSH service using cr.RunCommandInContainer (local) rather than executing inside the remote container via SSH. This will break remote instance verification. Use the same "22/tcp" format as verifySSHLocal and run the ssh start command via remoteManager.RunCommandInContainerOnServer.
kunrex
force-pushed
the
sadserver-instance
branch
from
a136716 to
60ccd88
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (3)
api/check.go:251
- On check failure, the API response includes the full raw
check.shoutput (LOGS: %s). This can leak sensitive data from inside the container and can produce very large responses. Consider returning a generic failure message (or a small truncated snippet) and logging the full output server-side (ideally with a size cap).
api/check.go:197 - After loading
instancebyinstance_id, the handler does not verify that this instance belongs to the submittedchall_id/challenge. This allows using an instance from a different challenge to markchall_idas solved. Add a check likeinstance.ChallengeName == challenge.Name(or store/compareChallengeID) before executingcheck.sh/ awarding points.
api/check.go:214 - Typo in error message: "existance" → "existence".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !ok { | ||
| return true | ||
| } | ||
| server := config.AvailableServers[serverName] |
There was a problem hiding this comment.
UseLocalDockerDaemon no longer handles unknown/empty serverName: a missing key now yields a zero-value AvailableServer and returns false, causing callers with serverName == "" (or an unknown name) to incorrectly take the remote path. This is a behavior change from the previous “default local” logic and will break deployments where ServerDeployed is unset. Consider restoring the map-lookup check and defaulting to true when the server is not found/empty, or explicitly special-case "" to mean localhost.
| server := config.AvailableServers[serverName] | |
| if serverName == "" { | |
| return true | |
| } | |
| server, ok := config.AvailableServers[serverName] | |
| if !ok { | |
| return true | |
| } |
| if err != nil { | ||
| return "", fmt.Errorf("failed to deploy instance %s: %w", instanceID, err) | ||
| return "", "", fmt.Errorf("failed to deploy instance %s: %w", instanceID, err) | ||
| } |
There was a problem hiding this comment.
In the local-daemon branch, DeployContainerFromCompose returns primaryContainer, but containerId is never set to it. The subsequent verification calls use containerId (still empty) and the function can return an empty container ID. Assign containerId = primaryContainer (or use primaryContainer consistently) before verification and return.
| } | |
| } | |
| containerId = primaryContainer |
|
|
||
| EXPOSE 5005 | ||
|
|
||
| ENTRYPOINT ["setup.sh"] |
There was a problem hiding this comment.
ENTRYPOINT ["setup.sh"] relies on setup.sh being in PATH. Since the script is copied to /usr/local/bin/setup.sh, it’s safer/more explicit to use the absolute path in ENTRYPOINT to avoid failures if PATH is modified.
| ENTRYPOINT ["setup.sh"] | |
| ENTRYPOINT ["/usr/local/bin/setup.sh"] |
| dockerfile: Dockerfile.app | ||
| network_mode: host | ||
| volumes: | ||
| - ${BEAST_DIR}:/root/.beast |
There was a problem hiding this comment.
BEAST_HOST_DIR is set to ${BEAST_DIR} (a host path), but the only bind-mount maps ${BEAST_DIR} to /root/.beast. If the app uses BEAST_HOST_DIR/BEAST_MOUNT_DIR for any in-process filesystem reads (e.g., validating staging/auth files), it won’t see those paths unless the host directory is also mounted at the same path inside the container. Either add a second volume mount ${BEAST_DIR}:${BEAST_DIR}, or set BEAST_HOST_DIR to /root/.beast and adjust host-bind-mount logic accordingly.
| - ${BEAST_DIR}:/root/.beast | |
| - ${BEAST_DIR}:/root/.beast | |
| - ${BEAST_DIR}:${BEAST_DIR} |
| if ports[config.Challenge.Env.DefaultPortVar] != core.SSH_PORT { | ||
| return "", "", fmt.Errorf("the default port variable does not map to %d for instance challenge %s", core.SSH_PORT, challengeName) | ||
| } |
There was a problem hiding this comment.
Same issue as above for docker-compose instanced challenges: enforcing ports[DefaultPortVar] == SSH_PORT will break _examples/instanced-compose (where the default port var is an arbitrary service port). If SSH-only is required, this should be scoped to the specific challenge flavor that needs it.
| stagingDirPath := filepath.Join(core.BEAST_MOUNT_DIR, core.BEAST_STAGING_DIR) | ||
| err = utils.CreateIfNotExistDir(stagingDirPath) | ||
| if err != nil { | ||
| log.Errorf("Error in validating staging mount point : %s", err) | ||
| return errors.New("INVALID_STAGING_AREA") |
There was a problem hiding this comment.
DeployStaticContentContainer now uses BEAST_MOUNT_DIR for local filesystem checks (CreateIfNotExistDir / ValidateFileExists). When running beast inside Docker, BEAST_MOUNT_DIR may be a host path (via BEAST_HOST_DIR) that is not mounted into the beast container, causing these checks to fail even though /root/.beast is mounted. Consider using BEAST_GLOBAL_DIR for on-process filesystem access and reserve BEAST_MOUNT_DIR for bind-mount source paths passed to the host Docker daemon.
| func addUserSSHKeyLocal(containerId string, sshKey string) error { | ||
| sshCmd := fmt.Sprintf("mkdir -p /home/beast/.ssh && echo '%s' >> /home/beast/.ssh/authorized_keys && chmod 700 /home/beast/.ssh && chmod 600 /home/beast/.ssh/authorized_keys && chown -R beast:beast-grp /home/beast/.ssh", sshKey) | ||
|
|
||
| result, err := cr.RunCommandInContainer(containerId, []string{ | ||
| "sh", "-c", sshCmd, |
There was a problem hiding this comment.
addUserSSHKeyLocal interpolates the user-provided SSH key into a shell command inside single quotes. This is a shell-injection risk if the key/comment contains quotes/newlines, and can also break on unusual key formatting. Prefer writing the key via stdin (docker exec -i ... sh -c 'cat >> authorized_keys'), or base64-encoding and decoding inside the container, instead of embedding it in the command string.
| } else if !slices.Contains(config.AptDeps, "openssh-server") { | ||
| log.Warn("openssh-server not found as dependency for sad challenge... adding dependency") | ||
| config.AptDeps = append(config.AptDeps, "openssh-server") |
There was a problem hiding this comment.
ValidateRequiredFields appends openssh-server to apt_deps for all non-compose challenges (the code isn’t gated by challenge type/tag). This silently changes the build/runtime dependencies for existing challenges and contradicts the log message that implies this is only for “sad challenge”. Add an explicit condition (e.g., only when instanced+sandbox requires SSH) or remove this auto-mutation and require authors to declare the dependency.
| } else if !slices.Contains(config.AptDeps, "openssh-server") { | |
| log.Warn("openssh-server not found as dependency for sad challenge... adding dependency") | |
| config.AptDeps = append(config.AptDeps, "openssh-server") |
| # Prerequisites: | ||
| # - config.toml must exist alongside this Makefile. | ||
| # - In config.toml set psql_config.host = "postgres" (the compose service name). | ||
| # |
There was a problem hiding this comment.
The Docker Compose helper comments instruct setting psql_config.host = "postgres", but the included docker-compose.yml in this PR only defines the beast service (no postgres service / compose network alias). This prerequisite is likely incorrect/misleading; either add the dependent services to compose or update the docs to reflect that Postgres/Redis are expected to be reachable on the host network.
|
|
||
| RUN cp scripts/docker-enter /usr/bin/docker-enter && \ | ||
| cp scripts/docker_enter /usr/bin/docker_enter && \ | ||
| chmod u+s /usr/bin/docker_enter && \ |
There was a problem hiding this comment.
The image sets the setuid bit on /usr/bin/docker_enter. If this binary is ever executable by a non-root user inside the container, it becomes a straightforward privilege-escalation path. If setuid is not strictly required, drop it; otherwise document why it’s needed and consider restricting permissions/ownership to reduce the attack surface.
| chmod u+s /usr/bin/docker_enter && \ |
kunrex
force-pushed
the
sadserver-instance
branch
from
04e1877 to
6a9f870
Compare
Add register route checks Fix Typos Fix ssh socket check Remove unused code Fix port in ssh commands
Return nil instead of err Escape ssh key while injection
kunrex
force-pushed
the
sadserver-instance
branch
from
6a9f870 to
bde55ee
Compare
No description provided.