Skip to content

add salt length calculation with PSSSaltLengthAuto - #96

Closed
maraino wants to merge 1 commit into
eclipse-keypont:masterfrom
maraino:rsa-pss-auto
Closed

add salt length calculation with PSSSaltLengthAuto#96
maraino wants to merge 1 commit into
eclipse-keypont:masterfrom
maraino:rsa-pss-auto

Conversation

@maraino

@maraino maraino commented Jul 13, 2022

Copy link
Copy Markdown
Contributor

Description

This PR adds support for signing using rsa.PSSSaltLengthAuto

When PSSSaltLengthAuto is set, the maximum salt length must equal:

(modulus_key_size - 1 + 7)/8 - hash_length - 2

For example, for a 4096-bit modules key and SHA256 it should be:

(4096 - 1 + 7)/8 - 32 - 2 = 478

See https://golang.org/cl/302230

When PSSSaltLengthAuto is set, the maximum salt length must equal:

  (modulus_key_size - 1 + 7)/8 - hash_length - 2

For example, for a 4096-bit modules key and SHA256 it should be:

  (4096 - 1 + 7)/8 - 32 - 2 = 478

See https://golang.org/cl/302230
@maraino maraino changed the title fix salt length calculation with PSSSaltLengthAuto add salt length calculation with PSSSaltLengthAuto Jul 13, 2022
@maraino

maraino commented Aug 12, 2022

Copy link
Copy Markdown
Contributor Author

@solcates, are you maintaining this repo? Any updates on this?

@optnfast optnfast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The change looks good to me.

@maraino

maraino commented Nov 11, 2022

Copy link
Copy Markdown
Contributor Author

@optnfast do you know who has approving rights in this repo and can merge this? @solcates?

@optnfast

Copy link
Copy Markdown
Contributor

@solcates would be my first thought but I no longer work for Thales so I don't really know.

@solcates solcates left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I haven't been checking up on this code like I should have

LGTM

@maraino

maraino commented Jan 27, 2025

Copy link
Copy Markdown
Contributor Author

Sorry, I haven't been checking up on this code like I should have

LGTM

@solcates is it possible to merge this and tag a new version?

@Nicolas-Peiffer Nicolas-Peiffer self-assigned this Aug 3, 2026
@Nicolas-Peiffer

Nicolas-Peiffer commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Hi @maraino 😃 , TY 🙏 for your interest and this PR. I am part of the new maintainers of eclipse-keypont/crypto11 (previously know as ThalesGroup/crypto11).

In 2026 the big project for crypto11 is to add the support of PKCS#11 v3.2 and the support of the ML-KEM PQC algorithm:

The other big project for 2026 is giving + moving the crypto11 project under the Eclipse Foundation.

This is an opportunity for us to reconsider PRs and issues. And I can see your PR was on track for a merge... until it wasn't anymore. I apologize for this, lets see if we can do better 😺 .

I see 2 options:

  • for eclipse-keypont/crypto11@v2, I will take into account your contribution. I will work on that this week. crypto11@v2.0.0 will probably be shipped soonish, some v2-rc release candidates are already available and will be this week with your fix.
  • for the v1 crypto11@v1, I could create a new tag from the latest eclipse-keypont/crypto11@v1.6.8). I call it v1.6.9 or v1.7.0. This tag could be a transition tag, meaning it adds the feature to crypto11@v1 while giving you time to transition to future eclipse-keypont/crypto11@v2.

In case I can not use your PR as is, I will mention your contribution in the commits and changelogs. Thank you for this valuable input.

Nicolas-Peiffer added a commit that referenced this pull request Aug 3, 2026
signPSS returned errUnsupportedRSAOptions for rsa.PSSSaltLengthAuto, behind
a TODO saying the biggest possible salt could in principle be worked out
from the key. It can, and crypto/rsa already does it:

  (modulus_bits - 1 + 7)/8 - hash_length - 2

For a 4096-bit modulus and SHA-256 that is (4096-1+7)/8 - 32 - 2 = 478.

The salt is the largest the encoded message can carry, so callers that pass
the zero-valued PSSOptions.SaltLength — the crypto.Signer default, and what
several callers hand us without thinking about salts at all — now get a
signature rather than an error. Verifiers accept it either way:
PSSSaltLengthAuto recovers the salt length from the encoding.

The calculation moves into maxPSSSaltLength so it can be tested without a
token, and differs from #96 in one respect. That PR computes the length as
uint(k.N.BitLen()-1+7)/8 - 2 - hLen, all unsigned; a modulus too small for
the chosen hash (SHA-512 at 521 bits or below) makes that subtraction wrap
and yields a salt length near 2^64 instead of an error. Doing the
arithmetic in int and reporting rsa.ErrMessageTooLong on a negative result
matches what crypto/rsa returns for the same case.

errUnsupportedRSAOptions is still returned for PSSSaltLengthAuto when the
key's public half is not an *rsa.PublicKey, since there is no modulus to
size the salt from. Both doc comments that described Auto as unsupported are
updated.

testRsaSigningPSS now runs each hash against both Auto and EqualsHash, and
TestMaxPSSSaltLength covers the helper against the crypto/rsa expression for
1024- through 4096-bit keys across SHA-1..SHA-512, round-trips a signature
through VerifyPSS with Auto, and pins the two error paths. It skips the
4096-bit key under -short, where generating it dominates the runtime.

Note that a token is free to reject a salt length it does not implement;
SoftHSMv3 does not advertise CKM_RSA_PKCS_PSS at all, so TestHardRSA's PSS
subtests skip there and this path is covered natively.

Fixes the TODO with the calculation from #96 by @maraino, who also supplied
the worked 4096/SHA-256 example above and the crypto/rsa change it follows
(https://golang.org/cl/302230).

Refs #96#96

Co-Authored-By: Mariano Cano <mariano.cano@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Nicolas-Peiffer Nicolas-Peiffer added this to the v2.0.0 milestone Aug 3, 2026
@Nicolas-Peiffer

Copy link
Copy Markdown
Contributor

Closing this issue as implemented by b272882 and mentioning @maraino for the contribution. Target crypto11@v2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants