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

Support setting cookie domain #6288

Merged
merged 4 commits into from Jul 12, 2019
Merged

Support setting cookie domain #6288

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
42d706d
Support setting cookie domain
tamalsaha Mar 9, 2019
6dea338
Merge branch 'master' into cookie-domain-pr
techknowlogick Jul 12, 2019
6ca12ef
Merge branch 'master' into cookie-domain-pr
lunny Jul 12, 2019
7bae38a
Merge branch 'master' into cookie-domain-pr
tamalsaha Jul 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions custom/conf/app.ini.sample
View file
Open in desktop
Expand Up @@ -321,6 +321,8 @@ IMPORT_LOCAL_PATHS = false
DISABLE_GIT_HOOKS = false
; Password Hash algorithm, either "pbkdf2", "argon2", "scrypt" or "bcrypt"
PASSWORD_HASH_ALGO = pbkdf2
; Set false to allow JavaScript to read CSRF cookie
CSRF_COOKIE_HTTP_ONLY = true

[openid]
;
Expand Down
1 change: 1 addition & 0 deletions docs/content/doc/advanced/config-cheat-sheet.en-us.md
View file
Open in desktop
Expand Up @@ -198,6 +198,7 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`.
- `INTERNAL_TOKEN`: **\<random at every install if no uri set\>**: Secret used to validate communication within Gitea binary.
- `INTERNAL_TOKEN_URI`: **<empty>**: Instead of defining internal token in the configuration, this configuration option can be used to give Gitea a path to a file that contains the internal token (example value: `file:/etc/gitea/internal_token`)
- `PASSWORD_HASH_ALGO`: **pbkdf2**: The hash algorithm to use \[pbkdf2, argon2, scrypt, bcrypt\].
- `CSRF_COOKIE_HTTP_ONLY`: **true**: Set false to allow JavaScript to read CSRF cookie.

## OpenID (`openid`)

Expand Down
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Expand Up @@ -47,8 +47,8 @@ require (
github.com/go-macaron/cache v0.0.0-20151013081102-561735312776
github.com/go-macaron/captcha v0.0.0-20190710000913-8dc5911259df
github.com/go-macaron/cors v0.0.0-20190309005821-6fd6a9bfe14e9
github.com/go-macaron/csrf v0.0.0-20180426211211-503617c6b372
github.com/go-macaron/i18n v0.0.0-20160612092837-ef57533c3b0f
github.com/go-macaron/csrf v0.0.0-20190131233648-3751b136073c
github.com/go-macaron/i18n v0.0.0-20190131234336-56731837a73b
github.com/go-macaron/inject v0.0.0-20160627170012-d8a0b8677191
github.com/go-macaron/session v0.0.0-20190131233854-0a0a789bf193
github.com/go-macaron/toolbox v0.0.0-20180818072302-a77f45a7ce90
Expand Down
8 changes: 4 additions & 4 deletions go.sum
View file
Open in desktop
Expand Up @@ -117,10 +117,10 @@ github.com/go-macaron/captcha v0.0.0-20190710000913-8dc5911259df h1:MdgvtI3Y1u/D
github.com/go-macaron/captcha v0.0.0-20190710000913-8dc5911259df/go.mod h1:j9TJ+0nwUOWBvNnm0bheHIPFf3cC62EQo7n7O6PbjZA=
github.com/go-macaron/cors v0.0.0-20190309005821-6fd6a9bfe14e9 h1:A0QGzY6UHHEil0I2e7C21JenNNG0mmrj5d9SFWTlgr8=
github.com/go-macaron/cors v0.0.0-20190309005821-6fd6a9bfe14e9/go.mod h1:utmMRnVIrXPSfA9MFcpIYKEpKawjKxf62vv62k4707E=
github.com/go-macaron/csrf v0.0.0-20180426211211-503617c6b372 h1:acrx8CnDmlKl+BPoOOLEK9Ko+SrWFB5pxRuGkKj4iqo=
github.com/go-macaron/csrf v0.0.0-20180426211211-503617c6b372/go.mod h1:oZGMxI7MBnicI0jJqJvH4qQzyrWKhtiKxLSJKHC+ydc=
github.com/go-macaron/i18n v0.0.0-20160612092837-ef57533c3b0f h1:wDKrZFc9pYJlqFOf7EzGbFMrSFFtyHt3plr2uTdo8Rg=
github.com/go-macaron/i18n v0.0.0-20160612092837-ef57533c3b0f/go.mod h1:MePM/dStkAh+PNzAdNSNl4SGDM2EZvZGken+KpJhM7s=
github.com/go-macaron/csrf v0.0.0-20190131233648-3751b136073c h1:yCyrJuFaxKX/VoV9hHqYXhkFEMtg+Hxsiitk1z/lHfQ=
github.com/go-macaron/csrf v0.0.0-20190131233648-3751b136073c/go.mod h1:oZGMxI7MBnicI0jJqJvH4qQzyrWKhtiKxLSJKHC+ydc=
github.com/go-macaron/i18n v0.0.0-20190131234336-56731837a73b h1:p19t0uFyv0zkBwp4dafzU3EcpHilHNffTVDmxpn/M+w=
github.com/go-macaron/i18n v0.0.0-20190131234336-56731837a73b/go.mod h1:MePM/dStkAh+PNzAdNSNl4SGDM2EZvZGken+KpJhM7s=
github.com/go-macaron/inject v0.0.0-20160627170012-d8a0b8677191 h1:NjHlg70DuOkcAMqgt0+XA+NHwtu66MkTVVgR4fFWbcI=
github.com/go-macaron/inject v0.0.0-20160627170012-d8a0b8677191/go.mod h1:VFI2o2q9kYsC4o7VP1HrEVosiZZTd+MVT3YZx4gqvJw=
github.com/go-macaron/session v0.0.0-20190131233854-0a0a789bf193 h1:z/nqwd+ql/r6Q3QGnwNd6B89UjPytM0be5pDQV9TuWw=
Expand Down
1 change: 1 addition & 0 deletions modules/setting/session.go
View file
Open in desktop
Expand Up @@ -34,6 +34,7 @@ func newSessionService() {
SessionConfig.Secure = Cfg.Section("session").Key("COOKIE_SECURE").MustBool(false)
SessionConfig.Gclifetime = Cfg.Section("session").Key("GC_INTERVAL_TIME").MustInt64(86400)
SessionConfig.Maxlifetime = Cfg.Section("session").Key("SESSION_LIFE_TIME").MustInt64(86400)
SessionConfig.Domain = Cfg.Section("session").Key("DOMAIN").String()

shadowConfig, err := json.Marshal(SessionConfig)
if err != nil {
Expand Down
5 changes: 4 additions & 1 deletion modules/setting/setting.go
View file
Open in desktop
Expand Up @@ -287,7 +287,8 @@ var (
// Time settings
TimeFormat string

CSRFCookieName = "_csrf"
CSRFCookieName = "_csrf"
CSRFCookieHTTPOnly = true

// Mirror settings
Mirror struct {
Expand Down Expand Up @@ -781,6 +782,8 @@ func NewContext() {
ImportLocalPaths = sec.Key("IMPORT_LOCAL_PATHS").MustBool(false)
DisableGitHooks = sec.Key("DISABLE_GIT_HOOKS").MustBool(false)
PasswordHashAlgo = sec.Key("PASSWORD_HASH_ALGO").MustString("pbkdf2")
CSRFCookieHTTPOnly = sec.Key("CSRF_COOKIE_HTTP_ONLY").MustBool(true)

InternalToken = loadInternalToken(sec)
IterateBufferSize = Cfg.Section("database").Key("ITERATE_BUFFER_SIZE").MustInt(50)
LogSQL = Cfg.Section("database").Key("LOG_SQL").MustBool(true)
Expand Down
16 changes: 9 additions & 7 deletions routers/routes/routes.go
View file
Open in desktop
Expand Up @@ -181,12 +181,13 @@ func NewMacaron() *macaron.Macaron {
}

m.Use(i18n.I18n(i18n.Options{
SubURL: setting.AppSubURL,
Files: localFiles,
Langs: setting.Langs,
Names: setting.Names,
DefaultLang: "en-US",
Redirect: false,
SubURL: setting.AppSubURL,
Files: localFiles,
Langs: setting.Langs,
Names: setting.Names,
DefaultLang: "en-US",
Redirect: false,
CookieDomain: setting.SessionConfig.Domain,
}))
m.Use(cache.Cacher(cache.Options{
Adapter: setting.CacheService.Adapter,
Expand All @@ -202,8 +203,9 @@ func NewMacaron() *macaron.Macaron {
Cookie: setting.CSRFCookieName,
SetCookie: true,
Secure: setting.SessionConfig.Secure,
CookieHttpOnly: true,
Copy link
Member

Choose a reason for hiding this comment

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

This is an important security measure and shouldn't be set to false. By setting this to false an attacker could gain access to the cookie.

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 am following this article: http://www.redotheweb.com/2015/11/09/api-security.html

You are right in that session cookie must be HTTPOnly to protect against XSS attack. But it is ok to make CSRF cookie readable by JS and usually passed as header or request body in a CORS ajax call.

Copy link
Member

Choose a reason for hiding this comment

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

I also agree with @techknowlogick that it should be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunny, please take a look at https://medium.com/@iaincollins/csrf-tokens-via-ajax-a885c7305d4a . When we are using Vue (ajax), we need to read the csrf from JS to pass it to api calls. There is no benefit to making csrf cookie HTTPOnly. Frameworks like Django / Laravel make csrf token not HTTPOnly.

It’s important to note you should use HTTP Only cookies for your session tokens (which is best practice and provides protection against XSS attacks). However, CSRF tokens do not need to be stored in cookies — but they can be and unlike session tokens they do not need to be HTTP Only cookies.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that the content from the meta tag (<meta name="_csrf" content=) is what is needed for JS to pass it to API calls using the X-Csrf-Token header.

Copy link
Contributor Author

@tamalsaha tamalsaha Jun 27, 2019 •
edited

Choose a reason for hiding this comment

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

@techknowlogick , it seems that both techniques are used. See here for Laravel https://laravel.com/docs/5.8/csrf .

In terms of security, the goal of HTTPOnly cookie is to ensure that JS can't read that. But if that same value is set on meta tag, that JS can easily read it. So, by making this cookie HTTPonly, we are not achieving anything more security wise.

In our case, _meta tag does not work, because we want to access the csrf token from a Vue / SPA app that is running on a subdomain. That Vue app is purely SPA and does not connect with the same gitea backend. So, we can't set the meta tag. But the cookie technique works well for us. The csrf cookie is set on the .domain.com, so my SPA running on subdom.domain.com can read and pass on that to ajax request as header.

Copy link
Member

Choose a reason for hiding this comment

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

The API should be used in this case, however I can see that you do have a use case for the way you are proposing, and while I feel it would be less secure perhaps we could reach a compromise. Just like your other PR with CORS, perhaps similar to that we make this customizable where HTTPOnly setting (default to true) could be configured along side with the domain for the cookie.

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 can make it configurable. That works for us. Should I call it setting.CSRFCookieHttpOnly ?

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 have added a setting under security.CSRF_COOKIE_HTTP_ONLY. PTAL.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks :)

CookieHttpOnly: setting.CSRFCookieHTTPOnly,
Header: "X-Csrf-Token",
CookieDomain: setting.SessionConfig.Domain,
CookiePath: setting.AppSubURL,
}))
m.Use(toolbox.Toolboxer(m, toolbox.Options{
Expand Down
24 changes: 12 additions & 12 deletions routers/user/auth.go
View file
Open in desktop
Expand Up @@ -58,8 +58,8 @@ func AutoSignIn(ctx *context.Context) (bool, error) {
defer func() {
if !isSucceed {
log.Trace("auto-login cookie cleared: %s", uname)
ctx.SetCookie(setting.CookieUserName, "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CookieRememberName, "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CookieUserName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CookieRememberName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)
}
}()

Expand All @@ -85,7 +85,7 @@ func AutoSignIn(ctx *context.Context) (bool, error) {
if err != nil {
return false, err
}
ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)
return true, nil
}

Expand Down Expand Up @@ -475,9 +475,9 @@ func handleSignIn(ctx *context.Context, u *models.User, remember bool) {
func handleSignInFull(ctx *context.Context, u *models.User, remember bool, obeyRedirect bool) string {
if remember {
days := 86400 * setting.LogInRememberDays
ctx.SetCookie(setting.CookieUserName, u.Name, days, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CookieUserName, u.Name, days, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)
ctx.SetSuperSecureCookie(base.EncodeMD5(u.Rands+u.Passwd),
setting.CookieRememberName, u.Name, days, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
setting.CookieRememberName, u.Name, days, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)
}

_ = ctx.Session.Delete("openid_verified_uri")
Expand Down Expand Up @@ -507,10 +507,10 @@ func handleSignInFull(ctx *context.Context, u *models.User, remember bool, obeyR
}
}

ctx.SetCookie("lang", u.Language, nil, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie("lang", u.Language, nil, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)

// Clear whatever CSRF has right now, force to generate a new one
ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)

// Register last login
u.SetLastLogin()
Expand Down Expand Up @@ -610,7 +610,7 @@ func handleOAuth2SignIn(u *models.User, gothUser goth.User, ctx *context.Context
}

// Clear whatever CSRF has right now, force to generate a new one
ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)

// Register last login
u.SetLastLogin()
Expand Down Expand Up @@ -968,10 +968,10 @@ func handleSignOut(ctx *context.Context) {
_ = ctx.Session.Delete("socialId")
_ = ctx.Session.Delete("socialName")
_ = ctx.Session.Delete("socialEmail")
ctx.SetCookie(setting.CookieUserName, "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CookieRememberName, "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie("lang", "", -1, setting.AppSubURL, "", setting.SessionConfig.Secure, true) // Setting the lang cookie will trigger the middleware to reset the language ot previous state.
ctx.SetCookie(setting.CookieUserName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CookieRememberName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)
ctx.SetCookie(setting.CSRFCookieName, "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)
ctx.SetCookie("lang", "", -1, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true) // Setting the lang cookie will trigger the middleware to reset the language ot previous state.
}

// SignOut sign out from login status
Expand Down
2 changes: 1 addition & 1 deletion routers/user/setting/profile.go
View file
Open in desktop
Expand Up @@ -104,7 +104,7 @@ func ProfilePost(ctx *context.Context, form auth.UpdateProfileForm) {
}

// Update the language to the one we just set
ctx.SetCookie("lang", ctx.User.Language, nil, setting.AppSubURL, "", setting.SessionConfig.Secure, true)
ctx.SetCookie("lang", ctx.User.Language, nil, setting.AppSubURL, setting.SessionConfig.Domain, setting.SessionConfig.Secure, true)

log.Trace("User settings updated: %s", ctx.User.Name)
ctx.Flash.Success(i18n.Tr(ctx.User.Language, "settings.update_profile_success"))
Expand Down
1 change: 0 additions & 1 deletion vendor/github.com/go-macaron/csrf/.travis.yml
View file
Open in desktop

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

11 changes: 8 additions & 3 deletions vendor/github.com/go-macaron/csrf/csrf.go
View file
Open in desktop

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

4 changes: 2 additions & 2 deletions vendor/github.com/go-macaron/csrf/xsrf.go
View file
Open in desktop

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

15 changes: 6 additions & 9 deletions vendor/github.com/go-macaron/i18n/.travis.yml
View file
Open in desktop

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

6 changes: 4 additions & 2 deletions vendor/github.com/go-macaron/i18n/i18n.go
View file
Open in desktop

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

4 changes: 2 additions & 2 deletions vendor/modules.txt
View file
Open in desktop
Expand Up @@ -128,9 +128,9 @@ github.com/go-macaron/cache/redis
github.com/go-macaron/captcha
# github.com/go-macaron/cors v0.0.0-20190309005821-6fd6a9bfe14e9
github.com/go-macaron/cors
# github.com/go-macaron/csrf v0.0.0-20180426211211-503617c6b372
# github.com/go-macaron/csrf v0.0.0-20190131233648-3751b136073c
github.com/go-macaron/csrf
# github.com/go-macaron/i18n v0.0.0-20160612092837-ef57533c3b0f
# github.com/go-macaron/i18n v0.0.0-20190131234336-56731837a73b
github.com/go-macaron/i18n
# github.com/go-macaron/inject v0.0.0-20160627170012-d8a0b8677191
github.com/go-macaron/inject
Expand Down