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 create_ak for ECC keys #464

Merged
merged 2 commits into from
Jan 25, 2024
Merged

Fix create_ak for ECC keys #464

merged 2 commits into from
Jan 25, 2024

Conversation

ionut-arm
Copy link
Member

Fixing the parameters for creating AKs in the Endorsement Hierarchy. The count value part of the EccScheme has been adjusted, and an empty EccPoint was added as unique identifier for the key.

@ionut-arm ionut-arm added the bug Something isn't working label Oct 26, 2023
@ionut-arm ionut-arm mentioned this pull request Oct 26, 2023
wiktor-k
wiktor-k previously approved these changes Oct 26, 2023
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Is there a test case for EcDaa already? (so that both codepaths are tested).

@ionut-arm
Copy link
Member Author

ionut-arm commented Oct 26, 2023

I'm curious now why that ecc_rsa test was supposed to fail 🤔

EDIT: nevermind, noticed the scheme... why does it work with swtpm, though....

@wiktor-k
Copy link
Collaborator

EDIT: nevermind, noticed the scheme... why does it work with swtpm, though....

I think a better explanation in the test would be a good idea. Also, instead of manual panic: #[should_panic] maybe even with expected...

Just throwing some random work ideas your way 😅

Superhepper
Superhepper previously approved these changes Oct 26, 2023
@ionut-arm ionut-arm dismissed stale reviews from Superhepper and wiktor-k via 735eb02 October 26, 2023 13:44
@@ -42,6 +42,7 @@ fn test_create_ak_rsa_rsa() {
}

#[test]
#[should_panic]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand this panic. Should you not test that the correct error is generated instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess Ionut did so because I suggested it in a previous comment. Sorry 🙇‍♂️! Maybe indeed assert!(matches!(... would be a better option 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean for me #[should_panic] is used to test that the library panics under certain conditions. And we try very hard to not panic any where in the library so that should hopefully not happen.

Things that one can do to test is:

  1. If the error in it self is not that import but rather that an error is returned at all then you can:
let result = ak::foo();
assert!(result.is_err(), "Function fo should have returned an error.");

This is not done any where in our code currently because the returned errors are a part of the API.

  1. The specific error is important:
let result = ak::foo();
if let Err(actual_error) = result {
    assert_eq!(
        Error::WrapperError(WrapperErrorKind::InvalidParam),
        actual_error,
        "Foo did not produce the expected error."
    );
} else {
    panic!("Foo is expected to return an error");
}

This is done extensively when testing the TSS return code errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the pushback, should've thought this through 😅

I'll tweak it and push back up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, only took about 3 months, hope it works!

@Firstyear
Copy link
Contributor

What should we do to unblock this pr?

@Superhepper
Copy link
Collaborator

Superhepper commented Dec 14, 2023

I do not think it is blocked I just think @ionut-arm has been busy and not been able to work on this. I think we decided to not use the #[should_panic]. So we are just waiting for him to re write the test to check that the correct error is being generated and this should be good to be merged.

@Superhepper
Copy link
Collaborator

@ionut-arm Tell me if you need any help with this. I can fix the tests for you if you like.

Fixing the parameters for creating AKs in the Endorsement Hierarchy. The
`count` value part of the `EccScheme` has been adjusted, and an empty
`EccPoint` was added as unique identifier for the key.

Signed-off-by: Ionut Mihalcea <[email protected]>
A proper match of elliptic curve and asymmetric scheme is more
thoroughly checked to avoid cases where the user can generate
PublicEccParameters that are invalid.

Signed-off-by: Ionut Mihalcea <[email protected]>
@ionut-arm
Copy link
Member Author

@ionut-arm Tell me if you need any help with this. I can fix the tests for you if you like.

ok, finally got to this... @Firstyear - apologies for the long wait, I reckon this is ready to go (pending approval, of course)

@Superhepper Superhepper requested a review from wiktor-k January 24, 2024 22:42
@hug-dev hug-dev merged commit db16a31 into parallaxsecond:main Jan 25, 2024
11 checks passed
Superhepper added a commit to Superhepper/rust-tss-esapi that referenced this pull request Oct 4, 2024
This takes the following PRs and from the main
branch and adapts them so that they can be merged
into the 7.x.y branch:

\parallaxsecond#464 (By Ionut Mihalcea <[email protected]>)
\parallaxsecond#414 (By Thore Sommer <[email protected]>)

Co-authored-by: Jesper Brynolf <[email protected]>
Co-authored-by: Thore Sommer <[email protected]>
Co-authored-by: Ionut Mihalcea <[email protected]>
Signed-off-by: Jesper Brynolf <[email protected]>
Superhepper added a commit to Superhepper/rust-tss-esapi that referenced this pull request Oct 4, 2024
This takes the following PRs and from the main
branch and adapts them so that they can be merged
into the 7.x.y branch:

\parallaxsecond#464 (By Ionut Mihalcea <[email protected]>)
\parallaxsecond#414 (By Thore Sommer <[email protected]>)

Co-authored-by: Jesper Brynolf <[email protected]>
Co-authored-by: Thore Sommer <[email protected]>
Co-authored-by: Ionut Mihalcea <[email protected]>
Signed-off-by: Jesper Brynolf <[email protected]>
Superhepper added a commit to Superhepper/rust-tss-esapi that referenced this pull request Nov 7, 2024
This takes the following PRs and from the main
branch and adapts them so that they can be merged
into the 7.x.y branch:

\parallaxsecond#464 (By Ionut Mihalcea <[email protected]>)
\parallaxsecond#414 (By Thore Sommer <[email protected]>)
\parallaxsecond#552 (By Thore Sommer <[email protected]>)

Co-authored-by: Jesper Brynolf <[email protected]>
Co-authored-by: Thore Sommer <[email protected]>
Co-authored-by: Ionut Mihalcea <[email protected]>
Signed-off-by: Jesper Brynolf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants