Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow public clients (e.g. SPAs using implicit flow or PKCE) to have redirect URLs other than localhost #1822

Merged
merged 4 commits into from
Nov 5, 2020
Merged

Allow public clients (e.g. SPAs using implicit flow or PKCE) to have redirect URLs other than localhost #1822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b894d9c
Allow public clients (e.g. using implicit flow or PKCE) to have redir…
heidemn-faro Oct 5, 2020
1ea481b
Fix gofmt in oauth2_test.go
heidemn-faro Nov 2, 2020
c15e288
Add oob, device and localhost redirect URI tests
heidemn-faro Nov 2, 2020
162073b
No longer allow desktop/mobile redirect URIs implicitly if RedirectUR…
heidemn-faro Nov 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions server/oauth2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,12 +588,16 @@ func (s *Server) validateCrossClientTrust(clientID, peerID string) (trusted bool
}

func validateRedirectURI(client storage.Client, redirectURI string) bool {
if !client.Public {
for _, uri := range client.RedirectURIs {
if redirectURI == uri {
return true
}
// Allow named RedirectURIs for both public and non-public clients.
// This is required make PKCE-enabled web apps work, when configured as public clients.
for _, uri := range client.RedirectURIs {
if redirectURI == uri {
return true
}
}
// For non-public clients or when RedirectURIs is set, we allow only explicitly named RedirectURIs.
// Otherwise, we check below for special URIs used for desktop or mobile apps.
if !client.Public || len(client.RedirectURIs) > 0 {
return false
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our use case is to use PKCE for a web app that doesn't run on "localhost".
Therefore, we would like that RedirectURIs are also considered for public clients.
PKCE was definitely also intended for web apps, and you can already configure such web apps e.g. in Okta or Auth0.

The following code to allow any localhost URI, redirectURIOOB or deviceCallbackURI for public clients is still active.
An alternative could be to no longer allow these special URLs implicitly when RedirectURIs is non-empty.
I don't have a strong opinion if this would be better. The maintainers should decide.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the implementation is good as it is. The alternative of not supporting the special URLs, when redirectURIs is defined, would break currently working configurations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally prefer the logic to only allow whitelisted redirect uris, if the list is non-empty.

This would tighten the security and would not break existing configurations that do not use RedirectURIs for public clients.
And I bet the most configurations that do currently use it for public clients operate under the false assumption that the list is actually respected (I know we did in our deployment for some time).

Copy link
Contributor

Choose a reason for hiding this comment

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

I fear that there are configurations that may be working by accident, because currently it is possible to have the RedirectURIs wrongly configured, and it still works.

Let's see what the maintainers decide.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can fix that by properly documenting this change or mentioning it in the change log.

Copy link
Member

Choose a reason for hiding this comment

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

I would personally prefer the logic to only allow whitelisted redirect uris, if the list is non-empty.

@tkleczek AFAICT this is not the case now, but the rest of the conditions provide backwards compatibility and limit redirect uris to redirectURIOOB, deviceCallbackURI or localhost for public clients which seems fine. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that for SPAs allowing localhost redirects seems like an unnecessary risk.
But if we care about potential backwards compatibility issues more, then I am fine with the change as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern is that for SPAs allowing localhost redirects seems like an unnecessary risk.

That's actually true. Consider that e.g. a malicious desktop app could obtain a token intended for https://webapp.com , since the desktop app can run a server listening on http://localhost .
If the user is already logged in at the IDP in the browser, then this could probably be done without user interaction.
The desktop app could then call the API of https://webapp.com with the token.

So I would prefer not to allow these special URLs implicitly if RedirectURIs is non-empty.

In the future, three additional boolean options could be added to explicitly (dis)allow each of redirectURIOOB, deviceCallbackURI or localhost-ish URLs. But this would definitely break backwards compatibility, and might be over-engineering.

Copy link
Contributor Author

@heidemn-faro heidemn-faro Nov 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@sagikazarmark I added more tests for these special URIs in c15e288 .
Then I made the change suggested by @tkleczek in 162073b : if !client.Public || len(client.RedirectURIs) > 0 { return false }
As you can see in the tests, it's still possible to explicitly add redirectURIOOB and deviceCallbackURI to the RedirectURIs if needed. So most mobile/desktop/localhost apps should just continue to work, and all others just need to update their configuration.

The only edge case that now doesn't work any more is a combination of 1) named RedirectURIs with 2) localhost URIs with random port. (For desktop apps, random ports must be used, since any hard-coded port might be occupied by another application.)
But this case is weird anyway (using the same client for web app + desktop app?). It can be solved by using two clients instead.

Please take another look.


Expand Down
116 changes: 116 additions & 0 deletions server/oauth2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,14 +340,23 @@ func TestValidRedirectURI(t *testing.T) {
RedirectURIs: []string{"http://foo.com/bar"},
},
redirectURI: "http://foo.com/bar/baz",
wantValid: false,
},
// These special desktop + device + localhost URIs are allowed by default.
{
client: storage.Client{
Public: true,
},
redirectURI: "urn:ietf:wg:oauth:2.0:oob",
wantValid: true,
},
{
client: storage.Client{
Public: true,
},
redirectURI: "/device/callback",
wantValid: true,
},
{
client: storage.Client{
Public: true,
Expand All @@ -369,6 +378,113 @@ func TestValidRedirectURI(t *testing.T) {
redirectURI: "http://localhost",
wantValid: true,
},
// Both Public + RedirectURIs configured: Could e.g. be a PKCE-enabled web app.
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar"},
},
redirectURI: "http://foo.com/bar",
wantValid: true,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar"},
},
redirectURI: "http://foo.com/bar/baz",
wantValid: false,
},
// These special desktop + device + localhost URIs are not allowed implicitly when RedirectURIs is non-empty.
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar"},
},
redirectURI: "urn:ietf:wg:oauth:2.0:oob",
wantValid: false,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar"},
},
redirectURI: "/device/callback",
wantValid: false,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar"},
},
redirectURI: "http://localhost:8080/",
wantValid: false,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar"},
},
redirectURI: "http://localhost:991/bar",
wantValid: false,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar"},
},
redirectURI: "http://localhost",
wantValid: false,
},
// These special desktop + device + localhost URIs can still be specified explicitly.
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar", "urn:ietf:wg:oauth:2.0:oob"},
},
redirectURI: "urn:ietf:wg:oauth:2.0:oob",
wantValid: true,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar", "/device/callback"},
},
redirectURI: "/device/callback",
wantValid: true,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar", "http://localhost:8080/"},
},
redirectURI: "http://localhost:8080/",
wantValid: true,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar", "http://localhost:991/bar"},
},
redirectURI: "http://localhost:991/bar",
wantValid: true,
},
{
client: storage.Client{
Public: true,
RedirectURIs: []string{"http://foo.com/bar", "http://localhost"},
},
redirectURI: "http://localhost",
wantValid: true,
},
// Non-localhost URIs are not allowed implicitly.
{
client: storage.Client{
Public: true,
},
redirectURI: "http://foo.com/bar",
wantValid: false,
},
{
client: storage.Client{
Public: true,
Expand Down