From 86a2f997d782b1785fce191b0cc8b5f85c779456 Mon Sep 17 00:00:00 2001 From: Joe Bowers Date: Fri, 16 Oct 2015 14:46:37 -0700 Subject: [PATCH 1/7] build: don't add nested symlinks into working directory on build --- build | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build b/build index 413506e1..08179882 100755 --- a/build +++ b/build @@ -20,7 +20,7 @@ fi rm -rf $GOPATH/src/github.com/coreos/dex mkdir -p $GOPATH/src/github.com/coreos/ -ln -s ${PWD} $GOPATH/src/github.com/coreos/dex +[ -d $GOPATH/src/github.com/coreos/dex ] || ln -s ${PWD} $GOPATH/src/github.com/coreos/dex LD_FLAGS="-X main.version=$(git rev-parse HEAD)" go build -o bin/dex-worker -ldflags="$LD_FLAGS" github.com/coreos/dex/cmd/dex-worker From 85113748a8a7f0353625865d1d8c4946deabe715 Mon Sep 17 00:00:00 2001 From: Joe Bowers Date: Fri, 16 Oct 2015 14:47:58 -0700 Subject: [PATCH 2/7] server: unify password reset and email verification code and behavior This patch proposes behavioral changes. In particular, referring systems will need to provide client ids under all circumstances. --- server/password.go | 34 +++++-- server/password_test.go | 185 +++++++++++++++++++++---------------- user/email/email.go | 17 ++-- user/email_verification.go | 91 ++++-------------- user/password.go | 87 ++++------------- user/user.go | 66 +++++++++++++ 6 files changed, 242 insertions(+), 238 deletions(-) diff --git a/server/password.go b/server/password.go index 1dfa3a8b..c91ff297 100644 --- a/server/password.go +++ b/server/password.go @@ -66,31 +66,49 @@ func (h *SendResetPasswordEmailHandler) handleGET(w http.ResponseWriter, r *http log.Errorf("could not exchange sessionKey: %v", err) } data := sendResetPasswordEmailData{} - h.fillData(r, &data) + if err := h.fillData(r, &data); err != nil { + writeAPIError(w, http.StatusBadRequest, err) + } + + if data.ClientID == "" { + writeAPIError(w, http.StatusBadRequest, newAPIError(errorInvalidRequest, + "missing required parameters")) + return + } + execTemplate(w, h.tpl, data) } -func (h *SendResetPasswordEmailHandler) fillData(r *http.Request, data *sendResetPasswordEmailData) { +func (h *SendResetPasswordEmailHandler) fillData(r *http.Request, data *sendResetPasswordEmailData) *apiError { data.Email = r.FormValue("email") - clientID := r.FormValue("client_id") + data.ClientID = r.FormValue("client_id") redirectURL := r.FormValue("redirect_uri") - if redirectURL != "" && clientID != "" { - if parsed, ok := h.validateRedirectURL(clientID, redirectURL); ok { - data.ClientID = clientID + if redirectURL != "" && data.ClientID != "" { + if parsed, ok := h.validateRedirectURL(data.ClientID, redirectURL); ok { data.RedirectURL = redirectURL data.RedirectURLParsed = parsed + } else { + return newAPIError(errorInvalidRequest, "invalid redirect url") } } + return nil } func (h *SendResetPasswordEmailHandler) handlePOST(w http.ResponseWriter, r *http.Request) { data := sendResetPasswordEmailData{} - h.fillData(r, &data) + if err := h.fillData(r, &data); err != nil { + writeAPIError(w, http.StatusBadRequest, err) + } + + if data.ClientID == "" { + writeAPIError(w, http.StatusBadRequest, newAPIError(errorInvalidRequest, "client id missing")) + return + } if !user.ValidEmail(data.Email) { - h.errPage(w, "Please supply a valid email addresss.", http.StatusBadRequest, &data) + h.errPage(w, "Please supply a valid email address.", http.StatusBadRequest, &data) return } diff --git a/server/password_test.go b/server/password_test.go index ff14dfdd..4c0e02ce 100644 --- a/server/password_test.go +++ b/server/password_test.go @@ -48,7 +48,7 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { wantPRPassword string }{ // First we'll test all the requests for happy path #1: - { + { // Case 0 // STEP 1.1 - User clicks on link from local-login page and has a // session_key, which will prompt a redirect to page which has @@ -69,7 +69,7 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { }.Encode(), }, }, - { + { // Case 1 // STEP 1.2 - This is the request that happens as a result of the // redirect. The client_id and redirect_uri should be in the form on @@ -87,7 +87,7 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { "email": str(""), }, }, - { + { // Case 2 // STEP 1.3 - User enters a valid email, gets success page. The // values from the GET redirect are resent in the form POST along // with the email. @@ -109,25 +109,28 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { wantPRPassword: "password", }, - // Happy Path #2 - same as above but without session_key - { + // Happy Path #2 - no email or redirect + { // Case 3 - // STEP 2.1 - user somehow ends up on reset page without a session_key - query: url.Values{}, + // STEP 2.1 - user somehow ends up on reset page with nothing but a client id + query: url.Values{ + "client_id": str(testClientID), + }, method: "GET", wantCode: http.StatusOK, wantFormValues: &url.Values{ - "client_id": str(""), + "client_id": str(testClientID), "redirect_uri": str(""), "email": str(""), }, }, - { + { // Case 4 // STEP 2.3 - There is no STEP 2 because we don't have the redirect. query: url.Values{ - "email": str("Email-1@example.com"), + "email": str("Email-1@example.com"), + "client_id": str(testClientID), }, method: "POST", @@ -142,7 +145,7 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { }, // Some error conditions: - { + { // Case 5 // STEP 1.3.1 - User enters an invalid email, gets form again. query: url.Values{ "client_id": str(testClientID), @@ -158,7 +161,7 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { "email": str(""), }, }, - { + { // Case 6 // STEP 1.3.2 - User enters a valid email but for a user not in the // system. They still get the success page, but no email is sent. query: url.Values{ @@ -170,55 +173,32 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { wantCode: http.StatusOK, }, - { - // STEP 1.3.3 - User enters a valid email but for a user not in the - // system. They still get the success page, but no email is sent. - query: url.Values{ - "client_id": str(testClientID), - "redirect_uri": str(testRedirectURL.String()), - "email": str("NOSUCHUSER@example.com"), - }, - method: "POST", - - wantCode: http.StatusOK, - }, { + { // Case 7 // STEP 1.1.1 - User clicks on link from local-login page and has a - // session_key, but it is not-recognized. There is no redirect, the - // user goes right to the form which has no client_id or - // redirect_uri + // session_key, but it is not-recognized. query: url.Values{ "session_key": str("code-UNKNOWN"), }, method: "GET", - wantCode: http.StatusOK, - wantFormValues: &url.Values{ - "client_id": str(""), - "redirect_uri": str(""), - "email": str(""), - }, - }, { + wantCode: http.StatusBadRequest, + }, + { // Case 8 // STEP 1.2.1 - Someone trying to replace a valid redirect_url with - // an invalid one; in this case we just give them the form but - // ignore client_id and redirect_uri. + // an invalid one. query: url.Values{ "client_id": str(testClientID), "redirect_uri": str("http://evilhackers.example.com"), }, method: "GET", - wantCode: http.StatusOK, - wantFormValues: &url.Values{ - "client_id": str(""), - "redirect_uri": str(""), - "email": str(""), - }, - }, { + wantCode: http.StatusBadRequest, + }, + { // Case 9 // STEP 1.3.4 - User enters a valid email for a user in the system, - // but with an invalid redirect_uri. They still get an email, but - // with no redirect url. + // but with an invalid redirect_uri. query: url.Values{ "client_id": str(testClientID), "redirect_uri": str("http://evilhackers.example.com"), @@ -226,15 +206,43 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { }, method: "POST", - wantCode: http.StatusOK, - wantEmailer: &testEmailer{ - to: str("Email-1@example.com"), - from: "noreply@example.com", - subject: "Reset your password.", + wantCode: http.StatusBadRequest, + }, + { // Case 10 + + // User hits the page with a valid email but no client id + query: url.Values{ + "email": str("Email-1@example.com"), }, - wantPRPassword: "password", - wantPRUserID: "ID-1", - wantPRRedirect: nil, + method: "GET", + + wantCode: http.StatusBadRequest, + }, + { // Case 10 + + // Don't send an email without a client id + query: url.Values{ + "email": str("Email-1@example.com"), + }, + method: "POST", + + wantCode: http.StatusBadRequest, + }, + { // Case 11 + + // Empty requests lack a client id + query: url.Values{}, + method: "GET", + + wantCode: http.StatusBadRequest, + }, + { // Case 12 + + // Empty requests lack a client id + query: url.Values{}, + method: "POST", + + wantCode: http.StatusBadRequest, }, } @@ -348,23 +356,19 @@ func TestSendResetPasswordEmailHandler(t *testing.T) { } func TestResetPasswordHandler(t *testing.T) { - makeToken := func(userID, password string, callback url.URL, expires time.Duration, signer jose.Signer) string { - var clientID string - if callback.String() == "" { - clientID = "" - } else { - clientID = testClientID - } + makeToken := func(userID, password, clientID string, callback url.URL, expires time.Duration, signer jose.Signer) string { pr := user.NewPasswordReset(user.User{ID: "ID-1"}, user.Password(password), testIssuerURL, clientID, callback, expires) - token, err := pr.Token(signer) + + jwt, err := jose.NewSignedJWT(pr.Claims, signer) if err != nil { t.Fatalf("couldn't make token: %q", err) } + token := jwt.Encode() return token } goodSigner := key.NewPrivateKeySet([]*key.PrivateKey{testPrivKey}, @@ -398,24 +402,24 @@ func TestResetPasswordHandler(t *testing.T) { wantPassword string }{ // Scenario 1: Happy Path - { + { // Case 0 // Step 1.1 - User clicks link in email, has valid token. query: url.Values{ - "token": str(makeToken("ID-1", "password", testRedirectURL, time.Hour*1, goodSigner)), + "token": str(makeToken("ID-1", "password", testClientID, testRedirectURL, time.Hour*1, goodSigner)), }, method: "GET", wantCode: http.StatusOK, wantFormValues: &url.Values{ "password": str(""), - "token": str(makeToken("ID-1", "password", testRedirectURL, time.Hour*1, goodSigner)), + "token": str(makeToken("ID-1", "password", testClientID, testRedirectURL, time.Hour*1, goodSigner)), }, wantPassword: "password", }, - { + { // Case 1 // Step 1.2 - User enters in new valid password, password is changed, user is redirected. query: url.Values{ - "token": str(makeToken("ID-1", "password", testRedirectURL, time.Hour*1, goodSigner)), + "token": str(makeToken("ID-1", "password", testClientID, testRedirectURL, time.Hour*1, goodSigner)), "password": str("new_password"), }, method: "POST", @@ -424,26 +428,25 @@ func TestResetPasswordHandler(t *testing.T) { wantFormValues: &url.Values{}, wantPassword: "NEW_PASSWORD", }, - // Scenario 2: Happy Path, but without redirect. - { + { // Case 2 // Step 2.1 - User clicks link in email, has valid token. query: url.Values{ - "token": str(makeToken("ID-1", "password", url.URL{}, time.Hour*1, goodSigner)), + "token": str(makeToken("ID-1", "password", testClientID, url.URL{}, time.Hour*1, goodSigner)), }, method: "GET", wantCode: http.StatusOK, wantFormValues: &url.Values{ "password": str(""), - "token": str(makeToken("ID-1", "password", url.URL{}, time.Hour*1, goodSigner)), + "token": str(makeToken("ID-1", "password", testClientID, url.URL{}, time.Hour*1, goodSigner)), }, wantPassword: "password", }, - { + { // Case 3 // Step 2.2 - User enters in new valid password, password is changed, user is redirected. query: url.Values{ - "token": str(makeToken("ID-1", "password", url.URL{}, time.Hour*1, goodSigner)), + "token": str(makeToken("ID-1", "password", testClientID, url.URL{}, time.Hour*1, goodSigner)), "password": str("new_password"), }, method: "POST", @@ -454,10 +457,10 @@ func TestResetPasswordHandler(t *testing.T) { wantPassword: "NEW_PASSWORD", }, // Errors - { + { // Case 4 // Step 1.1.1 - User clicks link in email, has invalid token. query: url.Values{ - "token": str(makeToken("ID-1", "password", testRedirectURL, time.Hour*1, badSigner)), + "token": str(makeToken("ID-1", "password", testClientID, testRedirectURL, time.Hour*1, badSigner)), }, method: "GET", @@ -466,10 +469,10 @@ func TestResetPasswordHandler(t *testing.T) { wantPassword: "password", }, - { - // Step 2.2.1 - User enters in new valid password, password is changed, user is redirected. + { // Case 5 + // Step 2.2.1 - User enters in new valid password, password is changed, no redirect query: url.Values{ - "token": str(makeToken("ID-1", "password", url.URL{}, time.Hour*1, goodSigner)), + "token": str(makeToken("ID-1", "password", testClientID, url.URL{}, time.Hour*1, goodSigner)), "password": str("shrt"), }, method: "POST", @@ -478,14 +481,14 @@ func TestResetPasswordHandler(t *testing.T) { wantCode: http.StatusBadRequest, wantFormValues: &url.Values{ "password": str(""), - "token": str(makeToken("ID-1", "password", url.URL{}, time.Hour*1, goodSigner)), + "token": str(makeToken("ID-1", "password", testClientID, url.URL{}, time.Hour*1, goodSigner)), }, wantPassword: "password", }, - { + { // Case 6 // Step 2.2.2 - User enters in new valid password, with suspicious token. query: url.Values{ - "token": str(makeToken("ID-1", "password", url.URL{}, time.Hour*1, badSigner)), + "token": str(makeToken("ID-1", "password", testClientID, url.URL{}, time.Hour*1, badSigner)), "password": str("shrt"), }, method: "POST", @@ -495,6 +498,28 @@ func TestResetPasswordHandler(t *testing.T) { wantFormValues: &url.Values{}, wantPassword: "password", }, + { // Case 7 + // Token lacking client id + query: url.Values{ + "token": str(makeToken("ID-1", "password", "", url.URL{}, time.Hour*1, goodSigner)), + "password": str("shrt"), + }, + method: "GET", + + wantCode: http.StatusBadRequest, + wantPassword: "password", + }, + { // Case 8 + // Token lacking client id + query: url.Values{ + "token": str(makeToken("ID-1", "password", "", url.URL{}, time.Hour*1, goodSigner)), + "password": str("shrt"), + }, + method: "POST", + + wantCode: http.StatusBadRequest, + wantPassword: "password", + }, } for i, tt := range tests { f, err := makeTestFixtures() diff --git a/user/email/email.go b/user/email/email.go index ab6377e8..9203327d 100644 --- a/user/email/email.go +++ b/user/email/email.go @@ -76,18 +76,19 @@ func (u *UserEmailer) SendResetPasswordEmail(email string, redirectURL url.URL, } signer, err := u.signerFn() - if err != nil { - log.Errorf("error getting signer: %v", err) + if err != nil || signer == nil { + log.Errorf("error getting signer: %v (%v)", err, signer) return nil, err } passwordReset := user.NewPasswordReset(usr, pwi.Password, u.issuerURL, clientID, redirectURL, u.tokenValidityWindow) - token, err := passwordReset.Token(signer) + jwt, err := jose.NewSignedJWT(passwordReset.Claims, signer) if err != nil { - log.Errorf("error getting tokenizing PasswordReset: %v", err) + log.Errorf("error constructing or signing PasswordReset JWT: %v", err) return nil, err } + token := jwt.Encode() resetURL := u.passwordResetURL q := resetURL.Query() @@ -124,15 +125,17 @@ func (u *UserEmailer) SendEmailVerification(userID, clientID string, redirectURL ev := user.NewEmailVerification(usr, clientID, u.issuerURL, redirectURL, u.tokenValidityWindow) signer, err := u.signerFn() - if err != nil { - log.Errorf("error getting signer: %v", err) + if err != nil || signer == nil { + log.Errorf("error getting signer: %v (signer: %v)", err, signer) return nil, err } - token, err := ev.Token(signer) + jwt, err := jose.NewSignedJWT(ev.Claims, signer) if err != nil { + log.Errorf("error constructing or signing EmailVerification JWT: %v", err) return nil, err } + token := jwt.Encode() verifyURL := u.verifyEmailURL q := verifyURL.Query() diff --git a/user/email_verification.go b/user/email_verification.go index 991941f9..ab8204a1 100644 --- a/user/email_verification.go +++ b/user/email_verification.go @@ -1,7 +1,6 @@ package user import ( - "errors" "fmt" "net/url" "time" @@ -13,15 +12,6 @@ import ( "github.com/coreos/go-oidc/oidc" ) -const ( - // Claim representing where a user should be sent after verifying their email address. - ClaimEmailVerificationCallback = "http://coreos.com/email/verification-callback" - - // ClaimEmailVerificationEmail represents the email to be verified. Note - // that we are intentionally not using the "email" claim for this purpose. - ClaimEmailVerificationEmail = "http://coreos.com/email/verificationEmail" -) - var ( clock = clockwork.NewRealClock() ) @@ -29,7 +19,6 @@ var ( // NewEmailVerification creates an object which can be sent to a user in serialized form to verify that they control an email address. // The clientID is the ID of the registering user. The callback is where a user should land after verifying their email. func NewEmailVerification(user User, clientID string, issuer url.URL, callback url.URL, expires time.Duration) EmailVerification { - claims := oidc.NewClaims(issuer.String(), user.ID, clientID, clock.Now(), clock.Now().Add(expires)) claims.Add(ClaimEmailVerificationCallback, callback.String()) claims.Add(ClaimEmailVerificationEmail, user.Email) @@ -37,90 +26,46 @@ func NewEmailVerification(user User, clientID string, issuer url.URL, callback u } type EmailVerification struct { - claims jose.Claims + Claims jose.Claims } -// Token serializes the EmailVerification into a signed JWT. -func (e EmailVerification) Token(signer jose.Signer) (string, error) { - if signer == nil { - return "", errors.New("no signer") - } - - jwt, err := jose.NewSignedJWT(e.claims, signer) - if err != nil { - return "", err - } - - return jwt.Encode(), nil -} - -// ParseAndVerifyEmailVerificationToken parses a string into a an EmailVerification, verifies the signature, and ensures that required claims are present. -// In addition to the usual claims required by the OIDC spec, "aud" and "sub" must be present as well as ClaimEmailVerificationCallback and ClaimEmailVerificationEmail. -func ParseAndVerifyEmailVerificationToken(token string, issuer url.URL, keys []key.PublicKey) (EmailVerification, error) { - jwt, err := jose.ParseJWT(token) +// Assumes that parseAndVerifyTokenClaims has already been called on claims +func verifyEmailVerificationClaims(claims jose.Claims) (EmailVerification, error) { + email, ok, err := claims.StringClaim(ClaimEmailVerificationEmail) if err != nil { return EmailVerification{}, err } - - claims, err := jwt.Claims() - if err != nil { - return EmailVerification{}, err - } - - clientID, ok, err := claims.StringClaim("aud") - if err != nil { - return EmailVerification{}, err - } - if !ok { - return EmailVerification{}, errors.New("no aud(client ID) claim") + if !ok || email == "" { + return EmailVerification{}, fmt.Errorf("no %q claim", ClaimEmailVerificationEmail) } cb, ok, err := claims.StringClaim(ClaimEmailVerificationCallback) if err != nil { return EmailVerification{}, err } - if cb == "" { + if !ok || cb == "" { return EmailVerification{}, fmt.Errorf("no %q claim", ClaimEmailVerificationCallback) } if _, err := url.Parse(cb); err != nil { return EmailVerification{}, fmt.Errorf("callback URL not parseable: %v", cb) } - email, ok, err := claims.StringClaim(ClaimEmailVerificationEmail) + return EmailVerification{claims}, nil +} + +// ParseAndVerifyEmailVerificationToken parses a string into a an EmailVerification, verifies the signature, and ensures that required claims are present. +// In addition to the usual claims required by the OIDC spec, "aud" and "sub" must be present as well as ClaimEmailVerificationCallback and ClaimEmailVerificationEmail. +func ParseAndVerifyEmailVerificationToken(token string, issuer url.URL, keys []key.PublicKey) (EmailVerification, error) { + tokenClaims, err := parseAndVerifyTokenClaims(token, issuer, keys) if err != nil { return EmailVerification{}, err } - if email == "" { - return EmailVerification{}, fmt.Errorf("no %q claim", ClaimEmailVerificationEmail) - } - - sub, ok, err := claims.StringClaim("sub") - if err != nil { - return EmailVerification{}, err - } - if sub == "" { - return EmailVerification{}, errors.New("no sub claim") - } - - noop := func() error { return nil } - - keysFunc := func() []key.PublicKey { - return keys - } - - verifier := oidc.NewJWTVerifier(issuer.String(), clientID, noop, keysFunc) - if err := verifier.Verify(jwt); err != nil { - return EmailVerification{}, err - } - - return EmailVerification{ - claims: claims, - }, nil + return verifyEmailVerificationClaims(tokenClaims.Claims) } func (e EmailVerification) UserID() string { - uid, ok, err := e.claims.StringClaim("sub") + uid, ok, err := e.Claims.StringClaim("sub") if !ok || err != nil { panic("EmailVerification: no sub claim. This should be impossible.") } @@ -128,7 +73,7 @@ func (e EmailVerification) UserID() string { } func (e EmailVerification) Email() string { - email, ok, err := e.claims.StringClaim(ClaimEmailVerificationEmail) + email, ok, err := e.Claims.StringClaim(ClaimEmailVerificationEmail) if !ok || err != nil { panic("EmailVerification: no email claim. This should be impossible.") } @@ -136,7 +81,7 @@ func (e EmailVerification) Email() string { } func (e EmailVerification) Callback() *url.URL { - cb, ok, err := e.claims.StringClaim(ClaimEmailVerificationCallback) + cb, ok, err := e.Claims.StringClaim(ClaimEmailVerificationCallback) if !ok || err != nil { panic("EmailVerification: no callback claim. This should be impossible.") } diff --git a/user/password.go b/user/password.go index b2afa5f9..a734d40e 100644 --- a/user/password.go +++ b/user/password.go @@ -26,14 +26,6 @@ const ( // since the bcrypt library will silently ignore portions of // a password past the first 72 characters. maxSecretLength = 72 - - // ClaimPasswordResetCallback represents where a user should be sent after - // resetting their password. - ClaimPasswordResetCallback = "http://coreos.com/password/reset-callback" - - // ClaimPasswordResetPassword represents the hash of the password to be - // reset; in other words, the old password. - ClaimPasswordResetPassword = "http://coreos.com/password/old-hash" ) var ( @@ -224,56 +216,22 @@ func NewPasswordInfoRepoFromFile(loc string) (PasswordInfoRepo, error) { func NewPasswordReset(user User, password Password, issuer url.URL, clientID string, callback url.URL, expires time.Duration) PasswordReset { claims := oidc.NewClaims(issuer.String(), user.ID, clientID, clock.Now(), clock.Now().Add(expires)) - claims.Add(ClaimPasswordResetCallback, callback.String()) claims.Add(ClaimPasswordResetPassword, string(password)) + claims.Add(ClaimPasswordResetCallback, callback.String()) return PasswordReset{claims} } type PasswordReset struct { - claims jose.Claims + Claims jose.Claims } -// Token serializes the PasswordReset into a signed JWT. -func (e PasswordReset) Token(signer jose.Signer) (string, error) { - if signer == nil { - return "", errors.New("no signer") - } - - jwt, err := jose.NewSignedJWT(e.claims, signer) - if err != nil { - return "", err - } - - return jwt.Encode(), nil -} - -// ParseAndVerifyPasswordResetToken parses a string into a an PasswordReset, verifies the signature, and ensures that required claims are present. -// In addition to the usual claims required by the OIDC spec, "aud" and "sub" must be present as well as ClaimPasswordResetCallback, ClaimPasswordResetEmail and ClaimPasswordResetPassword. -func ParseAndVerifyPasswordResetToken(token string, issuer url.URL, keys []key.PublicKey) (PasswordReset, error) { - jwt, err := jose.ParseJWT(token) - if err != nil { - return PasswordReset{}, err - } - - claims, err := jwt.Claims() - if err != nil { - return PasswordReset{}, err - } - +// Assumes that parseAndVerifyTokenClaims has already been called on claims +func verifyPasswordResetClaims(claims jose.Claims) (PasswordReset, error) { cb, ok, err := claims.StringClaim(ClaimPasswordResetCallback) if err != nil { return PasswordReset{}, err } - var clientID string - if ok && cb != "" { - clientID, ok, err = claims.StringClaim("aud") - if err != nil { - return PasswordReset{}, err - } - if !ok || clientID == "" { - return PasswordReset{}, errors.New("no aud(client ID) claim") - } - } + if _, err := url.Parse(cb); err != nil { return PasswordReset{}, fmt.Errorf("callback URL not parseable: %v", cb) } @@ -282,37 +240,26 @@ func ParseAndVerifyPasswordResetToken(token string, issuer url.URL, keys []key.P if err != nil { return PasswordReset{}, err } - if pw == "" { + if !ok || pw == "" { return PasswordReset{}, fmt.Errorf("no %q claim", ClaimPasswordResetPassword) } - sub, ok, err := claims.StringClaim("sub") + return PasswordReset{claims}, nil +} + +// ParseAndVerifyPasswordResetToken parses a string into a an PasswordReset, verifies the signature, and ensures that required claims are present. +// In addition to the usual claims required by the OIDC spec, "aud" and "sub" must be present as well as ClaimPasswordResetCallback, ClaimPasswordResetEmail and ClaimPasswordResetPassword. +func ParseAndVerifyPasswordResetToken(token string, issuer url.URL, keys []key.PublicKey) (PasswordReset, error) { + tokenClaims, err := parseAndVerifyTokenClaims(token, issuer, keys) if err != nil { return PasswordReset{}, err } - if sub == "" { - return PasswordReset{}, errors.New("no sub claim") - } - - noop := func() error { return nil } - - keysFunc := func() []key.PublicKey { - return keys - } - - verifier := oidc.NewJWTVerifier(issuer.String(), clientID, noop, keysFunc) - if err := verifier.Verify(jwt); err != nil { - return PasswordReset{}, err - } - - return PasswordReset{ - claims: claims, - }, nil + return verifyPasswordResetClaims(tokenClaims.Claims) } func (e PasswordReset) UserID() string { - uid, ok, err := e.claims.StringClaim("sub") + uid, ok, err := e.Claims.StringClaim("sub") if !ok || err != nil { panic("PasswordReset: no sub claim. This should be impossible.") } @@ -320,7 +267,7 @@ func (e PasswordReset) UserID() string { } func (e PasswordReset) Password() Password { - pw, ok, err := e.claims.StringClaim(ClaimPasswordResetPassword) + pw, ok, err := e.Claims.StringClaim(ClaimPasswordResetPassword) if !ok || err != nil { panic("PasswordReset: no password claim. This should be impossible.") } @@ -328,7 +275,7 @@ func (e PasswordReset) Password() Password { } func (e PasswordReset) Callback() *url.URL { - cb, ok, err := e.claims.StringClaim(ClaimPasswordResetCallback) + cb, ok, err := e.Claims.StringClaim(ClaimPasswordResetCallback) if err != nil { panic("PasswordReset: error getting string claim. This should be impossible.") } diff --git a/user/user.go b/user/user.go index e771e667..6a56eb8c 100644 --- a/user/user.go +++ b/user/user.go @@ -8,6 +8,7 @@ import ( "time" "net/mail" + "net/url" "os" "sort" @@ -15,10 +16,30 @@ import ( "github.com/coreos/dex/repo" "github.com/coreos/go-oidc/jose" + "github.com/coreos/go-oidc/key" + "github.com/coreos/go-oidc/oidc" ) const ( MaxEmailLength = 200 + + // ClaimPasswordResetPassword represents the hash of the password to be + // reset; in other words, the old password + ClaimPasswordResetPassword = "http://coreos.com/password/old-hash" + + // ClaimEmailVerificationEmail represents the email to be verified. Note + // that we are intentionally not using the "email" claim for this purpose. + ClaimEmailVerificationEmail = "http://coreos.com/email/verificationEmail" + + // ClaimPasswordResetCallback represents where a user should be sent after + // resetting their password. + ClaimPasswordResetCallback = "http://coreos.com/password/reset-callback" + + // Claim representing where a user should be sent after verifying their email address. + ClaimEmailVerificationCallback = "http://coreos.com/email/verification-callback" + + // Claim representing where a user should be sent after responding to an invitation + ClaimInvitationCallback = "http://coreos.com/invitation/callback" ) type UserIDGenerator func() (string, error) @@ -422,3 +443,48 @@ func (u *RemoteIdentity) UnmarshalJSON(data []byte) error { return nil } + +type TokenClaims struct { + Claims jose.Claims +} + +func parseAndVerifyTokenClaims(token string, issuer url.URL, keys []key.PublicKey) (TokenClaims, error) { + jwt, err := jose.ParseJWT(token) + if err != nil { + return TokenClaims{}, err + } + + claims, err := jwt.Claims() + if err != nil { + return TokenClaims{}, err + } + + clientID, ok, err := claims.StringClaim("aud") + if err != nil { + return TokenClaims{}, err + } + if !ok || clientID == "" { + return TokenClaims{}, errors.New("no aud(client ID) claim") + } + + sub, ok, err := claims.StringClaim("sub") + if err != nil { + return TokenClaims{}, err + } + if !ok || sub == "" { + return TokenClaims{}, errors.New("no sub claim") + } + + noop := func() error { return nil } + + keysFunc := func() []key.PublicKey { + return keys + } + + verifier := oidc.NewJWTVerifier(issuer.String(), clientID, noop, keysFunc) + if err := verifier.Verify(jwt); err != nil { + return TokenClaims{}, err + } + + return TokenClaims{claims}, nil +} From ce8b0a4c9e2a804b8a1441d34a4c24ef6e2f6594 Mon Sep 17 00:00:00 2001 From: Joe Bowers Date: Fri, 16 Oct 2015 16:43:52 -0700 Subject: [PATCH 3/7] tests: fix user tests with for new behavior --- user/email_verification_test.go | 9 +++++---- user/password_test.go | 19 ++++++++++++++----- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/user/email_verification_test.go b/user/email_verification_test.go index 7704dc25..3cfdc97b 100644 --- a/user/email_verification_test.go +++ b/user/email_verification_test.go @@ -59,7 +59,7 @@ func TestNewEmailVerification(t *testing.T) { } ev := NewEmailVerification(tt.user, tt.clientID, tt.issuer, *cbURL, tt.expires) - if diff := pretty.Compare(tt.want, ev.claims); diff != "" { + if diff := pretty.Compare(tt.want, ev.Claims); diff != "" { t.Errorf("case %d: Compare(want, got): %v", i, diff) } @@ -127,10 +127,11 @@ func TestEmailVerificationParseAndVerify(t *testing.T) { for i, tt := range tests { - token, err := tt.ev.Token(tt.signer) + jwt, err := jose.NewSignedJWT(tt.ev.Claims, tt.signer) if err != nil { - t.Errorf("case %d: non-nil error creating token: %v", i, err) + t.Fatalf("Failed to generate JWT, error=%v", err) } + token := jwt.Encode() ev, err := ParseAndVerifyEmailVerificationToken(token, *issuer, []key.PublicKey{*key.NewPublicKey(privKey.JWK())}) @@ -148,7 +149,7 @@ func TestEmailVerificationParseAndVerify(t *testing.T) { } - if diff := pretty.Compare(tt.ev.claims, ev.claims); diff != "" { + if diff := pretty.Compare(tt.ev.Claims, ev.Claims); diff != "" { t.Errorf("case %d: Compare(want, got): %v", i, diff) } } diff --git a/user/password_test.go b/user/password_test.go index 264eafbe..37d7acd2 100644 --- a/user/password_test.go +++ b/user/password_test.go @@ -124,7 +124,7 @@ func TestNewPasswordReset(t *testing.T) { } ev := NewPasswordReset(tt.user, tt.password, tt.issuer, tt.clientID, *cbURL, tt.expires) - if diff := pretty.Compare(tt.want, ev.claims); diff != "" { + if diff := pretty.Compare(tt.want, ev.Claims); diff != "" { t.Errorf("case %d: Compare(want, got): %v", i, diff) } @@ -145,12 +145,13 @@ func TestPasswordResetParseAndVerify(t *testing.T) { password := Password("passy") goodPR := NewPasswordReset(user, password, *issuer, client, *callback, expires) - goodPRNoCB := NewPasswordReset(user, password, *issuer, "", url.URL{}, expires) + goodPRNoCB := NewPasswordReset(user, password, *issuer, client, url.URL{}, expires) expiredPR := NewPasswordReset(user, password, *issuer, client, *callback, -expires) wrongIssuerPR := NewPasswordReset(user, password, *otherIssuer, client, *callback, expires) noSubPR := NewPasswordReset(User{}, password, *issuer, client, *callback, expires) noPWPR := NewPasswordReset(user, Password(""), *issuer, client, *callback, expires) noClientPR := NewPasswordReset(user, password, *issuer, "", *callback, expires) + noClientNoCBPR := NewPasswordReset(user, password, *issuer, "", url.URL{}, expires) privKey, err := key.GeneratePrivateKey() if err != nil { @@ -211,14 +212,22 @@ func TestPasswordResetParseAndVerify(t *testing.T) { signer: signer, wantErr: true, }, + { + ev: noClientNoCBPR, + signer: signer, + wantErr: true, + }, } for i, tt := range tests { - token, err := tt.ev.Token(tt.signer) + t.Logf("TODO claims are %v", tt.ev.Claims) + + jwt, err := jose.NewSignedJWT(tt.ev.Claims, tt.signer) if err != nil { - t.Errorf("case %d: non-nil error creating token: %v", i, err) + t.Fatalf("Failed to generate JWT, error=%v", err) } + token := jwt.Encode() ev, err := ParseAndVerifyPasswordResetToken(token, *issuer, []key.PublicKey{*key.NewPublicKey(privKey.JWK())}) @@ -236,7 +245,7 @@ func TestPasswordResetParseAndVerify(t *testing.T) { } - if diff := pretty.Compare(tt.ev.claims, ev.claims); diff != "" { + if diff := pretty.Compare(tt.ev.Claims, ev.Claims); diff != "" { t.Errorf("case %d: Compare(want, got): %v", i, diff) } } From 1e037a9a7c058b3d3afa18b19ef25b3d8224898b Mon Sep 17 00:00:00 2001 From: Joe Bowers Date: Fri, 16 Oct 2015 16:44:09 -0700 Subject: [PATCH 4/7] tests: ensure ./user/ tests are run as part of test suite --- test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test b/test index 6d8e5b51..7116d8ef 100755 --- a/test +++ b/test @@ -14,7 +14,7 @@ COVER=${COVER:-"-cover"} source ./build -TESTABLE="connector db integration pkg/crypto pkg/flag pkg/http pkg/net pkg/time pkg/html functional/repo server session user/api" +TESTABLE="connector db integration pkg/crypto pkg/flag pkg/http pkg/net pkg/time pkg/html functional/repo server session user user/api" FORMATTABLE="$TESTABLE cmd/dexctl cmd/dex-worker cmd/dex-overlord examples/app functional pkg/log" # user has not provided PKG override From 39ee1871e473c3176a05cce137820f48c27fe336 Mon Sep 17 00:00:00 2001 From: Joe Bowers Date: Fri, 16 Oct 2015 16:49:27 -0700 Subject: [PATCH 5/7] build: fixup - comment explaining guard in front of ln --- build | 2 ++ 1 file changed, 2 insertions(+) diff --git a/build b/build index 08179882..158e2f82 100755 --- a/build +++ b/build @@ -20,6 +20,8 @@ fi rm -rf $GOPATH/src/github.com/coreos/dex mkdir -p $GOPATH/src/github.com/coreos/ + +# Only attempt to link dex into godeps if it isn't already there [ -d $GOPATH/src/github.com/coreos/dex ] || ln -s ${PWD} $GOPATH/src/github.com/coreos/dex LD_FLAGS="-X main.version=$(git rev-parse HEAD)" From 12342149d3a28ec39db36eed066857da7443194d Mon Sep 17 00:00:00 2001 From: Joe Bowers Date: Fri, 16 Oct 2015 16:54:22 -0700 Subject: [PATCH 6/7] fixup: document parseAndVerifyTokenClaims behavior --- user/user.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/user/user.go b/user/user.go index 6a56eb8c..734eaf57 100644 --- a/user/user.go +++ b/user/user.go @@ -448,6 +448,11 @@ type TokenClaims struct { Claims jose.Claims } +// Returns TokenClaims if and only if +// - the given token string is an appropriately formatted JWT +// - the JWT contains nonempty "aud" and "sub" claims +// - the JWT can be verified for the client associated with the "aud" claim +// using the given keys func parseAndVerifyTokenClaims(token string, issuer url.URL, keys []key.PublicKey) (TokenClaims, error) { jwt, err := jose.ParseJWT(token) if err != nil { From b1e4369811097ffa190db5e11739c30263e406d5 Mon Sep 17 00:00:00 2001 From: Joe Bowers Date: Fri, 16 Oct 2015 17:14:51 -0700 Subject: [PATCH 7/7] fixup: remove debug logging from test --- user/password_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/user/password_test.go b/user/password_test.go index 37d7acd2..b0f78807 100644 --- a/user/password_test.go +++ b/user/password_test.go @@ -221,8 +221,6 @@ func TestPasswordResetParseAndVerify(t *testing.T) { for i, tt := range tests { - t.Logf("TODO claims are %v", tt.ev.Claims) - jwt, err := jose.NewSignedJWT(tt.ev.Claims, tt.signer) if err != nil { t.Fatalf("Failed to generate JWT, error=%v", err)