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

PublicKeyCredentialCreationOptions and AssertionRequest are not serializable #18

Closed
jdhoek opened this issue Mar 11, 2019 · 9 comments
Closed
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@jdhoek
Copy link
Contributor

jdhoek commented Mar 11, 2019

For both the registration and the authentication ceremonies, the options used to to initiate them are kept server-side for the verification step. It would be very helpful if PublicKeyCredentialCreationOptions and AssertionRequest could be made to implement Serializable; that would allow developers to store instances of these classes in sessions, database, or cache without having to serialize them themselves.

@emlun
Copy link
Member

emlun commented Mar 11, 2019

Thanks, I've been considering that too. Will do!

@emlun emlun added the enhancement New feature or request label Mar 11, 2019
@emlun
Copy link
Member

emlun commented Mar 13, 2019

On further thought, I'm no longer sure this is a good idea, for a couple of reasons:

  • Java object serialization has been a hot spot for security vulnerabilities in the past, and it would be very unfortunate for a security library to also fall victim to that.
  • Most of the relevant classes represent transient state that shouldn't need to live much longer than tens of seconds. Storing them in a database or the like is probably not what you want anyway, and the parts you do need to store (mainly public keys and attestation statements) can easily be extracted as strings or raw byte arrays.

With that in mind we're now very hesitant to implement this. We might reconsider if there's a concrete use case with a clear benefit outweighing the risks; you're welcome to re-open the issue if you have one.

@emlun emlun closed this as completed Mar 13, 2019
@emlun emlun added the wontfix This will not be worked on label Mar 13, 2019
@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 13, 2019

The two classes mentioned are intended to be reused in the follow up call for registration and authentication, so they have to be stored somewhere server-side. It's cumbersome to have to serialize them to a JSON String just to be able to store them in a HttpSession class (JAX-RS), or whatever keeps state across calls.

The canonical Java way of doing that is to make the classes Serializable. Alternatively, a built-in toJsonString and fromJsonString method pair on both would work as well, if you are not comfortable with implementing Serializable.

This problem doesn't exist with loading credential data from the database because RegisteredCredential has a suitable builder for passing the raw credential-ID and COSE data gotten from RegistrationResult when the data was stored.

@ghost
Copy link

ghost commented Mar 13, 2019

Implementing Serializable would need a lot of changes as Optional is not serializable.

@jdhoek You can solve your issue in injecting your objects in the HttpSession trough a wrapper class implementing Externalizable, delegating writeExternal and readExternal methods processing to a com.fasterxml.jackson.databind.ObjectMapper.

@emlun
Copy link
Member

emlun commented Mar 13, 2019

Are we talking about javax.servlet.HttpSession? Because that class supports putting a POJO into setAttribute with no serialization required.

@ghost
Copy link

ghost commented Mar 13, 2019

Serialization is required for HTTP Session State Replication in a clustered environment .

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 13, 2019

@emlun No, javax.servlet.http.HttpSession. It will throw an error during runtime if the class passed to setAttribute isn't Serializable.

Implementing Serializable would need a lot of changes as Optional is not serializable.

@ed5s Not too surprising; Optional is not designed to be used for class fields and method parameters (IntelliJ will even warn you when you do try to do that). It's possible of course, but not recommended.

@emlun
Copy link
Member

emlun commented Mar 13, 2019

My mistake, the class I linked to is javax.servlet.http.HttpSession. That one happily accepts and returns a com.yubico.webauthn.data.PublicKeyCredentialCreationOptions for me in both JDK 8 and JDK 10.

My fork will include a full reactive implementation for the demo server.

That sounds cool, I'd be interested in merging that if you want to.

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 13, 2019

My mistake, the class I linked to is javax.servlet.http.HttpSession. That one happily accepts and returns a com.yubico.webauthn.data.PublicKeyCredentialCreationOptions for me in both JDK 8 and JDK 10.

Interesting, I got stopped there with JDK 8. Will test again this Friday.

@ed5s The general contract and intent of Optional in Java seems to be that you consume them soon after receiving them as a return value of any method; their major benefit being the ability to work with them in a functional manner (ifPresent, map, and orElse* rather than isPresent and get) and having the assurance that a method that returns never returns null by strong convention.

Setters and other methods where a parameter is expected should either not be called, or called with a non-null value.

To be fair, there is a second school of Optional use that does use it everywhere (including fields), although that seems a minority. The relevant discussions on StackOverflow may be informative: https://stackoverflow.com/questions/23454952/uses-for-optional/23464794#23464794.

Project Reactor looks quite interesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Development

No branches or pull requests

2 participants