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.

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

A GOV.UK-wide session cookie & login #134

Merged
merged 1 commit into from
Feb 10, 2021
Merged

Conversation

barrucadu
Copy link
Contributor

@barrucadu barrucadu commented Jan 29, 2021

Rendered

Deadline: 2021-02-09.


We propose introducing a new cookie, __Host-govuk_account_session,
which will be an essential cookie, but only set when a user signs in
to use personalised parts of GOV.UK (currently just the Transition
Checker).

Similarly to how our A/B tests work we will manage this cookie at the
Fastly layer, in Varnish Configuration Language (VCL), and use custom
request headers to pass the cookie value to our apps. We will also use
custom response headers to set a new cookie value.

We will create a new app to provide the internal account-supporting
API, and extend frontend to handle the login and logout process.

@richardTowers
Copy link
Contributor

Very nice! I'll try to make time to give this a bit more thought next week, but generally I think the approach looks good.

I'm pretty sure your reasoning that this needs to happen in the CDN layer for caching reasons is sound.

Am I right in thinking that what you're referring to as session_id is actually not an ID, but some serialized data?

If it's a session ID that needs to be looked up in a DB on every request, I wonder "why bother with putting the expiry in the cookie and signing it, when you could just store the expiry in the DB (since you're going to have to look it up anyway)?". So I guess it's not an ID?

@barrucadu
Copy link
Contributor Author

If it's a session ID that needs to be looked up in a DB on every request, I wonder "why bother with putting the expiry in the cookie and signing it, when you could just store the expiry in the DB (since you're going to have to look it up anyway)?". So I guess it's not an ID?

I was actually thinking of it being an ID. Storing the expiration time in the database has the same problem as this:

  1. The user logs in to use some personalised part of GOV.UK.

  2. The user then spends 30 minutes viewing non-personalised parts of
    GOV.UK, but without leaving the site.

  3. The user then tries to use another personalised part of GOV.UK, but
    their session has expired, because the non-personalised parts
    weren't bumping the expiration time on every page view.

In this case the parts of GOV.UK visited during (2) would be parts which don't hit the database, and so the expiration time doesn't get updated. If only a handful of pages on GOV.UK (to start with) use personalisation, then it's pretty likely for this to happen.

We could work around that by setting a long expiration time in the database, though then if an attacker manages to get a user's cookie they'll be able to use it for longer.

We probably will want some database-level expiration, to avoid an ever-growing collection of old IDs and OAuth tokens, but I think we still need the tamper-proof cookie expiration, so that these cookies expire in a reasonable time.


If in the future we have some sort of personalisation on a majority of pages (eg, showing a sign in / sign out header on every page), then we'll be able to do the expiration purely based on database times. But we're not there yet.

@richardTowers
Copy link
Contributor

Ah yeah, that makes sense. If we want to handle the situation where a user noodles around on entirely cached bits of GOV.UK (even if they're personalised), and we don't want their session to time out due to an idle period, it has to be fastly that updates the expiry.

I guess we could use Fastly's private edge dictionary to store the expiry in a database that can be written at the edge. But then that's even more complex.

Makes sense to me! Thanks for writing this up. I'll have a proper look next week :)

@vmogoreanu
Copy link

CDN approach is probably the only way I can think of to do this

I am very happy we are passing _ga parameter :-). there are 2 _ga cookies currently -we should probably fix this so that there is less confusion about which one to pass (I don't think we need to pass both)

A few questions:

  • Why 30 minutes for session expiration? I feel we could have longer sessions on GOV.UK - needs to be reviewed with product people and cyber
  • POST /api/attributes to update the attributes should support partial update - not all the attributes.
  • POST /api/attributes do we need to worry about conflicting updates from multiple tabs/devices sharing the same session?
  • It would be good to have an interface to analyse all user data (not per user), that may be out of scope of this RFC though

I'll have another look next week

@barrucadu
Copy link
Contributor Author

Why 30 minutes for session expiration?

It's what we currently have for the account manager app. I'll add a note to the text that 30m is just an example and we'll figure out an appropriate session duration.

POST /api/attributes to update the attributes should support partial update - not all the attributes.

Yes, this'll be a partial update. I'll clarify that.

POST /api/attributes do we need to worry about conflicting updates from multiple tabs/devices sharing the same session?

I think we can probably leave this until we have more attributes. Erin is thinking about how a GOV.UK attribute-store-for-personalisation will work so I don't want to delve into that too deeply with this RFC.

It would be good to have an interface to analyse all user data (not per user), that may be out of scope of this RFC though

Yes, I think that'll tie into Erin's work.

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

This sounds great. A really well considered RFC and sounds like a good improvement on the transition checker flow.

I ended up with more questions than I was expecting. I hope they are helpful.


We propose introducing a new cookie, `govuk_session`, which will be an
optional cookie, required only for users who wish to use personalised
parts of GOV.UK (currently just the Transition Checker).
Copy link
Member

Choose a reason for hiding this comment

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

As I understand this cookie is just for users with accounts and not for general purpose session data, is that the fixed scope? If so, I wonder if this should have a name that specifies that this is just for accounts as we are likely to continue having user sessions for unauthenticated users and this naming may confuse that.

Copy link
Member

Choose a reason for hiding this comment

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

If we are likely to have sessions for unauthenticated users, might it be better to have just a single session type, which may or may not be attached to an account?

To support that use-case, if `Rails.env.development?`, the new app
will set an unencrypted `govuk_session` cookie, on the domain
`dev.gov.uk`, which contains the `GOVUK-Session-ID`, in addition to
sending the response headers.
Copy link
Member

Choose a reason for hiding this comment

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

I guess for this we'd need to code apps to understand cookies as well as headers as they'd not have access to the headers unless additional proxy infrastructure was involved. Would this be middleware?

Copy link
Contributor Author

@barrucadu barrucadu Feb 1, 2021

Choose a reason for hiding this comment

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

Yeah, this sounds like a good candidate for some middleware or a gem. I think to start with we'll just end up putting some code straight in the app, and when a couple of apps are on board we'll know what's most useful to pull out as common code.

@maxgds
Copy link
Contributor

maxgds commented Feb 1, 2021

Apologies if I overlooked it, but I'm not clear on how this fits into the overall GOV.UK cookies strategy as described here: https://www.gov.uk/help/cookies - you say this would be an optional cookie and it looks to me like it would best fit under the "remember your settings" header, in which case what would the outcome be if the user has not consented to that kind of cookie? Would part of the sign up process maybe insist on them accepting such cookies? In which case what happens to people who signed up already? And does accepting this cookie mean they accept all others under "remember your settings"? Or would you create a new "accounts" category for this particular cookie/set of cookies?

@barrucadu
Copy link
Contributor Author

you say this would be an optional cookie

I'm not totally clear myself on how to classify this cookie. It's optional in that you can use www.gov.uk without it (by not using any of the accounts stuff). But it's required if you do want to use the accounts stuff. I don't think bundling use for accounts in with consent for other cookies is a good idea, as that could be seen as nudging people.

@maxgds
Copy link
Contributor

maxgds commented Feb 1, 2021

It's optional in that you can use www.gov.uk without it

I think there's a strong argument that it's essential. If you drill down into what our existing cookies are here https://www.gov.uk/help/cookie-details it seems to match the use case of licensing_session - we don't claim that license cookies are optional as you could potentially apply on paper instead.

@barrucadu
Copy link
Contributor Author

I think there's a strong argument that it's essential.

Great, and in fact I see that our current account cookies are listed there as essential, even though they're optional in exactly the same way, I should have checked that first 🤦‍♂️

I'll update the RFC.

@OllieJC
Copy link
Member

OllieJC commented Feb 2, 2021

I may have misunderstood some of the reasoning here, but I think there's perhaps a lower complexity way of doing this.
I have reservations about the use of an expiry on the client side, use of symmetric keys for hashing and the use of the CDN to set headers, some of which has already been mentioned in the other comments.

My thoughts in a sort of generic user journey flow:

  • Have an endpoint www.gov.uk/sign-in that is the only place where a cookie is created for the www.gov.uk domain
  • (consider using the __Host- prefix to lock the cookie to the domain (ref))
  • After successful sign in, the /sign-in endpoint generates a session ID, where that ID is checked to see if already in use (to prevent randomly generating the same ID as another session) - or use a GUID that has a time seed component, then encrypt the ID with a private key (only available to that app, not shared with any other app), session ID is set in the database against a user account along with an expiry time (I think this can be a reasonable length - such as an hour)
  • Cookie is session-based, and doesn't have an expiry client-side (that's handled in the DB), the value in the cookie cannot be tampered with as it'll be encrypted
  • On an origin of another endpoint, fetch the cookie and decrypt with a public key from another endpoint (like www.gov.uk/.well-known/session-key.pub) the key can be truly public as it's checking if the paired private key encrypted it, it doesn't matter if someone can decrypt the session to get the session ID as they won't be able to re-encrypt it - this helps get around needing to share symmetric keys for hashing with other origins
  • (there could be multiple public/private key pairs and a reference used in the cookie, the value could be 1:base64 where 1 is the public key reference, base64 is of the encrypted session ID, this helps with any future rotations)
  • The decrypted session ID shouldn't be considered secret or sensitive on its own
  • Other origin apps call an internal (authed) API (/api/attributes) with the session ID (or just send the entire encrypted cookie) the API checks the expiry from the database and returns attributes for that app, the expiry time is updated in the DB to extend/roll the session
  • (should every personalisation origin request query the API to validate the session ID?)
  • Any app can set the session cookie to expired (e.g.: Set-Cookie: __Host-govuk_account_session=; secure; httponly; samesite=strict; max-age=0; path=/) if the API returns an expired/invalid state and then either act as logged out in the response, or redirect to a signed-out endpoint

@barrucadu
Copy link
Contributor Author

barrucadu commented Feb 3, 2021

A session cookie encrypted on the backend and not touched by Fastly would make this simpler. The only thing I'm not sure about is the expiration time,

session ID is set in the database against a user account along with an expiry time (I think this can be a reasonable length - such as an hour)

As I think we'd want this to work:

  • Someone logs into GOV.UK to use one personalised thing
  • They then continuously browse non-personalised parts of GOV.UK (so the session ID isn't used, and the expiration time isn't bumped up)
  • They then try to use another personalised thing (and we'd ideally like them to still be logged in at this point)

I'll check with Daisy what our typical session lengths are like.

@barrucadu
Copy link
Contributor Author

So, we have an average session duration of 2:29, but the PAs don't really like session duration as a metric because it's skewed by people who just leave the page open. So we could probably use 1 hour.

@barrucadu
Copy link
Contributor Author

Since we have config in place to strip out Set-Cookie headers from our apps, I'll keep the custom headers for starting / ending the session in place. But all of the crypto stuff can be removed from VCL.

header:

```vcl
set req.http.GOVUK-Account-Session = req.http.Cookie:__Host-govuk_account_session;
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the cookie isn't present? We should unset the GOVUK-Account-Session header so a malicious actor can't set that in a custom header, only the expected cookie should set it.

Suggested change
set req.http.GOVUK-Account-Session = req.http.Cookie:__Host-govuk_account_session;
unset req.http.GOVUK-Account-Session;
set req.http.GOVUK-Account-Session = req.http.Cookie:__Host-govuk_account_session;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that if the cookie isn't present the current code is equivalent to set req.http.GOVUK-Account-Session = "", but we can check that


[tc-cookie-pr]: https://github.com/alphagov/govuk-puppet/pull/10788

### Apps need to share the same session cookie
Copy link

@galund galund Feb 4, 2021

Choose a reason for hiding this comment

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

This isn't true in the general case, which is that services will have their own cookies, and will redirect users in an OIDC flow to the central authn service if the user isn't logged in, but needs to be. There will need to be some common standards to make that work - including a way for users to log themselves out, and some agreement about cookie lifetimes.

Not that GOV.UK has to have each of its apps implement authentication against the central service separately: where you draw the lines between your services is up to you. But the architecture must allow for operationally independent services operated by different departments in any event, and the more GOV.UK can itself behave like other services will do, the more good patterns can be embedded from the start, and the more the OIDC flows will be properly dog-fooded. And the fewer surprises there will be when we work with other government departments to make their services feel properly integrated with the GOV.UK Account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this RFC is specifically about www.gov.uk, not everything under the gov.uk domain.

Copy link

Choose a reason for hiding this comment

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

I get that, I still think that treating different services that the GOV.UK team happen to be running more separately might have some merit, and if you're excluding in general then it's worth stating why. Seeing as there are some benefits which I mentioned :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that makes sense. I think the main problems with apps on the www.gov.uk domain authenticating separately are:

  • It's probably confusing to users if they move between different parts of GOV.UK and their logged-in state changes. We've already found people being confused at the difference between being logged in to the Transition Checker on www.gov.uk and the OIDC thing on www.account.publishing.service.gov.uk, and they're not even on the same domain.
  • It adds friction to personalisation, where we'd have to get users to re-authenticate to see personalised content served by different apps.
  • It makes it harder to show some global logged-in state, eg a "logout" link in the header, because the user is individually logged into separate parts of GOV.UK.

But for services not on www.gov.uk, eg if there's some new big thing and the GOV.UK team spins up a prepare-for-meteor-impact.service.gov.uk, that should authenticate separately, because it's not on the same domain.

My thinking is that the microservices which make up GOV.UK (or prepare-for-meteor-impact.service.gov.uk, or whatever) are an implementation detail which shouldn't be exposed to the user, so you'd ideally want all the microservices that make up one service to share their authenticated-ness.

Copy link

Choose a reason for hiding this comment

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

I think it's confusing for users if logged-in state isn't maintained between all services. This will have to be tackled at some point (although not necessarily for this RFC of course!).

The whole point of gov.uk is that users see a single thing and that the question of which department or team provides any service that central government is responsible for is an implementation detail.

Copy link
Member

@erino erino Feb 4, 2021

Choose a reason for hiding this comment

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

treating different services that the GOV.UK team happen to be running more separately might have some merit

I was thinking that this approach isn't practical as there are 60+ services which make up GOV.UK Publishing, will the GOV.UK Account want to support that many connections for one client. Then scaling that to each of the micro services which make up the 300+ services across Government.

There's also an OpenID Connect spec for this Open ID Connect Federation, which I need to read and see how compatible it is with this RFC.

I would like to think that the GOV.UK Account will consider GOV.UK Publishing as a single service and allow it to manage users sessions in a way is appropriate for it's architecture. An implementation detail as @barrucadu says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, if we do end up wanting to split GOV.UK Publishing into separate clients, that can always happen later.

@barrucadu
Copy link
Contributor Author

It would be really great for the discussion to wrap up tomorrow, so that we can begin implementing things. Even if what we implement isn't perfect and we need to change it later, we'll need something in place to do our personalisation experiments.

I think the major point of discussion remaining is: what exactly does the session cookie hold? Some sort of randomly-generated (collision resistant) session ID yes, but is that plaintext? or signed? or encrypted?

Are all the other major parts generally considered ok?

  • We'll introduce a new session cookie for GOV.UK.
  • We won't be opening our nginx config to let our frontend apps use cookies, instead the value will be passed around with custom headers and some VCL magic.
  • We'll introduce a new API app to do the OAuth and attribute-related interactions.
  • We'll introduce a new machine class for that app.
  • We'll add some new public endpoints in frontend to do the OAuth redirection flow.
  • We'll switch over the Transition Checker to use this new auth mechanism and remove the existing session management code from finder-frontend
  • We'll make this work in local development by setting the session cookie directly on the dev.gov.uk domain.

@richardTowers
Copy link
Contributor

Yep, I'm happy with all of those points - happy to approve once we've worked out what we put in the session cookie.

I think the options on the table are:

  • Random session ID, plaintext
  • Random session ID + HMAC (symmetric)
  • Random session ID + asymmetric signature (e.g. using ECDSA or RSA)
  • Asymmetrically encrypted random session ID (e.g. using ECIES or RSA+OAEP)

I'm happy with any of those, so long as there's a rough consensus. They're all based on a strong random ID, so they should all be sufficiently secure.

My preference would be for Random session ID + HMAC (symmetric), as I feel that provides the best balance of security strength in depth and simplicity.

If we go for an asymmetric signature or encryption scheme, I'd like us to be specific about which algorithm we're planning on using, which libraries we're going to use to do the crypto, and what effect that will have on the size of the cookie. The devil's in the detail here, and I'd hate for us to make a decision that ends up costing us a lot of implementation time for little security benefit.

@kevindew
Copy link
Member

kevindew commented Feb 4, 2021

Thanks Michael for the various amends this is all looking good 👍

With the cookie concern have we identified what scenario(s) we're worried about? If we're expecting that the only way someone will get the session id is from the cookie leaking then I'm not sure how any of the additional protection measures help as someone could use a leaked encrypted cookie or a leaked signed cookie - so do they offer an advantage over plaintext? or is there another way we're concerned that the session id could leak (an internal bad actor seems the most likely one but then that same actor would probably have access to secrets/keys)? or is just preventing guesswork the main concern?

@barrucadu
Copy link
Contributor Author

A few of us are having a call on Tuesday morning to talk through the remaining issues around the exact auth flow used and what the cookie holds. I've bumped the deadline for discussion to 2021-02-09.


### We need to pass cookies to our apps

Our Nginx configuration blocks most cookies, [which we disabled for
Copy link
Contributor Author

@barrucadu barrucadu Feb 9, 2021

Choose a reason for hiding this comment

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

@richardTowers Out of interest, will we have this sort of thing (a proxy removing undesirable headers, both inbound and outbound) in the replatformed world?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've jotted it down on my board of things the current platform does, but it's not in the plan at the moment. I'm interested in how valuable we think the feature 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.

I suspect the main value we get out of it is not needing to confirm whether any of our apps try to set session cookies by default when they shouldn't do.

Copy link
Contributor

@bevanloon bevanloon left a comment

Choose a reason for hiding this comment

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

Thanks for all the work on this Michael - it looks good to me.

Worth a second ✅ from @erino and/or @richardTowers.

Copy link
Contributor

@richardTowers richardTowers left a comment

Choose a reason for hiding this comment

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

Thanks for all your work on this @barrucadu!

I'm happy 🚀

@barrucadu barrucadu merged commit e1c2e7c into master Feb 10, 2021
@barrucadu barrucadu deleted the msw/account-session branch February 10, 2021 16:32
Copy link
Member

@erino erino left a comment

Choose a reason for hiding this comment

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

👍🏻 from me. Good work @barrucadu, looking forwarding to signing in across GOV.UK

@suganya-s
Copy link

Hey @barrucadu , I'm really late to this party but just wanted to check if this ID would be passed to Google Analytics?

@barrucadu
Copy link
Contributor Author

@suganya-s No, we've ended up deciding the cookie is going to store session data, not an ID.

@suganya-s
Copy link

Thanks for clarifying @barrucadu, is there any way to join that data to Google Analytics in the works?

@barrucadu
Copy link
Contributor Author

Not yet. We do have unique user IDs, so one of the ideas we talked about a while back (like triggering a GA event containing the ID when the user successfully logs in) will be doable.

huwd added a commit to alphagov/static that referenced this pull request Mar 16, 2021
A part of the work to broaden the transition checker login prototype to
the whole of GOV.UK. As outlined in [RFC#134][1] we are moving sign in
and sign out paths from being nested in finder-frontend (which would be
strange for a GOV.UK-wide login!) onto their own paths.

See the RFC for more detail on these two paths:
- [Sign in][2]
- [Sign out][3]

[1]: alphagov/govuk-rfcs#134
[2]: https://github.com/alphagov/govuk-rfcs/pull/134/files#diff-f5a6037308ea735f2a6e63b3080e78792499608bcad7386a49cf94799634d1beR224
[3]: https://github.com/alphagov/govuk-rfcs/pull/134/files#diff-f5a6037308ea735f2a6e63b3080e78792499608bcad7386a49cf94799634d1beR243
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.