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

Don't set and enforce secure cookie if TLS disabled #1311

Closed
wants to merge 1 commit into from

Conversation

Legogris
Copy link
Contributor

@Legogris Legogris commented Jun 4, 2019

  • Disables secure middleware when TLS completely disabled
  • Don't set secure cookie when TLS completely disabled

@Legogris
Copy link
Contributor Author

Legogris commented Jun 4, 2019

Failing CI test looks unrelated to this change.

@j16r
Copy link
Contributor

j16r commented Jun 4, 2019

Hi @Legogris, thanks for the PR! We'd like to add a configuration flag to toggle secure cookies as we need secure cookies to be on, even when TLS is not being served by chainlink. See more details on https://www.pivotaltracker.com/n/projects/2129823/stories/166342566. This will allow us to keep the secure cookies flag even when we terminate TLS using nginx in front of our chainlink instances.

@dimroc
Copy link
Contributor

dimroc commented Jun 5, 2019

@Legogris the PR allowing one to toggle secure cookies has been merged into master and can be found here: #1316

Use:

SECURE_COOKIES=false chainlink node ...

The secureMiddleware works as is with the flags set in options:

func (c Config) SessionOptions() sessions.Options {
	return sessions.Options{
		Secure:   c.SecureCookies(),
		HttpOnly: true,
		MaxAge:   86400 * 30,
	}

Let us know if you have any issues. If you're good to go, we'll go ahead and close this PR.

@Legogris
Copy link
Contributor Author

Legogris commented Jun 6, 2019

@dimroc My bad, missed that it was done already - closing this.
BTW, how do you feel about adding a configuration option TLS_REDIRECT? We're doing the same thing as @j16r with our own TLS termination and handling this in the app would be nice.

@Legogris Legogris closed this Jun 6, 2019
@Legogris Legogris deleted the fix-http-401 branch June 6, 2019 10:45
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.

3 participants