Skip to content

http_proxy_errors: improve tls errors handling #910

Merged
mmatczuk merged 7 commits intomainfrom
hg/tls_errors
Sep 20, 2024
Merged

http_proxy_errors: improve tls errors handling #910
mmatczuk merged 7 commits intomainfrom
hg/tls_errors

Conversation

@Choraden
Copy link
Contributor

Fixes #549

* Request completely sent off
< HTTP/1.1 502 Bad Gateway
< Content-Length: 214
< Content-Type: text/plain; charset=utf-8
< X-Forwarder-Error: forwarder tls: failed to verify certificate: x509: certificate signed by unknown authority
<
forwarder tls handshake failed for host "localhost:8080"
unverified certificates:
- O=Sauce Labs Inc. (issued by O=Sauce Labs Inc.)

// ErrorHeader is the header that is set on error responses with the error message.
const ErrorHeader = "X-Forwarder-Error"
const (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not necessarily like the idea of grouping it, it's not related in any way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


func tlsRecordHeaderLooksLikeHTTP(hdr [5]byte) bool {
switch string(hdr[:]) {
case "HTTP/", "GET /", "HEAD ", "POST ", "PUT /", "OPTIO":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about bytes has prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this?

func tlsRecordHeaderLooksLikeHTTP(hdr [5]byte) bool {
	return bytes.HasPrefix(hdr[:], []byte("HTTP/")) ||
		bytes.HasPrefix(hdr[:], []byte("GET /")) ||
		bytes.HasPrefix(hdr[:], []byte("HEAD ")) ||
		bytes.HasPrefix(hdr[:], []byte("POST ")) ||
		bytes.HasPrefix(hdr[:], []byte("PUT /")) ||
		bytes.HasPrefix(hdr[:], []byte("OPTIO"))
}

Looks worse imo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my point is that it allows not to have /.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Done.

@Choraden Choraden force-pushed the hg/tls_errors branch 2 times, most recently from 162fa23 to bdfe2bd Compare September 18, 2024 09:56
@mmatczuk
Copy link
Contributor

Sweet.

@mmatczuk mmatczuk self-requested a review September 20, 2024 07:29
@mmatczuk mmatczuk assigned mmatczuk and unassigned mmatczuk Sep 20, 2024
@mmatczuk mmatczuk merged commit 9ec5f74 into main Sep 20, 2024
@mmatczuk mmatczuk deleted the hg/tls_errors branch September 20, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

verify that we return maximum available information when handling TLS errors

2 participants