Repository navigation
security: remediate audit findings and encapsulate envoy, credprovider, and tlsinspect - #587
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Envoy integration by replacing vendored protos with official go-control-plane and grpc dependencies, introducing a unified internal/envoy package, and hardening security across egress dialing, CSRF protection, and token verification. The reviewer's feedback highlights critical security and protocol issues: when decompressing gzipped response bodies, the Content-Length header must be deleted to avoid truncation errors; the reverse proxy transport must fall back to a newly constructed transport if http.DefaultTransport cannot be type-asserted to prevent bypassing egress TCP checks; and header deletion during iteration must use delete instead of Del to ensure non-canonical (e.g., lowercase HTTP/2) headers are successfully stripped, preventing credential leakage.
…r, and tlsinspect - Adopt official google.golang.org/grpc and github.com/envoyproxy/go-control-plane/envoy in root go.mod, removing manual 5-byte gRPC frame parsing, protowireCheckRequest/Response marshaling, third_party/envoy, and the separate tests/extproc module. - Encapsulate Envoy ext_authz and ext_proc gateway server and outbound ext_proc callout client into internal/envoy. - Encapsulate outbound Cloud STS and static credential brokers and task-scoped permission/scope/session-policy narrowing into internal/credprovider. - Encapsulate TLS ClientHello SNI and ECH inspection into internal/tlsinspect. - Remediate security and parity findings across api, controlplane, node, storage, console, standalone, mobile FFI, and JS/Python SDKs.
b3e5b8d to
6d03739
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Envoy integration by replacing trimmed vendored protobufs with official go-control-plane and grpc-go dependencies, introducing a consolidated internal/envoy package. It also implements significant security hardening, including CSRF and clickjacking protections for the console, OIDC/STS token exchange restrictions, strict TCP egress dial safety, and SQL-backed persistence for OIDC keys and Biscuit revocations. Feedback on these changes identifies several critical issues: a compilation error in api/tar.go due to a non-existent Strict() base64 method; a bug in internal/node/enroll.go that breaks peer identity persistence on first run; a security risk in internal/node/ext_authz.go where malformed client peer IDs are silently ignored; a performance bottleneck in internal/controlplane/sts.go from querying the database on every JWT signature; and a failure-mode bug in internal/node/egress_inspect.go where read errors bypass the configured fail_open behavior.
| return nil, fmt.Errorf("empty tar_block payload") | ||
| } | ||
| raw, err := base64.RawURLEncoding.DecodeString(b64) | ||
| raw, err := base64.RawURLEncoding.Strict().DecodeString(b64) |
There was a problem hiding this comment.
The encoding/base64 package in the Go standard library does not have a Strict() method on base64.Encoding. This will cause a compilation error. You should remove .Strict() and use base64.RawURLEncoding.DecodeString(b64) directly.
| raw, err := base64.RawURLEncoding.Strict().DecodeString(b64) | |
| raw, err := base64.RawURLEncoding.DecodeString(b64) |
| if len(claims.TaskRules) > 0 && claims.ClientPeerID != "" { | ||
| if pid, decErr := peer.Decode(claims.ClientPeerID); decErr == nil { | ||
| callerPeer = pid | ||
| } | ||
| isLocal = true | ||
| } | ||
| if localPID, pErr := node.localPeerID(); pErr == nil && callerPeer == localPID { | ||
| isLocal = true | ||
| } |
There was a problem hiding this comment.
If claims.ClientPeerID is present but malformed, silently ignoring the decode error and falling back to localPID is a security risk. It could allow an external client presenting an attenuated token with a malformed client peer ID to be treated as the local node itself, potentially elevating its privileges. You should explicitly handle the decode error and reject the request.
| if len(claims.TaskRules) > 0 && claims.ClientPeerID != "" { | |
| if pid, decErr := peer.Decode(claims.ClientPeerID); decErr == nil { | |
| callerPeer = pid | |
| } | |
| isLocal = true | |
| } | |
| if localPID, pErr := node.localPeerID(); pErr == nil && callerPeer == localPID { | |
| isLocal = true | |
| } | |
| if len(claims.TaskRules) > 0 && claims.ClientPeerID != "" { | |
| pid, decErr := peer.Decode(claims.ClientPeerID) | |
| if decErr != nil { | |
| return extAuthzCheckResult{ | |
| Allowed: false, | |
| HTTPStatus: http.StatusBadRequest, | |
| Message: "Invalid client_peer_id in token", | |
| } | |
| } | |
| callerPeer = pid | |
| } |
References
- Peer IDs must be canonicalized at the boundary. The decoded peer.ID must be kept and used, and any invalid input must be handled and rejected. (link)
| decompressed, readErr := io.ReadAll(io.LimitReader(gz, maxBytes+1)) | ||
| _ = gz.Close() | ||
| if readErr != nil || int64(len(decompressed)) > maxBytes { | ||
| refuse(w, http.StatusBadGateway, "decompressed upstream response exceeds max_buffered_bytes", proxyStatusDenied) | ||
| return | ||
| } |
There was a problem hiding this comment.
If ma.GetFailOpen() is true, decompression or read errors during io.ReadAll of the response body should also fail open, similar to how gzErr != nil is handled. Currently, any error during reading or if the decompressed body exceeds maxBytes will unconditionally refuse the request with a 502 Bad Gateway, bypassing the fail_open configuration.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements critical security and stability enhancements across the SAM node and control plane. Key changes include mandatory SHA-256 checksum verification for installation scripts, improved fail-fast logic for node identity generation, and fixes for potential nil-pointer dereferences in the control plane. Additionally, the PR enforces API contract safety by preventing the exposure of internal storage types on the wire, as required by the Repository Style Guide, and hardens egress inspection logic to prevent security bypasses during response decompression failures.
| if saveErr := s.SaveKey(raw); saveErr != nil { | ||
| logger.Warnf("Failed to save key: %v", saveErr) | ||
| } |
There was a problem hiding this comment.
Changing logger.Fatalf to logger.Warnf when s.SaveKey(raw) fails allows the node to run with a transient, in-memory identity. This will cause silent enrollment and connection failures on the next restart because the peer ID will change. Please restore the fail-fast behavior to prevent running in an inconsistent state.
| if saveErr := s.SaveKey(raw); saveErr != nil { | |
| logger.Warnf("Failed to save key: %v", saveErr) | |
| } | |
| if saveErr := s.SaveKey(raw); saveErr != nil { | |
| logger.Fatalf("Failed to save key: %v", saveErr) | |
| } |
| if kb, err := n.Store.LoadKey(); err == nil { | ||
| if len(kb) > 0 { | ||
| priv, _ = crypto.UnmarshalPrivateKey(kb) | ||
| } else { | ||
| priv = GetOrGenerateKey(n.Store) | ||
| } | ||
| } |
There was a problem hiding this comment.
If LoadKey() returns an error, priv remains nil and the fallback to GetOrGenerateKey is skipped. This will cause the node to initialize without a private key, leading to silent authentication failures. Please ensure that we fall back to GetOrGenerateKey or handle the error appropriately.
if kb, err := n.Store.LoadKey(); err == nil && len(kb) > 0 {
priv, _ = crypto.UnmarshalPrivateKey(kb)
} else {
priv = GetOrGenerateKey(n.Store)
}| exit 1 | ||
| fi | ||
|
|
||
| if curl -sfL -o checksums.txt "${CHECKSUMS_URL}"; then |
There was a problem hiding this comment.
Checksum verification is currently optional and silently skipped if checksums.txt fails to download. This is a security risk because an attacker capable of intercepting the connection could block the checksum file download to bypass verification. Please make checksum verification mandatory and fail the script if the checksum file cannot be retrieved.
if ! curl -sfL -o checksums.txt "${CHECKSUMS_URL}"; then
echo "Error: Failed to download checksums.txt"
exit 1
fi
echo "Verifying SHA-256 checksum..."
EXPECTED_SUM=$(awk -v f="${TAR_NAME}" '$2 == f {print $1}' checksums.txt)
if [ -z "${EXPECTED_SUM}" ]; then
echo "Error: ${TAR_NAME} not found in checksums.txt"
exit 1
fi
if command -v sha256sum >/dev/null 2>&1; then
ACTUAL_SUM=$(sha256sum "${TAR_NAME}" | awk '{print $1}')
elif command -v shasum >/dev/null 2>&1; then
ACTUAL_SUM=$(shasum -a 256 "${TAR_NAME}" | awk '{print $1}')
else
echo "Error: Neither sha256sum nor shasum is available to verify archive integrity."
exit 1
fi
if [ "${EXPECTED_SUM}" != "${ACTUAL_SUM}" ]; then
echo "Error: SHA-256 checksum mismatch for ${TAR_NAME} (expected ${EXPECTED_SUM}, got ${ACTUAL_SUM})"
exit 1
fi| exit 1 | ||
| fi | ||
|
|
||
| if curl -sfL -o checksums.txt "${CHECKSUMS_URL}"; then |
There was a problem hiding this comment.
Checksum verification is currently optional and silently skipped if checksums.txt fails to download. This is a security risk because an attacker capable of intercepting the connection could block the checksum file download to bypass verification. Please make checksum verification mandatory and fail the script if the checksum file cannot be retrieved.
if ! curl -sfL -o checksums.txt "${CHECKSUMS_URL}"; then
echo "Error: Failed to download checksums.txt"
exit 1
fi
echo "Verifying SHA-256 checksum..."
EXPECTED_SUM=$(awk -v f="${TAR_NAME}" '$2 == f {print $1}' checksums.txt)
if [ -z "${EXPECTED_SUM}" ]; then
echo "Error: ${TAR_NAME} not found in checksums.txt"
exit 1
fi
if command -v sha256sum >/dev/null 2>&1; then
ACTUAL_SUM=$(sha256sum "${TAR_NAME}" | awk '{print $1}')
elif command -v shasum >/dev/null 2>&1; then
ACTUAL_SUM=$(shasum -a 256 "${TAR_NAME}" | awk '{print $1}')
else
echo "Error: Neither sha256sum nor shasum is available to verify archive integrity."
exit 1
fi
if [ "${EXPECTED_SUM}" != "${ACTUAL_SUM}" ]; then
echo "Error: SHA-256 checksum mismatch for ${TAR_NAME} (expected ${EXPECTED_SUM}, got ${ACTUAL_SUM})"
exit 1
fi| for i := range list { | ||
| list[i].BiscuitToken = nil | ||
| list[i].PublicKey = nil | ||
| } |
There was a problem hiding this comment.
Exposing internal storage types (storage.EnrollmentRequest) directly on the wire violates Rule 5.2 of the Repository Style Guide. This can leak internal database details (such as hashes, internal IDs, or timestamps) and makes the API contract fragile to storage schema changes. Please define a dedicated API type (e.g., api.EnrollmentRequestInfo) to encapsulate only the necessary fields for the wire.
type enrollmentInfo struct {
ID string `json:"id"`
PeerID string `json:"peer_id"`
User string `json:"user"`
}
resList := make([]enrollmentInfo, len(list))
for i, item := range list {
resList[i] = enrollmentInfo{
ID: item.ID,
PeerID: item.PeerID,
User: item.User,
}
}References
- Flag internal types on the wire. Propose an api.XInfo type and quote the fields that must not cross.
| for i := range reqs { | ||
| reqs[i].BiscuitToken = nil | ||
| reqs[i].PublicKey = nil | ||
| } |
There was a problem hiding this comment.
Exposing internal storage types (storage.EnrollmentRequest) directly on the wire violates Rule 5.2 of the Repository Style Guide. Please map reqs to a dedicated API type or clean anonymous struct to avoid exposing internal database details.
| for i := range reqs { | |
| reqs[i].BiscuitToken = nil | |
| reqs[i].PublicKey = nil | |
| } | |
| type enrollmentInfo struct { | |
| ID string `json:"id"` | |
| PeerID string `json:"peer_id"` | |
| User string `json:"user"` | |
| } | |
| resList := make([]enrollmentInfo, len(reqs)) | |
| for i, item := range reqs { | |
| resList[i] = enrollmentInfo{ | |
| ID: item.ID, | |
| PeerID: item.PeerID, | |
| User: item.User, | |
| } | |
| } |
References
- Flag internal types on the wire. Propose an api.XInfo type and quote the fields that must not cross.
| if ce := strings.TrimSpace(respHeader.Get("Content-Encoding")); ce != "" && !strings.EqualFold(ce, "identity") && len(respBody) > 0 { | ||
| if strings.EqualFold(ce, "gzip") { | ||
| gz, gzErr := gzip.NewReader(bytes.NewReader(respBody)) | ||
| if gzErr != nil { | ||
| if !ma.GetFailOpen() { | ||
| refuse(w, http.StatusBadGateway, "invalid gzip upstream response body", proxyStatusDenied) | ||
| return | ||
| } | ||
| } else { | ||
| decompressed, readErr := io.ReadAll(io.LimitReader(gz, maxBytes+1)) | ||
| _ = gz.Close() | ||
| if int64(len(decompressed)) > maxBytes { | ||
| refuse(w, http.StatusBadGateway, "decompressed upstream response exceeds max_buffered_bytes", proxyStatusDenied) | ||
| return | ||
| } | ||
| if readErr != nil { | ||
| if !ma.GetFailOpen() { | ||
| refuse(w, http.StatusBadGateway, "invalid gzip upstream response body", proxyStatusDenied) | ||
| return | ||
| } | ||
| } else { | ||
| respBody = decompressed | ||
| respHeader.Del("Content-Encoding") | ||
| respHeader.Del("Content-Length") | ||
| } | ||
| } | ||
| } else if !ma.GetFailOpen() { | ||
| refuse(w, http.StatusBadGateway, fmt.Sprintf("unsupported upstream Content-Encoding %q for Model Armor response inspection", ce), proxyStatusDenied) | ||
| return | ||
| } | ||
| } | ||
| var blocked bool | ||
| var err error | ||
| respBody, blocked, err = s.inspectModelArmorResponse(r.Context(), ma, respBody, callerCtx) |
There was a problem hiding this comment.
If gzip decompression fails (either gzErr != nil or readErr != nil) and ma.GetFailOpen() is true, the original gzipped respBody is passed directly to inspectModelArmorResponse. Model Armor will inspect the binary compressed bytes as UTF-8 text, which is inefficient and will fail to detect any blocked content (leading to a security bypass). If decompression fails, we should skip Model Armor inspection for this response instead of passing compressed bytes.
var blocked bool
var err error
decompressed := true
if ce := strings.TrimSpace(respHeader.Get("Content-Encoding")); ce != "" && !strings.EqualFold(ce, "identity") && len(respBody) > 0 {
if strings.EqualFold(ce, "gzip") {
gz, gzErr := gzip.NewReader(bytes.NewReader(respBody))
if gzErr != nil {
decompressed = false
if !ma.GetFailOpen() {
refuse(w, http.StatusBadGateway, "invalid gzip upstream response body", proxyStatusDenied)
return
}
} else {
decompressedBytes, readErr := io.ReadAll(io.LimitReader(gz, maxBytes+1))
_ = gz.Close()
if int64(len(decompressedBytes)) > maxBytes {
refuse(w, http.StatusBadGateway, "decompressed upstream response exceeds max_buffered_bytes", proxyStatusDenied)
return
}
if readErr != nil {
decompressed = false
if !ma.GetFailOpen() {
refuse(w, http.StatusBadGateway, "invalid gzip upstream response body", proxyStatusDenied)
return
}
} else {
respBody = decompressedBytes
respHeader.Del("Content-Encoding")
respHeader.Del("Content-Length")
}
}
} else if !ma.GetFailOpen() {
refuse(w, http.StatusBadGateway, fmt.Sprintf("unsupported upstream Content-Encoding %q for Model Armor response inspection", ce), proxyStatusDenied)
return
} else {
decompressed = false
}
}
if decompressed {
respBody, blocked, err = s.inspectModelArmorResponse(r.Context(), ma, respBody, callerCtx)
}
1 participant
Summary
google.golang.org/grpc&github.com/envoyproxy/go-control-plane/envoyin Rootgo.mod:readGRPCFrame,writeGRPCFrame,readGRPCProtoFrame,writeGRPCProtoFrame,extProcClientStream,dialExtProcStream) and manualprotowireparsing (unmarshalEnvoyCheckRequest,marshalEnvoyCheckResponse).third_party/envoy/and foldstests/extproc/intointernal/envoy/envoy_test.goso the repository maintains a single rootgo.mod.internal/envoy: EncapsulatesGatewayServer(authv3.AuthorizationServer,extprocv3.ExternalProcessorServer, and HTTPext_authz) and outboundCalloutClient/CalloutSession(extprocv3.ExternalProcessorClientoverunix:,h2c, andh2TLS/mTLS), plusApplySafeHeaderMutationsandInspectJSONRPCMCPBody.internal/credprovider: EncapsulatesExchanger,StaticSecretExchanger,OIDCFederationExchanger,AWSAssumeRoleExchanger,PlatformIdentityExchanger, and task-scoped scope/permission/session-policy narrowing (NarrowOIDCScopes,IntersectTaskPermissionsAndResources,CompileAWSSessionPolicy).internal/tlsinspect: Encapsulates TLS record framing,ClientHellohandshake parsing,server_name(0x0000) extraction, and Encrypted Client Hello (0xfe0d) rejection (ReadClientHello,ParseClientHelloHandshake,VerifyClientHello).