-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTTP 301
redirection responses mangle Location
header
#272
Comments
Here's a partial reproducer in the form of k8s yamls: apiVersion: v1
items:
- apiVersion: apps/v1
kind: Deployment
metadata:
name: cryostat
spec:
progressDeadlineSeconds: 600
replicas: 1
revisionHistoryLimit: 10
selector:
matchLabels:
app.kubernetes.io/instance: cryostat
app.kubernetes.io/name: cryostat
strategy:
type: Recreate
template:
metadata:
creationTimestamp: null
labels:
app.kubernetes.io/instance: cryostat
app.kubernetes.io/name: cryostat
spec:
containers:
- args:
- --upstream=http://localhost:8181
- --cookie-secret=SECRET
- --openshift-service-account=default
- --http-address=0.0.0.0:4180
- --https-address=
- --htpasswd-file=/etc/htpasswd/htpasswd
image: quay.io/openshift/origin-oauth-proxy:latest
imagePullPolicy: Always
name: cryostat-proxy
ports:
- containerPort: 4180
protocol: TCP
resources: {}
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
volumeMounts:
- mountPath: /etc/htpasswd
name: htpasswd
readOnly: true
- env:
- name: QUARKUS_HTTP_HOST
value: 0.0.0.0
- name: QUARKUS_HTTP_PORT
value: "8181"
- name: QUARKUS_HTTP_PROXY_PROXY_ADDRESS_FORWARDING
value: "true"
- name: QUARKUS_HTTP_PROXY_ALLOW_X_FORWARDED
value: "true"
- name: QUARKUS_HTTP_PROXY_ENABLE_FORWARDED_HOST
value: "true"
- name: QUARKUS_HTTP_PROXY_ENABLE_FORWARDED_PREFIX
value: "true"
- name: QUARKUS_HIBERNATE_ORM_DATABASE_GENERATION
value: drop-and-create
- name: QUARKUS_DATASOURCE_USERNAME
value: cryostat3
- name: QUARKUS_DATASOURCE_PASSWORD
valueFrom:
secretKeyRef:
key: CONNECTION_KEY
name: cryostat-db-connection-key
optional: false
- name: QUARKUS_DATASOURCE_JDBC_URL
value: jdbc:postgresql://localhost:5432/cryostat3
- name: STORAGE_BUCKETS_ARCHIVES_NAME
value: archivedrecordings
- name: QUARKUS_S3_ENDPOINT_OVERRIDE
value: http://localhost:9000
- name: QUARKUS_S3_PATH_STYLE_ACCESS
value: "true"
- name: QUARKUS_S3_AWS_REGION
value: us-east-1
- name: QUARKUS_S3_AWS_CREDENTIALS_TYPE
value: static
- name: QUARKUS_S3_AWS_CREDENTIALS_STATIC_PROVIDER_ACCESS_KEY_ID
value: cryostat
- name: AWS_ACCESS_KEY_ID
value: cryostat
- name: QUARKUS_S3_AWS_CREDENTIALS_STATIC_PROVIDER_SECRET_ACCESS_KEY
valueFrom:
secretKeyRef:
key: SECRET_KEY
name: cryostat-storage-secret-key
optional: false
- name: AWS_SECRET_ACCESS_KEY
valueFrom:
secretKeyRef:
key: SECRET_KEY
name: cryostat-storage-secret-key
optional: false
- name: GRAFANA_DATASOURCE_URL
value: http://127.0.0.1:8080
- name: GRAFANA_DASHBOARD_URL
image: quay.io/cryostat/cryostat:3.0.0-snapshot
imagePullPolicy: Always
livenessProbe:
failureThreshold: 3
httpGet:
path: /health/liveness
port: 8181
scheme: HTTP
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
name: cryostat
ports:
- containerPort: 8181
protocol: TCP
- containerPort: 9090
protocol: TCP
- containerPort: 9091
protocol: TCP
resources: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
startupProbe:
failureThreshold: 18
httpGet:
path: /health/liveness
port: 8181
scheme: HTTP
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
- env:
- name: POSTGRESQL_USER
value: cryostat3
- name: POSTGRESQL_PASSWORD
valueFrom:
secretKeyRef:
key: CONNECTION_KEY
name: cryostat-db-connection-key
optional: false
- name: POSTGRESQL_DATABASE
value: cryostat3
- name: PG_ENCRYPT_KEY
valueFrom:
secretKeyRef:
key: ENCRYPTION_KEY
name: cryostat-db-encryption-key
optional: false
image: quay.io/cryostat/cryostat-db:2023-12-19
imagePullPolicy: Always
name: cryostat-db
ports:
- containerPort: 5432
protocol: TCP
readinessProbe:
exec:
command:
- pg_isready
- -U
- cryostat3
- -d
- cryostat3
failureThreshold: 3
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
resources: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
- args:
- server
- /tmp
- --address
- localhost:9000
env:
- name: MINIO_ROOT_USER
value: cryostat
- name: MINIO_ROOT_PASSWORD
valueFrom:
secretKeyRef:
key: SECRET_KEY
name: cryostat-storage-secret-key
optional: false
- name: MINIO_DEFAULT_BUCKETS
value: archivedrecordings
image: quay.io/cryostat/cryostat-storage:2023-12-19
imagePullPolicy: Always
name: cryostat-s3
readinessProbe:
exec:
command:
- mc
- ready
- local
failureThreshold: 3
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
resources: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
- env:
- name: JFR_DATASOURCE_URL
value: http://127.0.0.1:8080
- name: GF_AUTH_ANONYMOUS_ENABLED
value: "true"
image: quay.io/cryostat/cryostat-grafana-dashboard:latest
imagePullPolicy: Always
livenessProbe:
failureThreshold: 3
httpGet:
path: /api/health
port: 3000
scheme: HTTP
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
name: cryostat-grafana
ports:
- containerPort: 3000
protocol: TCP
resources: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
- env:
- name: LISTEN_HOST
value: 127.0.0.1
image: quay.io/cryostat/jfr-datasource:latest
imagePullPolicy: Always
livenessProbe:
exec:
command:
- curl
- --fail
- http://127.0.0.1:8080
failureThreshold: 3
periodSeconds: 10
successThreshold: 1
timeoutSeconds: 1
name: cryostat-jfr-datasource
ports:
- containerPort: 8080
protocol: TCP
resources: {}
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
dnsPolicy: ClusterFirst
restartPolicy: Always
schedulerName: default-scheduler
securityContext:
runAsNonRoot: true
seccompProfile:
type: RuntimeDefault
serviceAccount: cryostat
serviceAccountName: cryostat
terminationGracePeriodSeconds: 30
volumes:
- name: htpasswd
secret:
defaultMode: 420
secretName: basic-auth
- emptyDir: {}
name: cryostat
kind: List
metadata:
resourceVersion: "" apiVersion: v1
data:
htpasswd: dXNlcjokMnkkMDUkLnAxLzY4b0JXS1gxRmhBdFpPQXJZT29Ob01xc0I0eXVVTlhHT2VTQVRIUHEzZ2VLcUVhWlMK
kind: Secret
metadata:
name: basic-auth
type: Opaque apiVersion: v1
kind: Service
metadata:
name: cryostat
spec:
clusterIP: 10.217.5.30
clusterIPs:
- 10.217.5.30
internalTrafficPolicy: Cluster
ipFamilies:
- IPv4
ipFamilyPolicy: SingleStack
ports:
- name: cryostat-http
port: 8181
protocol: TCP
targetPort: 8181
- name: jfr-jmx
port: 9091
protocol: TCP
targetPort: 9091
- name: proxy
port: 4180
protocol: TCP
targetPort: 4180
selector:
app.kubernetes.io/instance: cryostat
app.kubernetes.io/name: cryostat
sessionAffinity: None
type: ClusterIP apiVersion: route.openshift.io/v1
kind: Route
metadata:
name: cryostat
spec:
host: cryostat-helm3.apps-crc.testing
port:
targetPort: 4180
tls:
insecureEdgeTerminationPolicy: Redirect
termination: edge
to:
kind: Service
name: cryostat
weight: 100
wildcardPolicy: None This was generated using a WIP of the Cryostat helm chart: https://github.com/andrewazores/cryostat-helm/tree/cryostat3-auth-proxy . This reproducer is partial because there are other Secrets and things missing, but hopefully it's enough on its own to illustrate roughly what the deployment looks like. $ https -v --follow --auth=user:pass cryostat-helm3.apps-crc.testing/api/v1/targets
GET /api/v1/targets HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Host: cryostat-helm3.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 308 Permanent Redirect
cache-control: no-cache
content-length: 0
date: Wed, 03 Jan 2024 15:24:19 GMT
gap-auth: user
gap-upstream-address: localhost:8181
location: https://cryostat-helm3.apps-crc.testing/api/v3/targets
set-cookie: 90b267a45b6f4102aa99aaf705f9db6c=45be247fc89f8a388efa3e6c865a1a1d; path=/; HttpOnly; Secure; SameSite=None
GET /api/v3/targets HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Cookie: 90b267a45b6f4102aa99aaf705f9db6c=45be247fc89f8a388efa3e6c865a1a1d
Host: cryostat-helm3.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 200 OK
cache-control: no-cache
content-encoding: gzip
content-length: 247
content-type: application/json;charset=UTF-8
date: Wed, 03 Jan 2024 15:24:19 GMT
gap-auth: user
gap-upstream-address: localhost:8181
[
{
"agent": false,
"alias": "service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Flocalhost%3A0%2Fjmxrmi",
"annotations": {
"cryostat": {
"REALM": "Custom Targets"
},
"platform": {}
},
"connectUrl": "service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi",
"id": 1,
"jvmId": "plkQmtTLc9mj5w87UBGbiWYQHK0f5HukfaCyXmRxmEw=",
"labels": {}
}
]
$ https -v --auth=user:pass cryostat-helm3.apps-crc.testing/api/v1/targets/$(echo -n service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi | jq -sRr @uri)
GET /api/v1/targets/service%3Ajmx%3Armi%3A%2F%2F%2Fjndi%2Frmi%3A%2F%2Flocalhost%3A0%2Fjmxrmi HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Host: cryostat-helm3.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 301 Moved Permanently
content-length: 96
content-type: text/html; charset=utf-8
date: Wed, 03 Jan 2024 15:24:35 GMT
gap-auth: user
location: /api/v1/targets/service:jmx:rmi:/jndi/rmi:/localhost:0/jmxrmi
set-cookie: 90b267a45b6f4102aa99aaf705f9db6c=45be247fc89f8a388efa3e6c865a1a1d; path=/; HttpOnly; Secure; SameSite=None
<a href="/api/v1/targets/service:jmx:rmi:/jndi/rmi:/localhost:0/jmxrmi">Moved Permanently</a>. In the first case, the 308 redirection response comes from the Cryostat application itself. This works fine because it's a simple case where there are no encoded path elements in the request nor in the redirect location. The second case with the 301 redirect response fails - this response comes from the proxy rather than Cryostat, and the decoded path is what causes a problem. |
It looks like this bug was already seen and partially fixed a long time ago: https://github.com/openshift/oauth-proxy/blob/master/oauthproxy.go#L159 I don't yet understand why this looks like it's done for the requests that actually get passed to the upstream, vs. the case I see where the request is answered with a 301 instead of being passed upstream. |
So the root cause is indeed in the golang standard library's HTTP server code. The other oauth2-proxy at some point switched to using |
Actually, perhaps this bug is fixed in the latest go development? |
That fix is supposed to land in go 1.22: |
Hmm, I tried a build using go 1.22 rc1 and it didn't seem to improve the situation. FROM registry.access.redhat.com/ubi8/ubi:8.9 AS builder
RUN dnf install -y go
RUN go install golang.org/dl/go1.22rc1@latest && \
~/go/bin/go1.22rc1 download
COPY . .
RUN ~/go/bin/go1.22rc1 build .
FROM registry.access.redhat.com/ubi8/ubi-minimal:8.9
COPY --from=builder ./oauth-proxy /usr/bin/oauth-proxy
ENTRYPOINT ["/usr/bin/oauth-proxy"] diff --git a/go.mod b/go.mod
index 1b6e297b..8d3b2034 100644
--- a/go.mod
+++ b/go.mod
@@ -1,6 +1,6 @@
module github.com/openshift/oauth-proxy
-go 1.20
+go 1.22rc1
require (
github.com/18F/hmacauth v0.0.0-20151013130326-9232a6386b73 This is running and working the same as before but the bug is not resolved. |
Maybe I did that wrong, or maybe that RC doesn't have that
$ https -v --follow --auth=user:pass cryostat-oauth-proxy.apps-crc.testing//api/v1/targets
GET //api/v1/targets HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Host: cryostat-oauth-proxy.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 301 Moved Permanently
content-length: 50
content-type: text/html; charset=utf-8
date: Thu, 04 Jan 2024 15:22:54 GMT
location: /api/v1/targets
set-cookie: ac5305cd769409458b46a6fb7e4e01e8=808f66c3cbf0311b465b26a55af74223; path=/; HttpOnly; Secure; SameSite=None
<a href="/api/v1/targets">Moved Permanently</a>.
GET /api/v1/targets HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Cookie: ac5305cd769409458b46a6fb7e4e01e8=808f66c3cbf0311b465b26a55af74223
Host: cryostat-oauth-proxy.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 308 Permanent Redirect
cache-control: no-cache
content-length: 0
date: Thu, 04 Jan 2024 15:22:54 GMT
gap-upstream-address: localhost:8181
location: https://cryostat-oauth-proxy.apps-crc.testing/api/v3/targets
GET /api/v3/targets HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Cookie: ac5305cd769409458b46a6fb7e4e01e8=808f66c3cbf0311b465b26a55af74223
Host: cryostat-oauth-proxy.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 200 OK
cache-control: no-cache
content-encoding: gzip
content-length: 28
content-type: application/json;charset=UTF-8
date: Thu, 04 Jan 2024 15:22:54 GMT
gap-upstream-address: localhost:8181
[]
$ https -v --follow --auth=user:pass cryostat-oauth-proxy.apps-crc.testing//api/v1/targets
GET //api/v1/targets HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Host: cryostat-oauth-proxy.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 301 Moved Permanently
content-length: 50
content-type: text/html; charset=utf-8
date: Thu, 04 Jan 2024 15:23:33 GMT
location: /api/v1/targets
set-cookie: ac5305cd769409458b46a6fb7e4e01e8=5943d7a600905310faf27dfd75612c51; path=/; HttpOnly; Secure; SameSite=None
<a href="/api/v1/targets">Moved Permanently</a>.
GET /api/v1/targets HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Cookie: ac5305cd769409458b46a6fb7e4e01e8=5943d7a600905310faf27dfd75612c51
Host: cryostat-oauth-proxy.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 308 Permanent Redirect
cache-control: no-cache
content-length: 0
date: Thu, 04 Jan 2024 15:23:33 GMT
gap-upstream-address: localhost:8181
location: https://cryostat-oauth-proxy.apps-crc.testing/api/v3/targets
GET /api/v3/targets HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate, br
Authorization: Basic dXNlcjpwYXNz
Connection: keep-alive
Cookie: ac5305cd769409458b46a6fb7e4e01e8=5943d7a600905310faf27dfd75612c51
Host: cryostat-oauth-proxy.apps-crc.testing
User-Agent: HTTPie/3.2.2
HTTP/1.1 200 OK
cache-control: no-cache
content-encoding: gzip
content-length: 28
content-type: application/json;charset=UTF-8
date: Thu, 04 Jan 2024 15:23:33 GMT
gap-upstream-address: localhost:8181
[] |
Updated container build: FROM registry.access.redhat.com/ubi8/ubi:8.9 AS builder
RUN dnf install -y go
RUN go install golang.org/dl/go1.22rc1@latest && \
~/go/bin/go1.22rc1 download
COPY . .
RUN ~/go/bin/go1.22rc1 build -a -v -x .
FROM registry.access.redhat.com/ubi8/ubi-minimal:8.9
COPY --from=builder ./oauth-proxy /usr/bin/oauth-proxy
ENTRYPOINT ["/usr/bin/oauth-proxy"] diff --git a/go.mod b/go.mod
index 1b6e297b..a9c8f8ad 100644
--- a/go.mod
+++ b/go.mod
@@ -1,6 +1,6 @@
module github.com/openshift/oauth-proxy
-go 1.20
+go 1.22
require (
github.com/18F/hmacauth v0.0.0-20151013130326-9232a6386b73
diff --git a/main.go b/main.go
index 637874d7..a055ad3c 100644
--- a/main.go
+++ b/main.go
@@ -188,9 +188,20 @@ func main() {
if opts.RequestLogging {
h = LoggingHandler(os.Stdout, h, true)
}
+ httpMux := oauthproxy.serveMux
s := &Server{
- Handler: h,
+ Handler: &interceptor{httpMux},
Opts: opts,
}
s.ListenAndServe(ctx)
}
+
+type interceptor struct {
+ mux http.Handler
+}
+
+func (h *interceptor) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+ // r.URL.Path = strings.Replace(r.URL.Path, "//", "/", -1)
+ log.Printf("intercepted request: %s", r.URL.String())
+ h.mux.ServeHTTP(w, r)
+} I can see from the
The upstream's response in this case is actually an HTTP 308, which we see the client then making a subsequent request that the proxy intercepts again. All working as intended. |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
@openshift-bot: Closing this issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/reopen |
@andrewazores: Reopened this issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/remove-lifecycle rotten |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
If I make a request to the proxy with a request URI containing encoded path segments, such as
GET /api/foo%2Fbar/baz
, I would expect the proxy to pass that request verbatim to the upstream service (assuming the client passes authz), or to return a redirection response with theLocation
header also set to that same verbatim request URI, potentially with segments removed as needed for proxying to different upstreams by path prefix.Instead, what I observe is that the proxy sends a 301 redirection response with the
Location
header mangled - the URI has been decoded, so my request toGET /api/foo%2Fbar/baz
is modified into a request toGET /api/foo/bar/baz
. The client issues this request as instructed by the proxy, and the upstream service can no longer understand this request.The
oauth2-proxy
had a similar bug: oauth2-proxy/oauth2-proxy#844 . Unfortunately it looks like that report and the fix came in much later than the point where these two projects diverged, so the codebase is substantially different and the patch cannot be easily backported.The text was updated successfully, but these errors were encountered: