website/package.json (1)

8-8: Make build/dev scripts cross-platform compatible.

Lines 8, 15, and 16 use POSIX-only VAR=$(...) command syntax that will fail on Windows default npm shells. While the project doesn't explicitly target Windows, it already uses shx for cross-platform operations in other scripts (clean tasks), so applying the same pattern here would ensure consistency and prevent breakage for any Windows contributors.

Consider refactoring to a Node wrapper:

Cross-platform refactor (Node wrapper)

-    "build": "HUGO_PARAMS_COMMITFULL=$(git log -1 --format=%H -- .) HUGO_PARAMS_COMMITSHORT=$(git log -1 --format=%h -- .) ./node_modules/.bin/hugo --minify --gc",
+    "build": "node scripts/run-hugo-with-commit.mjs --minify --gc",
...
-    "dev:drafts": "HUGO_PARAMS_COMMITFULL=$(git log -1 --format=%H -- .) HUGO_PARAMS_COMMITSHORT=$(git log -1 --format=%h -- .) ./node_modules/.bin/hugo server --bind=0.0.0.0 --disableFastRender --baseURL=http://localhost --noHTTPCache --buildDrafts",
+    "dev:drafts": "node scripts/run-hugo-with-commit.mjs server --bind=0.0.0.0 --disableFastRender --baseURL=http://localhost --noHTTPCache --buildDrafts",
-    "dev": "HUGO_PARAMS_COMMITFULL=$(git log -1 --format=%H -- .) HUGO_PARAMS_COMMITSHORT=$(git log -1 --format=%h -- .) ./node_modules/.bin/hugo server --bind=0.0.0.0 --disableFastRender --noHTTPCache",
+    "dev": "node scripts/run-hugo-with-commit.mjs server --bind=0.0.0.0 --disableFastRender --noHTTPCache",

Also applies to: 15-16

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@website/package.json` at line 8, The npm scripts in package.json (notably the
"build" script and the other scripts using VAR=$(...) syntax) use POSIX-only
variable-substitution which breaks on Windows; replace them with a
cross-platform approach by creating a small Node wrapper (e.g.,
scripts/build.js) that runs git to compute commit hashes via child_process, sets
HUGO_PARAMS_COMMITFULL and HUGO_PARAMS_COMMITSHORT on process.env, and then
spawns the Hugo binary (./node_modules/.bin/hugo) so package.json's "build" (and
the other offending scripts) simply call node scripts/build.js; alternatively
you can use cross-env for simple env var setting, but for computed values prefer
the Node wrapper so functions/variables in scripts/build.js (git hash capture
and spawn of Hugo) handle Windows correctly.

bindings/go/sigstore/integration/internal/trusted_root.go (2)

110-130: Same concern: hardcoded CT log URL.

The CT log base URL "http://tesseract:6962" is hardcoded. If this needs to be configurable in the future, consider adding it to TrustedRootParams alongside the public key.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/sigstore/integration/internal/trusted_root.go` around lines 110 -
130, The CT log base URL is hardcoded in buildCTLogEntries; update the
TrustedRootParams type to include a CTLogBaseURL (or similar) field and use that
value in buildCTLogEntries instead of the literal "http://tesseract:6962",
falling back to the current literal as a default when the new field is empty;
adjust any constructors or callers that build TrustedRootParams to populate
CTLogBaseURL as needed so the function uses params.CTLogBaseURL when present.

---

56-76: Consider parameterizing the hardcoded Fulcio URI.

The Fulcio URI is hardcoded as "http://fulcio-server:5555" while other values like RekorBaseURL are passed via TrustedRootParams. For consistency and flexibility, consider adding the Fulcio URI to the params struct.

♻️ Suggested change

 type TrustedRootParams struct {
 	FulcioRootPEM     []byte
+	FulcioBaseURL     string // base URL for Fulcio CA entry, e.g. "http://fulcio-server:5555"
 	RekorPublicKeyPEM []byte
 	RekorOrigin       string
 	RekorBaseURL      string
 	CTLogPublicKeyDER []byte
 	CTLogID           [sha256.Size]byte
 	TSACertChainPEM   []byte
 }

Then use params.FulcioBaseURL instead of the hardcoded string at line 64.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/sigstore/integration/internal/trusted_root.go` around lines 56 -
76, The trustedRoot map currently hardcodes the Fulcio URI
("http://fulcio-server:5555"); update the TrustedRootParams struct to include a
FulcioBaseURL (or similar) field and replace the literal in the trustedRoot
construction with params.FulcioBaseURL so the Fulcio endpoint is configurable;
ensure any callers constructing TrustedRootParams are updated to provide the new
FulcioBaseURL and that tests/usage that rely on the previous default are
adjusted or a sensible default is added to TrustedRootParams if needed.

bindings/go/sigstore/integration/internal/dex.go (1)

142-176: Consider using structured YAML generation instead of fmt.Sprintf.

The current approach using fmt.Sprintf with a template string works but could break if special characters appear in values (e.g., if email contained YAML-special characters). For test infrastructure with known static values this is acceptable, but a structured approach would be more robust.

♻️ Alternative using gopkg.in/yaml.v3

func generateDexConfig(issuerURL string) (string, error) {
    hash, err := bcrypt.GenerateFromPassword([]byte(dexPassword), bcrypt.DefaultCost)
    if err != nil {
        return "", err
    }

    config := map[string]any{
        "issuer": issuerURL,
        "storage": map[string]any{"type": "memory"},
        "web":     map[string]any{"http": "0.0.0.0:" + dexPort},
        "oauth2": map[string]any{
            "responseTypes":       []string{"code"},
            "skipApprovalScreen":  true,
            "passwordConnector":   "local",
        },
        "enablePasswordDB": true,
        "staticPasswords": []map[string]any{{
            "email":    dexUser,
            "hash":     string(hash),
            "username": "test-user",
            "userID":   "test-user-id",
        }},
        "staticClients": []map[string]any{{
            "id":           dexClientID,
            "public":       true,
            "name":         "Fulcio",
            "redirectURIs": []string{"http://localhost/callback"},
        }},
    }

    data, err := yaml.Marshal(config)
    if err != nil {
        return "", err
    }
    return string(data), nil
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/sigstore/integration/internal/dex.go` around lines 142 - 176, The
generateDexConfig function builds YAML via fmt.Sprintf which can produce invalid
YAML if values contain special characters; instead construct a typed Go
structure or map (referencing generateDexConfig, dexPassword, dexPort, dexUser,
dexClientID) and use gopkg.in/yaml.v3 (yaml.Marshal) to serialize it; keep the
bcrypt-generated hash (string(hash)) in the staticPasswords entry, set web.http
to "0.0.0.0:"+dexPort, oauth2 and other nested keys as maps/slices as in the
review example, handle and return marshal errors, and remove the fmt.Sprintf
template.

bindings/go/sigstore/integration/internal/fulcio.go (1)

203-208: Clarify the intent of the empty --ct-log-url= flag.

When ctLogURL is empty, the code explicitly passes --ct-log-url= with an empty value. This differs from simply omitting the flag entirely. If the intent is to disable CT logging, consider adding a brief comment explaining why the empty flag is needed versus omitting it.

📝 Suggested documentation

 func fulcioCmd(ctLogURL string) []string {
 	cmd := []string{
 		"serve",
 		"--host=0.0.0.0",
 		"--port=" + fulcioPort,
 		"--grpc-port=" + fulcioGRPCPort,
 		"--ca=fileca",
 		"--fileca-cert=/etc/fulcio/root.pem",
 		"--fileca-key=/etc/fulcio/root.key",
 		"--fileca-key-passwd=" + fulcioKeyPw,
 		"--config-path=/etc/fulcio/config.json",
 	}
 	if ctLogURL != "" {
 		cmd = append(cmd, "--ct-log-url="+ctLogURL)
 	} else {
+		// Explicitly set empty --ct-log-url to disable CT log submission;
+		// omitting the flag entirely may cause Fulcio to use a default.
 		cmd = append(cmd, "--ct-log-url=")
 	}
 	return cmd
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/sigstore/integration/internal/fulcio.go` around lines 203 - 208,
The code appends an explicit "--ct-log-url=" when ctLogURL is empty, which is
different from omitting the flag; update the logic in the command-building code
that uses ctLogURL (the branch that currently does cmd = append(cmd,
"--ct-log-url="+ctLogURL) / cmd = append(cmd, "--ct-log-url=")) to make intent
explicit: either remove the else branch and do not append the flag when ctLogURL
== "" (if omission should disable CT logging), or keep the else branch but add a
concise comment above it explaining why an empty "--ct-log-url=" must be passed
to disable/override CT logging behavior; ensure the change touches the code path
that constructs cmd and adjust any related tests or caller expectations
accordingly.

cli/internal/oidcflow/oidcflow.go (3)

223-232: PKCE S256 requirement may reject valid providers.

Some OIDC providers support PKCE but don't advertise code_challenge_methods_supported in their discovery document (it's optional per RFC 8414). This check will fail for such providers even if they accept S256 challenges.

Consider either:

1. Attempting PKCE anyway if the claim is absent (many providers accept it)
2. Making PKCE optional with a fallback

However, for Sigstore's public-good infrastructure this is likely fine since it does advertise S256.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/oidcflow/oidcflow.go` around lines 223 - 232, The current loop
in oidcflow.go rejects providers when claims.Methods does not include "S256",
but RFC 8414 makes code_challenge_methods_supported optional; update the logic
in the PKCE support check to treat an absent or empty claims.Methods as
"unknown" and either attempt S256 anyway or fall back to non-PKCE flow: modify
the block that inspects claims.Methods (the supported boolean check over
claims.Methods) to: if claims.Methods is empty, proceed with attempting PKCE
S256, and only error using provider.Endpoint().AuthURL when Methods is present
and explicitly does not contain "S256" (or implement a fallback branch that
continues without PKCE when S256 is not supported).

---

129-129: oidc.Nonce is an auth URL option, not a token exchange option.

The oidc.Nonce(nonce) is designed for the authorization URL (which you correctly use at line 116). Passing it to config.Exchange() has no effect because the token endpoint doesn't accept a nonce parameter—the nonce is embedded in the ID token by the IdP based on the auth request.

🔧 Remove ineffective nonce from Exchange call

-	token, err := config.Exchange(ctx, code, append(pkce.tokenURLOpts(), oidc.Nonce(nonce))...)
+	token, err := config.Exchange(ctx, code, pkce.tokenURLOpts()...)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/oidcflow/oidcflow.go` at line 129, Remove the ineffective
oidc.Nonce(nonce) option from the token exchange call: in the line with token,
err := config.Exchange(ctx, code, append(pkce.tokenURLOpts(),
oidc.Nonce(nonce))...), stop appending oidc.Nonce(nonce) and only pass
pkce.tokenURLOpts() (and any real token endpoint options if needed); the nonce
belongs on the authorization URL (as already used when building the auth
request) and should not be sent to config.Exchange().

---

91-92: Type assertion could panic if listener address is not TCP.

The type assertion listener.Addr().(*net.TCPAddr) will panic if the listener isn't TCP-based. While net.Listen("tcp", ...) should always return a TCP listener, a defensive check would be safer.

🛡️ Defensive type assertion

-	addr := listener.Addr().(*net.TCPAddr)
+	addr, ok := listener.Addr().(*net.TCPAddr)
+	if !ok {
+		listener.Close()
+		return nil, errors.New("listener address is not TCP")
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/oidcflow/oidcflow.go` around lines 91 - 92, The code currently
uses a direct type assertion listener.Addr().(*net.TCPAddr) which can panic if
the listener address is not a *net.TCPAddr; update the logic in the oidc flow to
use the safe "addr, ok := listener.Addr().(*net.TCPAddr)" pattern and handle the
fallback case (e.g., parse listener.Addr().String() to extract the port or
return a clear error) before forming redirectURL :=
fmt.Sprintf("http://localhost:%d%s", addr.Port, callbackPath); ensure references
to listener.Addr(), the addr variable, redirectURL and callbackPath are updated
accordingly so non-TCP addresses are handled defensively.

bindings/go/sigstore/integration/internal/ctlog.go (1)

77-80: Consider restricting private key file mode in container mount.

The private key is correctly written to disk with 0o600 (line 61), but is mounted into the container with 0o644 (line 78). While this is an ephemeral test key in an isolated container, using 0o600 for the container mount would be more consistent with security best practices.

🔧 Suggested change

 			Files: []testcontainers.ContainerFile{
-				{HostFilePath: keyPath, ContainerFilePath: "/etc/ctfe/privkey.pem", FileMode: 0o644},
+				{HostFilePath: keyPath, ContainerFilePath: "/etc/ctfe/privkey.pem", FileMode: 0o600},
 				{HostFilePath: fulcioRootCertPath, ContainerFilePath: "/etc/fulcio/root.pem", FileMode: 0o644},
 			},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/sigstore/integration/internal/ctlog.go` around lines 77 - 80,
Change the mounted private key file permission to be more restrictive: in the
Files slice where the container mount is defined (the ContainerFile entry that
uses keyPath and ContainerFilePath "/etc/ctfe/privkey.pem"), update its FileMode
from 0o644 to 0o600 so the key inside the container matches the on-disk
permission used when writing the key.

bindings/go/sigstore/signing/handler/cosign.go (1)

66-92: Permanent caching of download failure may hinder recovery.

When ensureOrDownloadCosign() fails (e.g., due to transient network issues), the error is cached permanently in checkErr. The documentation at lines 70-72 acknowledges this and suggests creating a new DefaultExecutor to retry.

This is a reasonable trade-off for simplicity, but consider whether a time-based retry or explicit reset mechanism would improve UX for long-running processes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/sigstore/signing/handler/cosign.go` around lines 66 - 92, The
current ensureCosignAvailable method permanently caches download failures in
DefaultExecutor.checkErr (set inside checkOnce) when ensureOrDownloadCosign()
fails, preventing retries; add a safe retry/reset mechanism: implement either a
time-based retry or an explicit Reset/Refresh method on DefaultExecutor that
clears checkErr and reinitializes checkOnce so callers can trigger a retry
without constructing a new DefaultExecutor, and update New()/documentation to
mention the new API; locate the logic in ensureCosignAvailable and the fields
checkOnce/checkErr and ensureOrDownloadCosign to make the change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes