Copilot commented on code in PR #931: URL: https://github.com/apache/mina-sshd/pull/931#discussion_r4313531346
########## docker/oqs-openssh/sshd_config: ########## @@ -0,0 +1,33 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Test server configuration. Only pure PQC algorithms for key exchange. +Port 22 +HostKey /etc/ssh/ssh_host_mldsa44_key +HostKey /etc/ssh/ssh_host_mldsa65_key +HostKey /etc/ssh/ssh_host_mldsa87_key +HostKey /etc/ssh/ssh_host_ed25519_key + +KexAlgorithms mlkem768-sha256,mlkem1024-sha384,mlkem512-sha256 +HostKeyAlgorithms ssh-mldsa-44,ssh-mldsa-65,ssh-mldsa-87,ssh-ed25519 +PubkeyAcceptedAlgorithms ssh-mldsa-44,ssh-mldsa-65,ssh-mldsa-87,ssh-ed25519 + +PasswordAuthentication yes +PubkeyAuthentication yes +PermitRootLogin no Review Comment: Enabling `PasswordAuthentication yes` combined with the entrypoint’s default `test` password makes the published image insecure-by-default if anyone runs it with an exposed port (intentionally or accidentally). Since the smoke test uses public keys anyway, consider setting `PasswordAuthentication no` by default and only enabling it explicitly (e.g., via a separate env flag that the entrypoint converts to `sshd -o PasswordAuthentication=yes`, or by requiring `TEST_PASSWORD` to be set to a non-default value before enabling password auth). ########## docker/oqs-openssh/entrypoint.sh: ########## @@ -0,0 +1,27 @@ +#!/bin/sh +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Generate host keys on first start, set the test user password, run sshd. +set -e +K=/etc/ssh +[ -f $K/ssh_host_ed25519_key ] || ssh-keygen -q -t ed25519 -N '' -f $K/ssh_host_ed25519_key +[ -f $K/ssh_host_mldsa44_key ] || ssh-keygen -q -t ssh-mldsa-44 -N '' -f $K/ssh_host_mldsa44_key +[ -f $K/ssh_host_mldsa65_key ] || ssh-keygen -q -t ssh-mldsa-65 -N '' -f $K/ssh_host_mldsa65_key +[ -f $K/ssh_host_mldsa87_key ] || ssh-keygen -q -t ssh-mldsa-87 -N '' -f $K/ssh_host_mldsa87_key +echo "test:${TEST_PASSWORD:-test}" | chpasswd Review Comment: `TEST_PASSWORD` is interpolated directly into `chpasswd` input. If it contains a newline, an attacker (or accidental value) could inject additional `user:password` lines and potentially set passwords for other accounts (e.g., `root`). Use `printf` with a fixed username and reject/strip `:` and newline characters in `TEST_PASSWORD` (fail fast if present) before passing it to `chpasswd`. ########## docker/oqs-openssh/Dockerfile: ########## @@ -0,0 +1,86 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Small, multi-arch (linux/amd64, linux/arm64) OpenSSH server with PURE ML-KEM +# key exchange (mlkem768-sha256, ...) and pure ML-DSA host/user keys +# (ssh-mldsa-44/65/87), for MINA SSHD interoperability tests (GH-928). +# +# Based on the Open Quantum Safe OpenSSH fork (branch OQS-v10, OpenSSH 10.3p1). +# Everything is compiled in the BUILD stage. The final image only has the +# runtime files, so tests pull a small prebuilt image and need no compiler. +# +# Build: docker buildx build --platform linux/amd64,linux/arm64 -t <name> . +# Run: docker run --rm -p 2222:22 <name> + +# Alpine 3.22.6 multi-arch index (includes linux/amd64 and linux/arm64). +# The digest wins over the tag; update the digest to change the base image. +FROM alpine:3.22.6@sha256:5291449c3df73caf6ed85e649dec1b9e818b39a5d8c871e97afc13e9cd5e8fa8 AS build +# Pinned versions. Change them together and re-run the interop tests. +# LIBOQS_REF is the commit of the 0.16.0 tag. +ARG LIBOQS_REF=5a1a854b0dc9f2141bdc771c555ee60c37950183 +ARG OQS_OPENSSH_REF=10d74f8b27da43705cfab85923ac7e1d769416f1 +RUN apk add --no-cache build-base cmake ninja git autoconf automake \ + openssl-dev zlib-dev linux-headers +WORKDIR /src + +# liboqs. Do NOT use OQS_MINIMAL_BUILD: ssh-oqs.c references many signature +# algorithms (mayo, ...), so the fork does not compile against a trimmed liboqs. +RUN git init liboqs \ + && git -C liboqs fetch --depth 1 https://github.com/open-quantum-safe/liboqs.git "${LIBOQS_REF}" \ + && git -C liboqs checkout FETCH_HEAD \ + && cmake -S liboqs -B liboqs/build -GNinja \ + -DCMAKE_BUILD_TYPE=Release \ + -DBUILD_SHARED_LIBS=ON \ + -DCMAKE_INSTALL_PREFIX=/opt/oqs \ + -DOQS_BUILD_ONLY_LIB=ON \ + -DOQS_DIST_BUILD=ON \ + -DOQS_USE_OPENSSL=ON \ + && cmake --build liboqs/build \ + && cmake --install liboqs/build + +# OQS-OpenSSH, installed under /opt/ssh. +# The fork calls sshkey_clear_pkey() without a visible prototype (an upstream +# header quirk). Its own CI hides this with -Wno-implicit-function-declaration; +# GCC 14 (Alpine 3.22) needs the "-Wno-error=" form. +RUN git clone https://github.com/open-quantum-safe/openssh.git openssh \ + && git -C openssh checkout "${OQS_OPENSSH_REF}" \ + && cd openssh \ + && autoreconf -i \ + && ./configure --prefix=/opt/ssh --sysconfdir=/etc/ssh \ + --with-ssl-dir=/usr --with-liboqs-dir=/opt/oqs \ + --with-ldflags="-Wl,-rpath,/opt/oqs/lib" --with-libs=-lm \ + --with-cflags="-Wno-error=implicit-function-declaration" \ + --without-pam --without-selinux --with-privsep-path=/var/empty \ + && make -j"$(nproc)" \ + && make install-nokeys \ + && strip /opt/ssh/sbin/sshd /opt/ssh/bin/ssh /opt/ssh/bin/ssh-keygen + +FROM alpine:3.22.6@sha256:5291449c3df73caf6ed85e649dec1b9e818b39a5d8c871e97afc13e9cd5e8fa8 +RUN apk add --no-cache libcrypto3 zlib \ + && addgroup -S sshd 2>/dev/null; adduser -S -D -H -h /var/empty -s /sbin/nologin -G sshd sshd 2>/dev/null; \ + adduser -D -s /bin/sh test \ Review Comment: This `RUN` mixes `&&` with `;`, which can mask failures: if `addgroup` or the first `adduser` fails, the shell will still continue after `;` and the overall layer exit status will be determined by the last command. Prefer chaining with `&&` throughout, and if the intent is to ignore 'already exists' errors, make that explicit (e.g., `... || true`) so unexpected failures don’t get swallowed. ########## docker/oqs-openssh/entrypoint.sh: ########## @@ -0,0 +1,27 @@ +#!/bin/sh +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Generate host keys on first start, set the test user password, run sshd. +set -e +K=/etc/ssh +[ -f $K/ssh_host_ed25519_key ] || ssh-keygen -q -t ed25519 -N '' -f $K/ssh_host_ed25519_key +[ -f $K/ssh_host_mldsa44_key ] || ssh-keygen -q -t ssh-mldsa-44 -N '' -f $K/ssh_host_mldsa44_key +[ -f $K/ssh_host_mldsa65_key ] || ssh-keygen -q -t ssh-mldsa-65 -N '' -f $K/ssh_host_mldsa65_key +[ -f $K/ssh_host_mldsa87_key ] || ssh-keygen -q -t ssh-mldsa-87 -N '' -f $K/ssh_host_mldsa87_key Review Comment: The variable-expanded paths aren’t quoted. While `/etc/ssh` is stable, quoting (`\"$K/...\"`) avoids word-splitting/globbing hazards and aligns with standard shell scripting practices (also makes ShellCheck happy). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
