Skip to content

Commit 1ffb866

Browse files
chrjones-rhclaudenickmazzi
authored
fix(automl,autorag): resolve MinIO storage access issues (#7221)
* fix(automl,autorag): add support for MinIO storage Add S3 client support for in-cluster MinIO and other S3-compatible object stores. Three configurable options control security behavior, all defaulting to permissive for MinIO compatibility: - S3_ALLOW_HTTP=true: permit plain HTTP S3 endpoints - S3_INSECURE_SKIP_VERIFY=true: skip TLS cert verification - S3_ALLOW_INTERNAL_IPS=true: allow RFC-1918 private IPs Customers can set any of these to "false" via environment variables to enforce stricter security policies. Loopback, link-local, and reserved IP ranges remain always blocked. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(automl,autorag): clone DefaultTransport for S3 client, add permissive mode tests - Clone http.DefaultTransport instead of bare http.Transport to preserve default timeouts and connection pooling. Add 30s client timeout. - Add tests for the permissive (production-default) code paths: HTTP accepted when AllowHTTP=true, private IPs accepted when AllowInternalIPs=true, loopback/link-local still blocked when permissive, HTTP+private IP combination works. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(automl,autorag): address code review feedback for S3 client - Add CGNAT 100.64.0.0/10 to automl blocked ranges (was already in autorag) - Use named blockedRange type instead of repeated anonymous struct literals - Fix autorag to use c.options.InsecureSkipVerify (post-defaults) instead of raw opts - Add CGNAT and IPv6 ULA test cases to permissive mode tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(automl,autorag): use CA bundles for S3 TLS instead of skipping verification Replace InsecureSkipVerify, AllowHTTP, and AllowInternalIPs env vars (which were not settable in production) with proper CA bundle-based TLS verification. The RHOAI operator already mounts cluster and custom CA bundles into the BFF pod via --bundle-paths, so self-signed MinIO certificates are validated against these bundles rather than skipped. - Remove S3_INSECURE_SKIP_VERIFY, S3_ALLOW_HTTP, S3_ALLOW_INTERNAL_IPS - Add RootCAs to S3ClientOptions, populated from operator-mounted bundles - HTTPS always required (no plain HTTP) - Private IPs always allowed (MinIO runs in-cluster) - Dev-mode fallback: skip TLS verification when no CA bundles provided - Add BUNDLE_PATHS support to Makefiles for local development Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(automl,autorag): quote BUNDLE_PATHS in Makefile to prevent word-splitting Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(automl,autorag): guard DefaultTransport assertion, add TLS tests, document SSRF allowlist - Extract cloneDefaultTransport() to safely handle non-standard http.DefaultTransport replacements (e.g. in test environments) - Add TestNewRealS3Client_WithRootCAs and TestNewRealS3Client_DevModeFallback to verify both TLS transport configuration paths - Document CGN (100.64/10, RFC 6598) alongside RFC-1918 and IPv6 ULA in validateIPAddress doc comments as permitted ranges Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Nicholas Mazzitelli <nickmazz@ca.ibm.com>
1 parent 38e3af3 commit 1ffb866

10 files changed

Lines changed: 257 additions & 74 deletions

File tree

packages/automl/bff/Makefile

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ AUTH_METHOD ?= internal
1010
AUTH_TOKEN_HEADER ?= Authorization
1111
AUTH_TOKEN_PREFIX ?= Bearer
1212
INSECURE_SKIP_VERIFY ?= false
13+
BUNDLE_PATHS ?=
1314
#frontend static assets root directory
1415
STATIC_ASSETS_DIR ?= ./static
1516
# ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
@@ -63,7 +64,7 @@ endif
6364
run: fmt vet envtest ## Runs the project.
6465
trap 'exit 0' INT; \
6566
ENVTEST_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" \
66-
go run ./cmd --port=$(PORT) --auth-method=${AUTH_METHOD} --auth-token-header=$(AUTH_TOKEN_HEADER) --auth-token-prefix="$(AUTH_TOKEN_PREFIX)" --static-assets-dir=$(STATIC_ASSETS_DIR) --mock-k8s-client=$(MOCK_K8S_CLIENT) --mock-http-client=$(MOCK_HTTP_CLIENT) --mock-pipeline-server-client=$(MOCK_PIPELINE_SERVER_CLIENT) --pipeline-server-url="$(PIPELINE_SERVER_URL)" --dev-mode=$(DEV_MODE) --dev-mode-client-port=$(DEV_MODE_CLIENT_PORT) --deployment-mode=$(DEPLOYMENT_MODE) --log-level=$(LOG_LEVEL) --allowed-origins=$(ALLOWED_ORIGINS) --insecure-skip-verify=$(INSECURE_SKIP_VERIFY)
67+
go run ./cmd --port=$(PORT) --auth-method=${AUTH_METHOD} --auth-token-header=$(AUTH_TOKEN_HEADER) --auth-token-prefix="$(AUTH_TOKEN_PREFIX)" --static-assets-dir=$(STATIC_ASSETS_DIR) --mock-k8s-client=$(MOCK_K8S_CLIENT) --mock-http-client=$(MOCK_HTTP_CLIENT) --mock-pipeline-server-client=$(MOCK_PIPELINE_SERVER_CLIENT) --pipeline-server-url="$(PIPELINE_SERVER_URL)" --dev-mode=$(DEV_MODE) --dev-mode-client-port=$(DEV_MODE_CLIENT_PORT) --deployment-mode=$(DEPLOYMENT_MODE) --log-level=$(LOG_LEVEL) --allowed-origins=$(ALLOWED_ORIGINS) --insecure-skip-verify=$(INSECURE_SKIP_VERIFY) $(if $(BUNDLE_PATHS),--bundle-paths="$(BUNDLE_PATHS)")
6768

6869
##@ Dependencies
6970

packages/automl/bff/internal/api/app.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,18 @@ func NewApp(cfg config.EnvConfig, logger *slog.Logger) (*App, error) {
152152
s3ClientFactory = s3mocks.NewMockClientFactory()
153153
} else {
154154
logger.Info("Using real S3 client factory")
155-
s3ClientFactory = s3int.NewRealClientFactory(s3int.S3ClientOptions{DevMode: cfg.DevMode})
155+
// TLS verification uses the operator-mounted CA bundles (rootCAs) so that
156+
// self-signed MinIO certificates are validated properly rather than skipped.
157+
// The RHOAI operator passes --bundle-paths with cluster CA, service-ca, and
158+
// odh-trusted-ca-bundle paths, which are loaded into rootCAs above.
159+
// HTTPS is always required; plain HTTP is rejected to prevent credentials
160+
// from being transmitted in cleartext.
161+
// RFC-1918 private IPs are allowed (MinIO runs in-cluster); loopback,
162+
// link-local, and reserved ranges are always blocked.
163+
s3ClientFactory = s3int.NewRealClientFactory(s3int.S3ClientOptions{
164+
DevMode: cfg.DevMode,
165+
RootCAs: rootCAs,
166+
})
156167
}
157168

158169
app := &App{

packages/automl/bff/internal/integrations/s3/client.go

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,15 @@ import (
44
"bufio"
55
"bytes"
66
"context"
7+
"crypto/tls"
78
"encoding/csv"
89
"errors"
910
"fmt"
1011
"io"
1112
"log/slog"
1213
"math"
1314
"net"
15+
"net/http"
1416
"net/url"
1517
"os"
1618
"strconv"
@@ -97,6 +99,37 @@ func NewRealS3Client(creds *S3Credentials, opts S3ClientOptions) (*RealS3Client,
9799
Credentials: credentials.NewStaticCredentialsProvider(creds.AccessKeyID, creds.SecretAccessKey, ""),
98100
}
99101

102+
if c.options.RootCAs != nil {
103+
// Clone the default transport to preserve its timeouts and connection pooling,
104+
// then supply the custom CA pool so that self-signed or cluster-issued
105+
// certificates (e.g. MinIO) are verified against the operator-mounted
106+
// CA bundles rather than the system default.
107+
transport := cloneDefaultTransport()
108+
transport.TLSClientConfig = &tls.Config{
109+
RootCAs: c.options.RootCAs,
110+
MinVersion: tls.VersionTLS12,
111+
}
112+
cfg.HTTPClient = &http.Client{
113+
Transport: transport,
114+
Timeout: 30 * time.Second,
115+
}
116+
} else if c.options.DevMode {
117+
// In dev mode without CA bundles, skip TLS verification so developers
118+
// can test against clusters with self-signed certificates from their
119+
// local machine. In production the operator always provides CA bundles
120+
// via --bundle-paths, so this path is never reached.
121+
slog.Warn("S3 TLS certificate verification disabled (dev mode, no CA bundles provided)")
122+
transport := cloneDefaultTransport()
123+
transport.TLSClientConfig = &tls.Config{
124+
InsecureSkipVerify: true, //nolint:gosec // dev-mode only fallback
125+
MinVersion: tls.VersionTLS12,
126+
}
127+
cfg.HTTPClient = &http.Client{
128+
Transport: transport,
129+
Timeout: 30 * time.Second,
130+
}
131+
}
132+
100133
c.s3Client = awss3.NewFromConfig(cfg, func(o *awss3.Options) {
101134
o.BaseEndpoint = aws.String(validatedEndpoint)
102135
o.UsePathStyle = true
@@ -422,8 +455,26 @@ func (c *RealS3Client) GetCSVSchema(ctx context.Context, bucket, key string) (CS
422455
}, nil
423456
}
424457

458+
// cloneDefaultTransport returns a clone of http.DefaultTransport if it is an
459+
// *http.Transport, or a fresh *http.Transport otherwise. This avoids a panic
460+
// if DefaultTransport has been replaced with a non-standard implementation
461+
// (e.g. in test environments).
462+
func cloneDefaultTransport() *http.Transport {
463+
if t, ok := http.DefaultTransport.(*http.Transport); ok {
464+
return t.Clone()
465+
}
466+
return &http.Transport{}
467+
}
468+
425469
// validateAndNormalizeEndpoint validates the S3 endpoint URL to prevent SSRF attacks.
426-
// Requires HTTPS and blocks private/reserved IP ranges.
470+
//
471+
// HTTPS is required — plain HTTP is rejected because S3 credentials would be
472+
// transmitted in cleartext. Self-signed or cluster-issued certificates are
473+
// supported via the RootCAs option (populated from operator-mounted CA bundles).
474+
//
475+
// RFC-1918 private IPs are permitted because MinIO commonly runs on the same
476+
// cluster using service IPs (e.g. 10.x). Loopback, link-local, and reserved
477+
// IP ranges are always blocked.
427478
func (c *RealS3Client) validateAndNormalizeEndpoint(endpoint string) (string, error) {
428479
if endpoint == "" {
429480
return "", fmt.Errorf("endpoint URL cannot be empty")
@@ -472,21 +523,26 @@ func (c *RealS3Client) validateAndNormalizeEndpoint(endpoint string) (string, er
472523
}
473524

474525
// validateIPAddress checks if an IP address is in a blocked range.
526+
//
527+
// RFC-1918 private ranges (10/8, 172.16/12, 192.168/16), Carrier-Grade NAT
528+
// (100.64/10, RFC 6598), and IPv6 unique local addresses (fc00::/7) are
529+
// permitted because MinIO and other S3-compatible stores commonly run on the
530+
// same cluster using service or pod IPs in these ranges.
531+
// Loopback, link-local, and reserved ranges are always blocked to prevent
532+
// SSRF targeting the node or cloud metadata services.
475533
func (c *RealS3Client) validateIPAddress(ip net.IP) error {
476-
blockedRanges := []struct {
534+
type blockedRange struct {
477535
cidr string
478536
description string
479-
}{
537+
}
538+
539+
blockedRanges := []blockedRange{
480540
{"0.0.0.0/8", "reserved 'this network' range (RFC 1122)"},
481-
{"10.0.0.0/8", "RFC-1918 private range (10.0.0.0/8)"},
482-
{"172.16.0.0/12", "RFC-1918 private range (172.16.0.0/12)"},
483-
{"192.168.0.0/16", "RFC-1918 private range (192.168.0.0/16)"},
484541
{"169.254.0.0/16", "link-local range (169.254.0.0/16)"},
485542
{"127.0.0.0/8", "loopback range (127.0.0.0/8)"},
486543
{"240.0.0.0/4", "reserved for future use (RFC 1112)"},
487544
{"::1/128", "IPv6 loopback"},
488545
{"fe80::/10", "IPv6 link-local"},
489-
{"fc00::/7", "IPv6 unique local addresses"},
490546
}
491547

492548
for _, blocked := range blockedRanges {

packages/automl/bff/internal/integrations/s3/client_factory.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package s3
22

33
import (
4+
"crypto/x509"
45
"log/slog"
56
"os"
67
)
@@ -21,6 +22,15 @@ const (
2122
type S3ClientOptions struct {
2223
DevMode bool // Controls whether ALLOW_UNRESOLVED_S3_ENDPOINTS is honoured
2324

25+
// RootCAs is the CA certificate pool used to verify S3 endpoint TLS certificates.
26+
// In production the RHOAI operator mounts cluster and custom CA bundles into the
27+
// pod (e.g. odh-trusted-ca-bundle, service-ca.crt) and passes them via
28+
// --bundle-paths. When non-nil this pool is used instead of the system default,
29+
// allowing connections to MinIO or other S3-compatible stores that use
30+
// self-signed or cluster-issued certificates without skipping verification.
31+
// When nil the system CA pool is used (suitable for public S3 endpoints).
32+
RootCAs *x509.CertPool
33+
2434
// Transfer manager tuning for GetObject. Zero values fall back to defaults.
2535
Concurrency int
2636
PartSizeBytes int64

packages/automl/bff/internal/integrations/s3/client_test.go

Lines changed: 40 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package s3
22

33
import (
4+
"crypto/x509"
45
"errors"
56
"fmt"
67
"testing"
@@ -28,6 +29,29 @@ func TestNewRealS3Client_WrapsErrEndpointValidation(t *testing.T) {
2829
// validateAndNormalizeEndpoint — SSRF protection tests
2930
// ---------------------------------------------------------------------------
3031

32+
func TestNewRealS3Client_WithRootCAs(t *testing.T) {
33+
t.Parallel()
34+
pool := x509.NewCertPool()
35+
_, err := NewRealS3Client(&S3Credentials{
36+
AccessKeyID: "a",
37+
SecretAccessKey: "b",
38+
Region: "us-east-1",
39+
EndpointURL: "https://s3.amazonaws.com",
40+
}, S3ClientOptions{RootCAs: pool})
41+
assert.NoError(t, err)
42+
}
43+
44+
func TestNewRealS3Client_DevModeFallback(t *testing.T) {
45+
t.Parallel()
46+
_, err := NewRealS3Client(&S3Credentials{
47+
AccessKeyID: "a",
48+
SecretAccessKey: "b",
49+
Region: "us-east-1",
50+
EndpointURL: "https://s3.amazonaws.com",
51+
}, S3ClientOptions{DevMode: true})
52+
assert.NoError(t, err)
53+
}
54+
3155
func TestValidateAndNormalizeEndpoint_AcceptsValidHTTPS(t *testing.T) {
3256
c := newTestClient()
3357
result, err := c.validateAndNormalizeEndpoint("https://s3.us-east-1.amazonaws.com")
@@ -56,25 +80,19 @@ func TestValidateAndNormalizeEndpoint_RejectsEmpty(t *testing.T) {
5680
assert.Contains(t, err.Error(), "empty")
5781
}
5882

59-
func TestValidateAndNormalizeEndpoint_RejectsPrivateIP_10(t *testing.T) {
60-
c := newTestClient()
61-
_, err := c.validateAndNormalizeEndpoint("https://10.0.0.1:9000")
62-
assert.Error(t, err)
63-
assert.Contains(t, err.Error(), "RFC-1918")
64-
}
65-
66-
func TestValidateAndNormalizeEndpoint_RejectsPrivateIP_172(t *testing.T) {
67-
c := newTestClient()
68-
_, err := c.validateAndNormalizeEndpoint("https://172.16.0.1:9000")
69-
assert.Error(t, err)
70-
assert.Contains(t, err.Error(), "RFC-1918")
71-
}
72-
73-
func TestValidateAndNormalizeEndpoint_RejectsPrivateIP_192(t *testing.T) {
83+
func TestValidateAndNormalizeEndpoint_AcceptsPrivateIPs(t *testing.T) {
7484
c := newTestClient()
75-
_, err := c.validateAndNormalizeEndpoint("https://192.168.1.1:9000")
76-
assert.Error(t, err)
77-
assert.Contains(t, err.Error(), "RFC-1918")
85+
for _, endpoint := range []string{
86+
"https://10.0.0.1:9000",
87+
"https://100.64.0.1:9000",
88+
"https://172.16.0.1:9000",
89+
"https://192.168.1.1:9000",
90+
"https://[fd00::1]:9000",
91+
} {
92+
result, err := c.validateAndNormalizeEndpoint(endpoint)
93+
assert.NoError(t, err, "should accept %s", endpoint)
94+
assert.Equal(t, endpoint, result)
95+
}
7896
}
7997

8098
func TestValidateAndNormalizeEndpoint_RejectsLoopback(t *testing.T) {
@@ -119,11 +137,11 @@ func TestValidateAndNormalizeEndpoint_RejectsIPv6LinkLocal(t *testing.T) {
119137
assert.Contains(t, err.Error(), "IPv6 link-local")
120138
}
121139

122-
func TestValidateAndNormalizeEndpoint_RejectsIPv6UniqueLocal(t *testing.T) {
140+
func TestValidateAndNormalizeEndpoint_AcceptsIPv6UniqueLocal(t *testing.T) {
123141
c := newTestClient()
124-
_, err := c.validateAndNormalizeEndpoint("https://[fc00::1]:9000")
125-
assert.Error(t, err)
126-
assert.Contains(t, err.Error(), "IPv6 unique local")
142+
result, err := c.validateAndNormalizeEndpoint("https://[fc00::1]:9000")
143+
assert.NoError(t, err)
144+
assert.Equal(t, "https://[fc00::1]:9000", result)
127145
}
128146

129147
// ---------------------------------------------------------------------------

packages/autorag/bff/Makefile

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ AUTH_METHOD ?= user_token
1212
AUTH_TOKEN_HEADER ?= Authorization
1313
AUTH_TOKEN_PREFIX ?= Bearer
1414
INSECURE_SKIP_VERIFY ?= false
15+
BUNDLE_PATHS ?=
1516
LLAMA_STACK_URL ?= ""
1617
#frontend static assets root directory
1718
STATIC_ASSETS_DIR ?= ./static
@@ -89,7 +90,8 @@ run: fmt vet envtest ## Runs the project.
8990
--deployment-mode="$(DEPLOYMENT_MODE)" \
9091
--log-level="$(LOG_LEVEL)" \
9192
--allowed-origins="$(ALLOWED_ORIGINS)" \
92-
--insecure-skip-verify="$(INSECURE_SKIP_VERIFY)"
93+
--insecure-skip-verify="$(INSECURE_SKIP_VERIFY)" \
94+
$(if $(BUNDLE_PATHS),--bundle-paths="$(BUNDLE_PATHS)")
9395

9496
##@ Dependencies
9597

packages/autorag/bff/internal/api/app.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,18 @@ func NewApp(cfg config.EnvConfig, logger *slog.Logger) (*App, error) {
157157
s3ClientFactory = s3mocks.NewMockClientFactory()
158158
} else {
159159
logger.Info("Using real S3 client factory")
160-
s3ClientOptions := s3int.S3ClientOptions{DevMode: cfg.DevMode}
161-
s3ClientFactory = s3int.NewRealClientFactory(s3ClientOptions)
160+
// TLS verification uses the operator-mounted CA bundles (rootCAs) so that
161+
// self-signed MinIO certificates are validated properly rather than skipped.
162+
// The RHOAI operator passes --bundle-paths with cluster CA, service-ca, and
163+
// odh-trusted-ca-bundle paths, which are loaded into rootCAs above.
164+
// HTTPS is always required; plain HTTP is rejected to prevent credentials
165+
// from being transmitted in cleartext.
166+
// RFC-1918 private IPs are allowed (MinIO runs in-cluster); loopback,
167+
// link-local, and reserved ranges are always blocked.
168+
s3ClientFactory = s3int.NewRealClientFactory(s3int.S3ClientOptions{
169+
DevMode: cfg.DevMode,
170+
RootCAs: rootCAs,
171+
})
162172
}
163173

164174
app := &App{

0 commit comments

Comments
 (0)