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

Encrypting user/email from cookie #120

Merged
merged 4 commits into from Apr 10, 2019
Merged

Encrypting user/email from cookie #120

merged 4 commits into from Apr 10, 2019

Conversation

costelmoraru
Copy link
Contributor

Description

This change does one thing:

if a cypher is provided, it encrypts also the email and user with the same logic and algorithm as the rest of the fields from the session state.

Motivation and Context

This is the PR for the issue #60 .
Don't expose any information part of the cookie, information that can be easily retrieved if not encrypted.

How Has This Been Tested?

All the test from session_state_test are passing.
Testing in our environments.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@costelmoraru costelmoraru requested a review from a team April 9, 2019 11:57
@costelmoraru
Copy link
Contributor Author

costelmoraru commented Apr 9, 2019

@JoelSpeed, Just pushed the code for encrypting the user and email for the new json encoding of the cookie.

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Just a couple of minor bits but generally looks pretty good, thanks for sorting this one 😄

providers/session_state.go Show resolved Hide resolved
@@ -62,6 +62,19 @@ func (s *SessionState) EncodeSessionState(c *cookie.Cipher) (string, error) {
} else {
ss = *s
var err error
// Encrypt also Email and User when cipher is provided
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this comment is necessary, personally I would drop it but happy to leave if you feel strongly about 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.

Yes, I guess from this point forward there is no different treatment in encrypting the email and user versus the other fields. Removing the comment.

Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for the fix! 👍

@JoelSpeed JoelSpeed merged commit 3f4420f into oauth2-proxy:master Apr 10, 2019
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.

None yet

2 participants