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

Fix verify test with ecdsa-sha2-nistp384 key #794

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Apr 24, 2024

Fix test, which successfully verifies a signature from an ecdsa nistp256 key, using a "ecdsa-sha2-nistp384" key.

This only worked because keytype/scheme/keyval combos in an SSlibKey are not validated sufficiently (see #766).

The test data was created using a snippet attached to the commit message. Note how the signature is not created with CryptoSigner, which does not support nistp384.

lukpueh added a commit to lukpueh/securesystemslib that referenced this pull request Apr 24, 2024
Added checks as requrested in secure-systems-lab#766:

- keytype matches scheme
- scheme matches deserialized key type (only for pem formatted keyvals;
  for ed25519 the check happens implicitly on deserialization)
- scheme matches deserialized key curve (only for ecdsa)

Note that this PR does not move the checks to the constructor as
suggested in secure-systems-lab#766. This may or may not be addressed in a follow-up PR.

failing tests fixed in secure-systems-lab#794

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to lukpueh/securesystemslib that referenced this pull request Apr 24, 2024
Added checks as requrested in secure-systems-lab#766:

- keytype matches scheme
- scheme matches deserialized key type (only for pem formatted keyvals;
  for ed25519 the check happens implicitly on deserialization)
- scheme matches deserialized key curve (only for ecdsa)

Note that this PR does not move the checks to the constructor as
suggested in secure-systems-lab#766. This may or may not be addressed in a follow-up PR.

failing tests fixed in secure-systems-lab#794

Signed-off-by: Lukas Puehringer <[email protected]>
Fix test, which successfully verifies a signature from an ecdsa
nistp256 key, using a "ecdsa-sha2-nistp384" key.

This only worked because keytype/scheme/keyval combos in an SSlibKey
are not validated sufficiently (see secure-systems-lab#766).

The test data was created using below snippet. Note how the signature is
not created with CryptoSigner, which does not support nistp384.

```
from cryptography.hazmat.primitives.asymmetric.ec import (
    ECDSA,
    SECP384R1,
    EllipticCurvePrivateKey,
)
from cryptography.hazmat.primitives.hashes import SHA384
from cryptography.hazmat.primitives.serialization import load_pem_private_key

from securesystemslib.signer import SSlibKey

with open("tests/data/pems/ecdsa_secp384r1_private.pem", "rb") as f:
    priv: EllipticCurvePrivateKey = load_pem_private_key(f.read(), None)

assert isinstance(priv.curve, SECP384R1)

pub = SSlibKey.from_crypto(priv.public_key())
sig = priv.sign(b"DATA", ECDSA(SHA384()))

print(pub.keyid)
print(sig.hex())
print(repr(pub.keyval["public"]).strip("'"))
```

Signed-off-by: Lukas Puehringer <[email protected]>
@lukpueh lukpueh requested a review from jku April 25, 2024 07:20
@jku jku merged commit f8aee26 into secure-systems-lab:main Apr 25, 2024
12 checks passed
lukpueh added a commit to lukpueh/securesystemslib that referenced this pull request Apr 25, 2024
Added checks as requrested in secure-systems-lab#766:

- keytype matches scheme
- scheme matches deserialized key type (only for pem formatted keyvals;
  for ed25519 the check happens implicitly on deserialization)
- scheme matches deserialized key curve (only for ecdsa)

Note that this PR does not move the checks to the constructor as
suggested in secure-systems-lab#766. This may or may not be addressed in a follow-up PR.

failing tests fixed in secure-systems-lab#794

Signed-off-by: Lukas Puehringer <[email protected]>
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.

2 participants