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.

Sign up for GitHub

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

Jump to bottom

Confusion over pkcs1v15::VerifyingKey::new vs ::new_with_prefix #238

Closed
tarcieri opened this issue Dec 19, 2022 · 2 comments · Fixed by #290
Closed

Confusion over pkcs1v15::VerifyingKey::new vs ::new_with_prefix #238

tarcieri opened this issue Dec 19, 2022 · 2 comments · Fixed by #290
Labels
api discussion enhancement New feature or request

Comments

@tarcieri
Copy link
Member

In #234 we had some people confused about signature verification failing with ::new, and they had trouble discovering ::new_with_prefix.

It would be good to either improve the documentation here, or eliminate the distinction between ::new and ::new_with_prefix and automatically determining if the prefix is needed (e.g. based on the OID of the provided digest).

It seems like other RSA libraries are able to make this determination automatically.
@tarcieri tarcieri mentioned this issue Dec 19, 2022
Closed
@tarcieri tarcieri mentioned this issue Dec 26, 2022
Closed
@dignifiedquire
Copy link
Member

I think it would be great if we had

  • new() -> determines it automatically
  • new_prefix(p: Prefix) explicitly expects a prefix or not. With enum Prefix { Yes, No } (names can be bikeshedded) for when you want to enforce one or the other.

@tarcieri
Copy link
Member Author

tarcieri commented Jan 6, 2023 •
edited
Loading

I think part of the rationale for the current split is avoiding the need to have an AssociatedOid on the digest used. However, that just seems to be causing confusion.

Looking at RFC8017 Appendix 2.4, I'm unclear under what circumstances the OID can actually be omitted?

So perhaps the current new should be renamed to something like new_unprefixed, and new_with_prefixed should be renamed to new?

tarcieri added a commit that referenced this issue Jan 20, 2023
@tarcieri
Add sha2 feature with oid subfeature enabled
b860e5c 
We seem to be running into a lot of people who are having trouble with
PKCS#1 v1.5 signatures because the failure mode for the `oid` feature of
the `sha2` crate being disabled is fairly unscrutable.

See #234, #253, and the semi-related tracking issue for #238.

If `rsa` has a `sha2` feature, we can always ensure `oid` is enabled,
and this can be used in code examples. It also means users don't need
two crates to create/verify PKCS#1 v1.5 signatures.

RSA is used commonly enough with the SHA2 family that this integration
probably makes sense.
@tarcieri tarcieri mentioned this issue Jan 20, 2023
Merged
tarcieri added a commit that referenced this issue Jan 20, 2023
@tarcieri
Add sha2 feature with oid subfeature enabled (#255)
134a061 
We seem to be running into a lot of people who are having trouble with
PKCS#1 v1.5 signatures because the failure mode for the `oid` feature of
the `sha2` crate being disabled is fairly unscrutable.

See #234, #253, and the semi-related tracking issue for #238.

If `rsa` has a `sha2` feature, we can always ensure `oid` is enabled,
and this can be used in code examples. It also means users don't need
two crates to create/verify PKCS#1 v1.5 signatures.

RSA is used commonly enough with the SHA2 family that this integration
probably makes sense.
@tarcieri tarcieri mentioned this issue Mar 6, 2023
Merged
tarcieri added a commit that referenced this issue Apr 10, 2023
@tarcieri
pkcs1v15: make *_with_prefix methods the default
ee09aab 
Renames the following:

- `SigningKey::new` => `SigningKey::new_unprefixed`
- `SigningKey::new_with_prefix` => `SigningKey::new`
- `VerifyingKey::new` => `VerifyingKey::new_unprefixed`
- `VerifyingKey::new_with_prefix` => `VerifyingKey::new`

The `*_with_prefix` methods are preserved with a deprecation warning,
which should help people migrate to the new versions.

Closes #238
@tarcieri tarcieri mentioned this issue Apr 10, 2023
Merged
tarcieri added a commit that referenced this issue Apr 11, 2023
@tarcieri
pkcs1v15: make *_with_prefix methods the default (#290)
bf1defd 
Renames the following:

- `SigningKey::new` => `SigningKey::new_unprefixed`
- `SigningKey::new_with_prefix` => `SigningKey::new`
- `VerifyingKey::new` => `VerifyingKey::new_unprefixed`
- `VerifyingKey::new_with_prefix` => `VerifyingKey::new`

The `*_with_prefix` methods are preserved with a deprecation warning,
which should help people migrate to the new versions.

Closes #238
@ixolius ixolius mentioned this issue Jun 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api discussion enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

pkcs1v15: make *_with_prefix methods the default RustCrypto/RSA
2 participants
@tarcieri @dignifiedquire