diff --git a/.dockerignore b/.dockerignore index 9931c830..6f620020 100644 --- a/.dockerignore +++ b/.dockerignore @@ -6,10 +6,13 @@ docs/ hack/ images/ !images/agent-base/goose-src/ +!images/agent-base/patches/ skills/ test/ changes/ .git/ +**/.git +**/target/ .github/ .claude/ .cursor/ diff --git a/.github/actions/goose-image/action.yml b/.github/actions/goose-image/action.yml index af50bc74..1c41a31b 100644 --- a/.github/actions/goose-image/action.yml +++ b/.github/actions/goose-image/action.yml @@ -12,53 +12,13 @@ outputs: runs: using: composite steps: - # Everything that decides what the binary is lives between the GOOSE_IMAGE - # arg and the stage that consumes it: the builder base, the build deps, the - # source repo, the version and the cargo invocation. Key on a hash of just - # that region, so editing the harness half of the Containerfile does not - # throw away a 35 minute compile. - # - # GOOSE_VERSION is inside that region, so a bump already changes the hash. - # It goes in the key as well because a bare hash tells you nothing on the - # Actions cache page or in a build log, and the version is the thing anyone - # looking at either actually wants to know. - name: Derive the cache key id: key shell: bash - env: - CONTAINERFILE: images/agent-base/Containerfile run: | - region=$(sed -n '/^ARG GOOSE_IMAGE=/,/^FROM .* AS goose$/p' "${CONTAINERFILE}") - - # sed's range prints to EOF when the end address never matches, so a - # renamed, lowercased or reflowed "AS goose" line would silently widen - # the region to the whole file -- putting the harness stages back in - # the key and bringing the compile back on every push, with nothing to - # point at. Check the region actually ends where it is supposed to. - case "$(printf '%s\n' "${region}" | tail -1)" in - "FROM "*" AS goose") ;; - *) - echo "Could not read the goose stages out of ${CONTAINERFILE}." - echo "Expected a region from 'ARG GOOSE_IMAGE=' to 'FROM ... AS goose'." - exit 1 - ;; - esac - - # This ends up in an image tag and, via images.yml, in build-image's - # unquoted build_args loop. A trailing comment on the ARG line is - # enough to produce "invalid reference format" 30 minutes in. - version=$(printf '%s\n' "${region}" | sed -n 's/^ARG GOOSE_VERSION=//p' | head -1) - case "${version}" in - '' | [-.]* | *[!A-Za-z0-9._-]*) - echo "Expected 'ARG GOOSE_VERSION=' in the goose stages of ${CONTAINERFILE}." - echo "Got: '${version}'" - exit 1 - ;; - esac - - digest=$(printf '%s\n' "${region}" | sha256sum | cut -c1-16) - echo "cache-key=goose-dist-${RUNNER_OS}-${RUNNER_ARCH}-${version}-${digest}" >> "$GITHUB_OUTPUT" - echo "image=localhost/goose-dist:${version}-${digest}" >> "$GITHUB_OUTPUT" + key=$(bash images/agent-base/patches/cache-key.sh) + echo "cache-key=goose-dist-${RUNNER_OS}-${RUNNER_ARCH}-${key}" >> "$GITHUB_OUTPUT" + echo "image=localhost/goose-dist:${key}" >> "$GITHUB_OUTPUT" # restore + save rather than actions/cache, which only saves from a post # step gated on post-if: success(). A cold run that compiles for 35 minutes @@ -105,6 +65,21 @@ runs: /tmp/crun --version | head -1 | grep -q "crun version ${CRUN_MIN}" sudo mv /tmp/crun /usr/bin/crun + # On a cache miss the compile needs the submodule's actual contents. If the + # calling workflow checked out without submodules the directory is empty, + # COPY silently copies nothing, and the first thing to notice is git apply + # reporting "crates/goose/Cargo.toml: No such file or directory" a couple + # of minutes into the container build. Say what is actually wrong instead. + - name: Check the goose submodule is populated + if: steps.cache.outputs.cache-hit != 'true' + shell: bash + run: | + if [ ! -f images/agent-base/goose-src/Cargo.toml ]; then + echo "images/agent-base/goose-src is empty, so goose cannot be compiled." + echo "Add 'submodules: true' to this job's actions/checkout step." + exit 1 + fi + - name: Compile goose if: steps.cache.outputs.cache-hit != 'true' shell: bash diff --git a/.github/workflows/test-e2e.yml b/.github/workflows/test-e2e.yml index 684e7382..4a871650 100644 --- a/.github/workflows/test-e2e.yml +++ b/.github/workflows/test-e2e.yml @@ -59,6 +59,10 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: persist-credentials: false + # goose is built from images/agent-base/goose-src, so a cache miss + # here compiles from the submodule. Without this the directory is + # empty and the build fails inside the container. + submodules: true - name: Setup Go uses: actions/setup-go@4b73464bb391d4059bd26b0524d20df3927bd417 # v6.3.0 diff --git a/Makefile b/Makefile index 657735ee..95720567 100644 --- a/Makefile +++ b/Makefile @@ -171,13 +171,13 @@ controller-agent-push: controller-agent-build ## Build and push the controller's # --platform build would put that one binary in every entry of the manifest. GOOSE_IMAGE ?= GOOSE_BUILD_ARG = $(if $(GOOSE_IMAGE),--build-arg GOOSE_IMAGE=$(GOOSE_IMAGE)) -# The default tag carries the version the Containerfile names, so a goose-dist -# image built before a GOOSE_VERSION bump cannot be silently reused after one: -# podman's default --pull=missing finds a stale :latest and says nothing, and -# you debug the new goose against the old binary. CI never sees this default, -# the action passes its own version+digest name. -GOOSE_DIST_VERSION := $(shell sed -n 's/^ARG GOOSE_VERSION=//p' images/agent-base/Containerfile | head -1) -GOOSE_DIST_IMG ?= localhost/goose-dist:$(GOOSE_DIST_VERSION) +# Include the pinned source, build recipe, and downstream patches in the tag. +# $(shell) swallows a non-zero exit, so guard the empty case rather than +# tagging the image `goose-dist:` and silently reusing the wrong binary. The +# error sits in the recursively expanded GOOSE_DIST_IMG so it fires only when +# a goose build actually needs the tag, not on every make invocation. +GOOSE_DIST_VERSION := $(shell bash images/agent-base/patches/cache-key.sh) +GOOSE_DIST_IMG ?= localhost/goose-dist:$(or $(GOOSE_DIST_VERSION),$(error images/agent-base/patches/cache-key.sh produced no version; run it directly to see why)) .PHONY: goose-dist-build goose-dist-build: ## Build just the goose binary as a standalone image, for reuse as GOOSE_IMAGE. diff --git a/changes/unreleased/19-goose-system-crypto.yaml b/changes/unreleased/19-goose-system-crypto.yaml new file mode 100644 index 00000000..319192cc --- /dev/null +++ b/changes/unreleased/19-goose-system-crypto.yaml @@ -0,0 +1,9 @@ +kind: bugfix +description: > + Build the Goose binary in the agent images against the system OpenSSL + instead of bundling its own crypto. The build drops the default rustls + feature set for native-tls, generates the self-signed ACP transport + certificate with OpenSSL rather than rcgen (whose only backends, ring and + aws-lc-rs, compile crypto into the binary), and sets OPENSSL_NO_VENDOR so + openssl-sys cannot fall back to a vendored OpenSSL. Without this the + release FIPS payload scan rejects the image. diff --git a/images/agent-base/Containerfile b/images/agent-base/Containerfile index 23fa34c0..cb052fab 100644 --- a/images/agent-base/Containerfile +++ b/images/agent-base/Containerfile @@ -35,7 +35,19 @@ ENV PATH="/root/.cargo/bin:${PATH}" # Run `git submodule update --init images/agent-base/goose-src` before building locally. WORKDIR /src/goose COPY images/agent-base/goose-src/ . -RUN cargo build --release -p goose-cli --bin goose \ +COPY images/agent-base/patches/ /tmp/goose-patches/ + +# Goose must carry no crypto of its own and use the system OpenSSL, or +# check-payload fails the release scan. OPENSSL_NO_VENDOR stops openssl-sys +# from compiling its own copy; the feature set and the patch keep rustls, +# aws-lc-rs and ring out. See images/agent-base/patches/README.md. +# The two checks either side of the build are the scan's pass condition -- +# don't drop them. +ENV OPENSSL_NO_VENDOR=1 +RUN git apply /tmp/goose-patches/goose-system-crypto.patch \ + && bash /tmp/goose-patches/verify-dependencies.sh \ + && cargo build --locked --release -p goose-cli --bin goose --no-default-features --features native-tls,aws-providers,disable-update \ + && ldd target/release/goose | grep -q 'libcrypto.so.3' \ && mv target/release/goose /usr/local/bin/goose # Just the binary, so what CI caches and reloads is ~100MB rather than the @@ -71,9 +83,14 @@ RUN CGO_ENABLED=1 GOEXPERIMENT=strictfipsruntime go build -tags strictfipsruntim # --- Runtime stage --- FROM registry.access.redhat.com/ubi10/ubi:latest +# openssl-fips-provider is already in the UBI base; naming it is what keeps a +# future base-image change from silently removing the FIPS module that goose +# and the harness both link against. RUN dnf install -y \ git \ ca-certificates \ + openssl-libs \ + openssl-fips-provider \ bzip2 \ tar \ && dnf clean all diff --git a/images/agent-base/patches/README.md b/images/agent-base/patches/README.md new file mode 100644 index 00000000..c5c00ead --- /dev/null +++ b/images/agent-base/patches/README.md @@ -0,0 +1,56 @@ +# Downstream Goose patch: system crypto + +Applies to Goose 1.51.0 at submodule revision +`7343b6f43276ec45ae10df10d6e591f91edd05e9`. Keep the submodule unmodified -- +the Containerfile applies the patch to its own copy of the source. + +## What this is for + +`openshift/check-payload` scans release images for crypto that is not the +system FIPS module. For a Rust binary it fails on crypto compiled *into* the +executable, which it detects by looking for defined symbols with known +prefixes: `ring_core_`/`GFp_` (ring), `aws_lc_`/`AWSLC_` (aws-lc-rs), +`BORINGSSL_` (boring), and `OPENSSL_` (statically vendored OpenSSL). A binary +with none of those that lists `libcrypto` in `DT_NEEDED` passes. + +Stock Goose fails: the default `rustls-tls` feature pulls in aws-lc-rs, and +`rcgen` links ring or aws-lc-rs to generate the self-signed transport +certificate. Three things move it onto system OpenSSL: + +- The build selects `--no-default-features --features + native-tls,aws-providers,disable-update`, so the TLS stack is OpenSSL rather + than rustls. +- The patch drops `rcgen` from the `native-tls` feature and builds the + self-signed certificate with OpenSSL instead. Without this, enabling + `native-tls` still drags ring in through `rcgen`. +- `OPENSSL_NO_VENDOR=1` in the Containerfile stops `openssl-sys` from falling + back to `openssl-src`, which would compile OpenSSL into the binary and define + the `OPENSSL_*` symbols the scan rejects. + +`verify-dependencies.sh` asserts the resolved graph still has no such crate, +and the `ldd | grep -q libcrypto.so.3` in the Containerfile asserts the +finished binary really does link the system library. Those two checks are the +pass condition, so keep them in the build rather than relying on the feature +list alone. + +## Scope + +This makes the binary carry no crypto of its own and route TLS through system +OpenSSL. It is not an audit of every cryptographic call site. Goose still +compiles pure-Rust primitives (`sha2`, `hmac`) for OAuth PKCE, pairing codes +and similar; those define no scanned symbol, and check-payload only reaches its +crate-name denylist through a `cargo auditable` SBOM, which this binary does +not embed. If Goose gains an embedded SBOM, or the scan grows a source-level +check, those call sites become the next thing to move onto OpenSSL. + +## Maintenance + +The patch touches only `crates/goose/Cargo.toml` (`[features]`) and +`crates/goose/src/acp/transport/tls.rs`. It deliberately does not touch +`Cargo.lock`: a `[features]` edit does not change dependency resolution, so +`cargo build --locked` still holds and the build needs no network beyond the +normal crate fetch. + +The image cache key (`cache-key.sh`) covers the submodule revision, the Goose +build region of the Containerfile, and these patch files. Regenerate and +revalidate the patch whenever the submodule revision changes. diff --git a/images/agent-base/patches/cache-key.sh b/images/agent-base/patches/cache-key.sh new file mode 100644 index 00000000..e38f48ff --- /dev/null +++ b/images/agent-base/patches/cache-key.sh @@ -0,0 +1,27 @@ +#!/usr/bin/env bash +# Prints the tag for the prebuilt goose binary: everything the binary is +# built from, hashed. Change any of it and the tag changes, so CI recompiles +# instead of reusing a stale binary. +set -euo pipefail +export LC_ALL=C +cd "$(git rev-parse --show-toplevel)" +revision=$(git ls-files --stage images/agent-base/goose-src | awk '{print $2}') +[[ $revision =~ ^[0-9a-f]{40}$ ]] || { echo "Missing Goose submodule revision" >&2; exit 1; } +region=$(sed -n '/^ARG GOOSE_IMAGE=/,/^FROM .* AS goose$/p' images/agent-base/Containerfile) +case "$(printf '%s\n' "$region" | tail -1)" in + "FROM "*" AS goose") ;; + *) echo "Cannot identify Goose build stages" >&2; exit 1 ;; +esac +digest=$({ + printf '%s\n%s\n' "$revision" "$region" + for file in images/agent-base/patches/*; do + # Prose and this script do not reach the binary. Hashing them would + # throw away a 35-minute compile over a typo fix. + if [[ $file == */README.md || $file == */cache-key.sh ]]; then + continue + fi + printf '%s\n' "$file" + cat "$file" + done +} | sha256sum | cut -c1-16) +printf '%s-%s\n' "${revision:0:12}" "$digest" diff --git a/images/agent-base/patches/goose-system-crypto.patch b/images/agent-base/patches/goose-system-crypto.patch new file mode 100644 index 00000000..84b45c68 --- /dev/null +++ b/images/agent-base/patches/goose-system-crypto.patch @@ -0,0 +1,158 @@ +diff --git a/crates/goose/Cargo.toml b/crates/goose/Cargo.toml +index 04b043bb4..43ed8733f 100644 +--- a/crates/goose/Cargo.toml ++++ b/crates/goose/Cargo.toml +@@ -59,14 +59,15 @@ rustls-tls = [ + "goose-providers/rustls-tls", + ] + native-tls = [ +- "dep:rcgen", + "dep:pem", + "dep:axum-server", + "dep:openssl", + "axum-server/tls-openssl", +- # rcgen requires a crypto backend; ring is used here because aws-lc-rs is +- # reserved for the rustls-tls path and openssl does not provide this interface. +- "rcgen/ring", ++ # rcgen is deliberately absent here. Its only crypto backends are ring and ++ # aws-lc-rs, both of which compile their own crypto into the binary, which ++ # is what check-payload's Rust scan rejects. This path builds the ++ # self-signed certificate with OpenSSL instead, so the binary carries no ++ # crypto of its own and uses the system FIPS provider. + "reqwest/native-tls", + "rmcp/reqwest-native-tls", + "smithy-transport-reqwest?/native-tls", +diff --git a/crates/goose/src/acp/transport/tls.rs b/crates/goose/src/acp/transport/tls.rs +index fa93d502c..052243ac4 100644 +--- a/crates/goose/src/acp/transport/tls.rs ++++ b/crates/goose/src/acp/transport/tls.rs +@@ -1,5 +1,6 @@ + use crate::config::paths::Paths; + use anyhow::{bail, Result}; ++#[cfg(feature = "rustls-tls")] + use rcgen::{CertificateParams, DnType, KeyPair, SanType}; + use std::path::Path; + +@@ -14,7 +15,11 @@ pub struct TlsSetup { + pub fingerprint: String, + } + +-fn generate_self_signed_cert() -> Result<(rcgen::Certificate, KeyPair)> { ++/// Returns the certificate DER, the certificate PEM, and the private key PEM. ++/// The two backends return the same triple so the caller does not care which ++/// crypto produced it. ++#[cfg(feature = "rustls-tls")] ++fn generate_self_signed_cert() -> Result<(Vec, String, String)> { + let mut params = CertificateParams::default(); + params + .distinguished_name +@@ -26,7 +31,52 @@ fn generate_self_signed_cert() -> Result<(rcgen::Certificate, KeyPair)> { + + let key_pair = KeyPair::generate()?; + let cert = params.self_signed(&key_pair)?; +- Ok((cert, key_pair)) ++ Ok((cert.der().to_vec(), cert.pem(), key_pair.serialize_pem())) ++} ++ ++/// Same certificate rcgen would produce -- P-256 key, ECDSA-SHA256 signature, ++/// CN and SANs for localhost -- built with OpenSSL so no Rust crypto backend ++/// is linked in. See the rcgen note in crates/goose/Cargo.toml. ++#[cfg(feature = "native-tls")] ++fn generate_self_signed_cert() -> Result<(Vec, String, String)> { ++ use openssl::{ ++ asn1::Asn1Time, ++ bn::{BigNum, MsbOption}, ++ ec::{EcGroup, EcKey}, ++ hash::MessageDigest, ++ nid::Nid, ++ pkey::PKey, ++ x509::{extension::SubjectAlternativeName, X509NameBuilder, X509}, ++ }; ++ ++ let group = EcGroup::from_curve_name(Nid::X9_62_PRIME256V1)?; ++ let key = PKey::from_ec_key(EcKey::generate(&group)?)?; ++ let mut name = X509NameBuilder::new()?; ++ name.append_entry_by_text("CN", "goosed localhost")?; ++ let name = name.build(); ++ let mut serial = BigNum::new()?; ++ serial.rand(128, MsbOption::MAYBE_ZERO, false)?; ++ let serial = serial.to_asn1_integer()?; ++ let mut cert = X509::builder()?; ++ cert.set_version(2)?; ++ cert.set_serial_number(&serial)?; ++ cert.set_subject_name(&name)?; ++ cert.set_issuer_name(&name)?; ++ cert.set_pubkey(&key)?; ++ cert.set_not_before(Asn1Time::days_from_now(0)?.as_ref())?; ++ cert.set_not_after(Asn1Time::days_from_now(365)?.as_ref())?; ++ let san = SubjectAlternativeName::new() ++ .dns("localhost") ++ .ip("127.0.0.1") ++ .build(&cert.x509v3_context(None, None))?; ++ cert.append_extension(san)?; ++ cert.sign(&key, MessageDigest::sha256())?; ++ let cert = cert.build(); ++ Ok(( ++ cert.to_der()?, ++ String::from_utf8(cert.to_pem()?)?, ++ String::from_utf8(key.private_key_to_pem_pkcs8()?)?, ++ )) + } + + fn sha256_fingerprint(der: &[u8]) -> String { +@@ -151,14 +201,11 @@ pub async fn self_signed_config() -> Result { + return Ok(cached); + } + +- let (cert, key_pair) = generate_self_signed_cert()?; ++ let (der, cert_pem, key_pem) = generate_self_signed_cert()?; + +- let fingerprint = sha256_fingerprint(cert.der()); ++ let fingerprint = sha256_fingerprint(&der); + println!("GOOSED_CERT_FINGERPRINT={fingerprint}"); + +- let cert_pem = cert.pem(); +- let key_pem = key_pair.serialize_pem(); +- + try_save_tls_to_cache(&cert_pem, &key_pem); + + #[cfg(feature = "rustls-tls")] +@@ -177,3 +224,38 @@ pub async fn self_signed_config() -> Result { + fingerprint, + }) + } ++ ++#[cfg(all(test, feature = "native-tls"))] ++mod tests { ++ use super::*; ++ use openssl::{nid::Nid, pkey::PKey, x509::X509}; ++ ++ #[test] ++ fn native_crypto_certificate_matches_key_and_localhost() { ++ let (der, cert_pem, key_pem) = generate_self_signed_cert().unwrap(); ++ let cert = X509::from_der(&der).unwrap(); ++ let key = PKey::private_key_from_pem(key_pem.as_bytes()).unwrap(); ++ assert!(cert.verify(&key).unwrap()); ++ assert!(cert.public_key().unwrap().public_eq(&key)); ++ assert_eq!( ++ cert.signature_algorithm().object().nid(), ++ Nid::ECDSA_WITH_SHA256 ++ ); ++ assert_eq!( ++ key.ec_key().unwrap().group().curve_name(), ++ Some(Nid::X9_62_PRIME256V1) ++ ); ++ assert_eq!( ++ X509::from_pem(cert_pem.as_bytes()) ++ .unwrap() ++ .to_der() ++ .unwrap(), ++ der ++ ); ++ let names = cert.subject_alt_names().unwrap(); ++ assert!(names.iter().any(|n| n.dnsname() == Some("localhost"))); ++ assert!(names ++ .iter() ++ .any(|n| n.ipaddress() == Some(&[127, 0, 0, 1][..]))); ++ } ++} diff --git a/images/agent-base/patches/verify-dependencies.sh b/images/agent-base/patches/verify-dependencies.sh new file mode 100644 index 00000000..80fe22bc --- /dev/null +++ b/images/agent-base/patches/verify-dependencies.sh @@ -0,0 +1,22 @@ +#!/usr/bin/env bash +# Fails the build if the Goose dependency graph resolves to a crate that +# carries its own crypto, or stops resolving to the OpenSSL bindings. The +# feature set is what keeps those crates out; this is the assertion that it +# still does. Run from the Goose source root, or pass it as $1. +set -euo pipefail +cd "${1:-.}" +packages=$(cargo tree --locked -p goose-cli --no-default-features \ + --features native-tls,aws-providers,disable-update --edges normal,build \ + --prefix none --format '{p}' | awk '{print $1}' | sort -u) +for forbidden in rustls ring aws-lc-rs aws-lc-sys aws-lc-fips-sys openssl-src; do + if grep -Fxq "$forbidden" <<< "$packages"; then + echo "Unexpected crypto dependency: $forbidden" >&2 + exit 1 + fi +done +for required in openssl openssl-sys; do + if ! grep -Fxq "$required" <<< "$packages"; then + echo "Missing system crypto dependency: $required" >&2 + exit 1 + fi +done