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

2019-02-19_fix-pss-saltlen #65

Merged
merged 2 commits into from
Jan 3, 2020

Conversation

ntrepid8
Copy link
Contributor

Changes

  • use -1 instead of -2 for sign according to RFC7518
  • update docs according to RFC7518 and pkeyutl/evp_pkey_ctrl_str

Notes

I was looking at the RFC here:

This line in section 3.5:

The size of the salt value is the same size as
the hash function output.

The issue was discovered trying to pass values between this library and an equivalent Python library. I don't think this should cause any backward compatibility problems because we only change to use -1 for creating the signature but -2 is still used to verify the signature which should work for the max-length and for the hash-length formats.

@ntrepid8
Copy link
Contributor Author

Is there some trick to get the tests to pass? When I run them locally they pass sometimes and they fail sometimes. I'm not really sure what makes the difference.

@opsb
Copy link

opsb commented May 20, 2019

We ran into this issue as well. We discovered that while this PR configures the correct salt length, jose doesn't actually use the jose_jws_alg_rsa_pss so the config is never used. To get it working we had to use this config in ets.

:ets.insert(:jose_jwa, {{:rsa_sign, :rsa_pkcs1_pss_padding}, {
      :public_key,
      [
        {:rsa_padding, :rsa_pkcs1_pss_padding},
        {:rsa_pss_saltlen, -1}
      ]
}})

By default https://github.com/potatosalad/erlang-jose/blob/master/src/jose_jwa.erl#L114 is returning {public_key, opts} so the jose_jws_alg_rsa_pss module isn't actually being used for the signing. For this reason the current PR seems like only half the story.

If erlang's public_key module is being used currently the question is, does it support the salt length configuration (it doesn't from what we can tell). If not then should the pure erlang implementation be used by default?

@opsb
Copy link

opsb commented May 20, 2019

@ntrepid8 were you able to use jose with your configuration change somehow?

@ntrepid8
Copy link
Contributor Author

@opsb as I recall we were able to make jose interact correctly with our other services once this change was made. Specifically we were able to use it with the Python library:

@potatosalad potatosalad added this to To Do in jose 1.10.x Jan 2, 2020
@potatosalad potatosalad moved this from To Do to In Progress in jose 1.10.x Jan 3, 2020
@potatosalad potatosalad self-assigned this Jan 3, 2020
@potatosalad
Copy link
Owner

@ntrepid8 You are exactly right, I misread RFC7518 — Section 3.5. Digital Signature with RSASSA-PSS where is says:

The size of the salt value is the same size as the hash function output.

Thank you for the fix, this has been merged into develop and will be released as part of version 1.10.0 soon-ish.

@potatosalad potatosalad moved this from In Progress to Done in jose 1.10.x Jan 3, 2020
potatosalad added a commit that referenced this pull request Jan 3, 2020
* Enhancements
  * Remove [base64url](https://github.com/dvv/base64url) dependency and include embedded version.
  * Add support for `C20P` and `XC20P` encryption based on [draft-amringer-jose-chacha](https://tools.ietf.org/html/draft-amringer-jose-chacha-01) (ChaCha20/Poly1305 and XChaCha20/Poly1305).
  * Add support for ECDH-ES keywrapping for AES-GCM, ChaCha20/Poly1305, and XChaCha20/Poly1305.
  * Add support for PBES2 keywrapping for AES-GCM, ChaCha20/Poly1305, and XChaCha20/Poly1305.
  * Add support for `ECDH-1PU` encryption based on [draft-madden-jose-ecdh-1pu](https://tools.ietf.org/html/draft-madden-jose-ecdh-1pu-02).
  * Add support for reading/writing DER format (or PKCS8 format).

* Fixes
  * Fix PSS salt length (thanks to [@ntrepid8](https://github.com/ntrepid8), see [#65](#65))
  * Speed up and stabilize tests on CI environment.
@potatosalad potatosalad merged commit 1cc90ea into potatosalad:master Jan 3, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 23, 2020
# Changelog

## 1.10.1 (2020-01-08)

* Fixes
  * Add PEM/DER compatibility layer for PKCS-8 incompatibilities with various versions of OTP, `crypto`, and `public_key`; see [#82](potatosalad/erlang-jose#82)

## 1.10.0 (2020-01-03)

* Enhancements
  * Remove [base64url](https://github.com/dvv/base64url) dependency and include embedded version.
  * Add support for `C20P` and `XC20P` encryption based on [draft-amringer-jose-chacha](https://tools.ietf.org/html/draft-amringer-jose-chacha-01) (ChaCha20/Poly1305 and XChaCha20/Poly1305).
  * Add support for ECDH-ES keywrapping for AES-GCM, ChaCha20/Poly1305, and XChaCha20/Poly1305.
  * Add support for PBES2 keywrapping for AES-GCM, ChaCha20/Poly1305, and XChaCha20/Poly1305.
  * Add support for `ECDH-1PU` encryption based on [draft-madden-jose-ecdh-1pu](https://tools.ietf.org/html/draft-madden-jose-ecdh-1pu-02).
  * Add support for reading/writing DER format (or PKCS8 format).

* Fixes
  * Fix PSS salt length (thanks to [@ntrepid8](https://github.com/ntrepid8), see [#65](potatosalad/erlang-jose#65))
  * Speed up and stabilize tests on CI environment.

## 1.9.0 (2018-12-31)

* Enhancements
  * Add support for [Jason](https://github.com/michalmuskala/jason) JSON encoding and decoding.
  * Add support for Poison 4.x and lexical ordering.
  * Use `public_key` over `cutkey` for RSA key generation if available.
  * Drop support for older versions of OTP (19+ now required).
  * Relicense library under MIT license.

* Fixes
  * Add macro so the application compiles without warnings after `erlang:get_stacktrace/0` has been deprecated.
  * Extra sanity check for RSA padding modes when falling back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
jose 1.10.x
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants