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.

Sign up for GitHub

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

Jump to bottom

enhancement: Make IDP cookies same site strict #8716

Merged
merged 1 commit into from
Mar 25, 2024
Merged

enhancement: Make IDP cookies same site strict #8716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions changelog/unreleased/enhancement-strict-same-site-cookie.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
Enhancement: Make IDP cookies same site strict

To enhance the security of our application and prevent Cross-Site Request Forgery (CSRF) attacks, we have updated the
SameSite attribute of the build in Identity Provider (IDP) cookies to Strict.

This change restricts the browser from sending these cookies with any cross-site requests,
thereby limiting the exposure of the user's session to potential threats.

This update does not impact the existing functionality of the application but provides an additional layer of security
where needed.

https://github.com/owncloud/ocis/pull/8716
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ require (
github.com/justinas/alice v1.2.0
github.com/leonelquinteros/gotext v1.5.3-0.20230317130943-71a59c05b2c1
github.com/libregraph/idm v0.4.1-0.20231213140724-56a222fb4215
github.com/libregraph/lico v0.61.2
github.com/libregraph/lico v0.61.3-0.20240322112242-72cf9221d3a7
github.com/mitchellh/mapstructure v1.5.0
github.com/mna/pigeon v1.2.1
github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1618,8 +1618,8 @@ github.com/leonelquinteros/gotext v1.5.3-0.20230317130943-71a59c05b2c1 h1:k56sFO
github.com/leonelquinteros/gotext v1.5.3-0.20230317130943-71a59c05b2c1/go.mod h1:AT4NpQrOmyj1L/+hLja6aR0lk81yYYL4ePnj2kp7d6M=
github.com/libregraph/idm v0.4.1-0.20231213140724-56a222fb4215 h1:Yw/I6l/0S/zDq2Hnibvwy8cVLpMaBwDe0aUSv/FNU6U=
github.com/libregraph/idm v0.4.1-0.20231213140724-56a222fb4215/go.mod h1:h/B7mB5OqrsrobydErMGewHxonYDKjGOaJsFabXyRo8=
github.com/libregraph/lico v0.61.2 h1:sU8eQ2E9Uq5wnTkD33YX5+gRj59MkPLgDVoB72L1q8w=
github.com/libregraph/lico v0.61.2/go.mod h1:TgZGBAYzVRQSRdBC8PgGQKjYhtXuTr6UCM3ZZyGTleQ=
github.com/libregraph/lico v0.61.3-0.20240322112242-72cf9221d3a7 h1:fcPgiBu7DGyGeokE0Qk+S+GW/3n+QWu1dIjw0TqadhI=
github.com/libregraph/lico v0.61.3-0.20240322112242-72cf9221d3a7/go.mod h1:TgZGBAYzVRQSRdBC8PgGQKjYhtXuTr6UCM3ZZyGTleQ=
github.com/libregraph/oidc-go v1.0.0 h1:l2tE/EwLyLXVy0B5BuVKgIFX9pNpz/5J3x5IBw0KEhc=
github.com/libregraph/oidc-go v1.0.0/go.mod h1:7TRHrY/H1Vg6JqFjV0oAe1+kN+mGFBqXYvclSyvhRyc=
github.com/linode/linodego v0.25.3/go.mod h1:GSBKPpjoQfxEfryoCRcgkuUOCuVtGHWhzI8OMdycNTE=
Expand Down
19 changes: 13 additions & 6 deletions services/idp/pkg/backends/cs3/bootstrap/cs3.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/libregraph/lico/identifier"
"github.com/libregraph/lico/identity"
"github.com/libregraph/lico/identity/managers"

cs3 "github.com/owncloud/ocis/v2/services/idp/pkg/backends/cs3/identifier"
)

Expand Down Expand Up @@ -88,12 +89,18 @@ func NewIdentityManager(bs bootstrap.Bootstrap) (identity.Manager, error) {
activeIdentifier, err := identifier.NewIdentifier(&identifier.Config{
Config: config.Config,

BaseURI: config.IssuerIdentifierURI,
PathPrefix: bs.MakeURIPath(bootstrap.APITypeSignin, ""),
StaticFolder: config.IdentifierClientPath,
LogonCookieName: "__Secure-KKT", // Kopano-Konnect-Token
ScopesConf: config.IdentifierScopesConf,
WebAppDisabled: config.IdentifierClientDisabled,
BaseURI: config.IssuerIdentifierURI,
PathPrefix: bs.MakeURIPath(bootstrap.APITypeSignin, ""),
StaticFolder: config.IdentifierClientPath,
ScopesConf: config.IdentifierScopesConf,
WebAppDisabled: config.IdentifierClientDisabled,

LogonCookieName: "__Secure-KKT", // Kopano-Konnect-Token
LogonCookieSameSite: config.CookieSameSite,

ConsentCookieSameSite: config.CookieSameSite,

StateCookieSameSite: config.CookieSameSite,

AuthorizationEndpointURI: fullAuthorizationEndpointURL,
SignedOutEndpointURI: fullSignedOutEndpointURL,
Expand Down
2 changes: 2 additions & 0 deletions services/idp/pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package config

import (
"context"
"net/http"

"github.com/owncloud/ocis/v2/ocis-pkg/shared"
)
Expand Down Expand Up @@ -112,6 +113,7 @@ type Settings struct {

CookieBackendURI string
CookieNames []string
CookieSameSite http.SameSite
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should have an env-var & yaml annotation to configure that.

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 no, since the idp is meant for internal use only... am i right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but on the same site.... im not against it... 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you decide to have it, can u take over and add it? i expect load testing will consume most of my day

Copy link
Member

Choose a reason for hiding this comment

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

wait yes you are right... this does not affect external idp configurations afaik....

Copy link
Member

Choose a reason for hiding this comment

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

unless we provide the ability to authenticate extenal apps via the internal idp this is not needed....


AccessTokenDurationSeconds uint64 `yaml:"access_token_duration_seconds" env:"IDP_ACCESS_TOKEN_EXPIRATION" desc:"'Access token lifespan in seconds (time before an access token is expired).'" introductionVersion:"pre5.0"`
IDTokenDurationSeconds uint64 `yaml:"id_token_duration_seconds" env:"IDP_ID_TOKEN_EXPIRATION" desc:"ID token lifespan in seconds (time before an ID token is expired)." introductionVersion:"pre5.0"`
Expand Down
2 changes: 2 additions & 0 deletions services/idp/pkg/config/defaults/defaultconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package defaults

import (
"net/http"
"path/filepath"
"strings"

Expand Down Expand Up @@ -64,6 +65,7 @@ func DefaultConfig() *config.Config {
ValidationKeysPath: "",
CookieBackendURI: "",
CookieNames: nil,
CookieSameSite: http.SameSiteStrictMode,
AccessTokenDurationSeconds: 60 * 5, // 5 minutes
IDTokenDurationSeconds: 60 * 5, // 5 minutes
RefreshTokenDurationSeconds: 60 * 60 * 24 * 30, // 30 days
Expand Down
18 changes: 12 additions & 6 deletions vendor/github.com/libregraph/lico/bootstrap/backends/ldap/ldap.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 12 additions & 6 deletions vendor/github.com/libregraph/lico/bootstrap/backends/libregraph/libregraph.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 14 additions & 4 deletions vendor/github.com/libregraph/lico/bootstrap/bootstrap.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions vendor/github.com/libregraph/lico/bootstrap/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions vendor/github.com/libregraph/lico/bootstrap/settings.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 10 additions & 3 deletions vendor/github.com/libregraph/lico/identifier/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions vendor/github.com/libregraph/lico/identifier/cookie.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 18 additions & 5 deletions vendor/github.com/libregraph/lico/identifier/identifier.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 7 additions & 4 deletions vendor/github.com/libregraph/lico/oidc/provider/config.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading