Skip to content
This repository has been archived by the owner on Jul 31, 2024. It is now read-only.

Should SessionId Cookies be considered "Essential" #2554

Closed
cdwaddell opened this issue Aug 16, 2018 · 10 comments
Closed

Should SessionId Cookies be considered "Essential" #2554

cdwaddell opened this issue Aug 16, 2018 · 10 comments
Assignees
Milestone

Comments

@cdwaddell
Copy link

Cross linked from oidc-js issue: IdentityModel/oidc-client-js#612

It looks like Session cookies are not considered essential for GDPR compliance. It looks like webservers may not send cookies that are not flagged IsEssential.

https://docs.microsoft.com/en-us/aspnet/core/security/gdpr?view=aspnetcore-2.1#essential-cookies

Message cookies may need this treatment as well:

cdwaddell@cb667c4

@cdwaddell
Copy link
Author

cdwaddell commented Aug 16, 2018

FYI, the default value of CheckConsentNeeded is false, so this will only affect users that turn this setting on as a way to comply with GDPR.

https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.cookiepolicyoptions.checkconsentneeded?view=aspnetcore-2.1

@markphillips100
Copy link

I've recently run into an issue where IS4 v2.2 was unable to continue past the consent screen when a user consents. This was due to me applying the AspNet 2.1 cookie middleware and, as I understand it, IS4 v2.2 doesn't use CookieOptions for MessageCookie<> that supports IsEssential as that only came in aspnet v2,1.

I've removed the cookie middleware for now in anticipation of some future fix/workaround.

I'm not an expert on GDPR at all so correct me if I'm wrong. Essential cookies are those considered essential for the normal function of the application. With this in mind, in the case of Identity Server I would have thought a session cookie, and temp (message) cookies for such functions as IS4's consent handling, are essential to IS4's operation.

Now, assuming this is correct, can I expect those cookies to be marked IsEssential=true by default in the 2.3 release, or will there at least be options for configuring as such?

@brockallen
Copy link
Member

Yes, we will be reviewing them and making the necessary changes for the next release.

@markphillips100
Copy link

markphillips100 commented Sep 23, 2018 via email

@brockallen brockallen self-assigned this Oct 3, 2018
@brockallen
Copy link
Member

Ok, so I just got some non-official guidance and some folks say that this is not set by default in the Microsoft stuff because they claim "it depends", meaning a site might work without authN. But for IdentityServer, it's sort of all about this, so of course they're considered essential.

Thoughts? I lean to enabling them, but I'm just looking for some discussion.

@markphillips100
Copy link

markphillips100 commented Oct 3, 2018 via email

@brockallen
Copy link
Member

Ok, yea, that didn't take much convincing :)

@cdwaddell -- did you also have to set that for your primary authN cookie? I would have expected that you did.

brockallen added a commit that referenced this issue Oct 3, 2018
@brockallen
Copy link
Member

Done (as well as in the ASP.NET Identity repo).

@cdwaddell
Copy link
Author

cdwaddell commented Oct 4, 2018

@brockallen , this issue didn't affect me because I don't use the CheckConsentNeeded flag. I looked into it for a user having the session issue that affected us in oidc.js. That being said, I use ASP.Net Identity and they made this change in December:

aspnet/Security#1561

aspnet/Security@f8b4f4c#diff-1c8ddf812e415522415ab85e577050d1

FYI, this should be considered a bug, the check session endpoint will fail if the developer turns on check consent and the session cookie is not considered essential.

brockallen pushed a commit that referenced this issue Oct 25, 2018
* Add built-in support for Confirmation (cnf) (#2440)

* add confirmation object to pipeline

* Add test validator

* fixing NRE

* Switching to string for cnf

* move test validator to test project

* revert client config change

* add try/catch in JSON logic

* added notes that CNF string must be a JSON object

* added test

* Move default payload creation to extension method - closes #2299

* Update README.md

* Scrub id_token_hint from authorize logs

* use constant instead of string

* add refresh_token to scrub list in token request logger

* move is4.csproj to top-level src folder, move host

* fix XML comment

* updating for july

* update ignore

* rework to use IdentityServerUser

* rework folder names

* rework using new storage abstractions

* remove cors service

* make EndSession public #2469

* add null check when unprotecting data #2504

* use GetIdentityServerBasePath instead of Request.PathBase #2446

* reorg default impls and interfaces for consistency

* nuget updates in test projects

* Documentation: Added claimsaction to map website claim (#1) (#2377)

* Make AddScriptCspHeaders and AddStyleCspHeaders public #2513

* Add more strict cache control headers when softer headers are already added by HttpContext.SignInAsync #2514

* add better/more error descriptions to authorize response validator #2218 (#2515)

* add invalid uri scheme validation (#2506)

* add invalid uri scheme validation

* move uri redirect uri prefix validation to client configuration validator

* add option to explicitly configure the cookie auth scheme for interactive users #2489 (#2516)

* Add parameters to IntrospectionRequestValidationResult - #2388 (#2512)

* Update refresh_tokens.rst (#2316)

Adapt text to indicate refresh tokens still expire according to the sliding refresh token timeline.

* "update"

* fix validation bug on config; better config logs for authN schemes

* Remove unused ctor (#2524)

* enable default client validator by default (#2525)

* Fixes 404 (#2527)

* CorsService doesn't handle null for origin #2523

* DistributedCacheStateDataFormatter should handle failed Unprotect workflows #2533

* 2.3.0-preview1

* resolve login/logout url, et al from named options (#2540)

* resolve login/logout url, et al from named options #2532

* log effective login, et al. paths

* preview1-update1

* bug in consent when user denies

* add Securing Angular Apps with OpenID and OAuth2

* Migrate tests to new IdentityModel style (1)

* Migrate tests to new IdentityModel style (2)

* Migrate tests to new IdentityModel style (3)

* Migrate tests to new IdentityModel style (4)

* remove unused handler

* Migrate tests to new IdentityModel style (5)

* Migrate tests to new IdentityModel style (6)

* Finished integration clients with new idm style

* added SO CC-BY-SA info and links

* Renamed Client -> BackChannelClient

* Update client authentication tests

* Migrated PKCE tests

* Migrated introspection tests

* Migrated revocation tests

* Found missing introspection test

* Migrated DiscoveryEndpointTests

* Merge fixes

* Matched PR to new IdentityServer project structure

* Switched to new device flow store

* Moved in-memory device flow store to singleton

* 6_aspnet_identity.rst (#2570)

Incorrectly states "which replaces the call to UseIdentity" instead of "which replaces the call to UseAuthentication".

* Added DeviceFlowCodeService to handle hashing codes and handle generation

* update preview version

* add new dotnet tool based build script

* Add alternative dotnet tool based build file for bash

* update bash

* update ignore file

* switch to new cake (#2593)

* august sponsor update

* Add strong name (#2597)

* add strong name

* update references to strongly named packages

* updated ignore

* Create jwk document when signing with JsonWebKey (#2604)

* Update introspection.rst (#2606)

Was referring to scope secrets. Reused sentence from https://github.com/IdentityServer/IdentityServer4/blob/release/docs/topics/reference_tokens.rst

* Update secrets.rst (#2611)

* add issue templates

* Update issue templates

* Update Feature_request.md

* Delete feature_request.md

* Delete bug_report.md

* Update Bug_report.md

* add NoBuild to build file

* fix build - again

* Create SECURITY.MD

* update to new build/versioning

* update bash script

* update bash script

* Switched validator to use code service instead of store

* recursion ftw

* Initial working device flow consent

* Changing cake file to skip versioning on non-Windows (#2637)

Changing cake file to skip versioning on non-Windows

* update bash script

* remove hard-coded versions

* disable source link support because of problems with msbuild task

* Update to new IdM docs

* update endpoint docs to use new IdentityModel style

* fix links

* change color coding style

* update from september

* update to IdentityModel 3.10

* add source link back

* Make some internal types public to facilitate custom service implementations (#2545)

* Make TokenCreationRequest.Validate() public so it can be invoked by custom impl of ITokenService

* Make ClientExtensions public so they can be reused by custom IClientSecretValidator impl

* move AccessTokenAudience to public constants for reuse in custom ITokenService impl

* Change: Made DefaultUserSession.AuthenticateAsync overrideable so that (#2607)

it will be easier to support user impersonation.

* Corrected value for parsed secret type (#2658)

* update csproj

* update csproj

* disable same-site for external cookie #2595

* remove redundant call  #2582

* make EndSessionRequestValidator public #2560

* set cookies to IsEssential #2554

* nuget update

* code comments

* support idp:local as idp hint #2641

* add logic to enfore client's user sso lifetime #2609

* Fixed access denied logic. Made use of new IdentityModel constants

* Reviewed TODOs

* Moved user code generator to correct folder

* Basic retry policy for response generator. Updated some comments and class name

* Added retry limit handling

* Update unpredictable test

* More IdentityModel constants

* Redacted device code from logging

* Updated IdentityServer4.Storage

* Thread safety for InMemoryDeviceFlowStore

* Ctrl+Shift+D
brockallen pushed a commit that referenced this issue Oct 30, 2018
* Add constants for device flow grant type

* add constants, endpoint options and discovery

* Add device flow authorize request validator (#2306)

* Added models and interfaces for device authorization request validator

* Added device authorization request validator

* Validator tests (#2391)

* Switched to underscores...

* Reworked Device Authoirzation Request Validator to be more inline with Authorization Request Validator

* Added tests for device authorization request validator

* Added client secret to device flow test client

* Response generator & tests (#2395)

* Added user code generation

* Added device flow options

* Initial implementation of device authorization response generator & stores

* Started response generator unit tests

* Completed device authorization response generator tests

* Store tests

* Device Flow Authorization Endpoint (#2403)

* Started device authorization endpoint

* Made scope parameter optional

* Device authorization endpoint tests. Added events. Added default interval

* Device Flow Response Validator (#2422)

* Initial device code validation

* Device code token request tests

* Initial throttling service. TODO: tests & IDistributedCache dependency

* Added dependency for IDistributedCache. Improved tests

* Throttling service tests and fixes

* Added device code grant to TokenResponseGenerator

* Initial working device flow end to end (#2449)

* Review1 changes (#2694)

* Add built-in support for Confirmation (cnf) (#2440)

* add confirmation object to pipeline

* Add test validator

* fixing NRE

* Switching to string for cnf

* move test validator to test project

* revert client config change

* add try/catch in JSON logic

* added notes that CNF string must be a JSON object

* added test

* Move default payload creation to extension method - closes #2299

* Update README.md

* Scrub id_token_hint from authorize logs

* use constant instead of string

* add refresh_token to scrub list in token request logger

* move is4.csproj to top-level src folder, move host

* fix XML comment

* updating for july

* update ignore

* rework to use IdentityServerUser

* rework folder names

* rework using new storage abstractions

* remove cors service

* make EndSession public #2469

* add null check when unprotecting data #2504

* use GetIdentityServerBasePath instead of Request.PathBase #2446

* reorg default impls and interfaces for consistency

* nuget updates in test projects

* Documentation: Added claimsaction to map website claim (#1) (#2377)

* Make AddScriptCspHeaders and AddStyleCspHeaders public #2513

* Add more strict cache control headers when softer headers are already added by HttpContext.SignInAsync #2514

* add better/more error descriptions to authorize response validator #2218 (#2515)

* add invalid uri scheme validation (#2506)

* add invalid uri scheme validation

* move uri redirect uri prefix validation to client configuration validator

* add option to explicitly configure the cookie auth scheme for interactive users #2489 (#2516)

* Add parameters to IntrospectionRequestValidationResult - #2388 (#2512)

* Update refresh_tokens.rst (#2316)

Adapt text to indicate refresh tokens still expire according to the sliding refresh token timeline.

* "update"

* fix validation bug on config; better config logs for authN schemes

* Remove unused ctor (#2524)

* enable default client validator by default (#2525)

* Fixes 404 (#2527)

* CorsService doesn't handle null for origin #2523

* DistributedCacheStateDataFormatter should handle failed Unprotect workflows #2533

* 2.3.0-preview1

* resolve login/logout url, et al from named options (#2540)

* resolve login/logout url, et al from named options #2532

* log effective login, et al. paths

* preview1-update1

* bug in consent when user denies

* add Securing Angular Apps with OpenID and OAuth2

* Migrate tests to new IdentityModel style (1)

* Migrate tests to new IdentityModel style (2)

* Migrate tests to new IdentityModel style (3)

* Migrate tests to new IdentityModel style (4)

* remove unused handler

* Migrate tests to new IdentityModel style (5)

* Migrate tests to new IdentityModel style (6)

* Finished integration clients with new idm style

* added SO CC-BY-SA info and links

* Renamed Client -> BackChannelClient

* Update client authentication tests

* Migrated PKCE tests

* Migrated introspection tests

* Migrated revocation tests

* Found missing introspection test

* Migrated DiscoveryEndpointTests

* Merge fixes

* Matched PR to new IdentityServer project structure

* Switched to new device flow store

* Moved in-memory device flow store to singleton

* 6_aspnet_identity.rst (#2570)

Incorrectly states "which replaces the call to UseIdentity" instead of "which replaces the call to UseAuthentication".

* Added DeviceFlowCodeService to handle hashing codes and handle generation

* update preview version

* add new dotnet tool based build script

* Add alternative dotnet tool based build file for bash

* update bash

* update ignore file

* switch to new cake (#2593)

* august sponsor update

* Add strong name (#2597)

* add strong name

* update references to strongly named packages

* updated ignore

* Create jwk document when signing with JsonWebKey (#2604)

* Update introspection.rst (#2606)

Was referring to scope secrets. Reused sentence from https://github.com/IdentityServer/IdentityServer4/blob/release/docs/topics/reference_tokens.rst

* Update secrets.rst (#2611)

* add issue templates

* Update issue templates

* Update Feature_request.md

* Delete feature_request.md

* Delete bug_report.md

* Update Bug_report.md

* add NoBuild to build file

* fix build - again

* Create SECURITY.MD

* update to new build/versioning

* update bash script

* update bash script

* Switched validator to use code service instead of store

* recursion ftw

* Initial working device flow consent

* Changing cake file to skip versioning on non-Windows (#2637)

Changing cake file to skip versioning on non-Windows

* update bash script

* remove hard-coded versions

* disable source link support because of problems with msbuild task

* Update to new IdM docs

* update endpoint docs to use new IdentityModel style

* fix links

* change color coding style

* update from september

* update to IdentityModel 3.10

* add source link back

* Make some internal types public to facilitate custom service implementations (#2545)

* Make TokenCreationRequest.Validate() public so it can be invoked by custom impl of ITokenService

* Make ClientExtensions public so they can be reused by custom IClientSecretValidator impl

* move AccessTokenAudience to public constants for reuse in custom ITokenService impl

* Change: Made DefaultUserSession.AuthenticateAsync overrideable so that (#2607)

it will be easier to support user impersonation.

* Corrected value for parsed secret type (#2658)

* update csproj

* update csproj

* disable same-site for external cookie #2595

* remove redundant call  #2582

* make EndSessionRequestValidator public #2560

* set cookies to IsEssential #2554

* nuget update

* code comments

* support idp:local as idp hint #2641

* add logic to enfore client's user sso lifetime #2609

* Fixed access denied logic. Made use of new IdentityModel constants

* Reviewed TODOs

* Moved user code generator to correct folder

* Basic retry policy for response generator. Updated some comments and class name

* Added retry limit handling

* Update unpredictable test

* More IdentityModel constants

* Redacted device code from logging

* Updated IdentityServer4.Storage

* Thread safety for InMemoryDeviceFlowStore

* Ctrl+Shift+D

* Merge fixes

* Cake merge fixes
@lock
Copy link

lock bot commented Jan 12, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants