Skip to content

Commit 1d73d83

Browse files
alnralnr
authored
fix: reject invalid JWKS in client configuration / dependency cleanup and bump (#3603)
1 parent 5704640 commit 1d73d83

File tree

9 files changed

+389
-459
lines changed

9 files changed

+389
-459
lines changed

‎client/handler.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ type createOAuth2Client struct {
9999
func (h *Handler) createOAuth2Client(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
100100
c, err := h.CreateClient(r, h.r.ClientValidator().Validate, false)
101101
if err != nil {
102-
h.r.Writer().WriteError(w, r, errorsx.WithStack(err))
102+
h.r.Writer().WriteError(w, r, err)
103103
return
104104
}
105105

@@ -164,7 +164,7 @@ func (h *Handler) createOidcDynamicClient(w http.ResponseWriter, r *http.Request
164164
func (h *Handler) CreateClient(r *http.Request, validator func(context.Context, *Client) error, isDynamic bool) (*Client, error) {
165165
var c Client
166166
if err := json.NewDecoder(r.Body).Decode(&c); err != nil {
167-
return nil, err
167+
return nil, errorsx.WithStack(herodot.ErrBadRequest.WithReasonf("Unable to decode the request body: %s", err))
168168
}
169169

170170
if isDynamic {

‎client/validator.go‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,13 @@ import (
1010
"net/url"
1111
"strings"
1212

13+
"github.com/hashicorp/go-retryablehttp"
14+
1315
"github.com/ory/herodot"
1416
"github.com/ory/hydra/v2/driver/config"
1517
"github.com/ory/hydra/v2/x"
16-
"github.com/ory/x/ipx"
17-
1818
"github.com/ory/x/errorsx"
19-
19+
"github.com/ory/x/ipx"
2020
"github.com/ory/x/stringslice"
2121
)
2222

@@ -65,6 +65,14 @@ func (v *Validator) Validate(ctx context.Context, c *Client) error {
6565
return errorsx.WithStack(ErrInvalidClientMetadata.WithHint("Fields jwks and jwks_uri can not both be set, you must choose one."))
6666
}
6767

68+
if c.JSONWebKeys != nil && c.JSONWebKeys.JSONWebKeySet != nil {
69+
for _, k := range c.JSONWebKeys.Keys {
70+
if !k.Valid() {
71+
return errorsx.WithStack(ErrInvalidClientMetadata.WithHint("Invalid JSON web key in set."))
72+
}
73+
}
74+
}
75+
6876
if v.r.Config().ClientHTTPNoPrivateIPRanges() {
6977
values := map[string]string{
7078
"jwks_uri": c.JSONWebKeysURI,
@@ -213,7 +221,11 @@ func (v *Validator) ValidateSectorIdentifierURL(ctx context.Context, location st
213221
return errorsx.WithStack(ErrInvalidClientMetadata.WithDebug("Value sector_identifier_uri must be an HTTPS URL but it is not."))
214222
}
215223

216-
response, err := v.r.HTTPClient(ctx).Get(location)
224+
req, err := retryablehttp.NewRequestWithContext(ctx, "GET", location, nil)
225+
if err != nil {
226+
return errorsx.WithStack(ErrInvalidClientMetadata.WithDebugf("Value sector_identifier_uri must be an HTTPS URL but it is not: %s", err.Error()))
227+
}
228+
response, err := v.r.HTTPClient(ctx).Do(req)
217229
if err != nil {
218230
return errorsx.WithStack(ErrInvalidClientMetadata.WithDebug(fmt.Sprintf("Unable to connect to URL set by sector_identifier_uri: %s", err)))
219231
}

‎client/validator_test.go‎

Lines changed: 75 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,16 @@ package client_test
55

66
import (
77
"context"
8+
"encoding/json"
89
"fmt"
910
"net/http"
1011
"net/http/httptest"
12+
"strings"
1113
"testing"
1214

1315
"github.com/hashicorp/go-retryablehttp"
1416

17+
"github.com/ory/fosite"
1518
"github.com/ory/hydra/v2/driver"
1619
"github.com/ory/x/httpx"
1720

@@ -38,11 +41,16 @@ func TestValidate(t *testing.T) {
3841

3942
testCtx := context.TODO()
4043

44+
dec := json.NewDecoder(strings.NewReader(validJWKS))
45+
dec.DisallowUnknownFields()
46+
var goodJWKS jose.JSONWebKeySet
47+
require.NoError(t, dec.Decode(&goodJWKS))
48+
4149
for k, tc := range []struct {
4250
in *Client
43-
check func(t *testing.T, c *Client)
44-
expectErr bool
45-
v func(t *testing.T) *Validator
51+
check func(*testing.T, *Client)
52+
assertErr func(t assert.TestingT, err error, msg ...interface{}) bool
53+
v func(*testing.T) *Validator
4654
}{
4755
{
4856
in: new(Client),
@@ -66,31 +74,57 @@ func TestValidate(t *testing.T) {
6674
},
6775
{
6876
in: &Client{LegacyClientID: "foo", UserinfoSignedResponseAlg: "foo"},
69-
expectErr: true,
77+
assertErr: assert.Error,
7078
},
7179
{
7280
in: &Client{LegacyClientID: "foo", TokenEndpointAuthMethod: "private_key_jwt"},
73-
expectErr: true,
81+
assertErr: assert.Error,
7482
},
7583
{
7684
in: &Client{LegacyClientID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, JSONWebKeysURI: "asdf", TokenEndpointAuthMethod: "private_key_jwt"},
77-
expectErr: true,
85+
assertErr: assert.Error,
7886
},
7987
{
8088
in: &Client{LegacyClientID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, TokenEndpointAuthMethod: "private_key_jwt", TokenEndpointAuthSigningAlgorithm: "HS256"},
81-
expectErr: true,
89+
assertErr: assert.Error,
90+
},
91+
{
92+
in: &Client{LegacyClientID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: new(jose.JSONWebKeySet)}, JSONWebKeysURI: "https://example.org/jwks.json"},
93+
assertErr: func(_ assert.TestingT, err error, msg ...interface{}) bool {
94+
e := new(fosite.RFC6749Error)
95+
assert.ErrorAs(t, err, &e)
96+
assert.Contains(t, e.HintField, "jwks and jwks_uri can not both be set")
97+
return true
98+
},
99+
},
100+
{
101+
in: &Client{LegacyClientID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: &goodJWKS}},
102+
check: func(t *testing.T, c *Client) {
103+
assert.Len(t, c.JSONWebKeys.Keys, 2)
104+
assert.Equal(t, c.JSONWebKeys.Keys[0].KeyID, "1")
105+
assert.Equal(t, c.JSONWebKeys.Keys[1].KeyID, "2011-04-29")
106+
},
107+
},
108+
{
109+
in: &Client{LegacyClientID: "foo", JSONWebKeys: &x.JoseJSONWebKeySet{JSONWebKeySet: &jose.JSONWebKeySet{Keys: []jose.JSONWebKey{{}}}}},
110+
assertErr: func(_ assert.TestingT, err error, msg ...interface{}) bool {
111+
e := new(fosite.RFC6749Error)
112+
assert.ErrorAs(t, err, &e)
113+
assert.Contains(t, e.HintField, "Invalid JSON web key in set")
114+
return true
115+
},
82116
},
83117
{
84118
in: &Client{LegacyClientID: "foo", PostLogoutRedirectURIs: []string{"https://bar/"}, RedirectURIs: []string{"https://foo/"}},
85-
expectErr: true,
119+
assertErr: assert.Error,
86120
},
87121
{
88122
in: &Client{LegacyClientID: "foo", PostLogoutRedirectURIs: []string{"http://foo/"}, RedirectURIs: []string{"https://foo/"}},
89-
expectErr: true,
123+
assertErr: assert.Error,
90124
},
91125
{
92126
in: &Client{LegacyClientID: "foo", PostLogoutRedirectURIs: []string{"https://foo:1234/"}, RedirectURIs: []string{"https://foo/"}},
93-
expectErr: true,
127+
assertErr: assert.Error,
94128
},
95129
{
96130
in: &Client{LegacyClientID: "foo", PostLogoutRedirectURIs: []string{"https://foo/"}, RedirectURIs: []string{"https://foo/"}},
@@ -122,18 +156,19 @@ func TestValidate(t *testing.T) {
122156
},
123157
{
124158
in: &Client{LegacyClientID: "foo", SubjectType: "foo"},
125-
expectErr: true,
159+
assertErr: assert.Error,
126160
},
127161
} {
162+
tc := tc
128163
t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
129164
if tc.v == nil {
130165
tc.v = func(t *testing.T) *Validator {
131166
return v
132167
}
133168
}
134169
err := tc.v(t).Validate(testCtx, tc.in)
135-
if tc.expectErr {
136-
require.Error(t, err)
170+
if tc.assertErr != nil {
171+
tc.assertErr(t, err)
137172
} else {
138173
require.NoError(t, err)
139174
tc.check(t, tc.in)
@@ -205,6 +240,33 @@ func TestValidateSectorIdentifierURL(t *testing.T) {
205240
}
206241
}
207242

243+
// from https://datatracker.ietf.org/doc/html/rfc7517#appendix-A.2
244+
const validJWKS = `
245+
{"keys":
246+
[
247+
{"kty":"EC",
248+
"crv":"P-256",
249+
"x":"MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4",
250+
"y":"4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM",
251+
"d":"870MB6gfuTJ4HtUnUvYMyJpr5eUZNP4Bk43bVdj3eAE",
252+
"use":"enc",
253+
"kid":"1"},
254+
255+
{"kty":"RSA",
256+
"n":"0vx7agoebGcQSuuPiLJXZptN9nndrQmbXEps2aiAFbWhM78LhWx4cbbfAAtVT86zwu1RK7aPFFxuhDR1L6tSoc_BJECPebWKRXjBZCiFV4n3oknjhMstn64tZ_2W-5JsGY4Hc5n9yBXArwl93lqt7_RN5w6Cf0h4QyQ5v-65YGjQR0_FDW2QvzqY368QQMicAtaSqzs8KJZgnYb9c7d0zgdAZHzu6qMQvRL5hajrn1n91CbOpbISD08qNLyrdkt-bFTWhAI4vMQFh6WeZu0fM4lFd2NcRwr3XPksINHaQ-G_xBniIqbw0Ls1jF44-csFCur-kEgU8awapJzKnqDKgw",
257+
"e":"AQAB",
258+
"d":"X4cTteJY_gn4FYPsXB8rdXix5vwsg1FLN5E3EaG6RJoVH-HLLKD9M7dx5oo7GURknchnrRweUkC7hT5fJLM0WbFAKNLWY2vv7B6NqXSzUvxT0_YSfqijwp3RTzlBaCxWp4doFk5N2o8Gy_nHNKroADIkJ46pRUohsXywbReAdYaMwFs9tv8d_cPVY3i07a3t8MN6TNwm0dSawm9v47UiCl3Sk5ZiG7xojPLu4sbg1U2jx4IBTNBznbJSzFHK66jT8bgkuqsk0GjskDJk19Z4qwjwbsnn4j2WBii3RL-Us2lGVkY8fkFzme1z0HbIkfz0Y6mqnOYtqc0X4jfcKoAC8Q",
259+
"p":"83i-7IvMGXoMXCskv73TKr8637FiO7Z27zv8oj6pbWUQyLPQBQxtPVnwD20R-60eTDmD2ujnMt5PoqMrm8RfmNhVWDtjjMmCMjOpSXicFHj7XOuVIYQyqVWlWEh6dN36GVZYk93N8Bc9vY41xy8B9RzzOGVQzXvNEvn7O0nVbfs",
260+
"q":"3dfOR9cuYq-0S-mkFLzgItgMEfFzB2q3hWehMuG0oCuqnb3vobLyumqjVZQO1dIrdwgTnCdpYzBcOfW5r370AFXjiWft_NGEiovonizhKpo9VVS78TzFgxkIdrecRezsZ-1kYd_s1qDbxtkDEgfAITAG9LUnADun4vIcb6yelxk",
261+
"dp":"G4sPXkc6Ya9y8oJW9_ILj4xuppu0lzi_H7VTkS8xj5SdX3coE0oimYwxIi2emTAue0UOa5dpgFGyBJ4c8tQ2VF402XRugKDTP8akYhFo5tAA77Qe_NmtuYZc3C3m3I24G2GvR5sSDxUyAN2zq8Lfn9EUms6rY3Ob8YeiKkTiBj0",
262+
"dq":"s9lAH9fggBsoFR8Oac2R_E2gw282rT2kGOAhvIllETE1efrA6huUUvMfBcMpn8lqeW6vzznYY5SSQF7pMdC_agI3nG8Ibp1BUb0JUiraRNqUfLhcQb_d9GF4Dh7e74WbRsobRonujTYN1xCaP6TO61jvWrX-L18txXw494Q_cgk",
263+
"qi":"GyM_p6JrXySiz1toFgKbWV-JdI3jQ4ypu9rbMWx3rQJBfmt0FoYzgUIZEVFEcOqwemRN81zoDAaa-Bk0KWNGDjJHZDdDmFhW3AN7lI-puxk_mHZGJ11rxyR8O55XLSe3SPmRfKwZI6yU24ZxvQKFYItdldUKGzO6Ia6zTKhAVRU",
264+
"alg":"RS256",
265+
"kid":"2011-04-29"}
266+
]
267+
}
268+
`
269+
208270
func TestValidateIPRanges(t *testing.T) {
209271
ctx := context.Background()
210272
c := internal.NewConfigurationWithDefaults()

‎consent/strategy_default.go‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"time"
1414

1515
"github.com/gorilla/sessions"
16+
"github.com/hashicorp/go-retryablehttp"
1617
"github.com/pborman/uuid"
1718
"github.com/pkg/errors"
1819
"github.com/sirupsen/logrus"
@@ -773,12 +774,21 @@ func (s *DefaultStrategy) executeBackChannelLogout(ctx context.Context, r *http.
773774
tasks = append(tasks, task{url: c.BackChannelLogoutURI, clientID: c.GetID(), token: t})
774775
}
775776

776-
var execute = func(t task) {
777+
span := trace.SpanFromContext(ctx)
778+
cl := s.r.HTTPClient(ctx)
779+
execute := func(t task) {
777780
log := s.r.Logger().WithRequest(r).
778781
WithField("client_id", t.clientID).
779782
WithField("backchannel_logout_url", t.url)
780783

781-
res, err := s.r.HTTPClient(ctx).PostForm(t.url, url.Values{"logout_token": {t.token}})
784+
body := url.Values{"logout_token": {t.token}}.Encode()
785+
req, err := retryablehttp.NewRequestWithContext(trace.ContextWithSpan(context.Background(), span), "POST", t.url, []byte(body))
786+
if err != nil {
787+
log.WithError(err).Error("Unable to construct OpenID Connect Back-Channel Logout Request")
788+
return
789+
}
790+
req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
791+
res, err := cl.Do(req)
782792
if err != nil {
783793
log.WithError(err).Error("Unable to execute OpenID Connect Back-Channel Logout Request")
784794
return

0 commit comments

Comments
 (0)