Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

Force TLS negotiation to only support HTTP/1.1 - Issue #298? #299

Closed
wants to merge 1 commit into from

Conversation

mvallaly-rally
Copy link

Seems OKTA has a broken implementation of HTTP2?
There seem to be some quirks with they way golang handles HTTP2 requests (specifically when receiving GOAWAY frames) golang/go#20979, which result in the HTTP2 connection not terminating.

Seems OKTA has a broken implementation of HTTP2?
There seem to be some quirks with they way golang handles HTTP2 requests (specifically when receiving GOAWAY frames) golang/go#20979, which result in the HTTP2 connection not terminating.
@brandt
Copy link
Contributor

brandt commented Sep 29, 2020

This does indeed appear to fix #298

transCfg := &http.Transport{
Proxy: http.ProxyFromEnvironment,
TLSHandshakeTimeout: Timeout,
DisableKeepAlives: true,
MaxIdleConnsPerHost: -1,
Copy link
Contributor

Choose a reason for hiding this comment

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

are these two settings also necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

These weren't required in our repro. DisableKeepAlives makes the client setup a new TLS connection for each request. Though, that's the behavior I'm already seeing without it explicitly set.

@sdann
Copy link
Contributor

sdann commented Sep 30, 2020

This fixes #298 in our environment as well, however, I've reduced it down to a minimal patch that still works:

diff --git a/lib/okta.go b/lib/okta.go
index f17cf86..a5a420d 100644
--- a/lib/okta.go
+++ b/lib/okta.go
@@ -2,6 +2,7 @@ package lib
 
 import (
        "bytes"
+       "crypto/tls"
        "encoding/json"
        "errors"
        "fmt"
@@ -578,9 +579,13 @@ func (o *OktaClient) Get(method string, path string, data []byte, recv interface
                }
        }
 
+       tlsCfg := &tls.Config{
+       }
+
        transCfg := &http.Transport{
                Proxy:               http.ProxyFromEnvironment,
                TLSHandshakeTimeout: Timeout,
+               TLSClientConfig: tlsCfg,
        }
        client = http.Client{
                Transport: transCfg,

I don't yet understand why though.

@sdann
Copy link
Contributor

sdann commented Sep 30, 2020

I don't yet understand why though.

Default is HTTP1, says the Go docs.

https://golang.org/pkg/net/http/

    // TLSClientConfig specifies the TLS configuration to use with
    // tls.Client.
    // If nil, the default configuration is used.
    // If non-nil, HTTP/2 support may not be enabled by default.
    TLSClientConfig *tls.Config

Still, better to be explicit.

@nickatsegment
Copy link
Contributor

nickatsegment commented Sep 30, 2020

Ok, I'm going to modify this PR to match @sdann's change and merge. It'd be nice to understand this behaviour better though

Update: an alternate PR apparently fixes the problem in a way that makes more sense to me. Feedback appreciated, since I haven't been able to repro myself yet.

@nickatsegment
Copy link
Contributor

Superseded by #300

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants