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

Implement SHA-256 in token based Remember-Me services #10675

Conversation

marcusdacoregio
Copy link
Contributor

Closes gh-8549

sargas and others added 2 commits January 3, 2022 08:44
A hashing algorithm property is added to TokenBasedRememberMeServices to
choose which algorithm is used when creating new Remember Me tokens.
This implementation is intended to preserve compatibility both with
Remember Me tokens that do not specify a hashing algorithm, and with
subclasses of TokenBasedRememberMeServices.

Closes spring-projectsgh-8549
@marcusdacoregio marcusdacoregio added status: duplicate A duplicate of another issue in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement labels Jan 4, 2022
@marcusdacoregio marcusdacoregio added this to the 5.7.0-M1 milestone Jan 4, 2022
@sjohnr sjohnr modified the milestones: 5.7.0-M1, 5.7.0-M2 Jan 14, 2022
@marcusdacoregio marcusdacoregio modified the milestones: 5.7.0-M2, 5.7.0-M3 Feb 21, 2022
@marcusdacoregio marcusdacoregio modified the milestones: 5.7.0-M3, 5.7.0-RC1 Mar 21, 2022
@rwinch rwinch modified the milestones: 5.7.0-RC1, 5.8.x Apr 18, 2022
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @marcusdacoregio. I've left my comments inline.

* @return the {@link RememberMeConfigurer} for further customization
* @since 5.7
*/
public RememberMeConfigurer<H> hashingAlgorithm(RememberMeHashingAlgorithm hashingAlgorithm) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since RememberMeServices can be constructed with relative ease, I'd recommend leaving this out of the DSL for the time being.

@@ -1,5 +1,5 @@
/*
* Copyright 2004, 2005, 2006 Acegi Technology Pty Limited
Copy link
Contributor

Choose a reason for hiding this comment

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

The ones that say "Acegi Technology Pty Limited", we leave as-is

* or, if a hashing algorithm is configured, the form:
*
* <pre>
* username + &quot;:&quot; + expiryTime + &quot;:&quot; + algorithmName + &quot;:&quot;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this should be thought of in the same way as how Spring Security deals with passwords.

For example, you might consider having an algorithm-for-matches and an algorithm-for-encoding, both defaulting to MD5.

  • When encoding, encode with the algorithm-for-encoding, adding the prefix either way (even if the app hasn't configured it directly)
  • When decoding
    ** if there is no algorithm in the cookie, use the algorithm-for-matches
    ** if there is an algorithm in the cookie, use that algorithm

This will allow folks to safely upgrade to SHA-256 without losing their old MD5 hash cookies.

* @param hashingAlgorithm the hashing algorithm to use in the Cookie value
*/
public TokenBasedRememberMeServices(String key, UserDetailsService userDetailsService,
RememberMeHashingAlgorithm hashingAlgorithm) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that since this is an optional parameter, it would be ideal as a setter.

@@ -1798,6 +1798,11 @@ Specifies the period in seconds for which the remember-me cookie should be valid
By default it will be valid for 14 days.


[[nsa-remember-me-hashing-algorithm]]
* **hashing-algorithm**
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise, would recommend that this be left out for now in favor of applications using services-ref.

@jzheaux jzheaux self-assigned this May 27, 2022
@marcusdacoregio
Copy link
Contributor Author

Hi @jzheaux, I'm closing this in favor of #11464. Can you please review that one? I've addressed your feedback there.

@marcusdacoregio marcusdacoregio added the status: declined A suggestion or change that we don't feel we should currently apply label Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: declined A suggestion or change that we don't feel we should currently apply status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants