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

feature: more attribute support #22

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

secDre4mer
Copy link

Name of feature:

Support for reading / changing unsigned attributes

Pain or issue this feature alleviates:

Unsigned attributes can contain mutable information. E.g. adding a 2nd signature to a PKCS7 struct is done by adding an additional unsigned attribute that contains the PKCS7 for the secondary signature.

Why is this important to the project (if not answered above):

Reading and changing unsigned signatures is a valuable extension, especially since support for adding unsigned signatures at signing time already exists.

Is there documentation on how to use this feature? If so, where?

In what environments or workflows is this feature supported?

In what environments or workflows is this feature explicitly NOT supported (if any)?

Supporting links/other PRs/issues:

saferwall/pe#95

💔Thank you!

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2024

CLA assistant check
All committers have signed the CLA.

@secDre4mer secDre4mer changed the title Feat/more attribute support feature: more attribute support Apr 8, 2024
@hslatman
Copy link
Member

Hey @secDre4mer, thank you for opening the PR. I'll have a look at this soon.

I'm curious about the Copy function name / signature. It seems like the goal is to get an exact copy of an existing p7 struct, but then with specific unsigned attributes set? Maybe it would be more logical to define it as a method on the p7 itself, and return a new instance? Or with a more explicit name, like CopyWithUnsignedAttributes?

@secDre4mer
Copy link
Author

secDre4mer commented Apr 12, 2024

Hey @hslatman , yes, you've described the use case well.

Maybe it would be more logical to define it as a method on the p7 itself, and return a new instance?

Defining this on the struct itself would make sense IMO, and returning a full PKCS7 struct makes sense. The raw content can then be queried anyway with PKCS7.Content.

Or with a more explicit name, like CopyWithUnsignedAttributes?

Maybe we can combine this? Like: Make Copy into (PKCS7).ModifyUnsignedAttributes(newAttributes ...Attribute) *PKCS7?

One other issue with the current API, now that I'm looking at it: There's no way to iterate over all attributes (signed or unsigned) yet. Should I add some functions for this?

@secDre4mer secDre4mer force-pushed the feat/more-attribute-support branch from b018e1a to c3bf0ff Compare May 3, 2024 12:51
@secDre4mer
Copy link
Author

Added CopyWithUnsignedAttributes as suggested by you. Also added a Marshal function that can be used to turn a PKCS7 struct back into an ASN1 byte struct.

@LordNoteworthy
Copy link

Thank @secDre4mer for the update !

What do you think @hslatman ?

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.

4 participants