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

ASN.1 Integer minimal encoding is required per X.690-0207 Section 8.3.2 #209

Merged
merged 3 commits into from
Mar 24, 2023

Conversation

brimworks
Copy link
Contributor

Validation of ASN1.Integer in Go code was failing due to the encoding not being minimal, and the spec indicates that it is required to be minimal.

@CBenoit
Copy link
Member

CBenoit commented Mar 4, 2023

Thank you! I approved CI run. I’ll take more time for reviewing next Monday 😃

@@ -379,8 +394,14 @@ impl IntegerAsn1 {
}
}

// TODO: This method should probably take a `&[u8]`:
Copy link
Member

Choose a reason for hiding this comment

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

We can’t take owership of bytes anymore in this case though. Is start == 0 not common enough? Also, do you have code in mind requiring keeping the bytes at call site? In my experience the input object is never used again once the IntegerAsn1 struct is built.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove this comment if you would like... but in our usage, we are trying to translate an EccParameter into an IntegerAsn1. The EccParameter only has a value() method that allows one to obtain a &[u8] which represents the parameter:

https://docs.rs/tss-esapi/latest/tss_esapi/structures/struct.EccParameter.html

As it stands, I believe our only option is to convert .to_vec() the &[u8] in order to construct the IntegerAsn1... and in the scenario where a resize needs to take place, we slice the Vec and create yet another Vec, but if the parameter was a &[u8] we could avoid one of the Vec conversions.

Alternatively, if the API took a mut Vec then we could drain(0..start) and avoid the need for the extra Vec allocation.

...although, perhaps all of this is premature optimization.

@brimworks
Copy link
Contributor Author

@CBenoit how would you like me to proceed? Would you like me to remove the comment in regard to taking a &[u8]?

@CBenoit
Copy link
Member

CBenoit commented Mar 21, 2023

@CBenoit how would you like me to proceed? Would you like me to remove the comment in regard to taking a &[u8]?

I’m open to changing the API to a &[u8] if you want to.

@brimworks
Copy link
Contributor Author

@CBenoit how would you like me to proceed? Would you like me to remove the comment in regard to taking a &[u8]?

I’m open to changing the API to a &[u8] if you want to.

How about if I just remove this comment for now, that idea can always be addressed in a separate PR.

@CBenoit
Copy link
Member

CBenoit commented Mar 24, 2023

@CBenoit how would you like me to proceed? Would you like me to remove the comment in regard to taking a &[u8]?

I’m open to changing the API to a &[u8] if you want to.

How about if I just remove this comment for now, that idea can always be addressed in a separate PR.

Sounds good!

Copy link
Member

@CBenoit CBenoit 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 to me! Thank you 😄
I’ll publish next candidate version next Monday!

@CBenoit CBenoit enabled auto-merge (squash) March 24, 2023 19:47
@CBenoit CBenoit disabled auto-merge March 24, 2023 19:47
@CBenoit CBenoit enabled auto-merge (squash) March 24, 2023 19:48
@CBenoit CBenoit merged commit 2bab4b4 into Devolutions:master Mar 24, 2023
@brimworks brimworks deleted the asn1-minimal-encoding branch March 25, 2023 03:53
@brimworks
Copy link
Contributor Author

Thanks!

anta5010 added a commit to anta5010/parsec-tool that referenced this pull request Aug 21, 2023
picky-asn1 0.7.2 contains a fix for occasional incorrect ASN1 Integer builds:
Devolutions/picky-rs#209

Fixes: parallaxsecond#101

Signed-off-by: Anton Antonov <[email protected]>
anta5010 added a commit to anta5010/parsec that referenced this pull request Aug 21, 2023
picky-asn1 0.7.2 contains a fix for occasional incorrect ASN1 Integer builds:
Devolutions/picky-rs#209

Signed-off-by: Anton Antonov <[email protected]>
anta5010 added a commit to anta5010/parsec that referenced this pull request Aug 22, 2023
picky-asn1 0.7.2 contains a fix for occasional incorrect ASN1 Integer builds:
Devolutions/picky-rs#209

Signed-off-by: Anton Antonov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants