feat: immutable sessions - change value using nonce #2761

kelkarajay · 2022-09-27T09:23:53Z

Changes -

Post After Login hooks execution for Refresh and AAL upgrade scenario's use nonce cookie modifier to change session cookie value.
Post After Settings hooks execution nonce cookie modifier to enable new session cookie value.

Related issue(s)

#2701

Checklist

I have read the contributing guidelines.
I have referenced an issue containing the design document if my change
introduces a new feature.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

Further Comments

Both the Login and Settings flows mentioned in the changes section, now issue a new kratos session to reflect changes in the session content

codecov · 2022-10-04T10:36:00Z

Codecov Report

Merging #2761 (745ccbb) into master (5ac7553) will increase coverage by 0.00%.
The diff coverage is 60.00%.

❗ Current head 745ccbb differs from pull request most recent head 2923497. Consider uploading reports for the commit 2923497 to get more accurate results

@@           Coverage Diff           @@
##           master    #2761   +/-   ##
=======================================
  Coverage   75.88%   75.88%           
=======================================
  Files         302      302           
  Lines       17806    17815    +9     
=======================================
+ Hits        13512    13519    +7     
- Misses       3258     3259    +1     
- Partials     1036     1037    +1

Impacted Files	Coverage Δ
selfservice/flow/settings/hook.go	`58.97% <55.55%> (-0.49%)`	⬇️
session/manager_http.go	`73.38% <100.00%> (+0.19%)`	⬆️
identity/credentials.go	`80.00% <0.00%> (+5.00%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

aeneasr · 2022-10-05T06:18:52Z

selfservice/flow/login/hook.go

-		if err := e.d.SessionPersister().UpsertSession(r.Context(), s); err != nil {
-			return errors.WithStack(err)
+		// Update session identifiers when Re-Auth or session upgrade
+		if a.Refresh || a.RequestedAAL > s.AuthenticatorAssuranceLevel { // TODO: Change to OR to allow block exec


How about a helper func a.Equals(b)

aeneasr · 2022-10-05T06:19:31Z

selfservice/flow/login/hook.go

+
+		s = ns
+
+		if err := e.d.SessionManager().IssueCookie(r.Context(), w, r, s); err != nil {


IMO this re-issuing logic should be handled in issuecookie, and not it's consumers!

aeneasr · 2022-10-05T15:12:08Z

All you need to change - IMO - is the identifier which we use to look up the session. You don't need to actually create a new session, or revoke the old one. If the value of the cookie changes, or the session token value, we're good!

aeneasr · 2022-10-05T15:12:33Z

For simplicity, I recommend first doing only the cookie stuff, as for the tokens we'll probably need a design first how refreshing would work there.

aeneasr · 2022-10-13T07:14:58Z

selfservice/flow/login/hook.go

+	var cookieModifiers []x.CookieModifier
+	// Use nonce cookie modifier when Re-Auth or session upgrade and then issue cookie.
+	// Using nonce modifies the cookie value without affecting the session attributes
+	if a.Refresh || a.RequestedAAL > s.AuthenticatorAssuranceLevel { // TODO: Change to OR to allow block exec


What about this TODO? What speaks against using a new cookie identifier? Can we just use x.CookieValuesWithNonce()?

The TODO comment wasn't cleaned up after I got it working. Considering - #2761 (comment), this if seems unnecessary now. I'll remove it.

aeneasr · 2022-10-13T07:16:08Z

selfservice/flow/settings/handler_test.go

+			t.Run("type=api", func(t *testing.T) {
+				user1 := testhelpers.NewHTTPClientWithArbitrarySessionToken(t, reg)
+				_, body := initFlow(t, user1, true)
+				var f kratos.SelfServiceSettingsFlow
+				require.NoError(t, json.Unmarshal(body, &f))
+
+				actual, res := testhelpers.SettingsMakeRequest(t, true, false, &f, user1, `{"method":"profile", "numby": 15}`)
+				assert.Equal(t, http.StatusOK, res.StatusCode)
+				assert.Equal(t, "Your changes have been saved!", gjson.Get(actual, "ui.messages.0.text").String(), actual)
+			})


This test isn't really doing anything new, maybe remove it?

replaced with a test for spa

aeneasr · 2022-10-13T07:21:22Z

session/manager_http.go


+	for _, mod := range cookieModifiers {
+		mod(cookie)
+	}


Remove this and just set the nonce at all times. We want immutable cookies always, not just sometimes. And it makes testing easier :)

aeneasr · 2022-10-13T07:22:22Z

session/manager_http.go

@@ -49,12 +49,12 @@ func NewManagerHTTP(r managerHTTPDependencies) *ManagerHTTP {
 	}
 }

-func (s *ManagerHTTP) UpsertAndIssueCookie(ctx context.Context, w http.ResponseWriter, r *http.Request, ss *Session) error {
+func (s *ManagerHTTP) UpsertAndIssueCookie(ctx context.Context, w http.ResponseWriter, r *http.Request, ss *Session, cookieModifiers ...x.CookieModifier) error {


Because the nonce is optional (passed as an argument), there are a couple of places where UpsertAndIssueCookie / IssueCookie are being called. There, the cookie would probably not be changed because the nonce is missing. Therefore, remove this parameter and just always set the nonce!

Dropped parameter

aeneasr · 2022-10-13T07:23:13Z

session/manager.go

+	UpsertAndIssueCookie(context.Context, http.ResponseWriter, *http.Request, *Session, ...x.CookieModifier) error

 	// IssueCookie issues a cookie for the given session.
 	//
 	// Also regenerates CSRF tokens due to assumed principal change.
-	IssueCookie(context.Context, http.ResponseWriter, *http.Request, *Session) error
+	IssueCookie(context.Context, http.ResponseWriter, *http.Request, *Session, ...x.CookieModifier) error


Please check all places where these two functions are being used and interpret whether they issue a new cookie with a new nonce or not.

3 usages of Upsert and Issue -> all perform prior activation of session (followed by addition of nonce) which results in a new kratos session (Login, Code and Link recovery).
3 usages of Issue -> nonce addition to the values map results in a new kratos session (PostSettingsHook, Post Registration, Refresh cookie)

session/manager_http.go

…ntifiers change

kelkarajay · 2022-10-13T13:10:36Z

Quite a bit of timeouts in the E2E stages -

making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434

This is unrelated to the change here. I couldn't yet find the cause of this. @aeneasr

session/manager_http_test.go

aeneasr · 2022-10-13T13:30:47Z

Quite a bit of timeouts in the E2E stages -

making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434
making HTTP(S) get request to  url:http://localhost:4434/health/ready ...
  HTTP(S) error for http://localhost:4434/health/ready Error: connect ECONNREFUSED 127.0.0.1:4434

This is unrelated to the change here. I couldn't yet find the cause of this. @aeneasr

It's not very likely that the timeouts are unrelated. Those typically happen if the server is unable to start and points to compile, run, or config issues! It could be related to the build failing because of a timeout but I did not check the logs. Let's see :)

Closes ory#2701

kelkarajay force-pushed the feat/Immutable-session-identifiers branch from 457c467 to ee57c74 Compare September 27, 2022 09:24

kelkarajay added the breaking change Changes behavior in a breaking manner. label Sep 27, 2022

kelkarajay force-pushed the feat/Immutable-session-identifiers branch 2 times, most recently from e8fd850 to 6a5cf7b Compare October 4, 2022 08:35

kelkarajay force-pushed the feat/Immutable-session-identifiers branch 2 times, most recently from 06001d0 to c954017 Compare October 4, 2022 16:28

aeneasr reviewed Oct 5, 2022

View reviewed changes

kelkarajay force-pushed the feat/Immutable-session-identifiers branch from c954017 to 083d009 Compare October 5, 2022 12:44

aeneasr mentioned this pull request Oct 5, 2022

Session identifiers should be immutable #2701

Closed

6 tasks

kelkarajay force-pushed the feat/Immutable-session-identifiers branch 2 times, most recently from a9867df to 4240e69 Compare October 11, 2022 10:53

kelkarajay changed the title ~~feat: Immutable sessions - reissue on re-auth, aal upgrade~~ feat: Immutable sessions - change value using nonce Oct 11, 2022

kelkarajay changed the title ~~feat: Immutable sessions - change value using nonce~~ feat: immutable sessions - change value using nonce Oct 11, 2022

kelkarajay force-pushed the feat/Immutable-session-identifiers branch 2 times, most recently from d8bf375 to 5590341 Compare October 12, 2022 11:42

kelkarajay requested a review from aeneasr October 12, 2022 15:59

kelkarajay marked this pull request as ready for review October 12, 2022 16:29

kelkarajay requested a review from zepatrik as a code owner October 12, 2022 16:29

kelkarajay linked an issue Oct 12, 2022 that may be closed by this pull request

Session identifiers should be immutable #2701

Closed

6 tasks

aeneasr removed the breaking change Changes behavior in a breaking manner. label Oct 13, 2022

aeneasr requested changes Oct 13, 2022

View reviewed changes

kelkarajay force-pushed the feat/Immutable-session-identifiers branch 2 times, most recently from c2aac54 to 05ccad4 Compare October 13, 2022 08:39

aeneasr reviewed Oct 13, 2022

View reviewed changes

session/manager_http.go Show resolved Hide resolved

kelkarajay force-pushed the feat/Immutable-session-identifiers branch from eea3255 to 16f2229 Compare October 13, 2022 10:39

test: add feature test expectation

e20181c

Ajay Kelkar added 19 commits October 13, 2022 14:38

feat: new identifiers when reauth or aal upgrade

4538361

feat: session replacement for api flow

786218e

feat: session replacement for browser flow

5af9999

fix: test asserts new session on refresh

3570a2f

feat: undo reissue new session; change cookie value

e025ba0

feat: issue session cookie after settings post hooks complete

6326563

feat: add cookie options to insert nonce k/v

7f56984

feat: cookie options param for upsert/issue cookie

b41704a

refactor: rename modifier

090ccd8

docs: fix comment

faba48b

refactor: use modifier slice

a5323e5

docs: add comment

4d55082

test: adding login flow - refresh test to validate kratos session ide…

d43e07b

…ntifiers change

test: add whoami check to assert session equality

885d6a1

refactor: json responses with updated session cookie

734c3ea

test: add tests for the settings submit flow handler

cf26397

fix: always set the nonce in cookie values

2b0273b

test: replace api test with one for spa

725594e

test: cookie identifier

f6bc54a

kelkarajay force-pushed the feat/Immutable-session-identifiers branch from 16f2229 to f6bc54a Compare October 13, 2022 12:38

kelkarajay requested a review from aeneasr October 13, 2022 13:10

aeneasr reviewed Oct 13, 2022

View reviewed changes

session/manager_http_test.go Outdated Show resolved Hide resolved

aeneasr previously approved these changes Oct 13, 2022

View reviewed changes

fix: update test assertion

2923497

aeneasr dismissed their stale review via 2923497 October 13, 2022 13:19

aeneasr merged commit a6f2793 into master Oct 13, 2022

aeneasr deleted the feat/Immutable-session-identifiers branch October 13, 2022 15:11

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023

feat: immutable cookie session values (ory#2761)

c601009

Closes ory#2701

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: immutable sessions - change value using nonce #2761

feat: immutable sessions - change value using nonce #2761

kelkarajay commented Sep 27, 2022 •

edited

Loading

codecov bot commented Oct 4, 2022 •

edited

Loading

aeneasr Oct 5, 2022

aeneasr Oct 5, 2022

aeneasr commented Oct 5, 2022

aeneasr commented Oct 5, 2022

aeneasr Oct 13, 2022

kelkarajay Oct 13, 2022

aeneasr Oct 13, 2022

kelkarajay Oct 13, 2022

aeneasr Oct 13, 2022

kelkarajay Oct 13, 2022

aeneasr Oct 13, 2022

kelkarajay Oct 13, 2022

aeneasr Oct 13, 2022

kelkarajay Oct 13, 2022

kelkarajay commented Oct 13, 2022

aeneasr commented Oct 13, 2022


		s = ns

		if err := e.d.SessionManager().IssueCookie(r.Context(), w, r, s); err != nil {

feat: immutable sessions - change value using nonce #2761

feat: immutable sessions - change value using nonce #2761

Conversation

kelkarajay commented Sep 27, 2022 • edited Loading

Changes -

Related issue(s)

Checklist

Further Comments

codecov bot commented Oct 4, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Oct 5, 2022

aeneasr commented Oct 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kelkarajay commented Oct 13, 2022

aeneasr commented Oct 13, 2022

kelkarajay commented Sep 27, 2022 •

edited

Loading

codecov bot commented Oct 4, 2022 •

edited

Loading