Skip to content

Commit

Permalink
Squashed commit of the following:
Browse files Browse the repository at this point in the history
commit aed8416
Author: Benjamin Foote <[email protected]>
Date:   Thu Jun 25 13:23:31 2020 -0700

    #281 gofmt, minor edits

commit c38b05d
Author: Benjamin Foote <[email protected]>
Date:   Thu Jun 25 13:23:15 2020 -0700

    #281 compile regex once, use cfg.Branding

commit d9cb3f6
Author: Benjamin Foote <[email protected]>
Date:   Thu Jun 25 13:20:01 2020 -0700

    error on empty URL

commit 4adbaad
Author: Ben Artin <[email protected]>
Date:   Thu Jun 25 12:57:06 2020 -0400

    Fixed handling of multiple vouch params in login request

commit dfc6cb3
Author: Ben Artin <[email protected]>
Date:   Thu Jun 25 12:59:24 2020 -0400

    Allow ‘error’ param

commit e50e1aa
Author: Ben Artin <[email protected]>
Date:   Thu Jun 25 12:29:16 2020 -0400

    Clarified README unit tests

commit dabee5c
Author: Ben Artin <[email protected]>
Date:   Thu Jun 25 12:28:32 2020 -0400

    Log about url normalization

commit c1b37a7
Author: Benjamin Foote <[email protected]>
Date:   Wed Jun 24 12:17:02 2020 -0700

    #281 add tests for README 302 syntax

commit ddbea36
Author: Ben Artin <[email protected]>
Date:   Tue Jun 23 17:23:25 2020 -0400

    Allow semicolon as param separator in URL

    https://www.w3.org/TR/html401/appendix/notes.html#h-B.2.2

commit f27d4b1
Author: Ben Artin <[email protected]>
Date:   Tue Jun 23 16:59:31 2020 -0400

    Added tests for the case of vouch param before url param

commit 5b4c71d
Author: Ben Artin <[email protected]>
Date:   Tue Jun 23 16:59:05 2020 -0400

    Unambiguous test name

commit 881dfc2
Author: Ben Artin <[email protected]>
Date:   Sat Jun 20 04:21:01 2020 -0400

    More robust parsing of url param to login; see #281
  • Loading branch information
bnfinet committed Jun 25, 2020
1 parent dfbd013 commit 054bff3
Show file tree
Hide file tree
Showing 2 changed files with 152 additions and 34 deletions.
121 changes: 89 additions & 32 deletions handlers/login.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,44 +92,101 @@ func LoginHandler(w http.ResponseWriter, r *http.Request) {
}

var (
errNoURL = errors.New("no destination URL requested")
errInvalidURL = errors.New("requested destination URL appears to be invalid")
errURLNotHTTP = errors.New("requested destination URL is not a valid URL (does not begin with 'http://' or 'https://')")
errDangerQS = errors.New("requested destination URL has a dangerous query string")
badStrings = []string{"http://", "https://", "data:", "ftp://", "ftps://", "//", "javascript:"}
errNoURL = errors.New("no destination URL requested")
errInvalidURL = errors.New("requested destination URL appears to be invalid")
errURLNotHTTP = errors.New("requested destination URL is not a valid URL (does not begin with 'http://' or 'https://')")
errLoginStrayParams = errors.New("login request included unrecognized parameters")
errDangerQS = errors.New("requested destination URL has a dangerous query string")
badStrings = []string{"http://", "https://", "data:", "ftp://", "ftps://", "//", "javascript:"}
reAmpSemi = regexp.MustCompile("[&;]")
)

func getValidRequestedURL(r *http.Request) (string, error) {
// nginx is configured with...
// `return 302 https://vouch.yourdomain.com/login?url=$scheme://$http_host$request_uri&vouch-failcount=$auth_resp_failcount&X-Vouch-Token=$auth_resp_jwt&error=$auth_resp_err;`
// because `url=$scheme://$http_host$request_uri` might be..
// `url=http://protectedapp.yourdomain.com/hello?arg1=val1&arg2=val2`
// it causes `arg2=val2` to get lost if a regular evaluation of the `url` param is performedwith...
// urlparam := r.URL.Query().Get("url")
// so instead we extract in manually

urlparam := r.URL.RawQuery
urlparam = strings.Split(urlparam, "&vouch-failcount")[0]
urlparam = strings.TrimPrefix(urlparam, "url=")
log.Debugf("raw URL is %s", urlparam)

// urlparam := r.URL.Query().Get("url")
// log.Debugf("url URL is %s", urlparam)

if urlparam == "" {
return "", errNoURL
// inspect login query params to located the url param, while taking into account that the login URL may be
// presented in an RFC-non-compliant way (for example, it is common for the url param to
// not have its own query params property encoded, leading to URLs like
// http://host/login?X-Vouch-Token=token&url=http://host/path?param=value&param2=value2&vouch-failcount=value3
// where some params -- here X-Vouch-Token and vouch-failcount -- belong to login, and some others
// -- here param and param2 -- belong to the url param of login)
// The algorithm is as follows:
// * All login params starting with vouch- or x-vouch- (case insensitively) are treated as true login params
// * The "error" login param (case sensitively) is treated as true login param
// * All other login params are treated as non-login params
// * All non-login params between the url param and the first true login param are folded into the url param
// * All remaining non-login params are considered stray non-login params
// * Error is returned if the url cannot be parsed *or* it contains stray non-login params
// * For the benefit of unit-testing, if the url contains stray non-login params, it's
// returned anyway (in addition to error return)
func normalizeLoginURLParam(loginURL *url.URL) (*url.URL, error) {
// url.URL.Query return a map and therefore makes no guarantees about param order
// Therefore we have to ascertain the param order by inspecting the raw query
var urlParam *url.URL = nil // Will be url.URL for the url param
strayParams := false // Will be true if stray params are found
urlParamDone := false // Will be true when we're done building urlParam (but we're still checking for stray params)

for _, param := range reAmpSemi.Split(loginURL.RawQuery, -1) {
paramKeyVal := strings.Split(param, "=")
paramKey := paramKeyVal[0]
lcParamKey := strings.ToLower(paramKey)
isVouchParam := strings.HasPrefix(lcParamKey, cfg.Branding.LCName) || strings.HasPrefix(lcParamKey, "x-"+cfg.Branding.LCName) || paramKey == "error"

if urlParam == nil {
// Still looking for url param
if paramKey == "url" {
// Found it
parsed, e := url.Parse(loginURL.Query().Get("url"))
if e != nil {
return nil, e // failure to parse url param
}

urlParam = parsed
} else if !isVouchParam {
// Non-vouch param before url param is a stray param
log.Infof("Stray param in login request (%s)", paramKey)
strayParams = true
} // else vouch param before url param, doesn't change outcome
} else {
// Looking at params after url param
if !urlParamDone && isVouchParam {
// First vouch param after url param
urlParamDone = true
// But keep going to check for strays
} else if !urlParamDone {
// Non-vouch param after url and before first vouch param, fold it into urlParam
if urlParam.RawQuery == "" {
urlParam.RawQuery = param
} else {
urlParam.RawQuery = urlParam.RawQuery + "&" + param
}
} else if !isVouchParam {
// Non-vouch param after vouch param is a stray param
log.Infof("Stray param in login request (%s)", paramKey)
strayParams = true
} // else vouch param after vouch param, doesn't change outcome
}
}
if !strings.HasPrefix(strings.ToLower(urlparam), "http://") && !strings.HasPrefix(strings.ToLower(urlparam), "https://") {
return "", errURLNotHTTP

// if there were stray parameters return an error
log.Debugf("Login url param normalized to '%s'", urlParam)
if strayParams {
return urlParam, errLoginStrayParams
}
u, err := url.Parse(urlparam)
return urlParam, nil

}

func getValidRequestedURL(r *http.Request) (string, error) {
u, err := normalizeLoginURLParam(r.URL)

if err != nil {
return "", fmt.Errorf("won't parse: %w %s", errInvalidURL, err)
return "", fmt.Errorf("Not a valid login URL: %w %s", errInvalidURL, err)
}

_, err = url.ParseQuery(u.RawQuery)
if err != nil {
return "", fmt.Errorf("query string won't parse: %w %s", errInvalidURL, err)
if u == nil || u.String() == "" {
return "", errNoURL
}

if u.Scheme != "http" && u.Scheme != "https" {
return "", errURLNotHTTP
}

for _, v := range u.Query() {
Expand Down Expand Up @@ -158,7 +215,7 @@ func getValidRequestedURL(r *http.Request) (string, error) {
return "", fmt.Errorf("%w: mismatch between requested destination URL and '%s.cookie.secure: %v' (the cookie is only visible to 'https' but the requested site is 'http')", errInvalidURL, cfg.Branding.LCName, cfg.Cfg.Cookie.Secure)
}

return urlparam, nil
return u.String(), nil
}

func oauthLoginURL(r *http.Request, state string) string {
Expand Down
65 changes: 63 additions & 2 deletions handlers/login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,67 @@ import (
"github.com/vouch/vouch-proxy/pkg/cfg"
)

func Test_normalizeLoginURL(t *testing.T) {
setUp("/config/testing/handler_login_url.yml")
tests := []struct {
name string
url string
want string
wantErr bool
}{
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
{"extra params", "http://host/login?url=http://host/path?p2=2", "http://host/path?p2=2", false},
{"extra params (blank)", "http://host/login?url=http://host/path?p2=", "http://host/path?p2=", false},
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
// Even though the p1 param is not a login param, we do not interpret is as part of the url param because it precedes it
{"prior params", "http://host/login?p1=1&url=http://host/path", "http://host/path", true},
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
// We assume vouch-* is a login param and do not fold it into url
{"vouch-* params after", "http://host/login?url=http://host/path&vouch-xxx=2", "http://host/path", false},
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
// We assume vouch-* is a login param and do not fold it into url
{"vouch-* params before", "http://host/login?vouch-xxx=1&url=http://host/path", "http://host/path", false},
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
// We assume x-vouch-* is a login param and do not fold it into url
{"x-vouch-* params after", "http://host/login?url=http://host/path&vouch-xxx=2", "http://host/path", false},
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
// We assume x-vouch-* is a login param and do not fold it into url
{"x-vouch-* params before", "http://host/login?x-vouch-xxx=1&url=http://host/path", "http://host/path", false},
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
// Even though p1 is not a login param, we do not interpret is as part of url because it follows a login param (vouch-*)
{"params after vouch-* params", "http://host/login?url=http://host/path&vouch-xxx=2&p3=3", "http://host/path", true},
// This is not an RFC-compliant URL because it does not encode :// in the url param; we accept it anyway
// Even though p1 is not a login param, we do not interpret is as part of url because it follows a login param (x-vouch-*)
{"params after x-vouch-* params", "http://host/login?url=http://host/path&x-vouch-xxx=2&p3=3", "http://host/path", true},
// This is not an RFC-compliant URL; it combines all the aspects above
{"all params", "http://host/login?p1=1&url=http://host/path?p2=2&p3=3&x-vouch-xxx=4&vouch=5&error=6&p7=7", "http://host/path?p2=2&p3=3", true},
// This is an RFC-compliant URL
{"all params (encoded)", "http://host/login?p1=1&url=http%3a%2f%2fhost/path%3fp2=2%26p3=3&x-vouch-xxx=4&vouch=5&error=6&p7=7", "http://host/path?p2=2&p3=3", true},
// This is not an RFC-compliant URL; it combines all the aspects above, and it uses semicolons as parameter separators
// Note that when we fold a stray param into the url param, we always do so with &s
{"all params (semicolons)", "http://host/login?p1=1;url=http://host/path?p2=2;p3=3;x-vouch-xxx=4;p5=5", "http://host/path?p2=2&p3=3", true},
// This is an RFC-compliant URL that uses semicolons as parameter separators
{"all params (encoded, semicolons)", "http://host/login?p1=1;url=http%3a%2f%2fhost/path%3fp2=2%3bp3=3;x-vouch-xxx=4;p5=5", "http://host/path?p2=2;p3=3", true},
// Real world tests
// since v0.4.0 the vouch README has specified an Nginx config including a 302 redirect in the following format...
{"Vouch README (with error)", "http://host/login?url=http://host/path?p2=2&vouch-failcount=3&X-Vouch-Token=TOKEN&error=anerror", "http://host/path?p2=2", false},
{"Vouch README (blank error)", "http://host/login?url=http://host/path?p2=2&vouch-failcount=&X-Vouch-Token=&error=", "http://host/path?p2=2", false},
{"Vouch README (semicolons, blank error)", "http://host/login?url=http://host/path?p2=2;p3=3&vouch-failcount=&X-Vouch-Token=&error=", "http://host/path?p2=2&p3=3", false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
u, _ := url.Parse(tt.url)
got, err := normalizeLoginURLParam(u)
if got.String() != tt.want {
t.Errorf("normalizeLoginURLParam() = %v, want %v", got, tt.want)
}
if (err != nil) != tt.wantErr {
t.Errorf("normalizeLoginURLParam() err = %v", err)
}
})
}
}

func Test_getValidRequestedURL(t *testing.T) {
setUp("/config/testing/handler_login_url.yml")
r := &http.Request{}
Expand All @@ -33,15 +94,15 @@ func Test_getValidRequestedURL(t *testing.T) {
{"redirection chaining", "http://example.com/dest?url=https://", "", true},
{"redirection chaining upper case", "http://example.com/dest?url=HTTPS://someplaceelse.com", "", true},
{"redirection chaining no protocol", "http://example.com/dest?url=//someplaceelse.com", "", true},
{"redirection chaining escaped https://", "http://example.com/dest?url=https%3a%2f%2fsomeplaceelse.com", "", true},
{"data uri", "http://example.com/dest?url=data:text/plain,Example+Text", "", true},
{"javascript uri", "http://example.com/dest?url=javascript:alert(1)", "", true},
{"not in domain", "http://somewherelse.com/", "", true},
{"should warn", "https://example.com/", "https://example.com/", false},
{"should be fine", "http://example.com/", "http://example.com/", false},
{"multiple query param", "http://example.com/?strange=but-true&also-strange=but-false", "http://example.com/?strange=but-true&also-strange=but-false", false},
{"multiple query params, one of them bad", "http://example.com/?strange=but-true&also-strange=but-false&strange-but-bad=https://badandstrange.com", "", true},

// TODO: Add test cases.
{"multiple query params, one of them bad (escaped)", "http://example.com/?strange=but-true&also-strange=but-false&strange-but-bad=https%3a%2f%2fbadandstrange.com", "", true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down

0 comments on commit 054bff3

Please sign in to comment.