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

Tests for 4.2 of PKITS #76

Closed
wants to merge 3 commits into from

Conversation

nick-mobilecoin
Copy link
Collaborator

@nick-mobilecoin nick-mobilecoin commented May 2, 2023

Fill in the tests for section 4.2 of PKITS. The test for 4.2.3 was
omitted as it uses a time of 1950 but the DER decoder only supports
times back to 1970 to work with UNIX_EPOCH

@github-actions
Copy link

github-actions bot commented May 2, 2023

❌ Unreviewed dependencies found

Crate Version Reviews (N/2) LoC Left-Pad Index Geiger Flags

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

❗ No coverage uploaded for pull request base (nick/pkits-integration-folder@ce93532). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head c5a46ab differs from pull request most recent head 9751c54. Consider uploading reports for the commit 9751c54 to get more accurate results

@@                       Coverage Diff                        @@
##             nick/pkits-integration-folder      #76   +/-   ##
================================================================
  Coverage                                 ?   97.85%           
================================================================
  Files                                    ?        8           
  Lines                                    ?     2008           
  Branches                                 ?        0           
================================================================
  Hits                                     ?     1965           
  Misses                                   ?       43           
  Partials                                 ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@awygle awygle left a comment

Choose a reason for hiding this comment

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

Please fix the incorrect test names, then this should be good.

verifier/tests/pkits-4-2.rs Outdated Show resolved Hide resolved
Err(Error::CertificateNotYetValid)
);
}

Copy link

Choose a reason for hiding this comment

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

Suggest (an issue for) a version of the 4.2.3 test that confirms some behavior on pre-Unix certs (presumably an error from the DER encoder).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a test case for 4.2.3.

I don't know if I'm scarred from dependabot, but if one has a better way to handle the date verification without pulling in another crate (chrono) I'm open to suggestions there.

verifier/tests/pkits-4-2.rs Outdated Show resolved Hide resolved

assert_eq!(
chain.signing_key(&root, unix_time, crls.as_slice()),
Err(Error::CertificateExpired)
Copy link

Choose a reason for hiding this comment

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

This is more of a comment on the general API design but, shouldn't there be a way to differentiate between the error codes produced by 4.2.5 and 4.2.6? The corrective actions taken for "ICA expired" and "EE expired" are different, so it seems like Error::CertificateExpired might want to include a data member.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm wonder about the same thing for signature failures as well, though less likely to happen.

It does feel like it would be nice to know which certificate was the issue. Since alloc is available here we could output the subject name as a String in the expired error, same for not before, and possibly others.

@github-actions github-actions bot added the rust Pull requests that update rust code label May 5, 2023
Fill in the tests for section 4.2 of PKITS.  The test for 4.2.3 was
omitted as it uses a time of 1950 but the DER decoder only supports
times back to 1970 to work with `UNIX_EPOCH`
nick-mobilecoin and others added 2 commits May 5, 2023 07:33
Fix copy pasta failure in test names

Co-authored-by: awygle <awygle@gmail.com>
The test case for PKITS 4.2.3 uses a `UTCTime` of "1950-01-01T00:00:00",
however the DER parsing logic is limited to the UNIX_EPOCH "1970-01-01".
In order to ensure the proper date handling of 1970-2000 a lower level
testing approach is used.
@nick-mobilecoin
Copy link
Collaborator Author

punting on implementing x509 chain parsing logic.

@nick-mobilecoin nick-mobilecoin deleted the nick/pkits-4.2 branch October 3, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Pull requests that update rust code size/L Large PRs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants