From 748ba91138d8d3a1212f24086cb5da29ee93cdf5 Mon Sep 17 00:00:00 2001 From: Chance Zibolski Date: Fri, 12 May 2017 15:37:56 -0700 Subject: [PATCH 1/3] Check response content-type to improve message if cannot decode as JSON If the Content-Type is not "application/json", add extra text indicating that the response was not JSON before propagating the unmarshal error to the caller. --- jwks.go | 7 ++++--- oidc.go | 25 ++++++++++++++++++++++--- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/jwks.go b/jwks.go index c8db7e4f..9bc6a982 100644 --- a/jwks.go +++ b/jwks.go @@ -2,7 +2,6 @@ package oidc import ( "context" - "encoding/json" "errors" "fmt" "io/ioutil" @@ -182,14 +181,16 @@ func (r *remoteKeySet) updateKeys() ([]jose.JSONWebKey, time.Time, error) { body, err := ioutil.ReadAll(resp.Body) if err != nil { - return nil, time.Time{}, fmt.Errorf("oidc: read response body: %v", err) + return nil, time.Time{}, fmt.Errorf("unable to read response body: %v", err) } + if resp.StatusCode != http.StatusOK { return nil, time.Time{}, fmt.Errorf("oidc: get keys failed: %s %s", resp.Status, body) } var keySet jose.JSONWebKeySet - if err := json.Unmarshal(body, &keySet); err != nil { + err = unmarshalResp(resp, body, &keySet) + if err != nil { return nil, time.Time{}, fmt.Errorf("oidc: failed to decode keys: %v %s", err, body) } diff --git a/oidc.go b/oidc.go index 199a9771..3385b437 100644 --- a/oidc.go +++ b/oidc.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "io/ioutil" + "mime" "net/http" "strings" "time" @@ -93,18 +94,23 @@ func NewProvider(ctx context.Context, issuer string) (*Provider, error) { if err != nil { return nil, err } + defer resp.Body.Close() + body, err := ioutil.ReadAll(resp.Body) if err != nil { - return nil, err + return nil, fmt.Errorf("unable to read response body: %v", err) } + if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("%s: %s", resp.Status, body) } - defer resp.Body.Close() + var p providerJSON - if err := json.Unmarshal(body, &p); err != nil { + err = unmarshalResp(resp, body, &p) + if err != nil { return nil, fmt.Errorf("oidc: failed to decode provider discovery object: %v", err) } + if p.Issuer != issuer { return nil, fmt.Errorf("oidc: issuer did not match the issuer returned by provider, expected %q got %q", issuer, p.Issuer) } @@ -307,3 +313,16 @@ func (j *jsonTime) UnmarshalJSON(b []byte) error { *j = jsonTime(time.Unix(unix, 0)) return nil } + +func unmarshalResp(r *http.Response, body []byte, v interface{}) error { + err := json.Unmarshal(body, &v) + if err == nil { + return nil + } + ct := r.Header.Get("Content-Type") + mediaType, _, parseErr := mime.ParseMediaType(ct) + if parseErr == nil && mediaType == "application/json" { + return fmt.Errorf("got Content-Type = application/json, but could not unmarshal as JSON: %v", err) + } + return fmt.Errorf("expected Content-Type = application/json, got %q: %v", ct, err) +} From 57af5c32b73fcc066ab21679c598ddf0dfc7c992 Mon Sep 17 00:00:00 2001 From: Taylor Thomas Date: Tue, 11 Jul 2017 15:54:42 -0700 Subject: [PATCH 2/3] fix(http): Allows 0 as an `Expires` header value This is allowed by the RFC and is common with a few OIDC providers. Partially addresses #136 as a temporary solution until k8s uses the top level package --- http/http.go | 7 ++++++- http/http_test.go | 7 +++++++ oidc/provider_test.go | 28 ++++++++++++++++++++++------ 3 files changed, 35 insertions(+), 7 deletions(-) diff --git a/http/http.go b/http/http.go index c3f51215..48717833 100644 --- a/http/http.go +++ b/http/http.go @@ -91,7 +91,12 @@ func expires(date, expires string) (time.Duration, bool, error) { return 0, false, nil } - te, err := time.Parse(time.RFC1123, expires) + var te time.Time + var err error + if expires == "0" { + return 0, false, nil + } + te, err = time.Parse(time.RFC1123, expires) if err != nil { return 0, false, err } diff --git a/http/http_test.go b/http/http_test.go index dc2cabff..48e723ab 100644 --- a/http/http_test.go +++ b/http/http_test.go @@ -177,6 +177,13 @@ func TestExpiresPass(t *testing.T) { wantTTL: 0, wantOK: false, }, + // Expires set to false + { + date: "Thu, 01 Dec 1983 22:00:00 GMT", + exp: "0", + wantTTL: 0, + wantOK: false, + }, // Expires < Date { date: "Fri, 02 Dec 1983 01:00:00 GMT", diff --git a/oidc/provider_test.go b/oidc/provider_test.go index 9b39f92c..b36e5ba3 100644 --- a/oidc/provider_test.go +++ b/oidc/provider_test.go @@ -473,8 +473,9 @@ func (g *fakeProviderConfigGetterSetter) Set(cfg ProviderConfig) error { } type fakeProviderConfigHandler struct { - cfg ProviderConfig - maxAge time.Duration + cfg ProviderConfig + maxAge time.Duration + noExpires bool } func (s *fakeProviderConfigHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { @@ -482,6 +483,9 @@ func (s *fakeProviderConfigHandler) ServeHTTP(w http.ResponseWriter, r *http.Req if s.maxAge.Seconds() >= 0 { w.Header().Set("Cache-Control", fmt.Sprintf("public, max-age=%d", int(s.maxAge.Seconds()))) } + if s.noExpires { + w.Header().Set("Expires", "0") + } w.Header().Set("Content-Type", "application/json") w.Write(b) } @@ -552,10 +556,11 @@ func TestHTTPProviderConfigGetter(t *testing.T) { now := fc.Now().UTC() tests := []struct { - dsc string - age time.Duration - cfg ProviderConfig - ok bool + dsc string + age time.Duration + cfg ProviderConfig + noExpires bool + ok bool }{ // everything is good { @@ -596,6 +601,17 @@ func TestHTTPProviderConfigGetter(t *testing.T) { }, ok: true, }, + // An expires header set to 0 + { + dsc: "https://example.com", + age: time.Minute, + cfg: ProviderConfig{ + Issuer: &url.URL{Scheme: "https", Host: "example.com"}, + ExpiresAt: now.Add(time.Minute), + }, + ok: true, + noExpires: true, + }, } for i, tt := range tests { From a5b30fd675a928770cff3d489f5dfe65912020cd Mon Sep 17 00:00:00 2001 From: Stephan Renatus Date: Sun, 1 Oct 2017 13:06:51 +0200 Subject: [PATCH 3/3] [nit] fix error message typo --- verify.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/verify.go b/verify.go index b46afdb6..d09690d1 100644 --- a/verify.go +++ b/verify.go @@ -120,7 +120,7 @@ func contains(sli []string, ele string) bool { func (v *IDTokenVerifier) Verify(ctx context.Context, rawIDToken string) (*IDToken, error) { jws, err := jose.ParseSigned(rawIDToken) if err != nil { - return nil, fmt.Errorf("oidc: mallformed jwt: %v", err) + return nil, fmt.Errorf("oidc: malformed jwt: %v", err) } // Throw out tokens with invalid claims before trying to verify the token. This lets