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

Sha256 support #19

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Sha256 support #19

wants to merge 13 commits into from

Conversation

groob
Copy link
Contributor

@groob groob commented Jul 10, 2017

Hi @fullsailor,

I use the pkcs7 package in my SCEP implementation. Although SCEP supports SHA1 and 3DES the recommended defaults are AES with SHA256.

I began working on implementing support for SHA256 and possibly SHA512 as well in this branch.
Most of the code in this pull request is actually coming from a new package, github.com/fullsailor/pkcs7/internal/x509util which is just some exported helpers from crypto/x509. I use the helpers to determine the hash function/signature algorithm when needed.

I would love your input on the direction on this pull request, and what requirements you would have for getting a change like this into the pkcs7 package.

Thanks!

groob added 2 commits July 10, 2017 13:35
copies a few private methods from the stdlib x509 package.
Uses the x509util package to get a wider range of supported hash functions.
@groob
Copy link
Contributor Author

groob commented Jul 10, 2017

Travis fails with 1.6/1.7 but passes with 1.8+

internal/x509util/x509util.go:115: undefined: x509.SHA256WithRSAPSS
internal/x509util/x509util.go:115: undefined: x509.SHA384WithRSAPSS
internal/x509util/x509util.go:115: undefined: x509.SHA512WithRSAPSS
internal/x509util/x509util.go:183: undefined: x509.SHA256WithRSAPSS
internal/x509util/x509util.go:184: undefined: x509.SHA384WithRSAPSS
internal/x509util/x509util.go:185: undefined: x509.SHA512WithRSAPSS

I can put those behind a build flag

pkcs7.go Outdated
@@ -864,13 +934,19 @@ func encryptDESCBC(content []byte) ([]byte, *encryptedContentInfo, error) {
// ContentEncryptionAlgorithm = EncryptionAlgorithmAES128GCM
//
// TODO(fullsailor): Add support for encrypting content with other algorithms
func Encrypt(content []byte, recipients []*x509.Certificate) ([]byte, error) {
func Encrypt(content []byte, recipients []*x509.Certificate, opts ...Option) ([]byte, error) {
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 changes the function signature, but is still backwards compatible depending on how Encrypt is called.
pkcs7.Encrypt(content, recipients) would still compile, but if someone created a callback, or interface their code would break.

Possible solutions:

  1. break compatibility and create a release with a new git tag
  2. create a new EncryptWithOptions function
  3. use global variables (personally I would avoid but I can work with that option if required)

@Beanow
Copy link

Beanow commented Nov 21, 2017

Any hopes of this seeing a merge?

@Beanow
Copy link

Beanow commented Nov 21, 2017

@groob signer.DigestAlgorithm.Algorithm might also be a hash only, not including encryption.
For example, my test matched oidSHA256.

This breaks the func verifySignature(p7 *PKCS7, signer signerInfo) error function with:
could not find SignatureAlgorithm details for oid: 2.16.840.1.101.3.4.2.1

@Beanow
Copy link

Beanow commented Nov 21, 2017

The algorithm could be composed, because:

signer.DigestAlgorithm.Algorithm == 2.16.840.1.101.3.4.2.1 (SHA256)
signer.DigestEncryptionAlgorithm.Algorithm == 1.2.840.113549.1.1.1 (RSA)

fullsailor and others added 9 commits June 13, 2018 10:04
Go 1.10 is more strict about Asn.1 annotations. This removes the incorrect “explicit” annotation from encryptedContentInfo.EncryptedContent.

I’m also using openssl to generate the fixture now so that we aren’t testing with our own output for `Decrypt()`

Fixes fullsailor#31
Fix failure to parse enveloped data in Go 1.10
Previously the encrypt method was grouping together multiple algorithms
and handling them the same. In the decryption code this somewhat
makes sense because the crypto.Cipher will take care of figuring out
which algorithm it needs to use. When encrypting though if AES-128-CBC
is requested we were encrypting as AES-128-GCM which didn't make sense.
chrisccoulson pushed a commit to chrisccoulson/pkcs7 that referenced this pull request Apr 25, 2020
Support setting the encalg when signing without attr
jessepeterson pushed a commit to jessepeterson/pkcs7 that referenced this pull request Apr 30, 2024
…content

EncryptedContent needs to be implicitly tagged and not explicit (updated)
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