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: parse all certificates from multi-signed files #95

Merged
merged 2 commits into from
May 16, 2024

Conversation

secDre4mer
Copy link
Contributor

@secDre4mer secDre4mer commented Mar 22, 2024

Add support for parsing (and returning) additional nested signatures.

Also fixes an issue where the signature algorithm of the file was detected incorrectly.

Multiple certificates are not, like the current code assumes,
appended; instead, a specific (unsigned) attribute is nested
inside the PKCS7 struct that contains the next PKCS7 struct.

Also improve the returned certificates to include structs for all
parsed, PKCS7 structs.
The signature algorithm listed in the signing certificate is not
necessarily the signature algorithm used for the file signature.
Extract the algorithm for the file signature from the authemticode
instead.
@LordNoteworthy
Copy link
Member

That's great work @secDre4mer .

I need to review this carefully, it's also a breaking change.

I will have a look later today !

@secDre4mer
Copy link
Contributor Author

I need to review this carefully, it's also a breaking change.

Yeah, I know. I didn't really see a way to add multiple certificates to the output without any breaking changes, unfortunately.

@@ -4,6 +4,6 @@ go 1.15

require (
github.com/edsrzf/mmap-go v1.1.0
go.mozilla.org/pkcs7 v0.0.0-20210826202110-33d05740a352
github.com/secDre4mer/pkcs7 v0.0.0-20240322103146-665324a4461d
Copy link
Member

Choose a reason for hiding this comment

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

I was hoping we apply the patches you made over: https://github.com/smallstep/pkcs7

Because @smallstep took a couple of PRs from the mozilla fork and applied them to his fork. If it makes sense, you can submit a PR on his repo.

If you think it is not relevant for people to benefit from the commits you did, then we can keep it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds good. I noticed that the mozilla repo was archived and no longer maintained and therefore created my own branch, but if there's a supported successor, I'll gladly create a PR there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The smallstep/pkcs7 maintainer hasn't responded to the PR for some weeks now... How should we proceed here?

Copy link
Member

Choose a reason for hiding this comment

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

Thank for your efforts @secDre4mer, let's merge this one then.

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.

2 participants