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

Support for RS384 and RS512 #235

Merged
merged 4 commits into from
Jan 26, 2023

Conversation

JohnnyJayJay
Copy link
Contributor

This PR makes a very small change to add support for the RSA PKCS#1 based signing algorithms that use SHA-384 or SHA-512 respectively.

I assume it can't hurt to support as many signing algorithms as possible and support for these is trivial, since they can be used exactly the same way as RS1 and RS256.

LMK if I missed something, e.g. if I still need to add tests somewhere.

@emlun
Copy link
Member

emlun commented Oct 10, 2022

Hi! Thanks, but yes, unfortunately there's quite a lot more that goes into adding support for new algorithms. Take a look at commit e573f14 for an example (that includes some updates to internal utilities as well, but is a pretty fair representation of the scale). But I'm happy to go through the motions if there's demand for it.

Is this PR motivated by a concrete demand for these features, or just wanting to contribute? I'm asking because I'm not aware of any WebAuthn authenticators that use these algorithms in practice. 🙂

@JohnnyJayJay
Copy link
Contributor Author

Thanks, but yes, unfortunately there's quite a lot more that goes into adding support for new algorithms. Take a look at commit e573f14 for an example

It seems like I missed some places where the algorithm names are used, but looking at this commit it doesn't seem like a lot of work, especially considering that the COSE format is the same for all the RS... algorithms. I can easily introduce the changes that are still required, it still seems more or less trivial.

Is this PR motivated by a concrete demand for these features, or just wanting to contribute?

Mostly just wanting to contribute - my rationale was that it can't hurt to support more algorithms. I don't know if there is any authenticator out there right now that uses it - I do know WebAuthn4J has support for it though.

@emlun
Copy link
Member

emlun commented Jan 20, 2023

Sorry for leaving this for so long, I've had to focus on other things for a while but now I've come back around to this.

Thanks for the suggestion and contribution! I've made the additional changes needed for this, but not pushed them yet. We'll credit you for the contribution in the release notes, unless you prefer we don't - would you like to be credited by GitHub username or real name?

@JohnnyJayJay
Copy link
Contributor Author

Oh, thanks for getting back to me. To be honest, I had somewhat forgotten about this in the last months as I got caught up in other things. I'm happy the change made it further, though.
You can credit me using my GitHub username.

@emlun emlun merged commit 39be670 into Yubico:main Jan 26, 2023
emlun added a commit that referenced this pull request Feb 15, 2023
`webauthn-server-core`:

New features:

- Added support for RS384 and RS512 signature algorithms.
  - Thanks to GitHub user JohnnyJayJay for the contribution, see
    #235
- Added `userHandle` field to `AssertionRequest` as part of the second
  bug fix below. `userHandle` is mutually exclusive with `username`.
  This was originally released in pre-release `1.12.3-RC3`, but was
  accidentally left out of the `1.12.3` release.

Fixes:

- During `RelyingParty.finishRegistration()` if an
  `attestationTrustSource` is configured, if the `aaguid` in the
  authenticator data is zero, the call to
  `AttestationTrustSource.findTrustRoots` will fall back to reading
  the AAGUID from the attestation certificate if possible.
- Fixed bug in `RelyingParty.finishAssertion` where if
  `StartAssertionOptions.userHandle` was set, it did not propagate to
  `RelyingParty.finishAssertion` and caused an error saying username
  and user handle are both absent unless a user handle was returned by
  the authenticator. This was originally released in pre-release
  `1.12.3-RC3`, but was accidentally left out of the `1.12.3` release.
- Fixed regression in
  `PublicKeyCredentialCreationOptions.toCredentialsCreateJson()`,
  which has not been emitting a `requireResidentKey` member since
  version `2.0.0`. This meant the JSON output was not backwards
  compatible with browsers that only support the Level 1 version of
  the WebAuthn spec.

`webauthn-server-attestation`:

Fixes:

- `findEntries` and `findTrustRoots` methods in `FidoMetadataService`
  now attempt to read AAGUID from the attestation certificate if the
  `aaguid` argument is absent or zero.
- Method `FidoMetadataService.Filters.allOf` now has `@SafeVarargs`
  annotation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants