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

Add various new OIDs, types and methods #14

Merged
merged 21 commits into from
Dec 2, 2022

Conversation

Pagten
Copy link
Contributor

@Pagten Pagten commented Dec 1, 2022

There is a separate commit for each atomic change made. The intention of this PR is not to change anything in a non-backwards compatible way. Therefore this PR only bumps the patch version of this crate.

A follow-up PR will be made for non-backwards compatible changes. That PR will bump the version to 0.2.0.

@arai-fortanix
Copy link

This looks good to me. I just had one question about whether a lifetime change was compatible or not.

@@ -113,8 +113,8 @@ impl<'e, K: DerWrite> DerWrite for CertificationRequestInfo<'e, K> {
}
}

impl<K: BERDecodable> BERDecodable for CertificationRequestInfo<'static, K> {
fn decode_ber<'a, 'b>(reader: BERReader<'a, 'b>) -> ASN1Result<Self> {
impl<'a, K: BERDecodable> BERDecodable for CertificationRequestInfo<'a, K> {

Choose a reason for hiding this comment

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

Is this change compatible? There's a lifetime change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be compatible. The original version implements BERDecodable only for CertificationRequestInfo<'static, K>. So any time you deserialize one of these structs, you would get one that has the static lifetime. The new version implements BERDecodable for any lifetime, which includes the static lifetime.

You would think that implementing BERDecodable for the static lifetime should be sufficient to cover all cases, since it can always still be coerced to a shorter lifetime. I recently encountered a situation in which that wasn't the case, though. I don't completely understand why, but the compiler was giving me this error in a piece of async code:

error: implementation of `SomeTrait` is not general enough
   --> [...]
    | [code omitted]
    = note: `SomeStruct` must implement `SomeTrait<'0>`, for any lifetime `'0`...
    = note: ...but it actually implements `SomeTrait<'static>`

The SomeTrait was indeed only implemented for the static lifetime on SomeStruct, just like we originally had here for CertificationRequestInfo. Changing the trait implementation to cover all lifetimes solved that issue.

src/types.rs Outdated
} else {
reader.read_tagged_implicit(Tag::context(tag_number), |r| {
match tag_number {
0 => {

Choose a reason for hiding this comment

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

Can we have some constants for this numbers and a comment of where they come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Pagten Pagten force-pushed the pieagt/various-additions-pkix-0.1.3 branch from 1e322b2 to 5f32352 Compare December 2, 2022 08:37
@Pagten
Copy link
Contributor Author

Pagten commented Dec 2, 2022

bors r=[arai-fortanix, janosimas]

@bors
Copy link
Contributor

bors bot commented Dec 2, 2022

Already running a review

@bors
Copy link
Contributor

bors bot commented Dec 2, 2022

Build succeeded:

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 8e9acd0 into master Dec 2, 2022
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.

3 participants