Add Support for Authenticated Encrypted Sessions #2711
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| */ | ||
| public final class SecurityContextAttributePersistentSessionRestorer implements PersistentSessionPrincipalRestorer { | ||
|
|
||
| private final UserDetailsService userDetailsService; |
There was a problem hiding this comment.
I wonder what the goals of this project are. I believe that many would want the session to no longer require lookup in an external system. I would expect that is why they are cryptographically signing the value in the cookie.
I am not certain I agree with the value of using cryptography to specify the username (extra complexity) to then lookup the User from a persistent store vs standard Spring Session which specifies a session id (not cryptography) to lookup all the session information from a persistent store.
I believe most users are looking to remove the lookup from an external system. This means the value would likely need to be self contained rather than requiring a lookup.
| @Override | ||
| public void restore(String principal, PersistentSessionRepository.PersistentSession session) { | ||
| UserDetails userDetails = getUserDetails(principal); | ||
| PersistentAuthenticationToken authentication = new PersistentAuthenticationToken(userDetails, session.getId()); |
There was a problem hiding this comment.
This seems to only support UserDetails. How so other authentication types (e.g. OIDC) work?
| * @author Marcus da Coregio | ||
| * @since 3.3 | ||
| */ | ||
| public interface PersistentSessionPrincipalNameResolver { |
There was a problem hiding this comment.
I'm curious why this interface exists. I'm guessing this is because the entire UsernamePasswordAuthenticationToken is quite large to encode in the session id and so this tries to reduce the value to a username.
However, this makes me wonder about other values that might be too large to persist. Will we need to add a new custom interface for everything that a user might want to simplify?
I wonder if using Spring's generic conversion APIs might be better. This would allow users to convert their own types of objects as well.
For Spring Security related objects, I wonder if we could also provide a SecurityContextPersistenceRepository that reduces the object size outside of Spring Session. For example perhaps it could write JSON serialization of the SecurityContext to the session attribute. We could probably optimize this within Spring Security. If the Authentication is a UsernamePasswordAuthenticationToken, it could just persist a UserDetails with the username and roles.
We could also experiment with writing in more compact formats. For example, Jackson supports other data formats (e.g. CBOR).
| this.clock = clock; | ||
| } | ||
|
|
||
| public final class PersistentSession implements Session { |
There was a problem hiding this comment.
All of Spring Session persists the session (in Redis, Hazelcast, etc) so this does not seem to uniquely identify the implementation.
I would try to name the implementation based upon the fact that AuthenticatedEncryption is used. Perhaps AEAD (Authenticated Encryption with Associated Data) could be used which is a well known, unique, and concise way to name it.
| } | ||
|
|
||
| @Override | ||
| public void save(PersistentSession session) { |
There was a problem hiding this comment.
As this is currently implemented, I think it should fail if additional attributes are in the session so that users are not surprised that session information is lost.
| * The session id later on be decrypted and the session contents restored. Supports | ||
| * clustering if all cluster members share the same encryption key. | ||
| * <p> | ||
| * Note that, by default, this implementation only serializes the username found in the |
There was a problem hiding this comment.
I think that by default needs removed since there is no way to customize the implementation to serialize other attributes.
| } | ||
| String formatted = session.getExpireAt().toEpochMilli() + SEPARATOR + username; | ||
| byte[] encrypted = this.encryptor.encrypt(formatted.getBytes(StandardCharsets.UTF_8)); | ||
| String encryptedId = new String(Hex.encode(encrypted)); |
There was a problem hiding this comment.
I think that URL Base64 encoding might be better as it works for HTTP spec and it is more compact that Hex.
| * Note that, by default, this implementation only serializes the username found in the | ||
| * session by using a {@link PersistentSessionPrincipalNameResolver}, all attributes will | ||
| * be lost during serialization. If you need the session attributes to be persisted during | ||
| * serialization/deserialization, this implementation might not be the best choice. |
There was a problem hiding this comment.
I'm not sure if this limitation is going to allow a majority of users to use it. Many users place additional attributes in the session. A few common examples:
- Spring Security's OIDC based authentication leverages the session to persist the OAuth2AuthorizationRequest in order to validate the state parameter within the OAuth dance.
- Many users leverage Spring MVC Flash Attributes.
I think we need to be able to support additional attributes to make this a solution that users can leverage.
| return this.secretKey; | ||
| } | ||
| try { | ||
| return KeyGenerator.getInstance("AES").generateKey(); |
There was a problem hiding this comment.
I think that it is ok that we generate the key if one is not provided, but we should log that one is being created.
To ensure that the key is not generated multiple times if getSecretKey is invoked more than once it should save to this.secretKey. It might be easier to invert the logic. If the key is null default it to the generated value and log. Then always return this.secretKey
We might consider asking Boot to add autoconfiguration to allow specifying the secret through a property so that users can leverage standard Boot mechanisms to configure the secret. For that to work ensure we have a recommended way for Boot to provide the SecretKey if it is specified as a property.
| @Autowired(required = false) | ||
| @Qualifier("springSessionPersistentSessionSecretKey") | ||
| public void setSecretKey(SecretKey secretKey) { | ||
| this.secretKey = secretKey; |
There was a problem hiding this comment.
I think that using a qualifier is ok. However, you might consider providing a meta annotation so that users do not need to remember the qualifier name.
I'm not sure but I think Qualifier only is used if there are multiple beans of the same type (you might double check that). If that is true, you might create a configuration object that could be provided that contains the secret key. That would also ensure that users don't accidentally using another SecretKey (which is a common type) with Spring Session.
marcusdacoregio
removed
the
status: waiting-for-triage
No description provided.